-
Notifications
You must be signed in to change notification settings - Fork 12
EXP-1865 add isRollout boolean property to Experiment type #179
Conversation
Kind of winging it on the example files as my first try. Let's see what I missed! |
91a5233
to
24e7fe2
Compare
24e7fe2
to
3e06e21
Compare
3e06e21
to
d880b64
Compare
Seeing all the comments on the example data in this PR, it seems maybe worth backing up and seeing if we have a better source for these? Maybe @heres-maria has a better way to come up with this sample data? That is - come up what is actually useful for testing these things, put that in this PR. My goal was just to set |
@lmorchard For the feature part at least, maybe these production experiments can be used as the starting point for the iOS and Fenix recipes? They are current "rollouts". |
d880b64
to
593afd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, another stab at maybe some useful test files? Not sure if these are it.
Main thing is that I copied the recipes for these experiments and tried hand-tweaking them to add rollout properties as noted in comments:
"appName": "firefox_desktop", | ||
"application": "firefox-desktop", | ||
"arguments": {}, | ||
"isRollout": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this new property, which is the main event in this PR.
], | ||
"bucketConfig": { | ||
"count": 1700, | ||
"namespace": "urlbar-21-rollout", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added -rollout
to the namespace, to simulate the nimbus model logic here
"proposedEnrollment": 7, | ||
"referenceBranch": "control", | ||
"schemaVersion": "1.7.0", | ||
"slug": "desktop-rollout-example-firefox-suggest-history-rollout-3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaked the slug and IDs
"proposedDuration": 28, | ||
"proposedEnrollment": 7, | ||
"referenceBranch": "control", | ||
"schemaVersion": "1.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what the eventual schemaVersion should be, here. It depends on what PR lands first, and we'd like to remove it anyway per issue #186 - but we need something here to pass CI in the meantime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop recipe looks good. Thanks.
593afd3
to
7d94813
Compare
Alright, maybe third try's the charm? Rebased off main, copied the multifeature example data files, changed some names and added |
7d94813
to
ef7c128
Compare
"appName": "firefox_ios", | ||
"application": "org.mozilla.ios.Firefox", | ||
"arguments": {}, | ||
"branches": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has multiple branches and affects multiple features. Should this be single branch and single feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also like to test the interaction between rollouts and a multifeature experiment of this type, but that would have isRollout false and a different namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heres-maria Can you compose the test data that would actually work for testing? We could add it to the repo after testing. I've tried a few times now in this PR - none of those tries has passed muster. Would rather just record what was used to successfully test the feature than keep guessing at what might work
Because: * we want to implement support for an isRollout flag in experiments This commit: * adds an isRollout boolean to the NimbusExperiment type * adds a test for boolean property schema validation for experiments
ef7c128
to
6f5d276
Compare
Revised this PR to drop the sample data. The schema change here is very small, but the testing data blocking merge of the PR seems to have been a moving target that I don't have the context to assemble properly. Maybe we should just add the sample data after successful testing? That way we record what was used rather than trying to guess what might work? |
Missed the thumbs up earlier, just going to merge this and we can move on with sample data when we have it |
Possible testing data: Desktop Rollout with pocketNewtab enabled
Desktop Rollout with privatebrowsing disabled
Desktop Experiment with isRollout false pocketNewtab enabled
Desktop Experiment with isRollout false pocketNewtab disabled
Desktop Multifeature Experiment with isRollout false pocketNewtab and privatebrowsing enabled
Desktop Multifeature Experiment with isRollout false pocketNewtab and privatebrowsing disabled
Firefox Android: Android Rollout with homescreen enabled
Android Rollout with homescreen disabled
Android Experiment with isRollout false homescreen enabled
Android Experiment with isRollout false homescreen disabled
Android Multifeature Experiment with isRollout false homescreen and fenix-default-browser enabled
Android Multifeature Experiment with isRollout false homescreen and fenix-default-browser disabled
Firefox iOS: iOS Rollout with homescreen enabled
iOS Rollout with homescreen disabled
iOS Experiment with isRollout false homescreen enabled
iOS Experiment with isRollout false homescreen disabled
iOS Multifeature Experiment with isRollout false homescreen and fenix-default-browser enabled
iOS Multifeature Experiment with isRollout false homescreen and fenix-default-browser disabled
|
Because:
This commit:
adds an isRollout boolean to the NimbusExperiment type
introduces example recipes for both mobile and desktop for testing