Bug 1556391: Fix experiments that never expire r=mythmon a=jcristau
authorEthan Glasser-Camp <ethan@betacantrips.com>
Mon, 03 Jun 2019 23:45:52 +0000
changeset 536682 078657522e9e0c0db743a772efa0322d1e436057
parent 536681 aa64aae1b32d2f2a1cc1829c67da7e18c6899bd1
child 536683 e6273336eca1ebbe8e810c04e887f08ada963bf9
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmythmon, jcristau
bugs1556391
milestone68.0
Bug 1556391: Fix experiments that never expire r=mythmon a=jcristau Add a test that simulates two Normandy "runs" to verify that the fix actually works. While we are here, fix a bug in the handling of userFacingName/userFacingDescription. Differential Revision: https://phabricator.services.mozilla.com/D33537
toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
--- a/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
+++ b/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
@@ -40,16 +40,18 @@ class PreferenceExperimentAction extends
   }
 
   async _run(recipe) {
     const {
       branches,
       isHighPopulation,
       isEnrollmentPaused,
       slug,
+      userFacingName,
+      userFacingDescription,
     } = recipe.arguments;
 
     this.seenExperimentNames.push(slug);
 
     // If we're not in the experiment, enroll!
     const hasSlug = await PreferenceExperiments.has(slug);
     if (!hasSlug) {
       // Check all preferences that could be used by this experiment.
@@ -77,16 +79,18 @@ class PreferenceExperimentAction extends
       const branch = await this.chooseBranch(slug, branches);
       const experimentType = isHighPopulation ? "exp-highpop" : "exp";
       await PreferenceExperiments.start({
         name: slug,
         actionName: this.name,
         branch: branch.slug,
         preferences: branch.preferences,
         experimentType,
+        userFacingName,
+        userFacingDescription,
       });
     } else {
       // If the experiment exists, and isn't expired, bump the lastSeen date.
       const experiment = await PreferenceExperiments.get(slug);
       if (experiment.expired) {
         this.log.debug(`Experiment ${slug} has expired, aborting.`);
       } else {
         await PreferenceExperiments.markLastSeen(slug);
@@ -114,17 +118,17 @@ class PreferenceExperimentAction extends
    * End any experiments which we didn't see during this session.
    * This is the "normal" way experiments end, as they are disabled on
    * the server and so we stop seeing them.  This can also happen if
    * the user doesn't match the filter any more.
    */
   async _finalize() {
     const activeExperiments = await PreferenceExperiments.getAllActive();
     return Promise.all(activeExperiments.map(experiment => {
-      if (this.name != experiment.action) {
+      if (this.name != experiment.actionName) {
         // Another action is responsible for cleaning this one
         // up. Leave it alone.
         return null;
       }
 
       if (this.seenExperimentNames.includes(experiment.name)) {
         return null;
       }
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
@@ -138,16 +138,18 @@ decorate_task(
       preferences: {
         "fake.preference": {
           preferenceValue: "branch1",
           preferenceBranchType: "user",
           preferenceType: "string",
         },
       },
       experimentType: "exp",
+      userFacingName: "Super Cool Test Experiment",
+      userFacingDescription: "Test experiment from browser_actions_PreferenceExperimentAction.",
     }]]);
   }
 );
 
 decorate_task(
   withStub(PreferenceExperiments, "markLastSeen"),
   PreferenceExperiments.withMockExperiments([{name: "test", expired: false}]),
   async function markSeen_if_experiment_active(markLastSeenStub) {
@@ -192,18 +194,18 @@ decorate_task(
 
     Assert.deepEqual(startStub.args, [], "start was not called");
   }
 );
 
 decorate_task(
   withStub(PreferenceExperiments, "stop"),
   PreferenceExperiments.withMockExperiments([
-    {name: "seen", expired: false, action: "PreferenceExperimentAction"},
-    {name: "unseen", expired: false, action: "PreferenceExperimentAction"},
+    {name: "seen", expired: false, actionName: "PreferenceExperimentAction"},
+    {name: "unseen", expired: false, actionName: "PreferenceExperimentAction"},
   ]),
   async function stop_experiments_not_seen(stopStub) {
     const action = new PreferenceExperimentAction();
     const recipe = preferenceExperimentFactory({
       slug: "seen",
     });
 
     await action.runRecipe(recipe);
@@ -212,18 +214,18 @@ decorate_task(
     Assert.deepEqual(stopStub.args,
                      [["unseen", {resetValue: true, reason: "recipe-not-seen"}]]);
   }
 );
 
 decorate_task(
   withStub(PreferenceExperiments, "stop"),
   PreferenceExperiments.withMockExperiments([
-    {name: "seen", expired: false, action: "SinglePreferenceExperimentAction"},
-    {name: "unseen", expired: false, action: "SinglePreferenceExperimentAction"},
+    {name: "seen", expired: false, actionName: "SinglePreferenceExperimentAction"},
+    {name: "unseen", expired: false, actionName: "SinglePreferenceExperimentAction"},
   ]),
   async function dont_stop_experiments_for_other_action(stopStub) {
     const action = new PreferenceExperimentAction();
     const recipe = preferenceExperimentFactory({
       slug: "seen",
     });
 
     await action.runRecipe(recipe);
@@ -333,8 +335,79 @@ decorate_task(
       sandbox.restore();
     }
 
     Assert.deepEqual(ratioSampleStub.args,
                      [["fake-id-exp-slug-branch", [1, 2]]]);
     Assert.deepEqual(result, branches[1]);
   }
 );
+
+decorate_task(
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments([]),
+  async function integration_test_enroll_and_unenroll(prefs) {
+    prefs.set("fake.preference", "oldvalue", "user");
+    const recipe = preferenceExperimentFactory({
+      slug: "integration test experiment",
+      branches: [
+        {
+          slug: "branch1",
+          preferences: {
+            "fake.preference": {
+              preferenceBranchType: "user",
+              preferenceValue: "branch1",
+            },
+          },
+          ratio: 1,
+        },
+        {
+          slug: "branch2",
+          preferences: {
+            "fake.preference": {
+              preferenceBranchType: "user",
+              preferenceValue: "branch2",
+            },
+          },
+          ratio: 1,
+        },
+      ],
+      userFacingName: "userFacingName",
+      userFacingDescription: "userFacingDescription",
+    });
+
+    // Session 1: we see the above recipe and enroll in the experiment.
+    const action = new PreferenceExperimentAction();
+    sinon.stub(action, "chooseBranch").callsFake(async function(slug, branches) {
+      return branches[0];
+    });
+    await action.runRecipe(recipe);
+    await action.finalize();
+
+    const activeExperiments = await PreferenceExperiments.getAllActive();
+    ok(activeExperiments.length > 0);
+    Assert.deepEqual(activeExperiments, [{
+      name: "integration test experiment",
+      actionName: "PreferenceExperimentAction",
+      branch: "branch1",
+      preferences: {
+        "fake.preference": {
+          preferenceBranchType: "user",
+          preferenceValue: "branch1",
+          preferenceType: "string",
+          previousPreferenceValue: "oldvalue",
+        },
+      },
+      expired: false,
+      lastSeen: activeExperiments[0].lastSeen,  // can't predict date
+      experimentType: "exp",
+      userFacingName: "userFacingName",
+      userFacingDescription: "userFacingDescription",
+    }]);
+
+    // Session 2: recipe is filtered out and so does not run.
+    const action2 = new PreferenceExperimentAction();
+    await action2.finalize();
+
+    // Experiment should be unenrolled
+    Assert.deepEqual(await PreferenceExperiments.getAllActive(), []);
+  }
+);