Revert 239635 "Revert 239615 "Retry: Clean up TestProfileSyncSer..."

Didn't help.

> Revert 239615 "Retry: Clean up TestProfileSyncService tests"
> 
> Failed on CrOS:
> 
> http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%29/builds/18496/steps/BuildPackages/logs/stdio
> 
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:417:11: error: 'MetaInfo' in namespace 'sync_pb' does not name a type
> chromeos-chrome-33.0.1734.0_alpha-r1:      const sync_pb::MetaInfo& proto) {
> chromeos-chrome-33.0.1734.0_alpha-r1:            ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:417:30: error: ISO C++ forbids declaration of 'proto' with no type [-fpermissive]
> chromeos-chrome-33.0.1734.0_alpha-r1:      const sync_pb::MetaInfo& proto) {
> chromeos-chrome-33.0.1734.0_alpha-r1:                               ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc: In function 'base::DictionaryValue* syncer::MetaInfoToValue(const int&)':
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:419:13: error: request for member 'has_key' in 'proto', which is of non-class type 'const int'
> chromeos-chrome-33.0.1734.0_alpha-r1:    SET_STR(key);
> chromeos-chrome-33.0.1734.0_alpha-r1:              ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:419:72: error: request for member 'key' in 'proto', which is of non-class type 'const int'
> chromeos-chrome-33.0.1734.0_alpha-r1:    SET_STR(key);
> chromeos-chrome-33.0.1734.0_alpha-r1:                                                                         ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:420:13: error: request for member 'has_value' in 'proto', which is of non-class type 'const int'
> chromeos-chrome-33.0.1734.0_alpha-r1:    SET_STR(value);
> chromeos-chrome-33.0.1734.0_alpha-r1:              ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:420:76: error: request for member 'value' in 'proto', which is of non-class type 'const int'
> chromeos-chrome-33.0.1734.0_alpha-r1:    SET_STR(value);
> chromeos-chrome-33.0.1734.0_alpha-r1:                                                                             ^
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc: In function 'base::DictionaryValue* syncer::BookmarkSpecificsToValue(const sync_pb::BookmarkSpecifics&)':
> chromeos-chrome-33.0.1734.0_alpha-r1: sync/protocol/proto_value_conversions.cc:432:51: error: 'const class sync_pb::BookmarkSpecifics' has no member named 'meta_info'
> chromeos-chrome-33.0.1734.0_alpha-r1:    SET_REP(meta_info, &MetaInfoToValue);
> chromeos-chrome-33.0.1734.0_alpha-r1:                                                    ^
> chromeos-chrome-33.0.1734.0_alpha-r1: make: *** [c/Release/obj.target/sync_core/sync/protocol/proto_value_conversions.o] Error 1
> chromeos-chrome-33.0.1734.0_alpha-r1: make: *** Waiting for unfinished jobs....
> chromeos-chrome-33.0.1734.0_alpha-r1:  * ERROR: chromeos-base/chromeos-chrome-33.0.1734.0_alpha-r1 failed (compile phase):
> chromeos-chrome-33.0.1734.0_alpha-r1:  *   emake failed
> chromeos-chrome-33.0.1734.0_alpha-r1:  * 
> 
> > Retry: Clean up TestProfileSyncService tests
> > 
> > This is a retry of r238348, which was reverted in r238368.  I believe
> > the build failure that led to the revert, though related to my change,
> > was a flake caused by stale object files on the build bot.
> > 
> > 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/102513002
> 
> TBR=rlarocque@chromium.org
> 
> Review URL: https://codereview.chromium.org/110993003

TBR=scottmg@chromium.org

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

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

1 files changed, 38 insertions, 17 deletions

diff --git a/sync/js/sync_js_controller_unittest.cc b/sync/js/sync_js_controller_unittest.cc
index ac46935..eca617c 100644
--- a/sync/js/sync_js_controller_unittest.cc
+++ b/sync/js/sync_js_controller_unittest.cc
@@ -30,10 +30,15 @@ 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;
@@ -41,17 +46,23 @@ 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), _));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _))
+      .WillOnce(ReplyToMessage("test1_reply"));
+  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _))
+      .WillOnce(ReplyToMessage("test2_reply"));
 
   sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle());
-  sync_js_controller.ProcessJsMessage("test1", args2,
-                                      WeakHandle<JsReplyHandler>());
-  sync_js_controller.ProcessJsMessage("test2", args1,
-                                      WeakHandle<JsReplyHandler>());
+  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", _));
   PumpLoop();
 
   // Let destructor of |sync_js_controller| call RemoveBackend().
@@ -60,6 +71,7 @@ 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;
@@ -68,20 +80,29 @@ TEST_F(SyncJsControllerTest, QueuedMessages) {
   JsArgList args1(&arg_list1), args2(&arg_list2);
 
   // Should queue messages.
-  sync_js_controller.ProcessJsMessage("test1", args2,
-                                      WeakHandle<JsReplyHandler>());
-  sync_js_controller.ProcessJsMessage("test2", args1,
-                                      WeakHandle<JsReplyHandler>());
+  sync_js_controller.ProcessJsMessage(
+      "test1",
+      args2,
+      mock_reply_handler.AsWeakHandle());
+  sync_js_controller.ProcessJsMessage(
+      "test2",
+      args1,
+      mock_reply_handler.AsWeakHandle());
 
+  // Should do nothing.
+  PumpLoop();
   Mock::VerifyAndClearExpectations(&mock_backend);
 
-  // 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), _));
-  EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _));
 
   // Should call the queued messages.
+  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", _));
+
   sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle());
   PumpLoop();