[Sync] Refactor sync configuration logic.

sync/internal_api/syncapi_unittest.cc

Index: sync/internal_api/syncapi_unittest.cc
diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc
index 991aed6b0a6f82dcc38a483c8f23e35a11e18a7f..a8b46d7bf4bcf04bdeca12a630ee6a01b135bb7b 100644
--- a/sync/internal_api/syncapi_unittest.cc
+++ b/sync/internal_api/syncapi_unittest.cc
@@ -24,6 +24,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "sync/engine/nigori_util.h"
+#include "sync/engine/sync_scheduler.h"
 #include "sync/internal_api/change_record.h"
 #include "sync/internal_api/http_post_provider_factory.h"
 #include "sync/internal_api/http_post_provider_interface.h"
@@ -54,6 +55,7 @@
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/syncable.h"
 #include "sync/syncable/syncable_id.h"
+#include "sync/test/callback_counter.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_extensions_activity_monitor.h"
 #include "sync/util/cryptographer.h"
@@ -64,6 +66,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ExpectDictStringValue;
+using browser_sync::CallbackCounter;
 using browser_sync::Cryptographer;
 using browser_sync::FakeEncryptor;
 using browser_sync::FakeExtensionsActivityMonitor;
@@ -720,6 +723,37 @@ class SyncNotifierMock : public sync_notifier::SyncNotifier {
   MOCK_METHOD1(SendNotification, void(syncable::ModelTypeSet));
 };
 
+class TestSyncManager : public SyncManager {
+ public:
+  TestSyncManager()
+    : SyncManager("Test sync manager"),
+      result_(true) {}
+
+  browser_sync::ConfigurationParams get_config_params() const {
+    return params_;
+  }
+  void set_config_result(bool result) { result_ = result; }
+
+ protected:
+  // SyncManager implementation.
+  virtual bool DoConfigureSyncer(

[tim (not reviewing), 2012/06/15 14:59:37]

Ugh.  I see the corner you're in here.  I'm really worried about adding more of
these Test impls, though. It leads to pain and brittleness (just look at
SyncBackendHostForTest).

It would be much better to pass a fake / mock SyncScheduler and have
set_config_result on that class instead, and here test the real SyncManager. You
could make the fixture a friend of SyncManager to allow setting your fake
scheduler.  I know some of these tests rely on a null scheduler, but perhaps you
could stuff it in in your specific test?  And in the future, tests should rely
on our specially crafted fake scheduler vs a null one (to make sure e.g. we
don't actually try and ping the server).

[Nicolas Zea, 2012/06/15 21:23:36]

I see what you're saying, but given that that's kind of a refactor of how we
test the SyncManager (and this patch is already fairly large), mind if I do it
as a followup patch? I've added a TODO/bug in sync_manager.h.

[tim (not reviewing), 2012/06/15 21:58:24]

As we discussed offline, I think we can special case your tests to have access
to reset the scheduler without touching the rest. 

+      const browser_sync::ConfigurationParams& params) OVERRIDE {
+    params_ = params;
+    if (params.ready_task.is_null()) {
+      ADD_FAILURE();

[tim (not reviewing), 2012/06/15 14:59:37]

nit, in general something like ADD_FAILURE() << "Null ready_task in
DoConfigureSyncer" ? would be more useful.

[Nicolas Zea, 2012/06/15 21:23:36]

Done.

+      return result_;
+    }
+
+    if (result_)
+      params.ready_task.Run();
+    return result_;
+  }
+
+ private:
+  browser_sync::ConfigurationParams params_;
+  bool result_;
+};
+
 }  // namespace
 
 class SyncManagerTest : public testing::Test,
@@ -738,7 +772,6 @@ class SyncManagerTest : public testing::Test,
 
   SyncManagerTest()
       : sync_notifier_mock_(NULL),
-        sync_manager_("Test sync manager"),
         sync_notifier_observer_(NULL),
         update_enabled_types_call_count_(0) {}
 
@@ -939,7 +972,7 @@ class SyncManagerTest : public testing::Test,
  protected:
   FakeEncryptor encryptor_;
   TestUnrecoverableErrorHandler handler_;
-  SyncManager sync_manager_;
+  TestSyncManager sync_manager_;
   WeakHandle<JsBackend> js_backend_;
   StrictMock<SyncManagerObserverMock> observer_;
   sync_notifier::SyncNotifierObserver* sync_notifier_observer_;
@@ -2507,4 +2540,58 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) {
   }
 }
 
+// Test that the configuration params are properly sent to DoConfigureSyncer.
+TEST_F(SyncManagerTest, BasicConfiguration) {
+  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
+  syncable::ModelTypeSet types_to_download(syncable::BOOKMARKS,
+                                           syncable::PREFERENCES);
+  browser_sync::ModelSafeRoutingInfo new_routing_info;
+  GetModelSafeRoutingInfo(&new_routing_info);
+  sync_manager_.set_config_result(true);
+
+  CallbackCounter ready_task_counter, retry_task_counter;
+  sync_manager_.ConfigureSyncer(
+      reason,
+      types_to_download,
+      new_routing_info,
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&ready_task_counter)),
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&retry_task_counter)));
+  EXPECT_EQ(1, ready_task_counter.times_called());
+  EXPECT_EQ(0, retry_task_counter.times_called());
+  browser_sync::ConfigurationParams params = sync_manager_.get_config_params();
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+            params.source);
+  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
+  EXPECT_EQ(new_routing_info, params.routing_info);
+}
+
+// Test that the retry callback is invoked on configuration failure.
+TEST_F(SyncManagerTest, ConfigurationRetry) {
+  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
+  syncable::ModelTypeSet types_to_download(syncable::BOOKMARKS,
+                                           syncable::PREFERENCES);
+  browser_sync::ModelSafeRoutingInfo new_routing_info;
+  GetModelSafeRoutingInfo(&new_routing_info);
+  sync_manager_.set_config_result(false);
+
+  CallbackCounter ready_task_counter, retry_task_counter;
+  sync_manager_.ConfigureSyncer(
+      reason,
+      types_to_download,
+      new_routing_info,
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&ready_task_counter)),
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&retry_task_counter)));
+  EXPECT_EQ(0, ready_task_counter.times_called());
+  EXPECT_EQ(1, retry_task_counter.times_called());
+  browser_sync::ConfigurationParams params = sync_manager_.get_config_params();
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+            params.source);
+  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
+  EXPECT_EQ(new_routing_info, params.routing_info);
+}
+
 }  // namespace browser_sync