1261055 - Test debugging addons

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

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...

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Use env variable instead of BrowserToolboxProcess contructor option.

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...

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Always assert DOM when installing/uninstalling addons.

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?

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Test addon toolbox console

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

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Always assert DOM when installing/uninstalling addons

Fixed eslint.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Test addon toolbox console

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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Use env variable instead of BrowserToolboxProcess contructor option - v2

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

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Always assert DOM when installing/uninstalling addons - v2

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 10

[Alexandre Poirot [:ochameau]]

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

Comment 11

[Jan Keromnes [:janx]]

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 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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", "");
});

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Use env variable instead of BrowserToolboxProcess contructor option - v3

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Always assert DOM when installing/uninstalling addons - v3

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.

Comment 15

[Alexandre Poirot [:ochameau]]

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

Comment 16

[Alexandre Poirot [:ochameau]]

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

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Fix Browser Toolbox leak of its parameters -v1

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

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Test addon toolbox console - v2

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 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 20

[Jan Keromnes [:janx]]

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.

Comment 21

[Alexandre Poirot [:ochameau]]

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

Comment 22

[Carsten Book [:Tomcat]]

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