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

[service-workers] Refactor to use async cleanup #13165

Merged
merged 2 commits into from Oct 18, 2018

Conversation

Projects
None yet
3 participants
@jugglinmike
Contributor

jugglinmike commented Sep 21, 2018

Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The Test#add_cleanup method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using promise_test. Update tests which
were previously declared with async_test and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] #8748


This patch is not intended to influence test results. To verify that, I used
the WPT CLI to run the affected tests in Chromium and Firefox, comparing the
summary it produced both on master and on this branch.

Chromium on master:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 96 checks (32 tests, 64 subtests)
Expected results: 93
Unexpected results: 3
  subtest: 3 (3 fail)

Chromium with patch applied:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 96 checks (32 tests, 64 subtests)
Expected results: 93
Unexpected results: 3
  subtest: 3 (3 fail)

Firefox on master:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 96 checks (32 tests, 64 subtests)
Expected results: 89
Unexpected results: 7
  test: 1 (1 timeout)
  subtest: 6 (4 fail, 2 timeout)

Firefox with patch applied:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 96 checks (32 tests, 64 subtests)
Expected results: 89
Unexpected results: 7
  test: 1 (1 timeout)
  subtest: 6 (4 fail, 1 notrun, 1 timeout)

The discrepancy in Firefox is expected. In master, two of the four
async_tests in fetch-request-css-images.https.html time out in Firefox.
This branch refactors all four into promise_tests. promise_tests run in
series (unlike the parallel async_tests), so the harness stops running the
tests following the first timeout and reports the final test as "NOT RUN".

I've attempted to ease the review process by splitting the work across a few
pull requests, but this patch is still pretty onerous. I'd be happy to break it
up further if desired.

[service-workers] Refactor to use async cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] #8748
@mattto

mattto approved these changes Oct 12, 2018

Sorry for the review delay. I didn't have an effective system for receiving GitHub reviews in my inbox. This is great. Thank you for all your work adding async cleanup and for converting these tests.

This patch is fine as-is. One observation is we sometimes remove frames inline and sometimes in add_cleanup. I think add_cleanup is better to isolate promise_tests from each other. We've also tried to remove the frames before calling unregister, as it makes it more likely for the service worker to actually get deactivated, since otherwise it waits for all clients to close. But that's sometimes inconvenient to code.

wait_for_state(t, worker, 'installed')
.then(function() {

This comment has been minimized.

@mattto

mattto Oct 12, 2018

Member

ditto

wait_for_state(t, worker, 'redundant')
.then(function() {

This comment has been minimized.

@mattto

mattto Oct 12, 2018

Member

ditto

wait_for_state(t, worker, 'installed')
.then(function() {

This comment has been minimized.

@mattto

mattto Oct 12, 2018

Member

extra line

service_worker_unregister_and_done(t, scope);
};
if (event.target.state != 'activated')
return onStateChange(expectedTarget);

This comment has been minimized.

@mattto

mattto Oct 12, 2018

Member

Hum this is felt too clever for me but it seems to work. Also I'm not sure why this test terminates at 'activated' while having checkStateTransition for activated and redundant.

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Oct 15, 2018

Happy to help out! Would you mind merging on my behalf, @mattto?

I noticed some inconsistency with iframe management while writing this patch. I'd be happy to normalize things a bit in a follow up.

@mattto mattto merged commit 1569ae0 into web-platform-tests:master Oct 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattto

This comment has been minimized.

Member

mattto commented Oct 18, 2018

Thanks! Sorry for the delay. I didn't know I could merge things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment