[Sync] Refactor sync configuration logic.

sync/internal_api/syncapi_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Unit tests for the SyncApi. Note that a lot of the underlying
 // functionality is provided by the Syncable layer, which has its own
 // unit tests. We'll test SyncApi specific things in this harness.
 
 #include <cstddef>
 #include <map>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_number_conversions.h"
 #include "base/stringprintf.h"
 #include "base/test/values_test_util.h"
 #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"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
 #include "sync/internal_api/public/syncable/model_type_test_util.h"
 #include "sync/internal_api/read_node.h"
 #include "sync/internal_api/read_transaction.h"
 #include "sync/internal_api/sync_manager.h"
 #include "sync/internal_api/syncapi_internal.h"
@@ skipping 10 matching lines @@
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/extension_specifics.pb.h"
 #include "sync/protocol/password_specifics.pb.h"
 #include "sync/protocol/preference_specifics.pb.h"
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
 #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"
 #include "sync/util/extensions_activity_monitor.h"
 #include "sync/util/test_unrecoverable_error_handler.h"
 #include "sync/util/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #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;
 using browser_sync::HasArgsAsList;
 using browser_sync::HasDetailsAsDictionary;
 using browser_sync::KeyParams;
 using browser_sync::kNigoriTag;
 using browser_sync::JsArgList;
 using browser_sync::JsBackend;
 using browser_sync::JsEventHandler;
@@ skipping 636 matching lines @@
   MOCK_METHOD1(RemoveObserver, void(sync_notifier::SyncNotifierObserver*));
   MOCK_METHOD1(SetUniqueId, void(const std::string&));
   MOCK_METHOD1(SetStateDeprecated, void(const std::string&));
   MOCK_METHOD2(UpdateCredentials,
                void(const std::string&, const std::string&));
   MOCK_METHOD1(UpdateEnabledTypes,
                void(syncable::ModelTypeSet));
   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,
                         public SyncManager::ChangeDelegate {
  protected:
   enum NigoriStatus {
     DONT_WRITE_NIGORI,
     WRITE_TO_NIGORI
   };
 
   enum EncryptionStatus {
     UNINITIALIZED,
     DEFAULT_ENCRYPTION,
     FULL_ENCRYPTION
   };
 
   SyncManagerTest()
       : sync_notifier_mock_(NULL),
-        sync_manager_("Test sync manager"),
         sync_notifier_observer_(NULL),
         update_enabled_types_call_count_(0) {}
 
   virtual ~SyncManagerTest() {
     EXPECT_FALSE(sync_notifier_mock_);
   }
 
   // Test implementation.
   void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
@@ skipping 180 matching lines @@
   // Needed by |sync_manager_|.
   ScopedTempDir temp_dir_;
   // Sync Id's for the roots of the enabled datatypes.
   std::map<ModelType, int64> type_roots_;
   FakeExtensionsActivityMonitor extensions_activity_monitor_;
   StrictMock<SyncNotifierMock>* sync_notifier_mock_;
 
  protected:
   FakeEncryptor encryptor_;
   TestUnrecoverableErrorHandler handler_;
-  SyncManager sync_manager_;
+  TestSyncManager sync_manager_;
   WeakHandle<JsBackend> js_backend_;
   StrictMock<SyncManagerObserverMock> observer_;
   sync_notifier::SyncNotifierObserver* sync_notifier_observer_;
   int update_enabled_types_call_count_;
 };
 
 TEST_F(SyncManagerTest, UpdateEnabledTypes) {
   EXPECT_EQ(0, update_enabled_types_call_count_);
 
   ModelSafeRoutingInfo routes;
@@ skipping 1547 matching lines @@
               node.InitByClientTagLookup(syncable::BOOKMARKS, client_tag));
     EXPECT_EQ(title, node.GetTitle());
     EXPECT_EQ(GURL(url2), node.GetURL());
     const syncable::Entry* node_entry = node.GetEntry();
     EXPECT_EQ(kEncryptedString, node_entry->Get(NON_UNIQUE_NAME));
     const sync_pb::EntitySpecifics& specifics = node_entry->Get(SPECIFICS);
     EXPECT_TRUE(specifics.has_encrypted());
   }
 }
 
+// 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