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

Deflake WPT test registration-schedule-job.https.html #27796

Merged
merged 1 commit into from Mar 1, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Feb 26, 2021

Make it so register jobs are not coalesced if they have
different updateViaCache properties, as per spec:
https://w3c.github.io/ServiceWorker/#dfn-job-equivalent

This makes the test about updateViaCache pass. The test was also
previously flaky in Chromium because it failed in two different ways
after coalescing the jobs: either registration.installing is null or
it's non-null and has the wrong updateViaCache property.

Other browsers also seemed to fail the test because they do not
make a new job and were possibly flaky. This CL also rearranges the
test to help browsers fail in a more stable way by accounting for
registration.installing being null. However it can still be flaky if
one register job is able to finish before the other one is queued up,
so it doesn't have a chance to coalesced.

Bug: 979593
Change-Id: I5395019d5733a6365b0b0ae2ae6051a077fb8176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2721300
Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858476}

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.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2721300 branch 2 times, most recently from 15c8678 to af0a965 Compare February 26, 2021 13:20
@mfalken
Copy link
Member

mfalken commented Feb 26, 2021

This fails on the stability bots but I think this test is inherently flaky for browsers that don't pass the test. Chromium was failing it because it was coalescing the two register jobs:

register(...);
await register(...);
registration.installing;  // this would sometimes be null or not depending on how far the job got

You can see here too all browsers are currently failing and in different ways:
https://wpt.fyi/results/service-workers/service-worker/registration-schedule-job.https.html?label=master&label=experimental&aligned&q=service-workers%2Fservice-worker%2Fregistration-schedule-job.https.html

As the commit description says, this PR updates the test to be a little more stable for failing browsers but is still not 100% stable.

cc @asutherland @wanderview

Make it so register jobs are not coalesced if they have
different updateViaCache properties, as per spec:
https://w3c.github.io/ServiceWorker/#dfn-job-equivalent

This makes the test about updateViaCache pass. The test was also
previously flaky in Chromium because it failed in two different ways
after coalescing the jobs: either registration.installing is null or
it's non-null and has the wrong updateViaCache property.

Other browsers also seemed to fail the test because they do not
make a new job and were possibly flaky. This CL also rearranges the
test to help browsers fail in a more stable way by accounting for
registration.installing being null. However it can still be flaky if
one register job is able to finish before the other one is queued up,
so it doesn't have a chance to coalesced.

Bug: 979593
Change-Id: I5395019d5733a6365b0b0ae2ae6051a077fb8176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2721300
Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858476}
@stephenmcgruer
Copy link
Contributor

Ack, thanks for the note. Admin-merging.

@stephenmcgruer stephenmcgruer merged commit 980c618 into master Mar 1, 2021
@stephenmcgruer stephenmcgruer deleted the chromium-export-cl-2721300 branch March 1, 2021 14:37
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.

None yet

4 participants