For now we do have tests against addon install/uninstall and the state of debug button, but we don't check that the toolbox actually works. As for the Browser Toolbox, we should have at least one simple test. It should be possible by using same trick we use for the browser toolbox. We need a trick as the toolbox lives in another process...

We may be able to test that somewhat reasonably. But we would need to tweak the way we pass test script to the browser toolbox. The test script testing addons on about:debugging can't pass custom option parameters to BrowserToolboxProcess constructor. I used environment variable instead, so that the test script can define it easily. But we could go for some other non-system way, like a magic global shared between test and ToolboxProcess.jsm...

We only assert that the addon list gets updated in browser_addons_install, I think it would be great to always assert that it is correct?

Main patch. Waiting for green try and review of the first patch before asking review, but works locally.

Fixed eslint.

Also fixed eslint. Unfortunately the test appear to leak on debug builds. No idea why...

Comment on attachment 8736695 [details] [diff] [review] Use env variable instead of BrowserToolboxProcess contructor option. Review of attachment 8736695 [details] [diff] [review]: ----------------------------------------------------------------- I think I can live with the env var approach. But you should also remove the testScript feature from BrowserToolboxProcess since it's not needed anymore. ::: devtools/client/framework/test/browser_browser_toolbox.js @@ +41,5 @@ > + return jsterm.execute(js); > + }) > + .then(() => toolbox.destroy()); > + }; > + env.set("MOZ_TOOLBOX_TEST_SCRIPT", "new " + testScript); Shouldn't you remove this at test cleanup time?

Removed the testScript from ToolboxProcess.jsm, reset the env variable on test end.

Fixing Services.obs.removeObserver (doesn't has a third argument) I may pull this patch out in a dedicated bug as :kumar is trying to do similar thing in bug 1246030.

Comment on attachment 8737150 [details] [diff] [review] Always assert DOM when installing/uninstalling addons - v2 Review of attachment 8737150 [details] [diff] [review]: ----------------------------------------------------------------- A few nits, but the patch looks good to me! And the tests also pass on my side. ::: devtools/client/aboutdebugging/test/browser_addons_install.js @@ +8,5 @@ > add_task(function* () { > let { tab, document } = yield openAboutDebugging("addons"); > > + // Install this addon, `installAddon` is going to do some assertions against > + // about:debugging DOM content Nit: "is going to do some assertions" is a bit vague. Maybe something more specific like "// Install this add-on, and verify that it appears in the about:debugging UI"? @@ +10,5 @@ > > + // Install this addon, `installAddon` is going to do some assertions against > + // about:debugging DOM content > + yield installAddon(document, "addons/unpacked/install.rdf", ADDON_NAME, > + "test-devtools"); Nit: Is there still a point in `yield`ing here, even though `installAddon` and `uninstallAddon` don't return anything anymore? Or maybe they should keep returning promises? @@ +15,2 @@ > > + // Now uninstall this addon, `uninstallAddon` is also doing assertions Nit: Maybe some more specific comment here too? @@ +15,3 @@ > > + // Now uninstall this addon, `uninstallAddon` is also doing assertions > + yield uninstallAddon(document, ADDON_ID, ADDON_NAME); Nit: Same comment about the `yield`. ::: devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js @@ +22,5 @@ > let { tab, document } = yield openAboutDebugging("addons"); > > info("Install a test addon."); > + yield installAddon(document, "addons/unpacked/install.rdf", ADDON_NAME, > + "test-devtools"); Nit: Same comment about the `yield`. @@ +54,5 @@ > debugButtons = [...document.querySelectorAll("#addons .debug-button")]; > ok(debugButtons.every(b => b.disabled), "Debug buttons should be disabled"); > > info("Uninstall addon installed earlier."); > + yield uninstallAddon(document, ADDON_ID, ADDON_NAME); Nit: Same comment about `yield`. ::: devtools/client/aboutdebugging/test/head.js @@ +145,5 @@ > addon.uninstall(); > }); > }); > + > + yield onAddonUninstalled; Nit: The intermediary `onAddonUninstalled` doesn't seem useful, since you can just `yield new Promise()`.

Comment on attachment 8737149 [details] [diff] [review] Use env variable instead of BrowserToolboxProcess contructor option - v2 Review of attachment 8737149 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/browser_browser_toolbox.js @@ +41,5 @@ > + return jsterm.execute(js); > + }) > + .then(() => toolbox.destroy()); > + }; > + env.set("MOZ_TOOLBOX_TEST_SCRIPT", "new " + testScript); Clear the env var registerCleanupFunction(() => {}) right after you set it, so there's more confidence it will be cleared even if the rest of the test fails: registerCleanupFunction(() => { env.set("MOZ_TOOLBOX_TEST_SCRIPT", ""); });

Updated the comments and removed the unecessary onAddonUninstalled. (In reply to Jan Keromnes [:janx] from comment #11) > @@ +10,5 @@ > > > > + // Install this addon, `installAddon` is going to do some assertions against > > + // about:debugging DOM content > > + yield installAddon(document, "addons/unpacked/install.rdf", ADDON_NAME, > > + "test-devtools"); > > Nit: Is there still a point in `yield`ing here, even though `installAddon` > and `uninstallAddon` don't return anything anymore? Or maybe they should > keep returning promises? Yes, waiting for the function to finish! Otherwise we are going to mixup install with uninstall (or any next step) installAddon and uninstallAddon are still async, even if they don't return a value.

Finally found the leak... it was quite simple but took me days to figure out.

Similar to: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/browser_browser_toolbox.js but against the addon toolbox. This depends on attachment 8737186 [details] [diff] [review], which allows to set a env variable to execute arbitrary code in the browser toolbox process.

Comment on attachment 8739083 [details] [diff] [review] Fix Browser Toolbox leak of its parameters -v1 Review of attachment 8739083 [details] [diff] [review]: ----------------------------------------------------------------- I wish it was easier find issues like this!

Comment on attachment 8739085 [details] [diff] [review] Test addon toolbox console - v2 Review of attachment 8739085 [details] [diff] [review]: ----------------------------------------------------------------- Works for me! ::: devtools/client/aboutdebugging/test/browser.ini @@ +9,5 @@ > service-workers/empty-sw.js > service-workers/push-sw.html > service-workers/push-sw.js > > +[browser_addons_debug_bootstrapped.js] Note: This hunk didn't apply on latest mozilla-central. You might want to rebase.

https://hg.mozilla.org/integration/fx-team/rev/19311e84545f49e17e8dae3382ee7dd995d46119 Bug 1261055 - Use env variable instead of BrowserToolboxProcess contructor option. r=jryans https://hg.mozilla.org/integration/fx-team/rev/6caaba7482eb805fac9c8ad30682a3bf97539861 Bug 1261055 - Always assert DOM when installing/uninstalling addons. r=janx https://hg.mozilla.org/integration/fx-team/rev/5fa204e17b8a5f5ecf24b7b3a30da5b8f1886198 Bug 1261055 - Fix Browser Toolbox leak of its parameters. r=jryans https://hg.mozilla.org/integration/fx-team/rev/c9369050bd736b67e8833033c6e27cef00090b8e Bug 1261055 - Test addon toolbox console. r=janx

https://hg.mozilla.org/mozilla-central/rev/19311e84545f https://hg.mozilla.org/mozilla-central/rev/6caaba7482eb https://hg.mozilla.org/mozilla-central/rev/5fa204e17b8a https://hg.mozilla.org/mozilla-central/rev/c9369050bd73