From 8cde81689dcfc74c9b59e088bbe00f95ca4bf68d Mon Sep 17 00:00:00 2001
From: Konstantin Varlamov <var-const@users.noreply.github.com>
Date: Thu, 10 May 2018 19:18:22 -0400
Subject: Firestore C++: quick fix for flaky tests in AsyncQueue and Executor
 (#1245)

---
 Firestore/core/test/firebase/firestore/util/async_queue_test.cc  | 8 ++++++--
 Firestore/core/test/firebase/firestore/util/executor_std_test.cc | 8 ++++++--
 Firestore/core/test/firebase/firestore/util/executor_test.cc     | 8 ++++++--
 3 files changed, 18 insertions(+), 6 deletions(-)

(limited to 'Firestore/core/test')

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'; });
 
-- 
cgit v1.2.3