1237363 - Fail a mochitest if it registers a service worker and forgets to unregister it

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(no longer active)]

We want to do this to avoid future occurrences of bug 1237158.

Comment 1

[(no longer active)]

Attachment: Fail mochitests which register a service worker without unregistering it

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8704758 [details] [diff] [review]
Fail mochitests which register a service worker without unregistering it

Review of attachment 8704758 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 3

[(no longer active)]

Attachment: Part 0: Unregister all service workers registered in mochitests at the end of the test

Comment 4

[Martijn Wargers (dead)]

Doesn't this need a negative test?

Comment 5

[(no longer active)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> Doesn't this need a negative test?

What's a negative test?

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8704879 [details] [diff] [review]
Part 0: Unregister all service workers registered in mochitests at the end of the test

Review of attachment 8704879 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/serviceworkerregistrationinfo_iframe.html
@@ +17,5 @@
> +                  reg = registration;
> +                });
> +            });
> +          } else if (event.data === "unregister") {
> +            reg.unregister();

This doesn't appear to unregister the first registration.

::: dom/workers/test/serviceworkers/test_notification_get.html
@@ +98,5 @@
>          is(notifications.length, 0, "There should be no more stored notifications");
>        }).catch(function(e) {
>          ok(false, "Something went wrong " + e.message);
> +      }).then(function() {
> +        return unregisterAlternateSWAndAddNotification();

This can be `then(unregisterAlternateSWAndAddNotification)`, right?

Comment 7

[Josh Matthews [:jdm]]

(In reply to :Ehsan Akhgari from comment #5)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> > Doesn't this need a negative test?
> 
> What's a negative test?

A test that only passes if a registered SW is reported at the end of a test that doesn't unregister it.

Comment 8

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #6)
> Comment on attachment 8704879 [details] [diff] [review]
> Part 0: Unregister all service workers registered in mochitests at the end
> of the test
> 
> Review of attachment 8704879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/test/serviceworkers/serviceworkerregistrationinfo_iframe.html
> @@ +17,5 @@
> > +                  reg = registration;
> > +                });
> > +            });
> > +          } else if (event.data === "unregister") {
> > +            reg.unregister();
> 
> This doesn't appear to unregister the first registration.

There is only one registration per scope.

> ::: dom/workers/test/serviceworkers/test_notification_get.html
> @@ +98,5 @@
> >          is(notifications.length, 0, "There should be no more stored notifications");
> >        }).catch(function(e) {
> >          ok(false, "Something went wrong " + e.message);
> > +      }).then(function() {
> > +        return unregisterAlternateSWAndAddNotification();
> 
> This can be `then(unregisterAlternateSWAndAddNotification)`, right?

Yes, will fix.

Comment 9

[(no longer active)]

Attachment: Part 3: Add a test for a mochitest finishing without unregistering its service worker

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8705842 [details] [diff] [review]
Part 3: Add a test for a mochitest finishing without unregistering its service worker

Review of attachment 8705842 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/Harness_sanity/test_sanityRegisteredServiceWorker.html
@@ +15,5 @@
> +]}, function() {
> +  navigator.serviceWorker.register("empty.js", {scope: "scope"})
> +    .then(function(registration) {
> +      ok(registration, "Registration succeeded");
> +      SimpleTest.expectRegisteredServiceWorker();

Move this out so the test fails if the registration fails for some reason, which will be a bit more informative than a generic test timeout.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1049,5 @@
>                                 + "SimpleTest.waitForExplicitFinish() if you need "
>                                 + "it.)");
>          }
> +        if (SimpleTest._expectingRegisteredServiceWorker) {
> +            if (!SpecialPowers.isServiceWorkerRegistered()) {

SimpleTest.ok(SpecialPowers.isServiceWorkerRegistered(), ...)

@@ +1053,5 @@
> +            if (!SpecialPowers.isServiceWorkerRegistered()) {
> +                SimpleTest.ok(false, "This test is expected to leave a service worker registered");
> +            }
> +        } else {
> +            if (SpecialPowers.isServiceWorkerRegistered()) {

SimpleTest.ok(!SpecialPowers.isServiceWorkerRegistered(), ...)

Comment 11

[(no longer active)]

Attachment: Also fix test_app_protocol

Comment 12

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #10)
> ::: testing/mochitest/tests/SimpleTest/SimpleTest.js
> @@ +1049,5 @@
> >                                 + "SimpleTest.waitForExplicitFinish() if you need "
> >                                 + "it.)");
> >          }
> > +        if (SimpleTest._expectingRegisteredServiceWorker) {
> > +            if (!SpecialPowers.isServiceWorkerRegistered()) {
> 
> SimpleTest.ok(SpecialPowers.isServiceWorkerRegistered(), ...)
> 
> @@ +1053,5 @@
> > +            if (!SpecialPowers.isServiceWorkerRegistered()) {
> > +                SimpleTest.ok(false, "This test is expected to leave a service worker registered");
> > +            }
> > +        } else {
> > +            if (SpecialPowers.isServiceWorkerRegistered()) {
> 
> SimpleTest.ok(!SpecialPowers.isServiceWorkerRegistered(), ...)

Can't do, since doing an ok(true) after the test finishes results in a "result logged after test finish" test failure.  :-)

Comment 13

[(no longer active)]

Attachment: More test fixes

This fails intermittently on e10s for reasons I cannot explain, and I have already invested way too much time into this.  I will probably land my test fixes in this bug and leave the rest for someone else to figure out.

Comment 14

[(no longer active)]

The file_child-src_service_worker.html changes uncovered bug 1238954.  I'll land the rest of the code changes for now.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6653f5a698

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec6653f5a698

Comment 17

[(no longer active)]

Attachment: Part 1.1: More test fixes

Comment 18

[(no longer active)]

Attachment: Part 2: Fail mochitests which register a service worker without unregistering it

Comment 19

[(no longer active)]

Attachment: Part 3: Add a test for a mochitest finishing without unregistering its service worker

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=767a3859e3ab

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8717622 [details] [diff] [review]
Part 1.1: More test fixes

I'm landing this patch with some more modifications over in bug 1238954.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Try looks reasonably green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=767a3859e3ab

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dab10c8a185a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aef995cc7c1

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dab10c8a185a
https://hg.mozilla.org/mozilla-central/rev/5aef995cc7c1