Closed Bug 1237363 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files, 6 obsolete files)

We want to do this to avoid future occurrences of bug 1237158.
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.
Attachment #8704758 - Flags: review?(josh) → review+
Doesn't this need a negative test?
(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> Doesn't this need a negative test?

What's a negative test?
Flags: needinfo?(martijn.martijn)
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?
Attachment #8704879 - Flags: review?(josh) → review+
(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.
Flags: needinfo?(martijn.martijn)
(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 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(), ...)
Attachment #8705842 - Flags: review?(josh) → review+
Attached patch Also fix test_app_protocol (obsolete) — Splinter Review
Attachment #8706477 - Flags: review?(josh)
(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.  :-)
Attachment #8706477 - Flags: review?(josh) → review+
Attached patch More test fixes (obsolete) — Splinter Review
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.
Attachment #8706588 - Flags: review?(josh)
Attachment #8706588 - Flags: review?(josh) → review+
Depends on: 1238954
The file_child-src_service_worker.html changes uncovered bug 1238954.  I'll land the rest of the code changes for now.
Keywords: leave-open
Attached patch Part 1.1: More test fixes (obsolete) — Splinter Review
Attachment #8704758 - Attachment is obsolete: true
Attachment #8704879 - Attachment is obsolete: true
Attachment #8705842 - Attachment is obsolete: true
Attachment #8706477 - Attachment is obsolete: true
Attachment #8706588 - Attachment is obsolete: true
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.
Attachment #8717622 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.