Bug 1426530 - Make the normandy test suite pass with --verify r=Gijs

MozReview-Commit-ID: Awnxu9IAhDQ

--HG--
extra : rebase_source : 8ec5f4e28c30264de402381d9775fc9abc99a05e

browser/extensions/shield-recipe-client/lib/AddonStudies.jsm

@@ -120,11 +120,12 @@ this.AddonStudies = {
         for (const study of studies) {
           await getStore(db).add(study);
         }
+        await AddonStudies.close();
 
         try {
           await testFunction(...args, studies);
         } finally {
-          db = await getDatabase(); // Re-acquire in case the test closed the connection.
+          db = await getDatabase();
           await AddonStudies.clear();
           for (const study of oldStudies) {
             await getStore(db).add(study);

browser/extensions/shield-recipe-client/test/browser/browser_AddonStudies.js

@@ -9,6 +9,16 @@ Cu.import("resource://shield-recipe-client/lib/AddonStudies.jsm", this);
 // Initialize test utils
 AddonTestUtils.initMochitest(this);
 
+let _startArgsFactoryId = 1;
+function startArgsFactory(args) {
+  return Object.assign({
+    recipeId: _startArgsFactoryId++,
+    name: "Test",
+    description: "Test",
+    addonUrl: "http://test/addon.xpi",
+  }, args);
+}
+
 decorate_task(
   AddonStudies.withStudies(),
   async function testGetMissing() {
@@ -103,16 +113,6 @@ decorate_task(
   }
 );
 
-let _startArgsFactoryId = 0;
-function startArgsFactory(args) {
-  return Object.assign({
-    recipeId: _startArgsFactoryId++,
-    name: "Test",
-    description: "Test",
-    addonUrl: "http://test/addon.xpi",
-  }, args);
-}
-
 add_task(async function testStartRequiredArguments() {
   const requiredArguments = startArgsFactory();
   for (const key in requiredArguments) {
@@ -175,6 +175,7 @@ decorate_task(
 
 decorate_task(
   withWebExtension({version: "2.0"}),
+  AddonStudies.withStudies(),
   async function testStart([addonId, addonFile]) {
     const startupPromise = AddonTestUtils.promiseWebExtensionStartup(addonId);
     const addonUrl = Services.io.newFileURI(addonFile).spec;
@@ -210,7 +211,7 @@ decorate_task(
     );
     ok(study.studyStartDate, "start assigns a value to the study start date.");
 
-    await Addons.uninstall(addonId);
+    await AddonStudies.stop(args.recipeId);
   }
 );
 

browser/extensions/shield-recipe-client/test/browser/browser_NormandyDriver.js

@@ -4,6 +4,7 @@ Cu.import("resource://gre/modules/AppConstants.jsm");
 Cu.import("resource://testing-common/AddonTestUtils.jsm", this);
 Cu.import("resource://shield-recipe-client/lib/AddonStudies.jsm", this);
 Cu.import("resource://shield-recipe-client/lib/NormandyDriver.jsm", this);
+Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this);
 
 add_task(withDriver(Assert, async function uuids(driver) {
   // Test that it is a UUID
@@ -192,6 +193,7 @@ decorate_task(
 decorate_task(
   withSandboxManager(Assert),
   withWebExtension({id: "driver-addon-studies@example.com"}),
+  AddonStudies.withStudies(),
   async function testAddonStudies(sandboxManager, [addonId, addonFile]) {
     const addonUrl = Services.io.newFileURI(addonFile).spec;
     const driver = new NormandyDriver(sandboxManager);
@@ -319,7 +321,8 @@ decorate_task(
 decorate_task(
   withSandboxManager(Assert),
   withMockPreferences,
-  async function testAddonStudies(sandboxManager) {
+  PreferenceExperiments.withMockExperiments,
+  async function testPreferenceStudies(sandboxManager) {
     const driver = new NormandyDriver(sandboxManager);
     sandboxManager.cloneIntoGlobal("driver", driver, {cloneFunctions: true});
 

browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js

@@ -3,6 +3,7 @@
 Cu.import("resource://gre/modules/Preferences.jsm", this);
 Cu.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
 Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this);
+Cu.import("resource://shield-recipe-client/lib/CleanupManager.jsm", this);
 
 // Save ourselves some typing
 const {withMockExperiments} = PreferenceExperiments;
@@ -25,7 +26,9 @@ function experimentFactory(attrs) {
 }
 
 // clearAllExperimentStorage
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     experiments.test = experimentFactory({name: "test"});
     ok(await PreferenceExperiments.has("test"), "Mock experiment is detected.");
     await PreferenceExperiments.clearAllExperimentStorage();
@@ -33,10 +36,13 @@ add_task(withMockExperiments(async function(experiments) {
       !(await PreferenceExperiments.has("test")),
       "clearAllExperimentStorage removed all stored experiments",
     );
-}));
+  }
+);
 
 // start should throw if an experiment with the given name already exists
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     experiments.test = experimentFactory({name: "test"});
     await Assert.rejects(
       PreferenceExperiments.start({
@@ -49,10 +55,13 @@ add_task(withMockExperiments(async function(experiments) {
       }),
       "start threw an error due to a conflicting experiment name",
     );
-}));
+  }
+);
 
 // start should throw if an experiment for the given preference is active
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     experiments.test = experimentFactory({name: "test", preferenceName: "fake.preference"});
     await Assert.rejects(
       PreferenceExperiments.start({
@@ -65,10 +74,13 @@ add_task(withMockExperiments(async function(experiments) {
       }),
       "start threw an error due to an active experiment for the given preference",
     );
-}));
+  }
+);
 
 // start should throw if an invalid preferenceBranchType is given
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     await Assert.rejects(
       PreferenceExperiments.start({
         name: "test",
@@ -80,7 +92,8 @@ add_task(withMockExperiments(async function() {
       }),
       "start threw an error due to an invalid preference branch type",
     );
-}));
+  }
+);
 
 // start should save experiment data, modify the preference, and register a
 // watcher.
@@ -139,10 +152,13 @@ decorate_task(
 );
 
 // start should modify the user preference for the user branch type
-add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
-  const startObserver = sinon.stub(PreferenceExperiments, "startObserver");
-  mockPreferences.set("fake.preference", "oldvalue", "user");
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "startObserver"),
+  async function(experiments, mockPreferences, startObserver) {
     mockPreferences.set("fake.preference", "olddefaultvalue", "default");
+    mockPreferences.set("fake.preference", "oldvalue", "user");
 
     await PreferenceExperiments.start({
       name: "test",
@@ -178,12 +194,13 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
       "start did not modify the default preference",
     );
     is(Preferences.get("fake.preference"), "newvalue", "start modified the user preference");
-
-  startObserver.restore();
-})));
+  }
+);
 
 // start should detect if a new preference value type matches the previous value type
-add_task(withMockPreferences(async function(mockPreferences) {
+decorate_task(
+  withMockPreferences,
+  async function(mockPreferences) {
     mockPreferences.set("fake.type_preference", "oldvalue");
 
     await Assert.rejects(
@@ -197,23 +214,29 @@ add_task(withMockPreferences(async function(mockPreferences) {
       }),
       "start threw error for incompatible preference type"
     );
-}));
-
+  }
+);
 
 // startObserver should throw if an observer for the experiment is already
 // active.
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     PreferenceExperiments.startObserver("test", "fake.preference", "string", "newvalue");
     Assert.throws(
       () => PreferenceExperiments.startObserver("test", "another.fake", "string", "othervalue"),
       "startObserver threw due to a conflicting active observer",
     );
     PreferenceExperiments.stopAllObservers();
-}));
+  }
+);
 
 // startObserver should register an observer that calls stop when a preference
 // changes from its experimental value.
-add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, mockPreferences) {
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  async function(mockExperiments, mockPreferences) {
     const tests = [
       ["string", "startvalue", "experimentvalue", "newvalue"],
       ["boolean", false, true, false],
@@ -228,40 +251,50 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments,
       PreferenceExperiments.startObserver("test" + type, "fake.preference" + type, type, experimentvalue);
 
       // Setting it to the experimental value should not trigger the call.
-    Preferences.set("fake.preference" + type, experimentvalue);
+      mockPreferences.set("fake.preference" + type, experimentvalue);
       ok(!stop.called, "Changing to the experimental pref value did not trigger the observer");
 
       // Setting it to something different should trigger the call.
-    Preferences.set("fake.preference" + type, newvalue);
+      mockPreferences.set("fake.preference" + type, newvalue);
       ok(stop.called, "Changing to a different value triggered the observer");
 
       PreferenceExperiments.stopAllObservers();
       stop.restore();
     }
-})));
+  }
+);
 
-add_task(withMockExperiments(async function testHasObserver() {
+decorate_task(
+  withMockExperiments,
+  async function testHasObserver() {
     PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentValue");
 
-  ok(await PreferenceExperiments.hasObserver("test"), "hasObserver detects active observers");
+    ok(await PreferenceExperiments.hasObserver("test"), "hasObserver should detect active observers");
     ok(
       !(await PreferenceExperiments.hasObserver("missing")),
-    "hasObserver doesn't detect inactive observers",
+      "hasObserver shouldn't detect inactive observers",
     );
 
     PreferenceExperiments.stopAllObservers();
-}));
+  }
+);
 
 // stopObserver should throw if there is no observer active for it to stop.
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     Assert.throws(
       () => PreferenceExperiments.stopObserver("neveractive", "another.fake", "othervalue"),
       "stopObserver threw because there was not matching active observer",
     );
-}));
+  }
+);
 
 // stopObserver should cancel an active observer.
-add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, mockPreferences) {
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  async function(mockExperiments, mockPreferences) {
     const stop = sinon.stub(PreferenceExperiments, "stop");
     mockPreferences.set("fake.preference", "startvalue");
 
@@ -270,7 +303,7 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments,
 
     // Setting the preference now that the observer is stopped should not call
     // stop.
-  Preferences.set("fake.preference", "newvalue");
+    mockPreferences.set("fake.preference", "newvalue");
     ok(!stop.called, "stopObserver successfully removed the observer");
 
     // Now that the observer is stopped, start should be able to start a new one
@@ -283,10 +316,14 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments,
 
     PreferenceExperiments.stopAllObservers();
     stop.restore();
-})));
+  }
+);
 
 // stopAllObservers
-add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, mockPreferences) {
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  async function(mockExperiments, mockPreferences) {
     const stop = sinon.stub(PreferenceExperiments, "stop");
     mockPreferences.set("fake.preference", "startvalue");
     mockPreferences.set("other.fake.preference", "startvalue");
@@ -297,8 +334,8 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments,
 
     // Setting the preference now that the observers are stopped should not call
     // stop.
-  Preferences.set("fake.preference", "newvalue");
-  Preferences.set("other.fake.preference", "newvalue");
+    mockPreferences.set("fake.preference", "newvalue");
+    mockPreferences.set("other.fake.preference", "newvalue");
     ok(!stop.called, "stopAllObservers successfully removed all observers");
 
     // Now that the observers are stopped, start should be able to start new
@@ -312,18 +349,24 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments,
 
     PreferenceExperiments.stopAllObservers();
     stop.restore();
-})));
+  }
+);
 
 // markLastSeen should throw if it can't find a matching experiment
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     await Assert.rejects(
       PreferenceExperiments.markLastSeen("neveractive"),
       "markLastSeen threw because there was not a matching experiment",
     );
-}));
+  }
+);
 
 // markLastSeen should update the lastSeen date
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     const oldDate = new Date(1988, 10, 1).toJSON();
     experiments.test = experimentFactory({name: "test", lastSeen: oldDate});
     await PreferenceExperiments.markLastSeen("test");
@@ -332,24 +375,31 @@ add_task(withMockExperiments(async function(experiments) {
       oldDate,
       "markLastSeen updated the experiment lastSeen date",
     );
-}));
+  }
+);
 
 // stop should throw if an experiment with the given name doesn't exist
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     await Assert.rejects(
       PreferenceExperiments.stop("test"),
       "stop threw an error because there are no experiments with the given name",
     );
-}));
+  }
+);
 
 // stop should throw if the experiment is already expired
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     experiments.test = experimentFactory({name: "test", expired: true});
     await Assert.rejects(
       PreferenceExperiments.stop("test"),
       "stop threw an error because the experiment was already expired",
     );
-}));
+  }
+);
 
 // stop should mark the experiment as expired, stop its observer, and revert the
 // preference value.
@@ -358,6 +408,7 @@ decorate_task(
   withMockPreferences,
   withSpy(PreferenceExperiments, "stopObserver"),
   async function testStop(experiments, mockPreferences, stopObserverSpy) {
+    is(Preferences.get("fake.preference"), null, "preference should start unset");
     mockPreferences.set(`${startupPrefs}.fake.preference`, "experimentvalue", "user");
     mockPreferences.set("fake.preference", "experimentvalue", "default");
     experiments.test = experimentFactory({
@@ -389,9 +440,12 @@ decorate_task(
 );
 
 // stop should also support user pref experiments
-add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
-  const stopObserver = sinon.stub(PreferenceExperiments, "stopObserver");
-  const hasObserver = sinon.stub(PreferenceExperiments, "hasObserver");
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "stopObserver"),
+  withStub(PreferenceExperiments, "hasObserver"),
+  async function testStopUserPrefs(experiments, mockPreferences, stopObserver, hasObserver) {
     hasObserver.returns(true);
 
     mockPreferences.set("fake.preference", "experimentvalue", "user");
@@ -416,10 +470,14 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
     );
     stopObserver.restore();
     PreferenceExperiments.stopAllObservers();
-})));
+  }
+);
 
 // stop should remove a preference that had no value prior to an experiment for user prefs
-add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  async function(experiments, mockPreferences) {
     const stopObserver = sinon.stub(PreferenceExperiments, "stopObserver");
     mockPreferences.set("fake.preference", "experimentvalue", "user");
     experiments.test = experimentFactory({
@@ -439,11 +497,16 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
     );
 
     stopObserver.restore();
-})));
+  }
+);
 
 // stop should not modify a preference if resetValue is false
-add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
-  const stopObserver = sinon.stub(PreferenceExperiments, "stopObserver");
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "stopObserver"),
+
+  async function(experiments, mockPreferences, stopObserver) {
     mockPreferences.set("fake.preference", "customvalue", "default");
     experiments.test = experimentFactory({
       name: "test",
@@ -461,20 +524,24 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
       "customvalue",
       "stop did not modify the preference",
     );
-
-  stopObserver.restore();
-})));
+  }
+);
 
 // get should throw if no experiment exists with the given name
-add_task(withMockExperiments(async function() {
+decorate_task(
+  withMockExperiments,
+  async function() {
     await Assert.rejects(
       PreferenceExperiments.get("neverexisted"),
       "get rejects if no experiment with the given name is found",
     );
-}));
+  }
+);
 
 // get
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     const experiment = experimentFactory({name: "test"});
     experiments.test = experiment;
 
@@ -484,9 +551,13 @@ add_task(withMockExperiments(async function(experiments) {
     // Modifying the fetched experiment must not edit the data source.
     fetchedExperiment.name = "othername";
     is(experiments.test.name, "test", "get returns a copy of the experiment");
-}));
+  }
+);
 
-add_task(withMockExperiments(async function testGetAll(experiments) {
+// get all
+decorate_task(
+  withMockExperiments,
+  async function testGetAll(experiments) {
     const experiment1 = experimentFactory({name: "experiment1"});
     const experiment2 = experimentFactory({name: "experiment2", disabled: true});
     experiments.experiment1 = experiment1;
@@ -508,9 +579,14 @@ add_task(withMockExperiments(async function testGetAll(experiments) {
 
     fetchedExperiment2.name = "othername";
     is(experiment2.name, "experiment2", "getAll returns copies of the experiments");
-}));
+  }
+);
 
-add_task(withMockExperiments(withMockPreferences(async function testGetAllActive(experiments) {
+// get all active
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  async function testGetAllActive(experiments) {
     experiments.active = experimentFactory({
       name: "active",
       expired: false,
@@ -533,14 +609,18 @@ add_task(withMockExperiments(withMockPreferences(async function testGetAllActive
       "newfakename",
       "getAllActive returns copies of stored experiments",
     );
-})));
+  }
+);
 
 // has
-add_task(withMockExperiments(async function(experiments) {
+decorate_task(
+  withMockExperiments,
+  async function(experiments) {
     experiments.test = experimentFactory({name: "test"});
     ok(await PreferenceExperiments.has("test"), "has returned true for a stored experiment");
     ok(!(await PreferenceExperiments.has("missing")), "has returned false for a missing experiment");
-}));
+  }
+);
 
 // init should register telemetry experiments
 decorate_task(
@@ -639,10 +719,13 @@ decorate_task(
       ["test", "branch", {type: "normandy-pref-test"}],
       "start() should register the experiment with the provided type",
     );
+
+    // start sets the passed preference in a way that is hard to mock.
+    // Reset the preference so it doesn't interfere with other tests.
+    Services.prefs.getDefaultBranch("fake.preference").deleteBranch("");
   },
 );
 
-
 // Experiments shouldn't be recorded by init() in telemetry if they are expired
 decorate_task(
   withMockExperiments,
@@ -655,8 +738,11 @@ decorate_task(
 );
 
 // Experiments should end if the preference has been changed when init() is called
-add_task(withMockExperiments(withMockPreferences(async function testInitChanges(experiments, mockPreferences) {
-  const stopStub = sinon.stub(PreferenceExperiments, "stop");
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "stop"),
+  async function testInitChanges(experiments, mockPreferences, stopStub) {
     mockPreferences.set("fake.preference", "experiment value", "default");
     experiments.test = experimentFactory({
       name: "test",
@@ -666,15 +752,21 @@ add_task(withMockExperiments(withMockPreferences(async function testInitChanges(
     mockPreferences.set("fake.preference", "changed value");
     await PreferenceExperiments.init();
     ok(stopStub.calledWith("test"), "Experiment is stopped because value changed");
-  ok(Preferences.get("fake.preference"), "changed value", "Preference value was not changed");
-  stopStub.restore();
-})));
-
+    is(Preferences.get("fake.preference"), "changed value", "Preference value was not changed");
+  }
+);
 
 // init should register an observer for experiments
-add_task(withMockExperiments(withMockPreferences(async function testInitRegistersObserver(experiments, mockPreferences) {
-  const startObserver = sinon.stub(PreferenceExperiments, "startObserver");
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "startObserver"),
+  withStub(PreferenceExperiments, "stop"),
+  withStub(CleanupManager, "addCleanupHandler"),
+  async function testInitRegistersObserver(experiments, mockPreferences, startObserver, stop) {
+    stop.throws("Stop should not be called");
     mockPreferences.set("fake.preference", "experiment value", "default");
+    is(Preferences.get("fake.preference"), "experiment value", "pref shouldn't have a user value");
     experiments.test = experimentFactory({
       name: "test",
       preferenceName: "fake.preference",
@@ -682,14 +774,16 @@ add_task(withMockExperiments(withMockPreferences(async function testInitRegister
     });
     await PreferenceExperiments.init();
 
-  ok(
-    startObserver.calledWith("test", "fake.preference", "string", "experiment value"),
-    "init registered an observer",
+    ok(startObserver.calledOnce, "init should register an observer");
+    Assert.deepEqual(
+      startObserver.getCall(0).args,
+      ["test", "fake.preference", "string", "experiment value"],
+      "init should register an observer with the right args",
+    );
+  }
 );
 
-  startObserver.restore();
-})));
-
+// saveStartupPrefs
 decorate_task(
   withMockExperiments,
   async function testSaveStartupPrefs(experiments) {
@@ -731,6 +825,7 @@ decorate_task(
   },
 );
 
+// saveStartupPrefs errors for invalid pref type
 decorate_task(
   withMockExperiments,
   async function testSaveStartupPrefsError(experiments) {

browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js

@@ -66,9 +66,11 @@ add_task(async function checkFilter() {
   ok(!(await RecipeRunner.checkFilter(recipe)), "The recipe is available in the filter context");
 });
 
-add_task(withMockNormandyApi(async function testClientClassificationCache() {
-  const getStub = sinon.stub(ClientEnvironment, "getClientClassification")
-    .returns(Promise.resolve(false));
+decorate_task(
+  withMockNormandyApi,
+  withStub(ClientEnvironment, "getClientClassification"),
+  async function testClientClassificationCache(api, getStub) {
+    getStub.returns(Promise.resolve(false));
 
     await SpecialPowers.pushPrefEnv({set: [
       ["extensions.shield-recipe-client.api_url",
@@ -91,9 +93,8 @@ add_task(withMockNormandyApi(async function testClientClassificationCache() {
     ok(!getStub.called, "getClientClassification hasn't been called");
     await RecipeRunner.run();
     ok(!getStub.called, "getClientClassification was not called eagerly");
-
-  getStub.restore();
-}));
+  }
+);
 
 /**
  * Mocks RecipeRunner.loadActionSandboxManagers for testing run.
@@ -118,12 +119,13 @@ async function withMockActionSandboxManagers(actions, testFunction) {
   }
 }
 
-add_task(withMockNormandyApi(async function testRun(mockApi) {
-  const closeSpy = sinon.spy(AddonStudies, "close");
-  const reportRunner = sinon.stub(Uptake, "reportRunner");
-  const reportAction = sinon.stub(Uptake, "reportAction");
-  const reportRecipe = sinon.stub(Uptake, "reportRecipe");
-
+decorate_task(
+  withMockNormandyApi,
+  withSpy(AddonStudies, "close"),
+  withStub(Uptake, "reportRunner"),
+  withStub(Uptake, "reportAction"),
+  withStub(Uptake, "reportRecipe"),
+  async function testRun(mockApi, closeSpy, reportRunner, reportAction, reportRecipe) {
     const matchAction = {name: "matchAction"};
     const noMatchAction = {name: "noMatchAction"};
     mockApi.actions = [matchAction, noMatchAction];
@@ -163,14 +165,12 @@ add_task(withMockNormandyApi(async function testRun(mockApi) {
 
     // Ensure storage is closed after the run.
     sinon.assert.calledOnce(closeSpy);
+  }
+);
 
-  closeSpy.restore();
-  reportRunner.restore();
-  reportAction.restore();
-  reportRecipe.restore();
-}));
-
-add_task(withMockNormandyApi(async function testRunRecipeError(mockApi) {
+decorate_task(
+  withMockNormandyApi,
+  async function testRunRecipeError(mockApi) {
     const reportRecipe = sinon.stub(Uptake, "reportRecipe");
 
     const action = {name: "action"};
@@ -194,9 +194,12 @@ add_task(withMockNormandyApi(async function testRunRecipeError(mockApi) {
     });
 
     reportRecipe.restore();
-}));
+  }
+);
 
-add_task(withMockNormandyApi(async function testRunFetchFail(mockApi) {
+decorate_task(
+  withMockNormandyApi,
+  async function testRunFetchFail(mockApi) {
     const closeSpy = sinon.spy(AddonStudies, "close");
     const reportRunner = sinon.stub(Uptake, "reportRunner");
 
@@ -231,9 +234,12 @@ add_task(withMockNormandyApi(async function testRunFetchFail(mockApi) {
 
     closeSpy.restore();
     reportRunner.restore();
-}));
+  }
+);
 
-add_task(withMockNormandyApi(async function testRunPreExecutionFailure(mockApi) {
+decorate_task(
+  withMockNormandyApi,
+  async function testRunPreExecutionFailure(mockApi) {
     const closeSpy = sinon.spy(AddonStudies, "close");
     const reportAction = sinon.stub(Uptake, "reportAction");
     const reportRecipe = sinon.stub(Uptake, "reportRecipe");
@@ -273,9 +279,12 @@ add_task(withMockNormandyApi(async function testRunPreExecutionFailure(mockApi)
     closeSpy.restore();
     reportAction.restore();
     reportRecipe.restore();
-}));
+  }
+);
 
-add_task(withMockNormandyApi(async function testRunPostExecutionFailure(mockApi) {
+decorate_task(
+  withMockNormandyApi,
+  async function testRunPostExecutionFailure(mockApi) {
     const reportAction = sinon.stub(Uptake, "reportAction");
 
     const failAction = {name: "failAction"};
@@ -304,9 +313,12 @@ add_task(withMockNormandyApi(async function testRunPostExecutionFailure(mockApi)
     });
 
     reportAction.restore();
-}));
+  }
+);
 
-add_task(withMockNormandyApi(async function testLoadActionSandboxManagers(mockApi) {
+decorate_task(
+  withMockNormandyApi,
+  async function testLoadActionSandboxManagers(mockApi) {
     mockApi.actions = [
       {name: "normalAction"},
       {name: "missingImpl"},
@@ -322,7 +334,8 @@ add_task(withMockNormandyApi(async function testLoadActionSandboxManagers(mockAp
       await normalManager.evalInSandbox("window.scriptRan"),
       "Implementations are run in the sandbox",
     );
-}));
+  }
+);
 
 // test init() in dev mode
 decorate_task(
@@ -402,8 +415,9 @@ decorate_task(
   withStub(RecipeRunner, "enable"),
   withStub(RecipeRunner, "disable"),
   withStub(CleanupManager, "addCleanupHandler"),
+  withStub(AddonStudies, "stop"),
 
-  async function testPrefWatching(runStub, enableStub, disableStub, addCleanupHandlerStub) {
+  async function testPrefWatching(runStub, enableStub, disableStub, addCleanupHandlerStub, stopStub) {
     await RecipeRunner.init();
     is(enableStub.callCount, 1, "Enable should be called initially");
     is(disableStub.callCount, 0, "Disable should not be called initially");

browser/extensions/shield-recipe-client/test/browser/browser_ShieldPreferences.js

@@ -6,14 +6,13 @@ Cu.import("resource://shield-recipe-client/lib/AddonStudies.jsm", this);
 const OPT_OUT_PREF = "app.shield.optoutstudies.enabled";
 
 decorate_task(
-  withPrefEnv({
-    set: [[OPT_OUT_PREF, true]],
-  }),
+  withMockPreferences,
   AddonStudies.withStudies([
     studyFactory({active: true}),
     studyFactory({active: true}),
   ]),
-  async function testDisableStudiesWhenOptOutDisabled([study1, study2]) {
+  async function testDisableStudiesWhenOptOutDisabled(mockPreferences, [study1, study2]) {
+    mockPreferences.set(OPT_OUT_PREF, true);
     const observers = [
       studyEndObserved(study1.recipeId),
       studyEndObserved(study2.recipeId),

browser/extensions/shield-recipe-client/test/browser/browser_bootstrap.js

@@ -31,12 +31,10 @@ const experimentPref3 = "test.initExperimentPrefs3";
 const experimentPref4 = "test.initExperimentPrefs4";
 
 decorate_task(
-  withPrefEnv({
-    clear: [[initPref1], [initPref2], [initPref3]],
-  }),
   withBootstrap,
   async function testInitShieldPrefs(Bootstrap) {
     const defaultBranch = Services.prefs.getDefaultBranch("");
+
     const prefDefaults = {
       [initPref1]: true,
       [initPref2]: 2,
@@ -74,6 +72,8 @@ decorate_task(
         `Pref ${pref} doesn't have a user value after being initialized.`,
       );
     }
+
+    defaultBranch.deleteBranch("test.");
   },
 );
 

browser/extensions/shield-recipe-client/test/browser/head.js

@@ -184,11 +184,27 @@ class MockPreferences {
     for (const [branchName, values] of Object.entries(this.oldValues)) {
       const preferenceBranch = preferenceBranches[branchName];
       for (const [name, {oldValue, existed}] of Object.entries(values)) {
+        const before = preferenceBranch.get(name);
+
+        if (before === oldValue) {
+          continue;
+        }
+
         if (existed) {
           preferenceBranch.set(name, oldValue);
+        } else if (branchName === "default") {
+          Services.prefs.getDefaultBranch(name).deleteBranch("");
         } else {
           preferenceBranch.reset(name);
         }
+
+        const after = preferenceBranch.get(name);
+        if (before === after && before !== undefined) {
+          throw new Error(
+            `Couldn't reset pref "${name}" to "${oldValue}" on "${branchName}" branch ` +
+            `(value stayed "${before}", did ${existed ? "" : "not "}exist)`
+          );
+        }
       }
     }
   }