Fix #652: Add option for setting user or default prefs for experiments. #670

Osmose · 2017-04-07T07:06:30Z

In which I continue to sidestep rewriting the entire PreferenceExperiments test suite to use async/await.

It turns out preference observers react to changes in the effective preference value, which covers default value changes, which helped cut down on a bunch of the code and tests I originally had here.

Osmose · 2017-04-07T07:08:16Z

recipe-client-addon/bootstrap.js

+  // Initialize experiments first to avoid a race between initializing prefs
+  // and recipes rolling back pref changes when experiments end.
+  PreferenceExperiments.init().catch(err => {
+    log.error("Failed to initialize preference experiments:", err);


I would've liked to test that PreferenceExperiments.init failing does not stop RecipeRunner.init from running, but I dunno how to test bootstrap stuff.

Yeah, I don't know how to do that either. I guess you could write a browser test that disables and re-enables the add-on (using add-on manager APIs)? I think that'd run this code, maybe?

@rhelmer Are there any better ways to test add-on startup code than ^^?

Chatted about this in IRC - using the public AddonManager API won't be great for this since built-in system add-ons cannot be uninstalled or disabled, so re-running bootstrap.js startup() isn't really doable.

I'd suggest either simplifyingbootstrap.js and moving the code you want to test into a different JSM that you write a test for, or if you want to have more control over the AddonManager lifecycle we have convenience methods for setting up and restarting AddonManager in http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm

Another option would be to use mochitest and restart the whole browser, but that's a bit more heavy-handed.

Osmose · 2017-04-07T07:10:53Z

recipe-client-addon/lib/PreferenceExperiments.jsm

-        Preferences.reset(preferenceName);
+        // This does nothing if we're on the default branch, which is fine. The
+        // preference will be reset on next restart, and most preferences should
+        // have had a default value set before the experiment anyway.


See also bsmedberg's email on dev-shield.

gijsk · 2017-04-07T14:24:05Z

General note, that might be worth bringing up on the mailing list: code can (and does) check whether the user has "modified" preferences (ie whether a pref has a value on the user branch, using the pref service's prefHasUserValue method) as a short cut in some cases. Changing the default value with an experiment in that case would in such cases potentially not yield the result you expect. I don't expect this to be a serious problem in practice, but it seems like it's worth flagging up for QE testing things or when coordinating with devs as something to look out for, as behaviour could potentially be different when comparing the preference experiment with QE setting the pref in about:config.

gijsk

I haven't reviewed the recipe-client-server/addon bit, I'll leave that to Andy. :-)

gijsk · 2017-04-07T14:28:43Z

recipe-client-addon/bootstrap.js

+  // Initialize experiments first to avoid a race between initializing prefs
+  // and recipes rolling back pref changes when experiments end.
+  PreferenceExperiments.init().catch(err => {
+    log.error("Failed to initialize preference experiments:", err);


Yeah, I don't know how to do that either. I guess you could write a browser test that disables and re-enables the add-on (using add-on manager APIs)? I think that'd run this code, maybe?

gijsk · 2017-04-07T14:29:49Z

recipe-client-addon/lib/PreferenceExperiments.jsm

+    const store = await ensureStorage();
+    for (const experiment of Object.values(store.data)) {
+      const {expired, preferenceBranchType, preferenceName, preferenceValue} = experiment;
+      if (!expired && preferenceBranchType === "default") {


I'm a bit surprised here - why do we not just set it for all of the pref experiments, including the ones using the user branch?

It's super edge case-y: If we're doing a user pref experiment (and the pref doesn't have a default value already), and the experiment ends, the default will retain the experimental value for the rest of the session. This check prevents setting the default when unnecessary. Although the reason I added it in the first place was more that it made sense only to set the pref when necessary, rather than because I thought that edge case mattered.

I'm fine with removing it, although I don't see the harm in keeping it.

gijsk · 2017-04-07T14:33:51Z

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

+  DefaultPreferences.set("fake.preference", "experimentvalue");
+  ok(!stop.called, "Changing to the experimental pref value did not trigger the observer");
+
+  // Setting it to something different should trigger the call.


Not entirely sure what we're testing here, and if it's worth testing this again once setting it back to the experiment value from the new value? Specifically, is calling startObserver expected to set the preference immediately, or is it not set until the test here calls DefaultPreferences.set ? In the latter case, I suppose testing this again would make no difference, but a comment might be nice. Otherwise, testing the correct behaviour (whatever that is) when setting the thing back to the expected experiment value would make sense.

Testing after setting it back doesn't really matter, because by that point stop has already been called, which (when not mocked, like in this test) would remove the observer.

startObserver doesn't actually modify the preference, it only sets up the preference watcher, so the preference isn't set to the experimental value until the first DefaultPreferences.set call. #645 isn't finished yet, but it will need to call this function on startup for the active experiments, and I figured it'd be easier to reason about how this behaves without setting the preference than it was to figure out if setting the experiment preferences on startup was an issue.

I'll add a comment clarifying that startObserver does not modify the preference to these tests.

mythmon · 2017-04-07T16:53:10Z

recipe-client-addon/bootstrap.js

+  // Initialize experiments first to avoid a race between initializing prefs
+  // and recipes rolling back pref changes when experiments end.
+  PreferenceExperiments.init().catch(err => {
+    log.error("Failed to initialize preference experiments:", err);


Since the return value of the bootstrap methods isn't used (afaik?), I bet you could make them async functions if you wanted.

@rhelmer Does ^^ sound right?

Yes you can make bootstrap methods async, and the return values are not used by AOM AFAICT.

andymikulski · 2017-04-07T16:48:00Z

recipe-client-addon/lib/PreferenceExperiments.jsm

+   */
+  async init() {
+    const store = await ensureStorage();
+    for (const experiment of Object.values(store.data)) {


Object.values returns an array, would it be more concise to use forEach instead of the for/of syntax?

ex: Object.values(store.data).forEach((experiment)=>{ ... });

It ends up only saving a few characters. I think we use for/of in most other places over forEach as well, but I might be misremembering.

andymikulski · 2017-04-07T16:56:01Z

recipe-client-addon/lib/PreferenceExperiments.jsm

   * @rejects {Error}
   *   If an experiment with the given name already exists, or if an experiment
   *   for the given preference is active.
   */
-  async start(experimentName, branch, preferenceName, preferenceValue) {
+  async start(experimentName, branch, preferenceName, preferenceValue, preferenceBranchType) {


Generally, when there are a bunch of function params, I prefer to just pass an object and destructure the values out from there. Ex:

async start({ experimentName, branch, preferenceName, preferenceValue, preferenceBranchType }) { ... somewhereElse.start({ experimentName: 'whatever', ... })

It makes things a little clearer when start is called, and devs skimming/reading the code don't have to refer to another file to determine what the params mean.

This may be a preference thing, though.

Nah, that's usually the right thing. I often wish JS had keyword arguments in the same way Python does.

andymikulski · 2017-04-07T17:06:23Z

recipe-server/client/control/components/action_fields/PreferenceExperimentFields.js

+          <option value="user">User</option>
+        </ControlField>
+        {preferenceBranchType === 'user' &&
+          <p className="field-warning">


Since this element isn't dynamic, we could add it as a static property of the class. That would have React "render" it once, and would let us write something like:

static userPrefWarning = (<p className="field-warning">...</p>); // in render { preferenceBranchType === 'user' && PreferenceExperimentFields.userPrefWarning }

Minor nit, I just prefer to keep render concise when possible.

andymikulski · 2017-04-07T17:09:59Z

recipe-server/client/control/tests/components/test_PreferenceExperimentFields.js

+
+  it('should render a warning if using the user preference branch type', () => {
+    const wrapper = shallow(<PreferenceExperimentFields preferenceBranchType="user" />);
+    const warning = wrapper.find('.field-warning');


re: adding the warning as a static class member - I thiiiink we could do something like wrapper.find(PreferenceExperimentFields.userPrefWarning) here.

It works! I used wrapper.contains instead of find, but I like this a lot better than the old test. Thanks!

Osmose · 2017-04-07T20:57:07Z

General note, that might be worth bringing up on the mailing list: code can (and does) check whether the user has "modified" preferences (ie whether a pref has a value on the user branch, using the pref service's prefHasUserValue method) as a short cut in some cases. Changing the default value with an experiment in that case would in such cases potentially not yield the result you expect. I don't expect this to be a serious problem in practice, but it seems like it's worth flagging up for QE testing things or when coordinating with devs as something to look out for, as behaviour could potentially be different when comparing the preference experiment with QE setting the pref in about:config.

I dunno if it should live in our docs (probably?), but a short guide on designing and implementing a preference experiment on the Firefox for developers seems like it'd be a nice place to put this.

Osmose · 2017-04-07T22:12:53Z

Feedback'd, waiting on responses before making more changes.

Osmose · 2017-04-10T21:11:06Z

Alright, updated with one last commit for testing bootstrap.js, sorry for invalidating ya'lls existing review.

gijsk · 2017-04-11T09:24:13Z

recipe-client-addon/bootstrap.js

@@ -45,6 +45,8 @@ const PREF_LOGGING_LEVEL = PREF_BRANCH + "logging.level";
 let shouldRun = true;
 let log = null;

+this.EXPORTED_SYMBOLS = ["install", "startup", "shutdown", "uninstall"];


Please add a comment here saying we export these for testing purposes.

…eriments.

Spoiler alert: It's for testing.

Osmose assigned gijsk and andymikulski Apr 7, 2017

Osmose requested review from gijsk and andymikulski April 7, 2017 07:06

Osmose commented Apr 7, 2017

View reviewed changes

Osmose force-pushed the default-prefs branch from 541e827 to 98de402 Compare April 7, 2017 07:10

Osmose commented Apr 7, 2017

View reviewed changes

gijsk suggested changes Apr 7, 2017

View reviewed changes

mythmon reviewed Apr 7, 2017

View reviewed changes

andymikulski reviewed Apr 7, 2017

View reviewed changes

andymikulski removed their assignment Apr 7, 2017

Osmose assigned Osmose and unassigned gijsk Apr 7, 2017

gijsk approved these changes Apr 8, 2017

View reviewed changes

andymikulski approved these changes Apr 8, 2017

View reviewed changes

Osmose requested review from gijsk and andymikulski April 10, 2017 21:10

gijsk reviewed Apr 11, 2017

View reviewed changes

gijsk approved these changes Apr 11, 2017

View reviewed changes

andymikulski approved these changes Apr 11, 2017

View reviewed changes

Michael Kelly added 4 commits April 12, 2017 10:16

Fix mozilla#652: Add option for setting user or default prefs for exp…

7d65dc0

…eriments.

Review feedback.

05fd278

Make bootstrap startup async and add tests.

3e09dce

Comment as to why we're exporting bootstrap methods in the add-on.

05a53da

Spoiler alert: It's for testing.

Osmose force-pushed the default-prefs branch from 7c98f98 to 05a53da Compare April 12, 2017 17:17

Osmose merged commit 86c8973 into mozilla:preference-experiment Apr 12, 2017

Osmose deleted the default-prefs branch April 12, 2017 23:37

Osmose mentioned this pull request Apr 13, 2017

Option to change default preference value for preference experiment #652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #652: Add option for setting user or default prefs for experiments. #670

Fix #652: Add option for setting user or default prefs for experiments. #670

Osmose commented Apr 7, 2017

Osmose Apr 7, 2017

gijsk Apr 7, 2017

Osmose Apr 7, 2017

rhelmer Apr 10, 2017 •

edited

Loading

Osmose Apr 7, 2017

gijsk commented Apr 7, 2017

gijsk left a comment

gijsk Apr 7, 2017

gijsk Apr 7, 2017

Osmose Apr 7, 2017

gijsk Apr 7, 2017

Osmose Apr 7, 2017

mythmon Apr 7, 2017

Osmose Apr 7, 2017

rhelmer Apr 10, 2017

andymikulski Apr 7, 2017

Osmose Apr 7, 2017

andymikulski Apr 7, 2017

Osmose Apr 7, 2017

andymikulski Apr 7, 2017

andymikulski Apr 7, 2017

Osmose Apr 7, 2017

Osmose commented Apr 7, 2017

Osmose commented Apr 7, 2017

Osmose commented Apr 10, 2017

gijsk Apr 11, 2017

Fix #652: Add option for setting user or default prefs for experiments. #670

Fix #652: Add option for setting user or default prefs for experiments. #670

Conversation

Osmose commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijsk commented Apr 7, 2017

gijsk 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

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 commented Apr 7, 2017

Osmose commented Apr 7, 2017

Osmose commented Apr 10, 2017

Choose a reason for hiding this comment

rhelmer Apr 10, 2017 •

edited

Loading