diff options
author | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-15 00:11:10 +0000 |
---|---|---|
committer | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-15 00:12:09 +0000 |
commit | b1f55bfeef8f8c87571e51a340dc51ed46cd7a0d (patch) | |
tree | 6bf39db2edc96fb2ac3fb96073e40a26cf49f8b2 /sync | |
parent | 7b5f3b5c8e4a33e01059ba0ed0036a49ec99ae77 (diff) | |
download | chromium_src-b1f55bfeef8f8c87571e51a340dc51ed46cd7a0d.zip chromium_src-b1f55bfeef8f8c87571e51a340dc51ed46cd7a0d.tar.gz chromium_src-b1f55bfeef8f8c87571e51a340dc51ed46cd7a0d.tar.bz2 |
sync: Attempt to fix sync scheduler test flakiness
Increases the delay duration in several sync scheduler unit tests.
According to [1], the timer resolution on Windows may be under 1ms. We
suspect the low timer resolution compbined with very short delays could
cause problems in some unit tests. This will make the tests slower, but
hopefully it will make them more reliable, too.
Changes a timestmap comparison in nudge_tracker.cc from < to <=. We
suspect that a lower timer resolution might allow two Time::Now() calls
that should be separated by a >1ms gap of time might still return the
same value in both calls. If this did happen, it could be the cause of
misbehaving unit tests.
It is believed that these issues only affect tests. The delay lengths
used in the real world should be much longer than 1ms. The timer
resolution should not be an issue for delays of that length.
[1] http://www.chromium.org/developers/design-documents/threading
BUG=402212
Review URL: https://codereview.chromium.org/463563005
Cr-Commit-Position: refs/heads/master@{#289731}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289731 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync')
-rw-r--r-- | sync/engine/sync_scheduler_unittest.cc | 66 | ||||
-rw-r--r-- | sync/sessions/nudge_tracker.cc | 4 |
2 files changed, 31 insertions, 39 deletions
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index 13c5441..268fae2 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -94,8 +94,18 @@ ModelSafeRoutingInfo TypesToRoutingInfo(ModelTypeSet types) { return routes; } -// Convenient to use in tests wishing to analyze SyncShare calls over time. + static const size_t kMinNumSamples = 5; + +// Test harness for the SyncScheduler. Test the delays and backoff timers used +// in response to various events. +// +// These tests execute in real time with real timers. We try to keep the +// delays short, but there is a limit to how short we can make them. The +// timers on some platforms (ie. Windows) have a timer resolution greater than +// 1ms. Using 1ms delays may result in test flakiness. +// +// See crbug.com/402212 for more info. class SyncSchedulerTest : public testing::Test { public: SyncSchedulerTest() : syncer_(NULL), delay_(NULL), weak_ptr_factory_(this) {} @@ -343,7 +353,7 @@ TEST_F(SyncSchedulerTest, Config) { TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { UseMockDelayProvider(); EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1))); + .WillRepeatedly(Return(TimeDelta::FromMilliseconds(20))); SyncShareTimes times; const ModelTypeSet model_types(BOOKMARKS); @@ -389,7 +399,7 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { TEST_F(SyncSchedulerTest, ConfigWithStop) { UseMockDelayProvider(); EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1))); + .WillRepeatedly(Return(TimeDelta::FromMilliseconds(20))); SyncShareTimes times; const ModelTypeSet model_types(BOOKMARKS); @@ -652,7 +662,7 @@ TEST_F(SyncSchedulerTest, SessionsCommitDelay) { // Test that no syncing occurs when throttled. TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) { const ModelTypeSet types(BOOKMARKS); - TimeDelta poll(TimeDelta::FromMilliseconds(5)); + TimeDelta poll(TimeDelta::FromMilliseconds(20)); TimeDelta throttle(TimeDelta::FromMinutes(10)); scheduler()->OnReceivedLongPollIntervalUpdate(poll); @@ -916,7 +926,7 @@ class BackoffTriggersSyncSchedulerTest : public SyncSchedulerTest { SyncSchedulerTest::SetUp(); UseMockDelayProvider(); EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1))); + .WillRepeatedly(Return(TimeDelta::FromMilliseconds(10))); } virtual void TearDown() { @@ -1001,7 +1011,7 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailGetEncryptionKey) { // Test that no polls or extraneous nudges occur when in backoff. TEST_F(SyncSchedulerTest, BackoffDropsJobs) { SyncShareTimes times; - TimeDelta poll(TimeDelta::FromMilliseconds(5)); + TimeDelta poll(TimeDelta::FromMilliseconds(10)); const ModelTypeSet types(BOOKMARKS); scheduler()->OnReceivedLongPollIntervalUpdate(poll); UseMockDelayProvider(); @@ -1027,7 +1037,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) { // Try (and fail) to schedule a nudge. scheduler()->ScheduleLocalNudge( - base::TimeDelta::FromMilliseconds(1), + base::TimeDelta::FromMilliseconds(10), types, FROM_HERE); @@ -1063,10 +1073,10 @@ TEST_F(SyncSchedulerTest, BackoffElevation) { RecordSyncShareMultiple(×, kMinNumSamples))); const TimeDelta first = TimeDelta::FromSeconds(kInitialBackoffRetrySeconds); - const TimeDelta second = TimeDelta::FromMilliseconds(2); - const TimeDelta third = TimeDelta::FromMilliseconds(3); - const TimeDelta fourth = TimeDelta::FromMilliseconds(4); - const TimeDelta fifth = TimeDelta::FromMilliseconds(5); + const TimeDelta second = TimeDelta::FromMilliseconds(20); + const TimeDelta third = TimeDelta::FromMilliseconds(30); + const TimeDelta fourth = TimeDelta::FromMilliseconds(40); + const TimeDelta fifth = TimeDelta::FromMilliseconds(50); const TimeDelta sixth = TimeDelta::FromDays(1); EXPECT_CALL(*delay(), GetDelay(first)).WillOnce(Return(second)) @@ -1099,7 +1109,7 @@ TEST_F(SyncSchedulerTest, BackoffRelief) { scheduler()->OnReceivedLongPollIntervalUpdate(poll); UseMockDelayProvider(); - const TimeDelta backoff = TimeDelta::FromMilliseconds(5); + const TimeDelta backoff = TimeDelta::FromMilliseconds(10); EXPECT_CALL(*delay(), GetDelay(_)).WillOnce(Return(backoff)); // Optimal start for the post-backoff poll party. @@ -1149,7 +1159,7 @@ TEST_F(SyncSchedulerTest, BackoffRelief) { // subsequent poll attempts, nor should they trigger a backoff/retry. TEST_F(SyncSchedulerTest, TransientPollFailure) { SyncShareTimes times; - const TimeDelta poll_interval(TimeDelta::FromMilliseconds(1)); + const TimeDelta poll_interval(TimeDelta::FromMilliseconds(10)); scheduler()->OnReceivedLongPollIntervalUpdate(poll_interval); UseMockDelayProvider(); // Will cause test failure if backoff is initiated. @@ -1311,17 +1321,11 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) { StopSyncScheduler(); } -#if defined(OS_WIN) -// Times out: http://crbug.com/402212 -#define MAYBE_SuccessfulRetry DISABLED_SuccessfulRetry -#else -#define MAYBE_SuccessfulRetry SuccessfulRetry -#endif -TEST_F(SyncSchedulerTest, MAYBE_SuccessfulRetry) { +TEST_F(SyncSchedulerTest, SuccessfulRetry) { StartSyncScheduler(SyncScheduler::NORMAL_MODE); SyncShareTimes times; - base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1); + base::TimeDelta delay = base::TimeDelta::FromMilliseconds(10); scheduler()->OnReceivedGuRetryDelay(delay); EXPECT_EQ(delay, GetRetryTimerDelay()); @@ -1336,20 +1340,14 @@ TEST_F(SyncSchedulerTest, MAYBE_SuccessfulRetry) { StopSyncScheduler(); } -#if defined(OS_WIN) -// Times out: http://crbug.com/402212 -#define MAYBE_FailedRetry DISABLED_FailedRetry -#else -#define MAYBE_FailedRetry FailedRetry -#endif -TEST_F(SyncSchedulerTest, MAYBE_FailedRetry) { +TEST_F(SyncSchedulerTest, FailedRetry) { UseMockDelayProvider(); EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1))); + .WillRepeatedly(Return(TimeDelta::FromMilliseconds(10))); StartSyncScheduler(SyncScheduler::NORMAL_MODE); - base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1); + base::TimeDelta delay = base::TimeDelta::FromMilliseconds(10); scheduler()->OnReceivedGuRetryDelay(delay); EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_)) @@ -1376,13 +1374,7 @@ ACTION_P2(VerifyRetryTimerDelay, scheduler_test, expected_delay) { EXPECT_EQ(expected_delay, scheduler_test->GetRetryTimerDelay()); } -#if defined(OS_WIN) -// Times out: http://crbug.com/402212 -#define MAYBE_ReceiveNewRetryDelay DISABLED_ReceiveNewRetryDelay -#else -#define MAYBE_ReceiveNewRetryDelay ReceiveNewRetryDelay -#endif -TEST_F(SyncSchedulerTest, MAYBE_ReceiveNewRetryDelay) { +TEST_F(SyncSchedulerTest, ReceiveNewRetryDelay) { StartSyncScheduler(SyncScheduler::NORMAL_MODE); SyncShareTimes times; diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc index a0bdbda..d9aaec6 100644 --- a/sync/sessions/nudge_tracker.cc +++ b/sync/sessions/nudge_tracker.cc @@ -63,7 +63,7 @@ bool NudgeTracker::IsRetryRequired() const { if (current_retry_time_.is_null()) return false; - return current_retry_time_ < sync_cycle_start_time_; + return current_retry_time_ <= sync_cycle_start_time_; } void NudgeTracker::RecordSuccessfulSyncCycle() { @@ -300,7 +300,7 @@ void NudgeTracker::SetSyncCycleStartTime(base::TimeTicks now) { // it is ready to go, then we set it as the current_retry_time_. It will stay // there until a GU retry has succeeded. if (!next_retry_time_.is_null() && - next_retry_time_ < sync_cycle_start_time_) { + next_retry_time_ <= sync_cycle_start_time_) { current_retry_time_ = next_retry_time_; next_retry_time_ = base::TimeTicks(); } |