Fix sync e2e tests flakiness

Currently e2e tests randomly fails with:
1 No real error, just failed to wait for data to sync to clients.
2 PostClientToServerMessage with SERVER_RETURN_NOT_MY_BIRTHDAY
3 PostClientToServerMessage with NETWORK_CONNECTION_UNAVAILABLE
4 Failed to log in to GCM, resetting connection. With 401 error code.

Most of time it is just because we didn't wait for enough time.
30 seconds is not enough. And I did see once, after reseting account,
I logged in in a new client and saw NOT_MY_BIRTHDAY in chrome://sync-internal
So adding a wait there to prevent flakiness.

This cl includes two fixes:
1 Wait for NOT_MY_BIRTHDAY to propagate.
2 Extend the invalidation wait time by flag. The actual timeout will
  be added later.

Test: Run test suite 10 times with no retry and all passing.
Bug: 1032731, 1034716
Change-Id: Ia4a5d6baa048fe16c311999ddfcdfcaac9b24e35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1977183
Commit-Queue: Sven Zheng <svenzheng@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728029}

chrome/browser/sync/test/integration/status_change_checker.cc

@@ -7,20 +7,38 @@
 #include <sstream>
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/logging.h"
-#include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
-constexpr base::TimeDelta kTimeout = base::TimeDelta::FromSeconds(30);
+constexpr base::TimeDelta kDefaultTimeout = base::TimeDelta::FromSeconds(30);
+
+base::TimeDelta GetTimeoutFromCommandLineOrDefault() {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          "sync-status-change-checker-timeout")) {
+    return kDefaultTimeout;
+  }
+  std::string timeout_string(
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          "sync-status-change-checker-timeout"));
+  int timeout_in_seconds = 0;
+  if (!base::StringToInt(timeout_string, &timeout_in_seconds)) {
+    LOG(FATAL) << "Timeout value \"" << timeout_string << "\" was parsed as "
+               << timeout_in_seconds;
+  }
+  return base::TimeDelta::FromSeconds(timeout_in_seconds);
+}
 
 }  // namespace
 
 StatusChangeChecker::StatusChangeChecker()
-    : run_loop_(base::RunLoop::Type::kNestableTasksAllowed),
+    : timeout_(GetTimeoutFromCommandLineOrDefault()),
+      run_loop_(base::RunLoop::Type::kNestableTasksAllowed),
       timed_out_(false) {}
 
 StatusChangeChecker::~StatusChangeChecker() {}
@@ -63,7 +81,7 @@ void StatusChangeChecker::StartBlockingWait() {
   DCHECK(!run_loop_.running());
 
   base::OneShotTimer timer;
-  timer.Start(FROM_HERE, kTimeout,
+  timer.Start(FROM_HERE, timeout_,
               base::BindRepeating(&StatusChangeChecker::OnTimeout,
                                   base::Unretained(this)));
 

chrome/browser/sync/test/integration/status_change_checker.h

@@ -57,6 +57,7 @@ class StatusChangeChecker {
   // Called when the blocking wait timeout is exceeded.
   void OnTimeout();
 
+  const base::TimeDelta timeout_;
   base::RunLoop run_loop_;
   bool timed_out_;
 };

chrome/browser/sync/test/integration/sync_disabled_checker.cc

+#include "chrome/browser/sync/test/integration/sync_disabled_checker.h"
+
+SyncDisabledChecker::SyncDisabledChecker(syncer::ProfileSyncService* service)
+    : SingleClientStatusChangeChecker(service) {}
+
+SyncDisabledChecker::~SyncDisabledChecker() = default;
+
+bool SyncDisabledChecker::IsExitConditionSatisfied(std::ostream* os) {
+  *os << "Waiting until sync is disabled."
+      << " IsSetupInProgress:" << service()->IsSetupInProgress()
+      << " IsFirstSetupComplete:"
+      << service()->GetUserSettings()->IsFirstSetupComplete();
+  return !service()->IsSetupInProgress() &&
+         !service()->GetUserSettings()->IsFirstSetupComplete();
+}

chrome/browser/sync/test/integration/sync_disabled_checker.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_DISABLED_CHECKER_H_
+#define CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_DISABLED_CHECKER_H_
+
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+
+class SyncDisabledChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit SyncDisabledChecker(syncer::ProfileSyncService* service);
+
+  ~SyncDisabledChecker() override;
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+
+ private:
+  base::OnceClosure condition_satisfied_callback_;
+};
+
+#endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_DISABLED_CHECKER_H_

chrome/browser/sync/test/integration/sync_errors_test.cc

@@ -3,13 +3,12 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
-#include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
-#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/sync_disabled_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/common/pref_names.h"
@@ -27,31 +26,6 @@ using syncer::ProfileSyncService;
 
 namespace {
 
-class SyncDisabledChecker : public SingleClientStatusChangeChecker {
- public:
-  explicit SyncDisabledChecker(ProfileSyncService* service)
-      : SingleClientStatusChangeChecker(service) {}
-
-  SyncDisabledChecker(ProfileSyncService* service,
-                      base::OnceClosure condition_satisfied_callback)
-      : SingleClientStatusChangeChecker(service),
-        condition_satisfied_callback_(std::move(condition_satisfied_callback)) {
-  }
-
-  bool IsExitConditionSatisfied(std::ostream* os) override {
-    *os << "Waiting until sync is disabled";
-    bool satisfied = !service()->IsSetupInProgress() &&
-                     !service()->GetUserSettings()->IsFirstSetupComplete();
-    if (satisfied && condition_satisfied_callback_) {
-      std::move(condition_satisfied_callback_).Run();
-    }
-    return satisfied;
-  }
-
- private:
-  base::OnceClosure condition_satisfied_callback_;
-};
-
 class SyncEngineStoppedChecker : public SingleClientStatusChangeChecker {
  public:
   explicit SyncEngineStoppedChecker(ProfileSyncService* service)
@@ -207,8 +181,7 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, BirthdayErrorUsingActionableErrorTest) {
   // Now make one more change so we will do another sync.
   const BookmarkNode* node2 = AddFolder(0, 0, "title2");
   SetTitle(0, node2, "new_title2");
-
-  auto condition = base::BindLambdaForTesting([&]() {
+  EXPECT_TRUE(SyncDisabledChecker(GetSyncService(0)).Wait());
   syncer::SyncStatus status;
   GetSyncService(0)->QueryDetailedSyncStatusForDebugging(&status);
 
@@ -217,10 +190,7 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, BirthdayErrorUsingActionableErrorTest) {
   // resets the status. So query the status that the checker recorded at the
   // time Sync was off.
   EXPECT_EQ(status.sync_protocol_error.error_type, syncer::NOT_MY_BIRTHDAY);
-    EXPECT_EQ(status.sync_protocol_error.action,
-              syncer::DISABLE_SYNC_ON_CLIENT);
-  });
-  EXPECT_TRUE(SyncDisabledChecker(GetSyncService(0), condition).Wait());
+  EXPECT_EQ(status.sync_protocol_error.action, syncer::DISABLE_SYNC_ON_CLIENT);
 }
 
 // Tests that on receiving CLIENT_DATA_OBSOLETE sync engine gets restarted and

chrome/browser/sync/test/integration/sync_test.cc

@@ -32,6 +32,7 @@
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
+#include "chrome/browser/sync/test/integration/sync_disabled_checker.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/browser/ui/browser.h"
@@ -941,7 +942,17 @@ void SyncTest::ResetSyncForPrimaryAccount() {
     // clearing.
     SetupSyncNoWaitingForCompletion();
     GetClient(0)->ResetSyncForPrimaryAccount();
-    GetClient(0)->StopSyncServiceAndClearData();
+    // After reset account, the client should get a NOT_MY_BIRTHDAY error
+    // and disable sync. Adding a wait to make sure this is propagated.
+    ASSERT_TRUE(SyncDisabledChecker(GetSyncService(0)).Wait());
+    CloseBrowserSynchronously(browsers_[0]);
+    // After reset, this client will disable sync. It may log some messages
+    // that do not contribute to test failures. It includes:
+    //   PostClientToServerMessage with SERVER_RETURN_NOT_MY_BIRTHDAY
+    //   PostClientToServerMessage with NETWORK_CONNECTION_UNAVAILABLE
+    //   mcs_client fails with 401.
+    LOG(WARNING) << "Finished reset account. Warning logs before "
+                 << "this log may be safe to ignore.";
     ClearProfiles();
     use_new_user_data_dir_ = old_use_new_user_data_dir;
     num_clients_ = old_num_clients;

chrome/test/BUILD.gn

@@ -6087,6 +6087,8 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/sync_app_helper.h",
       "../browser/sync/test/integration/sync_datatype_helper.cc",
       "../browser/sync/test/integration/sync_datatype_helper.h",
+      "../browser/sync/test/integration/sync_disabled_checker.cc",
+      "../browser/sync/test/integration/sync_disabled_checker.h",
       "../browser/sync/test/integration/sync_extension_helper.cc",
       "../browser/sync/test/integration/sync_extension_helper.h",
       "../browser/sync/test/integration/sync_extension_installer.cc",