Expand FeatureList to support FieldTrial association.

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 437c611fb39878a917a3879dea2189df7b38e69a..dad2406da307bb74a607d344882ba9562fbec035 100644
--- a/components/variations/variations_seed_processor_unittest.cc
+++ b/components/variations/variations_seed_processor_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/variations/processed_study.h"
@@ -119,16 +120,22 @@ class VariationsSeedProcessorTest : public ::testing::Test {
   }
 
   bool CreateTrialFromStudy(const Study* study) {
+    return CreateTrialFromStudyWithFeatureList(study, &feature_list_);
+  }
+
+  bool CreateTrialFromStudyWithFeatureList(const Study* study,
+                                           base::FeatureList* feature_list) {
     ProcessedStudy processed_study;
     if (processed_study.Init(study, false)) {
       VariationsSeedProcessor().CreateTrialFromStudy(
-          processed_study, override_callback_.callback());
+          processed_study, override_callback_.callback(), feature_list);
       return true;
     }
     return false;
   }
 
  protected:
+  base::FeatureList feature_list_;
   TestOverrideStringCallback override_callback_;
 
  private:
@@ -226,23 +233,27 @@ TEST_F(VariationsSeedProcessorTest,
   // Check that adding [expired, non-expired] activates the non-expired one.
   ASSERT_EQ(std::string(), base::FieldTrialList::FindFullName(kTrialName));
   {
+    base::FeatureList feature_list;
     base::FieldTrialList field_trial_list(NULL);
     study1->set_expiry_date(TimeToProtoTime(year_ago));
     seed_processor.CreateTrialsFromSeed(
         seed, "en-CA", base::Time::Now(), version, Study_Channel_STABLE,
-        Study_FormFactor_DESKTOP, "", "", "", override_callback_.callback());
+        Study_FormFactor_DESKTOP, "", "", "", override_callback_.callback(),
+        &feature_list);
     EXPECT_EQ(kGroup1Name, base::FieldTrialList::FindFullName(kTrialName));
   }
 
   // Check that adding [non-expired, expired] activates the non-expired one.
   ASSERT_EQ(std::string(), base::FieldTrialList::FindFullName(kTrialName));
   {
+    base::FeatureList feature_list;
     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, "", "", "", override_callback_.callback());
+        Study_FormFactor_DESKTOP, "", "", "", override_callback_.callback(),
+        &feature_list);
     EXPECT_EQ(kGroup1Name, base::FieldTrialList::FindFullName(kTrialName));
   }
 }
@@ -447,7 +458,7 @@ TEST_F(VariationsSeedProcessorTest, StartsActive) {
   seed_processor.CreateTrialsFromSeed(
       seed, "en-CA", base::Time::Now(), base::Version("20.0.0.0"),
       Study_Channel_STABLE, Study_FormFactor_DESKTOP, "", "", "",
-      override_callback_.callback());
+      override_callback_.callback(), &feature_list_);
 
   // Non-specified and ACTIVATION_EXPLICIT should not start active, but
   // ACTIVATION_AUTO should.
@@ -504,4 +515,151 @@ TEST_F(VariationsSeedProcessorTest, ForcingFlagAlreadyForced) {
   EXPECT_EQ(kExperimentId, id);
 }
 
+TEST_F(VariationsSeedProcessorTest, FeatureEnabledOrDisableByTrial) {
+  struct base::Feature kFeatureOffByDefault {
+    "kOff", base::FEATURE_DISABLED_BY_DEFAULT
+  };
+  struct base::Feature kFeatureOnByDefault {
+    "kOn", base::FEATURE_ENABLED_BY_DEFAULT
+  };
+  struct base::Feature kUnrelatedFeature {
+    "kUnrelated", base::FEATURE_DISABLED_BY_DEFAULT
+  };
+
+  struct {
+    const char* enable_feature;
+    const char* disable_feature;
+    bool expected_feature_off_state;
+    bool expected_feature_on_state;
+  } test_cases[] = {
+      {nullptr, nullptr, false, true},
+      {kFeatureOnByDefault.name, nullptr, false, true},
+      {kFeatureOffByDefault.name, nullptr, true, true},
+      {nullptr, kFeatureOnByDefault.name, false, false},
+      {nullptr, kFeatureOffByDefault.name, false, true},
+  };
+
+  for (size_t i = 0; i < arraysize(test_cases); i++) {
+    const auto& test_case = test_cases[i];
+
+    base::FieldTrialList field_trial_list(NULL);
+    base::FeatureList::ClearInstanceForTesting();

[Ilya Sherman, 2015/09/16 00:50:44]

Should this also be called at the end of each test, to avoid munging global
state?

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Good point. Put in the dtor of the harness.

+    scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
+
+    Study study;
+    study.set_name("Study1");
+    study.set_default_experiment_name("B");
+    AddExperiment("B", 0, &study);
+
+    Study_Experiment* experiment = AddExperiment("A", 1, &study);
+    Study_Experiment_FeatureAssociation* association =
+        experiment->mutable_feature_association();
+    if (test_case.enable_feature)
+      association->add_enable_feature(test_case.enable_feature);
+    else if (test_case.disable_feature)
+      association->add_disable_feature(test_case.disable_feature);
+
+    EXPECT_TRUE(
+        CreateTrialFromStudyWithFeatureList(&study, feature_list.get()));
+    base::FeatureList::SetInstance(feature_list.Pass());
+
+    // |kUnrelatedFeature| should not be affected.
+    EXPECT_FALSE(base::FeatureList::IsEnabled(kUnrelatedFeature));
+
+    // Before the associated feature is queried, the trial shouldn't be active.
+    EXPECT_FALSE(IsFieldTrialActive(study.name()));
+
+    EXPECT_EQ(test_case.expected_feature_off_state,
+              base::FeatureList::IsEnabled(kFeatureOffByDefault));
+    EXPECT_EQ(test_case.expected_feature_on_state,
+              base::FeatureList::IsEnabled(kFeatureOnByDefault));
+
+    // The field trial should get activated if it had a feature association.
+    const bool expected_field_trial_active =
+        test_case.enable_feature || test_case.disable_feature;
+    EXPECT_EQ(expected_field_trial_active, IsFieldTrialActive(study.name()));
+  }
+}
+
+TEST_F(VariationsSeedProcessorTest, FeatureForcingTrialReportingGroup) {
+  struct base::Feature kFeatureOffByDefault {
+    "kOff", base::FEATURE_DISABLED_BY_DEFAULT
+  };
+  struct base::Feature kFeatureOnByDefault {
+    "kOn", base::FEATURE_ENABLED_BY_DEFAULT
+  };
+
+  struct {
+    const base::Feature& feature;
+    bool set_forcing_feature_on;
+    bool set_forcing_feature_off;
+    const char* enable_features_command_line;
+    const char* disable_features_command_line;
+    bool expected_feature_state;
+    bool expected_trial_activated;
+  } test_cases[] = {
+      {kFeatureOffByDefault, false, false, "", "", false, false},
+      {kFeatureOffByDefault, true, false, kFeatureOffByDefault.name, "", true,
+       true},
+      {kFeatureOffByDefault, false, true, kFeatureOffByDefault.name, "", true,
+       false},
+      {kFeatureOffByDefault, false, true, "", kFeatureOffByDefault.name, false,
+       true},
+      {kFeatureOffByDefault, true, false, "", kFeatureOffByDefault.name, false,
+       false},
+      {kFeatureOnByDefault, false, false, "", "", true, false},
+      {kFeatureOnByDefault, true, false, kFeatureOnByDefault.name, "", true,
+       true},
+      {kFeatureOnByDefault, false, true, kFeatureOnByDefault.name, "", true,
+       false},
+      {kFeatureOnByDefault, false, true, "", kFeatureOnByDefault.name, false,
+       true},
+      {kFeatureOnByDefault, true, false, "", kFeatureOnByDefault.name, false,
+       false},
+  };

[Ilya Sherman, 2015/09/16 00:50:44]

This test is pretty dense, making it hard to read.  It might be significantly
clearer to just write 10 different, brief test cases; rather than trying to
share code in this way.  (You could factor out some of the boilerplate-y shared
setup code into a helper function to keep the tests short and clear.)

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Let me think about. I started it as individual tests, but made a table test
given the amount of test coverage I wanted. Plus, now you've given some
suggestions for additional tests.

Will save the thought for next patch set which will add extra tests.

[Alexei Svitkine (slow), 2015/09/23 22:01:27]

Thinking about this more, I agree that this test is hard to follow, but I also
think that splitting the cases into separate tests which just be way too much
boilerplate. Additionally, the test wasn't covering as many cases as we wanted.

So I went and re-wrote this test with a slightly different structure. The new
test is still table-driven, but I believe more clear and covers a lot more
cases. I've also added comments and grouped the entries in the table together to
make it clear what each section is trying to cover. I think it's much better
now, PTAL.

[Ilya Sherman, 2015/09/24 01:11:32]

The comments help a bunch -- thanks!

I think my main frustration with these table-based tests is not that they're
table based, but that they use a relatively complex backing struct, with several
boolean fields and string fields that are frequently empty.  So, when reading
the code, I have to do a lot of cross referencing to understand what an
individual "" means or what "false, true" means.  Maybe as a compromise, we
could have one TEST_F per grouped block of tests?  I think that might make the
logic easier to follow, since each test would have a simpler struct needed to
define the inputs.

FWIW, here are some TOTT articles that come to mind for me:
go/tott/2014/07/episode-338-dont-put-logic-in-tests.html
go/tott/2014/07/episode-339-writing-descriptive-test.html
go/tott/2014/03/episode-322-test-behaviors-not-methods.html

[Alexei Svitkine (slow), 2015/09/24 20:49:21]

I agree that complicated table-driven tests are hard to read. However, I think
my updated code actually has a good balance between the two. Sure, you do have
to go up to figure out what "" is, but hopefully this is not something too many
people have to do - other than during this code review and if the logic needs
updating.

I don't think the splitting the different blocks of tests in the table are super
useful in this case. Particularly, because this test basically is a sort of
"brute force" case where it tries all the different combinations of
{default_feature_state, enabled_by_cli, disable_by_cli, dice_rolled_group} for
the config.

 And splitting some of those off just makes the test seem incomplete - and also
would result in more boilerplate (i.e. either the code from 622 to 661 would
essentially be duplicated, or we have to make a helper method which makes the
test harder to follow).

So I don't find splitting this specific test compelling. On the other hand, I
agree that it's worth having a few more tests for this general functionality and
plan to add some, to cover some of your bullet point cases from earlier
comments. Haven't gotten to that yet, though.

+
+  for (size_t i = 0; i < arraysize(test_cases); i++) {
+    const auto& test_case = test_cases[i];
+
+    base::FieldTrialList field_trial_list(NULL);
+    base::FeatureList::ClearInstanceForTesting();
+    scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->InitializeFromCommandLine(
+        test_case.enable_features_command_line,
+        test_case.disable_features_command_line);
+
+    Study study;
+    study.set_name("Study1");
+    study.set_default_experiment_name("B");
+    Study_Experiment* experiment_with_probability =
+        AddExperiment("B", 1, &study);
+
+    Study_Experiment* experiment = AddExperiment("A", 0, &study);
+    Study_Experiment_FeatureAssociation* association =
+        experiment->mutable_feature_association();
+    if (test_case.set_forcing_feature_on)
+      association->set_forcing_feature_on(test_case.feature.name);
+    else if (test_case.set_forcing_feature_off)
+      association->set_forcing_feature_off(test_case.feature.name);
+
+    EXPECT_TRUE(
+        CreateTrialFromStudyWithFeatureList(&study, feature_list.get()));
+    base::FeatureList::SetInstance(feature_list.Pass());
+
+    // Trial should not be activated initially.
+    EXPECT_FALSE(IsFieldTrialActive(study.name()));
+    EXPECT_EQ(test_case.expected_feature_state,
+              base::FeatureList::IsEnabled(test_case.feature));
+    EXPECT_EQ(test_case.expected_trial_activated,
+              IsFieldTrialActive(study.name()));
+    if (test_case.expected_trial_activated) {
+      EXPECT_EQ(experiment->name(),
+                base::FieldTrialList::FindFullName(study.name()));
+    } else {
+      EXPECT_EQ(experiment_with_probability->name(),
+                base::FieldTrialList::FindFullName(study.name()));
+    }
+  }
+}

[Ilya Sherman, 2015/09/16 00:50:44]

It would be nice to add tests for what happens in corner cases such as:

  * Two studies both try to activate the same feature.
  * Two experiments within a study both try to activate the same feature.
  * One study tries to activate a feature, while another study tries to
deactivate it.
  * Two studies both try to associate with a single feature when it's turned on.
  * Two experiments within a study both try to associate with a single feature
when it's turned on.
  * Two studies try to associate with a single feature in opposite on/off
states.
  * Two experiments within a study try to associate with a single feature in
opposite on/off states.
  * An experiment tries to simultaneously associate with a feature both in its
on and off state.
  * An experiment tries to simultaneously associate with a feature in its on
state, and activate that feature.
  * An experiment tries to simultaneously associate with a feature in its on
state, and deactivate that feature.

[Alexei Svitkine (slow), 2015/09/16 20:54:08]

This is meant to be enforced server-side. I guess I can add a test to document
what the client does in that case, but the intention is for this to never happen
through server-side enforcement.
There's no problem there. Only the chosen experiment should have effect. But if
they're both doing the same thing, then I'm not sure what this will be testing
for? That having a second experiment with the same config doesn't make the
chosen experiment do nothing for some reason?
Again, should be enforced server-side.
Ditto.
Again, server-side validated. Same thing as two forcing_flags for the same flag.
Again, similar to two studies trying to associate to same feature, not allowed
via server-side checking.
Since this is an expected use case, seems reasonable to add.
Again, shouldn't be allowed via server-side checks.
Seems reasonable to test, but note that the experiment is associating with the
"feature being turned on from command-line", not "in its on state", but worth
testing.
Ditto.

[Ilya Sherman, 2015/09/17 01:02:46]

I guess for things that are strictly enforced server-side, it might be useful to
just include a comment at the bottom of this file listing those things as not
needing testing.  Would be even better to provide test coverage just for
completeness, but I realize that it's kind of a pain, and has little practical
value if we really do strictly enforce these checks server-side.
I think you're right that this case is fine.
Yep, I understood that.  Is this something that we want to support, or protect
against on the server-side?  It doesn't make sense to me for an experiment to be
set up this way.

+
 }  // namespace variations