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 <sorin@chromium.org> Commit-Queue: Joshua Pawlicki <waffles@chromium.org> Cr-Commit-Position: refs/heads/main@{#951501}
FairyWorld · Dec 14, 2021 · fe4e8ad · fe4e8ad
1 parent 1252f7e
commit fe4e8ad
Show file tree

Hide file tree

Showing 9 changed files with 30 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,30 +526,28 @@ TEST_F(IntegrationTest, UnregisterUninstalledApp) {
 
 TEST_F(IntegrationTest, UninstallIfMaxServerWakesBeforeRegistrationExceeded) {
   Install();
-  WaitForServerExit();
+  WaitForUpdaterExit();
   ExpectInstalled();
   SetServerStarts(24);
   RunWake(0);
-  WaitForServerExit();
-  SleepFor(2);
+  WaitForUpdaterExit();
   ExpectClean();
 }
 
 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
@@ -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<bool()> 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
@@ -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<bool()> 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
@@ -27,7 +27,7 @@ absl::optional<base::FilePath> 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
@@ -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
@@ -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(); }));
 }