Auto-instantiate FieldTrialList for tests

Tests often have indirect dependencies to code that requires a
FieldTrialList instantiated. To work that around, many existing tests
manually instantiate a FieldTrialList, but this is cumbersome,
distracting and hard to clean up once it no longer is necessary.

Instead, this patch adopts base::test::ScopedFeatureList as part of the
test suite for all tests.

Follow-up patches will simplify tests.

Bug: 1018667
Change-Id: I15b1f1097357f0fc3a30d4ac337d77b1ef607098
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1883567
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714882}

base/feature_list.cc

@@ -86,6 +86,19 @@ bool IsValidFeatureOrFieldTrialName(const std::string& name) {
   return IsStringASCII(name) && name.find_first_of(",<*") == std::string::npos;
 }
 
+// TODO(crbug.com/1018667): Remove this function once the DCHECK has been turned
+// into an equality check.
+bool MatchesSelfOrAncestor(const FieldTrialList* expected,
+                           const FieldTrialList* actual) {
+  if (expected == actual)
+    return true;
+
+  if (actual == nullptr)
+    return false;
+
+  return MatchesSelfOrAncestor(expected, actual->GetPreviousGlobal());
+}
+
 }  // namespace
 
 #if defined(DCHECK_IS_CONFIGURABLE)
@@ -439,8 +452,11 @@ void FeatureList::GetFeatureOverridesImpl(std::string* enable_overrides,
   // active one. If not, it likely indicates that this FeatureList has override
   // entries from a freed FieldTrial, which may be caused by an incorrect test
   // set up.
-  if (field_trial_list_)
-    DCHECK_EQ(field_trial_list_, FieldTrialList::GetInstance());
+  // TODO(crbug.com/1018667): Turn the DCHECK below into a equality check.
+  if (field_trial_list_) {
+    DCHECK(MatchesSelfOrAncestor(field_trial_list_,
+                                 FieldTrialList::GetInstance()));
+  }
 
   enable_overrides->clear();
   disable_overrides->clear();

base/metrics/field_trial.cc

@@ -33,6 +33,9 @@ namespace base {
 
 namespace {
 
+// TODO(crbug.com/1018667): Remove when all unit tests are migrated.
+bool nested_field_trial_list_allowed_for_testing = false;
+
 // Define a separator character to use when creating a persistent form of an
 // instance.  This is intended for use as a command line argument, passed to a
 // second process to mimic our state (i.e., provide the same group name).
@@ -442,8 +445,14 @@ FieldTrialList::FieldTrialList(
     : entropy_provider_(std::move(entropy_provider)),
       observer_list_(new ObserverListThreadSafe<FieldTrialList::Observer>(
           ObserverListPolicy::EXISTING_ONLY)) {
-  DCHECK(!global_);
   DCHECK(!used_without_global_);
+
+  if (nested_field_trial_list_allowed_for_testing) {
+    previous_global_ = global_;
+  } else {
+    DCHECK(!global_);
+  }
+
   global_ = this;
 }
 
@@ -458,7 +467,7 @@ FieldTrialList::~FieldTrialList() {
   // likely caused by nested ScopedFeatureLists being destroyed in a different
   // order than they are initialized.
   DCHECK_EQ(this, global_);
-  global_ = nullptr;
+  global_ = previous_global_;
 }
 
 // static
@@ -1156,6 +1165,15 @@ void FieldTrialList::RestoreInstanceForTesting(FieldTrialList* instance) {
   global_ = instance;
 }
 
+// static
+void FieldTrialList::AllowNestedFieldTrialListForTesting() {
+  nested_field_trial_list_allowed_for_testing = true;
+}
+
+const FieldTrialList* FieldTrialList::GetPreviousGlobal() const {
+  return previous_global_;
+}
+
 // static
 std::string FieldTrialList::SerializeSharedMemoryRegionMetadata(
     const ReadOnlySharedMemoryRegion& shm) {

base/metrics/field_trial.h

@@ -674,6 +674,16 @@ class BASE_EXPORT FieldTrialList {
   // For testing, sets the global instance to |instance|.
   static void RestoreInstanceForTesting(FieldTrialList* instance);
 
+  // Allows nested instances of FieldTrialList without having to use the
+  // backup&restore methods above.
+  // TODO(crbug.com/1018667): Remove after all unit tests have been migrated.
+  static void AllowNestedFieldTrialListForTesting();
+
+  // Exposes the previous instance of FieldTrialNest, where |this| was nested.
+  // Null except in tests that use AllowNestedFieldTrialListForTesting().
+  // TODO(crbug.com/1018667): Remove after all unit tests have been migrated.
+  const FieldTrialList* GetPreviousGlobal() const;
+
  private:
   // Allow tests to access our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, InstantiateAllocator);
@@ -803,6 +813,9 @@ class BASE_EXPORT FieldTrialList {
   // Tracks whether CreateTrialsFromCommandLine() has been called.
   bool create_trials_from_command_line_called_ = false;
 
+  // TODO(crbug.com/1018667): Remove after all unit tests have been migrated.
+  FieldTrialList* previous_global_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(FieldTrialList);
 };
 

base/metrics/field_trial_unittest.cc

@@ -21,6 +21,7 @@
 #include "base/test/mock_entropy_provider.h"
 #include "base/test/multiprocess_test.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_field_trial_list_resetter.h"
 #include "base/test/task_environment.h"
 #include "base/test/test_shared_memory_util.h"
 #include "base/test/test_timeouts.h"
@@ -1165,6 +1166,7 @@ TEST(FieldTrialTestWithoutList, StatesStringFormat) {
 }
 
 TEST(FieldTrialDeathTest, OneTimeRandomizedTrialWithoutFieldTrialList) {
+  test::ScopedFieldTrialListResetter resetter;
   // Trying to instantiate a one-time randomized field trial before the
   // FieldTrialList is created should crash.
   EXPECT_DEATH_IF_SUPPORTED(

base/test/BUILD.gn

@@ -97,6 +97,8 @@ static_library("test_support") {
     "scoped_environment_variable_override.h",
     "scoped_feature_list.cc",
     "scoped_feature_list.h",
+    "scoped_field_trial_list_resetter.cc",
+    "scoped_field_trial_list_resetter.h",
     "scoped_mock_clock_override.cc",
     "scoped_mock_clock_override.h",
     "scoped_mock_time_message_loop_task_runner.cc",

base/test/scoped_field_trial_list_resetter.cc

+// Copyright 2019 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_field_trial_list_resetter.h"
+
+#include "base/metrics/field_trial.h"
+
+namespace base {
+namespace test {
+
+ScopedFieldTrialListResetter::ScopedFieldTrialListResetter()
+    : original_field_trial_list_(
+          base::FieldTrialList::BackupInstanceForTesting()) {}
+
+ScopedFieldTrialListResetter::~ScopedFieldTrialListResetter() {
+  base::FieldTrialList::RestoreInstanceForTesting(original_field_trial_list_);
+}
+
+}  // namespace test
+}  // namespace base

base/test/scoped_field_trial_list_resetter.h

+// Copyright 2019 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.
+
+#ifndef BASE_TEST_SCOPED_FIELD_TRIAL_LIST_RESETTER_H_
+#define BASE_TEST_SCOPED_FIELD_TRIAL_LIST_RESETTER_H_
+
+namespace base {
+
+class FieldTrialList;
+
+namespace test {
+
+// DISCLAIMER: Please use ScopedFeatureList except for advanced cases where
+// custom instantiation of FieldTrialList is required.
+//
+// ScopedFieldTrialListResetter resets the global FieldTrialList instance to
+// null, and restores the original state when the class goes out of scope. This
+// allows client code to initialize FieldTrialList instances in a custom
+// fashion.
+class ScopedFieldTrialListResetter final {
+ public:
+  ScopedFieldTrialListResetter();
+  ScopedFieldTrialListResetter(const ScopedFieldTrialListResetter&) = delete;
+  ScopedFieldTrialListResetter(ScopedFieldTrialListResetter&&) = delete;
+
+  ~ScopedFieldTrialListResetter();
+
+ private:
+  base::FieldTrialList* const original_field_trial_list_;
+};
+
+}  // namespace test
+}  // namespace base
+
+#endif  // BASE_TEST_SCOPED_FIELD_TRIAL_LIST_RESETTER_H_

base/test/test_suite.cc

@@ -23,6 +23,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
 #include "base/no_destructor.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
@@ -34,7 +35,9 @@
 #include "base/test/gtest_xml_util.h"
 #include "base/test/icu_test_util.h"
 #include "base/test/launcher/unit_test_launcher.h"
+#include "base/test/mock_entropy_provider.h"
 #include "base/test/multiprocess_test.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
@@ -119,6 +122,70 @@ class ResetCommandLineBetweenTests : public testing::EmptyTestEventListener {
   DISALLOW_COPY_AND_ASSIGN(ResetCommandLineBetweenTests);
 };
 
+// Initializes a base::test::ScopedFeatureList for each individual test, which
+// involves a FeatureList and a FieldTrialList, such that unit test don't need
+// to initialize them manually.
+class FeatureListScopedToEachTest : public testing::EmptyTestEventListener {
+ public:
+  FeatureListScopedToEachTest() {
+    // Allow nested FieldTrialList instances, for unit tests that instantiate
+    // them explicitly despite |field_trial_list_| being auto-instantiated here.
+    // TODO(crbug.com/1018667): Remove after all unit tests have been migrated.
+    base::FieldTrialList::AllowNestedFieldTrialListForTesting();
+  }
+
+  FeatureListScopedToEachTest(const FeatureListScopedToEachTest&) = delete;
+
+  ~FeatureListScopedToEachTest() override = default;
+
+  FeatureListScopedToEachTest& operator=(const FeatureListScopedToEachTest&) =
+      delete;
+
+  void OnTestStart(const testing::TestInfo& test_info) override {
+    field_trial_list_ = std::make_unique<FieldTrialList>(
+        std::make_unique<MockEntropyProvider>());
+
+    const CommandLine* command_line = CommandLine::ForCurrentProcess();
+
+    // Set up a FeatureList instance, so that code using that API will not hit a
+    // an error that it's not set. It will be cleared automatically.
+    // TestFeatureForBrowserTest1 and TestFeatureForBrowserTest2 used in
+    // ContentBrowserTestScopedFeatureListTest to ensure ScopedFeatureList keeps
+    // features from command line.
+    std::string enabled =
+        command_line->GetSwitchValueASCII(switches::kEnableFeatures);
+    std::string disabled =
+        command_line->GetSwitchValueASCII(switches::kDisableFeatures);
+    enabled += ",TestFeatureForBrowserTest1";
+    disabled += ",TestFeatureForBrowserTest2";
+    scoped_feature_list_.InitFromCommandLine(enabled, disabled);
+
+    // The enable-features and disable-features flags were just slurped into a
+    // FeatureList, so remove them from the command line. Tests should enable
+    // and disable features via the ScopedFeatureList API rather than
+    // command-line flags.
+    CommandLine new_command_line(command_line->GetProgram());
+    CommandLine::SwitchMap switches = command_line->GetSwitches();
+
+    switches.erase(switches::kEnableFeatures);
+    switches.erase(switches::kDisableFeatures);
+
+    for (const auto& iter : switches)
+      new_command_line.AppendSwitchNative(iter.first, iter.second);
+
+    *CommandLine::ForCurrentProcess() = new_command_line;
+  }
+
+  void OnTestEnd(const testing::TestInfo& test_info) override {
+    scoped_feature_list_.Reset();
+    field_trial_list_.reset();
+  }
+
+ private:
+  std::unique_ptr<FieldTrialList> field_trial_list_;
+  test::ScopedFeatureList scoped_feature_list_;
+};
+
 class CheckForLeakedGlobals : public testing::EmptyTestEventListener {
  public:
   CheckForLeakedGlobals() = default;
@@ -523,33 +590,6 @@ void TestSuite::Initialize() {
     debug::WaitForDebugger(60, true);
   }
 #endif
-  // Set up a FeatureList instance, so that code using that API will not hit a
-  // an error that it's not set. It will be cleared automatically.
-  // TestFeatureForBrowserTest1 and TestFeatureForBrowserTest2 used in
-  // ContentBrowserTestScopedFeatureListTest to ensure ScopedFeatureList keeps
-  // features from command line.
-  std::string enabled =
-      command_line->GetSwitchValueASCII(switches::kEnableFeatures);
-  std::string disabled =
-      command_line->GetSwitchValueASCII(switches::kDisableFeatures);
-  enabled += ",TestFeatureForBrowserTest1";
-  disabled += ",TestFeatureForBrowserTest2";
-  scoped_feature_list_.InitFromCommandLine(enabled, disabled);
-
-  // The enable-features and disable-features flags were just slurped into a
-  // FeatureList, so remove them from the command line. Tests should enable and
-  // disable features via the ScopedFeatureList API rather than command-line
-  // flags.
-  CommandLine new_command_line(command_line->GetProgram());
-  CommandLine::SwitchMap switches = command_line->GetSwitches();
-
-  switches.erase(switches::kEnableFeatures);
-  switches.erase(switches::kDisableFeatures);
-
-  for (const auto& iter : switches)
-    new_command_line.AppendSwitchNative(iter.first, iter.second);
-
-  *CommandLine::ForCurrentProcess() = new_command_line;
 
 #if defined(OS_IOS)
   InitIOSTestMessageLoop();
@@ -604,6 +644,7 @@ void TestSuite::Initialize() {
       testing::UnitTest::GetInstance()->listeners();
   listeners.Append(new DisableMaybeTests);
   listeners.Append(new ResetCommandLineBetweenTests);
+  listeners.Append(new FeatureListScopedToEachTest);
   if (check_for_leaked_globals_)
     listeners.Append(new CheckForLeakedGlobals);
   if (check_for_thread_and_process_priority_) {

base/test/test_suite.h

@@ -15,7 +15,6 @@
 #include "base/at_exit.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/test/trace_to_file.h"
 #include "build/build_config.h"
 
@@ -93,8 +92,6 @@ class TestSuite {
 
   bool initialized_command_line_ = false;
 
-  test::ScopedFeatureList scoped_feature_list_;
-
   XmlUnitTestResultPrinter* printer_ = nullptr;
 
   std::unique_ptr<logging::ScopedLogAssertHandler> assert_handler_;

chrome/test/base/browser_perf_tests_main.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"
 #include "chrome/test/base/chrome_test_launcher.h"
 #include "chrome/test/base/chrome_test_suite.h"
 

chromecast/app/cast_main_delegate.cc

@@ -221,8 +221,13 @@ void CastMainDelegate::PostEarlyInitialization(bool is_running_tests) {
   CHECK(base::CreateDirectory(home_dir));
 #endif  // !defined(OS_ANDROID)
 
-  // The |FieldTrialList| is a dependency of the feature list.
-  field_trial_list_ = std::make_unique<base::FieldTrialList>(nullptr);
+  // The |FieldTrialList| is a dependency of the feature list. In tests, it
+  // gets constructed as part of the test suite.
+  if (is_running_tests) {
+    DCHECK(base::FieldTrialList::GetInstance());
+  } else {
+    field_trial_list_ = std::make_unique<base::FieldTrialList>(nullptr);
+  }
 
   // Initialize the base::FeatureList and the PrefService (which it depends on),
   // so objects initialized after this point can use features from

gpu/command_buffer/tests/gl_tests_main.cc

@@ -5,7 +5,6 @@
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/command_line.h"
-#include "base/feature_list.h"
 #include "base/message_loop/message_pump.h"
 #include "base/task/single_thread_task_executor.h"
 #include "base/test/launcher/unit_test_launcher.h"
@@ -35,7 +34,6 @@ class GlTestsSuite : public base::TestSuite {
   void Initialize() override {
     base::TestSuite::Initialize();
 
-    base::FeatureList::InitializeInstance(std::string(), std::string());
     task_environment_ = std::make_unique<base::test::TaskEnvironment>(
         base::test::TaskEnvironment::MainThreadType::UI);
 #if defined(USE_OZONE)