diff options
author | Konstantin Varlamov <var-const@users.noreply.github.com> | 2018-05-10 19:18:22 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-05-10 19:18:22 -0400 |
commit | 8cde81689dcfc74c9b59e088bbe00f95ca4bf68d (patch) | |
tree | 94c8bb8874ab756e79ffa9c63d335d3ffc513fec /Firestore/core/test | |
parent | c7667229e2ab35d14fa7855789ee0584a2f8e817 (diff) |
Firestore C++: quick fix for flaky tests in AsyncQueue and Executor (#1245)
Diffstat (limited to 'Firestore/core/test')
3 files changed, 18 insertions, 6 deletions
diff --git a/Firestore/core/test/firebase/firestore/util/async_queue_test.cc b/Firestore/core/test/firebase/firestore/util/async_queue_test.cc index bcee2e3..4247584 100644 --- a/Firestore/core/test/firebase/firestore/util/async_queue_test.cc +++ b/Firestore/core/test/firebase/firestore/util/async_queue_test.cc @@ -83,16 +83,20 @@ TEST_P(AsyncQueueTest, VerifyIsCurrentQueueWorksWithOperationInProgress) { queue.EnqueueBlocking([&] { EXPECT_NO_THROW(queue.VerifyIsCurrentQueue()); }); } +// TODO(varconst): this test is inherently flaky because it can't be guaranteed +// that the enqueued asynchronous operation didn't finish before the code has +// a chance to even enqueue the next operation. Delays are chosen so that the +// test is unlikely to fail in practice. Need to revisit this. TEST_P(AsyncQueueTest, CanScheduleOperationsInTheFuture) { std::string steps; queue.Enqueue([&steps] { steps += '1'; }); queue.Enqueue([&] { - queue.EnqueueAfterDelay(AsyncQueue::Milliseconds(5), kTimerId1, [&] { + queue.EnqueueAfterDelay(AsyncQueue::Milliseconds(20), kTimerId1, [&] { steps += '4'; signal_finished(); }); - queue.EnqueueAfterDelay(AsyncQueue::Milliseconds(1), kTimerId2, + queue.EnqueueAfterDelay(AsyncQueue::Milliseconds(10), kTimerId2, [&steps] { steps += '3'; }); queue.EnqueueRelaxed([&steps] { steps += '2'; }); }); diff --git a/Firestore/core/test/firebase/firestore/util/executor_std_test.cc b/Firestore/core/test/firebase/firestore/util/executor_std_test.cc index 43cad60..59c3c32 100644 --- a/Firestore/core/test/firebase/firestore/util/executor_std_test.cc +++ b/Firestore/core/test/firebase/firestore/util/executor_std_test.cc @@ -70,7 +70,6 @@ TEST_F(ScheduleTest, PopIfDue_Delayed) { schedule.Push(2, start_time + chr::milliseconds(3)); schedule.Push(3, start_time + chr::milliseconds(1)); - EXPECT_FALSE(schedule.PopIfDue().has_value()); std::this_thread::sleep_for(chr::milliseconds(5)); EXPECT_EQ(schedule.PopIfDue().value(), 3); @@ -212,11 +211,16 @@ TEST_F(ScheduleTest, PopBlockingIsNotAffectedByIrrelevantRemovals) { EXPECT_EQ(schedule.PopBlocking(), 1); }); - while (schedule.empty()) { + // Wait (with timeout) for both values to appear in the schedule. + while (schedule.size() != 2) { + if (now() - start_time >= kTimeout) { + Abort(); + } std::this_thread::sleep_for(chr::milliseconds(1)); } const auto maybe_removed = schedule.RemoveIf([](const int v) { return v == 2; }); + ASSERT_TRUE(maybe_removed.has_value()); EXPECT_EQ(maybe_removed.value(), 2); ABORT_ON_TIMEOUT(future); } diff --git a/Firestore/core/test/firebase/firestore/util/executor_test.cc b/Firestore/core/test/firebase/firestore/util/executor_test.cc index e277786..99bddce 100644 --- a/Firestore/core/test/firebase/firestore/util/executor_test.cc +++ b/Firestore/core/test/firebase/firestore/util/executor_test.cc @@ -66,15 +66,19 @@ TEST_P(ExecutorTest, DestructorDoesNotBlockIfThereArePendingTasks) { ABORT_ON_TIMEOUT(future); } +// TODO(varconst): this test is inherently flaky because it can't be guaranteed +// that the enqueued asynchronous operation didn't finish before the code has +// a chance to even enqueue the next operation. Delays are chosen so that the +// test is unlikely to fail in practice. Need to revisit this. TEST_P(ExecutorTest, CanScheduleOperationsInTheFuture) { std::string steps; executor->Execute([&steps] { steps += '1'; }); - Schedule(executor.get(), Executor::Milliseconds(5), [&] { + Schedule(executor.get(), Executor::Milliseconds(20), [&] { steps += '4'; signal_finished(); }); - Schedule(executor.get(), Executor::Milliseconds(1), + Schedule(executor.get(), Executor::Milliseconds(10), [&steps] { steps += '3'; }); executor->Execute([&steps] { steps += '2'; }); |