Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
71753004:
Allow variation id with forcing flag for special setup & unit tests. (Closed)
|
Created:
7 years, 1 month ago by yao Modified:
6 years, 10 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAllow variation id with forcing flag for special setup & unit tests.
BUG=317879
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235680
Patch Set 1 : Allow variation id with forcing flag for special setup & unit tests. #
Total comments: 16
Patch Set 2 : Address code review comments #
Total comments: 12
Patch Set 3 : Address code review comments. #
Total comments: 16
Patch Set 4 : Address code review comments. #Patch Set 5 : Renamed a test function. #
Total comments: 1
Patch Set 6 : Use existing const EMPTY_ID instead of creating a new const. #Patch Set 7 : Rebase. #
Messages
Total messages: 14 (0 generated)
I have a question about the test, which I put in the comment. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:597: NonExpiredStudyPrioritizedOverExpiredStudy) { This test is not declared in variation_seed_processor.h. I wonder why is this? https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:780: TEST_F(VariationsSeedProcessorTest, StartsActive) { This one as well.
https://codereview.chromium.org/71753004/diff/40001/components/variations/var... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:67: bool VariationsSeedProcessor::AllowVariationIdWithForcingFlag( This and the other function can just be free-standing functions in the anon namespace at the top of the file. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:72: if (filter.platform_size() == 0 || filter.channel_size() == 0) { This doesn't need {}'s. (single line if and single line body) https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:77: filter.platform(i) != Study_Platform_PLATFORM_IOS) This does need {}'s - multi-line if. (Same below). https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:234: void VariationsSeedProcessor::CheckVariationId( This should be called RegisterVariationIds(). https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:258: bool allowBoth = AllowVariationIdWithForcingFlag(study); Can we do this check lazily? (e.g. block below when we create the trial for the forced flag? That way, if there's 100 studies, we only do the check when necessary. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:304: if (experiment.has_forcing_flag() && !allowBoth) I think we don't need this logic in this case. It's fine not registering the ids except for the selected group. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:597: NonExpiredStudyPrioritizedOverExpiredStudy) { On 2013/11/13 23:41:43, yao wrote: > This test is not declared in variation_seed_processor.h. I wonder why is this? Only tests that need to call private APIs need to be declared. (The declaration allows tests to call private functions that they otherwise wouldn't be allowed to call.)
https://codereview.chromium.org/71753004/diff/40001/components/variations/var... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:67: bool VariationsSeedProcessor::AllowVariationIdWithForcingFlag( On 2013/11/14 16:54:15, Alexei Svitkine wrote: > This and the other function can just be free-standing functions in the anon > namespace at the top of the file. Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:72: if (filter.platform_size() == 0 || filter.channel_size() == 0) { On 2013/11/14 16:54:15, Alexei Svitkine wrote: > This doesn't need {}'s. (single line if and single line body) Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:77: filter.platform(i) != Study_Platform_PLATFORM_IOS) On 2013/11/14 16:54:15, Alexei Svitkine wrote: > This does need {}'s - multi-line if. (Same below). Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:234: void VariationsSeedProcessor::CheckVariationId( On 2013/11/14 16:54:15, Alexei Svitkine wrote: > This should be called RegisterVariationIds(). Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:258: bool allowBoth = AllowVariationIdWithForcingFlag(study); On 2013/11/14 16:54:15, Alexei Svitkine wrote: > Can we do this check lazily? (e.g. block below when we create the trial for the > forced flag? > > That way, if there's 100 studies, we only do the check when necessary. Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor.cc:304: if (experiment.has_forcing_flag() && !allowBoth) On 2013/11/14 16:54:15, Alexei Svitkine wrote: > I think we don't need this logic in this case. It's fine not registering the ids > except for the selected group. Done. https://codereview.chromium.org/71753004/diff/40001/components/variations/var... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/40001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:597: NonExpiredStudyPrioritizedOverExpiredStudy) { Ic, thanks:) On 2013/11/14 16:54:15, Alexei Svitkine wrote: > On 2013/11/13 23:41:43, yao wrote: > > This test is not declared in variation_seed_processor.h. I wonder why is this? > > > Only tests that need to call private APIs need to be declared. (The declaration > allows tests to call private functions that they otherwise wouldn't be allowed > to call.)
https://codereview.chromium.org/71753004/diff/90001/components/variations/var... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:62: const std::string& trial_name) { Nit: Remove extra space before trial_name https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:93: Study_Filter filter = study.filter(); This does a copy. Use a reference (const Study_Filter& filter). https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:272: if (AllowVariationIdWithForcingFlag(study)) { No need for {}'s. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:644: bool allowBoth = VariationsSeedProcessor(). Use C++ naming convention. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:645: AllowVariationIdWithForcingFlag(study); This should be indented 2 more. Also, can you re-use the same VariationSeedProcessor instance throughout the test? VariationsSeedProcessor processor; processor.AllowVariationIdWithForcingFlag(study); https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:646: EXPECT_FALSE(allowBoth); You can just inline this, without a variable. EXPECT_FALSE(processor.AllowVariationIdWithForcingFlag(study));
https://codereview.chromium.org/71753004/diff/90001/components/variations/var... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:62: const std::string& trial_name) { On 2013/11/14 22:34:33, Alexei Svitkine wrote: > Nit: Remove extra space before trial_name Done. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:93: Study_Filter filter = study.filter(); On 2013/11/14 22:34:33, Alexei Svitkine wrote: > This does a copy. Use a reference (const Study_Filter& filter). Done. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor.cc:272: if (AllowVariationIdWithForcingFlag(study)) { On 2013/11/14 22:34:33, Alexei Svitkine wrote: > No need for {}'s. Done. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:644: bool allowBoth = VariationsSeedProcessor(). On 2013/11/14 22:34:33, Alexei Svitkine wrote: > Use C++ naming convention. Done. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:645: AllowVariationIdWithForcingFlag(study); On 2013/11/14 22:34:33, Alexei Svitkine wrote: > This should be indented 2 more. > > Also, can you re-use the same VariationSeedProcessor instance throughout the > test? > > VariationsSeedProcessor processor; > processor.AllowVariationIdWithForcingFlag(study); Done. https://codereview.chromium.org/71753004/diff/90001/components/variations/var... components/variations/variations_seed_processor_unittest.cc:646: EXPECT_FALSE(allowBoth); On 2013/11/14 22:34:33, Alexei Svitkine wrote: > You can just inline this, without a variable. > > EXPECT_FALSE(processor.AllowVariationIdWithForcingFlag(study)); Done.
Almost, there just a few more comments. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor.cc:60: // variation id. Nit: Comment talks about a singular variation id, whereas the code checks for two different ones - update the comment to be in plural. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor.cc:63: if (experiment.has_google_web_experiment_id()) { Nit: Indentation is wrong here. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:96: AllowForceGroupAndVariationIdWithoutCommandLine) { Nit: Align. Also, same thing for other tests below. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:113: EXPECT_EQ(0, id); Don't hardcode 0. Use EMPTY_ID instead. However, I'm not sure this test case makes sense. I know it corresponds to the current behavior (per my suggestion, etc), but I think the use case of querying for a variation id for a field trial group that's not currently active is an unexpected use case (and I want us to refactor that API at some point to make that use case not possible). So testing that scenario doesn't seem worthwhile to me. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:488: kFlagGroup1Name); Similar comment as a above, we shouldn't test the case where the group isn't the active group. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:510: EXPECT_EQ(0, id); EMPTY_ID https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:642: TEST_F(VariationsSeedProcessorTest, TestAllowVariationIdWithForcingFlag) { Nit: TestAllowVariationIdWithForcingFlag -> AllowVariationIdWithForcingFlag (no need for an extra Test prefix in the name).
https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor.cc:60: // variation id. On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Nit: Comment talks about a singular variation id, whereas the code checks for > two different ones - update the comment to be in plural. Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor.cc:63: if (experiment.has_google_web_experiment_id()) { On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Nit: Indentation is wrong here. Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:96: AllowForceGroupAndVariationIdWithoutCommandLine) { On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Nit: Align. Also, same thing for other tests below. Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:113: EXPECT_EQ(0, id); On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Don't hardcode 0. Use EMPTY_ID instead. > > However, I'm not sure this test case makes sense. I know it corresponds to the > current behavior (per my suggestion, etc), but I think the use case of querying > for a variation id for a field trial group that's not currently active is an > unexpected use case (and I want us to refactor that API at some point to make > that use case not possible). > > So testing that scenario doesn't seem worthwhile to me. Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:488: kFlagGroup1Name); On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Similar comment as a above, we shouldn't test the case where the group isn't the > active group. Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:510: EXPECT_EQ(0, id); On 2013/11/15 15:20:43, Alexei Svitkine wrote: > EMPTY_ID Done. https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:642: TEST_F(VariationsSeedProcessorTest, TestAllowVariationIdWithForcingFlag) { On 2013/11/15 15:20:43, Alexei Svitkine wrote: > Nit: TestAllowVariationIdWithForcingFlag -> AllowVariationIdWithForcingFlag (no > need for an extra Test prefix in the name). The reason I put a test prefix here was trying to differentiate testing of just the function AllowVariationIdWithForcingFlag() and the whole functionality. It wasn't clear I guess. But having two tests naming as AllowForceGroupAndVariationId & AllowVariationIdWithForcingFlag is confusing as well I think. Also, if renaming this test to AllowVariationIdWithForcingFlag, it will have the same name of the original function. Will that cause a problem?
https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:642: TEST_F(VariationsSeedProcessorTest, TestAllowVariationIdWithForcingFlag) { On 2013/11/15 18:51:49, yao wrote: > On 2013/11/15 15:20:43, Alexei Svitkine wrote: > > Nit: TestAllowVariationIdWithForcingFlag -> AllowVariationIdWithForcingFlag > (no > > need for an extra Test prefix in the name). > > The reason I put a test prefix here was trying to differentiate testing of just > the function AllowVariationIdWithForcingFlag() and the whole functionality. It > wasn't clear I guess. But having two tests naming as > AllowForceGroupAndVariationId & > AllowVariationIdWithForcingFlag > is confusing as well I think. > > Also, if renaming this test to AllowVariationIdWithForcingFlag, it will have the > same name of the original function. Will that cause a problem? I think having the test be named the same as the function its testing is good, because it documents exactly that its testing (that function), and thus wouldn't be confused with the other test name (which tests specific functionality, rather than the function). I've used this convention before (naming the test after the function its testing) and I think its a reasonable thing to do.
https://codereview.chromium.org/71753004/diff/170001/components/variations/va... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/170001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:642: TEST_F(VariationsSeedProcessorTest, TestAllowVariationIdWithForcingFlag) { On 2013/11/15 19:00:56, Alexei Svitkine wrote: > On 2013/11/15 18:51:49, yao wrote: > > On 2013/11/15 15:20:43, Alexei Svitkine wrote: > > > Nit: TestAllowVariationIdWithForcingFlag -> AllowVariationIdWithForcingFlag > > (no > > > need for an extra Test prefix in the name). > > > > The reason I put a test prefix here was trying to differentiate testing of > just > > the function AllowVariationIdWithForcingFlag() and the whole functionality. It > > wasn't clear I guess. But having two tests naming as > > AllowForceGroupAndVariationId & > > AllowVariationIdWithForcingFlag > > is confusing as well I think. > > > > Also, if renaming this test to AllowVariationIdWithForcingFlag, it will have > the > > same name of the original function. Will that cause a problem? > > I think having the test be named the same as the function its testing is good, > because it documents exactly that its testing (that function), and thus wouldn't > be confused with the other test name (which tests specific functionality, rather > than the function). > > I've used this convention before (naming the test after the function its > testing) and I think its a reasonable thing to do. Done.
LGTM with a final comment, please address before landing https://codereview.chromium.org/71753004/diff/270001/components/variations/va... File components/variations/variations_seed_processor_unittest.cc (right): https://codereview.chromium.org/71753004/diff/270001/components/variations/va... components/variations/variations_seed_processor_unittest.cc:33: const VariationID kEmptyExperimentId = 0; EMPTY_ID is already defined in components/variations/variations_associated_data.h. Just use that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/71753004/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/71753004/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/71753004/340001
Message was sent while issue was closed.
Change committed as 235680 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
