Revert "Check that TaskScheduler is not leaked between tests, or test-cases."

This reverts commit a2a4e75d27.

Reason for revert: [sheriff] a culprit for cronet_tests failing on multiple builders
See https://crbug.com/877355 for details.

Original change's description:
> Check that TaskScheduler is not leaked between tests, or test-cases.
> 
> - Move some APIs off of base::TestSuite, that we unnecessarily public.
> - Rename the various internal TestEventListeners to express purpose.
> - Add CheckForLeakedGlobals check, and DisableCheckForLeakedGlobals API.
>   The latter is called by the content::ContentTestSuiteBase, since the
>   various browser- and ui-tests running under that suite base run more
>   like the browser itself, and so are expected to let global singletons
>   leak.
> 
> Bug: 875486
> Change-Id: If749dfe8530abc4a5c13a840bbf8258d48c377c5
> Reviewed-on: https://chromium-review.googlesource.com/1182612
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Commit-Queue: Wez <wez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#585652}

TBR=sky@chromium.org,wez@chromium.org,gab@chromium.org

Change-Id: I906458f276100d9f924a0a78c7cca3a55bcb28b6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 875486
Reviewed-on: https://chromium-review.googlesource.com/1187982
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585680}

base/test/test_suite.cc

@@ -26,7 +26,6 @@
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/memory.h"
-#include "base/task/task_scheduler/task_scheduler.h"
 #include "base/test/gtest_xml_unittest_result_printer.h"
 #include "base/test/gtest_xml_util.h"
 #include "base/test/icu_test_util.h"
@@ -71,26 +70,21 @@ namespace base {
 
 namespace {
 
-// Returns true if the test is marked as "MAYBE_".
-// When using different prefixes depending on platform, we use MAYBE_ and
-// preprocessor directives to replace MAYBE_ with the target prefix.
-bool IsMarkedMaybe(const testing::TestInfo& test) {
-  return strncmp(test.name(), "MAYBE_", 6) == 0;
-}
-
-class DisableMaybeTests : public testing::EmptyTestEventListener {
+class MaybeTestDisabler : public testing::EmptyTestEventListener {
  public:
   void OnTestStart(const testing::TestInfo& test_info) override {
-    ASSERT_FALSE(IsMarkedMaybe(test_info))
+    ASSERT_FALSE(TestSuite::IsMarkedMaybe(test_info))
         << "Probably the OS #ifdefs don't include all of the necessary "
            "platforms.\nPlease ensure that no tests have the MAYBE_ prefix "
            "after the code is preprocessed.";
   }
 };
 
-class ResetCommandLineBetweenTests : public testing::EmptyTestEventListener {
+class TestClientInitializer : public testing::EmptyTestEventListener {
  public:
-  ResetCommandLineBetweenTests() : old_command_line_(CommandLine::NO_PROGRAM) {}
+  TestClientInitializer()
+      : old_command_line_(CommandLine::NO_PROGRAM) {
+  }
 
   void OnTestStart(const testing::TestInfo& test_info) override {
     old_command_line_ = *CommandLine::ForCurrentProcess();
@@ -103,36 +97,7 @@ class ResetCommandLineBetweenTests : public testing::EmptyTestEventListener {
  private:
   CommandLine old_command_line_;
 
-  DISALLOW_COPY_AND_ASSIGN(ResetCommandLineBetweenTests);
-};
-
-class CheckForLeakedGlobals : public testing::EmptyTestEventListener {
- public:
-  CheckForLeakedGlobals() = default;
-
-  // Check for leaks in individual tests.
-  void OnTestStart(const testing::TestInfo& test) override {
-    scheduler_set_before_test_ = TaskScheduler::GetInstance();
-  }
-  void OnTestEnd(const testing::TestInfo& test) override {
-    DCHECK_EQ(scheduler_set_before_test_, TaskScheduler::GetInstance())
-        << " in test " << test.test_case_name() << "." << test.name();
-  }
-
-  // Check for leaks in test cases (consisting of one or more tests).
-  void OnTestCaseStart(const testing::TestCase& test_case) override {
-    scheduler_set_before_case_ = TaskScheduler::GetInstance();
-  }
-  void OnTestCaseEnd(const testing::TestCase& test_case) override {
-    DCHECK_EQ(scheduler_set_before_case_, TaskScheduler::GetInstance())
-        << " in case " << test_case.name();
-  }
-
- private:
-  TaskScheduler* scheduler_set_before_test_ = nullptr;
-  TaskScheduler* scheduler_set_before_case_ = nullptr;
-
-  DISALLOW_COPY_AND_ASSIGN(CheckForLeakedGlobals);
+  DISALLOW_COPY_AND_ASSIGN(TestClientInitializer);
 };
 
 std::string GetProfileName() {
@@ -175,7 +140,7 @@ int RunUnitTestsUsingBaseTestSuite(int argc, char **argv) {
                          Bind(&TestSuite::Run, Unretained(&test_suite)));
 }
 
-TestSuite::TestSuite(int argc, char** argv) {
+TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
   // Logging must be initialized before any thread has a chance to call logging
@@ -184,7 +149,8 @@ TestSuite::TestSuite(int argc, char** argv) {
 }
 
 #if defined(OS_WIN)
-TestSuite::TestSuite(int argc, wchar_t** argv) {
+TestSuite::TestSuite(int argc, wchar_t** argv)
+    : initialized_command_line_(false) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
   // Logging must be initialized before any thread has a chance to call logging
@@ -218,8 +184,6 @@ void TestSuite::InitializeFromCommandLine(int argc, wchar_t** argv) {
 #endif  // defined(OS_WIN)
 
 void TestSuite::PreInitialize() {
-  DCHECK(!is_initialized_);
-
 #if defined(OS_WIN)
   testing::GTEST_FLAG(catch_exceptions) = false;
 #endif
@@ -241,6 +205,24 @@ void TestSuite::PreInitialize() {
   // Initialize().  See bug 6436.
 }
 
+
+// static
+bool TestSuite::IsMarkedMaybe(const testing::TestInfo& test) {
+  return strncmp(test.name(), "MAYBE_", 6) == 0;
+}
+
+void TestSuite::CatchMaybeTests() {
+  testing::TestEventListeners& listeners =
+      testing::UnitTest::GetInstance()->listeners();
+  listeners.Append(new MaybeTestDisabler);
+}
+
+void TestSuite::ResetCommandLine() {
+  testing::TestEventListeners& listeners =
+      testing::UnitTest::GetInstance()->listeners();
+  listeners.Append(new TestClientInitializer);
+}
+
 void TestSuite::AddTestLauncherResultPrinter() {
   // Only add the custom printer if requested.
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
@@ -306,11 +288,6 @@ int TestSuite::Run() {
   return result;
 }
 
-void TestSuite::DisableCheckForLeakedGlobals() {
-  DCHECK(!is_initialized_);
-  check_for_leaked_globals_ = false;
-}
-
 void TestSuite::UnitTestAssertHandler(const char* file,
                                       int line,
                                       const base::StringPiece summary,
@@ -407,8 +384,6 @@ void TestSuite::SuppressErrorDialogs() {
 }
 
 void TestSuite::Initialize() {
-  DCHECK(!is_initialized_);
-
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
 #if !defined(OS_IOS)
   if (command_line->HasSwitch(switches::kWaitForDebugger)) {
@@ -493,14 +468,8 @@ void TestSuite::Initialize() {
   SetUpFontconfig();
 #endif
 
-  // Add TestEventListeners to enforce certain properties across tests.
-  testing::TestEventListeners& listeners =
-      testing::UnitTest::GetInstance()->listeners();
-  listeners.Append(new DisableMaybeTests);
-  listeners.Append(new ResetCommandLineBetweenTests);
-  if (check_for_leaked_globals_)
-    listeners.Append(new CheckForLeakedGlobals);
-
+  CatchMaybeTests();
+  ResetCommandLine();
   AddTestLauncherResultPrinter();
 
   TestTimeouts::Initialize();
@@ -508,12 +477,9 @@ void TestSuite::Initialize() {
   trace_to_file_.BeginTracingFromCommandLineOptions();
 
   base::debug::StartProfiling(GetProfileName());
-
-  is_initialized_ = true;
 }
 
 void TestSuite::Shutdown() {
-  DCHECK(is_initialized_);
   base::debug::StopProfiling();
 }
 

base/test/test_suite.h

@@ -41,10 +41,18 @@ class TestSuite {
 #endif  // defined(OS_WIN)
   virtual ~TestSuite();
 
-  int Run();
+  // Returns true if the test is marked as "MAYBE_".
+  // When using different prefixes depending on platform, we use MAYBE_ and
+  // preprocessor directives to replace MAYBE_ with the target prefix.
+  static bool IsMarkedMaybe(const testing::TestInfo& test);
 
-  // Disables checks for certain global objects being leaked across tests.
-  void DisableCheckForLeakedGlobals();
+  void CatchMaybeTests();
+
+  void ResetCommandLine();
+
+  void AddTestLauncherResultPrinter();
+
+  int Run();
 
  protected:
   // By default fatal log messages (e.g. from DCHECKs) result in error dialogs
@@ -69,8 +77,6 @@ class TestSuite {
   std::unique_ptr<base::AtExitManager> at_exit_manager_;
 
  private:
-  void AddTestLauncherResultPrinter();
-
   void InitializeFromCommandLine(int argc, char** argv);
 #if defined(OS_WIN)
   void InitializeFromCommandLine(int argc, wchar_t** argv);
@@ -81,7 +87,7 @@ class TestSuite {
 
   test::TraceToFile trace_to_file_;
 
-  bool initialized_command_line_ = false;
+  bool initialized_command_line_;
 
   test::ScopedFeatureList scoped_feature_list_;
 
@@ -89,10 +95,6 @@ class TestSuite {
 
   std::unique_ptr<logging::ScopedLogAssertHandler> assert_handler_;
 
-  bool check_for_leaked_globals_ = true;
-
-  bool is_initialized_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(TestSuite);
 };
 

chrome/test/base/chrome_test_launcher.cc

@@ -62,10 +62,7 @@ ChromeTestSuiteRunner::ChromeTestSuiteRunner() {}
 ChromeTestSuiteRunner::~ChromeTestSuiteRunner() {}
 
 int ChromeTestSuiteRunner::RunTestSuite(int argc, char** argv) {
-  ChromeTestSuite test_suite(argc, argv);
-  // Browser tests are expected not to tear-down various globals.
-  test_suite.DisableCheckForLeakedGlobals();
-  return test_suite.Run();
+  return ChromeTestSuite(argc, argv).Run();
 }
 
 ChromeTestLauncherDelegate::ChromeTestLauncherDelegate(

chrome/test/base/interactive_ui_tests_main.cc

@@ -37,9 +37,6 @@ class InteractiveUITestSuite : public ChromeTestSuite {
  protected:
   // ChromeTestSuite overrides:
   void Initialize() override {
-    // Browser tests are expected not to tear-down various globals.
-    base::TestSuite::DisableCheckForLeakedGlobals();
-
     ChromeTestSuite::Initialize();
 
     // Only allow ui_controls to be used in interactive_ui_tests, since they

chromecast/app/cast_test_launcher.cc

@@ -21,10 +21,7 @@ class CastTestLauncherDelegate : public content::TestLauncherDelegate {
   ~CastTestLauncherDelegate() override {}
 
   int RunTestSuite(int argc, char** argv) override {
-    base::TestSuite test_suite(argc, argv);
-    // Browser tests are expected not to tear-down various globals.
-    test_suite.DisableCheckForLeakedGlobals();
-    return test_suite.Run();
+    return base::TestSuite(argc, argv).Run();
   }
 
   bool AdjustChildProcessCommandLine(

content/test/content_test_launcher.cc

@@ -98,9 +98,6 @@ class ContentBrowserTestSuite : public ContentTestSuiteBase {
         content::CreateContentBrowserTestsCatalog());
 #endif
 
-    // Browser tests are expected not to tear-down various globals.
-    base::TestSuite::DisableCheckForLeakedGlobals();
-
     ContentTestSuiteBase::Initialize();
   }
 

extensions/shell/test/shell_test_launcher_delegate.cc

@@ -12,10 +12,7 @@
 namespace extensions {
 
 int AppShellTestLauncherDelegate::RunTestSuite(int argc, char** argv) {
-  base::TestSuite test_suite(argc, argv);
-  // Browser tests are expected not to tear-down various globals.
-  test_suite.DisableCheckForLeakedGlobals();
-  return test_suite.Run();
+  return base::TestSuite(argc, argv).Run();
 }
 
 bool AppShellTestLauncherDelegate::AdjustChildProcessCommandLine(

headless/test/headless_test_launcher.cc

@@ -37,10 +37,7 @@ class HeadlessTestLauncherDelegate : public content::TestLauncherDelegate {
 
   // content::TestLauncherDelegate implementation:
   int RunTestSuite(int argc, char** argv) override {
-    base::TestSuite test_suite(argc, argv);
-    // Browser tests are expected not to tear-down various globals.
-    test_suite.DisableCheckForLeakedGlobals();
-    return test_suite.Run();
+    return base::TestSuite(argc, argv).Run();
   }
 
   bool AdjustChildProcessCommandLine(

webrunner/browser/webrunner_test_launcher.cc

@@ -25,10 +25,7 @@ class WebRunnerTestLauncherDelegate : public content::TestLauncherDelegate {
 
   // content::TestLauncherDelegate implementation:
   int RunTestSuite(int argc, char** argv) override {
-    base::TestSuite test_suite(argc, argv);
-    // Browser tests are expected not to tear-down various globals.
-    test_suite.DisableCheckForLeakedGlobals();
-    return test_suite.Run();
+    return base::TestSuite(argc, argv).Run();
   }
 
   bool AdjustChildProcessCommandLine(