Closed Bug 1171917 Opened 5 years ago Closed 5 years ago

Improve about:serviceworkers tests on b2g

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
FxOS-S3 (24Jul)
Tracking Status
firefox42 --- fixed

People

(Reporter: ferjm, Assigned: jaoo)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Whiteboard: [s4]
Target Milestone: --- → NGA S3 (26Jun)
Assignee: nobody → ferjmoreno
Blocks: 1171915
Status: NEW → ASSIGNED
Whiteboard: [s4]
Assignee: ferjmoreno → jaoo
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
No longer blocks: 1171915
Depends on: 1171915
Bug 1171915 - about:serviceworkers in b2g should use originAttributes when calling ServiceWorkerManager. r=baku,fabrice
Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice
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
checkin-needed for the gaia bits here. Thanks!
Keywords: checkin-needed
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> checkin-needed for the gaia bits here. Thanks!

Wrong bug sorry!
Keywords: checkin-needed
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
Attachment #8626229 - Attachment description: MozReview Request: Bug 1171915 - about:serviceworkers in b2g should use originAttributes when calling ServiceWorkerManager. r=baku,fabrice → MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice
Attachment #8626230 - Attachment is obsolete: true
Target Milestone: FxOS-S1 (26Jun) → FxOS-S2 (10Jul)
See Also: → 1178233
Depends on: 1179161
Depends on: 1178233
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
Attachment #8626229 - Attachment description: MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=baku,fabrice → MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm
Attachment #8626229 - Flags: review?(ferjmoreno)
Attachment #8626229 - Attachment is obsolete: true
Attachment #8626229 - Flags: review?(ferjmoreno)
Attached patch v1 (obsolete) — Splinter Review
Fernando, could you provide some feedback on the patch please? Thanks!
Attachment #8632718 - Flags: feedback?(ferjmoreno)
Target Milestone: FxOS-S2 (10Jul) → FxOS-S3 (24Jul)
Blocks: 1181544
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.
Attachment #8632718 - Flags: feedback?(ferjmoreno) → feedback+
Attached patch v2Splinter Review
Feedback comments addressed (I hope all of them).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=052273e4a6d8
Attachment #8632718 - Attachment is obsolete: true
Attachment #8635204 - Flags: review?(ferjmoreno)
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.
Attachment #8635204 - Flags: review?(ferjmoreno) → review+
https://hg.mozilla.org/mozilla-central/rev/e20f0a1b7780
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 8626229 [details]
MozReview Request: Bug 1171917 - Improve about:serviceworkers tests on b2g. r=ferjm

https://reviewboard.mozilla.org/r/12003/#review133862
You need to log in before you can comment on or make changes to this bug.