Closed Bug 1261055 Opened 4 years ago Closed 4 years ago

Test debugging addons

Categories

(DevTools :: about:debugging, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

Details

Attachments

(4 files, 7 obsolete files)

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
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...
Attachment #8736695 - Flags: review?(jryans)
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)
Attached patch Test addon toolbox console (obsolete) — Splinter Review
Main patch. Waiting for green try and review of the first patch before asking review, but works locally.
Fixed eslint.
Attachment #8736731 - Flags: review?(janx)
Attachment #8736696 - Attachment is obsolete: true
Attachment #8736696 - Flags: review?(janx)
Attached patch Test addon toolbox console (obsolete) — Splinter Review
Also fixed eslint. Unfortunately the test appear to leak on debug builds.
No idea why...
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+
Removed the testScript from ToolboxProcess.jsm,
reset the env variable on test end.
Attachment #8737149 - Flags: review?(jryans)
Attachment #8736695 - Attachment is obsolete: true
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)
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+
Attachment #8737149 - Attachment is obsolete: true
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+
Attachment #8737150 - Attachment is obsolete: true
Finally found the leak... it was quite simple but took me days to figure out.
Attachment #8739083 - Flags: review?(jryans)
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)
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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.