Client-side changes to support retry GU.

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 e5876554e81a53bdc4f95d95c8bac398f93bc835..091e7042cd25f50060c39e339fd6c2dd3cc2e8a9 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -34,6 +34,7 @@ using testing::Mock;
 using testing::Return;
 using testing::WithArg;
 using testing::WithArgs;
+using testing::WithoutArgs;
 
 namespace syncer {
 using sessions::SyncSession;
@@ -51,6 +52,7 @@ class MockSyncer : public Syncer {
                     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource,
                     SyncSession*));
   MOCK_METHOD2(PollSyncShare, bool(ModelTypeSet, sessions::SyncSession*));
+  MOCK_METHOD2(RetrySyncShare, bool(ModelTypeSet, sessions::SyncSession*));
 };
 
 MockSyncer::MockSyncer()
@@ -216,6 +218,11 @@ class SyncSchedulerTest : public testing::Test {
     return scheduler_->nudge_tracker_.GetThrottledTypes();
   }
 
+  base::TimeDelta GetRetryTimerDelay() {
+    EXPECT_TRUE(scheduler_->retry_timer_.IsRunning());
+    return scheduler_->retry_timer_.GetCurrentDelay();

[rlarocque, 2014/01/10 22:41:13]

This seems like a grey area in the API.  I'm not sure that it's safe to call
GetCurrentDelay() on the timer if the timer has already fired.  Though looking
at the implementation, I see no reason why it wouldn't work.

Since these checks are not really essential to your tests, I guess this is OK. 
You could always remove them if this breaks.

+  }
+
  private:
   syncable::Directory* directory() {
     return dir_maker_.directory();
@@ -545,8 +552,9 @@ TEST_F(SyncSchedulerTest, Polling) {
   SyncShareTimes times;
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
   EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples))
-      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
-           RecordSyncShareMultiple(&times, kMinNumSamples)));
+      .WillRepeatedly(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   scheduler()->OnReceivedLongPollIntervalUpdate(poll_interval);
 
@@ -565,8 +573,9 @@ TEST_F(SyncSchedulerTest, PollNotificationsDisabled) {
   SyncShareTimes times;
   TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
   EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples))
-      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
-           RecordSyncShareMultiple(&times, kMinNumSamples)));
+      .WillRepeatedly(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   scheduler()->OnReceivedShortPollIntervalUpdate(poll_interval);
   scheduler()->SetNotificationsEnabled(false);
@@ -593,7 +602,7 @@ TEST_F(SyncSchedulerTest, PollIntervalUpdate) {
               sessions::test_util::SimulatePollIntervalUpdate(poll2)),
           Return(true)))
       .WillRepeatedly(
-          DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
                 WithArg<1>(
                     RecordSyncShareMultiple(&times, kMinNumSamples))));
 
@@ -684,8 +693,9 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromPoll) {
           Return(true)))
       .RetiresOnSaturation();
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
-           RecordSyncShareMultiple(&times, kMinNumSamples)));
+      .WillRepeatedly(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   TimeTicks optimal_start = TimeTicks::Now() + poll + throttle1;
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ -1117,7 +1127,7 @@ TEST_F(SyncSchedulerTest, BackoffRelief) {
   // Now let the Poll timer do its thing.
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
       .WillRepeatedly(DoAll(
-              Invoke(sessions::test_util::SimulatePollSuccess),
+              Invoke(sessions::test_util::SimulatePollRetrySuccess),
               RecordSyncShareMultiple(&times, kMinNumSamples)));
   RunLoop();
   Mock::VerifyAndClearExpectations(syncer());
@@ -1140,9 +1150,9 @@ TEST_F(SyncSchedulerTest, TransientPollFailure) {
   UseMockDelayProvider(); // Will cause test failure if backoff is initiated.
 
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollFailed),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed),
                       RecordSyncShare(&times)))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
                       RecordSyncShare(&times)));
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ -1275,8 +1285,9 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
 
   ::testing::InSequence seq;
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
-           RecordSyncShareMultiple(&times, kMinNumSamples)));
+      .WillRepeatedly(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShareMultiple(&times, kMinNumSamples)));
 
   connection()->SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
@@ -1288,7 +1299,7 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
   // but after poll finished with auth error from poll timer it should retry
   // poll once more
   EXPECT_CALL(*syncer(), PollSyncShare(_,_))
-      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
                       RecordSyncShare(&times)));
   scheduler()->OnCredentialsUpdated();
   connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
@@ -1296,4 +1307,89 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
   StopSyncScheduler();
 }
 
+TEST_F(SyncSchedulerTest, SuccessfulRetry) {
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  SyncShareTimes times;
+  base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1);
+  scheduler()->OnReceivedGuRetryDelay(delay);
+  EXPECT_EQ(delay, GetRetryTimerDelay());
+
+  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+      .WillOnce(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShare(&times)));
+
+  // Run to wait for retrying.
+  RunLoop();
+
+  StopSyncScheduler();
+}
+
+TEST_F(SyncSchedulerTest, FailedRetry) {
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1)));
+
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1);
+  scheduler()->OnReceivedGuRetryDelay(delay);
+
+  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+      .WillOnce(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed),
+                QuitLoopNowAction()));
+
+  // Run to wait for retrying.
+  RunLoop();
+
+  EXPECT_TRUE(scheduler()->IsBackingOff());
+  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+      .WillOnce(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                QuitLoopNowAction()));
+
+  // Run to wait for second retrying.
+  RunLoop();
+
+  StopSyncScheduler();
+}
+
+ACTION_P2(VerifyRetryTimerDelay, scheduler_test, expected_delay) {
+  EXPECT_EQ(expected_delay, scheduler_test->GetRetryTimerDelay());
+}
+
+TEST_F(SyncSchedulerTest, ReceiveNewRetryDelay) {
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  SyncShareTimes times;
+  base::TimeDelta delay1 = base::TimeDelta::FromMilliseconds(10);
+  base::TimeDelta delay2 = base::TimeDelta::FromMilliseconds(20);
+
+  scheduler()->OnReceivedGuRetryDelay(delay1);
+  scheduler()->ScheduleLocalRefreshRequest(zero(), ModelTypeSet(BOOKMARKS),
+                                           FROM_HERE);
+
+  EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_))
+      .WillOnce(DoAll(
+          WithoutArgs(VerifyRetryTimerDelay(this, delay1)),

[rlarocque, 2014/01/10 22:41:13]

nit: Maybe put this into an expectation just before the sync cycle is allowed to
run (ie. just before line 1382).

[haitaol1, 2014/01/10 23:45:03]

I'd like to verify pre/post conditions tightly around NormalSyncShare to avoid
interference of other code.

[rlarocque, 2014/01/10 23:47:59]

That's valid.  I suggested alternatives because:
- I dislike GMock, and would prefer to rely on it as little as possible.
- Seeing the retry timer get verified against two different values within two
lines of each other looks a bit suspicious.

Neither argument is very strong.  You can leave it as is if you feel strongly
about it.

+          WithArg<2>(sessions::test_util::SimulateGuRetryDelayCommand(delay2)),
+          WithoutArgs(VerifyRetryTimerDelay(this, delay2)),

[rlarocque, 2014/01/10 22:41:13]

nit: I think it would be more clear to perform this second timer delay
verification a few lines below.  (ie. just after line 1382).

+          RecordSyncShare(&times)));
+
+  // Run nudge GU.
+  RunLoop();
+
+  EXPECT_CALL(*syncer(), RetrySyncShare(_,_))
+      .WillOnce(
+          DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess),
+                RecordSyncShare(&times)));
+
+  // Run to wait for retrying.
+  RunLoop();
+
+  StopSyncScheduler();
+}
+
 }  // namespace syncer