[Sync] Add integration tests for migration cycle

Add integration tests to make sure that the migration cycle doesn't stall.

Add way for sync integration tests to turn off notifications.

BUG=92928
TEST=


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

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

8 files changed, 168 insertions, 31 deletions

diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
index e41a701..dbbd3a4 100644
--- a/chrome/browser/automation/testing_automation_provider.cc
+++ b/chrome/browser/automation/testing_automation_provider.cc
@@ -4510,7 +4510,8 @@ void TestingAutomationProvider::SignInToSync(Browser* browser,
   }
   if (sync_waiter_.get() == NULL) {
     sync_waiter_.reset(new ProfileSyncServiceHarness(
-        browser->profile(), username, password));
+        browser->profile(), username, password,
+        true /* expected_notifications_enabled */));
   } else {
     sync_waiter_->SetCredentials(username, password);
   }
diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc
index 0409545..993b727 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -897,7 +897,7 @@ void SyncScheduler::HandleConsecutiveContinuationError(
   TimeDelta length = delay_provider_->GetDelay(
       IsBackingOff() ? wait_interval_->length : TimeDelta::FromSeconds(1));
 
-  SVLOG(2) << "In handle continuation error with"
+  SVLOG(2) << "In handle continuation error with "
            << SyncSessionJob::GetPurposeString(old_job.purpose)
            << " job. The time delta(ms) is "
            << length.InMilliseconds();
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 116164f..efa717b 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -355,6 +355,9 @@ void SyncBackendHost::ConfigureDataTypes(
                               pending_config_mode_state_.get());
   }
 
+  // Cleanup disabled types before starting configuration so that
+  // callers can assume that the data types are cleaned up once
+  // configuration is done.
   if (!types_to_remove.empty()) {
     sync_thread_.message_loop()->PostTask(
         FROM_HERE,
diff --git a/chrome/browser/sync/profile_sync_service_harness.cc b/chrome/browser/sync/profile_sync_service_harness.cc
index 20a75ef..9149723 100644
--- a/chrome/browser/sync/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/profile_sync_service_harness.cc
@@ -95,7 +95,8 @@ bool StateChangeTimeoutEvent::Abort() {
 ProfileSyncServiceHarness::ProfileSyncServiceHarness(
     Profile* profile,
     const std::string& username,
-    const std::string& password)
+    const std::string& password,
+    bool expected_notifications_enabled)
     : waiting_for_encryption_type_(syncable::UNSPECIFIED),
       wait_state_(INITIAL_WAIT_STATE),
       profile_(profile),
@@ -103,6 +104,7 @@ ProfileSyncServiceHarness::ProfileSyncServiceHarness(
       timestamp_match_partner_(NULL),
       username_(username),
       password_(password),
+      expected_notifications_enabled_(expected_notifications_enabled),
       profile_debug_name_(profile->GetDebugName()) {
   if (IsSyncAlreadySetup()) {
     service_ = profile_->GetProfileSyncService();
@@ -120,7 +122,9 @@ ProfileSyncServiceHarness* ProfileSyncServiceHarness::CreateAndAttach(
     NOTREACHED() << "Profile has never signed into sync.";
     return NULL;
   }
-  return new ProfileSyncServiceHarness(profile, "", "");
+  return new ProfileSyncServiceHarness(
+      profile, "", "",
+      /* expected_notifications_enabled */ true);
 }
 
 void ProfileSyncServiceHarness::SetCredentials(const std::string& username,
@@ -616,7 +620,8 @@ bool ProfileSyncServiceHarness::IsSynced() {
   bool is_synced = snap &&
       snap->num_blocking_conflicting_updates == 0 &&
       ServiceIsPushingChanges() &&
-      GetStatus().notifications_enabled &&
+      (GetStatus().notifications_enabled ==
+       expected_notifications_enabled_) &&
       !service()->HasUnsyncedItems() &&
       !snap->has_more_to_sync &&
       snap->unsynced_count == 0 &&
diff --git a/chrome/browser/sync/profile_sync_service_harness.h b/chrome/browser/sync/profile_sync_service_harness.h
index 1cdeb60..1179bce 100644
--- a/chrome/browser/sync/profile_sync_service_harness.h
+++ b/chrome/browser/sync/profile_sync_service_harness.h
@@ -32,7 +32,8 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver {
  public:
   ProfileSyncServiceHarness(Profile* profile,
                             const std::string& username,
-                            const std::string& password);
+                            const std::string& password,
+                            bool expected_notifications_enabled);
 
   virtual ~ProfileSyncServiceHarness();
 
@@ -271,6 +272,9 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver {
   std::string username_;
   std::string password_;
 
+  // The expected value of GetStatus().notifications_enabled.
+  bool expected_notifications_enabled_;
+
   // Used for logging.
   const std::string profile_debug_name_;
 
diff --git a/chrome/test/live_sync/live_sync_test.cc b/chrome/test/live_sync/live_sync_test.cc
index 61c6b2c..b258d1c 100644
--- a/chrome/test/live_sync/live_sync_test.cc
+++ b/chrome/test/live_sync/live_sync_test.cc
@@ -112,7 +112,8 @@ LiveSyncTest::LiveSyncTest(TestType test_type)
       server_type_(SERVER_TYPE_UNDECIDED),
       num_clients_(-1),
       use_verifier_(true),
-      test_server_handle_(base::kNullProcessHandle) {
+      test_server_handle_(base::kNullProcessHandle),
+      enable_notifications_(true) {
   InProcessBrowserTest::set_show_window(true);
   sync_datatype_helper::AssociateWithTest(this);
   switch (test_type_) {
@@ -270,7 +271,8 @@ bool LiveSyncTest::SetupClients() {
         base::StringPrintf(FILE_PATH_LITERAL("Profile%d"), i)));
     EXPECT_FALSE(GetProfile(i) == NULL) << "GetProfile(" << i << ") failed.";
     clients_.push_back(
-        new ProfileSyncServiceHarness(GetProfile(i), username_, password_));
+        new ProfileSyncServiceHarness(GetProfile(i), username_, password_,
+                                      enable_notifications_));
     EXPECT_FALSE(GetClient(i) == NULL) << "GetClient(" << i << ") failed.";
     ui_test_utils::WaitForBookmarkModelToLoad(
         GetProfile(i)->GetBookmarkModel());
@@ -380,25 +382,27 @@ bool LiveSyncTest::SetUpLocalPythonTestServer() {
   cl->AppendSwitchASCII(switches::kSyncServiceURL, sync_service_url);
   VLOG(1) << "Started local python test server at " << sync_service_url;
 
-  int xmpp_port = 0;
-  if (!sync_server_.server_data().GetInteger("xmpp_port", &xmpp_port)) {
-    LOG(ERROR) << "Could not find valid xmpp_port value";
-    return false;
-  }
-  if ((xmpp_port <= 0) || (xmpp_port > kuint16max)) {
-    LOG(ERROR) << "Invalid xmpp port: " << xmpp_port;
-    return false;
-  }
+  if (enable_notifications_) {
+    int xmpp_port = 0;
+    if (!sync_server_.server_data().GetInteger("xmpp_port", &xmpp_port)) {
+      LOG(ERROR) << "Could not find valid xmpp_port value";
+      return false;
+    }
+    if ((xmpp_port <= 0) || (xmpp_port > kuint16max)) {
+      LOG(ERROR) << "Invalid xmpp port: " << xmpp_port;
+      return false;
+    }
 
-  net::HostPortPair xmpp_host_port_pair(sync_server_.host_port_pair());
-  xmpp_host_port_pair.set_port(xmpp_port);
-  xmpp_port_.reset(new net::ScopedPortException(xmpp_port));
+    net::HostPortPair xmpp_host_port_pair(sync_server_.host_port_pair());
+    xmpp_host_port_pair.set_port(xmpp_port);
+    xmpp_port_.reset(new net::ScopedPortException(xmpp_port));
 
-  if (!cl->HasSwitch(switches::kSyncNotificationHost)) {
-    cl->AppendSwitchASCII(switches::kSyncNotificationHost,
-                          xmpp_host_port_pair.ToString());
-    // The local XMPP server only supports insecure connections.
-    cl->AppendSwitch(switches::kSyncAllowInsecureXmppConnection);
+    if (!cl->HasSwitch(switches::kSyncNotificationHost)) {
+      cl->AppendSwitchASCII(switches::kSyncNotificationHost,
+                            xmpp_host_port_pair.ToString());
+      // The local XMPP server only supports insecure connections.
+      cl->AppendSwitch(switches::kSyncAllowInsecureXmppConnection);
+    }
   }
 
   return true;
@@ -489,6 +493,15 @@ void LiveSyncTest::DisableNetwork(Profile* profile) {
   net::NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
 }
 
+void LiveSyncTest::DisableNotifications() {
+  // TODO(akalin): It would be better to assert that the test server
+  // hasn't been started yet.  That would require adding an
+  // IsStarted() method to TestServer.
+  ASSERT_TRUE(profiles_.empty());
+  ASSERT_TRUE(clients_.empty());
+  enable_notifications_ = false;
+}
+
 bool LiveSyncTest::AwaitQuiescence() {
   return ProfileSyncServiceHarness::AwaitQuiescence(clients());
 }
@@ -512,24 +525,24 @@ void LiveSyncTest::TriggerMigrationDoneError(
     joiner = '&';
   }
   ui_test_utils::NavigateToURL(browser(), sync_server_.GetURL(path));
-  ASSERT_EQ(ASCIIToUTF16("Migration: 200"),
-            browser()->GetSelectedTabContents()->GetTitle());
+  ASSERT_EQ("Migration: 200",
+            UTF16ToASCII(browser()->GetSelectedTabContents()->GetTitle()));
 }
 
 void LiveSyncTest::TriggerBirthdayError() {
   ASSERT_TRUE(ServerSupportsErrorTriggering());
   std::string path = "chromiumsync/birthdayerror";
   ui_test_utils::NavigateToURL(browser(), sync_server_.GetURL(path));
-  ASSERT_EQ(ASCIIToUTF16("Birthday error"),
-            browser()->GetSelectedTabContents()->GetTitle());
+  ASSERT_EQ("Birthday error",
+            UTF16ToASCII(browser()->GetSelectedTabContents()->GetTitle()));
 }
 
 void LiveSyncTest::TriggerTransientError() {
   ASSERT_TRUE(ServerSupportsErrorTriggering());
   std::string path = "chromiumsync/transienterror";
   ui_test_utils::NavigateToURL(browser(), sync_server_.GetURL(path));
-  ASSERT_EQ(ASCIIToUTF16("Transient error"),
-            browser()->GetSelectedTabContents()->GetTitle());
+  ASSERT_EQ("Transient error",
+            UTF16ToASCII(browser()->GetSelectedTabContents()->GetTitle()));
 }
 
 void LiveSyncTest::SetProxyConfig(net::URLRequestContextGetter* context_getter,
diff --git a/chrome/test/live_sync/live_sync_test.h b/chrome/test/live_sync/live_sync_test.h
index fc5e008..a9c518e 100644
--- a/chrome/test/live_sync/live_sync_test.h
+++ b/chrome/test/live_sync/live_sync_test.h
@@ -126,6 +126,11 @@ class LiveSyncTest : public InProcessBrowserTest {
   // Disable outgoing network connections for the given profile.
   virtual void DisableNetwork(Profile* profile);
 
+  // Disable notifications for the current test.  Must be called
+  // before the test server is started (i.e., before either of
+  // SetupSync() or SetupClients() is called).
+  void DisableNotifications();
+
   // Blocks until all sync clients have completed their mutual sync cycles.
   // Returns true if a quiescent state was successfully reached.
   bool AwaitQuiescence();
@@ -266,6 +271,9 @@ class LiveSyncTest : public InProcessBrowserTest {
   // URLFetcher factory used to contact sync server.
   scoped_ptr<URLFetcherFactory> integration_factory_;
 
+  // Whether or not to use notifications for the current test.
+  bool enable_notifications_;
+
   DISALLOW_COPY_AND_ASSIGN(LiveSyncTest);
 };
 
diff --git a/chrome/test/live_sync/migration_errors_test.cc b/chrome/test/live_sync/migration_errors_test.cc
index 6fbeb48..6626ea2 100644
--- a/chrome/test/live_sync/migration_errors_test.cc
+++ b/chrome/test/live_sync/migration_errors_test.cc
@@ -19,6 +19,109 @@ using bookmarks_helper::IndexedURLTitle;
 using preferences_helper::BooleanPrefMatches;
 using preferences_helper::ChangeBooleanPref;
 
+// Tests to make sure that the migration cycle works properly,
+// i.e. doesn't stall.
+
+class MigrationCycleTest : public LiveSyncTest {
+ public:
+  MigrationCycleTest() : LiveSyncTest(SINGLE_CLIENT) {}
+  virtual ~MigrationCycleTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MigrationCycleTest);
+};
+
+IN_PROC_BROWSER_TEST_F(MigrationCycleTest, PrefsOnly) {
+  if (!ServerSupportsErrorTriggering()) {
+    LOG(WARNING) << "Test skipped in this server environment.";
+    return;
+  }
+
+  DisableNotifications();
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  // Phase 1: Trigger a preference migration on the server.
+  syncable::ModelTypeSet migrate_types;
+  migrate_types.insert(syncable::PREFERENCES);
+  TriggerMigrationDoneError(migrate_types);
+
+  // Phase 2: Modify a pref (to trigger migration) and wait for a sync
+  // cycle.
+  // TODO(akalin): Shouldn't need to wait for full sync cycle; see
+  // 93167.
+  ASSERT_TRUE(BooleanPrefMatches(prefs::kShowHomeButton));
+  ChangeBooleanPref(0, prefs::kShowHomeButton);
+  ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration"));
+}
+
+// TODO(akalin): Fails due to http://crbug.com/92928.
+IN_PROC_BROWSER_TEST_F(MigrationCycleTest, FAILS_PrefsNigori) {
+  if (!ServerSupportsErrorTriggering()) {
+    LOG(WARNING) << "Test skipped in this server environment.";
+    return;
+  }
+
+  DisableNotifications();
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  // Phase 1: Trigger a preference and nigori migration on the server.
+  {
+    syncable::ModelTypeSet migrate_types;
+    migrate_types.insert(syncable::PREFERENCES);
+    TriggerMigrationDoneError(migrate_types);
+  }
+  {
+    syncable::ModelTypeSet migrate_types;
+    migrate_types.insert(syncable::NIGORI);
+    TriggerMigrationDoneError(migrate_types);
+  }
+
+  // Phase 2: Modify a pref (to trigger migration) and wait for a sync
+  // cycle.
+  // TODO(akalin): Shouldn't need to wait for full sync cycle; see
+  // 93167.
+  ASSERT_TRUE(BooleanPrefMatches(prefs::kShowHomeButton));
+  ChangeBooleanPref(0, prefs::kShowHomeButton);
+  ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration"));
+}
+
+// TODO(akalin): Fails due to http://crbug.com/92928.
+IN_PROC_BROWSER_TEST_F(MigrationCycleTest, FAILS_BookmarksPrefs) {
+  if (!ServerSupportsErrorTriggering()) {
+    LOG(WARNING) << "Test skipped in this server environment.";
+    return;
+  }
+
+  DisableNotifications();
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  // Phase 1: Trigger a bookmark and preference migration on the
+  // server.
+  {
+    syncable::ModelTypeSet migrate_types;
+    migrate_types.insert(syncable::BOOKMARKS);
+    TriggerMigrationDoneError(migrate_types);
+  }
+  {
+    syncable::ModelTypeSet migrate_types;
+    migrate_types.insert(syncable::PREFERENCES);
+    TriggerMigrationDoneError(migrate_types);
+  }
+
+  // Phase 2: Modify a bookmark (to trigger migration) and wait for a
+  // sync cycle.
+  // TODO(akalin): Shouldn't need to wait for full sync cycle; see
+  // 93167.
+  ASSERT_TRUE(AddURL(0, IndexedURLTitle(0), GURL(IndexedURL(0))) != NULL);
+  ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration"));
+}
+
+// TODO(akalin): Add tests where the migration trigger is a poll or a
+// nudge from notifications.
+
 class MigrationErrorsTest : public LiveSyncTest {
  public:
   MigrationErrorsTest() : LiveSyncTest(TWO_CLIENT) {}