ProfileSyncServiceTest refactoring

Move following tests:
  TestStartupWithOldSyncData
  FailToOpenDatabase
  FailToDownloadControlTypes

Remove redundant parameters from StartSyncServiceAndSetInitialSyncEnded
and remove StartSyncService function.

TestStartupWithOldSyncData goes to SyncBackendHost tests. It verifies that SyncBackendHost cleans Sync Data directory before initialization.

FailToOpenDatabase goes to SyncManagerImpl test. It verifies that if SyncManager fails to open database it reports initialization failure.

FailToDownloadControlTypes is already covered by SyncBackendHostTest.SilentlyFailToDownloadControlTypes

BUG=312994

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

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

chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <cstddef>
 
+#include "base/file_util.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ -687,6 +688,21 @@ TEST_F(SyncBackendHostTest, DownloadControlTypesRestart) {
             fake_manager_->GetAndResetConfigureReason());
 }
 
+// It is SyncBackendHostCore responsibility to cleanup Sync Data folder if sync
+// setup hasn't been completed. This test ensures that cleanup happens.
+TEST_F(SyncBackendHostTest, TestStartupWithOldSyncData) {
+  const char* nonsense = "slon";
+  base::FilePath temp_directory =
+      profile_->GetPath().AppendASCII("Sync Data");
+  base::FilePath sync_file = temp_directory.AppendASCII("SyncData.sqlite3");
+  ASSERT_TRUE(file_util::CreateDirectory(temp_directory));
+  ASSERT_NE(-1, file_util::WriteFile(sync_file, nonsense, strlen(nonsense)));
+
+  InitializeBackend(true);
+
+  EXPECT_FALSE(base::PathExists(sync_file));
+}
+
 }  // namespace
 
 }  // namespace browser_sync

chrome/browser/sync/profile_sync_service_startup_unittest.cc

@@ -2,8 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "testing/gtest/include/gtest/gtest.h"
-
+#include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -32,6 +31,7 @@
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 using browser_sync::DataTypeManager;
 using browser_sync::DataTypeManagerMock;

chrome/browser/sync/profile_sync_service_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
-#include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/values.h"
@@ -86,19 +85,7 @@ class ProfileSyncServiceTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
-  // TODO(akalin): Refactor the StartSyncService*() functions below.
-
-  void StartSyncService() {
-    StartSyncServiceAndSetInitialSyncEnded(
-        true, true, false, true, syncer::STORAGE_IN_MEMORY);
-  }
-
-  void StartSyncServiceAndSetInitialSyncEnded(
-      bool set_initial_sync_ended,
-      bool issue_auth_token,
-      bool synchronous_sync_configuration,
-      bool sync_setup_completed,
-      syncer::StorageOption storage_option) {
+  void StartSyncServiceAndSetInitialSyncEnded() {
     if (service_)
       return;
 
@@ -116,23 +103,12 @@ class ProfileSyncServiceTest : public testing::Test {
         oauth2_token_service,
         ProfileSyncService::AUTO_START,
         true));
-    if (!set_initial_sync_ended)
-      service_->dont_set_initial_sync_ended_on_init();
-
-    if (synchronous_sync_configuration)
-      service_->set_synchronous_sync_configuration();
 
-    service_->set_storage_option(storage_option);
-    if (!sync_setup_completed)
-      profile_->GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false);
 
     // Register the bookmark data type.
     ON_CALL(*factory, CreateDataTypeManager(_, _, _, _, _, _)).
         WillByDefault(ReturnNewDataTypeManager());
 
-    if (issue_auth_token)
-      IssueTestTokens();
-
     service_->Initialize();
   }
 
@@ -453,7 +429,8 @@ TEST_F(ProfileSyncServiceSimpleTest, EnableSyncAndSignOut) {
 #endif  // !defined(OS_CHROMEOS)
 
 TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
-  StartSyncService();
+  StartSyncServiceAndSetInitialSyncEnded();
+  IssueTestTokens();
   EXPECT_TRUE(service_->sync_initialized());
   EXPECT_TRUE(service_->GetBackendForTest() != NULL);
 
@@ -466,8 +443,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerHandlersDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true,
-                                                  syncer::STORAGE_IN_MEMORY);
+  StartSyncServiceAndSetInitialSyncEnded();
 
   StrictMock<syncer::MockJsEventHandler> event_handler;
   EXPECT_CALL(event_handler, HandleJsEvent(_, _)).Times(AtLeast(1));
@@ -486,7 +462,8 @@ TEST_F(ProfileSyncServiceTest,
 }
 
 TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) {
-  StartSyncService();
+  StartSyncServiceAndSetInitialSyncEnded();
+  IssueTestTokens();
   WaitForBackendInitDone();
 
   StrictMock<syncer::MockJsReplyHandler> reply_handler;
@@ -519,8 +496,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerProcessJsMessageBasicDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true,
-                                                  syncer::STORAGE_IN_MEMORY);
+  StartSyncServiceAndSetInitialSyncEnded();
 
   StrictMock<syncer::MockJsReplyHandler> reply_handler;
 
@@ -548,76 +524,9 @@ TEST_F(ProfileSyncServiceTest,
   done.Wait();
 }
 
-// Make sure that things still work if sync is not enabled, but some old sync
-// databases are lingering in the "Sync Data" folder.
-TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
-  const char* nonsense1 = "reginald";
-  const char* nonsense2 = "beartato";
-  const char* nonsense3 = "harrison";
-  base::FilePath temp_directory =
-      profile_->GetPath().AppendASCII("Sync Data");
-  base::FilePath sync_file1 =
-      temp_directory.AppendASCII("BookmarkSyncSettings.sqlite3");
-  base::FilePath sync_file2 = temp_directory.AppendASCII("SyncData.sqlite3");
-  base::FilePath sync_file3 = temp_directory.AppendASCII("nonsense_file");
-  ASSERT_TRUE(file_util::CreateDirectory(temp_directory));
-  ASSERT_NE(-1,
-            file_util::WriteFile(sync_file1, nonsense1, strlen(nonsense1)));
-  ASSERT_NE(-1,
-            file_util::WriteFile(sync_file2, nonsense2, strlen(nonsense2)));
-  ASSERT_NE(-1,
-            file_util::WriteFile(sync_file3, nonsense3, strlen(nonsense3)));
-
-  StartSyncServiceAndSetInitialSyncEnded(true, false, true, false,
-                                                  syncer::STORAGE_ON_DISK);
-  EXPECT_FALSE(service_->HasSyncSetupCompleted());
-  EXPECT_FALSE(service_->sync_initialized());
-
-  // Since we're doing synchronous initialization, backend should be
-  // initialized by this call.
-  IssueTestTokens();
-
-  // Stop the service so we can read the new Sync Data files that were
-  // created.
-  service_->Shutdown();
-  service_.reset();
-
-  // This file should have been deleted when the whole directory was nuked.
-  ASSERT_FALSE(base::PathExists(sync_file3));
-  ASSERT_FALSE(base::PathExists(sync_file1));
-
-  // This will still exist, but the text should have changed.
-  ASSERT_TRUE(base::PathExists(sync_file2));
-  std::string file2text;
-  ASSERT_TRUE(base::ReadFileToString(sync_file2, &file2text));
-  ASSERT_NE(file2text.compare(nonsense2), 0);
-}
-
-// Simulates a scenario where a database is corrupted and it is impossible to
-// recreate it.  This test is useful mainly when it is run under valgrind.  Its
-// expectations are not very interesting.
-TEST_F(ProfileSyncServiceTest, FailToOpenDatabase) {
-  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true,
-                                                  syncer::STORAGE_INVALID);
-
-  // The backend is not ready.  Ensure the PSS knows this.
-  EXPECT_FALSE(service_->sync_initialized());
-}
-
-// This setup will allow the database to exist, but leave it empty.  The attempt
-// to download control types will silently fail (no downloads have any effect in
-// these tests).  The sync_backend_host will notice this and inform the profile
-// sync service of the failure to initialize the backed.
-TEST_F(ProfileSyncServiceTest, FailToDownloadControlTypes) {
-  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true,
-                                                  syncer::STORAGE_IN_MEMORY);
-
-  // The backend is not ready.  Ensure the PSS knows this.
-  EXPECT_FALSE(service_->sync_initialized());
-}
-
 TEST_F(ProfileSyncServiceTest, GetSyncTokenStatus) {
-  StartSyncService();
+  StartSyncServiceAndSetInitialSyncEnded();
+  IssueTestTokens();
   ProfileSyncService::SyncTokenStatus token_status =
       service_->GetSyncTokenStatus();
   EXPECT_EQ(syncer::CONNECTION_NOT_ATTEMPTED, token_status.connection_status);

sync/internal_api/sync_manager_impl_unittest.cc

@@ -807,7 +807,8 @@ class SyncManagerTest : public testing::Test,
 
     sync_manager_.AddObserver(&manager_observer_);
     EXPECT_CALL(manager_observer_, OnInitializationComplete(_, _, _, _)).
-        WillOnce(SaveArg<0>(&js_backend_));
+        WillOnce(DoAll(SaveArg<0>(&js_backend_),
+            SaveArg<2>(&initialization_succeeded_)));
 
     EXPECT_FALSE(js_backend_.IsInitialized());
 
@@ -841,11 +842,13 @@ class SyncManagerTest : public testing::Test,
 
     EXPECT_TRUE(js_backend_.IsInitialized());
 
+    if (initialization_succeeded_) {
       for (ModelSafeRoutingInfo::iterator i = routing_info.begin();
            i != routing_info.end(); ++i) {
         type_roots_[i->first] = MakeServerNodeForType(
             sync_manager_.GetUserShare(), i->first);
       }
+    }
     PumpLoop();
   }
 
@@ -1018,6 +1021,7 @@ class SyncManagerTest : public testing::Test,
   SyncManagerImpl sync_manager_;
   CancelationSignal cancelation_signal_;
   WeakHandle<JsBackend> js_backend_;
+  bool initialization_succeeded_;
   StrictMock<SyncManagerObserverMock> manager_observer_;
   StrictMock<SyncEncryptionHandlerObserverMock> encryption_observer_;
   InternalComponentsFactory::Switches switches_;
@@ -3482,4 +3486,28 @@ TEST_F(SyncManagerChangeProcessingTest, DeletionsAndChanges) {
   EXPECT_LT(folder_b_pos, folder_a_pos);
 }
 
+// During initialization SyncManagerImpl loads sqlite database. If it fails to
+// do so it should fail initialization. This test verifies this behavior.
+// Test reuses SyncManagerImpl initialization from SyncManagerTest but overrides
+// InternalComponentsFactory to return DirectoryBackingStore that always fails
+// to load.
+class SyncManagerInitInvalidStorageTest : public SyncManagerTest {
+ public:
+  SyncManagerInitInvalidStorageTest() {
+  }
+
+  virtual InternalComponentsFactory* GetFactory() OVERRIDE {
+    return new TestInternalComponentsFactory(GetSwitches(), STORAGE_INVALID);
+  }
+};
+
+// SyncManagerInitInvalidStorageTest::GetFactory will return
+// DirectoryBackingStore that ensures that SyncManagerImpl::OpenDirectory fails.
+// SyncManagerImpl initialization is done in SyncManagerTest::SetUp. This test's
+// task is to ensure that SyncManagerImpl reported initialization failure in
+// OnInitializationComplete callback.
+TEST_F(SyncManagerInitInvalidStorageTest, FailToOpenDatabase) {
+  EXPECT_FALSE(initialization_succeeded_);
+}
+
 }  // namespace