From 4ddc16ab27a927f0345014a0ba5202683d4912f9 Mon Sep 17 00:00:00 2001 From: Francois Doray Date: Tue, 29 Oct 2019 14:04:03 +0000 Subject: [PATCH] [base] Ensure that tests don't change the thread priority (reland #2). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of https://crrev.com/c/chromium/src/+/1865405 The difference is that we don't check thread priority in angle_unittest, as these tests change the 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: I8fb2230cbb9b9203884d1c440696d4ce2968dcaf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881707 Reviewed-by: Michael Giuffrida Reviewed-by: Zhenyao Mo Reviewed-by: Scott Violet Commit-Queue: François Doray Cr-Commit-Position: refs/heads/master@{#710326} --- .../test/ash_content_perf_test_launcher.cc | 5 ++- base/test/test_suite.cc | 40 +++++++++++++++++-- base/test/test_suite.h | 14 +++++-- .../test/base/browser_tests_main_chromeos.cc | 4 +- chrome/test/base/chrome_test_launcher.cc | 4 +- chrome/test/base/interactive_ui_tests_main.cc | 4 +- chromecast/app/cast_test_launcher.cc | 4 +- content/test/content_test_launcher.cc | 5 ++- .../test/shell_test_launcher_delegate.cc | 4 +- .../engine/test/web_engine_test_launcher.cc | 4 +- gpu/angle_deqp_tests_main.cc | 5 ++- gpu/angle_perftests_main.cc | 4 ++ gpu/angle_unittest_main.cc | 3 ++ headless/test/headless_test_launcher.cc | 4 +- weblayer/test/test_launcher_delegate_impl.cc | 1 + 15 files changed, 86 insertions(+), 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 index 947b77ff40ca95..fb682061861cf0 100644 --- 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 index 3b1de80c8346c1..50820a951126a5 100644 --- 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 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 index 6e1e750c6912b3..4d4d638d346a0f 100644 --- 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 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 index 55106b2933b019..5426703044591f 100644 --- 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 index 5e3c3d326bb02a..346c4b5aac953f 100644 --- 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 index 051d4b3ba6226d..abc8ab20496df5 100644 --- 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 index 186c3147f24caf..ca0544cec4119c 100644 --- 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 index 37aec6f601be4b..7fd5bcb2d1b8b8 100644 --- 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 index 2f680e4a083902..d0a9430eb4d1df 100644 --- 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 index 91095885b210f5..faaaf13d25d142 100644 --- a/fuchsia/engine/test/web_engine_test_launcher.cc +++ b/fuchsia/engine/test/web_engine_test_launcher.cc @@ -23,8 +23,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 index 7567b6a7eb2630..2c1351a6821f3e 100644 --- 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 index 341410b03858f6..09a97d4e8da136 100644 --- 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/gpu/angle_unittest_main.cc b/gpu/angle_unittest_main.cc index f41877188f55df..1c2199058ba6bd 100644 --- a/gpu/angle_unittest_main.cc +++ b/gpu/angle_unittest_main.cc @@ -24,7 +24,10 @@ int main(int argc, char** argv) { base::CommandLine::Init(argc, argv); testing::InitGoogleMock(&argc, argv); sh::Initialize(); + base::TestSuite test_suite(argc, argv); + test_suite.DisableCheckForThreadAndProcessPriority(); + int rt = base::LaunchUnitTestsSerially( argc, argv, base::BindOnce(&RunHelper, base::Unretained(&test_suite))); sh::Finalize(); diff --git a/headless/test/headless_test_launcher.cc b/headless/test/headless_test_launcher.cc index 6d8ee1c107d224..d0b1430d23e11e 100644 --- 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 index a1a52189e5bf0d..6f267fd4ee2742 100644 --- 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(); }