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

5 files changed, 44 insertions, 52 deletions

diff --git a/sync/internal_api/public/test/sync_manager_factory_for_profile_sync_test.h b/sync/internal_api/public/test/sync_manager_factory_for_profile_sync_test.h
index 76d6bef..d8615c0 100644
--- a/sync/internal_api/public/test/sync_manager_factory_for_profile_sync_test.h
+++ b/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
diff --git a/sync/internal_api/test/sync_manager_factory_for_profile_sync_test.cc b/sync/internal_api/test/sync_manager_factory_for_profile_sync_test.cc
index 34289c4..df4073b 100644
--- a/sync/internal_api/test/sync_manager_factory_for_profile_sync_test.cc
+++ b/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
diff --git a/sync/internal_api/test/sync_manager_for_profile_sync_test.cc b/sync/internal_api/test/sync_manager_for_profile_sync_test.cc
index e552c2f..aaa6c8d 100644
--- a/sync/internal_api/test/sync_manager_for_profile_sync_test.cc
+++ b/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,14 +27,18 @@ void SyncManagerForProfileSyncTest::NotifyInitializationSuccess() {
   if (!init_callback_.is_null())
     init_callback_.Run();
 
-  ModelTypeSet early_download_types;
-  early_download_types.PutAll(ControlTypes());
-  early_download_types.PutAll(PriorityUserTypes());
-  for (ModelTypeSet::Iterator it = early_download_types.First();
-       it.Good(); it.Inc()) {
-    if (!directory->InitialSyncEndedForType(it.Get())) {
-      syncer::TestUserShare::CreateRoot(it.Get(), user_share);
+  if (set_initial_sync_ended_) {
+    ModelTypeSet early_download_types;
+    early_download_types.PutAll(ControlTypes());
+    early_download_types.PutAll(PriorityUserTypes());
+    for (ModelTypeSet::Iterator it = early_download_types.First();
+         it.Good(); it.Inc()) {
+      if (!directory->InitialSyncEndedForType(it.Get())) {
+        syncer::TestUserShare::CreateRoot(it.Get(), user_share);
+      }
     }
+  } else {
+    VLOG(2) << "Skipping directory init";
   }
 
   SyncManagerImpl::NotifyInitializationSuccess();
diff --git a/sync/internal_api/test/sync_manager_for_profile_sync_test.h b/sync/internal_api/test/sync_manager_for_profile_sync_test.h
index 8493386..c9af22d 100644
--- a/sync/internal_api/test/sync_manager_for_profile_sync_test.h
+++ b/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
diff --git a/sync/js/sync_js_controller_unittest.cc b/sync/js/sync_js_controller_unittest.cc
index eca617c..ac46935 100644
--- a/sync/js/sync_js_controller_unittest.cc
+++ b/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();