Watch experiment prefs #684

mythmon · 2017-04-12T18:15:22Z

This is based on #674, to use the addition of the init method to PreferenceExperiments. The relevant commit is 13628fa.

Fixes #645.

The issue mentioned "Register the preference observer that monitors for changes to the preference." This was already done.

Osmose · 2017-04-12T18:22:32Z

The issue mentioned "Register the preference observer that monitors for changes to the preference." This was already done.

We also need to register the observer on startup, since it doesn't persist and there's a gap between when we check the value on startup and when the recipes are executed.

gijsk · 2017-04-12T18:22:11Z

recipe-client-addon/lib/PreferenceExperiments.jsm

-      const {name, branch, expired, preferenceBranchType, preferenceName, preferenceValue} = experiment;
-
+    Object.values(store.data)
+    .filter(experiment => !experiment.expired)


Firefox style would be indenting this and the next line.

Osmose

Looks good except for my earlier comment about registering the preference watcher on startup. Nice work!

mythmon · 2017-04-14T16:59:14Z

I've made the requested changes, and re-based this on mozilla/preference-experiments, so it includes the changes from #674. This is ready for review, and if approved, ready for merge.

Osmose

Looks good, just some minor changes are needed, I think. Nice work!

Osmose · 2017-04-14T17:27:42Z

recipe-client-addon/lib/PreferenceExperiments.jsm

-      const {name, branch, expired, preferenceBranchType, preferenceName, preferenceValue} = experiment;
+    Object.values(store.data)
+      .filter(experiment => !experiment.expired)
+      .forEach(experiment => {


You can just use PreferenceExperiments.getAllActive() for this.

Osmose · 2017-04-14T17:28:15Z

recipe-client-addon/lib/PreferenceExperiments.jsm

+        // Check that the current value of the preference is still what we set it to
+        if (Preferences.get(experiment.preferenceName, undefined) !== experiment.preferenceValue) {
+          // if not, stop the experiment, and skip the remaining steps
+          this.stop(experiment.experimentName, false);


I think the this keyword is broken in functions wrapped with wrapAsync, but I have a commit lying around that fixes that: Osmose@2bf5624

Osmose · 2017-04-14T17:29:05Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

+
+// init should register an observer for experiments
+add_task(withMockExperiments(withMockPreferences(async function testInitRegistersObserver(experiments, mockPreferences) {
+  dump("@@ starting test");


Leftover debugging!

Osmose · 2017-04-14T17:30:13Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

+  );
+
+  startObserver.restore();
+  ok(false);


That ain't ok

Osmose · 2017-04-14T17:31:24Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

+  });
+  mockPreferences.set("fake.preference", "changed value");
+  await PreferenceExperiments.init();
+  ok(experiments["test"].expired, "Experiment expires because value changed");


Maybe we should mock stop instead, in case its implementation changes later?

Osmose

I understand that the dependencies here are difficult, but it would be a lot easier to review this if we split out the wrapAsync and test mochitest port into separate PRs. Up to you if you want to avoid that, though.

Osmose · 2017-04-14T21:28:53Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

+    preferenceValue: "experiment value",
+    expired: false,
+    preferenceBranchType: "default",
+  });


Any reason why you need this extra test data?

Osmose · 2017-04-14T21:29:24Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

  experiments["test"] = experimentFactory({
    name: "test",
    preferenceName: "fake.preference",
    preferenceValue: "experiment value",
  });
  mockPreferences.set("fake.preference", "changed value");
  await PreferenceExperiments.init();
-  ok(experiments["test"].expired, "Experiment expires because value changed");
+  ok(stopStub.calledWith("test"), "Experiment is stopped because value changed");
  ok(mockPreferences.get("fake.preference"), "changed value", "Preference value was not changed");


This is unnecessary now that you're checking if stop was called.

Osmose · 2017-04-14T21:30:11Z

recipe-client-addon/test/unit/test_PreferenceExperiments.js

  experiments["test"] = experimentFactory({
    name: "test",
    preferenceName: "fake.preference",
    preferenceValue: "experiment value",
  });
  mockPreferences.set("fake.preference", "changed value");
  await PreferenceExperiments.init();
-  ok(experiments["test"].expired, "Experiment expires because value changed");
+  ok(stopStub.calledWith("test"), "Experiment is stopped because value changed");


This should fail because false was also passed, which you want to assert.

calledWith only asserts about the arguments you pass. so

const foo = sinon.stub(); foo(1, 2, 3); ok(foo.calledWith(1));

would pass. The behavior you're thinking of is a part of calledWithExact. Which we should probably be using.

Osmose · 2017-04-14T21:31:42Z

recipe-client-addon/lib/PreferenceExperiments.jsm

@@ -106,8 +106,9 @@ this.PreferenceExperiments = {
      // Check that the current value of the preference is still what we set it to
      if (Preferences.get(experiment.preferenceName, undefined) !== experiment.preferenceValue) {
        // if not, stop the experiment, and skip the remaining steps
+        log.info(`Stopping experiment "${experiment.name}" because it's value changed`);


s/it's/its/

Osmose · 2017-04-14T21:34:30Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -30,7 +28,7 @@ add_task(withMockExperiments(async function (experiments) {
  await PreferenceExperiments.clearAllExperimentStorage();
  ok(
    !(await PreferenceExperiments.has("test")),
-    "clearAllExperimentStorage removed all stored experiments",
+    "clearAllExperimentStorage removed all stored experiments"


Don't we always want trailing commas?

Osmose · 2017-04-14T21:35:46Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -399,6 +397,7 @@ add_task(withMockExperiments(withMockPreferences(async function (experiments) {
 // stop should remove a preference that had no value prior to an experiment for user prefs
 add_task(withMockExperiments(withMockPreferences(async function (experiments, mockPreferences) {
  const stopObserver = sinon.stub(PreferenceExperiments, "stopObserver");
+  mockPreferences.set("fake.preference", "defaultvalue", "default");


This negates the point of this test, which is to test that preferences that have no value are remove after the test.

Osmose · 2017-04-14T21:37:51Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

-  equal(
-    DefaultPreferences.get("default"),
+  is(
+    mockPreferences.get("default", undefined, "default"),


I'm not sure how I feel about using mockPreferences for reading values like this. Why is it better?

It seemed strange to deal with two different objects for interacting with preferences during tests. This way a test that uses prefs only needs one object, instead of two or three (in the case of using both default and non-default prefs). It also gives us a consistent API between setting and getting prefs, which is nice.

mythmon · 2017-04-17T18:02:51Z

I accidentally closed this be deleting the branch. I'm not sure what happened, most like my automation scripts got a bit over-zealous.

…ass. The implementation of Preferences.jsm doesn't support watching a preference on the default branch, only on the combined set of preferences. This test cannot pass.

Osmose

I think almost all of my comments are minor test fixes that landed while this PR has been in-progress. This looks good!

One thing we might want to add are tests that init doesn't do certain things (like setting up preference observers) for disabled prefs.

Osmose · 2017-04-19T23:19:40Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -126,6 +126,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc
 // 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", "defaultvalue", "default");


Is this leftover from a rebase? We set the default below.

Osmose · 2017-04-19T23:21:25Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js


-add_task(async function testHasObserver() {
+add_task(withMockExperiments(async function testHasObserver() {


Are all these mocks necessary?

I added these mocks because they make sure that observers get properly cleaned up. We don't strictly need them on every test, but they make the tests more consistent.

Osmose · 2017-04-19T23:21:49Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -331,6 +332,9 @@ add_task(withMockExperiments(async function(experiments) {
 // preference value.
 add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
  const stopObserver = sinon.spy(PreferenceExperiments, "stopObserver");
+  const hasObserver = sinon.stub(PreferenceExperiments, "hasObserver");


I don't think we need this mock anymore, as the test starts an observer itself now.

Osmose · 2017-04-19T23:23:56Z

recipe-client-addon/test/browser/browser_PreferenceExperiments.js

@@ -397,6 +401,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments) {
 // stop should remove a preference that had no value prior to an experiment for user prefs
 add_task(withMockExperiments(withMockPreferences(async function(experiments, mockPreferences) {
  const stopObserver = sinon.stub(PreferenceExperiments, "stopObserver");
+  mockPreferences.set("fake.preference", "defaultvalue", "default");


This shouldn't be necessary, the test asserts using isSet now, which only checks user preferences.

gijsk

I don't have anything to add to what @Osmose already noted. :-)

mythmon · 2017-04-20T21:44:29Z

Removed the unneeded stuff from the tests.

Osmose

mythmon added the do not merge label Apr 12, 2017

mythmon assigned Osmose and gijsk Apr 12, 2017

mythmon requested review from Osmose and gijsk April 12, 2017 18:15

gijsk suggested changes Apr 12, 2017

View reviewed changes

Osmose suggested changes Apr 13, 2017

View reviewed changes

Osmose assigned mythmon and unassigned Osmose and gijsk Apr 13, 2017

mythmon force-pushed the watch-experiment-prefs branch from 13628fa to 4915caf Compare April 14, 2017 16:58

mythmon assigned Osmose and gijsk and unassigned mythmon Apr 14, 2017

mythmon removed the do not merge label Apr 14, 2017

Osmose suggested changes Apr 14, 2017

View reviewed changes

Osmose removed their assignment Apr 14, 2017

mythmon force-pushed the watch-experiment-prefs branch from 2b58c88 to ba75614 Compare April 14, 2017 18:22

mythmon assigned Osmose and mythmon and unassigned Osmose, gijsk and mythmon Apr 14, 2017

Osmose suggested changes Apr 14, 2017

View reviewed changes

Osmose assigned mythmon and unassigned Osmose Apr 14, 2017

mythmon closed this Apr 17, 2017

mythmon deleted the watch-experiment-prefs branch April 17, 2017 17:57

mythmon restored the watch-experiment-prefs branch April 17, 2017 18:02

mythmon reopened this Apr 17, 2017

mythmon force-pushed the watch-experiment-prefs branch 4 times, most recently from 57741d4 to ab79ee5 Compare April 19, 2017 22:33

mythmon assigned Osmose and gijsk and unassigned mythmon Apr 19, 2017

recipe-client-addon: Stop experiments if their preference changes.

3c41d82

mythmon force-pushed the watch-experiment-prefs branch from ab79ee5 to 3c41d82 Compare April 19, 2017 22:45

recipe-client-addon: Remove watching default branch test; it cannot p…

26917db

…ass. The implementation of Preferences.jsm doesn't support watching a preference on the default branch, only on the combined set of preferences. This test cannot pass.

Osmose suggested changes Apr 19, 2017

View reviewed changes

gijsk reviewed Apr 20, 2017

View reviewed changes

gijsk removed their assignment Apr 20, 2017

Osmose assigned mythmon and unassigned Osmose Apr 20, 2017

recipe-client-addon: Trim uneeded pref changes from experiment test.

0094530

Osmose approved these changes Apr 20, 2017

View reviewed changes

Osmose assigned gijsk and unassigned mythmon Apr 21, 2017

gijsk approved these changes Apr 22, 2017

View reviewed changes

gijsk assigned mythmon and unassigned gijsk Apr 22, 2017

Osmose merged commit 44a7721 into mozilla:preference-experiment Apr 22, 2017

Osmose mentioned this pull request Apr 22, 2017

Check for changed preference on startup #645

Closed

mythmon deleted the watch-experiment-prefs branch May 24, 2017 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watch experiment prefs #684

Watch experiment prefs #684

mythmon commented Apr 12, 2017

Osmose commented Apr 12, 2017

gijsk Apr 12, 2017

Osmose left a comment

mythmon commented Apr 14, 2017

Osmose left a comment

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose left a comment

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

mythmon Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

Osmose Apr 14, 2017

mythmon Apr 17, 2017

mythmon commented Apr 17, 2017

Osmose left a comment

Osmose Apr 19, 2017

Osmose Apr 19, 2017

mythmon Apr 20, 2017

Osmose Apr 19, 2017

Osmose Apr 19, 2017

gijsk left a comment

mythmon commented Apr 20, 2017

Osmose left a comment


		add_task(async function testHasObserver() {
		add_task(withMockExperiments(async function testHasObserver() {

Watch experiment prefs #684

Watch experiment prefs #684

Conversation

mythmon commented Apr 12, 2017

Osmose commented Apr 12, 2017

Choose a reason for hiding this comment

Osmose left a comment

Choose a reason for hiding this comment

mythmon commented Apr 14, 2017

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythmon commented Apr 17, 2017

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijsk left a comment

Choose a reason for hiding this comment

mythmon commented Apr 20, 2017

Osmose left a comment

Choose a reason for hiding this comment