Closed
Bug 1450073
Opened 7 years ago
Closed 6 years ago
Application panel: add tests for the application panel
Categories
(DevTools :: Application Panel, enhancement, P3)
DevTools
Application Panel
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
Attachments
(2 files)
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 5•6 years ago
|
||
mozreview-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).
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 11•6 years ago
|
||
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 12•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
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!
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
bugherder |
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
Comment 18•6 years ago
|
||
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•