Upstream service worker `fetch` tests to WPT

Upstream service worker `fetch` tests to WPT

Reconcile the remaining Service Worker tests that concern the Fetch
standard. As noted below, some tests must be persisted in the Chromium
project due to unresolved timeout issues in the build environment (see
https://crbug.com/658997).

**fetch-event-async-respond-with**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to document its deprecated status
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event-network-error**

The upstream version of this test is almost identical to the
Chromium-specific version.

The Chromium-specific version of this test is almost identical to the
version available upstream. The upstream version is preferable because
it contains an additional assertion not found in the Chromium version.

Remove the Chromium-specific version of this test, and update the
upstream version to omit an unnecessary resource.

**fetch-event-respond-with-argument**

Re-locate Chromium-specific test to the Web Platform Test suite
directory so that it may be included in that project.

**fetch-event-respond-with-stops-propagation**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to reflect its deprecated status,
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event**

Remove sub-tests from the Chromium-specific version that are present in
the upstream version. Rename the test file to reflect the
implementation-specific nature of the test.

Extend the upstream version with a relaxed version of Chromium's
sub-test for request headers.

In both versions, formalize tear down logic to ensure consistent frame
removal and subsequent client de-registration.

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

Review-Url: https://codereview.chromium.org/2836233002
Cr-Commit-Position: refs/heads/master@{#468341}
Committed: https://chromium.googlesource.com/chromium/src/+/07b83d70ca740271a8acaa3c3e9b32eb01d5c48b

[mike3, 2017-04-24 22:42:55]

Hi mek: would you mind taking a look?

[Marijn Kruisselbrink, 2017-04-27 17:05:24]

sorry for the long delay in reviewing

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html:19:
add_completion_callback(function() { frame.remove(); });
nit: Should this be t.add_cleanup, rather than the global
add_completion_callback? Not that it'll make much difference in practice, and it
seems most (but not all) existing service worker tests use the global
add_completion_callback anyway for this kind of thing...

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

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html:10:
async_test(function(t) {
nit: I suppose this whole file predates the existence of promise_test, which is
why it uses async_test instead? Probably fine to leave as async_test for now for
consistency with the rest of the file, but eventually might be nice to update
all these tests that really are promise_tests to be written using
promise_test().

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html:322:
});
and it seems this one test is a great example of why these tests should be
written as promise_test, missing .catch(unreached_rejection(t)), so this test
can't fail...

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-async-respond-with.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-async-respond-with.html:7:
https://codereview.chromium.org/2805313003/ -->
Is there a bug you can link to instead of a code review?

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-headers.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-headers.html:14:
async_test(function(t) {
nit: promise_test?

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-respond-with-stops-propagation.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-respond-with-stops-propagation.html:7:
https://codereview.chromium.org/2805313003/ -->
link to bug rather than codereview?

[mike3, 2017-04-27 18:27:19]

Thanks for the review, Mek! I'm going to submit my latest work as a series of
smaller patches. I'm hoping that will be a little more convenient for you.

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html:19:
add_completion_callback(function() { frame.remove(); });
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> nit: Should this be t.add_cleanup, rather than the global
> add_completion_callback? Not that it'll make much difference in practice, and
it
> seems most (but not all) existing service worker tests use the global
> add_completion_callback anyway for this kind of thing...

Agreed. Beyond being more consistent, `add_cleanup` will produce more
meaningful/localized errors if this should ever fail.

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

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html:10:
async_test(function(t) {
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> nit: I suppose this whole file predates the existence of promise_test, which
is
> why it uses async_test instead? Probably fine to leave as async_test for now
for
> consistency with the rest of the file, but eventually might be nice to update
> all these tests that really are promise_tests to be written using
> promise_test().

Agreed. I'm trying to avoid changes that are not strictly necessary with these
patches because the migration makes it harder to detect modifications (both
during initial review and future bisects).

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html:322:
});
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> and it seems this one test is a great example of why these tests should be
> written as promise_test, missing .catch(unreached_rejection(t)), so this test
> can't fail...

The result of this particularly lengthy promise chain is being returned by a
non-test utility function, `run_referrer_policy_tests`. Its return value is
being used in the subsequent test, where rejections are correctly caught and
passed along to `unreached_rejection`.

Your point stands, though: using `promise_test` will make this more clear, less
error prone, and easier to review overall.

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-async-respond-with.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-async-respond-with.html:7:
https://codereview.chromium.org/2805313003/ -->
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> Is there a bug you can link to instead of a code review?

I was linking to the code review thread because that is where Matt and I
decided on this approach. That said, issue 658997 is just as helpful (and
probably more to the point):

https://crbug.com/658997

I'll update with references to that instead.

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-headers.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-headers.html:14:
async_test(function(t) {
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> nit: promise_test?

Acknowledged.

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-respond-with-stops-propagation.html
(right):

https://codereview.chromium.org/2836233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-respond-with-stops-propagation.html:7:
https://codereview.chromium.org/2805313003/ -->
On 2017/04/27 17:05:23, Marijn Kruisselbrink wrote:
> link to bug rather than codereview?

Acknowledged.

[Marijn Kruisselbrink, 2017-04-27 18:44:03]

thanks, lgtm

[mike3, 2017-04-27 18:58:34]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2017-04-27 18:59:23]

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-04-27 19:53:28]

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

[commit-bot: I haz the power, 2017-04-27 19:53:29]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mike3, 2017-05-01 16:15:54]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-05-01 16:15:54]

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/2836233002/#ps40001
(title: "Extend "-expected.txt" file with reference to new sub-test")

[commit-bot: I haz the power, 2017-05-01 16:16:30]

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-01 17:25:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493655354135340,
"parent_rev": "76aca77545743215506fc9e9eb0c1299e89553ae", "commit_rev":
"07b83d70ca740271a8acaa3c3e9b32eb01d5c48b"}

[commit-bot: I haz the power, 2017-05-01 17:25:35]

Description was changed from

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

Reconcile the remaining Service Worker tests that concern the Fetch
standard. As noted below, some tests must be persisted in the Chromium
project due to unresolved timeout issues in the build environment (see
https://crbug.com/658997).

**fetch-event-async-respond-with**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to document its deprecated status
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event-network-error**

The upstream version of this test is almost identical to the
Chromium-specific version.

The Chromium-specific version of this test is almost identical to the
version available upstream. The upstream version is preferable because
it contains an additional assertion not found in the Chromium version.

Remove the Chromium-specific version of this test, and update the
upstream version to omit an unnecessary resource.

**fetch-event-respond-with-argument**

Re-locate Chromium-specific test to the Web Platform Test suite
directory so that it may be included in that project.

**fetch-event-respond-with-stops-propagation**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to reflect its deprecated status,
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event**

Remove sub-tests from the Chromium-specific version that are present in
the upstream version. Rename the test file to reflect the
implementation-specific nature of the test.

Extend the upstream version with a relaxed version of Chromium's
sub-test for request headers.

In both versions, formalize tear down logic to ensure consistent frame
removal and subsequent client de-registration.

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

to

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

Reconcile the remaining Service Worker tests that concern the Fetch
standard. As noted below, some tests must be persisted in the Chromium
project due to unresolved timeout issues in the build environment (see
https://crbug.com/658997).

**fetch-event-async-respond-with**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to document its deprecated status
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event-network-error**

The upstream version of this test is almost identical to the
Chromium-specific version.

The Chromium-specific version of this test is almost identical to the
version available upstream. The upstream version is preferable because
it contains an additional assertion not found in the Chromium version.

Remove the Chromium-specific version of this test, and update the
upstream version to omit an unnecessary resource.

**fetch-event-respond-with-argument**

Re-locate Chromium-specific test to the Web Platform Test suite
directory so that it may be included in that project.

**fetch-event-respond-with-stops-propagation**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite, though it suffers from
timeout issues in the build environment.

Rename the Chromium-specific version to reflect its deprecated status,
and improve the upstream version to defer frame removal and omit
unnecessary resource.

**fetch-event**

Remove sub-tests from the Chromium-specific version that are present in
the upstream version. Rename the test file to reflect the
implementation-specific nature of the test.

Extend the upstream version with a relaxed version of Chromium's
sub-test for request headers.

In both versions, formalize tear down logic to ensure consistent frame
removal and subsequent client de-registration.

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

Review-Url: https://codereview.chromium.org/2836233002
Cr-Commit-Position: refs/heads/master@{#468341}
Committed:
https://chromium.googlesource.com/chromium/src/+/07b83d70ca740271a8acaa3c3e9b...
==========

[commit-bot: I haz the power, 2017-05-01 17:25:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/07b83d70ca740271a8acaa3c3e9b...