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

components/variations/variations_seed_processor_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/variations/variations_seed_processor.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/strings/string_split.h"
@@ skipping 11 matching lines @@
 }
 
 // Constants for testing associating command line flags with trial groups.
 const char kFlagStudyName[] = "flag_test_trial";
 const char kFlagGroup1Name[] = "flag_group1";
 const char kFlagGroup2Name[] = "flag_group2";
 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) {
   Study_Experiment* experiment = study->add_experiment();
   experiment->set_name(name);
   experiment->set_probability_weight(probability);
   return experiment;
 }
 
 // Populates |study| with test data used for testing associating command line
@@ skipping 41 matching lines @@
     // Ensure that the maps are cleared between tests, since they are stored as
     // process singletons.
     testing::ClearAllVariationIDs();
     testing::ClearAllVariationParams();
   }
 
  private:
   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;
 
   const Study_Channel channels[] = {
     Study_Channel_CANARY,
     Study_Channel_DEV,
     Study_Channel_BETA,
     Study_Channel_STABLE,
   };
   bool channel_added[arraysize(channels)] = { 0 };
@@ skipping 314 matching lines @@
       seed, "en-CA", base::Time::Now(), base::Version("20.0.0.0"),
       Study_Channel_STABLE, Study_FormFactor_DESKTOP, &processed_studies);
 
   // Check that only the first kTrial1Name study was kept.
   ASSERT_EQ(2U, processed_studies.size());
   EXPECT_EQ(kTrial1Name, processed_studies[0].study->name());
   EXPECT_EQ(kGroup1Name, processed_studies[0].study->experiment(0).name());
   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);
 
   base::FieldTrialList field_trial_list(NULL);
 
   Study study = CreateStudyWithFlagGroups(100, 0, 0);
   VariationsSeedProcessor().CreateTrialFromStudy(
       ProcessedStudy(&study, 100, false));
 
@@ skipping 109 matching lines @@
     base::FieldTrialList field_trial_list(NULL);
     study1->clear_expiry_date();
     study2->set_expiry_date(TimeToProtoTime(year_ago));
     seed_processor.CreateTrialsFromSeed(seed, "en-CA", base::Time::Now(),
                                         version, Study_Channel_STABLE,
                                         Study_FormFactor_DESKTOP);
     EXPECT_EQ(kGroup1Name, base::FieldTrialList::FindFullName(kTrialName));
   }
 }
 
+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;
 
   Study study;
   study.set_default_experiment_name("def");
   AddExperiment("abc", 100, &study);
   Study_Experiment* default_group = AddExperiment("def", 200, &study);
 
   base::FieldTrial::Probability total_probability = 0;
   bool valid = seed_processor.ValidateStudyAndComputeTotalProbability(
@@ skipping 137 matching lines @@
   EXPECT_EQ("BB", base::FieldTrialList::FindFullName("B"));
   EXPECT_EQ("CC", base::FieldTrialList::FindFullName("C"));
 
   // Now, all studies should be active.
   EXPECT_TRUE(IsFieldTrialActive("A"));
   EXPECT_TRUE(IsFieldTrialActive("B"));
   EXPECT_TRUE(IsFieldTrialActive("C"));
 }
 
 }  // namespace chrome_variations