Skip to content
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

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Feb 17, 2021

This is a rough testing plan for Reporting for COOP and COEP, for review.

Is something missing that we should write tests for?

The diff also contains some questions that aren't yet clear to me.

I'll start writing tests based on this and iterate on the testing plan along the way.

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Simon! I only had a chance to look over the Reporting API plan tonight. Since we can only observe these behaviors by using some other web platform feature, it makes me wonder: will these tests just use a single feature to trigger reporting, or will they use many? Do the existing tests have a consistent answer?

reporting/README.md Outdated Show resolved Hide resolved
- test that report destination URL's username/password, fragment are stripped
- test a URL that doesn't parse
- test javascript: mailto: data: ws: wss: etc URLs
- test settings objects
Copy link
Contributor

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?


https://w3c.github.io/reporting/#add-report

- test that the right report types are visible or not visible to ReportingObserver
Copy link
Contributor

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.

https://w3c.github.io/reporting/#add-report

- test that the right report types are visible or not visible to ReportingObserver
- test `options` with a `types` member that throws on getting
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options is passed to the ReportingObserver() constructor by author JS, so it can be any value. This would be more of a WebIDL test, though. Also, options is a dictionary and types is a sequence<DOMString>, so a types 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):

new ReportingObserver(() => {}, { types: { *[Symbol.iterator]() { throw 1; } } });
new ReportingObserver(() => {}, { types: [ { valueOf: () => { throw 2; } } ] });

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?

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Simon! I took a look at the test plan for COEP. The links to the specification are nice, but I think the document would be even better if it modeled the structure of the spec text more closely. As it stands, it's a little difficult to understand the complete surface area that you're considering, and that makes it hard to identify any gaps in the coverage you've laid out.


-> "Otherwise, if the result of Should navigation response to navigation request of type in target be blocked by Content Security Policy? given navigationParams's request, response, navigationType, and browsingContext is "Blocked", then set failure to true. [CSP]"
- test that a CSP failure doesn't give a COEP report.

Copy link
Contributor

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?

Copy link
Member Author

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)

html/cross-origin-embedder-policy/README.md Outdated Show resolved Hide resolved
html/cross-origin-embedder-policy/README.md Outdated Show resolved Hide resolved
html/cross-origin-embedder-policy/README.md Outdated Show resolved Hide resolved
html/cross-origin-embedder-policy/README.md Outdated Show resolved Hide resolved
html/cross-origin-embedder-policy/README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants