Change variations default feature association to use all features.

Previously, it would only take effect when a single feature was
enabled on the study. The new logic would associate with all the
features.

Bug: 816696
Change-Id: Ide5ace721ade5e78f8743b11a96998d3b08ded46
Reviewed-on: https://chromium-review.googlesource.com/963087
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543552}

components/variations/processed_study.cc

@@ -39,10 +39,10 @@ bool ValidateStudyAndComputeTotalProbability(
 
   bool multiple_assigned_groups = false;
   bool found_default_group = false;
-  std::string single_feature_name_seen;
-  bool has_multiple_features = false;
 
   std::set<std::string> experiment_names;
+  std::set<std::string> features_to_associate;
+
   for (int i = 0; i < study.experiment_size(); ++i) {
     const Study_Experiment& experiment = study.experiment(i);
     if (experiment.name().empty()) {
@@ -55,25 +55,17 @@ bool ValidateStudyAndComputeTotalProbability(
       return false;
     }
 
-    if (!has_multiple_features) {
+    // Note: This checks for ACTIVATION_EXPLICIT, since there is no reason to
+    // have this association with ACTIVATION_AUTO (where the trial starts
+    // active), as well as allowing flexibility to disable this behavior in the
+    // future from the server by introducing a new activation type.
+    if (study.activation_type() == Study_ActivationType_ACTIVATION_EXPLICIT) {
       const auto& features = experiment.feature_association();
       for (int i = 0; i < features.enable_feature_size(); ++i) {
-        const std::string& feature_name = features.enable_feature(i);
-        if (single_feature_name_seen.empty()) {
-          single_feature_name_seen = feature_name;
-        } else if (feature_name != single_feature_name_seen) {
-          has_multiple_features = true;
-          break;
-        }
+        features_to_associate.insert(features.enable_feature(i));
       }
       for (int i = 0; i < features.disable_feature_size(); ++i) {
-        const std::string& feature_name = features.disable_feature(i);
-        if (single_feature_name_seen.empty()) {
-          single_feature_name_seen = feature_name;
-        } else if (feature_name != single_feature_name_seen) {
-          has_multiple_features = true;
-          break;
-        }
+        features_to_associate.insert(features.disable_feature(i));
       }
     }
 
@@ -97,18 +89,13 @@ bool ValidateStudyAndComputeTotalProbability(
     return false;
   }
 
-  // Check if this study enables/disables a single feature and uses explicit
-  // activation (i.e. the trial should be activated when queried). If so, ensure
-  // that groups that don't explicitly enable/disable that feature are still
-  // associated with it (i.e. so "Default" group gets reported).
-  //
-  // Note: This checks for ACTIVATION_EXPLICIT, since there is no reason to
-  // have this association with ACTIVATION_AUTO (where the trial starts active),
-  // as well as allowing flexibility to disable this behavior in the future from
-  // the server by introducing a new activation type.
-  if (!has_multiple_features && !single_feature_name_seen.empty() &&
-      study.activation_type() == Study_ActivationType_ACTIVATION_EXPLICIT) {
-    associated_features->push_back(single_feature_name_seen);
+  // Ensure that groups that don't explicitly enable/disable that feature get
+  // associated with all features in the study (i.e. so "Default" group gets
+  // reported).
+  if (!features_to_associate.empty()) {
+    associated_features->insert(associated_features->end(),
+                                features_to_associate.begin(),
+                                features_to_associate.end());
   }
 
   *total_probability = divisor;

components/variations/variations_seed_processor_unittest.cc

@@ -404,7 +404,7 @@ TEST_F(VariationsSeedProcessorTest, ValidateStudy) {
   EXPECT_FALSE(processed_study.Init(&study, false));
 }
 
-TEST_F(VariationsSeedProcessorTest, ValidateStudySingleFeature) {
+TEST_F(VariationsSeedProcessorTest, ValidateStudyWithAssociatedFeatures) {
   Study study;
   study.set_default_experiment_name("def");
   Study_Experiment* exp1 = AddExperiment("exp1", 100, &study);
@@ -430,9 +430,10 @@ TEST_F(VariationsSeedProcessorTest, ValidateStudySingleFeature) {
   exp1->mutable_feature_association()->add_enable_feature(kFeature1Name);
   exp1->mutable_feature_association()->add_enable_feature(kFeature2Name);
   EXPECT_TRUE(processed_study.Init(&study, false));
-  // Since there's multiple different features, |associated_features| should be
-  // unset.
-  EXPECT_THAT(processed_study.associated_features(), IsEmpty());
+  // Since there's multiple different features, |associated_features| should now
+  // contain them all.
+  EXPECT_THAT(processed_study.associated_features(),
+              ElementsAre(kFeature1Name, kFeature2Name));
 
   exp1->clear_feature_association();
   exp1->mutable_feature_association()->add_enable_feature(kFeature1Name);
@@ -443,9 +444,16 @@ TEST_F(VariationsSeedProcessorTest, ValidateStudySingleFeature) {
               ElementsAre(kFeature1Name));
 
   // Setting a different feature name on exp2 should cause |associated_features|
-  // to be not set.
+  // to contain both feature names.
   exp2->mutable_feature_association()->set_enable_feature(0, kFeature2Name);
   EXPECT_TRUE(processed_study.Init(&study, false));
+  EXPECT_THAT(processed_study.associated_features(),
+              ElementsAre(kFeature1Name, kFeature2Name));
+
+  // Setting a different activation type should result in empty
+  // |associated_features|.
+  study.set_activation_type(Study_ActivationType_ACTIVATION_AUTO);
+  EXPECT_TRUE(processed_study.Init(&study, false));
   EXPECT_THAT(processed_study.associated_features(), IsEmpty());
 }