Fixing broken Integration tests.

Several integration tests use the wrong method to wait for sync to complete, as a result of which they fail during verification.

Also making the condition for the detection of sync completion more robust.

BUG=50114
TEST=sync_integration_tests

Review URL: http://codereview.chromium.org/3037019

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53759 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 52 insertions, 53 deletions

diff --git a/chrome/test/live_sync/live_sync_test.h b/chrome/test/live_sync/live_sync_test.h
index 3774ecc..8f6cf2f 100644
--- a/chrome/test/live_sync/live_sync_test.h
+++ b/chrome/test/live_sync/live_sync_test.h
@@ -26,8 +26,8 @@ namespace net {
 class ScopedDefaultHostResolverProc;
 }
 
-// Live sync tests are allowed to run for up to 5 minutes.
-const int kTestTimeoutInMS = 300000;
+// Live sync tests are allowed to run for up to 45 seconds.
+const int kTestTimeoutInMS = 45000;
 
 // This is the base class for integration tests for all sync data types. Derived
 // classes must be defined for each sync data type. Individual tests are defined
diff --git a/chrome/test/live_sync/profile_sync_service_test_harness.cc b/chrome/test/live_sync/profile_sync_service_test_harness.cc
index bd9ceab..081b0d8 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.cc
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.cc
@@ -140,7 +140,6 @@ bool ProfileSyncServiceTestHarness::RunStateChangeMachine() {
       if (snap->has_more_to_sync || snap->unsynced_count != 0) {
         break;
       }
-
       EXPECT_LE(last_timestamp_, snap->max_local_timestamp);
       last_timestamp_ = snap->max_local_timestamp;
       SignalStateCompleteWithNextState(FULLY_SYNCED);
@@ -149,9 +148,11 @@ bool ProfileSyncServiceTestHarness::RunStateChangeMachine() {
     case WAITING_FOR_UPDATES: {
       const SyncSessionSnapshot* snap = GetLastSessionSnapshot();
       DCHECK(snap) << "Should have been at least one sync session by now";
-      if (snap->max_local_timestamp < min_timestamp_needed_)
+      if (snap->max_local_timestamp < min_timestamp_needed_) {
         break;
-
+      }
+      EXPECT_LE(last_timestamp_, snap->max_local_timestamp);
+      last_timestamp_ = snap->max_local_timestamp;
       SignalStateCompleteWithNextState(FULLY_SYNCED);
       break;
     }
@@ -171,10 +172,18 @@ void ProfileSyncServiceTestHarness::OnStateChanged() {
 
 bool ProfileSyncServiceTestHarness::AwaitSyncCycleCompletion(
     const std::string& reason) {
-  if (service()->backend()->HasUnsyncedItems()) {
+  const SyncSessionSnapshot* snap = GetLastSessionSnapshot();
+  DCHECK(snap) << "Should have been at least one sync session by now";
+  // TODO(rsimha): Remove additional checks of snap->has_more_to_sync and
+  // snap->unsynced_count once http://crbug.com/48989 is fixed.
+  if (service()->backend()->HasUnsyncedItems() ||
+      snap->has_more_to_sync ||
+      snap->unsynced_count != 0) {
     wait_state_ = WAITING_FOR_SYNC_TO_FINISH;
     return AwaitStatusChangeWithTimeout(60, reason);
   } else {
+    EXPECT_LE(last_timestamp_, snap->max_local_timestamp);
+    last_timestamp_ = snap->max_local_timestamp;
     return true;
   }
 }
diff --git a/chrome/test/live_sync/two_client_live_autofill_sync_test.cc b/chrome/test/live_sync/two_client_live_autofill_sync_test.cc
index 8f344a6..dacb910 100644
--- a/chrome/test/live_sync/two_client_live_autofill_sync_test.cc
+++ b/chrome/test/live_sync/two_client_live_autofill_sync_test.cc
@@ -16,6 +16,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveAutofillSyncTest, Client1HasData) {
   AddFormFieldsToWebData(GetWebDataService(0), keys);
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   AutofillKeys wd1_keys;
   GetAllAutofillKeys(GetWebDataService(1), &wd1_keys);
@@ -39,8 +40,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveAutofillSyncTest, BothHaveData) {
   AddFormFieldsToWebData(GetWebDataService(1), keys2);
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  // Wait for client1 to get the new keys from client2.
-  EXPECT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("sync cycle"));
+  EXPECT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   AutofillKeys expected_keys;
   expected_keys.insert(AutofillKey("name0", "value0"));
@@ -142,6 +142,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveAutofillSyncTest, ProfileClient1HasData) {
   AddProfile(GetPersonalDataManager(0), *expected_profiles[1]);
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   EXPECT_TRUE(CompareAutoFillProfiles(expected_profiles,
       GetAllAutoFillProfiles(GetPersonalDataManager(0))));
@@ -167,6 +168,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveAutofillSyncTest,
   AddProfile(GetPersonalDataManager(1), *profiles2[0]);
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   // Since client1 associates first, client2's "Shipping" profile will
   // be overwritten by the one stored in the cloud by profile1.
@@ -192,6 +194,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveAutofillSyncTest,
   AddProfile(GetPersonalDataManager(0), *expected_profiles[1]);
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   // One of the duplicate profiles will have its label renamed to
   // "Shipping2".
diff --git a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
index f1b5045..08f83e8 100644
--- a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
+++ b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
@@ -132,9 +132,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
         BookmarkEditor::EditDetails(google_two), title, third_url);
   }
 
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  // Make sure that client2 has pushed all of it's changes as well.
-  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 
   {
@@ -158,7 +156,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
   v->ExpectMatch(bm1);
 
   {
-    // Let's add first bookmark folder to client1
+    // Let's add first bookmark folder to client0
     const BookmarkNode* new_folder_one =
         v->AddGroup(bm0, bm_bar0, 0, L"TestFolder");
     ASSERT_TRUE(new_folder_one != NULL);
@@ -284,7 +282,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 370563.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_AddSeveralBMsAndFolders) {
+    SC_AddSeveralBMsAndFolders) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -374,7 +372,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371817.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_RenameBMName) {
+    SC_RenameBMName) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -486,7 +484,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371826.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_RenameBMFolderWithLongHierarchy) {
+    SC_RenameBMFolderWithLongHierarchy) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -824,7 +822,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371857.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_DelBMsUnderBMFoldEmptyFolderAfterwards) {
+    SC_DelBMsUnderBMFoldEmptyFolderAfterwards) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -942,6 +940,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 }
 
 // Test Scribe ID - 371879.
+// TODO(rsimha): This currently fails due to http://crbug.com/50306.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     FAILS_SC_DelBMFoldWithBMsNonEmptyAccountAfterwards) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
@@ -997,19 +996,17 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     ASSERT_TRUE(nofavicon_bm != NULL);
   }
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 
   // Let's delete the bookmark folder (bm_folder_one)
   v->Remove(bm0, bm_bar0, 1);
 
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 }
 
-
 // Test Scribe ID - 371880.
+// TODO(rsimha): This currently fails due to http://crbug.com/50306.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     FAILS_SC_DelBMFoldWithBMsAndBMFoldsNonEmptyACAfterwards) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
@@ -1101,19 +1098,17 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
      }
     }
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 
   // Let's delete the bookmark folder (bm_folder_one)
   v->Remove(bm0, bm_bar0, 1);
 
-
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 }
 
 // Test Scribe ID - 371882.
+// TODO(rsimha): This currently fails due to http://crbug.com/50306.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     FAILS_SC_DelBMFoldWithParentAndChildrenBMsAndBMFolds) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
@@ -1169,17 +1164,15 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
   }
 
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 
   // Let's delete test_bm_folder
   v->Remove(bm0, parent_bm_folder, 0);
+
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  v->ExpectMatch(bm0);
-  v->ExpectMatch(bm1);
+  BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 }
 
-
 // Test Scribe ID - 371931.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     SC_ReverseTheOrderOfTwoBMs) {
@@ -1295,7 +1288,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371957.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_MovingBMsFromBMFoldToBMBar) {
+    SC_MovingBMsFromBMFoldToBMBar) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -1461,7 +1454,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371967.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_HoistBMs10LevelUp) {
+    SC_HoistBMs10LevelUp) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -1600,7 +1593,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371980.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_SinkEmptyBMFold5LevelsDown) {
+    SC_SinkEmptyBMFold5LevelsDown) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -1655,7 +1648,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 371997.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_SinkNonEmptyBMFold5LevelsDown) {
+    SC_SinkNonEmptyBMFold5LevelsDown) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -1821,7 +1814,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 372028.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_SC_ReverseTheOrderOfTenBMFolders) {
+    SC_ReverseTheOrderOfTenBMFolders) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
@@ -1879,31 +1872,25 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     ASSERT_TRUE(bm_foo4 != NULL);
   }
 
-
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  // Make sure that client2 has pushed all of it's changes as well.
-  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   BookmarkModelVerifier::ExpectModelsMatch(bm0, bm1);
 }
 
 // Test Scribe ID - 373506.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_MC_BootStrapEmptyStateEverywhere) {
+    MC_BootStrapEmptyStateEverywhere) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   BookmarkModelVerifier* v = verifier_helper();
   BookmarkModel* bm0 = GetBookmarkModel(0);
   BookmarkModel* bm1 = GetBookmarkModel(1);
-
-  // Wait for changes to propagate.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  // Let's compare and make sure both bookmark models are same after sync.
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   v->ExpectMatch(bm0);
   v->ExpectMatch(bm1);
 }
 
 // Test Scribe ID - 373508.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_MC_SimpleMergeOfDifferentBMModels) {
+    MC_SimpleMergeOfDifferentBMModels) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
   BookmarkModel* bm0 = GetBookmarkModel(0);
   BookmarkModel* bm1 = GetBookmarkModel(1);
@@ -1954,7 +1941,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   // Wait for changes to propagate.
-  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   // Let's make sure there aren't any duplicates after sync.
   BookmarkModelVerifier::VerifyNoDuplicates(bm0);
   // Let's compare and make sure both bookmark models are same after sync.
@@ -1964,7 +1951,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 386586.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_MC_MergeSimpleBMHierarchyUnderBMBar) {
+    MC_MergeSimpleBMHierarchyUnderBMBar) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
   BookmarkModel* bm0 = GetBookmarkModel(0);
   BookmarkModel* bm1 = GetBookmarkModel(1);
@@ -2004,7 +1991,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   // Wait for changes to propagate.
-  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   // Let's make sure there aren't any duplicates after sync.
   BookmarkModelVerifier::VerifyNoDuplicates(bm0);
   // Let's compare and make sure both bookmark models are same after sync.
@@ -2014,7 +2001,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 
 // Test Scribe ID - 386589.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-    FAILS_MC_MergeSimpleBMHierarchyEqualSetsUnderBMBar) {
+    MC_MergeSimpleBMHierarchyEqualSetsUnderBMBar) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
   BookmarkModel* bm0 = GetBookmarkModel(0);
   BookmarkModel* bm1 = GetBookmarkModel(1);
@@ -2042,7 +2029,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   // Wait for changes to propagate.
-  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
   // Let's make sure there aren't any duplicates after sync.
   BookmarkModelVerifier::VerifyNoDuplicates(bm0);
   // Let's compare and make sure both bookmark models are same after sync.
diff --git a/chrome/test/live_sync/two_client_live_preferences_sync_test.cc b/chrome/test/live_sync/two_client_live_preferences_sync_test.cc
index 390806b..5e72c13 100644
--- a/chrome/test/live_sync/two_client_live_preferences_sync_test.cc
+++ b/chrome/test/live_sync/two_client_live_preferences_sync_test.cc
@@ -21,12 +21,12 @@ IN_PROC_BROWSER_TEST_F(TwoClientLivePreferencesSyncTest, Sanity) {
             GetPrefs(1)->GetBoolean(prefs::kHomePageIsNewTabPage));
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientLivePreferencesSyncTest, FAILS_Race) {
+IN_PROC_BROWSER_TEST_F(TwoClientLivePreferencesSyncTest, Race) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   GetPrefs(0)->SetString(prefs::kHomePage, "http://www.google.com/1");
   GetPrefs(1)->SetString(prefs::kHomePage, "http://www.google.com/2");
-  EXPECT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(ProfileSyncServiceTestHarness::AwaitQuiescence(clients()));
 
   EXPECT_EQ(GetPrefs(0)->GetString(prefs::kHomePage),
             GetPrefs(1)->GetString(prefs::kHomePage));