Bug 1436701 - Handle resetValue / didResetValue correctly in Normany preference experiments r?Gijs draft
authorMike Cooper <mcooper@mozilla.com>
Thu, 08 Feb 2018 11:35:39 -0800
changeset 752681 a087a085aa25275f17459aa427e9281c074663d4
parent 752427 7144fcd531df304bea9bc2031fab6bc56c405095
push id98344
push userbmo:mcooper@mozilla.com
push dateThu, 08 Feb 2018 19:36:08 +0000
reviewersGijs
bugs1436701
milestone60.0a1
Bug 1436701 - Handle resetValue / didResetValue correctly in Normany preference experiments r?Gijs MozReview-Commit-ID: 1UfvmpgvaIx
browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm
browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
--- a/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm
+++ b/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm
@@ -182,17 +182,17 @@ this.PreferenceExperiments = {
     CleanupManager.addCleanupHandler(this.saveStartupPrefs.bind(this));
 
     for (const experiment of await this.getAllActive()) {
       // Check that the current value of the preference is still what we set it to
       if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType) !== experiment.preferenceValue) {
         // if not, stop the experiment, and skip the remaining steps
         log.info(`Stopping experiment "${experiment.name}" because its value changed`);
         await this.stop(experiment.name, {
-          didResetValue: false,
+          resetValue: false,
           reason: "user-preference-changed-sideload",
         });
         continue;
       }
 
       // Notify Telemetry of experiments we're running, since they don't persist between restarts
       TelemetryEnvironment.setExperimentActive(
         experiment.name,
@@ -461,17 +461,17 @@ this.PreferenceExperiments = {
    * @param {String} [options.reason = "unknown"]
    *   Reason that the experiment is ending. Optional, defaults to
    *   "unknown".
    * @rejects {Error}
    *   If there is no stored experiment with the given name, or if the
    *   experiment has already expired.
    */
   async stop(experimentName, {resetValue = true, reason = "unknown"} = {}) {
-    log.debug(`PreferenceExperiments.stop(${experimentName})`);
+    log.debug(`PreferenceExperiments.stop(${experimentName}, {resetValue: ${resetValue}, reason: ${reason}})`);
     if (reason === "unknown") {
       log.warn(`experiment ${experimentName} ending for unknown reason`);
     }
 
     const store = await ensureStorage();
     if (!(experimentName in store.data)) {
       throw new Error(`Could not find a preference experiment named "${experimentName}"`);
     }
--- a/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
+++ b/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
@@ -789,28 +789,36 @@ decorate_task(
   },
 );
 
 // Experiments should end if the preference has been changed when init() is called
 decorate_task(
   withMockExperiments,
   withMockPreferences,
   withStub(PreferenceExperiments, "stop"),
-  withStub(TelemetryEvents, "sendEvent"),
-  async function testInitChanges(experiments, mockPreferences, stopStub, sendEventStub) {
+  async function testInitChanges(experiments, mockPreferences, stopStub) {
     mockPreferences.set("fake.preference", "experiment value", "default");
     experiments.test = experimentFactory({
       name: "test",
       preferenceName: "fake.preference",
       preferenceValue: "experiment value",
     });
     mockPreferences.set("fake.preference", "changed value");
     await PreferenceExperiments.init();
-    ok(stopStub.calledWith("test"), "Experiment is stopped because value changed");
+
     is(Preferences.get("fake.preference"), "changed value", "Preference value was not changed");
+
+    Assert.deepEqual(
+      stopStub.getCall(0).args,
+      ["test", {
+        resetValue: false,
+        reason: "user-preference-changed-sideload",
+      }],
+      "Experiment is stopped correctly because value changed"
+    );
   },
 );
 
 // init should register an observer for experiments
 decorate_task(
   withMockExperiments,
   withMockPreferences,
   withStub(PreferenceExperiments, "startObserver"),
@@ -1001,8 +1009,37 @@ decorate_task(
     await PreferenceExperiments.stop("test");
     is(
       sendEventStub.getCall(0).args[3].reason,
       "unknown",
       "PreferenceExperiments.stop() should use unknown as the default reason",
     );
   }
 );
+
+// stop should pass along the value for resetValue to Telemetry Events as didResetValue
+decorate_task(
+  withMockExperiments,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "stopObserver"),
+  withStub(TelemetryEvents, "sendEvent"),
+  async function testStopResetValue(experiments, mockPreferences, stopObserverStub, sendEventStub) {
+    mockPreferences.set("fake.preference1", "default value", "default");
+    experiments.test1 = experimentFactory({ name: "test1", preferenceName: "fake.preference1" });
+    await PreferenceExperiments.stop("test1", {resetValue: true});
+    is(sendEventStub.callCount, 1);
+    is(
+      sendEventStub.getCall(0).args[3].didResetValue,
+      "true",
+      "PreferenceExperiments.stop() should pass true values of resetValue as didResetValue",
+    );
+
+    mockPreferences.set("fake.preference2", "default value", "default");
+    experiments.test2 = experimentFactory({ name: "test2", preferenceName: "fake.preference2" });
+    await PreferenceExperiments.stop("test2", {resetValue: false});
+    is(sendEventStub.callCount, 2);
+    is(
+      sendEventStub.getCall(1).args[3].didResetValue,
+      "false",
+      "PreferenceExperiments.stop() should pass false values of resetValue as didResetValue",
+    );
+  }
+);