Upstream service worker `fetch` test to WPT

Upstream service worker `fetch` test to WPT

- Re-write expected "credentials" values according to WPT's definition.
- Create a dedicated sub-test for each request. This ensures deterministic
  failure and allows much finer-grained error reporting.

BUG=688116
R=mek@chromium.org

Review-Url: https://codereview.chromium.org/2856863003
Cr-Commit-Position: refs/heads/master@{#469767}
Committed: https://chromium.googlesource.com/chromium/src/+/9b0ef8da376582de0ba4d3aaa223bd78c3ab2f19

[mike3, 2017-05-02 18:45:07]

Hi Mek,

This is the test we've been discussing on IRC. The migration was a little more
straightforward than we expected because it turns out that Chromium's behavior
is actually correct. Still, the re-structuring we agreed on made for a slightly
more complex diff, so I'm submitting this test on its own.

I tried to coax codereview into rendering the patch as a "rename" operation by
using `git cl upload --similarity 10`, but that doesn't seem to have worked.
Instead, I've generated a separate patch via `git diff -M10` and uploaded to
GitHub in case that helps you review:

https://gist.github.com/jugglinmike/5f188a27b8c8c6179d74af0c53727d4c

[mike3, 2017-05-02 22:02:17]

It looks like one of the project's test expectations files was recently updated
to reference this test:

61b8dd80224f465e251452cf9ca08bea8cf9d28e

I've rebased and updated the reference accordingly.

[Marijn Kruisselbrink, 2017-05-03 16:18:47]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 16:19:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-05-03 17:23:58]

Currently what you did is something like:

var p1 = promise_for_a_test(...);
promise_test(() => p1);
var p2 = promise_for_a_test(...);
promise_test(() => p2);
promise_test(() => shared_initialization().then(wait_for_all(p1, p2, ...)));

Which looks kind of clunky/confusing to me. Would it make sense to rewrite this
to something like:

var p = shared_initialization();
promise_test(() => p.then((frame) => promise_for_a_test(...)));
promise_test(() => p.then((frame) => promise_for_a_test(...)));

etc?

shared_initialization would do the login, registering service worker, and
creating the iframe, resolving with the iframe; and rather than t.add_cleanup()
to remove the iframe/unregister the service worker you'd have to use
add_completion_callback to remove the iframe and unregister the service worker
instead. But that still seems like it would potentially be cleaner than the
current code.

https://codereview.chromium.org/2856863003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:5:
<script src="/common/get-host-info.sub.js?pipe=sub"></script>
nit: 
I believe you don't need the "?pipe=sub" when the file already has .sub.js as
extension

[mike3, 2017-05-03 18:44:03]

> shared_initialization would do the login, registering service worker, and
> creating the iframe, resolving with the iframe; and rather than
> t.add_cleanup() to remove the iframe/unregister the service worker you'd have
> to use add_completion_callback to remove the iframe and unregister the
> service worker instead. But that still seems like it would potentially be
> cleaner than the current code.

Agreed, that is a much better approach. I had to make a few modifications to
your suggestion before I could implement it, though:

- The "helpers" code (beginning with `login_https`) is authored to rely on a
  valid `Test` instance. In order to use it, I've had to define a
  `promise_test` for the purposes of set up. I've named it "initialize global
  state"; hopefully that is enough to communicate its purpose as "setup" logic.
- The `service_worker_unregister_and_done` function also required a `Test`
  instance. That function would be much easier to re-implement here, but the
  trouble is that `add_completion_callback` is a reporting mechanism that does
  not consider the result of the provided function. This means that if we
  triggered un-registration from here (regardless of the function we use),
  there would be no guarantee that the operation was completed before the test
  runner navigated to the next test. So I've introduced another
  state-management test for this purpose and named it, "restore global state."
- This factoring results in longer test runs because each request is now made
  in series. Firefox does not complete all tests before the default timeout
  value, so I've added the metadata necessary to lengthen that value.
- I am not invoking `promise_test` directly for each test definition. Although
  I would also prefer to see this done explicitly, the test body *and* test
  title are generated dynamically, making it awkward to define in the terms
  you've suggested. We still could do this by making the `ok_test` and
  `ng_test` functions return an array containing the test body and test title,
  but I think this may be a little too noisy:

      var pair;
      pair = ok_test(BASE_URL, 'same-origin', 'omit', 'basic', 'undefined');
      promise_test(pair[0], pair[1]);
      pair = ok_test(BASE_URL, 'no-cors', 'omit', 'basic', 'undefined');
      promise_test(pair[0], pair[1]);
      // etc.

I should also point out that due to the first point, the harness does not
execute any of the tests until after the iframe has been created and service
worker registered. In other words, it's not technically necessary for the tests
to begin with `login_and_register.then(...)`. We could define a global binding
like `frame`, set it when the iframe is available, and know that it will be
defined before the subsequent tests are executed. I'm not sure the distinction
is very important given that this detail is abstracted away into `ok_test` and
`ng_test`, but I thought I should call that out here anyway.

https://codereview.chromium.org/2856863003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:5:
<script src="/common/get-host-info.sub.js?pipe=sub"></script>
On 2017/05/03 17:23:58, Marijn Kruisselbrink wrote:
> nit: 
> I believe you don't need the "?pipe=sub" when the file already has .sub.js as
> extension

You are correct:

> In order for the latter to work, a file must either have a name of the form
> `{name}.sub.{ext}` e.g. `example-test.sub.html` or be referenced through a
> URL containing `pipe=sub` in the query string e.g.
> `example-test.html?pipe=sub`.

http://web-platform-tests.org/writing-tests/server-features.html

[commit-bot: I haz the power, 2017-05-03 18:55:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 18:55:06]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-05-04 00:08:05]

On 2017/05/03 at 18:44:03, mike wrote:
> > shared_initialization would do the login, registering service worker, and
> > creating the iframe, resolving with the iframe; and rather than
> > t.add_cleanup() to remove the iframe/unregister the service worker you'd
have
> > to use add_completion_callback to remove the iframe and unregister the
> > service worker instead. But that still seems like it would potentially be
> > cleaner than the current code.
> 
> Agreed, that is a much better approach. I had to make a few modifications to
> your suggestion before I could implement it, though:
> 
> - The "helpers" code (beginning with `login_https`) is authored to rely on a
>   valid `Test` instance. In order to use it, I've had to define a
>   `promise_test` for the purposes of set up. I've named it "initialize global
>   state"; hopefully that is enough to communicate its purpose as "setup"
logic.
Ah yes, good point. It's rather unfortunate that the various test-helpers.js
methods require passing in Test instances, purely because they predate
promise_test (and are still used with regular async_tests)... But good enough I
suppose.

> - This factoring results in longer test runs because each request is now made
>   in series. Firefox does not complete all tests before the default timeout
>   value, so I've added the metadata necessary to lengthen that value.
Ah, that's unfortunate, but makes sense.

> - I am not invoking `promise_test` directly for each test definition. Although
>   I would also prefer to see this done explicitly, the test body *and* test
>   title are generated dynamically, making it awkward to define in the terms
>   you've suggested. We still could do this by making the `ok_test` and
>   `ng_test` functions return an array containing the test body and test title,
>   but I think this may be a little too noisy:
What you did now looks good to me (and was more or less what I meant).

> I should also point out that due to the first point, the harness does not
> execute any of the tests until after the iframe has been created and service
> worker registered. In other words, it's not technically necessary for the
tests
> to begin with `login_and_register.then(...)`. We could define a global binding
> like `frame`, set it when the iframe is available, and know that it will be
> defined before the subsequent tests are executed. I'm not sure the distinction
> is very important given that this detail is abstracted away into `ok_test` and
> `ng_test`, but I thought I should call that out here anyway.
Good point. Either way works for me, since as you say it's mostly abstracted
away anyway.

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:46:
promise_test(function(t) {
Could you add a comment explaining:
1) why this is a separate promise test (i.e. because otherwise there is no way
to do asynchronous cleanup jobs)
2) why this nested promise_test ends up being called at the right time (i.e.
something about how all the other promise_test calls in this file are
synchronous, so this will always be queued up after all other tests have already
been queued)

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:48:
return service_worker_unregister_and_done(t, SCOPE);
calling unregister_and_done in a promise test always seems like a bit of an
anti-pattern to me, since the promise resolving itself will already mark the
test as done... So either just call service_worker_unregister(t, SCOPE), or even
simpler since you could just save the registration from line 41, just call
registration.unregister() directly?

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:59:
return frame_fetch(frame, url, mode, credentials);
return promise_rejects(t, null, frame_fetch(...)) is cleaner than the explicit
then and rejection handlers (probably even replace null with whatever error this
is supposed to fail with).

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:133:
ok_test(
nit: why the line-break?

[mike3, 2017-05-05 16:21:38]

Thanks, Mek! New version is up.

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:46:
promise_test(function(t) {
On 2017/05/04 00:08:05, Marijn Kruisselbrink wrote:
> Could you add a comment explaining:
> 1) why this is a separate promise test (i.e. because otherwise there is no way
> to do asynchronous cleanup jobs)
> 2) why this nested promise_test ends up being called at the right time (i.e.
> something about how all the other promise_test calls in this file are
> synchronous, so this will always be queued up after all other tests have
already
> been queued)

Acknowledged.

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:48:
return service_worker_unregister_and_done(t, SCOPE);
On 2017/05/04 00:08:05, Marijn Kruisselbrink wrote:
> calling unregister_and_done in a promise test always seems like a bit of an
> anti-pattern to me, since the promise resolving itself will already mark the
> test as done... So either just call service_worker_unregister(t, SCOPE), or
even
> simpler since you could just save the registration from line 41, just call
> registration.unregister() directly?

Acknowledged.

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:59:
return frame_fetch(frame, url, mode, credentials);
On 2017/05/04 00:08:05, Marijn Kruisselbrink wrote:
> return promise_rejects(t, null, frame_fetch(...)) is cleaner than the explicit
> then and rejection handlers (probably even replace null with whatever error
this
> is supposed to fail with).

I wasn't aware of that helper. Thanks for the tip.

These tests produce a TypeError, but `promise_rejects` has been written to
operate on DOMExceptions, so I think we'll have to settle (literally... Promise
humor, sorry) for `null`.

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:133:
ok_test(
On 2017/05/04 00:08:05, Marijn Kruisselbrink wrote:
> nit: why the line-break?

A vestige from the prior version. Fixed.

[Marijn Kruisselbrink, 2017-05-05 17:32:42]

thanks, lgtm with two small nits

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:59:
return frame_fetch(frame, url, mode, credentials);
On 2017/05/05 at 16:21:37, mike3 wrote:
> On 2017/05/04 00:08:05, Marijn Kruisselbrink wrote:
> > return promise_rejects(t, null, frame_fetch(...)) is cleaner than the
explicit
> > then and rejection handlers (probably even replace null with whatever error
this
> > is supposed to fail with).
> 
> I wasn't aware of that helper. Thanks for the tip.
> 
> These tests produce a TypeError, but `promise_rejects` has been written to
operate on DOMExceptions, so I think we'll have to settle (literally... Promise
humor, sorry) for `null`.

I think using 'null' might make annevk sad
(https://github.com/w3c/web-platform-tests/issues/5317). You can check for
TypeError by either passing {name: 'TypeError'} or new TypeError() as expected
exception (both formats seem to be used about equally often in existing WPT
assert_throws usage...)

https://codereview.chromium.org/2856863003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:163:
// The response type from the SW should be basic.
nit: in all the other similarly structured code you have the "else" comment
after the else line, but here you have it before?

[mike3, 2017-05-05 19:11:48]

> You can check for TypeError by either passing {name: 'TypeError'} or new
> TypeError() as expected exception (both formats seem to be used about equally
> often in existing WPT assert_throws usage...)

I see now. Sorry for suggesting otherwise. Just so you know where I was coming
from, that was based on my attempt to use specify the TypeError constructor
directly, which was based on my reading of the documentation:

> object - the thrown exception must have a property called “name” that matches
> code.name

Using an error constructor doesn't work because the documentation's use of
"object" is the "typeof" sense of the word. When the library finds that `typeof
TypeError` is not "object", it produces an error which suggests that *only*
DOMExceptions are supported.

Anyway, being explicit is clearly desirable, so I've fixed that to use `new
TypeError()`.

https://codereview.chromium.org/2856863003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html
(right):

https://codereview.chromium.org/2856863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-taint.https.html:163:
// The response type from the SW should be basic.
On 2017/05/05 17:32:42, Marijn Kruisselbrink wrote:
> nit: in all the other similarly structured code you have the "else" comment
> after the else line, but here you have it before?

Not sure why I did that. I'll move it for consistency.

[mike3, 2017-05-05 19:12:44]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-05-05 19:12:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2856863003/#ps80001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2017-05-05 19:13:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 20:38:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494011564301150,
"parent_rev": "3fd00a51e22a0ffa55d0ab14725f9f300b2fc56b", "commit_rev":
"9b0ef8da376582de0ba4d3aaa223bd78c3ab2f19"}

[commit-bot: I haz the power, 2017-05-05 20:38:42]

Description was changed from

==========
Upstream service worker `fetch` test to WPT

- Re-write expected "credentials" values according to WPT's definition.
- Create a dedicated sub-test for each request. This ensures deterministic
  failure and allows much finer-grained error reporting.

BUG=688116
R=mek@chromium.org
==========

to

==========
Upstream service worker `fetch` test to WPT

- Re-write expected "credentials" values according to WPT's definition.
- Create a dedicated sub-test for each request. This ensures deterministic
  failure and allows much finer-grained error reporting.

BUG=688116
R=mek@chromium.org

Review-Url: https://codereview.chromium.org/2856863003
Cr-Commit-Position: refs/heads/master@{#469767}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b0ef8da376582de0ba4d3aaa223...
==========

[commit-bot: I haz the power, 2017-05-05 20:38:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9b0ef8da376582de0ba4d3aaa223...