Upstream service worker "XHR" test to WPT

Upstream service worker "XHR" test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2907443002
Cr-Commit-Position: refs/heads/master@{#476324}
Committed: https://chromium.googlesource.com/chromium/src/+/7fdac9fc2a776ad2ed4c3df752896e0f33861b69

[mike3, 2017-05-24 21:46:09]

Hello again, Matt. I believe that this test was invalid. I'm recommending a
correction here, but unlike in other CL's where this has been necessary, I'm
not persisting a Chromium-specific version. That's because if my interpretation
is correct, the original version of thet test wasn't really asserting anything
interesting at all.

This is a brand-new for WPT, so there's nothing to compare it with. But since
I'm re-naming and modifying at the same time (including whitespace changes), I
expect Rietveld will render a noisy diff. Here's an alternate version, as
created via `git show -M10 -w`:

https://gist.github.com/jugglinmike/f74f42c59bb7601018b919e0f227c98a

[mike3, 2017-05-25 13:58:09]

Description was changed from

==========
Upstream service worker 'XHR' test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

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

to

==========
Upstream service worker 'XHR' test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 726327
R=falken@chromium.org
==========

[mike3, 2017-05-25 16:16:42]

Description was changed from

==========
Upstream service worker 'XHR' test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 726327
R=falken@chromium.org
==========

to

==========
Upstream service worker 'XHR' test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org
==========

[mike3, 2017-05-25 16:24:37]

Description was changed from

==========
Upstream service worker 'XHR' test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org
==========

to

==========
Upstream service worker "XHR" test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org
==========

[falken, 2017-05-29 04:22:15]

Yes the Chromium implementation made a design decision really early on that sync
XHR should not be intercepted. I didn't realize the spec strongly has asserted
the opposite since then.

I think it's OK to remove the Chromium test but I'd like the WPT test to be more
specific that it's testing interception. It doesn't actually exercise the path
that would cause Chromium to deadlock, which would be a fetch() from the worker,
but that's OK, as it's an implementation specific issue.

So I think the only change is to remove the mentions of " "deadlock" from the
test.

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:2:
<title>Service Worker: Synchronous XHR doesn't deadlock</title>
is intercepted

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:38:
}, 'Verify SyncXHR does not deadlock');
Verify SyncXHR is intercepted

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html
(right):

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html:13:
throw 'FAIL';
just for expectation hardening, can you include the output of responseTest in
the FAIL? (I'm curious what it is actually)

[mike3, 2017-05-29 16:14:21]

Thanks, Matt! Just one question...

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:2:
<title>Service Worker: Synchronous XHR doesn't deadlock</title>
On 2017/05/29 04:22:15, falken wrote:
> is intercepted

Done.

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:38:
}, 'Verify SyncXHR does not deadlock');
On 2017/05/29 04:22:15, falken wrote:
> Verify SyncXHR is intercepted

Done.

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html
(right):

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html:2:
<title>Service Worker: Synchronous XHR doesn't deadlock iframe</title>
I'll also remove the reference to "blocking" from this title.

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html:13:
throw 'FAIL';
On 2017/05/29 04:22:15, falken wrote:
> just for expectation hardening, can you include the output of responseTest in
> the FAIL? (I'm curious what it is actually)

Sure. I've refactored this to return the value of the `responseText` property
and use `assert_equals` in the test body. The resulting error in Chromium
reads:

> assert_equals: value of responseText expected "hello" but got "{\"error\":
{\"message\": \"\", \"code\": 404}}"

This is the default payload for 404 responses from wptserve [1] [2]. I can
see how this extra information may be more informative, but it may also make
the test more fragile, for Chromium anyway. That's because we'll need to
encode the body in the "expectations" file, and it's possible that some
future change to wptserve modifies this message. Because the response body
isn't tested within the wptserve project [3], it's tough to say whether it
is considered stable. Do you think we should avoid relying on it for that
reason? Or should I shore things up by contributing documentation and a few
tests to wptserve?

[1]
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...
[2]
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...
[3]
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...

[falken, 2017-05-30 01:08:42]

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html
(right):

https://codereview.chromium.org/2907443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html:13:
throw 'FAIL';
On 2017/05/29 16:14:21, mike3 wrote:
> On 2017/05/29 04:22:15, falken wrote:
> > just for expectation hardening, can you include the output of responseTest
in
> > the FAIL? (I'm curious what it is actually)
> 
> Sure. I've refactored this to return the value of the `responseText` property
> and use `assert_equals` in the test body. The resulting error in Chromium
> reads:
> 
> > assert_equals: value of responseText expected "hello" but got "{\"error\":
> {\"message\": \"\", \"code\": 404}}"
> 
> This is the default payload for 404 responses from wptserve [1] [2]. I can
> see how this extra information may be more informative, but it may also make
> the test more fragile, for Chromium anyway. That's because we'll need to
> encode the body in the "expectations" file, and it's possible that some
> future change to wptserve modifies this message. Because the response body
> isn't tested within the wptserve project [3], it's tough to say whether it
> is considered stable. Do you think we should avoid relying on it for that
> reason? Or should I shore things up by contributing documentation and a few
> tests to wptserve?
> 
> [1]
>
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...
> [2]
>
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...
> [3]
>
https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818...

Good point, I think you're right: my suggestion made the test more fragile.

I can think of some approaches:
(1) Go back to the first approach where we just test for the expected string.
(Would prefer using 'PASS' instead of 'hello' though.)
(2) Add the server-side file with 'FAIL: service worker did not intercept the
request' as the response text, instead of relying on 404 from WPT.
(3) Use assert_equals on response status code instead of response text.

I kind of think (2) makes the test easiest to understand, but may be overkill. I
think (3) should be fairly simple and robust. WDYT?

https://codereview.chromium.org/2907443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-worker.js
(right):

https://codereview.chromium.org/2907443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-worker.js:5:
event.respondWith(new Response('hello'));
how about changing this string to 'PASS'

[mike3, 2017-05-30 14:48:15]

> I kind of think (2) makes the test easiest to understand, but may be
> overkill. I think (3) should be fairly simple and robust. WDYT?

Agree on all accounts. Thanks for being so thorough; I really enjoy getting
into the weeds like this :)

https://codereview.chromium.org/2907443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-worker.js
(right):

https://codereview.chromium.org/2907443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-worker.js:5:
event.respondWith(new Response('hello'));
On 2017/05/30 01:08:41, falken wrote:
> how about changing this string to 'PASS'

It probably doesn't matter as much given the change of assertion, but I've used
the value 'Response from service worker' to make this even more explicit.

[falken, 2017-05-31 01:39:33]

Thanks for revising! After seeing my suggestion in action, now I'm paranoid
someone could accidentally add the -iframe.html file and the test would pass
inappropriately.

A final suggestion: test the body text after all, but after the response code.
That should be fine, right?

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:10:
var url = 'resources/fetch-request-xhr-sync-worker.js';
Suggest comment:
// The |scope| file does not exist. Test that the service worker intercepts the
request and responds.

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:34:
);
Adding an assert for the body text should be fine now, right?

[mike3, 2017-05-31 16:12:11]

I've taken your advice and re-introduced the `responseText` assertion. This
was making the Promise executor function a little heavy, so I've moved the
assertions to a subsequent handler in the chain.

In lieu of a code comment, I've opted to use a different file name to
produce a 404 in non-conforming implementations: "non-existent-file.txt".
This should further discourage the kind of mistake you were afraid
of--future contributors would be hard-pressed to accidentally create a file
with a name like that. I've also parameterized the iframe's function so that
this name can be declared within the test body itself. Hopefully this makes
our intent a little more obvious.

This is shaping up to be the strongest test ever written for a deprecated
feature.

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:10:
var url = 'resources/fetch-request-xhr-sync-worker.js';
On 2017/05/31 01:39:33, falken wrote:
> Suggest comment:
> // The |scope| file does not exist. Test that the service worker intercepts
the
> request and responds.

That file *does* exist; it's the file requested from that document which is
non-existent--further justification for making the code more clear.

https://codereview.chromium.org/2907443002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:34:
);
On 2017/05/31 01:39:33, falken wrote:
> Adding an assert for the body text should be fine now, right?

Acknowledged.

[falken, 2017-06-01 03:57:11]

LGTM!

> This is shaping up to be the strongest test ever written for a deprecated
feature.

Ha! Yep, looks great now.

https://codereview.chromium.org/2907443002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:12:
var non_existent_file = 'non-existent-file.txt?bustcache=' + Date.now();
By the way: do you know why this bustcache= thing is needed? I don't know why it
was in the original test. We seem to not use this technique in other tests. If
you don't know either, I think we can just remove it.

[mike3, 2017-06-01 15:01:04]

I've rebased and removed a reference to the deleted test from a
recently-introduced "expectations" file. Now requesting merge.

https://codereview.chromium.org/2907443002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html
(right):

https://codereview.chromium.org/2907443002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html:12:
var non_existent_file = 'non-existent-file.txt?bustcache=' + Date.now();
On 2017/06/01 03:57:11, falken wrote:
> By the way: do you know why this bustcache= thing is needed? I don't know why
it
> was in the original test. We seem to not use this technique in other tests. If
> you don't know either, I think we can just remove it.

Now that you mention it, it looks like a vestige of the original (erroneous)
factoring. Removing...

[mike3, 2017-06-01 15:01:22]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-06-01 15:01:22]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2907443002/#ps80001
(title: "Simplify request URL and remove outdated reference")

[commit-bot: I haz the power, 2017-06-01 15:01:50]

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-06-01 16:44:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496329282225640,
"parent_rev": "1704affd92a33a48072fa0dfb0a63af1853a493a", "commit_rev":
"7fdac9fc2a776ad2ed4c3df752896e0f33861b69"}

[commit-bot: I haz the power, 2017-06-01 16:45:00]

Description was changed from

==========
Upstream service worker "XHR" test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org
==========

to

==========
Upstream service worker "XHR" test to WPT

The original version of this test would only pass if the service worker
did *not* intercept the XHR request. That behavior conflicted with the
Fetch and Service Worker specification and with the test's documentation
itself. Update the test to pass only if the service worker intercepts
the request.

In addition:

- Update URLs to suitable values for the Web Platform Tests project
- Add "use strict" directives to script bodies
- Increase precision of "cleanup" logic scheduling
- Re-name files to adhere to precent set by existing WPT tests
- Catch and report errors within Promise chain rather than relying on
  implicit reporting of uncaught errors.

BUG=688116, 602051
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2907443002
Cr-Commit-Position: refs/heads/master@{#476324}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fdac9fc2a776ad2ed4c3df75289...
==========

[commit-bot: I haz the power, 2017-06-01 16:45:01]

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

[mike3, 2017-06-01 23:41:08]

On 2017/06/01 16:45:01, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/7fdac9fc2a776ad2ed4c3df75289...

Thanks, Matt!