Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coop coep reporting testing plan #27659
base: master
Are you sure you want to change the base?
Coop coep reporting testing plan #27659
Changes from 4 commits
584415c
07e071c
509aae7
e8a89e4
10585d0
39c5542
d719338
9e746f1
01b3472
490e052
db8fd83
cf5c0bb
172d653
66e0fa8
6e098ab
dc6cf39
bba78be
d610e5f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a step between these two which invokes "check a navigation response's adherence to its embedder policy". Why is that omitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to test the ordering of these 3 checks, which should be possible with 2 tests: for steps A, B, C, test these combinations: (A, B) and (B, C)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment settings object may influence two observable steps in this algorithm: the derivation of the report's URL, and the decision to notify reporting observer. Does this note describe tests for both of those things? Or something else?
Also, there might be another opportunity to improve the spec here. In the algorithm, both "url" and "settings" are optional. Step two assumes that if "url" is not provided, then "settings" will be. Should there be an assertion stating that at least one of those two parameters will always be specified? Or is the algorithm missing a possible condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if this test plan enumerated the report types that you want to cover because they aren't listed in the specification itself and because that information will probably inform effort estimates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm under the impression that the
options
object will always generated from specification language, which would make this condition impossible. Take that with a grain of salt, though; I haven't reviewed the relevant algorithms in quite some time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
is passed to theReportingObserver()
constructor by author JS, so it can be any value. This would be more of a WebIDL test, though. Also,options
is a dictionary andtypes
is asequence<DOMString>
, so atypes
getter that is a function doesn't get to throwing the exception because a function isn't iterable... But these two cases should throw (when the constructor is called, because webidl conversion of the arguments):https://heycam.github.io/webidl/#create-sequence-from-iterable
Demo: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/8942
Only the first one throws in chromium and gecko, though. Are they waiting with actually iterating the sequence? Or am I misunderstanding the WebIDL spec?