Test debugging addons

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

4.83 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
8.94 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
1.00 KB, patch
jryans
: review+
Details | Diff | Splinter Review
4.92 KB, patch
janx
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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...
(Assignee)

Comment 1

2 years ago
Created attachment 8736695 [details] [diff] [review]
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...
Attachment #8736695 - Flags: review?(jryans)
(Assignee)

Comment 2

2 years ago
Created attachment 8736696 [details] [diff] [review]
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?
Attachment #8736696 - Flags: review?(janx)
(Assignee)

Comment 3

2 years ago
Created attachment 8736697 [details] [diff] [review]
Test addon toolbox console

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

Comment 5

2 years ago
Created attachment 8736731 [details] [diff] [review]
Always assert DOM when installing/uninstalling addons

Fixed eslint.
Attachment #8736731 - Flags: review?(janx)
(Assignee)

Updated

2 years ago
Attachment #8736696 - Attachment is obsolete: true
Attachment #8736696 - Flags: review?(janx)
(Assignee)

Comment 6

2 years ago
Created attachment 8736732 [details] [diff] [review]
Test addon toolbox console

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

Updated

2 years ago
Attachment #8736697 - Attachment is obsolete: true
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?
Attachment #8736695 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8737149 [details] [diff] [review]
Use env variable instead of BrowserToolboxProcess contructor option - v2

Removed the testScript from ToolboxProcess.jsm,
reset the env variable on test end.
Attachment #8737149 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8736695 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8737150 [details] [diff] [review]
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.
Attachment #8737150 - Flags: review?(janx)
(Assignee)

Updated

2 years ago
Attachment #8736731 - Attachment is obsolete: true
Attachment #8736731 - Flags: review?(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()`.
Attachment #8737150 - Flags: review?(janx) → review+
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", "");
});
Attachment #8737149 - Flags: review?(jryans) → review+
(Assignee)

Comment 13

2 years ago
Created attachment 8737186 [details] [diff] [review]
Use env variable instead of BrowserToolboxProcess contructor option - v3
Attachment #8737186 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8737149 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Created attachment 8737187 [details] [diff] [review]
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.
Attachment #8737187 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8737150 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8739083 [details] [diff] [review]
Fix Browser Toolbox leak of its parameters -v1

Finally found the leak... it was quite simple but took me days to figure out.
Attachment #8739083 - Flags: review?(jryans)
(Assignee)

Comment 18

2 years ago
Created attachment 8739085 [details] [diff] [review]
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.
Attachment #8739085 - Flags: review?(janx)
(Assignee)

Updated

2 years ago
Attachment #8736732 - Attachment is obsolete: true
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!
Attachment #8739083 - Flags: review?(jryans) → review+
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.
Attachment #8739085 - Flags: review?(janx) → review+

Comment 22

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.