Fix service worker tests generated from non-https .any.js #12381

jgraham · 2018-08-09T15:42:42Z

Note: this contains a change to the expected behaviour of test
runners.

Previously .any.js tests specifying a serviceworker variant generated
a url like foo.https.any.serviceworker.html. But that causes a problem
because that looks exctly like a url generated from a file named
foo.https.js. This ambiguity, together with the fact that wptrunner
wasn't actually looking at the https flag in the test id, but only in
the filename, meant that service worker tests weren't being run
properly.

This change stops adding .https. to serviceworker tests generated from
.any.js files and instead makes .serviceworker. in the test id
directly mean that the test should be loaded over https.

foolip · 2018-08-09T18:03:14Z

Can you add tests in /infrastructure/ that will fail in runners that don't load .serviceworker. over https? We'll need to import the wptserve change into Chromium and have a test fail if we don't would be sweet.

jgraham · 2018-08-10T10:09:06Z

Added a test.

gsnedders

Can you add some unit test for this as well? Checking URLManifestItem has the expected behaviour?

gsnedders · 2018-08-10T10:42:16Z

tools/manifest/item.py

@@ -46,14 +46,19 @@ def id(self):
        """The test's id (usually its url)"""
        pass

+    @property
+    def meta_flags(self):
+        return self.source_file.meta_flags


Should this logically not be a set rather than a list? (I realise it currently isn't, and I'm not gonna block this PR on that.)

Ms2ger · 2018-08-10T12:49:34Z

tools/manifest/tests/test_item.py

+from ..sourcefile import SourceFile
+
+
+class TestManifestItem(ManifestItem):


Maybe use SupportFile instead of a mock object?

Ms2ger · 2018-08-10T13:33:02Z

And can you run infrastructure/server/secure-context.https.any.js on a service worker as well?

Note: this contains a change to the expected behaviour of test runners. Previously .any.js tests specifying a serviceworker variant generated a url like foo.https.any.serviceworker.html. But that causes a problem because that looks exctly like a url generated from a file named foo.https.js. This ambiguity, together with the fact that wptrunner wasn't actually looking at the https flag in the test id, but only in the filename, meant that service worker tests weren't being run properly. This change stops adding .https. to serviceworker tests generated from .any.js files and instead makes .serviceworker. in the test id directly mean that the test should be loaded over https.

Hexcles · 2018-08-10T14:55:38Z

tools/manifest/item.py

@@ -95,6 +100,10 @@ def __init__(self, source_file, url, url_base="/", manifest=None):
    def id(self):
        return self.url

+    @property
+    def meta_flags(self):
+        return set(urlparse(self.url).path.rsplit("/", 1)[1].split(".")[1:-1])


Is there any guarantee that the test name in URL will always have at least a dot?

It doesn't matter.

In [1]: "abc".split(".")[1:-1] Out[1]: []

Ahh, TIL. Please disregard :)

Slices work differently to indexes. :)

gsnedders · 2018-08-10T16:42:52Z

(not merging because Travis keeps failing because of ongoing network issues :/)

Context: web-platform-tests/wpt#12381 .serviceworker. now implies .https.

The format itself hasn't changed, but incremental updating from the current version doesn't work after #12381. Fixes #12559.

Automatic update from web-platform-testsBump the manifest version to 5 (#12563) The format itself hasn't changed, but incremental updating from the current version doesn't work after web-platform-tests/wpt#12381. Fixes web-platform-tests/wpt#12559. -- wpt-commits: 052d3a71637062a4ecf7266aa138ccdc8ae2c983 wpt-pr: 12563

The format itself hasn't changed, but incremental updating from the current version doesn't work after #12381. Fixes #12559.

Mainly for the .serviceworker. change in tools/manifest/item.py, from web-platform-tests/wpt#12381 port/driver.py is updated accordingly to match upstream wptrunner to use HTTPS for .serviceworker. tests. This fixes an infrastructure/ test. Multiple other .serviceworker. tests are also rebaselined. There have been two recent manifest changes: web-platform-tests/wpt#12182 web-platform-tests/wpt#12563 (version bump) In order to not require a full manifest rebuild once this lands, WPT_BASE_MANIFEST.json needs to be updated. However, updating it in place has proven very difficult due to the size of the resulting diff. Instead, a WPT_BASE_MANIFEST_5.json is created. The old one will be deleted separately. Related Gerrit bug: https://bugs.chromium.org/p/gerrit/issues/detail?id=9640 This also brings in HTTP/2.0 support from web-platform-tests/wpt#12193. For this, new third_party python code is needed, namely h2, hpack, hyperframe, and enum. In addition to adding those to WPTWhiteList, it is also sorted and lines which aren't existing files (i.e., directories and removed files) are dropped. Reviewed at https://chromium-review.googlesource.com/1183425 but size of CL causing trouble updating it on Gerrit. [email protected] Bug: 876717 Change-Id: Ia43bd2eca54883a5d72a48008b6677d0e7187056 Reviewed-on: https://chromium-review.googlesource.com/1196424 Reviewed-by: Robert Ma <[email protected]> Reviewed-by: Philip Jägenstedt <[email protected]> Commit-Queue: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/master@{#588057}

Automatic update from web-platform-testsBump the manifest version to 5 (#12563) The format itself hasn't changed, but incremental updating from the current version doesn't work after web-platform-tests/wpt#12381. Fixes web-platform-tests/wpt#12559. -- wpt-commits: 052d3a71637062a4ecf7266aa138ccdc8ae2c983 wpt-pr: 12563 UltraBlame original commit: ef3bbe29c33d91e65e992657f56cc86dd223f749

wpt-pr-bot added infra manifest serve labels Aug 9, 2018

wpt-pr-bot requested a review from gsnedders August 9, 2018 15:42

jgraham requested a review from Hexcles August 9, 2018 15:43

jgraham force-pushed the serviceworker_https branch from 2b3f0e6 to 79c83a2 Compare August 10, 2018 10:04

wpt-pr-bot requested review from jugglinmike and kereliuk August 10, 2018 10:04

gsnedders reviewed Aug 10, 2018

View reviewed changes

jgraham force-pushed the serviceworker_https branch from 79c83a2 to 0a15b98 Compare August 10, 2018 12:36

Ms2ger reviewed Aug 10, 2018

View reviewed changes

jgraham force-pushed the serviceworker_https branch from 0a15b98 to 7c3c1cd Compare August 10, 2018 13:51

Hexcles reviewed Aug 10, 2018

View reviewed changes

gsnedders approved these changes Aug 10, 2018

View reviewed changes

jgraham merged commit bfec539 into master Aug 10, 2018

gsnedders deleted the serviceworker_https branch August 10, 2018 21:14

Hexcles mentioned this pull request Aug 12, 2018

Impossible to write .any.js test to be run by service workers with a secure origin #11105

Closed

This was referenced Aug 13, 2018

Fix accessing serviceworker tests for .https.any.js files. #11151

Closed

Update computeTestScheme to treat .serviceworker. as requiring HTTPS web-platform-tests/wpt.fyi#428

Closed

Hexcles added a commit to web-platform-tests/wpt.fyi that referenced this pull request Aug 14, 2018

Use HTTPS for .serviceworker. tests too

59de6a1

Context: web-platform-tests/wpt#12381 .serviceworker. now implies .https.

Hexcles mentioned this pull request Aug 14, 2018

Use HTTPS for .serviceworker. tests too web-platform-tests/wpt.fyi#437

Merged

Hexcles added a commit to web-platform-tests/wpt.fyi that referenced this pull request Aug 15, 2018

Use HTTPS for .serviceworker. tests too

85a4bc6

Context: web-platform-tests/wpt#12381 .serviceworker. now implies .https.

foolip mentioned this pull request Aug 18, 2018

Broken .https.any.serviceworker. for non-https .any.js remain in MANIFEST.json #12559

Closed

foolip added a commit that referenced this pull request Aug 19, 2018

Bump the manifest version to 5

a638ff8

The format itself hasn't changed, but incremental updating from the current version doesn't work after #12381. Fixes #12559.

foolip mentioned this pull request Aug 19, 2018

Bump the manifest version to 5 #12563

Merged

jgraham pushed a commit that referenced this pull request Aug 20, 2018

Bump the manifest version to 5 (#12563)

052d3a7

The format itself hasn't changed, but incremental updating from the current version doesn't work after #12381. Fixes #12559.

zcorpan pushed a commit that referenced this pull request Aug 27, 2018

Bump the manifest version to 5 (#12563)

1f8cb7e

The format itself hasn't changed, but incremental updating from the current version doesn't work after #12381. Fixes #12559.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix service worker tests generated from non-https .any.js #12381

Fix service worker tests generated from non-https .any.js #12381

jgraham commented Aug 9, 2018

foolip commented Aug 9, 2018

jgraham commented Aug 10, 2018

gsnedders left a comment

gsnedders Aug 10, 2018

Ms2ger Aug 10, 2018

Ms2ger commented Aug 10, 2018

Hexcles Aug 10, 2018

jgraham Aug 10, 2018

Hexcles Aug 10, 2018

gsnedders Aug 10, 2018

gsnedders commented Aug 10, 2018

		from ..sourcefile import SourceFile


		class TestManifestItem(ManifestItem):

Fix service worker tests generated from non-https .any.js #12381

Fix service worker tests generated from non-https .any.js #12381

Conversation

jgraham commented Aug 9, 2018

foolip commented Aug 9, 2018

jgraham commented Aug 10, 2018

gsnedders left a comment

Choose a reason for hiding this comment

gsnedders Aug 10, 2018

Choose a reason for hiding this comment

Ms2ger Aug 10, 2018

Choose a reason for hiding this comment

Ms2ger commented Aug 10, 2018

Hexcles Aug 10, 2018

Choose a reason for hiding this comment

jgraham Aug 10, 2018

Choose a reason for hiding this comment

Hexcles Aug 10, 2018

Choose a reason for hiding this comment

gsnedders Aug 10, 2018

Choose a reason for hiding this comment

gsnedders commented Aug 10, 2018