Backed out 3 changesets (bug 1604365, bug 1604367, bug 1618408) for causing xpcshell failures in test_addon_unenroll.js CLOSED TREE

Backed out changeset da98cf612f95 (bug 1618408)
Backed out changeset d0267467fe87 (bug 1604365)
Backed out changeset 34215be38526 (bug 1604367)

toolkit/components/normandy/NormandyMigrations.jsm

@@ -60,14 +60,12 @@ const NormandyMigrations = {
   migrations: [
     migrateShieldPrefs,
     migrateStudiesEnabledWithoutHealthReporting,
-    AddonStudies.migrations
-      .migration01AddonStudyFieldsToSlugAndUserFacingFields,
+    AddonStudies.migrateAddonStudyFieldsToSlugAndUserFacingFields,
     PreferenceExperiments.migrations.migration01MoveExperiments,
     PreferenceExperiments.migrations.migration02MultiPreference,
     PreferenceExperiments.migrations.migration03AddActionName,
     PreferenceExperiments.migrations.migration04RenameNameToSlug,
     RecipeRunner.migrations.migration01RemoveOldRecipesCollection,
-    AddonStudies.migrations.migration02RemoveOldAddonStudyAction,
   ],
 };
 

toolkit/components/normandy/actions/AddonStudyAction.jsm

@@ -0,0 +1,113 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
+const { BranchedAddonStudyAction } = ChromeUtils.import(
+  "resource://normandy/actions/BranchedAddonStudyAction.jsm"
+);
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  ActionSchemas: "resource://normandy/actions/schemas/index.js",
+  AddonStudies: "resource://normandy/lib/AddonStudies.jsm",
+});
+
+var EXPORTED_SYMBOLS = ["AddonStudyAction"];
+
+/*
+ * This action was originally the only form of add-on studies. Later, a version
+ * of add-on studies was addded that supported having more than one
+ * experimental branch per study, instead of relying on the installed add-on to
+ * manage its branches. To reduce duplicated code, the no-branches version of
+ * the action inherits from the multi-branch version.
+ *
+ * The schemas of the arguments for these two actions are different. As well as
+ * supporting branches within the study, the multi-branch version also changed
+ * its metadata fields to better match the use cases of studies.
+ *
+ * This action translates a legacy no branches study into a single branched
+ * study with the proper metadata. This should be considered a temporary
+ * measure, and eventually all studies will be native multi-branch studies.
+ *
+ * The existing schema can't be changed, because these legacy recipes are also
+ * sent by the server to older clients that don't support the newer schema
+ * format.
+ */
+
+class AddonStudyAction extends BranchedAddonStudyAction {
+  get schema() {
+    return ActionSchemas["addon-study"];
+  }
+
+  /**
+   * This hook is executed once for each recipe that currently applies to this
+   * client. It is responsible for:
+   *
+   *   - Translating recipes to match BranchedAddonStudy's schema
+   *   - Validating that transformation
+   *   - Calling BranchedAddonStudy's _run hook.
+   *
+   * If the recipe fails to enroll or update, it should throw to properly
+   * report its status.
+   */
+  async _run(recipe) {
+    const args = recipe.arguments; // save some typing
+
+    /*
+     * The argument schema of no-branches add-ons don't include a separate slug
+     * and name, and use different names for the description. Convert from the
+     * old to the new one.
+     */
+    let transformedArguments = {
+      slug: args.name,
+      userFacingName: args.name,
+      userFacingDescription: args.description,
+      isEnrollmentPaused: !!args.isEnrollmentPaused,
+      branches: [
+        {
+          slug: AddonStudies.NO_BRANCHES_MARKER,
+          ratio: 1,
+          extensionApiId: recipe.arguments.extensionApiId,
+        },
+      ],
+    };
+
+    // This will throw if the arguments aren't valid, and BaseAction will catch it.
+    transformedArguments = this.validateArguments(
+      transformedArguments,
+      ActionSchemas["branched-addon-study"]
+    );
+
+    const transformedRecipe = { ...recipe, arguments: transformedArguments };
+    return super._run(transformedRecipe);
+  }
+
+  /**
+   * This hook is executed once after all recipes that apply to this client
+   * have been processed. It is responsible for unenrolling the client from any
+   * studies that no longer apply, based on this.seenRecipeIds, which is set by
+   * the super class.
+   */
+  async _finalize() {
+    const activeStudies = await AddonStudies.getAllActive({
+      branched: AddonStudies.FILTER_NOT_BRANCHED,
+    });
+
+    for (const study of activeStudies) {
+      if (!this.seenRecipeIds.has(study.recipeId)) {
+        this.log.debug(
+          `Stopping non-branched add-on study for recipe ${study.recipeId}`
+        );
+        try {
+          await this.unenroll(study.recipeId, "recipe-not-seen");
+        } catch (err) {
+          Cu.reportError(err);
+        }
+      }
+    }
+  }
+}

toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm

@@ -0,0 +1,95 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { PreferenceExperimentAction } = ChromeUtils.import(
+  "resource://normandy/actions/PreferenceExperimentAction.jsm"
+);
+ChromeUtils.defineModuleGetter(
+  this,
+  "ActionSchemas",
+  "resource://normandy/actions/schemas/index.js"
+);
+ChromeUtils.defineModuleGetter(
+  this,
+  "JsonSchemaValidator",
+  "resource://gre/modules/components-utils/JsonSchemaValidator.jsm"
+);
+
+var EXPORTED_SYMBOLS = ["SinglePreferenceExperimentAction"];
+
+/**
+ * The backwards-compatible version of the preference experiment that
+ * can only accept a single preference.
+ */
+class SinglePreferenceExperimentAction extends PreferenceExperimentAction {
+  get schema() {
+    return ActionSchemas["single-preference-experiment"];
+  }
+
+  async _run(recipe) {
+    const {
+      preferenceBranchType,
+      preferenceName,
+      preferenceType,
+      branches,
+      ...remainingArguments
+    } = recipe.arguments;
+
+    const newArguments = {
+      // The multi-preference-experiment schema requires a string
+      // name/description, which are necessary in the wire format, but
+      // experiment objects can have null for these fields. Add some
+      // filler fields here and remove them after validation.
+      userFacingName: "temp-name",
+      userFacingDescription: "temp-description",
+      ...remainingArguments,
+      branches: branches.map(branch => {
+        const { value, ...branchProps } = branch;
+        return {
+          ...branchProps,
+          preferences: {
+            [preferenceName]: {
+              preferenceBranchType,
+              preferenceType,
+              preferenceValue: value,
+            },
+          },
+        };
+      }),
+    };
+
+    const multiprefSchema = ActionSchemas["multi-preference-experiment"];
+
+    let [
+      valid,
+      validatedArguments,
+    ] = JsonSchemaValidator.validateAndParseParameters(
+      newArguments,
+      multiprefSchema
+    );
+    if (!valid) {
+      throw new Error(
+        `Transformed arguments do not match schema. Original arguments: ${JSON.stringify(
+          recipe.arguments
+        )}, new arguments: ${JSON.stringify(
+          newArguments
+        )}, schema: ${JSON.stringify(multiprefSchema)}`
+      );
+    }
+
+    validatedArguments.userFacingName = null;
+    validatedArguments.userFacingDescription = null;
+
+    recipe.arguments = validatedArguments;
+
+    const newRecipe = {
+      ...recipe,
+      arguments: validatedArguments,
+    };
+
+    return super._run(newRecipe);
+  }
+}

toolkit/components/normandy/content/AboutPages.jsm

@@ -15,8 +15,8 @@ ChromeUtils.defineModuleGetter(
 );
 ChromeUtils.defineModuleGetter(
   this,
-  "BranchedAddonStudyAction",
-  "resource://normandy/actions/BranchedAddonStudyAction.jsm"
+  "AddonStudyAction",
+  "resource://normandy/actions/AddonStudyAction.jsm"
 );
 ChromeUtils.defineModuleGetter(
   this,
@@ -159,7 +159,7 @@ XPCOMUtils.defineLazyGetter(AboutPages, "aboutStudies", () => {
      */
     async removeAddonStudy(recipeId, reason) {
       try {
-        const action = new BranchedAddonStudyAction();
+        const action = new AddonStudyAction();
         await action.unenroll(recipeId, reason);
       } catch (err) {
         // If the exception was that the study was already removed, that's ok.

toolkit/components/normandy/lib/ActionsManager.jsm

@@ -12,6 +12,7 @@ const { LogManager } = ChromeUtils.import(
 XPCOMUtils.defineLazyModuleGetters(this, {
   AddonRollbackAction: "resource://normandy/actions/AddonRollbackAction.jsm",
   AddonRolloutAction: "resource://normandy/actions/AddonRolloutAction.jsm",
+  AddonStudyAction: "resource://normandy/actions/AddonStudyAction.jsm",
   BaseAction: "resource://normandy/actions/BaseAction.jsm",
   BranchedAddonStudyAction:
     "resource://normandy/actions/BranchedAddonStudyAction.jsm",
@@ -23,6 +24,8 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   PreferenceRolloutAction:
     "resource://normandy/actions/PreferenceRolloutAction.jsm",
   ShowHeartbeatAction: "resource://normandy/actions/ShowHeartbeatAction.jsm",
+  SinglePreferenceExperimentAction:
+    "resource://normandy/actions/SinglePreferenceExperimentAction.jsm",
   Uptake: "resource://normandy/lib/Uptake.jsm",
 });
 
@@ -31,6 +34,7 @@ var EXPORTED_SYMBOLS = ["ActionsManager"];
 const log = LogManager.getLogger("recipe-runner");
 
 const actionConstructors = {
+  "addon-study": AddonStudyAction,
   "addon-rollback": AddonRollbackAction,
   "addon-rollout": AddonRolloutAction,
   "branched-addon-study": BranchedAddonStudyAction,
@@ -39,6 +43,13 @@ const actionConstructors = {
   "preference-rollback": PreferenceRollbackAction,
   "preference-rollout": PreferenceRolloutAction,
   "show-heartbeat": ShowHeartbeatAction,
+  "single-preference-experiment": SinglePreferenceExperimentAction,
+};
+
+// Legacy names used by the server and older clients for actions.
+const actionAliases = {
+  "opt-out-study": "addon-study",
+  "preference-experiment": "single-preference-experiment",
 };
 
 /**
@@ -48,10 +59,15 @@ class ActionsManager {
   constructor() {
     this.finalized = false;
 
+    // Build a set of local actions, and aliases to them. The aliased names are
+    // used by the server to keep compatibility with older clients.
     this.localActions = {};
     for (const [name, Constructor] of Object.entries(actionConstructors)) {
       this.localActions[name] = new Constructor();
     }
+    for (const [alias, target] of Object.entries(actionAliases)) {
+      this.localActions[alias] = this.localActions[target];
+    }
   }
 
   static getCapabilities() {
@@ -60,6 +76,9 @@ class ActionsManager {
     for (const actionName of Object.keys(actionConstructors)) {
       capabilities.add(`action.${actionName}`);
     }
+    for (const actionAlias of Object.keys(actionAliases)) {
+      capabilities.add(`action.${actionAlias}`);
+    }
     return capabilities;
   }
 
@@ -89,7 +108,7 @@ class ActionsManager {
     this.finalized = true;
 
     // Finalize local actions
-    for (const action of Object.values(this.localActions)) {
+    for (const action of new Set(Object.values(this.localActions))) {
       action.finalize();
     }
   }

toolkit/components/normandy/lib/AddonStudies.jsm

@@ -52,11 +52,6 @@ ChromeUtils.defineModuleGetter(
   "AddonManager",
   "resource://gre/modules/AddonManager.jsm"
 );
-ChromeUtils.defineModuleGetter(
-  this,
-  "BranchedAddonStudyAction",
-  "resource://normandy/actions/BranchedAddonStudyAction.jsm"
-);
 ChromeUtils.defineModuleGetter(
   this,
   "CleanupManager",
@@ -200,76 +195,51 @@ var AddonStudies = {
   },
 
   /**
-   * These migrations should only be called from `NormandyMigrations.jsm` and
-   * tests.
+   * Change from "name" and "description" to "slug", "userFacingName",
+   * and "userFacingDescription".
+   *
+   * This is called as needed by NormandyMigrations.jsm, which handles tracking
+   * if this migration has already been run.
    */
-  migrations: {
-    /**
-     * Change from "name" and "description" to "slug", "userFacingName",
-     * and "userFacingDescription".
-     */
-    async migration01AddonStudyFieldsToSlugAndUserFacingFields() {
-      const db = await getDatabase();
-      const studies = await db.objectStore(STORE_NAME, "readonly").getAll();
+  async migrateAddonStudyFieldsToSlugAndUserFacingFields() {
+    const db = await getDatabase();
+    const studies = await db.objectStore(STORE_NAME, "readonly").getAll();
 
-      // If there are no studies, stop here to avoid opening the DB again.
-      if (studies.length === 0) {
-        return;
+    // If there are no studies, stop here to avoid opening the DB again.
+    if (studies.length === 0) {
+      return;
+    }
+
+    // Object stores expire after `await`, so this method accumulates a bunch of
+    // promises, and then awaits them at the end.
+    const writePromises = [];
+    const objectStore = db.objectStore(STORE_NAME, "readwrite");
+
+    for (const study of studies) {
+      // use existing name as slug
+      if (!study.slug) {
+        study.slug = study.name;
       }
 
-      // Object stores expire after `await`, so this method accumulates a bunch of
-      // promises, and then awaits them at the end.
-      const writePromises = [];
-      const objectStore = db.objectStore(STORE_NAME, "readwrite");
+      // Rename `name` and `description` as `userFacingName` and `userFacingDescription`
+      if (study.name && !study.userFacingName) {
+        study.userFacingName = study.name;
+      }
+      delete study.name;
+      if (study.description && !study.userFacingDescription) {
+        study.userFacingDescription = study.description;
+      }
+      delete study.description;
 
-      for (const study of studies) {
-        // use existing name as slug
-        if (!study.slug) {
-          study.slug = study.name;
-        }
-
-        // Rename `name` and `description` as `userFacingName` and `userFacingDescription`
-        if (study.name && !study.userFacingName) {
-          study.userFacingName = study.name;
-        }
-        delete study.name;
-        if (study.description && !study.userFacingDescription) {
-          study.userFacingDescription = study.description;
-        }
-        delete study.description;
-
-        // Specify that existing recipes don't have branches
-        if (!study.branch) {
-          study.branch = AddonStudies.NO_BRANCHES_MARKER;
-        }
-
-        writePromises.push(objectStore.put(study));
+      // Specify that existing recipes don't have branches
+      if (!study.branch) {
+        study.branch = AddonStudies.NO_BRANCHES_MARKER;
       }
 
-      await Promise.all(writePromises);
-    },
+      writePromises.push(objectStore.put(study));
+    }
 
-    async migration02RemoveOldAddonStudyAction() {
-      const studies = await AddonStudies.getAllActive({
-        branched: AddonStudies.FILTER_NOT_BRANCHED,
-      });
-      if (!studies.length) {
-        return;
-      }
-      const action = new BranchedAddonStudyAction();
-      for (const study of studies) {
-        try {
-          await action.unenroll(
-            study.recipeId,
-            "migration-removing-unbranched-action"
-          );
-        } catch (e) {
-          log.error(
-            `Stopping add-on study ${study.slug} during migration failed: ${e}`
-          );
-        }
-      }
-    },
+    await Promise.all(writePromises);
   },
 
   /**

toolkit/components/normandy/lib/PreferenceExperiments.jsm

@@ -47,9 +47,6 @@
  *   A random ID generated at time of enrollment. It should be included on all
  *   telemetry related to this experiment. It should not be re-used by other
  *   studies, or any other purpose. May be null on old experiments.
- * @property {string} actionName
- *   The action who knows about this experiment and is responsible for cleaning
- *   it up. This should correspond to the `name` of some BaseAction subclass.
  */
 
 /**
@@ -944,25 +941,5 @@ var PreferenceExperiments = {
       }
       storage.saveSoon();
     },
-
-    async migration05RemoveOldAction() {
-      const experiments = await PreferenceExperiments.getAllActive();
-      for (const experiment of experiments) {
-        if (experiment.actionName == "SinglePreferenceExperimentAction") {
-          try {
-            await PreferenceExperiments.stop(experiment.slug, {
-              resetValue: true,
-              reason: "migration-removing-single-pref-action",
-            });
-          } catch (e) {
-            log.error(
-              `Stopping preference experiment ${
-                experiment.slug
-              } during migration failed: ${e}`
-            );
-          }
-        }
-      }
-    },
   },
 };

toolkit/components/normandy/lib/ShieldPreferences.jsm

@@ -9,8 +9,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   Services: "resource://gre/modules/Services.jsm",
-  BranchedAddonStudyAction:
-    "resource://normandy/actions/BranchedAddonStudyAction.jsm",
+  AddonStudyAction: "resource://normandy/actions/AddonStudyAction.jsm",
   AddonStudies: "resource://normandy/lib/AddonStudies.jsm",
   CleanupManager: "resource://normandy/lib/CleanupManager.jsm",
   PreferenceExperiments: "resource://normandy/lib/PreferenceExperiments.jsm",
@@ -49,7 +48,7 @@ var ShieldPreferences = {
       case PREF_OPT_OUT_STUDIES_ENABLED: {
         prefValue = Services.prefs.getBoolPref(PREF_OPT_OUT_STUDIES_ENABLED);
         if (!prefValue) {
-          const action = new BranchedAddonStudyAction();
+          const action = new AddonStudyAction();
           const studyPromises = (await AddonStudies.getAll()).map(study => {
             if (!study.active) {
               return null;

toolkit/components/normandy/test/NormandyTestUtils.jsm

@@ -104,7 +104,6 @@ const NormandyTestUtils = {
           lastSeen: new Date().toJSON(),
           experimentType: "exp",
           enrollmentId: NormandyUtils.generateUuid(),
-          actionName: "PreferenceExperimentAction",
         },
         attrs,
         {

toolkit/components/normandy/test/browser/browser.ini

@@ -14,12 +14,14 @@ head = head.js
 [browser_about_studies.js]
 [browser_actions_AddonRollbackAction.js]
 [browser_actions_AddonRolloutAction.js]
+[browser_actions_AddonStudyAction.js]
 [browser_actions_BranchedAddonStudyAction.js]
 [browser_actions_ConsoleLogAction.js]
 [browser_actions_PreferenceExperimentAction.js]
 [browser_actions_PreferenceRolloutAction.js]
 [browser_actions_PreferenceRollbackAction.js]
 [browser_actions_ShowHeartbeatAction.js]
+[browser_actions_SinglePreferenceExperimentAction.js]
 [browser_ActionsManager.js]
 [browser_AddonRollouts.js]
 [browser_AddonStudies.js]

toolkit/components/normandy/test/browser/browser_AddonStudies.js

@@ -233,7 +233,6 @@ decorate_task(
   }
 );
 
-// Test that AddonStudies.init() ends studies that have been uninstalled
 decorate_task(
   AddonStudies.withStudies([
     addonStudyFactory({
@@ -264,44 +263,3 @@ decorate_task(
     );
   }
 );
-
-decorate_task(
-  AddonStudies.withStudies([
-    NormandyTestUtils.factories.addonStudyFactory({ active: true }),
-    NormandyTestUtils.factories.branchedAddonStudyFactory(),
-  ]),
-  async function testRemoveOldAddonStudies([noBranchStudy, branchedStudy]) {
-    // pre check, both studies are active
-    const preActiveIds = (await AddonStudies.getAllActive()).map(
-      addon => addon.recipeId
-    );
-    Assert.deepEqual(
-      preActiveIds,
-      [noBranchStudy.recipeId, branchedStudy.recipeId],
-      "Both studies should be active"
-    );
-
-    // run the migration
-    await AddonStudies.migrations.migration02RemoveOldAddonStudyAction();
-
-    // The unbrached study should end
-    const postActiveIds = (await AddonStudies.getAllActive()).map(
-      addon => addon.recipeId
-    );
-    Assert.deepEqual(
-      postActiveIds,
-      [branchedStudy.recipeId],
-      "The unbranched study should end"
-    );
-
-    // But both studies should still be present
-    const postAllIds = (await AddonStudies.getAll()).map(
-      addon => addon.recipeId
-    );
-    Assert.deepEqual(
-      postAllIds,
-      [noBranchStudy.recipeId, branchedStudy.recipeId],
-      "Both studies should still be present"
-    );
-  }
-);

toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js

@@ -181,30 +181,6 @@ const mockV5Data = {
   },
 };
 
-const migrationsInfo = [
-  {
-    migration: PreferenceExperiments.migrations.migration01MoveExperiments,
-    dataBefore: mockV1Data,
-    dataAfter: mockV2Data,
-  },
-  {
-    migration: PreferenceExperiments.migrations.migration02MultiPreference,
-    dataBefore: mockV2Data,
-    dataAfter: mockV3Data,
-  },
-  {
-    migration: PreferenceExperiments.migrations.migration03AddActionName,
-    dataBefore: mockV3Data,
-    dataAfter: mockV4Data,
-  },
-  {
-    migration: PreferenceExperiments.migrations.migration04RenameNameToSlug,
-    dataBefore: mockV4Data,
-    dataAfter: mockV5Data,
-  },
-  // Migration 5 is not a simple data migration. This style of tests does not apply to it.
-];
-
 /**
  * Make a mock `JsonFile` object with a no-op `saveSoon` method and a deep copy
  * of the data passed.
@@ -220,30 +196,27 @@ function makeMockJsonFile(data = {}) {
 
 /** Test that each migration results in the expected data */
 add_task(async function test_migrations() {
-  for (const { migration, dataAfter, dataBefore } of migrationsInfo) {
-    let mockJsonFile = makeMockJsonFile(dataBefore);
-    await migration(mockJsonFile);
-    Assert.deepEqual(
-      mockJsonFile.data,
-      dataAfter,
-      `Migration ${migration.name} should result in the expected data`
-    );
-  }
-});
+  let mockJsonFile = makeMockJsonFile(mockV1Data);
+  await PreferenceExperiments.migrations.migration01MoveExperiments(
+    mockJsonFile
+  );
+  Assert.deepEqual(mockJsonFile.data, mockV2Data);
 
-add_task(async function migrations_are_idempotent() {
-  for (const { migration, dataBefore } of migrationsInfo) {
-    const mockJsonFileOnce = makeMockJsonFile(dataBefore);
-    const mockJsonFileTwice = makeMockJsonFile(dataBefore);
-    await migration(mockJsonFileOnce);
-    await migration(mockJsonFileTwice);
-    await migration(mockJsonFileTwice);
-    Assert.deepEqual(
-      mockJsonFileOnce.data,
-      mockJsonFileTwice.data,
-      "migrating data twice should be idempotent for " + migration.name
-    );
-  }
+  mockJsonFile = makeMockJsonFile(mockV2Data);
+  await PreferenceExperiments.migrations.migration02MultiPreference(
+    mockJsonFile
+  );
+  Assert.deepEqual(mockJsonFile.data, mockV3Data);
+
+  mockJsonFile = makeMockJsonFile(mockV3Data);
+  await PreferenceExperiments.migrations.migration03AddActionName(mockJsonFile);
+  Assert.deepEqual(mockJsonFile.data, mockV4Data);
+
+  mockJsonFile = makeMockJsonFile(mockV4Data);
+  await PreferenceExperiments.migrations.migration04RenameNameToSlug(
+    mockJsonFile
+  );
+  Assert.deepEqual(mockJsonFile.data, mockV5Data);
 });
 
 add_task(async function migration03KeepsActionName() {
@@ -258,51 +231,26 @@ add_task(async function migration03KeepsActionName() {
   Assert.deepEqual(mockJsonFile.data, migratedData);
 });
 
-// Test that migration 5 works as expected
-decorate_task(
-  PreferenceExperiments.withMockExperiments([
-    NormandyTestUtils.factories.preferenceStudyFactory({
-      actionName: "PreferenceExperimentAction",
-      expired: false,
-    }),
-    NormandyTestUtils.factories.preferenceStudyFactory({
-      actionName: "SinglePreferenceExperimentAction",
-      expired: false,
-    }),
-  ]),
-  async function migration05Works([expKeep, expExpire]) {
-    // pre check
-    const activeSlugsBefore = (await PreferenceExperiments.getAllActive()).map(
-      e => e.slug
-    );
+add_task(async function migrations_are_idempotent() {
+  let dataVersions = [
+    [PreferenceExperiments.migrations.migration01MoveExperiments, mockV1Data],
+    [PreferenceExperiments.migrations.migration02MultiPreference, mockV2Data],
+    [PreferenceExperiments.migrations.migration03AddActionName, mockV3Data],
+    [PreferenceExperiments.migrations.migration04RenameNameToSlug, mockV4Data],
+  ];
+  for (const [migration, mockOldData] of dataVersions) {
+    const mockJsonFileOnce = makeMockJsonFile(mockOldData);
+    const mockJsonFileTwice = makeMockJsonFile(mockOldData);
+    await migration(mockJsonFileOnce);
+    await migration(mockJsonFileTwice);
+    await migration(mockJsonFileTwice);
     Assert.deepEqual(
-      activeSlugsBefore,
-      [expKeep.slug, expExpire.slug],
-      "Both experiments should be present and active before the migration"
-    );
-
-    // run the migration
-    await PreferenceExperiments.migrations.migration05RemoveOldAction();
-
-    // verify behavior
-    const activeSlugsAfter = (await PreferenceExperiments.getAllActive()).map(
-      e => e.slug
-    );
-    Assert.deepEqual(
-      activeSlugsAfter,
-      [expKeep.slug],
-      "The single pref experiment should be ended by the migration"
-    );
-    const allSlugsAfter = (await PreferenceExperiments.getAll()).map(
-      e => e.slug
-    );
-    Assert.deepEqual(
-      allSlugsAfter,
-      [expKeep.slug, expExpire.slug],
-      "Both experiments should still exist after the migration"
+      mockJsonFileOnce.data,
+      mockJsonFileTwice.data,
+      "migrating data twice should be idempotent for " + migration.name
     );
   }
-);
+});
 
 // clearAllExperimentStorage
 decorate_task(

toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js

@@ -0,0 +1,116 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+ChromeUtils.import(
+  "resource://gre/modules/components-utils/Sampling.jsm",
+  this
+);
+ChromeUtils.import("resource://gre/modules/Services.jsm", this);
+ChromeUtils.import("resource://gre/modules/Preferences.jsm", this);
+ChromeUtils.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
+ChromeUtils.import("resource://normandy/actions/BaseAction.jsm", this);
+ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this);
+ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
+ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
+ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
+ChromeUtils.import(
+  "resource://normandy/actions/PreferenceExperimentAction.jsm",
+  this
+);
+ChromeUtils.import(
+  "resource://normandy/actions/SinglePreferenceExperimentAction.jsm",
+  this
+);
+
+function argumentsFactory(args) {
+  return {
+    slug: "test",
+    preferenceName: "fake.preference",
+    preferenceType: "string",
+    preferenceBranchType: "default",
+    branches: [{ slug: "test", value: "foo", ratio: 1 }],
+    isHighPopulation: false,
+    ...args,
+  };
+}
+
+function preferenceExperimentFactory(args) {
+  return recipeFactory({
+    name: "preference-experiment",
+    arguments: argumentsFactory(args),
+  });
+}
+
+decorate_task(
+  withStudiesEnabled,
+  withStub(PreferenceExperimentAction.prototype, "_run"),
+  PreferenceExperiments.withMockExperiments([]),
+  async function enroll_user_if_never_been_in_experiment(runStub) {
+    const action = new SinglePreferenceExperimentAction();
+    const recipe = preferenceExperimentFactory({
+      slug: "test",
+      preferenceName: "fake.preference",
+      preferenceBranchType: "user",
+      branches: [
+        {
+          slug: "branch1",
+          value: "branch1",
+          ratio: 1,
+        },
+        {
+          slug: "branch2",
+          value: "branch2",
+          ratio: 1,
+        },
+      ],
+    });
+    sinon
+      .stub(action, "chooseBranch")
+      .callsFake(async function(slug, branches) {
+        return branches[0];
+      });
+
+    await action.processRecipe(recipe, BaseAction.suitability.FILTER_MATCH);
+    await action.finalize();
+
+    Assert.deepEqual(runStub.args, [
+      [
+        {
+          id: recipe.id,
+          name: "preference-experiment",
+          arguments: {
+            slug: "test",
+            userFacingName: null,
+            userFacingDescription: null,
+            isHighPopulation: false,
+            branches: [
+              {
+                slug: "branch1",
+                ratio: 1,
+                preferences: {
+                  "fake.preference": {
+                    preferenceValue: "branch1",
+                    preferenceType: "string",
+                    preferenceBranchType: "user",
+                  },
+                },
+              },
+              {
+                slug: "branch2",
+                ratio: 1,
+                preferences: {
+                  "fake.preference": {
+                    preferenceValue: "branch2",
+                    preferenceType: "string",
+                    preferenceBranchType: "user",
+                  },
+                },
+              },
+            ],
+          },
+        },
+      ],
+    ]);
+  }
+);