[Sync] Refactor sync configuration logic.

sync/engine/sync_scheduler_unittest.cc

Index: sync/engine/sync_scheduler_unittest.cc
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 43da04e79d4e840879d1791cb6bfd745f5c085b2..9432bcf515772afcb1c300b63f84fdc055fd0370 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -26,6 +26,7 @@ using testing::DoAll;
 using testing::Eq;
 using testing::Invoke;
 using testing::Mock;
+using testing::Not;
 using testing::Return;
 using testing::WithArg;
 
@@ -68,6 +69,18 @@ void PumpLoop() {
   RunLoop();
 }
 
+void SetBool(bool* flag) {

[rlarocque, 2012/06/09 01:44:35]

Could you do this with mocks instead?  This looks odd.

(Not that there's anything wrong with it.  I'm just trying to limit the number
of eyebrow-raising moments one might experience when reading the code.)

[Nicolas Zea, 2012/06/11 23:05:20]

Given that we pass in a callback, not an object, I think this is a more
straightforward way of testing that the callback is actually run. Mocks are more
useful for testing member methods of mock classes (although even there,
Fred/Drew have convinced me to use fake's over mock's more often).

[rlarocque, 2012/06/12 17:45:13]

You have a point that the mock framework is probably more trouble than its
worth.  

Maybe you could write a simple "CallbackCounter" class?  My main concern is that
I'd like to see a more descriptive type name than "bool".  It would also have
the advantage that it could initialize itself.

[Nicolas Zea, 2012/06/12 20:44:44]

Done.

+  *flag = true;
+}
+
+ModelSafeRoutingInfo TypesToRoutingInfo(const ModelTypeSet& types) {
+  ModelSafeRoutingInfo routes;
+  for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) {
+    routes[iter.Get()] = GROUP_PASSIVE;
+  }
+  return routes;
+}
+
 // Convenient to use in tests wishing to analyze SyncShare calls over time.
 static const size_t kMinNumSamples = 5;
 class SyncSchedulerTest : public testing::Test {
@@ -150,7 +163,7 @@ class SyncSchedulerTest : public testing::Test {
   }
 
   void StartSyncScheduler(SyncScheduler::Mode mode) {
-    scheduler()->Start(mode, base::Closure());
+    scheduler()->Start(mode);
   }
 
   // This stops the scheduler synchronously.
@@ -288,14 +301,23 @@ TEST_F(SyncSchedulerTest, Config) {
   SyncShareRecords records;
   const ModelTypeSet model_types(syncable::BOOKMARKS);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(),
+              SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  scheduler()->Configure(
-      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      model_types,
+      TypesToRoutingInfo(model_types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_TRUE(scheduler()->Configure(params));
+  ASSERT_TRUE(callback_called);
 
   ASSERT_EQ(1U, records.snapshots.size());
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types,
@@ -312,7 +334,9 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   SyncShareRecords records;
   const ModelTypeSet model_types(syncable::BOOKMARKS);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(),
+              SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),

[rlarocque, 2012/06/09 01:44:35]

I disagree with the use of SimulateCommitFailed here.  A configure session isn't
commit anything.  The scheduler would be perfectly justified in adding a
DCHECK_EQ(UNSET, commit_result).  This test really should be simulating a
download failure.  

(This is not related to your changes, so feel free to not fix this if it looks
like a lot of work.)

[Nicolas Zea, 2012/06/11 23:05:20]

This change is large enough, I'd rather leave it as is.

                       WithArg<0>(RecordSyncShare(&records))))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
@@ -321,61 +345,26 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ASSERT_EQ(0U, records.snapshots.size());
-  scheduler()->Configure(
-      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      model_types,
+      TypesToRoutingInfo(model_types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_FALSE(scheduler()->Configure(params));
 

[rlarocque, 2012/06/09 01:44:35]

ASSERT_FALSE(callback_called) here?

See also the more verbose comment around line 404.

[Nicolas Zea, 2012/06/11 23:05:20]

Done.

   ASSERT_EQ(1U, records.snapshots.size());
   RunLoop();
 
   ASSERT_EQ(2U, records.snapshots.size());
+  ASSERT_TRUE(callback_called);
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types,
       records.snapshots[1].source().types));
   EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
             records.snapshots[1].source().updates_source);
 }
 
-// Issue 2 config commands. Second one right after the first has failed
-// and make sure LATEST is executed.
-TEST_F(SyncSchedulerTest, MultipleConfigWithBackingOff) {
-  const ModelTypeSet
-      model_types1(syncable::BOOKMARKS),
-      model_types2(syncable::AUTOFILL);
-  UseMockDelayProvider();
-  EXPECT_CALL(*delay(), GetDelay(_))
-      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(30)));
-  SyncShareRecords records;
-
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
-                      WithArg<0>(RecordSyncShare(&records))))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
-                      WithArg<0>(RecordSyncShare(&records))))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
-                      WithArg<0>(RecordSyncShare(&records))));
-
-  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
-
-  ASSERT_EQ(0U, records.snapshots.size());
-  scheduler()->Configure(
-      model_types1, GetUpdatesCallerInfo::RECONFIGURATION);
-
-  ASSERT_EQ(1U, records.snapshots.size());
-  scheduler()->Configure(
-      model_types2, GetUpdatesCallerInfo::RECONFIGURATION);
-
-  // A canary job gets posted when we go into exponential backoff.
-  RunLoop();
-
-  ASSERT_EQ(2U, records.snapshots.size());
-  RunLoop();
-
-  ASSERT_EQ(3U, records.snapshots.size());
-  EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types2,
-      records.snapshots[2].source().types));
-  EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
-            records.snapshots[2].source().updates_source);
-}
-
 // Issue a nudge when the config has failed. Make sure both the config and
 // nudge are executed.
 TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
@@ -398,8 +387,14 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ASSERT_EQ(0U, records.snapshots.size());
-  scheduler()->Configure(
-      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      model_types,
+      TypesToRoutingInfo(model_types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_FALSE(scheduler()->Configure(params));
 
   ASSERT_EQ(1U, records.snapshots.size());
   scheduler()->ScheduleNudge(
@@ -410,6 +405,7 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
   RunLoop();
 
   // Now change the mode so nudge can execute.
+  ASSERT_TRUE(callback_called);
   ASSERT_EQ(3U, records.snapshots.size());
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
@@ -604,7 +600,7 @@ TEST_F(SyncSchedulerTest, Polling) {
   TimeTicks optimal_start = TimeTicks::Now() + poll_interval;
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
-  // Run again to wait for polling.
+  // Runn again to wait for polling.

[rlarocque, 2012/06/09 01:44:35]

typo.

[Nicolas Zea, 2012/06/11 23:05:20]

Done.

   RunLoop();
 
   StopSyncScheduler();
@@ -715,13 +711,19 @@ TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) {
   EXPECT_TRUE(RunAndGetBackoff());
 }
 
-// Test that no syncing occurs when throttled.
+// Test that no syncing occurs when throttled (although CleanupDisabledTypes
+// is allowed).
 TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   const ModelTypeSet types(syncable::BOOKMARKS);
   TimeDelta poll(TimeDelta::FromMilliseconds(5));
   TimeDelta throttle(TimeDelta::FromMinutes(10));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  EXPECT_CALL(*syncer(), SyncShare(_,Not(CLEANUP_DISABLED_TYPES),
+                                     Not(CLEANUP_DISABLED_TYPES)))
       .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle)))
       .WillRepeatedly(AddFailureAndQuitLoopNow());
 
@@ -733,8 +735,15 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  scheduler()->Configure(
-      types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      types,
+      TypesToRoutingInfo(types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_FALSE(scheduler()->Configure(params));
+  ASSERT_FALSE(callback_called);
 }
 
 TEST_F(SyncSchedulerTest, ThrottlingExpires) {
@@ -754,7 +763,7 @@ TEST_F(SyncSchedulerTest, ThrottlingExpires) {
   TimeTicks optimal_start = TimeTicks::Now() + poll + throttle1;
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
-  // Run again to wait for polling.
+  // Runn again to wait for polling.

[rlarocque, 2012/06/09 01:44:35]

typo.

[Nicolas Zea, 2012/06/11 23:05:20]

Done.

   RunLoop();
 
   StopSyncScheduler();
@@ -767,6 +776,7 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
   SyncShareRecords records;
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))

[rlarocque, 2012/06/09 01:44:35]

Why make the first invocation of SyncShare special?  Could we not record its
snapshot, too?

[Nicolas Zea, 2012/06/11 23:05:20]

This is getting removed altogether in a later patch (see
http://codereview.chromium.org/10541079/)

       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
@@ -780,8 +790,15 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
 
   const ModelTypeSet config_types(syncable::BOOKMARKS);
 
-  scheduler()->Configure(
-      config_types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      config_types,
+      TypesToRoutingInfo(config_types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_TRUE(scheduler()->Configure(params));
+  ASSERT_TRUE(callback_called);
 
   ASSERT_EQ(1U, records.snapshots.size());
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(config_types,
@@ -850,7 +867,8 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
   UseMockDelayProvider();
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1)
+  EXPECT_CALL(*syncer(),

[rlarocque, 2012/06/09 01:44:35]

This indentation looks strange.  Could you move the SyncShare part onto the
first line?

[Nicolas Zea, 2012/06/11 23:05:20]

Done.

+              SyncShare(_,_,_))
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
                             RecordSyncShareMultiple(&r, 1U)));
   EXPECT_CALL(*delay(), GetDelay(_)).
@@ -891,9 +909,15 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  scheduler()->CleanupDisabledTypes();
-  scheduler()->Configure(
-      types, GetUpdatesCallerInfo::RECONFIGURATION);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      types,
+      TypesToRoutingInfo(types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_FALSE(scheduler()->Configure(params));
+  ASSERT_FALSE(callback_called);
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
@@ -1062,27 +1086,48 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  // Configuration.
+  // Configuration (always includes a cleanup disabled types).
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  scheduler()->Configure(
-      ModelTypeSet(), GetUpdatesCallerInfo::RECONFIGURATION);
-
+  syncable::ModelTypeSet model_types(syncable::BOOKMARKS);
+  bool callback_called = false;
+  ConfigureParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      model_types,
+      TypesToRoutingInfo(model_types),
+      false,
+      base::Bind(&SetBool, &callback_called));
+  ASSERT_TRUE(scheduler()->Configure(params));
+  // Runs directly so no need to pump the loop.
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
+  ASSERT_TRUE(callback_called);
 
-  // Cleanup disabled types.
+  // Cleanup disabled types. Because no types are being configured, we just
+  // perform the cleanup.
   EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
-  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
-
-  scheduler()->CleanupDisabledTypes();
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)).
+      WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
+  bool callback2_called = false;
+  ConfigureParams params2(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      ModelTypeSet(),
+      ModelSafeRoutingInfo(),
+      false,
+      base::Bind(&SetBool, &callback2_called));
+  ASSERT_TRUE(scheduler()->Configure(params2));
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
+  ASSERT_TRUE(callback2_called);
+
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   // Poll.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))