Closed Bug 1450073 Opened 6 years ago Closed 6 years ago

Application panel: add tests for the application panel

Categories

(DevTools :: Application Panel, enhancement, P3)

Product:
DevTools
Component:
Application Panel
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1450073 - Add integration tests for the application panel;
(deleted), text/x-review-board-request
ladybenko
: review+
nchevobbe
: review+
Details
Bug 1450073 - Update listAllWorkers try/catch to be resilient if listWorkers failed;
(deleted), text/x-review-board-request
ochameau
: review+
Details 
I would like to focus on the implementation in Bug 1445197 and land early so that we can iterate. For this reason I would like to have the testing handled in a follow up. I expect to work on this right after landing the initial app panel bug.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8968338 [details]
Bug 1450073 - Add integration tests for the application panel;

https://reviewboard.mozilla.org/r/237000/#review245680

The tests look nice to me. I only have a couple of nits and question, but this is ready to go in my opinion

::: devtools/client/application/test/browser_application_panel_list-several-workers.js:15
(Diff revision 3)
> +  await enableApplicationPanel();
> +  await enableServiceWorkerDebugging();

I guess we are going to do those for every test of this panel ? Maybe we could move them in head.js so they are executed before each test ?

::: devtools/client/application/test/head.js:19
(Diff revision 3)
> +  let options = { "set": [
> +    // Enable service workers.
> +    ["dom.serviceWorkers.enabled", true],
> +    // Accept workers from mochitest's http.
> +    ["dom.serviceWorkers.testing.enabled", true],
> +    // Force single content process.
> +    ["dom.ipc.processCount", 1],
> +  ]};
> +
> +  // Wait for dom.ipc.processCount to be updated before releasing processes.
> +  await new Promise(done => {
> +    SpecialPowers.pushPrefEnv(options, done);
> +  });

not sure about this, but could 3 separate pushPref calls be more readable ?

```js
// Enable service workers.
await pushPref("dom.serviceWorkers.enabled", true);
// Enable service workers.
await pushPref("dom.serviceWorkers.testing.enabled", true);
// Force single content process.
await pushPref("dom.serviceWorkers.testing.enabled", 1);
```

it introduces delay since we wait for each preference to be set, so I don't feel strongly about this.

::: devtools/client/application/test/head.js:24
(Diff revision 3)
> +    // Force single content process.
> +    ["dom.ipc.processCount", 1],

could you elaborate a bit more why this is needed ? is there a bug we can track so we don't have to do this anymore ?

::: devtools/client/application/test/service-workers/dynamic-registration.html:1
(Diff revision 3)
> +<!DOCTYPE HTML>

I don't remember if we *have to* put license headers in support file (in console, some have it, some don't)
Attachment #8968338 - Flags: review?(nchevobbe) → review+
Comment on attachment 8968338 [details]
Bug 1450073 - Add integration tests for the application panel;

https://reviewboard.mozilla.org/r/236998/#review246114

Thanks! Besides the unregister workers issue we already talked about, I only have minor comments.

::: devtools/client/application/test/browser_application_panel_list-several-workers.js:22
(Diff revision 3)
> +
> +  let { panel, target } = await openNewTabAndApplicationPanel(SIMPLE_URL);
> +  let doc = panel.panelWin.document;
> +
> +  info("Wait until the service worker appears in the application panel");
> +  await waitUntil(() => getWorkerContainers(doc).length == 1);

Question: shouldn't we prefer `===` instead of `==` for equality?

::: devtools/client/application/test/browser_application_panel_list-several-workers.js:33
(Diff revision 3)
> +
> +  info("Navigate to another page for the same domain with another service worker");
> +  await navigate(target, OTHER_SCOPE_URL);
> +
> +  info("Wait until the service worker appears in the application panel");
> +  await waitUntil(() => getWorkerContainers(doc).length == 2);

Same concern as before (`==` vs `===`)

::: devtools/client/application/test/browser_application_panel_list-single-worker.js:38
(Diff revision 3)
> +  });
> +
> +  info("Wait until the service worker is removed from the application panel");
> +  await waitUntil(() => getWorkerContainers(doc).length == 0);
> +});
> +

Maybe we should also test the info displayed for the service worker, like the url? (vs instead of just checking that there is a unregister button).
Thank you both for the reviews! The last patch should address all the comments raised here.

It seems that an unrelated change makes both about:debugging and the application panel fail if a service worker is registered with a scope different from the default. This was not covered by about:debugging tests, but is making one of my new tests added here fail. Currently bisecting.
Comment on attachment 8968338 [details]
Bug 1450073 - Add integration tests for the application panel;

https://reviewboard.mozilla.org/r/237000/#review245674

::: devtools/client/application/test/.eslintrc.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Question: why do we have an `eslintrc` file here, but not at the client itself? Shouldn't we have one (or none) at both places?

::: devtools/client/application/test/browser_application_panel_list-several-workers.js:1
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.

Why are we using different licenses across files? (is it defined  which license we should use where?)
Attachment #8968338 - Flags: review?(balbeza) → review+
Thanks for the review!

(In reply to Belén [:ladybenko] from comment #10)
> Comment on attachment 8968338 [details]
> Bug 1450073 - Add integration tests for the application panel;
> 
> https://reviewboard.mozilla.org/r/237000/#review245674
> 
> ::: devtools/client/application/test/.eslintrc.js:1
> (Diff revision 3)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Question: why do we have an `eslintrc` file here, but not at the client
> itself? Shouldn't we have one (or none) at both places?
> 

We have a root eslintrc file at `devtools/.eslintrc.js`. We also have one in `devtools/client/.eslintrc.js`. Tests use a common set of eslint rules, defined at `devtools/.eslintrc.mochitests.js`, which we inherit here. Not sure this answers your question though :/

> :::
> devtools/client/application/test/browser_application_panel_list-several-
> workers.js:1
> (Diff revision 3)
> > +/* Any copyright is dedicated to the Public Domain.
> 
> Why are we using different licenses across files? (is it defined  which
> license we should use where?)

The policy is defined at https://www.mozilla.org/en-US/MPL/license-policy/, with a flowchart at https://www.mozilla.org/media/img/mozorg/mpl/license-policy-flowchart.3d01662c2c7d.png

In practice we use MPL 2.0 for product code and Public Domain for test code.
Comment on attachment 8971697 [details]
Bug 1450073 - Update listAllWorkers try/catch to be resilient if listWorkers failed;

https://reviewboard.mozilla.org/r/240464/#review246962

Do you know which precise RDP request is throwing here?
This try/catch is quite large, still. If there is one explicit error from one particular request it would be better to catch only this one.
If this is client disconnection, closing in between two requests, it looks more like a test isn't waiting correctly before closing the toolbox and the exception would help knowing about.

Anyway, the change looks good given the current state of the code. So feel free to proceed here.
Attachment #8971697 - Flags: review?(poirot.alex) → review+
Comment on attachment 8968338 [details]
Bug 1450073 - Add integration tests for the application panel;

https://reviewboard.mozilla.org/r/237000/#review245680

> could you elaborate a bit more why this is needed ? is there a bug we can track so we don't have to do this anymore ?

This limitation is linked to the multi e10s service worker rewrite, added a comment.

> I don't remember if we *have to* put license headers in support file (in console, some have it, some don't)

In general I think we should have license headers everywhere. Added it here, thanks!
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Comment on attachment 8971697 [details]
> Bug 1450073 - Update listAllWorkers try/catch to be resilient if listWorkers
> failed;
> 
> https://reviewboard.mozilla.org/r/240464/#review246962
> 
> Do you know which precise RDP request is throwing here?
> This try/catch is quite large, still. If there is one explicit error from
> one particular request it would be better to catch only this one.
> If this is client disconnection, closing in between two requests, it looks
> more like a test isn't waiting correctly before closing the toolbox and the
> exception would help knowing about.
> 
> Anyway, the change looks good given the current state of the code. So feel
> free to proceed here.

Thanks! The full exception is:

Console message: [JavaScript Error: "error occurred while processing 'listWorkers: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIServiceWorkerManager.getRegistrationByPrincipal]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/worker.js :: _getServiceWorkerRegistrationInfo :: line 157"  data: no]
Stack: _getServiceWorkerRegistrationInfo@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/worker.js:157:12
form@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/worker.js:53:26
onListWorkers/</<.workers<@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/child-process.js:133:40
onListWorkers/<@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/child-process.js:133:20
promise callback*onListWorkers@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/child-pro" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js" line: 1616}]

This issue is actually not making the tests fail, they eventually succeed after waiting for some time. I will log a follow up to investigate this.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00e3fcb67636
Update listAllWorkers try/catch to be resilient if listWorkers failed;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/edf921dc7222
Add integration tests for the application panel;r=ladybenko,nchevobbe
Blocks: 1450064
https://hg.mozilla.org/mozilla-central/rev/00e3fcb67636
https://hg.mozilla.org/mozilla-central/rev/edf921dc7222
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: