CTest: Fix --test-load regression

The `ctest --test-load` option is implemented in `StartNextTests` by not
starting any tests when the load is too high and instead sleeping and
then returning.  Prior to commit v3.11.0-rc1~117^2 (CTest: Re-implement
test process handling using libuv, 2017-12-10) our outer loop in
`RunTests` would immediately call `StartNextTests` again.  However, now
the `uv_run` loop may simply terminate if there are no tests running
because no events are left pending.

Fix this by converting the sleep in `StartNextTests` into a libuv timer
that it starts instead.  This avoids leaving `uv_run` with no pending
events.  In the case that there are other running tests this also allows
CTest to detect when they finish even if it during the wait period where
we previously slept.

This regression was not caught by the test suite because it only
verified that we do not start new tests when the load was too high and
not that we proceed to start tests when the load drops.  Revise the test
suite to cover both.

Fixes: #18338

16 files changed, 74 insertions, 38 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index dcef8a0..322da23 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -5,7 +5,6 @@
 #include "cmAffinity.h"
 #include "cmCTest.h"
 #include "cmCTestRunTest.h"
-#include "cmCTestScriptHandler.h"
 #include "cmCTestTestHandler.h"
 #include "cmSystemTools.h"
 #include "cmWorkingDirectory.h"
@@ -53,6 +52,7 @@ cmCTestMultiProcessHandler::cmCTestMultiProcessHandler()
 {
   this->ParallelLevel = 1;
   this->TestLoad = 0;
+  this->FakeLoadForTesting = 0;
   this->Completed = 0;
   this->RunningCount = 0;
   this->ProcessorsAvailable = cmAffinity::GetProcessorsAvailable();
@@ -97,6 +97,16 @@ void cmCTestMultiProcessHandler::SetParallelLevel(size_t level)
 void cmCTestMultiProcessHandler::SetTestLoad(unsigned long load)
 {
   this->TestLoad = load;
+
+  std::string fake_load_value;
+  if (cmSystemTools::GetEnv("__CTEST_FAKE_LOAD_AVERAGE_FOR_TESTING",
+                            fake_load_value)) {
+    if (!cmSystemTools::StringToULong(fake_load_value.c_str(),
+                                      &this->FakeLoadForTesting)) {
+      cmSystemTools::Error("Failed to parse fake load value: ",
+                           fake_load_value.c_str());
+    }
+  }
 }
 
 void cmCTestMultiProcessHandler::RunTests()
@@ -259,12 +269,19 @@ bool cmCTestMultiProcessHandler::StartTest(int test)
 
 void cmCTestMultiProcessHandler::StartNextTests()
 {
-  size_t numToStart = 0;
+  if (this->TestLoadRetryTimer.get() != nullptr) {
+    // This timer may be waiting to call StartNextTests again.
+    // Since we have been called it is no longer needed.
+    uv_timer_stop(this->TestLoadRetryTimer);
+  }
 
   if (this->Tests.empty()) {
+    this->TestLoadRetryTimer.reset();
     return;
   }
 
+  size_t numToStart = 0;
+
   if (this->RunningCount < this->ParallelLevel) {
     numToStart = this->ParallelLevel - this->RunningCount;
   }
@@ -280,7 +297,6 @@ void cmCTestMultiProcessHandler::StartNextTests()
   }
 
   bool allTestsFailedTestLoadCheck = false;
-  bool usedFakeLoadForTesting = false;
   size_t minProcessorsRequired = this->ParallelLevel;
   std::string testWithMinProcessors;
 
@@ -293,15 +309,11 @@ void cmCTestMultiProcessHandler::StartNextTests()
     allTestsFailedTestLoadCheck = true;
 
     // Check for a fake load average value used in testing.
-    std::string fake_load_value;
-    if (cmSystemTools::GetEnv("__CTEST_FAKE_LOAD_AVERAGE_FOR_TESTING",
-                              fake_load_value)) {
-      usedFakeLoadForTesting = true;
-      if (!cmSystemTools::StringToULong(fake_load_value.c_str(),
-                                        &systemLoad)) {
-        cmSystemTools::Error("Failed to parse fake load value: ",
-                             fake_load_value.c_str());
-      }
+    if (this->FakeLoadForTesting > 0) {
+      systemLoad = this->FakeLoadForTesting;
+      // Drop the fake load for the next iteration to a value low enough
+      // that the next iteration will start tests.
+      this->FakeLoadForTesting = 1;
     }
     // If it's not set, look up the true load average.
     else {
@@ -385,18 +397,25 @@ void cmCTestMultiProcessHandler::StartNextTests()
     }
     cmCTestLog(this->CTest, HANDLER_OUTPUT, "*****" << std::endl);
 
-    if (usedFakeLoadForTesting) {
-      // Break out of the infinite loop of waiting for our fake load
-      // to come down.
-      this->StopTimePassed = true;
-    } else {
-      // Wait between 1 and 5 seconds before trying again.
-      cmCTestScriptHandler::SleepInSeconds(cmSystemTools::RandomSeed() % 5 +
-                                           1);
+    // Wait between 1 and 5 seconds before trying again.
+    unsigned int milliseconds = (cmSystemTools::RandomSeed() % 5 + 1) * 1000;
+    if (this->FakeLoadForTesting) {
+      milliseconds = 10;
+    }
+    if (this->TestLoadRetryTimer.get() == nullptr) {
+      this->TestLoadRetryTimer.init(this->Loop, this);
     }
+    this->TestLoadRetryTimer.start(
+      &cmCTestMultiProcessHandler::OnTestLoadRetryCB, milliseconds, 0);
   }
 }
 
+void cmCTestMultiProcessHandler::OnTestLoadRetryCB(uv_timer_t* timer)
+{
+  auto self = static_cast<cmCTestMultiProcessHandler*>(timer->data);
+  self->StartNextTests();
+}
+
 void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
                                                    bool started)
 {
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index 203170e..07a5fe7 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -12,6 +12,7 @@
 #include <string>
 #include <vector>
 
+#include "cmUVHandlePtr.h"
 #include "cm_uv.h"
 
 class cmCTest;
@@ -101,6 +102,8 @@ protected:
   void EraseTest(int index);
   void FinishTestProcess(cmCTestRunTest* runner, bool started);
 
+  static void OnTestLoadRetryCB(uv_timer_t* timer);
+
   void RemoveTest(int index);
   // Check if we need to resume an interrupted test set
   void CheckResume();
@@ -135,7 +138,9 @@ protected:
   std::vector<cmCTestTestHandler::cmCTestTestResult>* TestResults;
   size_t ParallelLevel; // max number of process that can be run at once
   unsigned long TestLoad;
+  unsigned long FakeLoadForTesting;
   uv_loop_t Loop;
+  cm::uv_timer_ptr TestLoadRetryTimer;
   cmCTestTestHandler* TestHandler;
   cmCTest* CTest;
   bool HasCycles;
diff --git a/Tests/RunCMake/CTestCommandLine/RunCMakeTest.cmake b/Tests/RunCMake/CTestCommandLine/RunCMakeTest.cmake
index 3033c9c..9e8d050 100644
--- a/Tests/RunCMake/CTestCommandLine/RunCMakeTest.cmake
+++ b/Tests/RunCMake/CTestCommandLine/RunCMakeTest.cmake
@@ -111,8 +111,8 @@ endfunction()
 set(ENV{__CTEST_FAKE_LOAD_AVERAGE_FOR_TESTING} 5)
 
 # Verify that new tests are not started when the load average exceeds
-# our threshold.
-run_TestLoad(test-load-fail 2)
+# our threshold and that they then run once the load average drops.
+run_TestLoad(test-load-wait 3)
 
 # Verify that warning message is displayed but tests still start when
 # an invalid argument is given.
diff --git a/Tests/RunCMake/CTestCommandLine/test-load-fail-stderr.txt b/Tests/RunCMake/CTestCommandLine/test-load-fail-stderr.txt
deleted file mode 100644
index eafba1c..0000000
--- a/Tests/RunCMake/CTestCommandLine/test-load-fail-stderr.txt
+++ /dev/null
@@ -1 +0,0 @@
-No tests were found!!!
diff --git a/Tests/RunCMake/CTestCommandLine/test-load-fail-stdout.txt b/Tests/RunCMake/CTestCommandLine/test-load-fail-stdout.txt
deleted file mode 100644
index 153da09..0000000
--- a/Tests/RunCMake/CTestCommandLine/test-load-fail-stdout.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-^Test project .*/Tests/RunCMake/CTestCommandLine/TestLoad
-\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 2, Smallest test TestLoad[1-2] requires 1\*\*\*\*\*
diff --git a/Tests/RunCMake/CTestCommandLine/test-load-pass-stderr.txt b/Tests/RunCMake/CTestCommandLine/test-load-pass-stderr.txt
deleted file mode 100644
index 10f3293..0000000
--- a/Tests/RunCMake/CTestCommandLine/test-load-pass-stderr.txt
+++ /dev/null
@@ -1 +0,0 @@
-^$
diff --git a/Tests/RunCMake/CTestCommandLine/test-load-wait-stdout.txt b/Tests/RunCMake/CTestCommandLine/test-load-wait-stdout.txt
new file mode 100644
index 0000000..11a768a
--- /dev/null
+++ b/Tests/RunCMake/CTestCommandLine/test-load-wait-stdout.txt
@@ -0,0 +1,8 @@
+^Test project .*/Tests/RunCMake/CTestCommandLine/TestLoad
+\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 3, Smallest test TestLoad[1-2] requires 1\*\*\*\*\*
+    Start 1: TestLoad1
+    Start 2: TestLoad2
+1/2 Test #[1-2]: TestLoad[1-2] ........................   Passed +[0-9.]+ sec
+2/2 Test #[1-2]: TestLoad[1-2] ........................   Passed +[0-9.]+ sec
++
+100% tests passed, 0 tests failed out of 2
diff --git a/Tests/RunCMake/ctest_test/CTestTestLoadFail-result.txt b/Tests/RunCMake/ctest_test/CTestTestLoadFail-result.txt
deleted file mode 100644
index b57e2de..0000000
--- a/Tests/RunCMake/ctest_test/CTestTestLoadFail-result.txt
+++ /dev/null
@@ -1 +0,0 @@
-(-1|255)
diff --git a/Tests/RunCMake/ctest_test/CTestTestLoadFail-stderr.txt b/Tests/RunCMake/ctest_test/CTestTestLoadFail-stderr.txt
deleted file mode 100644
index eafba1c..0000000
--- a/Tests/RunCMake/ctest_test/CTestTestLoadFail-stderr.txt
+++ /dev/null
@@ -1 +0,0 @@
-No tests were found!!!
diff --git a/Tests/RunCMake/ctest_test/CTestTestLoadFail-stdout.txt b/Tests/RunCMake/ctest_test/CTestTestLoadFail-stdout.txt
deleted file mode 100644
index e203c10..0000000
--- a/Tests/RunCMake/ctest_test/CTestTestLoadFail-stdout.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-Test project .*/Tests/RunCMake/ctest_test/CTestTestLoadFail-build
-\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 4, Smallest test RunCMakeVersion requires 1\*\*\*\*\*$
diff --git a/Tests/RunCMake/ctest_test/CTestTestLoadWait-stdout.txt b/Tests/RunCMake/ctest_test/CTestTestLoadWait-stdout.txt
new file mode 100644
index 0000000..2af3838
--- /dev/null
+++ b/Tests/RunCMake/ctest_test/CTestTestLoadWait-stdout.txt
@@ -0,0 +1,8 @@
+Test project .*/Tests/RunCMake/ctest_test/CTestTestLoadWait-build
+\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 4, Smallest test RunCMakeVersion requires 1\*\*\*\*\*
+    Start 1: RunCMakeVersion
+1/1 Test #1: RunCMakeVersion ..................   Passed +[0-9.]+ sec
++
+100% tests passed, 0 tests failed out of 1
++
+Total Test time \(real\) = +[0-9.]+ sec$
diff --git a/Tests/RunCMake/ctest_test/RunCMakeTest.cmake b/Tests/RunCMake/ctest_test/RunCMakeTest.cmake
index 1b31726..6877e6a 100644
--- a/Tests/RunCMake/ctest_test/RunCMakeTest.cmake
+++ b/Tests/RunCMake/ctest_test/RunCMakeTest.cmake
@@ -21,8 +21,8 @@ set(ENV{__CTEST_FAKE_LOAD_AVERAGE_FOR_TESTING} 5)
 run_ctest_test(TestLoadPass TEST_LOAD 6)
 
 # Verify that new tests are not started when the load average exceeds
-# our threshold.
-run_ctest_test(TestLoadFail TEST_LOAD 2)
+# our threshold and that they then run once the load average drops.
+run_ctest_test(TestLoadWait TEST_LOAD 2)
 
 # Verify that when an invalid "TEST_LOAD" value is given, a warning
 # message is displayed and the value is ignored.
@@ -34,9 +34,9 @@ set(CASE_CTEST_TEST_LOAD 7)
 run_ctest_test(CTestTestLoadPass)
 
 # Verify that new tests are not started when the load average exceeds
-# our threshold.
+# our threshold and that they then run once the load average drops.
 set(CASE_CTEST_TEST_LOAD 4)
-run_ctest_test(CTestTestLoadFail)
+run_ctest_test(CTestTestLoadWait)
 
 # Verify that when an invalid "CTEST_TEST_LOAD" value is given,
 # a warning message is displayed and the value is ignored.
diff --git a/Tests/RunCMake/ctest_test/TestLoadFail-result.txt b/Tests/RunCMake/ctest_test/TestLoadFail-result.txt
deleted file mode 100644
index b57e2de..0000000
--- a/Tests/RunCMake/ctest_test/TestLoadFail-result.txt
+++ /dev/null
@@ -1 +0,0 @@
-(-1|255)
diff --git a/Tests/RunCMake/ctest_test/TestLoadFail-stderr.txt b/Tests/RunCMake/ctest_test/TestLoadFail-stderr.txt
deleted file mode 100644
index eafba1c..0000000
--- a/Tests/RunCMake/ctest_test/TestLoadFail-stderr.txt
+++ /dev/null
@@ -1 +0,0 @@
-No tests were found!!!
diff --git a/Tests/RunCMake/ctest_test/TestLoadFail-stdout.txt b/Tests/RunCMake/ctest_test/TestLoadFail-stdout.txt
deleted file mode 100644
index 4d7ce48..0000000
--- a/Tests/RunCMake/ctest_test/TestLoadFail-stdout.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-Test project .*/Tests/RunCMake/ctest_test/TestLoadFail-build
-\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 2, Smallest test RunCMakeVersion requires 1\*\*\*\*\*$
diff --git a/Tests/RunCMake/ctest_test/TestLoadWait-stdout.txt b/Tests/RunCMake/ctest_test/TestLoadWait-stdout.txt
new file mode 100644
index 0000000..07f4ed3
--- /dev/null
+++ b/Tests/RunCMake/ctest_test/TestLoadWait-stdout.txt
@@ -0,0 +1,8 @@
+Test project .*/Tests/RunCMake/ctest_test/TestLoadWait-build
+\*\*\*\*\* WAITING, System Load: 5, Max Allowed Load: 2, Smallest test RunCMakeVersion requires 1\*\*\*\*\*
+    Start 1: RunCMakeVersion
+1/1 Test #1: RunCMakeVersion ..................   Passed +[0-9.]+ sec
++
+100% tests passed, 0 tests failed out of 1
++
+Total Test time \(real\) = +[0-9.]+ sec$