Significantly speed up sync integration tests.

The sync integration tests currently use the method AwaitSyncCycleCompletion(), which ends up waiting for a sync event before it signals the end of a sync cycle. However, if it is called after the sync cycle is already complete, it ends up waiting for several seconds for a random unrelated sync event before it returns.

This checkin fixes this unnecessary delay by checking to see if the client has any unsynced items before it waits, thereby significantly speeding up the sync integration tests.

In addition to this, a new method called AwaitQuiescence() is now available to test cases so they can wait for racy updates initiated by multiple clients to propagate across the system.

BUG=49998
TEST=sync_integration_tests

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

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

8 files changed, 66 insertions, 19 deletions

diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index d9c5a9f..e0fe352 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -2139,4 +2139,9 @@ UserShare* SyncManager::GetUserShare() const {
   return data_->GetUserShare();
 }
 
+bool SyncManager::HasUnsyncedItems() const {
+  sync_api::ReadTransaction trans(GetUserShare());
+  return (trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0);
+}
+
 }  // namespace sync_api
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index 90e1a5c..4ea5ebd 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -812,6 +812,10 @@ class SyncManager {
 
   UserShare* GetUserShare() const;
 
+  // Uses a read-only transaction to determine if the directory being synced has
+  // any remaining unsynced items.
+  bool HasUnsyncedItems() const;
+
  private:
   // An opaque pointer to the nested private class.
   SyncInternal* data_;
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index bdcf2e1..2578c7e 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -344,6 +344,10 @@ void SyncBackendHost::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {
   out->swap(copy);
 }
 
+bool SyncBackendHost::HasUnsyncedItems() const {
+  return core_->syncapi()->HasUnsyncedItems();
+}
+
 SyncBackendHost::Core::Core(SyncBackendHost* backend)
     : host_(backend),
       syncapi_(new sync_api::SyncManager()) {
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 16a9980..4b4ce1d 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -178,6 +178,10 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
   virtual void GetWorkers(std::vector<browser_sync::ModelSafeWorker*>* out);
   virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out);
 
+  // Determines if the underlying sync engine has made any local changes to
+  // items that have not yet been synced with the server.
+  bool HasUnsyncedItems() const;
+
 #if defined(UNIT_TEST)
   // Called from unit test to bypass authentication and initialize the syncapi
   // to a state suitable for testing but not production.
diff --git a/chrome/test/live_sync/multiple_client_live_bookmarks_sync_test.cc b/chrome/test/live_sync/multiple_client_live_bookmarks_sync_test.cc
index c0b56a1..5a570dd 100644
--- a/chrome/test/live_sync/multiple_client_live_bookmarks_sync_test.cc
+++ b/chrome/test/live_sync/multiple_client_live_bookmarks_sync_test.cc
@@ -14,10 +14,9 @@ IN_PROC_BROWSER_TEST_F(MultipleClientLiveBookmarksSyncTest, Sanity) {
         StringPrintf(L"Google URL %d", i),
         GURL(StringPrintf("http://www.google.com/%d", i)));
   }
-  for (int i = 0; i < num_clients(); ++i) {
-    GetClient(i)->AwaitGroupSyncCycleCompletion(clients());
-  }
-  for (int i = 0; i < num_clients(); ++i) {
-    v->ExpectMatch(GetBookmarkModel(i));
+  ProfileSyncServiceTestHarness::AwaitQuiescence(clients());
+  for (int i = 1; i < num_clients(); ++i) {
+    BookmarkModelVerifier::ExpectModelsMatch(GetBookmarkModel(0),
+        GetBookmarkModel(i));
   }
 }
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 0447e84..bd9ceab 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.cc
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.cc
@@ -14,8 +14,6 @@
 #include "chrome/test/ui_test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using browser_sync::sessions::SyncSessionSnapshot;
-
 // The default value for min_timestamp_needed_ when we're not in the
 // WAITING_FOR_UPDATES state.
 static const int kMinTimestampNeededNone = -1;
@@ -133,8 +131,7 @@ bool ProfileSyncServiceTestHarness::RunStateChangeMachine() {
       break;
     }
     case WAITING_FOR_SYNC_TO_FINISH: {
-      const SyncSessionSnapshot* snap =
-          service_->backend()->GetLastSessionSnapshot();
+      const SyncSessionSnapshot* snap = GetLastSessionSnapshot();
       DCHECK(snap) << "Should have been at least one sync session by now";
       // TODO(rsimha): In an ideal world, snap->has_more_to_sync == false should
       // be a sufficient condition for sync to have completed. However, the
@@ -150,8 +147,7 @@ bool ProfileSyncServiceTestHarness::RunStateChangeMachine() {
       break;
     }
     case WAITING_FOR_UPDATES: {
-      const SyncSessionSnapshot* snap =
-          service_->backend()->GetLastSessionSnapshot();
+      const SyncSessionSnapshot* snap = GetLastSessionSnapshot();
       DCHECK(snap) << "Should have been at least one sync session by now";
       if (snap->max_local_timestamp < min_timestamp_needed_)
         break;
@@ -175,8 +171,12 @@ void ProfileSyncServiceTestHarness::OnStateChanged() {
 
 bool ProfileSyncServiceTestHarness::AwaitSyncCycleCompletion(
     const std::string& reason) {
-  wait_state_ = WAITING_FOR_SYNC_TO_FINISH;
-  return AwaitStatusChangeWithTimeout(60, reason);
+  if (service()->backend()->HasUnsyncedItems()) {
+    wait_state_ = WAITING_FOR_SYNC_TO_FINISH;
+    return AwaitStatusChangeWithTimeout(60, reason);
+  } else {
+    return true;
+  }
 }
 
 bool ProfileSyncServiceTestHarness::AwaitMutualSyncCycleCompletion(
@@ -207,11 +207,22 @@ bool ProfileSyncServiceTestHarness::AwaitGroupSyncCycleCompletion(
   return return_value;
 }
 
+// static
+bool ProfileSyncServiceTestHarness::AwaitQuiescence(
+    std::vector<ProfileSyncServiceTestHarness*>& clients) {
+  bool return_value = true;
+  for (std::vector<ProfileSyncServiceTestHarness*>::iterator it =
+      clients.begin(); it != clients.end(); ++it) {
+    return_value = return_value &&
+        (*it)->AwaitGroupSyncCycleCompletion(clients);
+  }
+  return return_value;
+}
+
 bool ProfileSyncServiceTestHarness::WaitUntilTimestampIsAtLeast(
     int64 timestamp, const std::string& reason) {
   min_timestamp_needed_ = timestamp;
-  const SyncSessionSnapshot* snap =
-      service_->backend()->GetLastSessionSnapshot();
+  const SyncSessionSnapshot* snap = GetLastSessionSnapshot();
   DCHECK(snap) << "Should have been at least one sync session by now";
   if (snap->max_local_timestamp < min_timestamp_needed_) {
     wait_state_ = WAITING_FOR_UPDATES;
@@ -268,3 +279,9 @@ bool ProfileSyncServiceTestHarness::WaitForServiceInit() {
 
   return true;
 }
+
+const SyncSessionSnapshot*
+    ProfileSyncServiceTestHarness::GetLastSessionSnapshot() const {
+  EXPECT_FALSE(service_ == NULL) << "Sync service has not yet been set up.";
+  return service_->backend()->GetLastSessionSnapshot();
+}
diff --git a/chrome/test/live_sync/profile_sync_service_test_harness.h b/chrome/test/live_sync/profile_sync_service_test_harness.h
index 9f409ec..190e3f6 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.h
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.h
@@ -11,6 +11,8 @@
 #include "base/time.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 
+using browser_sync::sessions::SyncSessionSnapshot;
+
 class Profile;
 
 // An instance of this class is basically our notion of a "sync client" for
@@ -44,14 +46,25 @@ class ProfileSyncServiceTestHarness : public ProfileSyncServiceObserver {
   // the |partner| should be the passive responder who responds to the actions
   // of |this|.  This method relies upon the synchronization of callbacks
   // from the message queue. Returns true if two sync cycles have completed.
+  // Note: Use this method when exactly one client makes local change(s), and
+  // exactly one client is waiting to receive those changes.
   bool AwaitMutualSyncCycleCompletion(ProfileSyncServiceTestHarness* partner);
 
   // Blocks the caller until |this| completes its ongoing sync cycle and every
   // other client in |partners| has a timestamp that is greater than or equal to
-  // the timestamp of |this|.
+  // the timestamp of |this|. Note: Use this method when exactly one client
+  // makes local change(s), and more than one client is waiting to receive those
+  // changes.
   bool AwaitGroupSyncCycleCompletion(
       std::vector<ProfileSyncServiceTestHarness*>& partners);
 
+  // Blocks the caller until every client in |clients| completes its ongoing
+  // sync cycle and all the clients' timestamps match.  Note: Use this method
+  // when more than one client makes local change(s), and more than one client
+  // is waiting to receive those changes.
+  static bool AwaitQuiescence(
+      std::vector<ProfileSyncServiceTestHarness*>& clients);
+
   ProfileSyncService* service() { return service_; }
 
   // See ProfileSyncService::ShouldPushChanges().
@@ -96,6 +109,9 @@ class ProfileSyncServiceTestHarness : public ProfileSyncServiceObserver {
   Profile* profile_;
   ProfileSyncService* service_;
 
+  // Returns a snapshot of the current sync session.
+  const SyncSessionSnapshot* GetLastSessionSnapshot() const;
+
   // State tracking.  Used for debugging and tracking of state.
   ProfileSyncService::Status last_status_;
 
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 8d0a50d..f1b5045 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
@@ -90,9 +90,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest, Sanity) {
     bm1->SetTitle(google_two, L"Google--");
   }
 
-  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);
 }