1171917 - Improve about:serviceworkers tests on b2g

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Right now we are only testing the ServiceWorkerManager interface. That is not enough, we need to test the whole functionality to catch regressions like the ones caused by bug 1162088

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

Bug 1171915 - about:serviceworkers in b2g should use originAttributes when calling ServiceWorkerManager. r=baku,fabrice

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Attachment: MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice

Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8626230 [details]
MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice

Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

checkin-needed for the gaia bits here. Thanks!

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> checkin-needed for the gaia bits here. Thanks!

Wrong bug sorry!

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8626229 [details]
MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8626229 [details]
MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Fernando, could you provide some feedback on the patch please? Thanks!

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Try results for these tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68fb0e6015b4

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8632718 [details] [diff] [review]
v1

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

Thank you for working on this :) LGTM in general.

The tests could use some comments explaining its overall purpose and some of the specifics.

Also, use single or double quotes but not a mix of them, please.

::: b2g/components/test/mochitest/app/client.html
@@ +4,5 @@
> +    <title>Test app for bug 1171917</title>
> +    <script type='application/javascript;version=1.7'>
> +
> +function onLoad() {
> +  navigator.serviceWorker.ready.then(function(swr) {

swr is not used

nit: use fat arrow here and whenever is possible in the rest of files, please

::: b2g/components/test/mochitest/app/index.html
@@ +20,5 @@
> +  alert('DONE');
> +}
> +
> +function testFrame(src) {
> +  return new Promise(function(resolve, reject) {

reject is unused

@@ +27,5 @@
> +    window.onmessage = function(e) {
> +      if (e.data.status == "callback") {
> +        window.onmessage = null;
> +        var result = e.data.data;
> +        iframe.src = "about:blank";

Why are you setting about:blank here?

@@ +59,5 @@
> +        });
> +      });
> +    })
> +    .then(function() {
> +      // Wait until the service worker start controlling the client.

starts

@@ +63,5 @@
> +      // Wait until the service worker start controlling the client.
> +      return testFrame('client.html');
> +    })
> +    .then(function() {
> +      return new Promise((resolve, reject) => {

reject is unused

@@ +75,5 @@
> +        });
> +        update();
> +      });
> +    })
> +    .then(done);

.catch(// dump error), please

Otherwise, any thrown errors will be swallowed, and you won't even see them in the console. Which means painful debugging times.

::: b2g/components/test/mochitest/test_aboutserviceworkers.html
@@ +12,5 @@
> +
> +const ASW_CHROME_EVENT = 'mozAboutServiceWorkersChromeEvent';
> +const ASW_CONTENT_EVENT = 'mozAboutServiceWorkersContentEvent';
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Cc and Ci are not used

@@ +15,5 @@
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "gAppsService",

You are not using gAppsService anywhere

@@ +40,5 @@
> +  window.addEventListener(ASW_CONTENT_EVENT, AboutServiceWorkers);
> +}
> +
> +function mockSendResult(aId, aResult) {
> +  debug("mockSendResult, " + JSON.stringify({ detail : {

nit: extra space before ":"

@@ +44,5 @@
> +  debug("mockSendResult, " + JSON.stringify({ detail : {
> +    id: aId,
> +    result: aResult
> +  }}));
> +  let event = new CustomEvent(ASW_CHROME_EVENT, { detail : {

You can build the event object once before the debug statement and reuse it for "debug" and "CustomEvent"

@@ +51,5 @@
> +  }});
> +  window.dispatchEvent(event);
> +}
> +
> +function mockSendError(aId, aError) {

Same comment about reusing the event object and the extra space before ":"

@@ +54,5 @@
> +
> +function mockSendError(aId, aError) {
> +  debug("mockSendError, " + JSON.stringify({ detail : {
> +    id: aId,
> +    result: aError

This must be "error" instead of "result"

https://mxr.mozilla.org/mozilla-central/source/b2g/components/AboutServiceWorkers.jsm#77

@@ +194,5 @@
> +function setup() {
> +  info('Setting up');
> +  return new Promise((resolve, reject) => {
> +    SpecialPowers.setAllAppsLaunchable(true);
> +    SpecialPowers.pushPrefEnv({'set': [

This is supposed to be a chrome test. I don't think you need to use SpecialPowers here. But it's ok if you prefer to do so.

@@ +242,5 @@
> +      ok(result.registrations && result.registrations.length === 0,
> +         "Service worker registration was successfuly unregistered");
> +    })
> +    .then(uninstallApp)
> +    .then(restoreMocks)

Create a function like

function finish() {
  restoreMocks();
  SimpleTest.finish();
}

and call it here and inside the .catch handler. This way we make sure that we always restore the mocks even when there's an unexpected error.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Feedback comments addressed (I hope all of them).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=052273e4a6d8

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8635204 [details] [diff] [review]
v2

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

LGTM. I still see a lack of comments about the overall flow and things like the interaction with the sjs. It would be nice if you could add some comments before landing. Thanks!

::: b2g/components/AboutServiceWorkers.jsm
@@ +18,5 @@
>                                    "@mozilla.org/serviceworkers/manager;1",
>                                    "nsIServiceWorkerManager");
>  
>  function debug(aMsg) {
> +  dump("AboutServiceWorkers - " + aMsg + "\n");

Comment this line before landing, please.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e20f0a1b7780

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Last try results before pushing this at:

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8626229 [details]
MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

https://reviewboard.mozilla.org/r/12003/#review133862