Upstream service worker `fetch` tests to WPT

Upstream service worker `fetch` tests to WPT

The Web Platform Tests project includes equivalent versions of the
following tests:

- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html
- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html

Remove the above Chromium-specific files to promote the use of the more
widely shared versions.

The following test file is more thorough than the equivalent version in
WPT. It contains the same assertions as those found in the WPT version,
but it also includes versions with an alternative value of the `img-src`
CSP directive.

- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-csp.html

Extend the WPT version with the new assertions and remove the
Chromium-specific file. Also remove infrastructure configuration which
previously prevented the WPT version from being executed.

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

Review-Url: https://codereview.chromium.org/2805313003
Cr-Commit-Position: refs/heads/master@{#466697}
Committed: https://chromium.googlesource.com/chromium/src/+/b5cd9b369845c99709d101b09c350170b34fd51f

[mike3, 2017-04-07 22:36:06]

Getting started with a few `fetch`-related tests. This contains two test
deletion and one test migration.

The deleted tests do not differ substantially from WPT's version. WPT provides
a more specific alternative to a helper function named `login`--it is named
`login_https`. Comparisons available here:
https://gist.github.com/jugglinmike/bc0525e9710ef1d477ad07035b9d2ff7

The migrated test passes in both Chromium and Firefox. One thing that confuses
me, though, is how this test was previously interpreted.

First, the WPT version was being skipped for timing out. Although the format is
distinct (e.g. using `promise_test` instead of `async_test`), I don't see any
substantive difference in behavior exercised by the two versions of the test.
This makes me think that the WPT version may have been included among the
blacklist only accidentally. To be safe, I've referenced the corresponding bug
in this patch's description.

Second, the Chromium version had a corresponding `-expected.txt` file. I was
prepared to move this as I migrated the test, but that doesn't seem to be
necessary; the command:

    third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
external/wpt/service-workers/service-worker/fetch-csp.https.html

Produced the following output:

    Found 1 test; running 1, skipping 0.
    
    Running 1 content_shell.
    
    The test ran as expected.

So I'm not sure if I'm missing something about how this test is run in
automation.

[jsbell, 2017-04-08 00:00:00]

Drive by...

On 2017/04/07 22:36:06, mike3 wrote:
> Second, the Chromium version had a corresponding `-expected.txt` file. I was
> prepared to move this as I migrated the test, but that doesn't seem to be
> necessary

If no cases FAIL for a testharness.js test then the `-expected.txt` file is not
needed and we usually don't include it. So deleting it here is great.

[falken, 2017-04-10 14:10:51]

lgtm, the fetch-csp test is much easier to read!

But external/wpt/service-workers/service-worker/fetch-csp.https.html was skipped
because it was timing out:
https://bugs.chromium.org/p/chromium/issues/detail?id=658997

Did the test change somehow fix the timeout problem?

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

https://codereview.chromium.org/2805313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-csp.https.html:108:
service_worker_unregister_and_done(t, SCOPE);
nit: This could just be registration.unregister() if we keep the registration
around. I like that better since s_w_u_a_d() does another lookup of the
registration and it needlessly calls t.done() in a promise_test.

[mike3, 2017-04-10 16:59:34]

> Did the test change somehow fix the timeout problem?

That's difficult for me to say because the reported timeout was limited to
release builds. Would you like me to research this further?

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

https://codereview.chromium.org/2805313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-csp.https.html:108:
service_worker_unregister_and_done(t, SCOPE);
On 2017/04/10 14:10:51, falken wrote:
> nit: This could just be registration.unregister() if we keep the registration
> around. I like that better since s_w_u_a_d() does another lookup of the
> registration and it needlessly calls t.done() in a promise_test.

This is a pattern we'll want to follow throughout the service workers tests, but
it's a little cumbersome to extend it as part of the `promise_test` return
value. testharness.js defines a `t.cleanup` method, which would be much easier
to use (and therefore more likely to be used). Unfortunately that method does
not currently support asynchronous operations, so we'd risk a race condition by
calling `unregister` from there. I'm going to see about extending the method to
allow returning a Promise instance:

https://github.com/w3c/testharness.js/issues/261

I hope to report back with an improved version of this test in the next few
days.

[falken, 2017-04-11 06:52:08]

Actually "Release" build is the typical build when you build Chromium locally.
It's distinguished from a "Debug" build which includes debug symbols/asserts
etc. The Debug builds are given a longer time limit when running the tests. So
likely the test timed out because it's slow but debug builds had enough time to
complete.

If you could investigate locally whether the test times out before the patch,
and doesn't time out after the patch, that'd be useful information.

[mike3, 2017-04-14 22:52:21]

This test is one of a set of tests that has apparently been causing issues in
the build process since last October. They were first classified as "Slow" in
October of last year with the comment, "This patch is to keep the bots green."
[1] One month later, they were changed to "Timeout" following a
re-configuration of testharness.js with the explanation that they would, "be
continuing to run longer, and either timing out or failing in a different way."
[2] The next day, they were re-classified as "Skip" with the comment, "These
tests constantly time out and are time-consuming." [3]

It seems like this behavior was limited to the continuous integration
environment, at least initially. Judging from the subsequent commit messages
alone, I can't determine if this was ever a problem experienced during
development. My inclination is to run a bisect here, but I'm not sure how to do
that effectively in this project because building Chromium takes over an hour
on my system. I'm wondering if regular contributors have dedicated
tooling/hardware for performing bisects. Or did I just stumble on an excuse to
upgrade my machine? :)

[1] https://chromium.googlesource.com/chromium/src.git/+/74e0e4302
[2] https://chromium.googlesource.com/chromium/src.git/+/1a8851a50
[3] https://chromium.googlesource.com/chromium/src.git/+/14b49db68

[falken, 2017-04-17 02:25:53]

Yes we never tracked down the cause of the timeout failures. Actually all the
tests that are being removed from Chromium in this patch have a Skip expectation
for their corresponding WPT test:
fetch-canvas-tainting.https.html
fetch-cors-xhr.https.html
fetch-csp.https.html

Since we can’t verify that the test would pass in WPT, it’s safest to not remove
the Chromium-side tests.

Unfortunately the tests are marked skipped so we’ll never know if they pass.

When I run locally none of the tests timeout. However fetch-canvas-tainting.html
and fetch-cors-xhr.https.html fail with wrong asserts.

Let’s try this:
- Update the *expected.txt files for fetch-canvas-tainting.html and
fetch-cors-xhr.https.html by running locally with --reset-results. Actually I
can provide those files now.
- In TestExpectations, change the [ Skip ] for the three tests to [ Pass Timeout
].
- Don’t remove the tests from Chromium yet. You could put a TODO that they could
be removed when the WPT tests pass, with a link to this codereview/bug.

This way, we can see on the bots if the tests are passing or still timing out.
If they still timeout consistently, the service worker team can investigate
further and try to fix.

[falken, 2017-04-17 03:25:56]

On 2017/04/17 02:25:53, falken wrote:
> Yes we never tracked down the cause of the timeout failures. Actually all the
> tests that are being removed from Chromium in this patch have a Skip
expectation
> for their corresponding WPT test:
> fetch-canvas-tainting.https.html
> fetch-cors-xhr.https.html
> fetch-csp.https.html
> 
> Since we can’t verify that the test would pass in WPT, it’s safest to not
remove
> the Chromium-side tests.
> 
> Unfortunately the tests are marked skipped so we’ll never know if they pass.
> 
> When I run locally none of the tests timeout. However
fetch-canvas-tainting.html
> and fetch-cors-xhr.https.html fail with wrong asserts.
> 
> Let’s try this:
> - Update the *expected.txt files for fetch-canvas-tainting.html and
> fetch-cors-xhr.https.html by running locally with --reset-results. Actually I
> can provide those files now.
> - In TestExpectations, change the [ Skip ] for the three tests to [ Pass
Timeout
> ].
> - Don’t remove the tests from Chromium yet. You could put a TODO that they
could
> be removed when the WPT tests pass, with a link to this codereview/bug.
> 
> This way, we can see on the bots if the tests are passing or still timing out.
> If they still timeout consistently, the service worker team can investigate
> further and try to fix.

Here's what I ran:
$ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
--skipped=ignore --no-retry-failures --reset-results
external/wpt/service-workers/service-worker/fetch-cors-xhr*
$ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
--skipped=ignore --no-retry-failures --reset-results
external/wpt/service-workers/service-worker/fetch-canvas*

$ cat
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-canvas-tainting.https-expected.txt
This is a testharness.js-based test.
FAIL Verify canvas tainting of fetched image in a Service Worker assert_equals:
expected "finish" but got "failure:Result of
url:https://www1.web-platform.test:8444/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=same-origin&url=https%3A%2F%2Fweb-platform.test%3A8444%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE
 cross_origin:  must be TAINTED but NOT_TAINTED"
Harness: the test ran to completion.

$ cat
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-cors-xhr.https-expected.txt
This is a testharness.js-based test.
FAIL Verify CORS XHR of fetch() in a Service Worker assert_equals: expected
"finish" but got "failure:Result of
url:https://web-platform.test:8444/service-workers/service-worker/resources/fetch-access-control.py?mode=cors&url=https%3A%2F%2Fwww1.web-platform.test%3A8444%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8444
 with_credentials: false must be SUCCESS but FAIL"
Harness: the test ran to completion.

[falken, 2017-04-17 07:50:28]

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

[commit-bot: I haz the power, 2017-04-17 07:50:46]

Dry run: 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-17 09:24:29]

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

[commit-bot: I haz the power, 2017-04-17 09:24:30]

Dry run: This issue passed the CQ dry run.

[mike3, 2017-04-17 23:04:04]

Thanks for the help, @falken! Unfortunately, I was unable to make use of it
today because the build of Chromium that I completed on Friday evening was
crashing for any webkittest. Here's the crash log:

https://gist.github.com/jugglinmike/7cb7575fd777f740ce43aaf859cfa380

I've confirmed my approach to building with a few folks in #chromium, and
nothing seems wrong there. I wasn't able to learn anything more about the error
itself, so I decided to fetch the latest and try building again.

That just now finished, and I'm still seeing a renderer crash. I should mention
that the build (invoked via `ninja -C out/Default chrome`) failed three times
before ultimately completing all the artifacts. Since I wasn't sure if this was
just a resource limitation issue, I simply re-ran that command after each
failure. I don't know if this piecemeal process can be expected to produce a
valid build, but I didn't know of a better way to proceed.

Here is some data on the latest crash:

https://gist.github.com/jugglinmike/80d9da2cc76d456b1e394aedfe462d74

Do you have any tips for how I could debug this myself? Or maybe someone that
wouldn't mind holding my hand during US working hours?

[falken, 2017-04-18 01:41:30]

On 2017/04/17 23:04:04, mike3 wrote:
> Thanks for the help, @falken! Unfortunately, I was unable to make use of it
> today because the build of Chromium that I completed on Friday evening was
> crashing for any webkittest. Here's the crash log:
> 
> https://gist.github.com/jugglinmike/7cb7575fd777f740ce43aaf859cfa380
> 
> I've confirmed my approach to building with a few folks in #chromium, and
> nothing seems wrong there. I wasn't able to learn anything more about the
error
> itself, so I decided to fetch the latest and try building again.
> 
> That just now finished, and I'm still seeing a renderer crash. I should
mention
> that the build (invoked via `ninja -C out/Default chrome`) failed three times
> before ultimately completing all the artifacts. Since I wasn't sure if this
was
> just a resource limitation issue, I simply re-ran that command after each
> failure. I don't know if this piecemeal process can be expected to produce a
> valid build, but I didn't know of a better way to proceed.
> 
> Here is some data on the latest crash:
> 
> https://gist.github.com/jugglinmike/80d9da2cc76d456b1e394aedfe462d74
> 
> Do you have any tips for how I could debug this myself? Or maybe someone that
> wouldn't mind holding my hand during US working hours?

This looks like https://bugs.chromium.org/p/chromium/issues/detail?id=600425 but
I'm not sure that bug will help diagnose the issue. "Apparently the build step
that creates an updated snapshot upon building V8 does not get executed in your
workflow. You could check this by looking for the timestamp of
snapshot_blob.bin"

If I were you I'd sync to the latest source ("git rebase-update" and "gclient
sync") then do "gn clean" and build again but I know that's painful on your
system.

I think you might be able to finish this patch without building Chrome just by
undeleting the Chrome test files, adjustign the TestExpectations, and adding the
-expected.txt files from the above comments. Then the trybots would be able to
catch any errors (of course those also take time).

[mike3, 2017-04-18 16:54:10]

Okay, I've made those changes with one slight difference: I've moved the tests
to the `chromium/` directory. I'll trigger the Try bots now to see if these
"blind" modifications were valid.

Otherwise, I've left some information on the bug report you linked (if only so
folks know it's still an issue). I'm starting a new from-scratch build now.

As ever, I appreciate your patience through this!

[mike3, 2017-04-18 16:54:16]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-04-18 16:54:17]

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/2805313003/#ps20001
(title: "Re-introduce Chromium-specific tests")

[commit-bot: I haz the power, 2017-04-18 16:54:37]

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-18 18:09:19]

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

[commit-bot: I haz the power, 2017-04-18 18:09:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mike3, 2017-04-18 22:29:51]

As reported by commit-bot, the build is failing. Relevant details:

>     Unexpected Failures:
>     *
external/wpt/service-workers/service-worker/fetch-canvas-tainting.https.html
>     * external/wpt/service-workers/service-worker/fetch-cors-xhr.https.html
>     * fast/events/pointerevents/multi-pointer-event-in-slop-region.html
>     * fast/events/pointerevents/pointer-event-in-slop-region.html
>     * http/tests/serviceworker/chromium/fetch-canvas-tainting.html
>     * http/tests/serviceworker/chromium/fetch-cors-xhr.html
>     * http/tests/serviceworker/chromium/fetch-csp.html
>     *
virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-canvas-tainting.html
>     *
virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-cors-xhr.html
>     * virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-csp.html

The failures in Chromium-specific tests are likely my own fault--moving the
tests around effect the relative paths, and without being able to verify
locally, I expect I made a few errors.

There are some things I can't account for, though (and I haven't been able to
find more information in the logs themselves):

- It looks like the runner didn't recognize the `-expected.txt` files, since it
  is reporting those WPT failures as "unexpected"
- The `pointerevents` failures seem completely unrelated. Is it possible that
  we happened to run across a couple flaky tests?

I was hoping to get some answers for myself, but my Chromium build is only just
now completing at the end of the day. I hope to use that to find some answers
tomorrow morning.

[falken, 2017-04-18 22:52:53]

On 2017/04/18 22:29:51, mike3 wrote:
> As reported by commit-bot, the build is failing. Relevant details:
> 
> >     Unexpected Failures:
> >     *
> external/wpt/service-workers/service-worker/fetch-canvas-tainting.https.html
> >     * external/wpt/service-workers/service-worker/fetch-cors-xhr.https.html
> >     * fast/events/pointerevents/multi-pointer-event-in-slop-region.html
> >     * fast/events/pointerevents/pointer-event-in-slop-region.html
> >     * http/tests/serviceworker/chromium/fetch-canvas-tainting.html
> >     * http/tests/serviceworker/chromium/fetch-cors-xhr.html
> >     * http/tests/serviceworker/chromium/fetch-csp.html
> >     *
>
virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-canvas-tainting.html
> >     *
> virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-cors-xhr.html
> >     * virtual/mojo-loading/http/tests/serviceworker/chromium/fetch-csp.html
> 
> The failures in Chromium-specific tests are likely my own fault--moving the
> tests around effect the relative paths, and without being able to verify
> locally, I expect I made a few errors.
> 
> There are some things I can't account for, though (and I haven't been able to
> find more information in the logs themselves):
> 
> - It looks like the runner didn't recognize the `-expected.txt` files, since
it
>   is reporting those WPT failures as "unexpected"
> - The `pointerevents` failures seem completely unrelated. Is it possible that
>   we happened to run across a couple flaky tests?
> 
> I was hoping to get some answers for myself, but my Chromium build is only
just
> now completing at the end of the day. I hope to use that to find some answers
> tomorrow morning.

Hi Mike, I'm in a bit early today. Would you be able to chat on IRC?

[falken, 2017-04-18 22:59:06]

... though I have a meeting soon... probably it's too late for your time zone
now anyway.

Quick response before the meeting:
- You can likely ignore pointerevents just because you didn't touch that code.
To be sure you can check the Flakiness Dashboard like this:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType...
You can see the test was failing for some recent window of time.

- To get nicer diff output, try the "layout_test_results" link under
"archive_webkit_tests_results" from the bot run page. E.g.,:
from
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
go to
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

[falken, 2017-04-19 01:53:49]

On 2017/04/18 22:59:06, falken (away until May 8) wrote:
> ... though I have a meeting soon... probably it's too late for your time zone
> now anyway.
> 
> Quick response before the meeting:
> - You can likely ignore pointerevents just because you didn't touch that code.
> To be sure you can check the Flakiness Dashboard like this:
>
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType...
> You can see the test was failing for some recent window of time.
> 
> - To get nicer diff output, try the "layout_test_results" link under
> "archive_webkit_tests_results" from the bot run page. E.g.,:
> from
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> go to
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

It looks like the failures are due to
- whitespace errors in the expected.txt files
- the path restriction for service worker scopes: "Failed to register a
ServiceWorker: The path of the provided scope
('/serviceworker/chromium/resources/fetch-canvas-tainting-iframe.html') is not
under the max scope allowed ('/serviceworker/resources/')."

So moving to chromium/ would also require moving or copying the resources/ files
under the chromium/ path. Also at least one helper function "login()" used in
fetch-canvas-tainting.html hardcodes a path.

In retrospect yea it's probably difficult to do this patch without a local build
working :)

[mike3, 2017-04-24 17:19:43]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-04-24 17:19:44]

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/2805313003/#ps40001
(title: "Move files up a directory")

[commit-bot: I haz the power, 2017-04-24 17:20: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-04-24 18:39:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493054383979640,
"parent_rev": "136757e4c0f776623264be10696a68afec588a8e", "commit_rev":
"b5cd9b369845c99709d101b09c350170b34fd51f"}

[commit-bot: I haz the power, 2017-04-24 18:39:32]

Description was changed from

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

The Web Platform Tests project includes equivalent versions of the
following tests:

-
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html
- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html

Remove the above Chromium-specific files to promote the use of the more
widely shared versions.

The following test file is more thorough than the equivalent version in
WPT. It contains the same assertions as those found in the WPT version,
but it also includes versions with an alternative value of the `img-src`
CSP directive.

- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-csp.html

Extend the WPT version with the new assertions and remove the
Chromium-specific file. Also remove infrastructure configuration which
previously prevented the WPT version from being executed.

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

to

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

The Web Platform Tests project includes equivalent versions of the
following tests:

-
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html
- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html

Remove the above Chromium-specific files to promote the use of the more
widely shared versions.

The following test file is more thorough than the equivalent version in
WPT. It contains the same assertions as those found in the WPT version,
but it also includes versions with an alternative value of the `img-src`
CSP directive.

- third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-csp.html

Extend the WPT version with the new assertions and remove the
Chromium-specific file. Also remove infrastructure configuration which
previously prevented the WPT version from being executed.

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

Review-Url: https://codereview.chromium.org/2805313003
Cr-Commit-Position: refs/heads/master@{#466697}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5cd9b369845c99709d101b09c35...
==========

[commit-bot: I haz the power, 2017-04-24 18:39:33]

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