sync: Make nudge + config jobs reliable in SyncerThread2

chrome/browser/sync/engine/syncer_thread2_unittest.cc

 // Copyright (c) 2011 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/synchronization/waitable_event.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/sync/engine/mock_model_safe_workers.h"
 #include "chrome/browser/sync/engine/syncer.h"
 #include "chrome/browser/sync/engine/syncer_thread2.h"
 #include "chrome/browser/sync/sessions/test_util.h"
@@ skipping 29 matching lines @@
 namespace s3 {
 
 // Used when tests want to record syncing activity to examine later.
 struct SyncShareRecords {
   std::vector<TimeTicks> times;
   std::vector<linked_ptr<SyncSessionSnapshot> > snapshots;
 };
 
 // Convenient to use in tests wishing to analyze SyncShare calls over time.
 static const size_t kMinNumSamples = 5;
-
+struct SyncerThread::SyncSessionJob;
+struct SyncerThread::WaitInterval;
 class SyncerThread2Test : public testing::Test {
  public:
   class MockDelayProvider : public SyncerThread::DelayProvider {
    public:
     MOCK_METHOD1(GetDelay, TimeDelta(const TimeDelta&));
   };
 
   virtual void SetUp() {
     syncdb_.SetUp();
     syncer_ = new MockSyncer();
     delay_ = NULL;
     registrar_.reset(MockModelSafeWorkerRegistrar::PassiveBookmarks());
     connection_.reset(new MockConnectionManager(syncdb_.manager(), "Test"));
     connection_->SetServerReachable();
     context_ = new SyncSessionContext(connection_.get(), syncdb_.manager(),
         registrar_.get(), std::vector<SyncEngineEventListener*>());
     context_->set_notifications_enabled(true);
     context_->set_account_name("Test");
     syncer_thread_.reset(new SyncerThread(context_, syncer_));
   }
 
+  virtual void SetUpWithTypes(syncable::ModelTypeBitSet types) {
+    syncdb_.SetUp();
+    syncer_ = new MockSyncer();
+    delay_ = NULL;
+    registrar_.reset(MockModelSafeWorkerRegistrar::SetPassiveTypes(types));
+    connection_.reset(new MockConnectionManager(syncdb_.manager(), "Test"));
+    connection_->SetServerReachable();
+    context_ = new SyncSessionContext(connection_.get(), syncdb_.manager(),
+        registrar_.get(), std::vector<SyncEngineEventListener*>());
+    context_->set_notifications_enabled(true);
+    context_->set_account_name("Test");
+    syncer_thread_.reset(new SyncerThread(context_, syncer_));
+  }
+
   SyncerThread* syncer_thread() { return syncer_thread_.get(); }
   MockSyncer* syncer() { return syncer_; }
   MockDelayProvider* delay() { return delay_; }
   MockConnectionManager* connection() { return connection_.get(); }
   TimeDelta zero() { return TimeDelta::FromSeconds(0); }
   TimeDelta timeout() {
     return TimeDelta::FromMilliseconds(TestTimeouts::action_timeout_ms());
   }
 
   virtual void TearDown() {
@@ skipping 66 matching lines @@
       if (!lhs.test(i->first))
         return false;
     }
     if (lhs.count() != count)
       return false;
     return true;
   }
 
   SyncSessionContext* context() { return context_; }
 
+  void SetMode(SyncerThread::Mode mode) {
+    syncer_thread_->mode_ = mode;

[tim (not reviewing), 2011/04/09 23:52:58]

it seems bad / dangerous to have all these setters here.
The tests in this file should be doing it the normal, non-racy way by using the
public API.  These tests are not intended to be as white-box as these methods
allow.

If these functions are only needed for a subset of the tests (e.g. the ones
testing decidejob), then at the very least they should be part of a separate
test fixture, or else I guarantee one day someone will try and use these in the
normal tests.  I'd almost say we should move the new ones to a separate file. 
But at a minimu, we need a separate test fixture

e.g. class SyncerThread2JobDecisionsTest { {

[lipalani1, 2011/04/12 02:33:00]

Moved to a different file.
On 2011/04/09 23:52:58, timsteele wrote:
Done.

+  }
+
+  void SetLastSyncedTime(base::TimeTicks ticks) {
+    syncer_thread_->last_sync_session_end_time_ = ticks;
+  }
+
+  void SetServerConnection(bool connected) {
+    syncer_thread_->server_connection_ok_ = connected;
+  }
+
+  void ResetWaitInterval() {
+    syncer_thread_->wait_interval_.reset();
+  }
+
+  void SetWaitIntervalToThrottled() {

[tim (not reviewing), 2011/04/09 23:52:58]

the existing tests would set this state naturally with test_util.  It's
confusing to have both approaches used.

[lipalani1, 2011/04/12 02:33:00]

This is moved to a different file. It is needed there for white box testing.
On 2011/04/09 23:52:58, timsteele wrote:

+    syncer_thread_->wait_interval_.reset(new SyncerThread::WaitInterval(
+        SyncerThread::WaitInterval::THROTTLED, TimeDelta::FromSeconds(1)));
+  }
+
+  void SetWaitIntervalToExponentialBackoff() {
+    syncer_thread_->wait_interval_.reset(
+       new SyncerThread::WaitInterval(
+       SyncerThread::WaitInterval::EXPONENTIAL_BACKOFF,
+       TimeDelta::FromSeconds(1)));
+  }
+
+  SyncerThread::JobProcessDecision DecideOnJob(
+      const SyncerThread::SyncSessionJob& job) {
+    return syncer_thread_->DecideOnJob(job);
+  }
+
+  void InitializeSyncerOnNormalMode() {
+    SetMode(SyncerThread::NORMAL_MODE);
+    ResetWaitInterval();
+    SetServerConnection(true);
+    SetLastSyncedTime(base::TimeTicks::Now());
+  }
+
+  SyncerThread::JobProcessDecision CreateAndDecideJob(
+      SyncerThread::SyncSessionJobPurpose purpose) {
+    struct SyncerThread::SyncSessionJob job;
+    job.purpose = purpose;
+    job.scheduled_start = TimeTicks::Now();
+    return DecideOnJob(job);
+  }
+
+ protected:
+  scoped_ptr<SyncerThread> syncer_thread_;
+
  private:
-  scoped_ptr<SyncerThread> syncer_thread_;
   scoped_ptr<MockConnectionManager> connection_;
   SyncSessionContext* context_;
   MockSyncer* syncer_;
   MockDelayProvider* delay_;
   scoped_ptr<MockModelSafeWorkerRegistrar> registrar_;
   MockDirectorySetterUpper syncdb_;
 };
 
 bool RecordSyncShareImpl(SyncSession* s, SyncShareRecords* record,
                          size_t signal_after) {
@@ skipping 50 matching lines @@
                                  FROM_HERE);
   done.TimedWait(timeout());
 
   EXPECT_EQ(1U, records2.snapshots.size());
   EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types,
       records2.snapshots[0]->source.types));
   EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
             records2.snapshots[0]->source.updates_source);
 }
 
+TEST_F(SyncerThread2Test, Config) {
+  base::WaitableEvent done(false, false);
+  SyncShareRecords records;
+  syncable::ModelTypeBitSet model_types;
+  model_types[syncable::BOOKMARKS] = true;
+
+  syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL);
+
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))

[tim (not reviewing), 2011/04/09 23:52:58]

In general it's bad to have the expectations before the call to Start as google
mock is not threadsafe.

[lipalani1, 2011/04/12 02:33:00]

Good point. Even if they were thread safe expectations need to be set before we
call start.
On 2011/04/09 23:52:58, timsteele wrote:
Done.

+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done))));
+  syncer_thread()->ScheduleConfig(model_types);
+  done.TimedWait(timeout());
+
+  EXPECT_EQ(1U, records.snapshots.size());
+  EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types,
+      records.snapshots[0]->source.types));
+  EXPECT_EQ(GetUpdatesCallerInfo::FIRST_UPDATE,
+            records.snapshots[0]->source.updates_source);
+}
+
+TEST_F(SyncerThread2Test, ConfigWithBackingOff) {
+  base::WaitableEvent done(false, false);
+  base::WaitableEvent* dummy = NULL;
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1)));
+  SyncShareRecords records;
+  syncable::ModelTypeBitSet model_types;
+  model_types[syncable::BOOKMARKS] = true;
+
+  syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL);
+
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+          WithArg<0>(RecordSyncShare(&records, 1U, dummy))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done))));
+  syncer_thread()->ScheduleConfig(model_types);
+  done.TimedWait(timeout());
+
+  EXPECT_EQ(2U, records.snapshots.size());
+  EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types,
+      records.snapshots[1]->source.types));
+  EXPECT_EQ(GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION,
+            records.snapshots[1]->source.updates_source);
+}
+
+TEST_F(SyncerThread2Test, MultipleConfigWithBackingOff) {
+  syncable::ModelTypeBitSet model_types1, model_types2;
+  model_types1[syncable::BOOKMARKS] = true;
+  model_types2[syncable::AUTOFILL] = true;
+  SetUpWithTypes(model_types1 | model_types2);
+  base::WaitableEvent done(false, false);
+  base::WaitableEvent done1(false, false);
+  base::WaitableEvent* dummy = NULL;
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(30)));
+  SyncShareRecords records;
+
+  syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL);
+
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+          WithArg<0>(RecordSyncShare(&records, 1U, dummy))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done1))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done))));
+  syncer_thread()->ScheduleConfig(model_types1);
+  done1.TimedWait(timeout());
+  syncer_thread()->ScheduleConfig(model_types2);
+  done.TimedWait(timeout());
+

[tim (not reviewing), 2011/04/09 23:52:58]

how come all these tests now wait after each post... seems rather simulated

[lipalani1, 2011/04/12 02:33:00]

Comments added explaining the rationale for them.
On 2011/04/09 23:52:58, timsteele wrote:

+  EXPECT_EQ(3U, records.snapshots.size());
+  EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types2,
+      records.snapshots[2]->source.types));
+  EXPECT_EQ(GetUpdatesCallerInfo::FIRST_UPDATE,
+            records.snapshots[2]->source.updates_source);
+}
+
+TEST_F(SyncerThread2Test, NudgeWithConfigWithBackingOff) {

[tim (not reviewing), 2011/04/09 23:52:58]

it's not clear what is being tested here versus BackoffDropsJobs.

Note the existing tests had comments explaining what was going on.  Please do
the same, and try to contrast the differences.

[lipalani1, 2011/04/12 02:33:00]

I have added comments. In short backoffdropjobs test backing off of a nudge(in
normal mode) whereas this puts config in backing off state and induces a nudge.
On 2011/04/09 23:52:58, timsteele wrote:

+  syncable::ModelTypeBitSet model_types;
+  model_types[syncable::BOOKMARKS] = true;
+  base::WaitableEvent done(false, false);
+  base::WaitableEvent done1(false, false);
+  base::WaitableEvent done2(false, false);
+  base::WaitableEvent* dummy = NULL;
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(50)));
+  SyncShareRecords records;
+
+  syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL);
+
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+          WithArg<0>(RecordSyncShare(&records, 1U, dummy))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done1))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done2))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+          WithArg<0>(RecordSyncShare(&records, 1U, &done))));
+
+  syncer_thread()->ScheduleConfig(model_types);
+  done1.TimedWait(timeout());
+  syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, model_types,
+                                 FROM_HERE);
+  done2.TimedWait(timeout());
+  syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL);
+  done.TimedWait(timeout());
+  EXPECT_EQ(4U, records.snapshots.size());
+
+  EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types,
+      records.snapshots[2]->source.types));
+  EXPECT_EQ(GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION,
+            records.snapshots[2]->source.updates_source);
+
+  EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(model_types,
+      records.snapshots[3]->source.types));
+  EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
+            records.snapshots[3]->source.updates_source);
+
+}
+
+
 // Test that nudges are coalesced.
 TEST_F(SyncerThread2Test, NudgeCoalescing) {
   syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL);
   base::WaitableEvent done(false, false);
   SyncShareRecords r;
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
            WithArg<0>(RecordSyncShare(&r, 1U, &done))));
   syncable::ModelTypeBitSet types1, types2, types3;
   types1[syncable::BOOKMARKS] = true;
@@ skipping 233 matching lines @@
 }
 
 // Test nudges / polls don't run in config mode and config tasks do.
 TEST_F(SyncerThread2Test, ConfigurationMode) {
   TimeDelta poll(TimeDelta::FromMilliseconds(15));
   SyncShareRecords records;
   base::WaitableEvent done(false, false);
   base::WaitableEvent* dummy = NULL;
   syncer_thread()->OnReceivedLongPollIntervalUpdate(poll);
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+      .WillOnce((Invoke(sessions::test_util::SimulateSuccess),
            WithArg<0>(RecordSyncShare(&records, 1U, dummy))));
   syncer_thread()->Start(SyncerThread::CONFIGURATION_MODE, NULL);
+  FlushLastTask(&done);

[tim (not reviewing), 2011/04/09 23:52:58]

Why are there calls to this sprinkled over this test?  They appear to serialize
all requests and this was trying to test multiple at the same time.

[lipalani1, 2011/04/12 02:33:00]

argh. failed to remove something I was playing with.
On 2011/04/09 23:52:58, timsteele wrote:

   syncable::ModelTypeBitSet nudge_types;
   nudge_types[syncable::AUTOFILL] = true;
   syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, nudge_types,
                                  FROM_HERE);
+    FlushLastTask(&done);
   syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, nudge_types,
                                  FROM_HERE);
 
+  FlushLastTask(&done);
   syncable::ModelTypeBitSet config_types;
   config_types[syncable::BOOKMARKS] = true;
   // TODO(tim): This will fail once CONFIGURATION tasks are implemented. Update
   // the EXPECT when that happens.
   syncer_thread()->ScheduleConfig(config_types);
   FlushLastTask(&done);
   syncer_thread()->Stop();
 
   EXPECT_EQ(1U, records.snapshots.size());
   EXPECT_TRUE(CompareModelTypeBitSetToModelTypePayloadMap(config_types,
@@ skipping 233 matching lines @@
 // TODO(tim): Implement this test and then the functionality!
 TEST_F(SyncerThread2Test, DISABLED_NoConfigDuringNormal) {
 }
 
 // Test that starting the syncer thread without a valid connection doesn't
 // break things when a connection is detected.
 TEST_F(SyncerThread2Test, StartWhenNotConnected) {
   base::WaitableEvent done(false, false);
   MessageLoop cur;
   connection()->SetServerNotReachable();
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done));
   syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL);
   syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet(),
                                  FROM_HERE);
   FlushLastTask(&done);
 
   connection()->SetServerReachable();
   cur.PostTask(FROM_HERE, NewRunnableFunction(
       &SyncerThread2Test::QuitMessageLoop));
   cur.Run();
 
   // By now, the server connection event should have been posted to the
   // SyncerThread.
   FlushLastTask(&done);
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done));
-  syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet(),
-                                 FROM_HERE);
   done.TimedWait(timeout());
 }
 
 TEST_F(SyncerThread2Test, SetsPreviousRoutingInfo) {
   base::WaitableEvent done(false, false);
   ModelSafeRoutingInfo info;
   EXPECT_TRUE(info == context()->previous_session_routing_info());
   ModelSafeRoutingInfo expected;
   context()->registrar()->GetModelSafeRoutingInfo(&expected);
   ASSERT_FALSE(expected.empty());
   EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1);
 
   syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL);
   syncer_thread()->ScheduleNudge(zero(), NUDGE_SOURCE_LOCAL, ModelTypeBitSet(),
                                  FROM_HERE);
   FlushLastTask(&done);
   syncer_thread()->Stop();
 
   EXPECT_TRUE(expected == context()->previous_session_routing_info());
 }
 
+TEST_F(SyncerThread2Test, SaveNudge) {
+  InitializeSyncerOnNormalMode();
+
+  // Now set the mode to configure.
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+
+  SyncerThread::JobProcessDecision decision =
+      CreateAndDecideJob(SyncerThread::NUDGE);
+
+  DCHECK_EQ(decision, SyncerThread::SAVE);
+}
+
+TEST_F(SyncerThread2Test, ContinueNudge) {
+  InitializeSyncerOnNormalMode();
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::NUDGE);
+
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
+TEST_F(SyncerThread2Test, DropPoll) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::POLL);
+
+  DCHECK_EQ(decision, SyncerThread::DROP);
+}
+
+TEST_F(SyncerThread2Test, ContinuePoll) {
+  InitializeSyncerOnNormalMode();
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::POLL);
+
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
+TEST_F(SyncerThread2Test, ContinueConfiguration) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::CONFIGURATION);
+
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
+TEST_F(SyncerThread2Test, SaveConfigurationWhileThrottled) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+
+  SetWaitIntervalToThrottled();
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::CONFIGURATION);
+
+  DCHECK_EQ(decision, SyncerThread::SAVE);
+}
+
+TEST_F(SyncerThread2Test, SaveNudgeWhileThrottled) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+
+  SetWaitIntervalToThrottled();
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::NUDGE);
+
+  DCHECK_EQ(decision, SyncerThread::SAVE);
+
+}
+
+TEST_F(SyncerThread2Test, ContinueClearUserDataUnderAllCircumstances) {
+  InitializeSyncerOnNormalMode();
+
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+  SetWaitIntervalToThrottled();
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::CLEAR_USER_DATA);
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+
+  SetMode(SyncerThread::NORMAL_MODE);
+  SetWaitIntervalToExponentialBackoff();
+  decision = CreateAndDecideJob(
+      SyncerThread::CLEAR_USER_DATA);
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
+TEST_F(SyncerThread2Test, ContinueNudgeWhileExponentialBackOff) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::NORMAL_MODE);
+  SetWaitIntervalToExponentialBackoff();
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::NUDGE);
+
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
+TEST_F(SyncerThread2Test, DropNudgeWhileExponentialBackOff) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::NORMAL_MODE);
+  SetWaitIntervalToExponentialBackoff();
+
+  syncer_thread_->wait_interval_->had_nudge = true;
+
+  SyncerThread::JobProcessDecision decision = CreateAndDecideJob(
+      SyncerThread::NUDGE);
+
+  DCHECK_EQ(decision, SyncerThread::DROP);
+}
+
+TEST_F(SyncerThread2Test, ContinueCanaryJobConfig) {
+  InitializeSyncerOnNormalMode();
+  SetMode(SyncerThread::CONFIGURATION_MODE);
+  SetWaitIntervalToExponentialBackoff();
+
+  struct SyncerThread::SyncSessionJob job;
+  job.purpose = SyncerThread::CONFIGURATION;
+  job.scheduled_start = TimeTicks::Now();
+  job.is_canary_job = true;
+  SyncerThread::JobProcessDecision decision = DecideOnJob(job);
+
+  DCHECK_EQ(decision, SyncerThread::CONTINUE);
+}
+
 }  // namespace s3
 }  // namespace browser_sync
 
 // SyncerThread won't outlive the test!
 DISABLE_RUNNABLE_METHOD_REFCOUNT(browser_sync::s3::SyncerThread2Test);