sync: Refactored Typed URLs Sync Integration Tests

This patch continues the refactoring started on integrations tests in r261300. 

In this patch AwaitQuiescence is removed from MultipleClientTypedURLSyncTest 
and some form of synchronization is also removed from TwoClientTypedURLSyncTest.
For this to be done, first the helper typed url functions had to be modified to replace 
the assertions on the helper side and move them to the test side.

AwaitQuiescence is replaced in each test by wrapping the actual checking 
function in a helper object implementing MultiClientStatusChangeChecker. 

BUG=97780

Review URL: https://codereview.chromium.org/233323005

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

6 files changed, 99 insertions, 87 deletions

diff --git a/chrome/browser/sync/test/integration/multiple_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/multiple_client_typed_urls_sync_test.cc
index ec81db8..6afafd1 100644
--- a/chrome/browser/sync/test/integration/multiple_client_typed_urls_sync_test.cc
+++ b/chrome/browser/sync/test/integration/multiple_client_typed_urls_sync_test.cc
@@ -12,7 +12,7 @@
 #include "chrome/browser/sync/test/integration/typed_urls_helper.h"
 
 using typed_urls_helper::AddUrlToHistory;
-using typed_urls_helper::AssertAllProfilesHaveSameURLsAsVerifier;
+using typed_urls_helper::AwaitCheckAllProfilesHaveSameURLsAsVerifier;
 using typed_urls_helper::GetTypedUrlsFromClient;
 
 class MultipleClientTypedUrlsSyncTest : public SyncTest {
@@ -37,11 +37,8 @@ IN_PROC_BROWSER_TEST_F(MultipleClientTypedUrlsSyncTest, AddToOne) {
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitGroupSyncCycleCompletion(clients()));
-
   // All clients should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(MultipleClientTypedUrlsSyncTest, AddToAll) {
@@ -63,9 +60,6 @@ IN_PROC_BROWSER_TEST_F(MultipleClientTypedUrlsSyncTest, AddToAll) {
     ASSERT_EQ(new_url, urls[0].url());
   }
 
-  // Wait for sync.
-  ASSERT_TRUE(AwaitQuiescence());
-
   // Verify that all clients have all urls.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
diff --git a/chrome/browser/sync/test/integration/performance/typed_urls_sync_perf_test.cc b/chrome/browser/sync/test/integration/performance/typed_urls_sync_perf_test.cc
index 49bd90a..78396f0 100644
--- a/chrome/browser/sync/test/integration/performance/typed_urls_sync_perf_test.cc
+++ b/chrome/browser/sync/test/integration/performance/typed_urls_sync_perf_test.cc
@@ -11,7 +11,6 @@
 #include "sync/sessions/sync_session_context.h"
 
 using typed_urls_helper::AddUrlToHistory;
-using typed_urls_helper::AssertAllProfilesHaveSameURLsAsVerifier;
 using typed_urls_helper::DeleteUrlsFromHistory;
 using typed_urls_helper::GetTypedUrlsFromClient;
 
diff --git a/chrome/browser/sync/test/integration/single_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/single_client_typed_urls_sync_test.cc
index 9199ec7..c754985 100644
--- a/chrome/browser/sync/test/integration/single_client_typed_urls_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_typed_urls_sync_test.cc
@@ -13,7 +13,7 @@
 using sync_integration_test_util::AwaitCommitActivityCompletion;
 using typed_urls_helper::AddUrlToHistory;
 using typed_urls_helper::AddUrlToHistoryWithTransition;
-using typed_urls_helper::AssertAllProfilesHaveSameURLsAsVerifier;
+using typed_urls_helper::CheckAllProfilesHaveSameURLsAsVerifier;
 using typed_urls_helper::DeleteUrlFromHistory;
 using typed_urls_helper::GetTypedUrlsFromClient;
 
@@ -39,11 +39,11 @@ IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, Sanity) {
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Wait for sync and verify client did not change.
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, TwoVisits) {
@@ -59,11 +59,11 @@ IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, TwoVisits) {
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Wait for sync and verify client did not change.
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, DeleteTyped) {
@@ -79,18 +79,18 @@ IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, DeleteTyped) {
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Wait for sync and verify client did not change.
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Now delete the URL we just added, wait for sync, and verify the deletion.
   DeleteUrlFromHistory(0, new_url);
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(0U, urls.size());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, DeleteNonTyped) {
@@ -105,16 +105,16 @@ IN_PROC_BROWSER_TEST_F(SingleClientTypedUrlsSyncTest, DeleteNonTyped) {
 
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(0U, urls.size());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Wait for sync and verify client did not change.
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 
   // Now delete the URL we just added, wait for sync and verify the deletion.
   DeleteUrlFromHistory(0, new_url);
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(0U, urls.size());
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(CheckAllProfilesHaveSameURLsAsVerifier());
 }
diff --git a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
index 9a47a59..b783a09 100644
--- a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
@@ -20,8 +20,8 @@ using typed_urls_helper::AddUrlToHistoryWithTimestamp;
 using typed_urls_helper::AddUrlToHistoryWithTransition;
 using typed_urls_helper::AreVisitsEqual;
 using typed_urls_helper::AreVisitsUnique;
-using typed_urls_helper::AssertAllProfilesHaveSameURLsAsVerifier;
-using typed_urls_helper::AssertURLRowVectorsAreEqual;
+using typed_urls_helper::AwaitCheckAllProfilesHaveSameURLsAsVerifier;
+using typed_urls_helper::CheckURLRowVectorsAreEqual;
 using typed_urls_helper::DeleteUrlFromHistory;
 using typed_urls_helper::GetTypedUrlsFromClient;
 using typed_urls_helper::GetUrlFromClient;
@@ -33,18 +33,19 @@ class TwoClientTypedUrlsSyncTest : public SyncTest {
   TwoClientTypedUrlsSyncTest() : SyncTest(TWO_CLIENT) {}
   virtual ~TwoClientTypedUrlsSyncTest() {}
 
-  bool CheckClientsEqual() {
+  ::testing::AssertionResult CheckClientsEqual() {
     history::URLRows urls = GetTypedUrlsFromClient(0);
     history::URLRows urls2 = GetTypedUrlsFromClient(1);
-    AssertURLRowVectorsAreEqual(urls, urls2);
+    if (!CheckURLRowVectorsAreEqual(urls, urls2))
+      return ::testing::AssertionFailure() << "URLVectors are not equal";
     // Now check the visits.
     for (size_t i = 0; i < urls.size() && i < urls2.size(); i++) {
       history::VisitVector visit1 = GetVisitsFromClient(0, urls[i].id());
       history::VisitVector visit2 = GetVisitsFromClient(1, urls2[i].id());
       if (!AreVisitsEqual(visit1, visit2))
-        return false;
+        return ::testing::AssertionFailure() << "Visits are not equal";
     }
-    return true;
+    return ::testing::AssertionSuccess();
   }
 
   bool CheckNoDuplicateVisits() {
@@ -84,11 +85,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, Add) {
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, AddExpired) {
@@ -173,20 +171,14 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, DISABLED_AddThenDelete) {
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 
   // Delete from first client, should delete from second.
   DeleteUrlFromHistory(0, new_url);
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Neither client should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 // TCM: 3643277
@@ -216,9 +208,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, DisableEnableSync) {
 
   // Enable typed url sync, make both URLs are synced to each client.
   GetClient(0)->EnableSyncForDatatype(syncer::TYPED_URLS);
-  ASSERT_TRUE(AwaitQuiescence());
 
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 // flaky, see crbug.com/108511
@@ -234,22 +225,16 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, DISABLED_AddOneDeleteOther) {
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 
   // Now, delete the URL from the second client.
   DeleteUrlFromHistory(1, new_url);
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL removed.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 // flaky, see crbug.com/108511
@@ -266,32 +251,23 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(new_url, urls[0].url());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 
   // Now, delete the URL from the second client.
   DeleteUrlFromHistory(1, new_url);
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL removed.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 
   // Add it to the first client again, should succeed (tests that the deletion
   // properly disassociates that URL).
   AddUrlToHistory(0, new_url);
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have this URL added again.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
@@ -381,11 +357,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, UpdateToNonTypedURL) {
   history::URLRows urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(0U, urls.size());
 
-  // Let sync finish.
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-
   // Both clients should have 0 typed URLs.
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
   urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(0U, urls.size());
 
@@ -467,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, BookmarksWithTypedVisit) {
   AddUrlToHistory(0, bookmark_url);
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
 
-  AssertAllProfilesHaveSameURLsAsVerifier();
+  ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
   history::URLRows urls = GetTypedUrlsFromClient(0);
   ASSERT_EQ(1U, urls.size());
   ASSERT_EQ(bookmark_url, urls[0].url());
diff --git a/chrome/browser/sync/test/integration/typed_urls_helper.cc b/chrome/browser/sync/test/integration/typed_urls_helper.cc
index 0c60665..54fea80 100644
--- a/chrome/browser/sync/test/integration/typed_urls_helper.cc
+++ b/chrome/browser/sync/test/integration/typed_urls_helper.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/test/integration/multi_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 
@@ -308,22 +309,26 @@ void DeleteUrlsFromHistory(int index, const std::vector<GURL>& urls) {
   WaitForHistoryDBThread(index);
 }
 
-void AssertURLRowVectorsAreEqual(const history::URLRows& left,
-                                 const history::URLRows& right) {
-  ASSERT_EQ(left.size(), right.size());
+bool CheckURLRowVectorsAreEqual(const history::URLRows& left,
+                                const history::URLRows& right) {
+  if (left.size() != right.size())
+    return false;
   for (size_t i = 0; i < left.size(); ++i) {
     // URLs could be out-of-order, so look for a matching URL in the second
     // array.
     bool found = false;
     for (size_t j = 0; j < right.size(); ++j) {
       if (left[i].url() == right[j].url()) {
-        AssertURLRowsAreEqual(left[i], right[j]);
-        found = true;
-        break;
+        if (CheckURLRowsAreEqual(left[i], right[j])) {
+          found = true;
+          break;
+        }
       }
     }
-    ASSERT_TRUE(found);
+    if (!found)
+      return false;
   }
+  return true;
 }
 
 bool AreVisitsEqual(const history::VisitVector& visit1,
@@ -349,17 +354,17 @@ bool AreVisitsUnique(const history::VisitVector& visits) {
   return true;
 }
 
-void AssertURLRowsAreEqual(
+bool CheckURLRowsAreEqual(
     const history::URLRow& left, const history::URLRow& right) {
-  ASSERT_EQ(left.url(), right.url());
-  ASSERT_EQ(left.title(), right.title());
-  ASSERT_EQ(left.visit_count(), right.visit_count());
-  ASSERT_EQ(left.typed_count(), right.typed_count());
-  ASSERT_EQ(left.last_visit(), right.last_visit());
-  ASSERT_EQ(left.hidden(), right.hidden());
+  return (left.url() == right.url()) &&
+      (left.title() == right.title()) &&
+      (left.visit_count() == right.visit_count()) &&
+      (left.typed_count() == right.typed_count()) &&
+      (left.last_visit() == right.last_visit()) &&
+      (left.hidden() == right.hidden());
 }
 
-void AssertAllProfilesHaveSameURLsAsVerifier() {
+bool CheckAllProfilesHaveSameURLsAsVerifier() {
   HistoryService* verifier_service =
       HistoryServiceFactory::GetForProfile(test()->verifier(),
                                            Profile::IMPLICIT_ACCESS);
@@ -367,8 +372,45 @@ void AssertAllProfilesHaveSameURLsAsVerifier() {
       GetTypedUrlsFromHistoryService(verifier_service);
   for (int i = 0; i < test()->num_clients(); ++i) {
     history::URLRows urls = GetTypedUrlsFromClient(i);
-    AssertURLRowVectorsAreEqual(verifier_urls, urls);
+    if (!CheckURLRowVectorsAreEqual(verifier_urls, urls))
+      return false;
   }
+  return true;
+}
+
+namespace {
+
+// Helper class used in the implementation of
+// AwaitCheckAllProfilesHaveSameURLsAsVerifier.
+class ProfilesHaveSameURLsChecker : public MultiClientStatusChangeChecker {
+ public:
+  ProfilesHaveSameURLsChecker();
+  virtual ~ProfilesHaveSameURLsChecker();
+
+  virtual bool IsExitConditionSatisfied() OVERRIDE;
+  virtual std::string GetDebugMessage() const OVERRIDE;
+};
+
+ProfilesHaveSameURLsChecker::ProfilesHaveSameURLsChecker()
+    : MultiClientStatusChangeChecker(
+        sync_datatype_helper::test()->GetSyncServices()) {}
+
+ProfilesHaveSameURLsChecker::~ProfilesHaveSameURLsChecker() {}
+
+bool ProfilesHaveSameURLsChecker::IsExitConditionSatisfied() {
+  return CheckAllProfilesHaveSameURLsAsVerifier();
+}
+
+std::string ProfilesHaveSameURLsChecker::GetDebugMessage() const {
+  return "Waiting for matching typed urls profiles";
+}
+
+}  //  namespace
+
+bool AwaitCheckAllProfilesHaveSameURLsAsVerifier() {
+  ProfilesHaveSameURLsChecker checker;
+  checker.Wait();
+  return !checker.TimedOut();
 }
 
 }  // namespace typed_urls_helper
diff --git a/chrome/browser/sync/test/integration/typed_urls_helper.h b/chrome/browser/sync/test/integration/typed_urls_helper.h
index df2f46f..d057953 100644
--- a/chrome/browser/sync/test/integration/typed_urls_helper.h
+++ b/chrome/browser/sync/test/integration/typed_urls_helper.h
@@ -57,16 +57,20 @@ void DeleteUrlFromHistory(int index, const GURL& url);
 void DeleteUrlsFromHistory(int index, const std::vector<GURL>& urls);
 
 // Returns true if all clients match the verifier profile.
-void AssertAllProfilesHaveSameURLsAsVerifier();
+bool CheckAllProfilesHaveSameURLsAsVerifier();
+
+// Returns true if all clients match the verifier profile and if the
+// verification doesn't time out.
+bool AwaitCheckAllProfilesHaveSameURLsAsVerifier();
 
 // Checks that the two vectors contain the same set of URLRows (possibly in
 // a different order).
-void AssertURLRowVectorsAreEqual(const history::URLRows& left,
-                                 const history::URLRows& right);
+bool CheckURLRowVectorsAreEqual(const history::URLRows& left,
+                                const history::URLRows& right);
 
 // Checks that the passed URLRows are equivalent.
-void AssertURLRowsAreEqual(const history::URLRow& left,
-                           const history::URLRow& right);
+bool CheckURLRowsAreEqual(const history::URLRow& left,
+                          const history::URLRow& right);
 
 // Returns true if two sets of visits are equivalent.
 bool AreVisitsEqual(const history::VisitVector& visit1,