Allow variation id with forcing flag for special setup & unit tests.

components/variations/variations_seed_processor_unittest.cc

Index: components/variations/variations_seed_processor_unittest.cc
diff --git a/components/variations/variations_seed_processor_unittest.cc b/components/variations/variations_seed_processor_unittest.cc
index 7b86def9eb3566f808012193305866d4e1a15d7e..07810d5f2a04183996852f1ea0e7edf319a59c91 100644
--- a/components/variations/variations_seed_processor_unittest.cc
+++ b/components/variations/variations_seed_processor_unittest.cc
@@ -29,6 +29,8 @@ const char kNonFlagGroupName[] = "non_flag_group";
 const char kForcingFlag1[] = "flag_test1";
 const char kForcingFlag2[] = "flag_test2";
 
+const VariationID kExperimentId = 123;
+
 // Adds an experiment to |study| with the specified |name| and |probability|.
 Study_Experiment* AddExperiment(const std::string& name, int probability,
                                 Study* study) {
@@ -90,6 +92,50 @@ class VariationsSeedProcessorTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(VariationsSeedProcessorTest);
 };
 
+TEST_F(VariationsSeedProcessorTest,
+    AllowForceGroupAndVariationIdWithoutCommandLine) {

[Alexei Svitkine (slow), 2013/11/15 15:20:43]

Nit: Align. Also, same thing for other tests below.

[yao, 2013/11/15 18:51:49]

Done.

+  base::FieldTrialList field_trial_list(NULL);
+
+  Study study = CreateStudyWithFlagGroups(100, 0, 0);
+  study.mutable_experiment(1)->set_google_web_experiment_id(kExperimentId);
+  study.mutable_filter()->add_channel(Study_Channel_DEV);
+  study.mutable_filter()->add_channel(Study_Channel_CANARY);
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_ANDROID);
+
+  VariationsSeedProcessor().CreateTrialFromStudy(
+      ProcessedStudy(&study, 100, false));
+
+  EXPECT_EQ(kNonFlagGroupName,
+            base::FieldTrialList::FindFullName(kFlagStudyName));
+
+  VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, kFlagStudyName,
+                                        kFlagGroup1Name);
+  EXPECT_EQ(0, id);

[Alexei Svitkine (slow), 2013/11/15 15:20:43]

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.

[yao, 2013/11/15 18:51:49]

Done.

+}
+
+TEST_F(VariationsSeedProcessorTest,
+    AllowForceGroupAndVariationIdWithCommandLine) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(kForcingFlag1);
+
+  base::FieldTrialList field_trial_list(NULL);
+
+  Study study = CreateStudyWithFlagGroups(100, 0, 0);
+  study.mutable_experiment(1)->set_google_web_experiment_id(kExperimentId);
+  study.mutable_filter()->add_channel(Study_Channel_DEV);
+  study.mutable_filter()->add_channel(Study_Channel_CANARY);
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_ANDROID);
+
+  VariationsSeedProcessor().CreateTrialFromStudy(
+      ProcessedStudy(&study, 100, false));
+
+  EXPECT_EQ(kFlagGroup1Name,
+            base::FieldTrialList::FindFullName(kFlagStudyName));
+
+  VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, kFlagStudyName,
+                                        kFlagGroup1Name);
+  EXPECT_EQ(kExperimentId, id);
+}
+
 TEST_F(VariationsSeedProcessorTest, CheckStudyChannel) {
   VariationsSeedProcessor seed_processor;
 
@@ -424,6 +470,46 @@ TEST_F(VariationsSeedProcessorTest, FilterAndValidateStudies) {
   EXPECT_EQ(kTrial3Name, processed_studies[1].study->name());
 }
 
+TEST_F(VariationsSeedProcessorTest,
+    ForbidForceGroupWithVariationIdWithoutCommandLine) {
+  base::FieldTrialList field_trial_list(NULL);
+
+  Study study = CreateStudyWithFlagGroups(100, 0, 0);
+  study.mutable_experiment(1)->set_google_web_experiment_id(kExperimentId);
+  // Add windows platform makes forcing_flag and variation Id incompatible.
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_WINDOWS);
+
+  VariationsSeedProcessor().CreateTrialFromStudy(
+      ProcessedStudy(&study, 100, false));
+
+  EXPECT_EQ(kNonFlagGroupName,
+            base::FieldTrialList::FindFullName(kFlagStudyName));
+  VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, kFlagStudyName,
+                                        kFlagGroup1Name);

[Alexei Svitkine (slow), 2013/11/15 15:20:43]

Similar comment as a above, we shouldn't test the case where the group isn't the
active group.

[yao, 2013/11/15 18:51:49]

Done.

+  EXPECT_EQ(0, id);
+}
+
+TEST_F(VariationsSeedProcessorTest,
+    ForbidForceGroupWithVariationIdWithCommandLine) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(kForcingFlag1);
+
+  base::FieldTrialList field_trial_list(NULL);
+
+  Study study = CreateStudyWithFlagGroups(100, 0, 0);
+  study.mutable_experiment(1)->set_google_web_experiment_id(kExperimentId);
+  // Add windows platform makes forcing_flag and variation Id incompatible.
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_WINDOWS);
+
+  VariationsSeedProcessor().CreateTrialFromStudy(
+      ProcessedStudy(&study, 100, false));
+
+  EXPECT_EQ(kFlagGroup1Name,
+            base::FieldTrialList::FindFullName(kFlagStudyName));
+  VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, kFlagStudyName,
+                                        kFlagGroup1Name);
+  EXPECT_EQ(0, id);

[Alexei Svitkine (slow), 2013/11/15 15:20:43]

EMPTY_ID

[yao, 2013/11/15 18:51:49]

Done.

+}
+
 // Test that the group for kForcingFlag1 is forced.
 TEST_F(VariationsSeedProcessorTest, ForceGroupWithFlag1) {
   CommandLine::ForCurrentProcess()->AppendSwitch(kForcingFlag1);
@@ -553,6 +639,25 @@ TEST_F(VariationsSeedProcessorTest,
   }
 }
 
+TEST_F(VariationsSeedProcessorTest, TestAllowVariationIdWithForcingFlag) {

[Alexei Svitkine (slow), 2013/11/15 15:20:43]

Nit: TestAllowVariationIdWithForcingFlag -> AllowVariationIdWithForcingFlag (no
need for an extra Test prefix in the name).

[yao, 2013/11/15 18:51:49]

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? 

[Alexei Svitkine (slow), 2013/11/15 19:00:56]

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.

[yao, 2013/11/15 19:12:25]

Done.

+  VariationsSeedProcessor seed_processor;
+  Study study = CreateStudyWithFlagGroups(100, 0, 0);
+  EXPECT_FALSE(seed_processor.AllowVariationIdWithForcingFlag(study));
+
+  study.mutable_filter()->add_channel(Study_Channel_DEV);
+  EXPECT_FALSE(seed_processor.AllowVariationIdWithForcingFlag(study));
+
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_ANDROID);
+  EXPECT_TRUE(seed_processor.AllowVariationIdWithForcingFlag(study));
+
+  study.mutable_filter()->add_channel(Study_Channel_CANARY);
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_IOS);
+  EXPECT_TRUE(seed_processor.AllowVariationIdWithForcingFlag(study));
+
+  study.mutable_filter()->add_platform(Study_Platform_PLATFORM_WINDOWS);
+  EXPECT_FALSE(seed_processor.AllowVariationIdWithForcingFlag(study));
+}
+
 TEST_F(VariationsSeedProcessorTest, ValidateStudy) {
   VariationsSeedProcessor seed_processor;