Revert 238348 "Clean up TestProfileSyncService and related tests"

This CL appeared to be the cause of a link failure on Win dbg: http://goo.gl/PTFpFF

> Clean up TestProfileSyncService and related tests
> 
> This CL refactors many of the tests in profile_sync_service_unittest.cc.
> It continues the refactoring work begun in r233533, r235661, and
> r235854.
> 
> The JsController tests have been deleted.  There is not much point in
> testing the JsController here; it can be more easily tested on its own
> in sync_js_controller_uniittest.cc.  The SyncJsController unit tests
> have been updated so they now cover the few cases that were formerly
> only exercised in the ProfileSyncService unit tests.
> 
> It converts all remaining uncoverted tests from relying on the
> TestProfileSyncService to using a real ProfileSyncService with an
> injected backend.  The injected backend makes it easier to create the
> scenarios we want to test.  We can inject a specially crafted SBH rather
> than fiddling with "synchronous init" and "fail initial download" flags.
> 
> Since the TestProfileSyncService no longer needs to support the wide
> variety of test scenarios required by the tests in
> profile_sync_service_unittest.cc, we can greatly simplify its
> implementation.  Many of its parameters and associated code have been
> removed.
> 
> BUG=140354,312994
> 
> Review URL: https://codereview.chromium.org/67683005

TBR=rlarocque@chromium.org

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

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

chrome/browser/sync/profile_sync_service.cc

@@ -1735,6 +1735,12 @@ bool ProfileSyncService::IsCryptographerReady(
   return backend_.get() && backend_->IsCryptographerReady(trans);
 }
 
+SyncBackendHost* ProfileSyncService::GetBackendForTest() {
+  // We don't check |backend_initialized_|; we assume the test class
+  // knows what it's doing.
+  return backend_.get();
+}
+
 void ProfileSyncService::ConfigurePriorityDataTypes() {
   const syncer::ModelTypeSet priority_types =
       Intersection(GetPreferredDataTypes(), syncer::PriorityUserTypes());

chrome/browser/sync/profile_sync_service.h

@@ -700,6 +700,9 @@ class ProfileSyncService
   browser_sync::FaviconCache* GetFaviconCache();
 
  protected:
+  // Used by test classes that derive from ProfileSyncService.
+  virtual browser_sync::SyncBackendHost* GetBackendForTest();
+
   // Helper to configure the priority data types.
   void ConfigurePriorityDataTypes();
 

chrome/browser/sync/profile_sync_service_autofill_unittest.cc

@@ -21,7 +21,6 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/time/time.h"
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
@@ -39,7 +38,6 @@
 #include "chrome/browser/webdata/autocomplete_syncable_service.h"
 #include "chrome/browser/webdata/autofill_profile_syncable_service.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
-#include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/browser/autofill_test_utils.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill/core/browser/webdata/autofill_change.h"

chrome/browser/sync/profile_sync_service_password_unittest.cc

@@ -18,8 +18,6 @@
 #include "chrome/browser/password_manager/mock_password_store.h"
 #include "chrome/browser/password_manager/password_store.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
-#include "chrome/browser/signin/profile_oauth2_token_service.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
@@ -34,7 +32,6 @@
 #include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/sync/test_profile_sync_service.h"
 #include "chrome/common/pref_names.h"
-#include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/common/password_form.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/test/mock_notification_observer.h"
@@ -104,7 +101,8 @@ class PasswordTestProfileSyncService : public TestProfileSyncService {
                                profile,
                                signin,
                                oauth2_token_service,
-                               ProfileSyncService::AUTO_START) {}
+                               ProfileSyncService::AUTO_START,
+                               false) {}
 
   virtual ~PasswordTestProfileSyncService() {}
 

chrome/browser/sync/profile_sync_service_preference_unittest.cc

@@ -17,7 +17,6 @@
 #include "base/strings/string_piece.h"
 #include "chrome/browser/invalidation/invalidation_service_factory.h"
 #include "chrome/browser/prefs/pref_model_associator.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"

chrome/browser/sync/profile_sync_service_session_unittest.cc

@@ -19,8 +19,6 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/invalidation/invalidation_service_factory.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
-#include "chrome/browser/signin/profile_oauth2_token_service.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
@@ -86,12 +84,14 @@ class FakeProfileSyncService : public TestProfileSyncService {
       Profile* profile,
       SigninManagerBase* signin,
       ProfileOAuth2TokenService* oauth2_token_service,
-      ProfileSyncService::StartBehavior behavior)
+      ProfileSyncService::StartBehavior behavior,
+      bool synchronous_backend_initialization)
       : TestProfileSyncService(factory,
                                profile,
                                signin,
                                oauth2_token_service,
-                               behavior) {}
+                               behavior,
+                               synchronous_backend_initialization) {}
   virtual ~FakeProfileSyncService() {}
 
   virtual scoped_ptr<DeviceInfo> GetLocalDeviceInfo() const OVERRIDE {
@@ -198,7 +198,8 @@ class ProfileSyncServiceSessionTest
         profile(),
         signin,
         oauth2_token_service,
-        ProfileSyncService::AUTO_START));
+        ProfileSyncService::AUTO_START,
+        false));
     sync_service_->set_backend_init_callback(callback);
 
     // Register the session data type.

chrome/browser/sync/profile_sync_service_typed_url_unittest.cc

@@ -23,7 +23,6 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/invalidation/invalidation_service_factory.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"

chrome/browser/sync/test_profile_sync_service.h

@@ -10,18 +10,21 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/invalidation/invalidator_storage.h"
 #include "chrome/browser/signin/profile_oauth2_token_service.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/sync/glue/data_type_manager_impl.h"
 #include "chrome/browser/sync/glue/sync_backend_host_impl.h"
+#include "chrome/browser/sync/profile_sync_components_factory_mock.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/sync_prefs.h"
+#include "chrome/test/base/profile_mock.h"
+#include "google_apis/gaia/oauth2_token_service.h"
+#include "sync/internal_api/public/test/test_internal_components_factory.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 class Profile;
-class ProfileOAuth2TokenService;
-class ProfileSyncComponentsFactory;
-class ProfileSyncComponentsFactoryMock;
 
 ACTION(ReturnNewDataTypeManager) {
   return new browser_sync::DataTypeManagerImpl(arg0,
@@ -36,12 +39,24 @@ namespace browser_sync {
 
 class SyncBackendHostForProfileSyncTest : public SyncBackendHostImpl {
  public:
+  // |synchronous_init| causes initialization to block until the syncapi has
+  //     completed setting itself up and called us back.
+  // TOOD(akalin): Remove |synchronous_init| (http://crbug.com/140354).
   SyncBackendHostForProfileSyncTest(
       Profile* profile,
       const base::WeakPtr<SyncPrefs>& sync_prefs,
-      base::Closure& callback);
+      base::Closure& callback,
+      bool set_initial_sync_ended_on_init,
+      bool synchronous_init,
+      bool fail_initial_download,
+      syncer::StorageOption storage_option);
   virtual ~SyncBackendHostForProfileSyncTest();
 
+  MOCK_METHOD1(RequestNudge, void(const tracked_objects::Location&));
+
+  virtual void UpdateCredentials(
+      const syncer::SyncCredentials& credentials) OVERRIDE;
+
   virtual void RequestConfigureSyncer(
       syncer::ConfigureReason reason,
       syncer::ModelTypeSet to_download,
@@ -54,31 +69,67 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHostImpl {
                                 syncer::ModelTypeSet)>& ready_task,
       const base::Closure& retry_callback) OVERRIDE;
 
+  virtual void HandleInitializationSuccessOnFrontendLoop(
+    const syncer::WeakHandle<syncer::JsBackend> js_backend,
+    const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>
+        debug_info_listener) OVERRIDE;
+
  protected:
   virtual void InitCore(scoped_ptr<DoInitializeOptions> options) OVERRIDE;
 
  private:
+
+
   // Invoked at the start of HandleSyncManagerInitializationOnFrontendLoop.
   // Allows extra initialization work to be performed before the backend comes
   // up.
   base::Closure& callback_;
+
+  // Saved closure in case we failed the initial download but then received
+  // new credentials. Holds the results of
+  // HandleSyncManagerInitializationOnFrontendLoop, and if
+  // |fail_initial_download_| was true, finishes the initialization process
+  // once we receive new credentials.
+  base::Closure initial_download_closure_;
+  bool fail_initial_download_;
+
+  bool set_initial_sync_ended_on_init_;
+  bool synchronous_init_;
+  syncer::StorageOption storage_option_;
+
+  base::WeakPtrFactory<SyncBackendHostForProfileSyncTest> weak_ptr_factory_;
 };
 
 }  // namespace browser_sync
 
 class TestProfileSyncService : public ProfileSyncService {
  public:
-  // TODO(tim): Add ability to inject TokenService alongside SigninManager.
+  // TODO(tim): Remove |synchronous_backend_initialization|, and add ability to
+  // inject TokenService alongside SigninManager.
   // TODO(rogerta): what does above comment mean?
   TestProfileSyncService(
       ProfileSyncComponentsFactory* factory,
       Profile* profile,
       SigninManagerBase* signin,
       ProfileOAuth2TokenService* oauth2_token_service,
-      ProfileSyncService::StartBehavior behavior);
+      ProfileSyncService::StartBehavior behavior,
+      bool synchronous_backend_initialization);
 
   virtual ~TestProfileSyncService();
 
+  virtual void RequestAccessToken() OVERRIDE;
+  virtual void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
+                                 const std::string& access_token,
+                                 const base::Time& expiration_time) OVERRIDE;
+  virtual void OnGetTokenFailure(const OAuth2TokenService::Request* request,
+      const GoogleServiceAuthError& error) OVERRIDE;
+
+  virtual void OnBackendInitialized(
+      const syncer::WeakHandle<syncer::JsBackend>& backend,
+      const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&
+          debug_info_listener,
+      bool success) OVERRIDE;
+
   virtual void OnConfigureDone(
       const browser_sync::DataTypeManager::ConfigureResult& result) OVERRIDE;
 
@@ -90,6 +141,16 @@ class TestProfileSyncService : public ProfileSyncService {
 
   ProfileSyncComponentsFactoryMock* components_factory_mock();
 
+  // If this is called, configuring data types will require a syncer
+  // nudge.
+  void dont_set_initial_sync_ended_on_init();
+  void set_synchronous_sync_configuration();
+
+  // Fails initial download until a new auth token is provided.
+  void fail_initial_download();
+
+  void set_storage_option(syncer::StorageOption option);
+
   // |callback| can be used to populate nodes before the OnBackendInitialized
   // callback fires.
   void set_backend_init_callback(const base::Closure& callback) {
@@ -98,6 +159,12 @@ class TestProfileSyncService : public ProfileSyncService {
 
   syncer::TestIdFactory* id_factory();
 
+  // Override of ProfileSyncService::GetBackendForTest() with a more
+  // specific return type (since C++ supports covariant return types)
+  // that is made public.
+  virtual browser_sync::SyncBackendHostForProfileSyncTest*
+      GetBackendForTest() OVERRIDE;
+
  protected:
   virtual void CreateBackend() OVERRIDE;
 
@@ -110,7 +177,17 @@ class TestProfileSyncService : public ProfileSyncService {
  private:
   syncer::TestIdFactory id_factory_;
 
+  bool synchronous_backend_initialization_;
+
+  // Set to true when a mock data type manager is being used and the configure
+  // step is performed synchronously.
+  bool synchronous_sync_configuration_;
+
   base::Closure callback_;
+  bool set_initial_sync_ended_on_init_;
+
+  bool fail_initial_download_;
+  syncer::StorageOption storage_option_;
 };
 
 #endif  // CHROME_BROWSER_SYNC_TEST_PROFILE_SYNC_SERVICE_H_

chrome/browser/ui/sync/one_click_signin_helper_unittest.cc

@@ -196,7 +196,8 @@ class OneClickTestProfileSyncService : public TestProfileSyncService {
                                 profile,
                                 NULL,
                                 NULL,
-                                ProfileSyncService::MANUAL_START),
+                                ProfileSyncService::MANUAL_START,
+                                false),  // synchronous_backend_init
          first_setup_in_progress_(false) {}
 
    bool first_setup_in_progress_;

sync/internal_api/public/test/sync_manager_factory_for_profile_sync_test.h

@@ -15,12 +15,14 @@ namespace syncer {
 
 class SyncManagerFactoryForProfileSyncTest : public syncer::SyncManagerFactory {
  public:
-  SyncManagerFactoryForProfileSyncTest(base::Closure init_callback);
+  SyncManagerFactoryForProfileSyncTest(base::Closure init_callback,
+                                       bool set_initial_sync_ended);
   virtual ~SyncManagerFactoryForProfileSyncTest();
   virtual scoped_ptr<syncer::SyncManager> CreateSyncManager(
       std::string name) OVERRIDE;
  private:
   base::Closure init_callback_;
+  bool set_initial_sync_ended_;
 };
 
 }  // namespace syncer

sync/internal_api/test/sync_manager_factory_for_profile_sync_test.cc

@@ -9,8 +9,10 @@
 namespace syncer {
 
 SyncManagerFactoryForProfileSyncTest::SyncManagerFactoryForProfileSyncTest(
-    base::Closure init_callback)
-  : init_callback_(init_callback) {
+    base::Closure init_callback,
+    bool set_initial_sync_ended)
+  : init_callback_(init_callback),
+    set_initial_sync_ended_(set_initial_sync_ended) {
 }
 
 SyncManagerFactoryForProfileSyncTest::~SyncManagerFactoryForProfileSyncTest() {}
@@ -20,7 +22,8 @@ SyncManagerFactoryForProfileSyncTest::CreateSyncManager(std::string name) {
   return scoped_ptr<syncer::SyncManager>(
       new SyncManagerForProfileSyncTest(
           name,
-          init_callback_));
+          init_callback_,
+          set_initial_sync_ended_));
 }
 
 }  // namespace syncer

sync/internal_api/test/sync_manager_for_profile_sync_test.cc

@@ -12,9 +12,11 @@ namespace syncer {
 
 SyncManagerForProfileSyncTest::SyncManagerForProfileSyncTest(
     std::string name,
-    base::Closure init_callback)
+    base::Closure init_callback,
+    bool set_initial_sync_ended)
   : SyncManagerImpl(name),
-    init_callback_(init_callback) {}
+    init_callback_(init_callback),
+    set_initial_sync_ended_(set_initial_sync_ended) {}
 
 SyncManagerForProfileSyncTest::~SyncManagerForProfileSyncTest() {}
 
@@ -25,6 +27,7 @@ void SyncManagerForProfileSyncTest::NotifyInitializationSuccess() {
   if (!init_callback_.is_null())
     init_callback_.Run();
 
+  if (set_initial_sync_ended_) {
     ModelTypeSet early_download_types;
     early_download_types.PutAll(ControlTypes());
     early_download_types.PutAll(PriorityUserTypes());
@@ -34,6 +37,9 @@ void SyncManagerForProfileSyncTest::NotifyInitializationSuccess() {
         syncer::TestUserShare::CreateRoot(it.Get(), user_share);
       }
     }
+  } else {
+    VLOG(2) << "Skipping directory init";
+  }
 
   SyncManagerImpl::NotifyInitializationSuccess();
 }

sync/internal_api/test/sync_manager_for_profile_sync_test.h

@@ -19,12 +19,14 @@ class SyncManagerForProfileSyncTest
     : public syncer::SyncManagerImpl {
  public:
   SyncManagerForProfileSyncTest(std::string name,
-                                base::Closure init_callback);
+                                base::Closure init_callback,
+                                bool set_initial_sync_ended);
   virtual ~SyncManagerForProfileSyncTest();
   virtual void NotifyInitializationSuccess() OVERRIDE;
 
  private:
   base::Closure init_callback_;
+  bool set_initial_sync_ended_;
 };
 
 }  // namespace syncer

sync/js/sync_js_controller_unittest.cc

@@ -30,15 +30,10 @@ class SyncJsControllerTest : public testing::Test {
   base::MessageLoop message_loop_;
 };
 
-ACTION_P(ReplyToMessage, reply_name) {
-  arg2.Call(FROM_HERE, &JsReplyHandler::HandleJsReply, reply_name, JsArgList());
-}
-
 TEST_F(SyncJsControllerTest, Messages) {
   InSequence dummy;
   // |mock_backend| needs to outlive |sync_js_controller|.
   StrictMock<MockJsBackend> mock_backend;
-  StrictMock<MockJsReplyHandler> mock_reply_handler;
   SyncJsController sync_js_controller;
 
   base::ListValue arg_list1, arg_list2;
@@ -46,23 +41,17 @@ TEST_F(SyncJsControllerTest, Messages) {
   arg_list2.Append(new base::FundamentalValue(5));
   JsArgList args1(&arg_list1), args2(&arg_list2);
 
+  // TODO(akalin): Write matchers for WeakHandle and use them here
+  // instead of _.
   EXPECT_CALL(mock_backend, SetJsEventHandler(_));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _))
-      .WillOnce(ReplyToMessage("test1_reply"));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _))
-      .WillOnce(ReplyToMessage("test2_reply"));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _));
 
   sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle());
-  sync_js_controller.ProcessJsMessage("test1",
-                                      args2,
-                                      mock_reply_handler.AsWeakHandle());
-  sync_js_controller.ProcessJsMessage("test2",
-                                      args1,
-                                      mock_reply_handler.AsWeakHandle());
-
-  // The replies should be waiting on our message loop.
-  EXPECT_CALL(mock_reply_handler, HandleJsReply("test1_reply", _));
-  EXPECT_CALL(mock_reply_handler, HandleJsReply("test2_reply", _));
+  sync_js_controller.ProcessJsMessage("test1", args2,
+                                      WeakHandle<JsReplyHandler>());
+  sync_js_controller.ProcessJsMessage("test2", args1,
+                                      WeakHandle<JsReplyHandler>());
   PumpLoop();
 
   // Let destructor of |sync_js_controller| call RemoveBackend().
@@ -71,7 +60,6 @@ TEST_F(SyncJsControllerTest, Messages) {
 TEST_F(SyncJsControllerTest, QueuedMessages) {
   // |mock_backend| needs to outlive |sync_js_controller|.
   StrictMock<MockJsBackend> mock_backend;
-  StrictMock<MockJsReplyHandler> mock_reply_handler;
   SyncJsController sync_js_controller;
 
   base::ListValue arg_list1, arg_list2;
@@ -80,29 +68,20 @@ TEST_F(SyncJsControllerTest, QueuedMessages) {
   JsArgList args1(&arg_list1), args2(&arg_list2);
 
   // Should queue messages.
-  sync_js_controller.ProcessJsMessage(
-      "test1",
-      args2,
-      mock_reply_handler.AsWeakHandle());
-  sync_js_controller.ProcessJsMessage(
-      "test2",
-      args1,
-      mock_reply_handler.AsWeakHandle());
+  sync_js_controller.ProcessJsMessage("test1", args2,
+                                      WeakHandle<JsReplyHandler>());
+  sync_js_controller.ProcessJsMessage("test2", args1,
+                                      WeakHandle<JsReplyHandler>());
 
-  // Should do nothing.
-  PumpLoop();
   Mock::VerifyAndClearExpectations(&mock_backend);
 
-
-  // Should call the queued messages.
+  // TODO(akalin): Write matchers for WeakHandle and use them here
+  // instead of _.
   EXPECT_CALL(mock_backend, SetJsEventHandler(_));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _))
-      .WillOnce(ReplyToMessage("test1_reply"));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _))
-      .WillOnce(ReplyToMessage("test2_reply"));
-  EXPECT_CALL(mock_reply_handler, HandleJsReply("test1_reply", _));
-  EXPECT_CALL(mock_reply_handler, HandleJsReply("test2_reply", _));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _));
 
+  // Should call the queued messages.
   sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle());
   PumpLoop();