[Sync] Refactor sync configuration logic.

sync/engine/sync_scheduler_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.
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/test/test_timeouts.h"
 #include "sync/engine/sync_scheduler.h"
 #include "sync/engine/syncer.h"
 #include "sync/sessions/test_util.h"
 #include "sync/test/engine/fake_model_worker.h"
 #include "sync/test/engine/mock_connection_manager.h"
 #include "sync/test/engine/test_directory_setter_upper.h"
 #include "sync/test/fake_extensions_activity_monitor.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::TimeDelta;
 using base::TimeTicks;
 using testing::_;
 using testing::AtLeast;
 using testing::DoAll;
 using testing::Eq;
 using testing::Invoke;
 using testing::Mock;
+using testing::Not;
 using testing::Return;
 using testing::WithArg;
 
 namespace browser_sync {
 using sessions::SyncSession;
 using sessions::SyncSessionContext;
 using sessions::SyncSessionSnapshot;
 using syncable::ModelTypeSet;
 using sync_pb::GetUpdatesCallerInfo;
 
@@ skipping 22 matching lines @@
 }
 
 void PumpLoop() {
   // Do it this way instead of RunAllPending to pump loop exactly once
   // (necessary in the presence of timers; see comment in
   // QuitLoopNow).
   MessageLoop::current()->PostTask(FROM_HERE, base::Bind(&QuitLoopNow));
   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 {
  public:
   SyncSchedulerTest()
       : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
         context_(NULL),
         syncer_(NULL),
         delay_(NULL) {}
 
@@ skipping 62 matching lines @@
       EXPECT_EQ(GetUpdatesCallerInfo::PERIODIC,
                 records.snapshots[i].source().updates_source);
     }
   }
 
   void DoQuitLoopNow() {
     QuitLoopNow();
   }
 
   void StartSyncScheduler(SyncScheduler::Mode mode) {
-    scheduler()->Start(mode, base::Closure());
+    scheduler()->Start(mode);
   }
 
   // This stops the scheduler synchronously.
   void StopSyncScheduler() {
     scheduler()->RequestStop(base::Bind(&SyncSchedulerTest::DoQuitLoopNow,
                              weak_ptr_factory_.GetWeakPtr()));
     RunLoop();
   }
 
   bool RunAndGetBackoff() {
@@ skipping 117 matching lines @@
   EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
             records2.snapshots[0].source().updates_source);
 }
 
 // Make sure a regular config command is scheduled fine in the absence of any
 // errors.
 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,
       records.snapshots[0].source().types));
   EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
             records.snapshots[0].source().updates_source);
 }
 
 // Simulate a failure and make sure the config request is retried.
 TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   UseMockDelayProvider();
   EXPECT_CALL(*delay(), GetDelay(_))
       .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1)));
   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),
                       WithArg<0>(RecordSyncShare(&records))));
 
   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) {
   const ModelTypeSet model_types(syncable::BOOKMARKS);
   UseMockDelayProvider();
   EXPECT_CALL(*delay(), GetDelay(_))
       .WillRepeatedly(Return(TimeDelta::FromMilliseconds(50)));
   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))))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
   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(
       zero(), NUDGE_SOURCE_LOCAL, model_types, FROM_HERE);
   RunLoop();
 
   ASSERT_EQ(2U, records.snapshots.size());

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

Could you ASSERT_FALSE(callback_called) here?

I think the records.snapshots.size() made a lot more sense when we were picking
up on the configure done by examining snapshots in SyncBackendHost.  We probably
should rely less on those kinds of asserts going forward.  

I'm not sure we should start deleting them yet.  They aren't causing any harm. 
However, we should have their replacements ready to go in case we do decide to
get rid of them.

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

Done.

I think the snapshot size is still a useful way of tracking the sync cycles.
Yes, we don't rely on the sync cycle completion event for configuration anymore,
but we still have expectations about the cycles occurring and how often.

   RunLoop();
 
   // Now change the mode so nudge can execute.
+  ASSERT_TRUE(callback_called);
   ASSERT_EQ(3U, records.snapshots.size());
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   ASSERT_EQ(4U, records.snapshots.size());
 
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types,
       records.snapshots[2].source().types));
   EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
             records.snapshots[2].source().updates_source);
 
@@ skipping 174 matching lines @@
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
   EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(AtLeast(kMinNumSamples))
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
            WithArg<0>(RecordSyncShareMultiple(&records, kMinNumSamples))));
 
   scheduler()->OnReceivedLongPollIntervalUpdate(poll_interval);
 
   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();
   AnalyzePollRun(records, kMinNumSamples, optimal_start, poll_interval);
 }
 
 // Test that the short poll interval is used.
 TEST_F(SyncSchedulerTest, PollNotificationsDisabled) {
   SyncShareRecords records;
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
@@ skipping 90 matching lines @@
                     QuitLoopNowAction()));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, ModelTypeSet(), FROM_HERE);
 
   // We should detect the failure on the second sync share, and go into backoff.
   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());
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, types, FROM_HERE);
   PumpLoop();
 
   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) {
   SyncShareRecords records;
   TimeDelta poll(TimeDelta::FromMilliseconds(15));
   TimeDelta throttle1(TimeDelta::FromMilliseconds(150));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
 
   ::testing::InSequence seq;
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle1)))
       .RetiresOnSaturation();
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
            WithArg<0>(RecordSyncShareMultiple(&records, kMinNumSamples))));
 
   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();
   AnalyzePollRun(records, kMinNumSamples, optimal_start, poll);
 }
 
 // Test nudges / polls don't run in config mode and config tasks do.
 TEST_F(SyncSchedulerTest, ConfigurationMode) {
   TimeDelta poll(TimeDelta::FromMilliseconds(15));
   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))));
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   const ModelTypeSet nudge_types(syncable::AUTOFILL);
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, nudge_types, FROM_HERE);
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, nudge_types, FROM_HERE);
 
   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,
       records.snapshots[0].source().types));
 }
 
 class BackoffTriggersSyncSchedulerTest : public SyncSchedulerTest {
   void SetUp() {
     SyncSchedulerTest::SetUp();
     UseMockDelayProvider();
@@ skipping 48 matching lines @@
 }
 
 // Test that no polls or extraneous nudges occur when in backoff.
 TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   SyncShareRecords r;
   TimeDelta poll(TimeDelta::FromMilliseconds(5));
   const ModelTypeSet types(syncable::BOOKMARKS);
   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(_)).
       WillRepeatedly(Return(TimeDelta::FromDays(1)));
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   // This nudge should fail and put us into backoff.  Thanks to our mock
   // GetDelay() setup above, this will be a long backoff.
   scheduler()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, types, FROM_HERE);
@@ skipping 20 matching lines @@
             r.snapshots[1].source().updates_source);
 
   // Cleanup is not affected by backoff, but it should not relieve it either.
   EXPECT_CALL(*syncer(),
               SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*delay(), GetDelay(_)).Times(0);
 
   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);
 
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, types, FROM_HERE);
   scheduler()->ScheduleNudge(
       zero(), NUDGE_SOURCE_LOCAL, types, FROM_HERE);
   PumpLoop();
 }
 
@@ skipping 148 matching lines @@
   // ClearUserData.
   EXPECT_CALL(*syncer(), SyncShare(_, CLEAR_PRIVATE_DATA, CLEAR_PRIVATE_DATA))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   scheduler()->ClearUserData();
 
   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);
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)).
+      WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  scheduler()->CleanupDisabledTypes();
-
+  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))
       .Times(AtLeast(1))
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                             QuitLoopNowAction()));
   const TimeDelta poll(TimeDelta::FromMilliseconds(10));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ skipping 45 matching lines @@
   PumpLoop();
   // Pump again to run job.
   PumpLoop();
 
   StopSyncScheduler();
 
   EXPECT_TRUE(expected == context()->previous_session_routing_info());
 }
 
 }  // namespace browser_sync