[AF Profile] Fix integration flaky tests with delayed SetupSync()

Tests that setup local data and only after that SetupSync are flaky on
multiple bots. This spans many different integration test files and
probably lies deeper in the interaction of Directory and USS types
(these tests become very flaky after the last type gets switched to USS
on trybots).

This CL switches to sequential setup for such flaky tests for
autofill_profile. In follow-up CLs, this will get fixed for further
affected tests.

Bug: 956043
Change-Id: I0e3c3bb76cbe1825ea5d55b6103972077b825e9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1581806
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653668}

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

@@ -35,6 +35,7 @@
 #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_integration_test_util.h"
+#include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -694,10 +695,14 @@ void SyncTest::InitializeInvalidations(int index) {
 }
 
 void SyncTest::SetupSyncNoWaitingForCompletion() {
-  SetupSyncInternal(/*wait_for_completion=*/false);
+  SetupSyncInternal(/*setup_mode=*/NO_WAITING);
 }
 
-void SyncTest::SetupSyncInternal(bool wait_for_completion) {
+void SyncTest::SetupSyncOneClientAfterAnother() {
+  SetupSyncInternal(/*setup_mode=*/WAIT_FOR_COMMITS_TO_COMPLETE);
+}
+
+void SyncTest::SetupSyncInternal(SetupSyncMode setup_mode) {
   // Create sync profiles and clients if they haven't already been created.
   if (profiles_.empty()) {
     if (!SetupClients()) {
@@ -747,18 +752,29 @@ void SyncTest::SetupSyncInternal(bool wait_for_completion) {
           syncer::UserSelectableTypeSet::All()))
           << "SetupSync() failed.";
     }
-    if (wait_for_completion) {
-      // It's important to wait for each client before setting up the next one,
-      // otherwise multi-client tests get flaky.
-      // TODO(tschumann): It would be nice to figure out why.
-      client->AwaitSyncSetupCompletion();
+
+    // It's important to wait for each client before setting up the next one,
+    // otherwise multi-client tests get flaky.
+    // TODO(crbug.com/956043): It would be nice to figure out why.
+    switch (setup_mode) {
+      case NO_WAITING:
+        break;
+      case WAIT_FOR_SYNC_SETUP_TO_COMPLETE:
+        client->AwaitSyncSetupCompletion();
+        break;
+      case WAIT_FOR_COMMITS_TO_COMPLETE:
+        DCHECK(TestUsesSelfNotifications())
+            << "We need that for the UpdatedProgressMarkerChecker";
+        UpdatedProgressMarkerChecker checker(GetSyncService(client_index));
+        checker.Wait();
+        break;
     }
   }
 }
 
 bool SyncTest::SetupSync() {
   base::ScopedAllowBlockingForTesting allow_blocking;
-  SetupSyncInternal(/*wait_for_completion=*/true);
+  SetupSyncInternal(/*setup_mode=*/WAIT_FOR_SYNC_SETUP_TO_COMPLETE);
 
   // Because clients may modify sync data as part of startup (for example
   // local session-releated data is rewritten), we need to ensure all

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

@@ -160,6 +160,13 @@ class SyncTest : public InProcessBrowserTest {
   // Like SetupSync() but does not wait for the clients to be ready to sync.
   void SetupSyncNoWaitingForCompletion();
 
+  // Like SetupSync() but does wait for commits to complete before proceeding to
+  // another client.
+  // TODO(crbug.com/956043): Investigate deeper why such sequential setup is
+  // needed by some tests and why using SetupSync() instead is causing
+  // flakiness. Ideally get rid of this function.
+  void SetupSyncOneClientAfterAnother();
+
   // Sets whether or not the sync clients in this test should respond to
   // notifications of their own commits.  Real sync clients do not do this, but
   // many test assertions require this behavior.
@@ -267,6 +274,11 @@ class SyncTest : public InProcessBrowserTest {
   network::TestURLLoaderFactory test_url_loader_factory_;
 
  private:
+  enum SetupSyncMode {
+    NO_WAITING,
+    WAIT_FOR_SYNC_SETUP_TO_COMPLETE,
+    WAIT_FOR_COMMITS_TO_COMPLETE
+  };
   // Handles Profile creation for given index. Profile's path and type is
   // determined at runtime based on server type.
   bool CreateProfile(int index);
@@ -327,7 +339,7 @@ class SyncTest : public InProcessBrowserTest {
   void InitializeInvalidations(int index);
 
   // Internal routine for setting up sync.
-  void SetupSyncInternal(bool wait_for_completion);
+  void SetupSyncInternal(SetupSyncMode setup_mode);
 
   // GAIA account used by the test case.
   std::string username_;

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

@@ -148,7 +148,10 @@ IN_PROC_BROWSER_TEST_P(TwoClientAutofillProfileSyncTest,
 
   base::HistogramTester histograms;
 
-  ASSERT_TRUE(SetupSync());
+  // Commit sequentially to make sure there is no race condition.
+  SetupSyncOneClientAfterAnother();
+  EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
+
   ASSERT_EQ(2U, GetAllAutoFillProfiles(0).size());
 
   // The order of events is roughly: First client (whichever that happens to be)
@@ -218,7 +221,8 @@ IN_PROC_BROWSER_TEST_P(TwoClientAutofillProfileSyncTest,
 
   AddProfile(0, profile0);
   AddProfile(1, profile1);
-  ASSERT_TRUE(SetupSync());
+  // Commit sequentially to make sure there is no race condition.
+  SetupSyncOneClientAfterAnother();
   EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
 
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
@@ -330,8 +334,8 @@ IN_PROC_BROWSER_TEST_P(TwoClientAutofillProfileSyncTest,
   ASSERT_NE(GetAllAutoFillProfiles(0)[0]->guid(),
             GetAllAutoFillProfiles(1)[0]->guid());
 
-  // Wait for the sync to happen.
-  ASSERT_TRUE(SetupSync());
+  // Commit sequentially to make sure there is no race condition.
+  SetupSyncOneClientAfterAnother();
   EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
 
   // Make sure that both clients have one profile.