From e3fd8c27fb1053e4c911e090c292561a58927483 Mon Sep 17 00:00:00 2001
From: Adam Simpkins <simpkins@fb.com>
Date: Tue, 30 Jan 2018 14:31:25 -0800
Subject: [PATCH] fix flaky time handling and other issues in
 EventBaseTest.IdleTime

Summary:
Update EventBaseTest.IdleTime() to drive the loop once before starting the bulk
of the test logic.  This triggers the initial NotificationQueue callbacks which
can otherwise slow down the test slightly and make it more prone to failing due
to not accounting for the time required for these callbacks.

Also replace `ASSERT_*()` checks inside a lambda with `EXPECT_*()`.
The googletest `ASSERT_*()` macros only work directly inside the main test
function, as they `return` from the current function.  When used inside the
lambda these skip the remainder of the lambda but continue running the
remainder of the test checks, which causes incorrect failures.

Reviewed By: yfeldblum

Differential Revision: D6843335

fbshipit-source-id: 879901edb6e5a5a84736705fb2d1cb8d2e0cc252
---
 folly/io/async/test/EventBaseTest.cpp | 36 ++++++++++++++++-----------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/folly/io/async/test/EventBaseTest.cpp b/folly/io/async/test/EventBaseTest.cpp
index ac2ff0c15..de1149c14 100644
--- a/folly/io/async/test/EventBaseTest.cpp
+++ b/folly/io/async/test/EventBaseTest.cpp
@@ -1540,11 +1540,11 @@ class IdleTimeTimeoutSeries : public AsyncTimeout {
     timeouts_(0),
     timeout_(timeout) {
       scheduleTimeout(1);
-    }
+  }
 
-    ~IdleTimeTimeoutSeries() override {}
+  ~IdleTimeTimeoutSeries() override {}
 
-    void timeoutExpired() noexcept override {
+  void timeoutExpired() noexcept override {
     ++timeouts_;
 
     if(timeout_.empty()){
@@ -1579,18 +1579,23 @@ class IdleTimeTimeoutSeries : public AsyncTimeout {
  */
 TEST(EventBaseTest, IdleTime) {
   EventBase eventBase;
-  eventBase.setLoadAvgMsec(1000ms);
-  eventBase.resetLoadAvg(5900.0);
   std::deque<uint64_t> timeouts0(4, 8080);
   timeouts0.push_front(8000);
   timeouts0.push_back(14000);
   IdleTimeTimeoutSeries tos0(&eventBase, timeouts0);
   std::deque<uint64_t> timeouts(20, 20);
   std::unique_ptr<IdleTimeTimeoutSeries> tos;
-  int64_t testStart = duration_cast<microseconds>(
-    std::chrono::steady_clock::now().time_since_epoch()).count();
   bool hostOverloaded = false;
 
+  // Loop once before starting the main test.  This will run NotificationQueue
+  // callbacks that get automatically installed when the EventBase is first
+  // created.  We want to make sure they don't interfere with the timing
+  // operations below.
+  eventBase.loopOnce(EVLOOP_NONBLOCK);
+  eventBase.setLoadAvgMsec(1000ms);
+  eventBase.resetLoadAvg(5900.0);
+  auto testStart = std::chrono::steady_clock::now();
+
   int latencyCallbacks = 0;
   eventBase.setMaxLatency(6000us, [&]() {
     ++latencyCallbacks;
@@ -1601,25 +1606,26 @@ TEST(EventBaseTest, IdleTime) {
     if (tos0.getTimeouts() < 6) {
       // This could only happen if the host this test is running
       // on is heavily loaded.
-      int64_t maxLatencyReached = duration_cast<microseconds>(
-          std::chrono::steady_clock::now().time_since_epoch()).count();
-      ASSERT_LE(43800, maxLatencyReached - testStart);
+      int64_t usElapsed = duration_cast<microseconds>(
+                              std::chrono::steady_clock::now() - testStart)
+                              .count();
+      EXPECT_LE(43800, usElapsed);
       hostOverloaded = true;
       return;
     }
-    ASSERT_EQ(6, tos0.getTimeouts());
-    ASSERT_GE(6100, eventBase.getAvgLoopTime() - 1200);
-    ASSERT_LE(6100, eventBase.getAvgLoopTime() + 1200);
+    EXPECT_EQ(6, tos0.getTimeouts());
+    EXPECT_GE(6100, eventBase.getAvgLoopTime() - 1200);
+    EXPECT_LE(6100, eventBase.getAvgLoopTime() + 1200);
     tos = std::make_unique<IdleTimeTimeoutSeries>(&eventBase, timeouts);
   });
 
-  // Kick things off with an "immedite" timeout
+  // Kick things off with an "immediate" timeout
   tos0.scheduleTimeout(1);
 
   eventBase.loop();
 
   if (hostOverloaded) {
-    return;
+    SKIP() << "host too heavily loaded to execute test";
   }
 
   ASSERT_EQ(1, latencyCallbacks);
-- 
2.26.2