Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360053002:
Document the Combined Field Trial Testing Configuration Format (Closed)
DescriptionDocument the Combined Field Trial Testing Configuration Format
BUG=637095
Committed: https://crrev.com/2ff53323955f3528c4e62f34344a178b12f80a5a
Cr-Commit-Position: refs/heads/master@{#420694}
Patch Set 1 #Patch Set 2 : Quick fixes #
Total comments: 4
Patch Set 3 : CR Feedback #
Total comments: 6
Patch Set 4 : CR Feedback #Patch Set 5 : Experiment Configuration -> Study Configuration #
Messages
Total messages: 25 (13 generated)
robliao@chromium.org changed reviewers: + asvitkine@chromium.org, rkaplow@chromium.org
Here's a first pass for the Chromium docs.
https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... File testing/variations/README.md (right): https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... testing/variations/README.md:53: bots and browser tests in the waterfall. I feel like this paragraph should live in the first section of the doc, which also discusses Chromium builds. https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... testing/variations/README.md:82: //0: "This is the first comment line.", The syntax seems different here than on line 18. Please make consistent More generally, this syntax is pretty awful and it would be nice if we could just support normal // and /* */ comments. We do this for internal experiment configs, so it would be nice to make consistent. (If it's something you'd like to look at, we definitely appreciate the help - but I don't want to make you shave even more yaks unless you want it - so I'm OK with just adding a TODO() here to support a better comment syntax and we can look at it in the future.)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... File testing/variations/README.md (right): https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... testing/variations/README.md:53: bots and browser tests in the waterfall. On 2016/09/21 21:38:17, Alexei Svitkine (very slow) wrote: > I feel like this paragraph should live in the first section of the doc, which > also discusses Chromium builds. Done. https://codereview.chromium.org/2360053002/diff/20001/testing/variations/READ... testing/variations/README.md:82: //0: "This is the first comment line.", On 2016/09/21 21:38:17, Alexei Svitkine (very slow) wrote: > The syntax seems different here than on line 18. Please make consistent > > More generally, this syntax is pretty awful and it would be nice if we could > just support normal // and /* */ comments. We do this for internal experiment > configs, so it would be nice to make consistent. > > (If it's something you'd like to look at, we definitely appreciate the help - > but I don't want to make you shave even more yaks unless you want it - so I'm OK > with just adding a TODO() here to support a better comment syntax and we can > look at it in the future.) Made it consistent, although this change would be better tracked as a bug rather than in docs. http://crbug.com/649152 Agreed the syntax could be better, but that's the cost of using JSON to represent data. http://stackoverflow.com/questions/244777/can-comments-be-used-in-json Compliant JSON doesn't have a way to support comments. If we changed our serialization format, then maybe we can support comments. I was surprised to see this added recently in https://codereview.chromium.org/2306473002 while this CL was going through review.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... File testing/variations/README.md (right): https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:41: ### Experiment Descriptions This shouldn't be Experiment Description since the overall concept is called a Study (even in your example we use StudyName). So this should just s/experiment description/study here https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:55: ### Experiments just to make it easier for regular people to understand, i might even call this Experiments (Groups) since people might think this is related to a full experiment (what we call a study) https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:65: `enable_features` and `disable_features` indicate which features should be can you be more explicit with what feature menas here (i.e. point to the feature API)
https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... File testing/variations/README.md (right): https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:41: ### Experiment Descriptions On 2016/09/22 19:00:19, rkaplow wrote: > This shouldn't be Experiment Description since the overall concept is called a > Study (even in your example we use StudyName). > > So this should just s/experiment description/study here A study can have one or more experiment descriptions associated with it (like the different platforms case below). Happy to entertain a better name for this. https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:55: ### Experiments On 2016/09/22 19:00:19, rkaplow wrote: > just to make it easier for regular people to understand, i might even call this > Experiments (Groups) since people might think this is related to a full > experiment (what we call a study) Done. https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... testing/variations/README.md:65: `enable_features` and `disable_features` indicate which features should be On 2016/09/22 19:00:19, rkaplow wrote: > can you be more explicit with what feature menas here (i.e. point to the feature > API) Done. Added a link.
On 2016/09/22 21:13:40, robliao wrote: > https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... > File testing/variations/README.md (right): > > https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... > testing/variations/README.md:41: ### Experiment Descriptions > On 2016/09/22 19:00:19, rkaplow wrote: > > This shouldn't be Experiment Description since the overall concept is called a > > Study (even in your example we use StudyName). > > > > So this should just s/experiment description/study here > > A study can have one or more experiment descriptions associated with it (like > the different platforms case below). > > Happy to entertain a better name for this. Sure, I see your point, but I don't like the word experiment to describe this since we use that term for groups. I would still think Study config is better, even if we can have several configs per Study. Maybe even Study subconfig if we want to be very pedantic. > > https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... > testing/variations/README.md:55: ### Experiments > On 2016/09/22 19:00:19, rkaplow wrote: > > just to make it easier for regular people to understand, i might even call > this > > Experiments (Groups) since people might think this is related to a full > > experiment (what we call a study) > > Done. > > https://codereview.chromium.org/2360053002/diff/60001/testing/variations/READ... > testing/variations/README.md:65: `enable_features` and `disable_features` > indicate which features should be > On 2016/09/22 19:00:19, rkaplow wrote: > > can you be more explicit with what feature menas here (i.e. point to the > feature > > API) > > Done. Added a link.
lgtm
lgtm % Rob's comments
Done. Experiment Configuration -> Study Configuration
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2360053002/#ps100001 (title: "Experiment Configuration -> Study Configuration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Document the Combined Field Trial Testing Configuration Format BUG=637095 ========== to ========== Document the Combined Field Trial Testing Configuration Format BUG=637095 Committed: https://crrev.com/2ff53323955f3528c4e62f34344a178b12f80a5a Cr-Commit-Position: refs/heads/master@{#420694} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2ff53323955f3528c4e62f34344a178b12f80a5a Cr-Commit-Position: refs/heads/master@{#420694} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
