allow permanent as well as temporary add-ons via MockExtension

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Temporary add-ons are fine for most things, but don't work for a few cases such as testing updates.

Right now MockExtension can automatically install a temporary add-on, it should be able to install a permanent add-on as well.
(Assignee)

Comment 1

a year ago
Created attachment 8776195 [details]
Bug 1290617 - allow non-temp mock webextensions

Review commit: https://reviewboard.mozilla.org/r/68098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68098/
Attachment #8776195 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Blocks: 1279012
(Assignee)

Comment 2

a year ago
I tested this by temporarily modifying the tests in `test_ext_simple.js` (needs signature check disabled and AddonManager started.)
Disabling the signature check will work fine on mozilla-central and various integration branches, but if folks start using that technique, tests will break when their code makes it to beta (where signing may not be disabled).
Comment on attachment 8776195 [details]
Bug 1290617 - allow non-temp mock webextensions

https://reviewboard.mozilla.org/r/68098/#review65252

::: toolkit/components/extensions/Extension.jsm:1252
(Diff revision 1)
>  
>    startup() {
> +    // Enable more extensive EM logging
> +    Services.prefs.setBoolPref("extensions.logging.enabled", true);
> +    // By default, don't cache add-ons in AddonRepository.jsm
> +    Services.prefs.setBoolPref("extensions.getAddons.cache.enabled", false);

We should make these pref changes in the appropriate head.js or test files, and make sure to revert them. This may be OK for xpcshell tests, but we shouldn't let pref changes like this stick around in mochitests.

::: toolkit/components/extensions/Extension.jsm:1307
(Diff revision 1)
>    Services.ppmm.broadcastAsyncMessage("Extension:FlushJarCache", {path: file.path});
>  
>    let fileURI = Services.io.newFileURI(file);
>    let jarURI = Services.io.newURI("jar:" + fileURI.spec + "!/", null, null);
>  
> +  // this may be "temporary" or "permanent".

Please capitalize.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_background_debug_global.html:38
(Diff revision 1)
>    window.testThing = "test!";
>    browser.test.notifyPass("background script ran");
>  }
>  
>  let extensionData = {
> -  useAddonManager: true,
> +  useAddonManager: "temporary",

There are a couple of tests in browser/components/extensions/test/browser that need this change too.
Attachment #8776195 - Flags: review?(kmaglione+bmo) → review+
(In reply to Andrew Swan [:aswan] from comment #3)
> Disabling the signature check will work fine on mozilla-central and various
> integration branches, but if folks start using that technique, tests will
> break when their code makes it to beta (where signing may not be disabled).

It will still work in xpcshell tests where we've disabled signature checking. The sheriffs run periodic aurora and release channel simulation builds, so if it somehow makes it into a mochitest, we should catch it well before an actual merge.
(In reply to Kris Maglione [:kmag] from comment #5)
> (In reply to Andrew Swan [:aswan] from comment #3)
> > Disabling the signature check will work fine on mozilla-central and various
> > integration branches, but if folks start using that technique, tests will
> > break when their code makes it to beta (where signing may not be disabled).
> 
> It will still work in xpcshell tests where we've disabled signature
> checking. The sheriffs run periodic aurora and release channel simulation
> builds, so if it somehow makes it into a mochitest, we should catch it well
> before an actual merge.

Yes, I said it in a roundabout way but I meant to say "we shouldn't ever use the technique of disabling the signature check in mochitests".  And of course it is irrelevant for xpcshell as you point out.

Also, drive-by review comment, pushPrefEnv() it a nice way to apply temporary preference changes, you can undo them all with one line and the undo happens automatically at the end of a test.
(Assignee)

Comment 7

a year ago
(In reply to Andrew Swan [:aswan] from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > (In reply to Andrew Swan [:aswan] from comment #3)
> > > Disabling the signature check will work fine on mozilla-central and various
> > > integration branches, but if folks start using that technique, tests will
> > > break when their code makes it to beta (where signing may not be disabled).
> > 
> > It will still work in xpcshell tests where we've disabled signature
> > checking. The sheriffs run periodic aurora and release channel simulation
> > builds, so if it somehow makes it into a mochitest, we should catch it well
> > before an actual merge.
> 
> Yes, I said it in a roundabout way but I meant to say "we shouldn't ever use
> the technique of disabling the signature check in mochitests".  And of
> course it is irrelevant for xpcshell as you point out.


This is a good point - I only plan to use this feature in an xpcshell test, I'll make sure not to use `useAddonManager: "permanent"` in mochitests.

 
> Also, drive-by review comment, pushPrefEnv() it a nice way to apply
> temporary preference changes, you can undo them all with one line and the
> undo happens automatically at the end of a test.


That is only for SpecialPowers in mochitests?
(Assignee)

Comment 8

a year ago
https://reviewboard.mozilla.org/r/68098/#review65252

> We should make these pref changes in the appropriate head.js or test files, and make sure to revert them. This may be OK for xpcshell tests, but we shouldn't let pref changes like this stick around in mochitests.

OK I just removed these, the only really important one for this feature to work is disabling the add-on cache.

I'll just set it along with disabling signature etc. when I use it in the test for bug 1279012.
(Assignee)

Comment 9

a year ago
Comment on attachment 8776195 [details]
Bug 1290617 - allow non-temp mock webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68098/diff/1-2/

Updated

a year ago
Priority: -- → P2
Whiteboard: triaged

Comment 10

a year ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff654490bc1e
allow non-temp mock webextensions r=kmag

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff654490bc1e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.