Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServiceWorker: Add new WPT tests to make sure to update a registration with different script type and identical script content. #13755

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 29, 2018

These tests check that a registration is updated correctly with
different script type. At first Service Worker is registered as
classic script type, then it is re-registered as module script type,
and vice versa. A main script is identical.

Bug: 824647
Change-Id: I2a3f87da1013f84c6e9495f362899dfe6ab97b45
Reviewed-on: https://chromium-review.googlesource.com/c/1298822
Commit-Queue: Asami Doi <[email protected]>
Reviewed-by: Hiroki Nakagawa <[email protected]>
Reviewed-by: Matt Falkenhagen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#604551}


Edited by @foolip to include a follow-up fix:

ServiceWorker: Deflake update-with-script-type.https.html

To sheriff: Please re-disable the test if it's still flaky.

In this test file, some tests start re-registering the second service worker
before the first service worker gets activated. In my theory, this sometimes
squashes the second registration job into the first registration job, and
results in test flakiness. This CL makes sure the second register runs after
the first service worker gets activated.

Note that it would be the correct behavior that the second registration job
isn't squashed when the script type is changed. There is a spec issue about
this: https://github.com/w3c/ServiceWorker/issues/1358

Bug: 901317
Change-Id: I7ce379071a35ef9aeb98e4492d651ee6fc4714ec
Reviewed-on: https://chromium-review.googlesource.com/c/1328546
Reviewed-by: Makoto Shimazu <[email protected]>
Commit-Queue: Hiroki Nakagawa <[email protected]>
Cr-Commit-Position: refs/heads/master@{#607494}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1298822 branch 10 times, most recently from 5dbe4c2 to 8ba3f12 Compare November 1, 2018 10:03
with different script type and identical script content.

These tests check that a registration is updated correctly with
different script type. At first Service Worker is registered as
classic script type, then it is re-registered as module script type,
and vice versa. A main script is identical.

Bug: 824647
Change-Id: I2a3f87da1013f84c6e9495f362899dfe6ab97b45
Reviewed-on: https://chromium-review.googlesource.com/c/1298822
Commit-Queue: Asami Doi <[email protected]>
Reviewed-by: Hiroki Nakagawa <[email protected]>
Reviewed-by: Matt Falkenhagen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#604551}
@Hexcles
Copy link
Member

Hexcles commented Nov 6, 2018

@d0iasm the test seems to be flaky on Firefox. Can you take a look?

Test Subtest Results Messages
/service-workers/service-worker/update-registration-with-type.https.html Does not update the registration with the same script type and the same main script. FAIL: 2/10, PASS: 8/10 promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"

@rakuco
Copy link
Member

rakuco commented Nov 8, 2018

FTR, I also sent a ping in the Gerrit review a week ago, and the test's been marked as flaky in Blink: https://bugs.chromium.org/p/chromium/issues/detail?id=901317

@foolip
Copy link
Member

foolip commented Nov 13, 2018

A CL to deflake the test was landed in https://chromium-review.googlesource.com/c/1328546 and failure to export that CL is now blocking export. I'll cherry-pick it onto this branch and see if that fixes the problem here too.

To sheriff: Please re-disable the test if it's still flaky.

In this test file, some tests start re-registering the second service worker
before the first service worker gets activated. In my theory, this sometimes
squashes the second registration job into the first registration job, and
results in test flakiness. This CL makes sure the second register runs after
the first service worker gets activated.

Note that it would be the correct behavior that the second registration job
isn't squashed when the script type is changed. There is a spec issue about
this: w3c/ServiceWorker#1358

Bug: 901317
Change-Id: I7ce379071a35ef9aeb98e4492d651ee6fc4714ec
Reviewed-on: https://chromium-review.googlesource.com/c/1328546
Reviewed-by: Makoto Shimazu <[email protected]>
Commit-Queue: Hiroki Nakagawa <[email protected]>
Cr-Commit-Position: refs/heads/master@{#607494}
@foolip
Copy link
Member

foolip commented Nov 13, 2018

ci_stability on Travis is still things this is flaky in Firefox. I've tried to repro locally by reloading the test manually over and over without success, and also by running ./wpt run --install-browser --verify firefox service-workers/service-worker/update-registration-with-type.https.html without issue.

@jugglinmike, this is a PR that should come up in your scripts as having different stability results from Travis and Taskcluster.

I'll admin merge this now.

@foolip foolip merged commit 3571d11 into master Nov 13, 2018
@foolip foolip deleted the chromium-export-cl-1298822 branch November 13, 2018 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants