From fe4e8ad0b185ad3ee85158243221c0d894f72b5a Mon Sep 17 00:00:00 2001 From: Joshua Pawlicki Date: Tue, 14 Dec 2021 15:21:52 +0000 Subject: [PATCH] Updater: eliminate sleeps in integration tests. Fixed: 1217765 Change-Id: Ifa06cdb31b26f7c6332d95f877957c39ffb000b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3330675 Reviewed-by: Sorin Jianu Commit-Queue: Joshua Pawlicki Cr-Commit-Position: refs/heads/main@{#951501} --- .../updater/test/integration_test_commands.h | 2 +- .../test/integration_test_commands_system.cc | 4 +- .../test/integration_test_commands_user.cc | 4 +- chrome/updater/test/integration_tests.cc | 44 ++++++++----------- chrome/updater/test/integration_tests_impl.cc | 12 ----- chrome/updater/test/integration_tests_impl.h | 7 +-- .../updater/test/integration_tests_linux.cc | 2 +- chrome/updater/test/integration_tests_mac.mm | 2 +- chrome/updater/test/integration_tests_win.cc | 10 ++--- 9 files changed, 30 insertions(+), 57 deletions(-) diff --git a/chrome/updater/test/integration_test_commands.h b/chrome/updater/test/integration_test_commands.h index 29090b63ab8620..8a9464dab05f6d 100644 --- a/chrome/updater/test/integration_test_commands.h +++ b/chrome/updater/test/integration_test_commands.h @@ -60,7 +60,7 @@ class IntegrationTestCommands virtual void UpdateAll() const = 0; virtual void PrintLog() const = 0; virtual base::FilePath GetDifferentUserPath() const = 0; - virtual void WaitForServerExit() const = 0; + virtual void WaitForUpdaterExit() const = 0; #if defined(OS_WIN) virtual void ExpectInterfacesRegistered() const = 0; virtual void ExpectLegacyUpdate3WebSucceeds( diff --git a/chrome/updater/test/integration_test_commands_system.cc b/chrome/updater/test/integration_test_commands_system.cc index 16ebb5fe6b7eef..b016a44ed60d19 100644 --- a/chrome/updater/test/integration_test_commands_system.cc +++ b/chrome/updater/test/integration_test_commands_system.cc @@ -146,8 +146,8 @@ class IntegrationTestCommandsSystem : public IntegrationTestCommands { RunCommand("register_app", {Param("app_id", app_id)}); } - void WaitForServerExit() const override { - updater::test::WaitForServerExit(updater_scope_); + void WaitForUpdaterExit() const override { + updater::test::WaitForUpdaterExit(updater_scope_); } #if defined(OS_WIN) diff --git a/chrome/updater/test/integration_test_commands_user.cc b/chrome/updater/test/integration_test_commands_user.cc index cc1997a6888b6d..c70a01d53c992a 100644 --- a/chrome/updater/test/integration_test_commands_user.cc +++ b/chrome/updater/test/integration_test_commands_user.cc @@ -135,8 +135,8 @@ class IntegrationTestCommandsUser : public IntegrationTestCommands { updater::test::RegisterApp(updater_scope_, app_id); } - void WaitForServerExit() const override { - updater::test::WaitForServerExit(updater_scope_); + void WaitForUpdaterExit() const override { + updater::test::WaitForUpdaterExit(updater_scope_); } #if defined(OS_WIN) diff --git a/chrome/updater/test/integration_tests.cc b/chrome/updater/test/integration_tests.cc index f9fbc75b9373fb..be20f5ca4d1570 100644 --- a/chrome/updater/test/integration_tests.cc +++ b/chrome/updater/test/integration_tests.cc @@ -120,6 +120,7 @@ class IntegrationTest : public ::testing::Test { PrintLog(); CopyLog(); test_commands_->Uninstall(); + WaitForUpdaterExit(); } void ExpectCandidateUninstalled() { @@ -211,7 +212,7 @@ class IntegrationTest : public ::testing::Test { return test_commands_->GetDifferentUserPath(); } - void WaitForServerExit() { test_commands_->WaitForServerExit(); } + void WaitForUpdaterExit() { test_commands_->WaitForUpdaterExit(); } void SetUpTestService() { #if defined(OS_WIN) @@ -265,7 +266,7 @@ class IntegrationTest : public ::testing::Test { TEST_F(IntegrationTest, InstallUninstall) { Install(); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectInstalled(); ExpectVersionActive(kUpdaterVersion); ExpectActiveUpdater(); @@ -281,16 +282,12 @@ TEST_F(IntegrationTest, InstallUninstall) { TEST_F(IntegrationTest, SelfUninstallOutdatedUpdater) { Install(); ExpectInstalled(); - SleepFor(2); + WaitForUpdaterExit(); SetupFakeUpdaterHigherVersion(); ExpectVersionNotActive(kUpdaterVersion); RunWake(0); - - // The mac server will remain active for 10 seconds after it replies to the - // wake client, then shut down and uninstall itself. Sleep to wait for this - // to happen. - SleepFor(11); + WaitForUpdaterExit(); ExpectCandidateUninstalled(); // The candidate uninstall should not have altered global prefs. @@ -306,7 +303,7 @@ TEST_F(IntegrationTest, QualifyUpdater) { ExpectRegistrationEvent(&test_server, kUpdaterAppId); Install(); ExpectInstalled(); - WaitForServerExit(); + WaitForUpdaterExit(); SetupFakeUpdaterLowerVersion(); ExpectVersionNotActive(kUpdaterVersion); @@ -315,7 +312,7 @@ TEST_F(IntegrationTest, QualifyUpdater) { base::Version("0.2")); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); // This instance is now qualified and should activate itself and check itself // for updates on the next check. @@ -324,7 +321,7 @@ TEST_F(IntegrationTest, QualifyUpdater) { base::StringPrintf(".*%s.*", kUpdaterAppId))}, ")]}'\n"); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectVersionActive(kUpdaterVersion); Uninstall(); @@ -341,7 +338,7 @@ TEST_F(IntegrationTest, SelfUpdate) { base::Version(kUpdaterVersion), next_version); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectAppVersion(kUpdaterAppId, next_version); Uninstall(); @@ -404,7 +401,7 @@ TEST_F(IntegrationTest, UpdateApp) { base::Version v2("2"); ExpectUpdateSequence(&test_server, kAppId, v1, v2); Update(kAppId); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectAppVersion(kAppId, v2); Uninstall(); @@ -502,8 +499,7 @@ TEST_F(IntegrationTest, UninstallCmdLine) { ExpectActiveUpdater(); RunUninstallCmdLine(); - WaitForServerExit(); - SleepFor(2); + WaitForUpdaterExit(); ExpectClean(); } #endif // defined(OS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING) @@ -514,14 +510,14 @@ TEST_F(IntegrationTest, UnregisterUninstalledApp) { RegisterApp("test1"); RegisterApp("test2"); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectVersionActive(kUpdaterVersion); ExpectActiveUpdater(); SetExistenceCheckerPath("test1", base::FilePath(FILE_PATH_LITERAL("NONE"))); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectInstalled(); ExpectAppUnregisteredExistenceCheckerPath("test1"); @@ -530,12 +526,11 @@ TEST_F(IntegrationTest, UnregisterUninstalledApp) { TEST_F(IntegrationTest, UninstallIfMaxServerWakesBeforeRegistrationExceeded) { Install(); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectInstalled(); SetServerStarts(24); RunWake(0); - WaitForServerExit(); - SleepFor(2); + WaitForUpdaterExit(); ExpectClean(); } @@ -543,17 +538,16 @@ TEST_F(IntegrationTest, UninstallUpdaterWhenAllAppsUninstalled) { Install(); RegisterApp("test1"); ExpectInstalled(); - WaitForServerExit(); + WaitForUpdaterExit(); SetServerStarts(24); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectInstalled(); ExpectVersionActive(kUpdaterVersion); ExpectActiveUpdater(); SetExistenceCheckerPath("test1", base::FilePath(FILE_PATH_LITERAL("NONE"))); RunWake(0); - WaitForServerExit(); - SleepFor(2); + WaitForUpdaterExit(); ExpectClean(); } @@ -572,7 +566,7 @@ TEST_F(IntegrationTest, UnregisterUnownedApp) { SetExistenceCheckerPath("test1", GetDifferentUserPath()); RunWake(0); - WaitForServerExit(); + WaitForUpdaterExit(); ExpectAppUnregisteredExistenceCheckerPath("test1"); diff --git a/chrome/updater/test/integration_tests_impl.cc b/chrome/updater/test/integration_tests_impl.cc index 9ca520761f83a8..06e90b0abee0c1 100644 --- a/chrome/updater/test/integration_tests_impl.cc +++ b/chrome/updater/test/integration_tests_impl.cc @@ -313,18 +313,6 @@ bool Run(UpdaterScope scope, base::CommandLine command_line, int* exit_code) { return process.WaitForExitWithTimeout(base::Seconds(45), exit_code); } -void SleepFor(int seconds) { - VLOG(2) << "Sleeping " << seconds << " seconds..."; - base::WaitableEvent sleep(base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED); - base::ThreadPool::PostDelayedTask( - FROM_HERE, {base::MayBlock()}, - base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(&sleep)), - base::Seconds(seconds)); - sleep.Wait(); - VLOG(2) << "Sleep complete."; -} - bool WaitFor(base::RepeatingCallback predicate) { base::TimeTicks deadline = base::TimeTicks::Now() + TestTimeouts::action_max_timeout(); diff --git a/chrome/updater/test/integration_tests_impl.h b/chrome/updater/test/integration_tests_impl.h index dc8c38b043309a..7e5937895a53ac 100644 --- a/chrome/updater/test/integration_tests_impl.h +++ b/chrome/updater/test/integration_tests_impl.h @@ -51,11 +51,6 @@ void EnterTestMode(const GURL& url); // Copies the logs to a location where they can be retrieved by ResultDB. void CopyLog(const base::FilePath& src_dir); -// Sleeps for the given number of seconds. This should be avoided, but in some -// cases surrounding uninstall it is necessary since the processes can exit -// prior to completing the actual uninstallation. -void SleepFor(int seconds); - // Waits for a given predicate to become true, testing it by polling. Returns // true if the predicate becomes true before a timeout, otherwise returns false. bool WaitFor(base::RepeatingCallback predicate); @@ -143,7 +138,7 @@ void ExpectAppVersion(UpdaterScope scope, void RegisterApp(UpdaterScope scope, const std::string& app_id); -void WaitForServerExit(UpdaterScope scope); +void WaitForUpdaterExit(UpdaterScope scope); #if defined(OS_WIN) void ExpectInterfacesRegistered(UpdaterScope scope); diff --git a/chrome/updater/test/integration_tests_linux.cc b/chrome/updater/test/integration_tests_linux.cc index d4fb5b6795dc86..6850c64921d38f 100644 --- a/chrome/updater/test/integration_tests_linux.cc +++ b/chrome/updater/test/integration_tests_linux.cc @@ -27,7 +27,7 @@ absl::optional GetInstalledExecutablePath(UpdaterScope scope) { return absl::nullopt; } -void WaitForServerExit(UpdaterScope scope) { +void WaitForUpdaterExit(UpdaterScope scope) { NOTREACHED(); } diff --git a/chrome/updater/test/integration_tests_mac.mm b/chrome/updater/test/integration_tests_mac.mm index 89aa572bfc6612..4ff231403eab88 100644 --- a/chrome/updater/test/integration_tests_mac.mm +++ b/chrome/updater/test/integration_tests_mac.mm @@ -300,7 +300,7 @@ void ExpectNotActive(UpdaterScope scope, const std::string& app_id) { EXPECT_FALSE(base::PathIsWritable(*path)); } -void WaitForServerExit(UpdaterScope /*scope*/) { +void WaitForUpdaterExit(UpdaterScope /*scope*/) { ASSERT_TRUE(WaitFor(base::BindRepeating([]() { std::string ps_stdout; EXPECT_TRUE(base::GetAppOutput({"ps", "ax", "-o", "command"}, &ps_stdout)); diff --git a/chrome/updater/test/integration_tests_win.cc b/chrome/updater/test/integration_tests_win.cc index e7bbeacb6319ae..1a29f59ffefe53 100644 --- a/chrome/updater/test/integration_tests_win.cc +++ b/chrome/updater/test/integration_tests_win.cc @@ -290,8 +290,8 @@ void CheckInstallation(UpdaterScope scope, // Returns true is any updater process is found running in any session in the // system, regardless of its path. bool IsUpdaterRunning() { - ProcessFilterName filter(kUpdaterProcessName); - return base::ProcessIterator(&filter).NextProcessEntry(); + return IsProcessRunning(kUpdaterProcessName) || + IsProcessRunning(base::UTF8ToWide(kUninstallScript).c_str()); } } // namespace @@ -411,10 +411,6 @@ void Uninstall(UpdaterScope scope) { int exit_code = -1; ASSERT_TRUE(Run(scope, command_line, &exit_code)); EXPECT_EQ(0, exit_code); - - // Uninstallation involves a race with the uninstall.cmd script and the - // process exit. Sleep to allow the script to complete its work. - SleepFor(5); } void SetActive(UpdaterScope /*scope*/, const std::string& id) { @@ -450,7 +446,7 @@ void ExpectNotActive(UpdaterScope /*scope*/, const std::string& id) { // Waits for all updater processes to end, including the server process holding // the prefs lock. -void WaitForServerExit(UpdaterScope /*scope*/) { +void WaitForUpdaterExit(UpdaterScope /*scope*/) { WaitFor(base::BindRepeating([]() { return !IsUpdaterRunning(); })); }