Refactor some ProfileSyncServiceTests

chrome/browser/sync/profile_sync_service_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/invalidation/invalidation_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/token_service.h"
 #include "chrome/browser/signin/token_service_factory.h"
 #include "chrome/browser/sync/fake_oauth2_token_service.h"
 #include "chrome/browser/sync/glue/bookmark_data_type_controller.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
+#include "chrome/browser/sync/glue/sync_backend_host_mock.h"
 #include "chrome/browser/sync/profile_sync_components_factory_mock.h"
 #include "chrome/browser/sync/test_profile_sync_service.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "google/cacheinvalidation/include/types.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ skipping 17 matching lines @@
 using testing::Mock;
 using testing::Return;
 using testing::StrictMock;
 
 void SignalDone(base::WaitableEvent* done) {
   done->Signal();
 }
 
 class ProfileSyncServiceTest : public testing::Test {
  protected:
   ProfileSyncServiceTest()

[Nicolas Zea, 2013/11/13 21:06:45]

nit: this should have a virtual destructor btw

       : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD |
                        content::TestBrowserThreadBundle::REAL_FILE_THREAD |
                        content::TestBrowserThreadBundle::REAL_IO_THREAD) {
    }
 
-   virtual void SetUp() OVERRIDE {
+  virtual void SetUp() OVERRIDE {
     TestingProfile::Builder builder;
     builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(),
                               FakeOAuth2TokenService::BuildTokenService);
     profile_ = builder.Build().Pass();
     invalidation::InvalidationServiceFactory::GetInstance()->
         SetBuildOnlyFakeInvalidatorsForTest(true);
   }
 
   virtual void TearDown() OVERRIDE {
     // Kill the service before the profile.
@@ skipping 92 matching lines @@
       : service_(service), first_setup_in_progress_(false) {}
   virtual void OnStateChanged() OVERRIDE {
     first_setup_in_progress_ = service_->FirstSetupInProgress();
   }
   bool first_setup_in_progress() const { return first_setup_in_progress_; }
  private:
   ProfileSyncService* service_;
   bool first_setup_in_progress_;
 };
 
-TEST_F(ProfileSyncServiceTest, InitialState) {
-  SigninManagerBase* signin =
-      SigninManagerFactory::GetForProfile(profile_.get());
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
-  service_.reset(new TestProfileSyncService(
-      new ProfileSyncComponentsFactoryMock(),
-      profile_.get(),
-      signin,
-      oauth2_token_service,
-      ProfileSyncService::MANUAL_START,
-      true));
-  service_->Initialize();
-  EXPECT_TRUE(
-      service_->sync_service_url().spec() ==
-        ProfileSyncService::kSyncServerUrl ||
-      service_->sync_service_url().spec() ==
-        ProfileSyncService::kDevServerUrl);
-}
-
-// Tests that the sync service doesn't forget to notify observers about
-// setup state.
-TEST(ProfileSyncServiceTestBasic, SetupInProgress) {
-  ProfileSyncService service(
-      NULL, NULL, NULL, NULL, ProfileSyncService::MANUAL_START);
-  TestProfileSyncServiceObserver observer(&service);
-  service.AddObserver(&observer);
-  service.SetSetupInProgress(true);
+// A variant of the SyncBackendHostMock that won't automatically
+// call back when asked to initialized.  Allows us to test things
+// that could happen while backend init is in progress.
+class SyncBackendHostNoReturn : public SyncBackendHostMock {
+  virtual void Initialize(
+      SyncFrontend* frontend,
+      scoped_ptr<base::Thread> sync_thread,
+      const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
+      const GURL& service_url,
+      const syncer::SyncCredentials& credentials,
+      bool delete_sync_data_folder,
+      scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory,
+      scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler,
+      syncer::ReportUnrecoverableErrorFunction
+      report_unrecoverable_error_function) OVERRIDE {}
+};
+
+ACTION(ReturnNewSyncBackendHostMock) {
+  return new browser_sync::SyncBackendHostMock();
+}
+
+ACTION(ReturnNewSyncBackendHostNoReturn) {
+  return new browser_sync::SyncBackendHostNoReturn();
+}
+
+class ProfileSyncServiceSimpleTest : public ::testing::Test {

[Nicolas Zea, 2013/11/13 21:06:45]

nit: Comment about how this differs?

[rlarocque, 2013/11/13 22:28:25]

Added a comment that describes the code.

The plan is to remove the ProfileSyncServiceTest test class soon.  Pavel is
working to move some of the other tests.  I think we can refactor the
JsController tests, too.  Once that's done, there won't be anything left that
relies on the ProfileSyncServiceTest.

+ protected:
+  ProfileSyncServiceSimpleTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD |
+                       content::TestBrowserThreadBundle::REAL_FILE_THREAD |
+                       content::TestBrowserThreadBundle::REAL_IO_THREAD) {}
+  virtual ~ProfileSyncServiceSimpleTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    TestingProfile::Builder builder;
+
+    builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(),
+                              FakeOAuth2TokenService::BuildTokenService);
+    invalidation::InvalidationServiceFactory::GetInstance()->
+        SetBuildOnlyFakeInvalidatorsForTest(true);
+
+    profile_ = builder.Build().Pass();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    // Kill the service before the profile.
+    if (service_)
+      service_->Shutdown();
+
+    service_.reset();
+    profile_.reset();
+
+    // Pump messages posted by the sync thread (which may end up
+    // posting on the IO thread).
+    base::RunLoop().RunUntilIdle();
+    content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void IssueTestTokens() {
+    ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get())

[Nicolas Zea, 2013/11/13 21:06:45]

extract this into a standalone function both classes can use? (are there any
other duplicated functions this could be applied to as well?)

[rlarocque, 2013/11/13 22:28:25]

Since we plan to remove the ProfileSyncServiceTest soon, I don't think that's a
good idea.  I'm willing to live with a bit of code duplication for now.

+        ->UpdateCredentials("test", "oauth2_login_token");
+  }
+
+  void CreateService(ProfileSyncService::StartBehavior behavior) {
+    SigninManagerBase* signin =
+        SigninManagerFactory::GetForProfile(profile_.get());
+    signin->SetAuthenticatedUsername("test");
+    ProfileOAuth2TokenService* oauth2_token_service =
+        ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
+    components_factory_ = new StrictMock<ProfileSyncComponentsFactoryMock>();
+    service_.reset(new ProfileSyncService(
+        components_factory_,
+        profile_.get(),
+        signin,
+        oauth2_token_service,
+        behavior));
+  }
+
+  void ShutdownAndDeleteService() {
+    if (service_)
+      service_->Shutdown();
+    service_.reset();
+  }
+
+  void Initialize() {
+    service_->Initialize();
+  }
+
+  void ExpectDataTypeManagerCreation() {
+    EXPECT_CALL(*components_factory_, CreateDataTypeManager(_, _, _, _, _, _)).
+        WillOnce(ReturnNewDataTypeManager());
+  }
+
+  void ExpectSyncBackendHostCreation() {
+    EXPECT_CALL(*components_factory_, CreateSyncBackendHost(_, _, _)).
+        WillOnce(ReturnNewSyncBackendHostMock());
+  }
+
+  void PrepareDelayedInitSyncBackendHost() {
+    EXPECT_CALL(*components_factory_, CreateSyncBackendHost(_, _, _)).
+        WillOnce(ReturnNewSyncBackendHostNoReturn());
+  }
+
+  TestingProfile* profile() {
+    return profile_.get();
+  }
+
+  ProfileSyncService* service() {
+    return service_.get();
+  }
+
+  ProfileSyncComponentsFactoryMock* components_factory() {
+    return components_factory_;
+  }
+
+ private:
+  scoped_ptr<TestingProfile> profile_;
+  scoped_ptr<ProfileSyncService> service_;
+
+  // Pointer to the components factory.  Not owned.  May be null.
+  ProfileSyncComponentsFactoryMock* components_factory_;
+
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+// Verify that the server URLs are sane.
+TEST_F(ProfileSyncServiceSimpleTest, InitialState) {
+  CreateService(ProfileSyncService::AUTO_START);
+  Initialize();
+  const std::string& url = service()->sync_service_url().spec();
+  EXPECT_TRUE(url == ProfileSyncService::kSyncServerUrl ||
+              url == ProfileSyncService::kDevServerUrl);
+}
+
+// Verify that the SetSetupInProgress function call updates state
+// and notifies observers.
+TEST_F(ProfileSyncServiceSimpleTest, SetupInProgress) {
+  CreateService(ProfileSyncService::MANUAL_START);
+  Initialize();
+
+  TestProfileSyncServiceObserver observer(service());
+  service()->AddObserver(&observer);
+
+  service()->SetSetupInProgress(true);
   EXPECT_TRUE(observer.first_setup_in_progress());
-  service.SetSetupInProgress(false);
+  service()->SetSetupInProgress(false);
   EXPECT_FALSE(observer.first_setup_in_progress());
-  service.RemoveObserver(&observer);
-}
-
-TEST_F(ProfileSyncServiceTest, DisabledByPolicy) {
-  profile_->GetTestingPrefService()->SetManagedPref(
+
+  service()->RemoveObserver(&observer);
+}
+
+// Verify that disable by enterprise policy works.
+TEST_F(ProfileSyncServiceSimpleTest, DisabledByPolicy) {
+  profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kSyncManaged,
       Value::CreateBooleanValue(true));
-  SigninManagerBase* signin =
-      SigninManagerFactory::GetForProfile(profile_.get());
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
-  service_.reset(new TestProfileSyncService(
-      new ProfileSyncComponentsFactoryMock(),
-      profile_.get(),
-      signin,
-      oauth2_token_service,
-      ProfileSyncService::MANUAL_START,
-      true));
-  service_->Initialize();
-  EXPECT_TRUE(service_->IsManaged());
-}
-
-TEST_F(ProfileSyncServiceTest, AbortedByShutdown) {
-  SigninManagerBase* signin =
-      SigninManagerFactory::GetForProfile(profile_.get());
-  signin->SetAuthenticatedUsername("test");
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
-  ProfileSyncComponentsFactoryMock* factory =
-      new ProfileSyncComponentsFactoryMock();
-  service_.reset(new TestProfileSyncService(
-      factory,
-      profile_.get(),
-      signin,
-      oauth2_token_service,
-      ProfileSyncService::AUTO_START,
-      true));
-  EXPECT_CALL(*factory, CreateDataTypeManager(_, _, _, _, _, _)).Times(0);
-  EXPECT_CALL(*factory, CreateBookmarkSyncComponents(_, _)).
-      Times(0);
-  service_->RegisterDataTypeController(
-      new BookmarkDataTypeController(service_->factory(),
-                                     profile_.get(),
-                                     service_.get()));
-
-  service_->Initialize();
-  service_->Shutdown();
-  service_.reset();
-}
-
-TEST_F(ProfileSyncServiceTest, DisableAndEnableSyncTemporarily) {
-  SigninManagerBase* signin =
-      SigninManagerFactory::GetForProfile(profile_.get());
-  signin->SetAuthenticatedUsername("test");
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
-  ProfileSyncComponentsFactoryMock* factory =
-      new ProfileSyncComponentsFactoryMock();
-  service_.reset(new TestProfileSyncService(
-      factory,
-      profile_.get(),
-      signin,
-      oauth2_token_service,
-      ProfileSyncService::AUTO_START,
-      true));
-  // Register the bookmark data type.
-  EXPECT_CALL(*factory, CreateDataTypeManager(_, _, _, _, _, _)).
-      WillRepeatedly(ReturnNewDataTypeManager());
-
-  IssueTestTokens();
-
-  service_->Initialize();
-  EXPECT_TRUE(service_->sync_initialized());
-  EXPECT_TRUE(service_->GetBackendForTest() != NULL);
+  IssueTestTokens();
+  CreateService(ProfileSyncService::AUTO_START);
+  Initialize();
+  EXPECT_TRUE(service()->IsManaged());
+  EXPECT_FALSE(service()->sync_initialized());
+}
+
+// Verify a successful init.
+//
+// This test is complementary to the one above.  The DisabledByPolicy test could
+// return a false positive if there was something else (eg. lack of auth tokens)
+// that prevents sync startup.  This test ensures that not setting the
+// enterprise policy disable flag results in a successful init.
+TEST_F(ProfileSyncServiceSimpleTest, NotDisabledByPolicy) {

[Nicolas Zea, 2013/11/13 21:06:45]

Isn't this just a normal init test (which I agree we should have)? Perhaps it
makes sense to set the managed pref after initialization, and ensure the sync
service disables itself?

[rlarocque, 2013/11/13 22:28:25]

Yeah, it's pretty much a normal init test.  I added it and named it this way
when I realized that the old DisabledByPolicy was working in part because that
test didn't issue any auth tokens.  The disabled by policy functionality could
have been broken and that test would not have noticed.  I was hoping the name
would encourage people making future modifications to this file to keep it in
sync with the DisabledByPolicy test, so that it could act as a "control" for the
experiment.

Your idea is much better.  It's more likely to break if either the enterprise
policy doesn't work, or the test framework fails to take into account all the
prerequisites for backend init.

I've moved this test up a bit and renamed it to SuccessfulInitialization.  I've
also added a new one named DisabledByPolicyAfterInit.

+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kSyncManaged,
+      Value::CreateBooleanValue(false));
+  IssueTestTokens();
+  CreateService(ProfileSyncService::AUTO_START);
+  ExpectDataTypeManagerCreation();
+  ExpectSyncBackendHostCreation();
+  Initialize();
+  EXPECT_FALSE(service()->IsManaged());
+  EXPECT_TRUE(service()->sync_initialized());
+}
+
+// Exercies the ProfileSyncService's code paths related to getting shut down
+// before the backend initialize call returns.
+TEST_F(ProfileSyncServiceSimpleTest, AbortedByShutdown) {
+  CreateService(ProfileSyncService::AUTO_START);
+  PrepareDelayedInitSyncBackendHost();
+
+  IssueTestTokens();
+  Initialize();
+  EXPECT_FALSE(service()->sync_initialized());
+
+  ShutdownAndDeleteService();
+}
+
+TEST_F(ProfileSyncServiceSimpleTest, DisableAndEnableSyncTemporarily) {
+  CreateService(ProfileSyncService::AUTO_START);
+  IssueTestTokens();
+  ExpectDataTypeManagerCreation();
+  ExpectSyncBackendHostCreation();
+  Initialize();
+
+  EXPECT_TRUE(service()->sync_initialized());
+  EXPECT_FALSE(profile()->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
+
+  testing::Mock::VerifyAndClearExpectations(components_factory());
+
+  service()->StopAndSuppress();
+  EXPECT_FALSE(service()->sync_initialized());
+  EXPECT_TRUE(profile()->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
+
+  ExpectDataTypeManagerCreation();
+  ExpectSyncBackendHostCreation();

[Nicolas Zea, 2013/11/13 21:06:45]

test that a suppressed start doesn't initialize before unsuppresing?

[rlarocque, 2013/11/13 22:28:25]

I don't follow.  Are you suggesting there should be more assertions in this
test, or a separate test for the case where StopAndSuppress() is called before
Initialize()?

[Nicolas Zea, 2013/11/13 22:37:20]

More assertions. I.e, before you call UnsupressAndStart, verify that supressing
actually prevents startup.

+
+  service()->UnsuppressAndStart();
+  EXPECT_TRUE(service()->sync_initialized());
   EXPECT_FALSE(
-      profile_->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
-
-  service_->StopAndSuppress();
-  EXPECT_FALSE(service_->sync_initialized());
-  EXPECT_TRUE(
-      profile_->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
-
-  service_->UnsuppressAndStart();
-  EXPECT_TRUE(service_->sync_initialized());
-  EXPECT_FALSE(
-      profile_->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
+      profile()->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
 }
 
 // Certain ProfileSyncService tests don't apply to Chrome OS, for example
 // things that deal with concepts like "signing out" and policy.
 #if !defined (OS_CHROMEOS)
 
-TEST_F(ProfileSyncServiceTest, EnableSyncAndSignOut) {
-  SigninManager* signin =
-      SigninManagerFactory::GetForProfile(profile_.get());
-  signin->SetAuthenticatedUsername("test");
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get());
-  ProfileSyncComponentsFactoryMock* factory =
-      new ProfileSyncComponentsFactoryMock();
-  service_.reset(new TestProfileSyncService(
-      factory,
-      profile_.get(),
-      signin,
-      oauth2_token_service,
-      ProfileSyncService::AUTO_START,
-      true));
-  // Register the bookmark data type.
-  EXPECT_CALL(*factory, CreateDataTypeManager(_, _, _, _, _, _)).
-      WillRepeatedly(ReturnNewDataTypeManager());
-
-  IssueTestTokens();
-
-  service_->Initialize();
-  EXPECT_TRUE(service_->sync_initialized());
-  EXPECT_TRUE(service_->GetBackendForTest() != NULL);
-  EXPECT_FALSE(
-      profile_->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
-
-  signin->SignOut();
-  EXPECT_FALSE(service_->sync_initialized());
+TEST_F(ProfileSyncServiceSimpleTest, EnableSyncAndSignOut) {
+  CreateService(ProfileSyncService::AUTO_START);
+  ExpectDataTypeManagerCreation();
+  ExpectSyncBackendHostCreation();
+  IssueTestTokens();
+  Initialize();
+
+  EXPECT_TRUE(service()->sync_initialized());
+  EXPECT_FALSE(profile()->GetPrefs()->GetBoolean(prefs::kSyncSuppressStart));
+
+  SigninManagerFactory::GetForProfile(profile())->SignOut();
+  EXPECT_FALSE(service()->sync_initialized());
 }
 
 #endif  // !defined(OS_CHROMEOS)
 
 TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
   StartSyncService();
   EXPECT_TRUE(service_->sync_initialized());
   EXPECT_TRUE(service_->GetBackendForTest() != NULL);
 
   base::WeakPtr<syncer::JsController> js_controller =
@@ skipping 177 matching lines @@
   EXPECT_TRUE(token_status.next_token_request_time.is_null());
 
   // Simulate successful connection.
   service_->OnConnectionStatusChange(syncer::CONNECTION_OK);
   token_status = service_->GetSyncTokenStatus();
   EXPECT_EQ(syncer::CONNECTION_OK, token_status.connection_status);
 }
 
 }  // namespace
 }  // namespace browser_sync