[service-workers] Use asynchronous cleanup #13164

jugglinmike · 2018-09-21T22:03:54Z

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]. Use that 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 145 checks (32 tests, 113 subtests)
Expected results: 120
Unexpected results: 25
  test: 2 (2 timeout)
  subtest: 23 (15 fail, 6 notrun, 2 timeout)

Chromium with patch applied:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 145 checks (32 tests, 113 subtests)
Expected results: 120
Unexpected results: 25
  test: 2 (2 timeout)
  subtest: 23 (15 fail, 6 notrun, 2 timeout)

Firefox on master:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 145 checks (32 tests, 113 subtests)
Expected results: 134
Unexpected results: 11
  test: 2 (2 timeout)
  subtest: 9 (6 fail, 1 notrun, 2 timeout)

Firefox with patch applied:

 web-platform-test
 ~~~~~~~~~~~~~~~~~
 Ran 145 checks (32 tests, 113 subtests)
 Expected results: 134
 Unexpected results: 11
   test: 2 (2 timeout)
   subtest: 9 (6 fail, 1 notrun, 2 timeout)

mkruisselbrink

I think this looks good, modulo some nits and the thing I raised on IRC where it appears that using service_worker_unregister() from a cleanup function will result in any failures during that unregister to be silently ignored (as rather than returning a rejected promise service_worker_unregister() runs some code inside a t.step() to handle errors). Not sure how bad it is if those are ignored though, so it might be okay to leave it like this until somebody has time for more extensive cleanups of this code?

mkruisselbrink · 2018-09-21T22:22:07Z

service-workers/service-worker/clients-matchall-order.https.html

-  }).then(_ => {
-    return service_worker_unregister_and_done(t, opts.scope);
+
+    t.done();


is there actually a need to call t.done()? It always seems suspicious to me if a promise_test calls done(), since the promise resolving itself should have that same effect?

You're right: that's superfluous.

mkruisselbrink · 2018-09-21T22:52:04Z

service-workers/service-worker/claim-fetch.https.html

@@ -32,9 +32,13 @@

    // Register a service worker.
    .then(() => service_worker_unregister_and_register(t, script, scope))
-    .then(r => worker = r.installing)
-    .then(() => wait_for_state(t, worker, 'activated'))
+    .then((r) => {


nit: not sure why you're adding () around r? it seems more consistent with the rest of the file to not add those.

Yup, my personal style leaked through while re-writing these callbacks. Should be fixed, now.

mkruisselbrink · 2018-09-21T22:52:29Z

service-workers/service-worker/claim-shared-worker-fetch.https.html

@@ -30,9 +30,13 @@
                                         'fetch() should not be intercepted.'))
    // Register a service worker.
    .then(() => service_worker_unregister_and_register(t, script, scope))
-    .then(r => worker = r.installing)
-    .then(() => wait_for_state(t, worker, 'activated'))
+    .then((r) => {


nit: here too, no () for consistency with rest of file

mkruisselbrink · 2018-09-21T22:52:59Z

service-workers/service-worker/performance-timeline.https.html

@@ -17,7 +17,11 @@
  let slowURL = url + '&slow';
  let frame;
  return service_worker_unregister_and_register(t, script, scope)
-    .then(reg => wait_for_state(t, reg.installing, 'activated'))
+    .then((reg) => {


nit: here too no ()

mkruisselbrink · 2018-09-21T22:57:50Z

(the test that is flaky in chrome that is causing travis to fail is https://crbug.com/831509)

jugglinmike · 2018-09-22T01:07:14Z

Thanks for the review, @mkruisselbrink. Regarding cleanup failures: I wouldn't mind extending this patch to trigger harness errors. To me, it's more a question of how much change we should tolerate in one patch.

We could define a new function that does everything service_worker_unregister does up until the t.step invocation. We should probably keep service_worker_unregister as is, but that makes this new function hard to name. service_worker_unregister_simple is the best I have right now. Pretty dopey, right?

We could overload service_worker_unregister:

 function service_worker_unregister(test, scope) {
   var absoluteScope = (new URL(scope, window.location).href);
+  var fail = test === null ?
+    null : unreached_rejection(test, 'unregister should not fail')
   return navigator.serviceWorker.getRegistration(scope)
     .then(function(registration) {
         if (registration && registration.scope === absoluteScope)
           return registration.unregister();
       })
-    .catch(unreached_rejection(test, 'unregister should not fail'));
+    .catch(fail);
 }

That's an awkward API, but intentionally so--we wouldn't want folks accidentally opting in to this behavior.

My favorite option is more drastic. I've had my eyes on removing service_worker_unregister_and_register because as far as I can tell, it's obviated by proper cleanup. I've held off on that for these patches because they're large enough as it is. If we wanted to start that here, we could define a replacement helper that would consolidate registration and cleanup:

function service_worker_hygienic_register(test, url, scope) {
  if (!scope)
    return Promise.reject(new Error('tests must define a scope'));

  var options = { scope: scope };
  return navigator.serviceWorker.register(url, options)
    .then(function(registration) {
      test.add_cleanup(function() {
          return registration.unregister();
        });

      return registration;
    });
}

mkruisselbrink · 2018-09-22T02:33:34Z

I think I'd prefer not to have too many unrelated changes in one PR, so probably not the change to service_worker_hygienic_register (as an aside, even if proper cleanup should alleviate the need for an unregister before register, it might still be a good idea to keep that unregister anyway, just in case... At least personally I'm a fan of tests of course cleaning up everything they do, but still also being somewhat resistant to other tests not doing so where it doesn't complicate stuff too much).

I'd be fine with just landing this as is, or something like that overload your suggesting/a separate method (unregister_service_worker? or unregister_service_worker_by_scope?)

jugglinmike · 2018-10-15T20:49:34Z

@mattto Do you have a preference between the options @mkruisselbrink has selected?

mfalken · 2018-10-18T08:15:32Z

Not totally sure I've followed everything in the thread, but I'd say land this as-is and then add service_worker_hygienic_register (just call it service_worker_register?) in another patch. I'd also prefer to bundle unregister at the beginning of service_worker_register just to make good tests resilient to bad tests.

BTW we can probably remove that "tests must define a scope" thing. I added that back when everything was an async_test. You had to give each test a unique scope so the tests didn't stomp on each other, so that warning helped ensure you added a scope.

jugglinmike · 2018-11-15T19:17:40Z

@mattto reminder that you can merge things :)

Also, the instability that TravisCI has reported is unrelated--see gh-14083

mfalken

I'm very sorry for the delay. I think I finally have a system that filters the "review requested" mails to a folder that I'll notice.

Unfortunately I can't merge on the web UI due to the CI error, so I'm looking into doing this by the command line.

mfalken · 2018-12-03T08:22:42Z

I couldn't figure out the command line but I've heard Travis stability is no longer blocking, so I've restarted the Travis check.

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]. Use that API to schedule service worker un-registration so that it occurs regardless of the result of each test. [1] web-platform-tests#8748

jugglinmike · 2018-12-03T18:34:48Z

You heard right, but this branch predated the change which allowed instability. I've rebased to incorporate that change, so this pull request is no longer blocked by unrelated instability (though the instability is still present).

The prior version of this branch is available on Bocoup's fork of wpt:

master...bocoup:service-workers-async-cleanup-use-orig

mfalken · 2018-12-04T00:25:30Z

Thanks for the patch and for rebasing!

jugglinmike requested a review from mkruisselbrink September 21, 2018 22:03

wpt-pr-bot added service-workers wg-sw labels Sep 21, 2018

wpt-pr-bot requested review from asutherland, mfalken and wanderview September 21, 2018 22:04

mkruisselbrink approved these changes Sep 21, 2018

View reviewed changes

wanghongjuan mentioned this pull request Sep 25, 2018

service workers: Simplified tests with async/await - Part 1 #13103

Open

jugglinmike mentioned this pull request Nov 15, 2018

[service-workers] performance-timeline.https.html is flaky in Firefox #14083

Open

mfalken approved these changes Dec 3, 2018

View reviewed changes

wpt-pr-bot assigned mkruisselbrink Dec 3, 2018

jugglinmike force-pushed the service-workers-async-cleanup-use branch from d9ab513 to e065b16 Compare December 3, 2018 18:25

mfalken merged commit 1abcb70 into web-platform-tests:master Dec 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[service-workers] Use asynchronous cleanup #13164

[service-workers] Use asynchronous cleanup #13164

jugglinmike commented Sep 21, 2018

mkruisselbrink left a comment

mkruisselbrink Sep 21, 2018

jugglinmike Sep 22, 2018

mkruisselbrink Sep 21, 2018

jugglinmike Sep 22, 2018

mkruisselbrink Sep 21, 2018

jugglinmike Sep 22, 2018

mkruisselbrink Sep 21, 2018

jugglinmike Sep 22, 2018

mkruisselbrink commented Sep 21, 2018

jugglinmike commented Sep 22, 2018

mkruisselbrink commented Sep 22, 2018

jugglinmike commented Oct 15, 2018

mfalken commented Oct 18, 2018

jugglinmike commented Nov 15, 2018

mfalken left a comment

mfalken commented Dec 3, 2018

jugglinmike commented Dec 3, 2018

mfalken commented Dec 4, 2018

[service-workers] Use asynchronous cleanup #13164

[service-workers] Use asynchronous cleanup #13164

Conversation

jugglinmike commented Sep 21, 2018

mkruisselbrink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkruisselbrink commented Sep 21, 2018

jugglinmike commented Sep 22, 2018

mkruisselbrink commented Sep 22, 2018

jugglinmike commented Oct 15, 2018

mfalken commented Oct 18, 2018

jugglinmike commented Nov 15, 2018

mfalken left a comment

Choose a reason for hiding this comment

mfalken commented Dec 3, 2018

jugglinmike commented Dec 3, 2018

mfalken commented Dec 4, 2018