Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG=713390

Review-Url: https://codereview.chromium.org/2834583002
Cr-Commit-Position: refs/heads/master@{#468210}

base/BUILD.gn

@@ -2130,6 +2130,7 @@ test("base_unittests") {
     "template_util_unittest.cc",
     "test/histogram_tester_unittest.cc",
     "test/mock_callback_unittest.cc",
+    "test/scoped_feature_list_unittest.cc",
     "test/scoped_mock_time_message_loop_task_runner_unittest.cc",
     "test/scoped_task_scheduler_unittest.cc",
     "test/test_pending_task_unittest.cc",

base/test/scoped_feature_list.cc

@@ -4,24 +4,79 @@
 
 #include "base/test/scoped_feature_list.h"
 
+#include <algorithm>
 #include <string>
+#include <vector>
+
+#include "base/stl_util.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 
 namespace base {
 namespace test {
 
 namespace {
 
-static std::string GetFeatureString(
+std::vector<StringPiece> GetFeatureVector(
     const std::initializer_list<base::Feature>& features) {
-  std::string output;
+  std::vector<StringPiece> output;
   for (const base::Feature& feature : features) {
-    if (!output.empty())
-      output += ",";
-    output += feature.name;
+    output.push_back(feature.name);
   }
+
   return output;
 }
 
+// Extracts a feature name from a feature state string. For example, given
+// the input "*MyLovelyFeature<SomeFieldTrial", returns "MyLovelyFeature".
+StringPiece GetFeatureName(StringPiece feature) {
+  StringPiece feature_name = feature;
+
+  // Remove default info.
+  if (feature_name.starts_with("*"))
+    feature_name = feature_name.substr(1);
+
+  // Remove field_trial info.
+  std::size_t index = feature_name.find("<");
+  if (index != std::string::npos)
+    feature_name = feature_name.substr(0, index);
+
+  return feature_name;
+}
+
+struct Features {
+  std::vector<StringPiece> enabled_feature_list;
+  std::vector<StringPiece> disabled_feature_list;
+};
+
+// Merges previously-specified feature overrides with those passed into one of
+// the Init() methods. |features| should be a list of features previously
+// overridden to be in the |override_state|. |merged_features| should contain
+// the enabled and disabled features passed into the Init() method, plus any
+// overrides merged as a result of previous calls to this function.
+void OverrideFeatures(const std::string& features,
+                      base::FeatureList::OverrideState override_state,
+                      Features* merged_features) {
+  std::vector<StringPiece> features_list =
+      SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+
+  for (StringPiece feature : features_list) {
+    StringPiece feature_name = GetFeatureName(feature);
+
+    if (ContainsValue(merged_features->enabled_feature_list, feature_name) ||
+        ContainsValue(merged_features->disabled_feature_list, feature_name))
+      continue;
+
+    if (override_state == FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE) {
+      merged_features->enabled_feature_list.push_back(feature);
+    } else {
+      DCHECK_EQ(override_state,
+                FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE);
+      merged_features->disabled_feature_list.push_back(feature);
+    }
+  }
+}
+
 }  // namespace
 
 ScopedFeatureList::ScopedFeatureList() {}
@@ -40,13 +95,6 @@ void ScopedFeatureList::Init() {
   InitWithFeatureList(std::move(feature_list));
 }
 
-void ScopedFeatureList::InitWithFeatures(
-    const std::initializer_list<base::Feature>& enabled_features,
-    const std::initializer_list<base::Feature>& disabled_features) {
-  InitFromCommandLine(GetFeatureString(enabled_features),
-                      GetFeatureString(disabled_features));
-}
-
 void ScopedFeatureList::InitWithFeatureList(
     std::unique_ptr<FeatureList> feature_list) {
   DCHECK(!original_feature_list_);
@@ -62,12 +110,43 @@ void ScopedFeatureList::InitFromCommandLine(
   InitWithFeatureList(std::move(feature_list));
 }
 
+void ScopedFeatureList::InitWithFeatures(
+    const std::initializer_list<base::Feature>& enabled_features,
+    const std::initializer_list<base::Feature>& disabled_features) {
+  Features merged_features;
+  merged_features.enabled_feature_list = GetFeatureVector(enabled_features);
+  merged_features.disabled_feature_list = GetFeatureVector(disabled_features);
+
+  base::FeatureList* feature_list = base::FeatureList::GetInstance();
+
+  // |current_enabled_features| and |current_disabled_features| must declare out
+  // of if scope to avoid them out of scope before JoinString calls because
+  // |merged_features| may contains StringPiece which holding pointer points to
+  // |current_enabled_features| and |current_disabled_features|.
+  std::string current_enabled_features;
+  std::string current_disabled_features;
+  if (feature_list) {
+    base::FeatureList::GetInstance()->GetFeatureOverrides(
+        &current_enabled_features, &current_disabled_features);
+    OverrideFeatures(current_enabled_features,
+                     FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+                     &merged_features);
+    OverrideFeatures(current_disabled_features,
+                     FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE,
+                     &merged_features);
+  }
+
+  std::string enabled = JoinString(merged_features.enabled_feature_list, ",");
+  std::string disabled = JoinString(merged_features.disabled_feature_list, ",");
+  InitFromCommandLine(enabled, disabled);
+}
+
 void ScopedFeatureList::InitAndEnableFeature(const base::Feature& feature) {
-  InitFromCommandLine(feature.name, std::string());
+  InitWithFeatures({feature}, {});
 }
 
 void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) {
-  InitFromCommandLine(std::string(), feature.name);
+  InitWithFeatures({}, {feature});
 }
 
 }  // namespace test

base/test/scoped_feature_list.h

@@ -22,29 +22,42 @@ class ScopedFeatureList final {
   ScopedFeatureList();
   ~ScopedFeatureList();
 
+  // WARNING: This method will reset any globally configured features to their
+  // default values, which can hide feature interaction bugs. Please use
+  // sparingly.  https://crbug.com/713390
   // Initializes and registers a FeatureList instance with no overrides.
   void Init();
 
+  // WARNING: This method will reset any globally configured features to their
+  // default values, which can hide feature interaction bugs. Please use
+  // sparingly.  https://crbug.com/713390
   // Initializes and registers the given FeatureList instance.
   void InitWithFeatureList(std::unique_ptr<FeatureList> feature_list);
 
-  // Initializes and registers a FeatureList instance with the given enabled
-  // and disabled features.
-  void InitWithFeatures(
-      const std::initializer_list<base::Feature>& enabled_features,
-      const std::initializer_list<base::Feature>& disabled_features);
-
-  // Initializes and registers a FeatureList instance with the given
+  // WARNING: This method will reset any globally configured features to their
+  // default values, which can hide feature interaction bugs. Please use
+  // sparingly.  https://crbug.com/713390
+  // Initializes and registers a FeatureList instance with only the given
   // enabled and disabled features (comma-separated names).
   void InitFromCommandLine(const std::string& enable_features,
                            const std::string& disable_features);
 
-  // Initializes and registers a FeatureList instance enabling a single
-  // feature.
+  // Initializes and registers a FeatureList instance based on present
+  // FeatureList and overridden with the given enabled and disabled features.
+  // Any feature overrides already present in the global FeatureList will
+  // continue to apply, unless they conflict with the overrides passed into this
+  // method. This is important for testing potentially unexpected feature
+  // interactions.
+  void InitWithFeatures(
+      const std::initializer_list<base::Feature>& enabled_features,
+      const std::initializer_list<base::Feature>& disabled_features);
+
+  // Initializes and registers a FeatureList instance based on present
+  // FeatureList and overridden with single enabled feature.
   void InitAndEnableFeature(const base::Feature& feature);
 
-  // Initializes and registers a FeatureList instance disabling a single
-  // feature.
+  // Initializes and registers a FeatureList instance based on present
+  // FeatureList and overridden with single disabled feature.
   void InitAndDisableFeature(const base::Feature& feature);
 
  private:

base/test/scoped_feature_list_unittest.cc

+// Copyright 2017 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 "base/test/scoped_feature_list.h"
+
+#include <string>
+#include "base/metrics/field_trial.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace test {
+
+namespace {
+
+const base::Feature kTestFeature1{"TestFeature1",
+                                  base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kTestFeature2{"TestFeature2",
+                                  base::FEATURE_DISABLED_BY_DEFAULT};
+
+void ExpectFeatures(const std::string& enabled_features,
+                    const std::string& disabled_features) {
+  base::FeatureList* list = base::FeatureList::GetInstance();
+  std::string actual_enabled_features;
+  std::string actual_disabled_features;
+
+  list->GetFeatureOverrides(&actual_enabled_features,
+                            &actual_disabled_features);
+
+  EXPECT_EQ(enabled_features, actual_enabled_features);
+  EXPECT_EQ(disabled_features, actual_disabled_features);
+}
+
+}  // namespace
+
+class ScopedFeatureListTest : public testing::Test {
+ public:
+  ScopedFeatureListTest() {
+    // Clear default feature list.
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->InitializeFromCommandLine(std::string(), std::string());
+    original_feature_list_ = base::FeatureList::ClearInstanceForTesting();
+    base::FeatureList::SetInstance(std::move(feature_list));
+  }
+
+  ~ScopedFeatureListTest() override {
+    // Restore feature list.
+    if (original_feature_list_) {
+      base::FeatureList::ClearInstanceForTesting();
+      base::FeatureList::RestoreInstanceForTesting(
+          std::move(original_feature_list_));
+    }
+  }
+
+ private:
+  // Save the present FeatureList and restore it after test finish.
+  std::unique_ptr<FeatureList> original_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedFeatureListTest);
+};
+
+TEST_F(ScopedFeatureListTest, BasicScoped) {
+  ExpectFeatures(std::string(), std::string());
+  EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
+  {
+    test::ScopedFeatureList feature_list1;
+    feature_list1.InitFromCommandLine("TestFeature1", std::string());
+    ExpectFeatures("TestFeature1", std::string());
+    EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
+  }
+  ExpectFeatures(std::string(), std::string());
+  EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
+}
+
+TEST_F(ScopedFeatureListTest, EnableFeatureOverrideDisable) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitWithFeatures({}, {kTestFeature1});
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({kTestFeature1}, {});
+    ExpectFeatures("TestFeature1", std::string());
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideNotMakeDuplicate) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitWithFeatures({}, {kTestFeature1});
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({}, {kTestFeature1});
+    ExpectFeatures(std::string(), "TestFeature1");
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDefault) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitFromCommandLine("*TestFeature1", std::string());
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({kTestFeature1}, {});
+    ExpectFeatures("TestFeature1", std::string());
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDefault2) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitFromCommandLine("*TestFeature1", std::string());
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({}, {kTestFeature1});
+    ExpectFeatures(std::string(), "TestFeature1");
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithEnabledFieldTried) {
+  test::ScopedFeatureList feature_list1;
+
+  std::unique_ptr<FeatureList> feature_list(new FeatureList);
+  FieldTrialList field_trial_list(nullptr);
+  FieldTrial* trial = FieldTrialList::CreateFieldTrial("TrialExample", "A");
+  feature_list->RegisterFieldTrialOverride(
+      kTestFeature1.name, FeatureList::OVERRIDE_ENABLE_FEATURE, trial);
+  feature_list1.InitWithFeatureList(std::move(feature_list));
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({kTestFeature1}, {});
+    ExpectFeatures("TestFeature1", std::string());
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDisabledFieldTried) {
+  test::ScopedFeatureList feature_list1;
+
+  std::unique_ptr<FeatureList> feature_list(new FeatureList);
+  FieldTrialList field_trial_list(nullptr);
+  FieldTrial* trial = FieldTrialList::CreateFieldTrial("TrialExample", "A");
+  feature_list->RegisterFieldTrialOverride(
+      kTestFeature1.name, FeatureList::OVERRIDE_DISABLE_FEATURE, trial);
+  feature_list1.InitWithFeatureList(std::move(feature_list));
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({kTestFeature1}, {});
+    ExpectFeatures("TestFeature1", std::string());
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingFeature) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitWithFeatures({}, {kTestFeature1});
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({}, {kTestFeature2});
+    EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
+    EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature2));
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingFeature2) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitWithFeatures({}, {kTestFeature1});
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({kTestFeature2}, {});
+    ExpectFeatures("TestFeature2", "TestFeature1");
+  }
+}
+
+TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingDefaultFeature) {
+  test::ScopedFeatureList feature_list1;
+  feature_list1.InitFromCommandLine("*TestFeature1", std::string());
+
+  {
+    test::ScopedFeatureList feature_list2;
+    feature_list2.InitWithFeatures({}, {kTestFeature2});
+    ExpectFeatures("*TestFeature1", "TestFeature2");
+  }
+}
+
+}  // namespace test
+}  // namespace base