1571412 - Update addon manager browser tests to use pushPrefEnv where appropriate

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task, P5)

Description

[Andrew Swan [:aswan]]

There are some old tests in toolkit/mozapps/extensions/tests/browser that directly set preferences and have their own logic to restore the previous values. There are individual cases such as:
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js#45-57

As well as some general purpose code in head.js:
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/toolkit/mozapps/extensions/test/browser/head.js#65-93
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/toolkit/mozapps/extensions/test/browser/head.js#165-176

All of this can and should be replaced with SpecialPowers.pushPrefEnv()

Marking this as a good-first-bug. Its a little more complicated than other good first bugs but should still be approachable for somebody new to this code base.

Comment 1

[bugtastico]

Hi, I would like to give this one a go - can I be assigned this bug please?

I can see these files locally and have the tests running successfully with "mach mochitest -f browser toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js" but I haven't been able to figure out what's actually required.

e.g. in browser_CTP_plugins.js I see two calls to setAndUpdateBlocklist - are you suggesting replacing those calls or a part of that function with a call to SpecialPowers.pushPrefEnv - it seems to get rather than set though?

I've also done a quick search https://searchfox.org/mozilla-central/search?q=SpecialPowers.&path= and saw .popPrefEnv and some other functions.

Can you provide some more guidance please - I'm probably overcomplicating it for myself at this stage!

Comment 2

[Andrew Swan [:aswan]]

Hi, thanks for your interest!

I think a good place to start would be familiarizing yourself with pushPrefEnv:
https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/testing/specialpowers/content/SpecialPowersAPIParent.jsm#314-336

The idea is that individual tests have a simple API to set preferences just for the duration of a test that automatically resets those preferences after the test is over (or when manually reset with popPrefEnv). The goal of this bug is to reduce code duplication: since pushPrefEnv takes care of remembering and eventually restoring the original value of a preference, lets use it instead of having any test-specific code to do the same thing.

browser_CTP_plugins looks like it might be the most complicated case but briefly: setAndUpdateBlocklist stores the original value of extensions.blocklist.url and then it calls into updateBlocklist which sometimes changes that pref. Then resetBlocklist always restores the original value of the pref. We want to remove the code that saves the original value, replace the call to set the pref inside updateBlocklist with pushPrefEnv, and replace the body of resetBlocklist with popPrefEnv. This is complicated by the fact that updateBlocklist doesn't always set a pref, a really expedient fix would be to make updateBlocklist just call pushPrefEnv with an empty object (ie SpecialPowers.pushPrefEnv({})) in the branch where it doesn't set a pref.

The majority of the cases here are a little different, they involve the code in head.js that I mentioned in comment 0. (head.js is automatically sourced before the actual test script contents for all the tests in this directory). The code there does the same thing, it stores a bunch of preference values in a variable called gRestorePrefs and then automatically restores them when the test is over. So a bunch of tests in this directory just set those prefs, knowing that they will be restored by the code in head.js. The fix here is to locate the individual tests that set any of the prefs listed in gRestorePrefs and update them to use pushPrefEnv instead of setting prefs directly. Once that's done you can remove all the code that refers to gRestorePrefs from head.js.

Finally, there are a few cases like this:
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js#3-6

That code can be directly replaced with a call to pushPrefEnv. The only issue there is that pushPrefEnv is async and you can't just await it at the top level, so it will need to move into an actual test function.

Whew, that's a lot of stuff! I'd suggest taking things one-by-one and feel free to post patches as you're working on them and ask questions if you get stuck. You can also come ask questions in the #webextensions channel on IRC (https://wiki.mozilla.org/IRC) I'm around during typical North America working hours, but there are other folks in the channel who might be able to help at other hours as well.

Good luck!

Comment 3

[bugtastico]

Attachment: Bug 1571412 - replace test-specific code with SpecialPowers.pushPrefEnv and popPrefEnv

Comment 4

[bugtastico]

Attachment: Bug 1571412 - WIP update head and most remaining test-specific code with SpecialPowers.pushPrefEnv

Depends on D40936

Comment 5

[bugtastico]

Hi, thanks very much for the detailed overview, that helped a lot!

I've submitted two patches. The first is the changes to browser_CTP_plugins.

The other is all the other changes to the other tests, and the changes I think are needed to head. All test files pass except for 2, which I've commented out for now so I could flag them and run all tests together. I'm not quite sure why they are failing yet, I still need to dig into that but if you have any thoughts or anything obvious wrong in these patches - please let me know.

Also I noticed pushPrefEnv being used with and without await - so I've just used await where it is explicitly inside an async function - let me know if that's wrong.

I look forward to your comments to see what can be improved.

Comment 6

[Andrew Swan [:aswan]]

Commented on phabricator. Luca, I chose you semi-randomly to take over mentorship if that's okay (it looks like you and bugtastico are in closer time zones at least)

Comment 7

[bugtastico]

Attachment: Bug 1571412 - replace test-specific code with SpecialPowers.pushPrefEnv r=aswan

Comment 8

[Caitlin Neiman [:caitmuenster]]

Hey bugtastico, since we haven't heard from you in awhile, we're going to open this up to another contributor. If you would like to keep working on it, please go ahead and update the patch!

Comment 9

[Jayati Shrivastava]

Hi, may I work on this bug??

Comment 10

[Caitlin Neiman [:caitmuenster]]

Hi Jatati, go for it! Comment #2 has some great info for getting started. Please also check out our onboarding documentation at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp, and needinfo rpl (the bug's mentor) if you have any questions.

Comment 11

[Jayati Shrivastava]

Attachment: Bug 1571412 - Update addon manager browser tests to use pushPrefEnv where appropriate. r=rpl

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/931f4791ca89
Update addon manager browser tests to use pushPrefEnv where appropriate. r=rpl

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/931f4791ca89