sync: Improve testing of download functions

Refactor the download functions so the commit to the server can be
separated from the building of the commit message.  This allows us to
test these functions in download_unittest.cc without needing to use the
heavyweight MockConnectionManager.  It also lets us improve our testing
of AppendClientDebugInfoIfNeeded by using less mocks and more real code.

An important side benefit of this change is that it makes progress
towards the removal of ModelTypeInvalidationMap, which is a step
towards implementing crbug.com/233437.

Also included in this change is a new TRACE_EVENT.  Though it's not
really new, since it was removed (accidentally) in the Syncer refactor
(r209867).  It's not related to this change, but it's a one-liner and I
happen to be modifying nearby code.

BUG=233437

Review URL: https://chromiumcodereview.appspot.com/22849021

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

4 files changed, 57 insertions, 59 deletions

diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc
index 65998e955..5bf2b4b 100644
--- a/sync/test/engine/mock_connection_manager.cc
+++ b/sync/test/engine/mock_connection_manager.cc
@@ -502,12 +502,6 @@ void MockConnectionManager::ProcessGetUpdates(
         GetProgressMarkerForType(gu.from_progress_marker(), model_type);
     EXPECT_EQ(expected_filter_.Has(model_type), (progress_marker != NULL))
         << "Syncer requested_types differs from test expectation.";
-    if (progress_marker) {
-      EXPECT_EQ((expected_states_.count(model_type) > 0 ?
-                 expected_states_[model_type].payload :
-                 std::string()),
-                progress_marker->notification_hint());
-    }
   }
 
   // Verify that the items we're about to send back to the client are of
diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h
index 9f7cdb1..bab81ae 100644
--- a/sync/test/engine/mock_connection_manager.h
+++ b/sync/test/engine/mock_connection_manager.h
@@ -234,10 +234,6 @@ class MockConnectionManager : public ServerConnectionManager {
     expected_filter_ = expected_filter;
   }
 
-  void ExpectGetUpdatesRequestStates(const ModelTypeInvalidationMap& states) {
-    expected_states_ = states;
-  }
-
   void SetServerReachable();
 
   void SetServerNotReachable();
@@ -394,8 +390,6 @@ class MockConnectionManager : public ServerConnectionManager {
 
   ModelTypeSet expected_filter_;
 
-  ModelTypeInvalidationMap expected_states_;
-
   int num_get_updates_requests_;
 
   std::string next_token_;
diff --git a/sync/test/engine/syncer_command_test.cc b/sync/test/engine/syncer_command_test.cc
index 68edbbd..3baf2d1 100644
--- a/sync/test/engine/syncer_command_test.cc
+++ b/sync/test/engine/syncer_command_test.cc
@@ -9,6 +9,49 @@ namespace syncer {
 const unsigned int kMaxMessages = 10;
 const unsigned int kMaxMessageSize = 5 * 1024;
 
+void SyncerCommandTestBase::OnThrottled(
+    const base::TimeDelta& throttle_duration) {
+  FAIL() << "Should not get silenced.";
+}
+
+void SyncerCommandTestBase::OnTypesThrottled(
+    ModelTypeSet types,
+    const base::TimeDelta& throttle_duration) {
+  FAIL() << "Should not get silenced.";
+}
+
+bool SyncerCommandTestBase::IsCurrentlyThrottled() {
+  return false;
+}
+
+void SyncerCommandTestBase::OnReceivedLongPollIntervalUpdate(
+    const base::TimeDelta& new_interval) {
+  FAIL() << "Should not get poll interval update.";
+}
+
+void SyncerCommandTestBase::OnReceivedShortPollIntervalUpdate(
+    const base::TimeDelta& new_interval) {
+  FAIL() << "Should not get poll interval update.";
+}
+
+void SyncerCommandTestBase::OnReceivedSessionsCommitDelay(
+    const base::TimeDelta& new_delay) {
+  FAIL() << "Should not get sessions commit delay.";
+}
+
+void SyncerCommandTestBase::OnReceivedClientInvalidationHintBufferSize(
+    int size) {
+  FAIL() << "Should not get hint buffer size.";
+}
+
+void SyncerCommandTestBase::OnShouldStopSyncingPermanently() {
+  FAIL() << "Shouldn't be called.";
+}
+
+void SyncerCommandTestBase::OnSyncProtocolError(
+    const sessions::SyncSessionSnapshot& session) {
+  return;
+}
 SyncerCommandTestBase::SyncerCommandTestBase()
     : traffic_recorder_(kMaxMessages, kMaxMessageSize) {
 }
@@ -41,10 +84,4 @@ void SyncerCommandTest::TearDown() {
   dir_maker_.TearDown();
 }
 
-MockDebugInfoGetter::MockDebugInfoGetter() {
-}
-
-MockDebugInfoGetter::~MockDebugInfoGetter() {
-}
-
 }  // namespace syncer
diff --git a/sync/test/engine/syncer_command_test.h b/sync/test/engine/syncer_command_test.h
index f0904cc..6e1f672 100644
--- a/sync/test/engine/syncer_command_test.h
+++ b/sync/test/engine/syncer_command_test.h
@@ -14,6 +14,7 @@
 #include "base/message_loop/message_loop.h"
 #include "sync/engine/model_changing_syncer_command.h"
 #include "sync/engine/traffic_recorder.h"
+#include "sync/internal_api/debug_info_event_listener.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/sessions/debug_info_getter.h"
 #include "sync/sessions/sync_session.h"
@@ -26,17 +27,8 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::NiceMock;
-
 namespace syncer {
 
-class MockDebugInfoGetter : public sessions::DebugInfoGetter {
- public:
-  MockDebugInfoGetter();
-  virtual ~MockDebugInfoGetter();
-  MOCK_METHOD1(GetAndClearDebugInfo, void(sync_pb::DebugInfo* debug_info));
-};
-
 // A test fixture that simplifies writing unit tests for individual
 // SyncerCommands, providing convenient access to a test directory
 // and a syncer session.
@@ -45,39 +37,21 @@ class SyncerCommandTestBase : public testing::Test,
  public:
   // SyncSession::Delegate implementation.
   virtual void OnThrottled(
-      const base::TimeDelta& throttle_duration) OVERRIDE {
-    FAIL() << "Should not get silenced.";
-  }
+      const base::TimeDelta& throttle_duration) OVERRIDE;
   virtual void OnTypesThrottled(
       ModelTypeSet types,
-      const base::TimeDelta& throttle_duration) OVERRIDE {
-    FAIL() << "Should not get silenced.";
-  }
-  virtual bool IsCurrentlyThrottled() OVERRIDE {
-    return false;
-  }
+      const base::TimeDelta& throttle_duration) OVERRIDE;
+  virtual bool IsCurrentlyThrottled() OVERRIDE;
   virtual void OnReceivedLongPollIntervalUpdate(
-      const base::TimeDelta& new_interval) OVERRIDE {
-    FAIL() << "Should not get poll interval update.";
-  }
+      const base::TimeDelta& new_interval) OVERRIDE;
   virtual void OnReceivedShortPollIntervalUpdate(
-      const base::TimeDelta& new_interval) OVERRIDE {
-    FAIL() << "Should not get poll interval update.";
-  }
+      const base::TimeDelta& new_interval) OVERRIDE;
   virtual void OnReceivedSessionsCommitDelay(
-      const base::TimeDelta& new_delay) OVERRIDE {
-    FAIL() << "Should not get sessions commit delay.";
-  }
-  virtual void OnReceivedClientInvalidationHintBufferSize(int size) OVERRIDE {
-    FAIL() << "Should not get hint buffer size.";
-  }
-  virtual void OnShouldStopSyncingPermanently() OVERRIDE {
-    FAIL() << "Shouldn't be called.";
-  }
+      const base::TimeDelta& new_delay) OVERRIDE;
+  virtual void OnReceivedClientInvalidationHintBufferSize(int size) OVERRIDE;
+  virtual void OnShouldStopSyncingPermanently() OVERRIDE;
   virtual void OnSyncProtocolError(
-      const sessions::SyncSessionSnapshot& session) OVERRIDE {
-    return;
-  }
+      const sessions::SyncSessionSnapshot& session) OVERRIDE;
 
   std::vector<ModelSafeWorker*> GetWorkers() {
     std::vector<ModelSafeWorker*> workers;
@@ -117,7 +91,7 @@ class SyncerCommandTestBase : public testing::Test,
             mock_server_.get(), directory(),
             GetWorkers(), extensions_activity_.get(),
             std::vector<SyncEngineEventListener*>(),
-            &mock_debug_info_getter_,
+            &debug_info_event_listener_,
             &traffic_recorder_,
             true,  // enable keystore encryption
             false,  // force enable pre-commit GU avoidance experiment
@@ -147,10 +121,9 @@ class SyncerCommandTestBase : public testing::Test,
     return mock_server_.get();
   }
 
-  MockDebugInfoGetter* mock_debug_info_getter() {
-    return &mock_debug_info_getter_;
+  DebugInfoEventListener* debug_info_event_listener() {
+    return &debug_info_event_listener_;
   }
-
   // Helper functions to check command.GetGroupsToChange().
 
   void ExpectNoGroupsToChange(const ModelChangingSyncerCommand& command) {
@@ -193,7 +166,7 @@ class SyncerCommandTestBase : public testing::Test,
   scoped_ptr<sessions::SyncSession> session_;
   std::vector<scoped_refptr<ModelSafeWorker> > workers_;
   ModelSafeRoutingInfo routing_info_;
-  NiceMock<MockDebugInfoGetter> mock_debug_info_getter_;
+  DebugInfoEventListener debug_info_event_listener_;
   scoped_refptr<ExtensionsActivity> extensions_activity_;
   TrafficRecorder traffic_recorder_;
   DISALLOW_COPY_AND_ASSIGN(SyncerCommandTestBase);