[base] Ensure that tests don't change the thread priority (reland).

This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1804786. The difference is that we don't check thread priority in angle_deqp_tests_main, as these tests do change thread priority on purpose. This CL verifies that the thread priority is normal when a test process is launched and before/after each test. The goal is to avoid having tests that assume they run at normal priority be disabled because of other misbehaving tests (e.g. https://crbug.com/931706). TBR=sky@chromium.org,michaelpg@chromium.org,thakis@chromium.org,eseckler@chromium.org Bug: 931706 Change-Id: I92a827d014278b1f5b3504f48c2441ad2e83bc65 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865405 Reviewed-by: Eric Seckler <eseckler@chromium.org> Reviewed-by: Zhenyao Mo <zmo@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#708194}
cptjacky · Oct 22, 2019 · 2b7ca8e · 2b7ca8e
1 parent dc92968
commit 2b7ca8e
Show file tree

Hide file tree

Showing 14 changed files with 83 additions and 19 deletions.
diff --git a/ash/shell/content/test/ash_content_perf_test_launcher.cc b/ash/shell/content/test/ash_content_perf_test_launcher.cc
@@ -34,9 +34,10 @@ class AshContentTestSuite : public content::ContentTestSuiteBase {
  protected:
   // content::ContentTestSuiteBase:
   void Initialize() override {
-    // Browser tests are expected not to tear-down various globals. (Must run
-    // before the base class is initialized.)
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     base::TestSuite::DisableCheckForLeakedGlobals();
+    base::TestSuite::DisableCheckForThreadPriorityAtTestEnd();
     ContentTestSuiteBase::Initialize();
     ui_controls::InstallUIControlsAura(ash::test::CreateAshUIControls());
   }

diff --git a/base/test/test_suite.cc b/base/test/test_suite.cc
@@ -37,6 +37,7 @@
 #include "base/test/multiprocess_test.h"
 #include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -188,6 +189,31 @@ class CheckProcessPriority : public testing::EmptyTestEventListener {
 };
 #endif  // !defined(OS_IOS)
 
+class CheckThreadPriority : public testing::EmptyTestEventListener {
+ public:
+  CheckThreadPriority(bool check_thread_priority_at_test_end)
+      : check_thread_priority_at_test_end_(check_thread_priority_at_test_end) {
+    CHECK_EQ(base::PlatformThread::GetCurrentThreadPriority(),
+             base::ThreadPriority::NORMAL);
+  }
+
+  void OnTestStart(const testing::TestInfo& test) override {
+    EXPECT_EQ(base::PlatformThread::GetCurrentThreadPriority(),
+              base::ThreadPriority::NORMAL);
+  }
+  void OnTestEnd(const testing::TestInfo& test) override {
+    if (check_thread_priority_at_test_end_) {
+      EXPECT_EQ(base::PlatformThread::GetCurrentThreadPriority(),
+                base::ThreadPriority::NORMAL);
+    }
+  }
+
+ private:
+  const bool check_thread_priority_at_test_end_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckThreadPriority);
+};
+
 const std::string& GetProfileName() {
   static const NoDestructor<std::string> profile_name([]() {
     const CommandLine& command_line = *CommandLine::ForCurrentProcess();
@@ -376,9 +402,14 @@ void TestSuite::DisableCheckForLeakedGlobals() {
   check_for_leaked_globals_ = false;
 }
 
-void TestSuite::DisableCheckForProcessPriority() {
+void TestSuite::DisableCheckForThreadAndProcessPriority() {
   DCHECK(!is_initialized_);
-  check_for_process_priority_ = false;
+  check_for_thread_and_process_priority_ = false;
+}
+
+void TestSuite::DisableCheckForThreadPriorityAtTestEnd() {
+  DCHECK(!is_initialized_);
+  check_for_thread_priority_at_test_end_ = false;
 }
 
 void TestSuite::UnitTestAssertHandler(const char* file,
@@ -567,10 +598,13 @@ void TestSuite::Initialize() {
   listeners.Append(new ResetCommandLineBetweenTests);
   if (check_for_leaked_globals_)
     listeners.Append(new CheckForLeakedGlobals);
+  if (check_for_thread_and_process_priority_) {
+    listeners.Append(
+        new CheckThreadPriority(check_for_thread_priority_at_test_end_));
 #if !defined(OS_IOS)
-  if (check_for_process_priority_)
     listeners.Append(new CheckProcessPriority);
 #endif
+  }
 
   AddTestLauncherResultPrinter();
 

diff --git a/base/test/test_suite.h b/base/test/test_suite.h
@@ -43,8 +43,15 @@ class TestSuite {
 
   int Run();
 
-  // Disables checks for process priority. Most tests should not use this.
-  void DisableCheckForProcessPriority();
+  // Disables checks for thread and process priority at the beginning and end of
+  // each test. Most tests should not use this.
+  void DisableCheckForThreadAndProcessPriority();
+
+  // Disables checks for thread priority at the end of each test (still checks
+  // at the beginning of each test). This should be used for tests that run in
+  // their own process and should start with normal priorities but are allowed
+  // to end with different priorities.
+  void DisableCheckForThreadPriorityAtTestEnd();
 
   // Disables checks for certain global objects being leaked across tests.
   void DisableCheckForLeakedGlobals();
@@ -93,7 +100,8 @@ class TestSuite {
   std::unique_ptr<logging::ScopedLogAssertHandler> assert_handler_;
 
   bool check_for_leaked_globals_ = true;
-  bool check_for_process_priority_ = true;
+  bool check_for_thread_and_process_priority_ = true;
+  bool check_for_thread_priority_at_test_end_ = true;
 
   bool is_initialized_ = false;
 

diff --git a/chrome/test/base/browser_tests_main_chromeos.cc b/chrome/test/base/browser_tests_main_chromeos.cc
@@ -30,8 +30,10 @@ class BrowserTestSuiteRunnerChromeOS : public ChromeTestSuiteRunner {
  public:
   int RunTestSuite(int argc, char** argv) override {
     BrowserTestSuiteChromeOS test_suite(argc, argv);
-    // Browser tests are expected not to tear-down various globals.
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     test_suite.DisableCheckForLeakedGlobals();
+    test_suite.DisableCheckForThreadPriorityAtTestEnd();
     return test_suite.Run();
   }
 };

diff --git a/chrome/test/base/chrome_test_launcher.cc b/chrome/test/base/chrome_test_launcher.cc
@@ -77,8 +77,10 @@ ChromeTestSuiteRunner::~ChromeTestSuiteRunner() {}
 
 int ChromeTestSuiteRunner::RunTestSuite(int argc, char** argv) {
   ChromeTestSuite test_suite(argc, argv);
-  // Browser tests are expected not to tear-down various globals.
+  // Browser tests are expected not to tear-down various globals and may
+  // complete with the thread priority being above NORMAL.
   test_suite.DisableCheckForLeakedGlobals();
+  test_suite.DisableCheckForThreadPriorityAtTestEnd();
 #if defined(OS_ANDROID)
   // Android browser tests run child processes as threads instead.
   content::ContentTestSuiteBase::RegisterInProcessThreads();

diff --git a/chrome/test/base/interactive_ui_tests_main.cc b/chrome/test/base/interactive_ui_tests_main.cc
@@ -41,8 +41,10 @@ class InteractiveUITestSuite : public ChromeTestSuite {
  protected:
   // ChromeTestSuite overrides:
   void Initialize() override {
-    // Browser tests are expected not to tear-down various globals.
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     base::TestSuite::DisableCheckForLeakedGlobals();
+    base::TestSuite::DisableCheckForThreadPriorityAtTestEnd();
 
     ChromeTestSuite::Initialize();
 

diff --git a/chromecast/app/cast_test_launcher.cc b/chromecast/app/cast_test_launcher.cc
@@ -24,8 +24,10 @@ class CastTestLauncherDelegate : public content::TestLauncherDelegate {
 
   int RunTestSuite(int argc, char** argv) override {
     base::TestSuite test_suite(argc, argv);
-    // Browser tests are expected not to tear-down various globals.
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     test_suite.DisableCheckForLeakedGlobals();
+    test_suite.DisableCheckForThreadPriorityAtTestEnd();
     return test_suite.Run();
   }
 

diff --git a/content/test/content_test_launcher.cc b/content/test/content_test_launcher.cc
@@ -39,9 +39,10 @@ class ContentBrowserTestSuite : public ContentTestSuiteBase {
 
  protected:
   void Initialize() override {
-    // Browser tests are expected not to tear-down various globals. (Must run
-    // before the base class is initialized.)
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     base::TestSuite::DisableCheckForLeakedGlobals();
+    base::TestSuite::DisableCheckForThreadPriorityAtTestEnd();
 
     ContentTestSuiteBase::Initialize();
 

diff --git a/extensions/shell/test/shell_test_launcher_delegate.cc b/extensions/shell/test/shell_test_launcher_delegate.cc
@@ -13,8 +13,10 @@ 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.
+  // Browser tests are expected not to tear-down various globals and may
+  // complete with the thread priority being above NORMAL.
   test_suite.DisableCheckForLeakedGlobals();
+  test_suite.DisableCheckForThreadPriorityAtTestEnd();
   return test_suite.Run();
 }
 

diff --git a/fuchsia/engine/test/web_engine_test_launcher.cc b/fuchsia/engine/test/web_engine_test_launcher.cc
@@ -24,8 +24,10 @@ class WebEngineTestLauncherDelegate : 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.
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     test_suite.DisableCheckForLeakedGlobals();
+    test_suite.DisableCheckForThreadPriorityAtTestEnd();
     return test_suite.Run();
   }
 

diff --git a/gpu/angle_deqp_tests_main.cc b/gpu/angle_deqp_tests_main.cc
@@ -35,8 +35,9 @@ int main(int argc, char** argv) {
   angle::InitTestHarness(&argc, argv);
   base::TestSuite test_suite(argc, argv);
 
-  // The process priority is lowered by the constructor of tcu::ANGLEPlatform().
-  test_suite.DisableCheckForProcessPriority();
+  // The process and thread priorities are modified by
+  // StabilizeCPUForBenchmarking()/SetLowPriorityProcess().
+  test_suite.DisableCheckForThreadAndProcessPriority();
 
   int rt = base::LaunchUnitTestsSerially(
       argc, argv, base::BindOnce(&RunHelper, base::Unretained(&test_suite)));

diff --git a/gpu/angle_perftests_main.cc b/gpu/angle_perftests_main.cc
@@ -27,6 +27,10 @@ int main(int argc, char** argv) {
   ANGLEProcessPerfTestArgs(&argc, argv);
 
   base::TestSuite test_suite(argc, argv);
+
+  // The thread priority is modified by StabilizeCPUForBenchmarking().
+  test_suite.DisableCheckForThreadAndProcessPriority();
+
   int rt = base::LaunchUnitTestsSerially(
       argc, argv, base::BindOnce(&RunHelper, base::Unretained(&test_suite)));
   return rt;

diff --git a/headless/test/headless_test_launcher.cc b/headless/test/headless_test_launcher.cc
@@ -41,8 +41,10 @@ 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.
+    // Browser tests are expected not to tear-down various globals and may
+    // complete with the thread priority being above NORMAL.
     test_suite.DisableCheckForLeakedGlobals();
+    test_suite.DisableCheckForThreadPriorityAtTestEnd();
     return test_suite.Run();
   }
 

diff --git a/weblayer/test/test_launcher_delegate_impl.cc b/weblayer/test/test_launcher_delegate_impl.cc
@@ -17,6 +17,7 @@ int TestLauncherDelegateImpl::RunTestSuite(int argc, char** argv) {
   // Browser tests are expected not to tear-down various globals and may
   // complete with the thread priority being above NORMAL.
   test_suite.DisableCheckForLeakedGlobals();
+  test_suite.DisableCheckForThreadPriorityAtTestEnd();
   return test_suite.Run();
 }