From 724508295d0ea16328fcff99f6693ff9e226789b Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Mon, 14 Aug 2017 15:38:28 -0700
Subject: [PATCH] test: make timers-blocking-callback more reliable

test-timers-blocking-callback may fail erroneously on
resource-constrained machines due to the timing nature of the test.

There is likely no way around the timing issue. This change tries to
decrease the probability of the test failing erroneously by having it
retry a small number of times on failure.

Tested on 0.10.38 (which has a bug that this test was written for) and
(modifying the test slightly to remove ES6 stuff) the test still seems
to fail 100% of the time there, which is what we want/expect.

PR-URL: https://github.com/nodejs/node/pull/14831
Fixes: https://github.com/nodejs/node/issues/14792
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 .../test-timers-blocking-callback.js          | 70 ++++++++++++++-----
 1 file changed, 52 insertions(+), 18 deletions(-)

diff --git a/test/sequential/test-timers-blocking-callback.js b/test/sequential/test-timers-blocking-callback.js
index 73b0f13997..d500b813af 100644
--- a/test/sequential/test-timers-blocking-callback.js
+++ b/test/sequential/test-timers-blocking-callback.js
@@ -1,8 +1,9 @@
 'use strict';
 
 /*
- * This is a regression test for https://github.com/joyent/node/issues/15447
- * and https://github.com/joyent/node/issues/9333.
+ * This is a regression test for
+ * https://github.com/nodejs/node-v0.x-archive/issues/15447 and
+ * and https://github.com/nodejs/node-v0.x-archive/issues/9333.
  *
  * When a timer is added in another timer's callback, its underlying timer
  * handle was started with a timeout that was actually incorrect.
@@ -28,9 +29,15 @@ const Timer = process.binding('timer_wrap').Timer;
 
 const TIMEOUT = 100;
 
-let nbBlockingCallbackCalls = 0;
-let latestDelay = 0;
-let timeCallbackScheduled = 0;
+let nbBlockingCallbackCalls;
+let latestDelay;
+let timeCallbackScheduled;
+
+// These tests are timing dependent so they may fail even when the bug is
+// not present (if the host is sufficiently busy that the timers are delayed
+// significantly). However, they fail 100% of the time when the bug *is*
+// present, so to increase reliability, allow for a small number of retries.
+let retries = 2;
 
 function initTest() {
   nbBlockingCallbackCalls = 0;
@@ -38,7 +45,7 @@ function initTest() {
   timeCallbackScheduled = 0;
 }
 
-function blockingCallback(callback) {
+function blockingCallback(retry, callback) {
   ++nbBlockingCallbackCalls;
 
   if (nbBlockingCallbackCalls > 1) {
@@ -47,8 +54,15 @@ function blockingCallback(callback) {
     // to fire, they shouldn't generally be more than 100% late in this case.
     // But they are guaranteed to be at least 100ms late given the bug in
     // https://github.com/nodejs/node-v0.x-archive/issues/15447 and
-    // https://github.com/nodejs/node-v0.x-archive/issues/9333..
-    assert(latestDelay < TIMEOUT * 2);
+    // https://github.com/nodejs/node-v0.x-archive/issues/9333.
+    if (latestDelay >= TIMEOUT * 2) {
+      if (retries > 0) {
+        retries--;
+        return retry(callback);
+      }
+      assert.fail(null, null,
+                  `timeout delayed by more than 100% (${latestDelay}ms)`);
+    }
     if (callback)
       return callback();
   } else {
@@ -56,25 +70,45 @@ function blockingCallback(callback) {
     common.busyLoop(TIMEOUT);
 
     timeCallbackScheduled = Timer.now();
-    setTimeout(blockingCallback.bind(null, callback), TIMEOUT);
+    setTimeout(blockingCallback.bind(null, retry, callback), TIMEOUT);
   }
 }
 
-const testAddingTimerToEmptyTimersList = common.mustCall(function(callback) {
+function testAddingTimerToEmptyTimersList(callback) {
   initTest();
   // Call setTimeout just once to make sure the timers list is
   // empty when blockingCallback is called.
-  setTimeout(blockingCallback.bind(null, callback), TIMEOUT);
-});
+  setTimeout(
+    blockingCallback.bind(null, testAddingTimerToEmptyTimersList, callback),
+    TIMEOUT
+  );
+}
+
+function testAddingTimerToNonEmptyTimersList() {
+  // If both timers fail and attempt a retry, only actually do anything for one
+  // of them.
+  let retryOK = true;
+  const retry = () => {
+    if (retryOK)
+      testAddingTimerToNonEmptyTimersList();
+    retryOK = false;
+  };
 
-const testAddingTimerToNonEmptyTimersList = common.mustCall(function() {
   initTest();
   // Call setTimeout twice with the same timeout to make
   // sure the timers list is not empty when blockingCallback is called.
-  setTimeout(blockingCallback, TIMEOUT);
-  setTimeout(blockingCallback, TIMEOUT);
-});
+  setTimeout(
+    blockingCallback.bind(null, retry),
+    TIMEOUT
+  );
+  setTimeout(
+    blockingCallback.bind(null, retry),
+    TIMEOUT
+  );
+}
 
 // Run the test for the empty timers list case, and then for the non-empty
-// timers list one
-testAddingTimerToEmptyTimersList(testAddingTimerToNonEmptyTimersList);
+// timers list one.
+testAddingTimerToEmptyTimersList(
+  common.mustCall(testAddingTimerToNonEmptyTimersList)
+);