Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

chrome/browser/sync/glue/sync_backend_host_unittest.cc

Index: chrome/browser/sync/glue/sync_backend_host_unittest.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
index 14774a4568c71b0aca9d7a5216047e0c390b546b..075726b8c67664c9cde6f7fb7bf58c6001eb95c5 100644
--- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc
+++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
@@ -8,6 +8,8 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/test_timeouts.h"
 #include "chrome/browser/sync/invalidations/invalidator_storage.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/test/base/testing_profile.h"
@@ -16,6 +18,8 @@
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
+#include "sync/internal_api/public/sync_manager_factory.h"
+#include "sync/internal_api/public/test/fake_sync_manager.h"
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/sync_protocol_error.h"
@@ -24,11 +28,27 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserThread;
+using syncer::FakeSyncManager;
+using syncer::SyncManager;
+using ::testing::InvokeWithoutArgs;
+using ::testing::_;
 
 namespace browser_sync {
 
 namespace {
 
+ACTION_P(Signal, event) {
+  event->Signal();
+}
+
+void SignalEvent(base::WaitableEvent* event) {
+  event->Signal();
+}
+
+static void QuitMessageLoop() {
+  MessageLoop::current()->Quit();
+}
+
 class MockSyncFrontend : public SyncFrontend {
  public:
   virtual ~MockSyncFrontend() {}
@@ -56,21 +76,103 @@ class MockSyncFrontend : public SyncFrontend {
   MOCK_METHOD0(OnSyncConfigureRetry, void());
 };
 
-}  // namespace
+class FakeSyncManagerFactory : public syncer::SyncManagerFactory {
+ public:
+  FakeSyncManagerFactory() : manager_(NULL) {}
+  virtual ~FakeSyncManagerFactory() {}
+
+  // Takes ownership of |manager|.
+  void SetSyncManager(FakeSyncManager* manager) {
+    DCHECK(!manager_.get());
+    manager_.reset(manager);
+  }
+
+  // Passes ownership of |manager_|.
+  // SyncManagerFactory implementation.
+  virtual scoped_ptr<SyncManager> CreateSyncManager(std::string name) OVERRIDE {
+    DCHECK(manager_.get());
+    return manager_.Pass();
+  }
+
+ private:
+  scoped_ptr<SyncManager> manager_;
+};
+
+class TestSyncBackendHost : public SyncBackendHost {
+ public:
+  TestSyncBackendHost(
+      const std::string& name,
+      Profile* profile,
+      const base::WeakPtr<SyncPrefs>& sync_prefs,
+      const base::WeakPtr<InvalidatorStorage>& invalidator_storage)
+    : SyncBackendHost(name, profile, sync_prefs, invalidator_storage) {
+    StartSyncThread();
+  }
+  virtual ~TestSyncBackendHost() {}
+
+  void PumpSyncLoop() {

[rlarocque, 2012/07/04 06:39:35]

I'm amazed that there isn't a better way to do this.

Would calling MessageLoop::Quit() not work?  I know it won't help us if one of
the tasks on that loop got stuck, but I think that's not really the test's
responsibility.  We can handle that outside the tests, with a wrapper script
that kills the test execution if it takes too long.

[Nicolas Zea, 2012/07/09 18:40:45]

How would MessageLoop::Quit help? The issue is that we don't know how long the
posted tasks (and possibly any tasks they post) would take to complete.
MessageLoop quit would have the same effect as this really, except we wouldn't
know if we timed out or not.

More importantly, since this message loop is in fact part of a thread, we
shouldn't be calling quit on it anyways, but using the thread's stop method.
DCHECK's are hit otherwise.

[rlarocque, 2012/07/09 19:22:58]

My point is that it shouldn't be the test's responsibility to ensure that it
terminates in a reasonable amount of time.  We don't bother implementing these
checks in single-threaded tests, why should multi-threaded tests be any
different?

[Nicolas Zea, 2012/07/09 19:42:50]

We do these kind of checks anywhere a single action has the possibility of
failing without any noticeable side effect. The test framework does have timeout
functionality, but this is useful for limiting the amount of time a single
action takes.

The benefit is that we can fail faster in the cases where a bug is introduced,
and know that the failure comes from this particular sport.  The alternative is
test timeouts, which manifest as crashes and don't tell us where they came from.

It's not necessarily true we never do it in single threaded scenarios (I believe
some of the tests that rely on i/o do this too). But yeah, multi threaded
scenarios are more prone to this, so use this framework.

+    DCHECK(sync_loop());
+    base::WaitableEvent done(true, false);
+    sync_loop()->PostTask(FROM_HERE, base::Bind(&SignalEvent, &done));
+    done.TimedWait(base::TimeDelta::FromMilliseconds(
+        TestTimeouts::action_timeout_ms()));
+    if (!done.IsSignaled())
+      FAIL() << "Timed out waiting for sync loop.";
+  }
+
+  void PostToSyncLoop(const base::Closure& closure) {
+    DCHECK(sync_loop());
+    sync_loop()->PostTask(FROM_HERE, closure);
+  }
+};
 
 class SyncBackendHostTest : public testing::Test {
  protected:
   SyncBackendHostTest()
       : ui_thread_(BrowserThread::UI, &ui_loop_),
-        io_thread_(BrowserThread::IO) {}
+        io_thread_(BrowserThread::IO),
+        fake_manager_(NULL) {}
 
   virtual ~SyncBackendHostTest() {}
 
-  virtual void SetUp() {
+  virtual void SetUp() OVERRIDE {
     io_thread_.StartIOThread();
+    profile_.reset(new TestingProfile());
+    profile_->CreateRequestContext();
+    sync_prefs_.reset(new SyncPrefs(profile_->GetPrefs()));
+    invalidator_storage_.reset(new InvalidatorStorage(profile_->GetPrefs()));
+    backend_.reset(new TestSyncBackendHost(
+        profile_->GetDebugName(),
+        profile_.get(),
+        sync_prefs_->AsWeakPtr(),
+        invalidator_storage_->AsWeakPtr()));
+    credentials_.email = "user@example.com";
+    credentials_.sync_token = "sync_token";
+
+    // The sync manager must be created on the sync loop in order to allow
+    // thread safety enforcement.
+    PostToSyncLoop(base::Bind(&SyncBackendHostTest::BuildSyncManager,
+                              base::Unretained(this)));
+    PumpSyncLoop();
+    fake_sync_manager_factory_.SetSyncManager(fake_manager_);
+
+    // NOTE: We can't include Passwords or Typed URLs due to the Sync Backend
+    // Registrar removing them if it can't find their model workers.
+    enabled_types_.Put(syncer::BOOKMARKS);
+    enabled_types_.Put(syncer::NIGORI);
+    enabled_types_.Put(syncer::PREFERENCES);
+    enabled_types_.Put(syncer::SESSIONS);
+    enabled_types_.Put(syncer::SEARCH_ENGINES);
+    enabled_types_.Put(syncer::AUTOFILL);
   }
 
-  virtual void TearDown() {
+  virtual void TearDown() OVERRIDE {
+    backend_->StopSyncingForShutdown();
+    backend_->Shutdown(false);
+    backend_.reset();
+    sync_prefs_.reset();
+    invalidator_storage_.reset();
+    profile_.reset();
     // Pump messages posted by the sync core thread (which may end up
     // posting on the IO thread).
     ui_loop_.RunAllPending();
@@ -79,44 +181,168 @@ class SyncBackendHostTest : public testing::Test {
     ui_loop_.RunAllPending();
   }
 
- private:
+  // Synchronously initializes the backend.
+  void InitializeBackend(syncer::ModelTypeSet enabled_types) {
+    EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, true)).
+        WillOnce(InvokeWithoutArgs(QuitMessageLoop));
+    backend_->Initialize(&mock_frontend_,
+                         syncer::WeakHandle<syncer::JsEventHandler>(),
+                         GURL(""),
+                         enabled_types,
+                         credentials_,
+                         true,
+                         &fake_sync_manager_factory_,
+                         &handler_,
+                         NULL);
+    ui_loop_.PostDelayedTask(
+        FROM_HERE,
+        ui_loop_.QuitClosure(),
+        base::TimeDelta::FromMilliseconds(TestTimeouts::action_timeout_ms()));
+    ui_loop_.Run();
+  }
+
+  // Synchronously configures the backend's datatypes.
+  void ConfigureDataTypes(syncer::ModelTypeSet types_to_add,
+                          syncer::ModelTypeSet types_to_remove,
+                          BackendDataTypeConfigurer::NigoriState nigori_state) {
+    backend_->ConfigureDataTypes(
+        syncer::CONFIGURE_REASON_RECONFIGURATION,
+        types_to_add,
+        types_to_remove,
+        nigori_state,
+        base::Bind(&SyncBackendHostTest::DownloadReady,
+                   base::Unretained(this)),
+        base::Bind(&SyncBackendHostTest::OnDownloadRetry,
+                   base::Unretained(this)));
+    ui_loop_.PostDelayedTask(
+        FROM_HERE,
+        ui_loop_.QuitClosure(),
+        base::TimeDelta::FromMilliseconds(TestTimeouts::action_timeout_ms()));
+    ui_loop_.Run();
+  }
+
+ protected:
+  // Note: should run on the sync thread.
+  void BuildSyncManager() {
+    DCHECK(!fake_manager_);
+    fake_manager_ = new FakeSyncManager("name");
+  }
+
+  void PumpSyncLoop() {
+    backend_->PumpSyncLoop();
+  }
+
+  void PostToSyncLoop(const base::Closure& closure) {
+    backend_->PostToSyncLoop(closure);
+  }
+
+  void DownloadReady(syncer::ModelTypeSet types) {
+    MessageLoop::current()->Quit();
+  }
+
+  void OnDownloadRetry() {
+    NOTIMPLEMENTED();
+  }
+
   MessageLoop ui_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread io_thread_;
+  MockSyncFrontend mock_frontend_;
+  syncer::SyncCredentials credentials_;
+  syncer::TestUnrecoverableErrorHandler handler_;
+  scoped_ptr<TestingProfile> profile_;
+  scoped_ptr<SyncPrefs> sync_prefs_;
+  scoped_ptr<InvalidatorStorage> invalidator_storage_;
+  scoped_ptr<TestSyncBackendHost> backend_;
+  FakeSyncManagerFactory fake_sync_manager_factory_;
+  FakeSyncManager* fake_manager_;
+  syncer::ModelTypeSet enabled_types_;
 };
 
+// Test basic initialization with no initial types (first time initialization).
+// Only the nigori should be configured.
 TEST_F(SyncBackendHostTest, InitShutdown) {
-  std::string k_mock_url = "http://www.example.com";
-  net::FakeURLFetcherFactory test_factory_;
-  test_factory_.SetFakeResponse(k_mock_url + "/time?command=get_time", "",
-      false);
-
-  TestingProfile profile;
-  profile.CreateRequestContext();
-
-  SyncPrefs sync_prefs(profile.GetPrefs());
-  InvalidatorStorage invalidator_storage(profile.GetPrefs());
-  SyncBackendHost backend(profile.GetDebugName(),
-                          &profile, sync_prefs.AsWeakPtr(),
-                          invalidator_storage.AsWeakPtr());
-
-  MockSyncFrontend mock_frontend;
-  syncer::SyncCredentials credentials;
-  credentials.email = "user@example.com";
-  credentials.sync_token = "sync_token";
-  syncer::TestUnrecoverableErrorHandler handler;
-  backend.Initialize(&mock_frontend,
-                     syncer::WeakHandle<syncer::JsEventHandler>(),
-                     GURL(k_mock_url),
+  InitializeBackend(syncer::ModelTypeSet());
+
+  EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Has(syncer::NIGORI));
+  EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      syncer::ModelTypeSet(syncer::NIGORI)).Empty());
+}
+
+// Test first time sync scenario. All types should be properly configured.
+TEST_F(SyncBackendHostTest, FirstTimeSync) {
+  InitializeBackend(syncer::ModelTypeSet());
+  ConfigureDataTypes(enabled_types_,
+                     syncer::ModelTypeSet(),
+                     BackendDataTypeConfigurer::WITH_NIGORI);
+
+  EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll(
+      enabled_types_));
+  EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_));
+  EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      enabled_types_).Empty());
+}
+
+// Test the restart after setting up sync scenario. No enabled types should be
+// downloaded or cleaned.
+TEST_F(SyncBackendHostTest, Restart) {
+  syncer::ModelTypeSet all_but_nigori = enabled_types_;
+  fake_manager_->set_progress_marker_types(
+      enabled_types_);
+  fake_manager_->set_initial_sync_ended_types(enabled_types_);
+  InitializeBackend(enabled_types_);
+  ConfigureDataTypes(enabled_types_,
+                     syncer::ModelTypeSet(),
+                     BackendDataTypeConfigurer::WITH_NIGORI);
+
+  EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Empty());
+  EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
+                           enabled_types_).Empty());
+  EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_));
+  EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      enabled_types_).Empty());
+}
+
+// Test a sync restart scenario where the nigori had never finished configuring.
+// The nigori should be cleaned up, then reconfigured properly.
+TEST_F(SyncBackendHostTest, PartialNigori) {
+  // Set sync manager behavior before passing it down. All types have progress
+  // markers, but nigori is missing initial sync ended.
+  syncer::ModelTypeSet all_but_nigori = enabled_types_;
+  all_but_nigori.Remove(syncer::NIGORI);
+  fake_manager_->set_progress_marker_types(
+      enabled_types_);
+  fake_manager_->set_initial_sync_ended_types(all_but_nigori);
+  InitializeBackend(enabled_types_);
+
+  // Nigori should have been cleaned up, then configured.
+  EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
+                           enabled_types_).
+      Equals(syncer::ModelTypeSet(syncer::NIGORI)));
+  EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
+      syncer::ModelTypeSet(syncer::NIGORI)));
+  EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Has(syncer::NIGORI));
+}
+
+// Test the behavior when we lose the sync db. Although we already have types
+// enabled, we should re-download all of them because we lost their data.
+TEST_F(SyncBackendHostTest, LostDB) {
+  // Don't set any progress marker or initial_sync_ended types before
+  // initializing.
+  InitializeBackend(enabled_types_);
+  ConfigureDataTypes(enabled_types_,
                      syncer::ModelTypeSet(),
-                     credentials,
-                     true,
-                     &handler,
-                     NULL);
-  backend.StopSyncingForShutdown();
-  backend.Shutdown(false);
+                     BackendDataTypeConfigurer::WITH_NIGORI);
+
+  EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll(
+      enabled_types_));
+  EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
+                           enabled_types_).Empty());
+  EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_));
+  EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      enabled_types_).Empty());
 }
 
-// TODO(akalin): Write more SyncBackendHost unit tests.
+}  // namespace
 
 }  // namespace browser_sync