CTest: Improve stop-time implementation

The CTestTestStopTime test has been failing sporadically because the
stop time causes the first internal test to have a timeout short enough
that we might hit it and start the second test just before the stop time
is reached.  Instead we should track when a timeout is shortened in
order to stay within the stop time.  If a test times out for this reason
then we should consider the stop time reached and not start any more
tests.

Source/CTest/cmCTestMultiProcessHandler.cxx

@@ -56,7 +56,6 @@ cmCTestMultiProcessHandler::cmCTestMultiProcessHandler()
   this->RunningCount = 0;
   this->ProcessorsAvailable = cmAffinity::GetProcessorsAvailable();
   this->HaveAffinity = this->ProcessorsAvailable.size();
-  this->StopTimePassed = false;
   this->HasCycles = false;
   this->SerialTestRunning = false;
 }
@@ -130,17 +129,6 @@ void cmCTestMultiProcessHandler::RunTests()
 
 bool cmCTestMultiProcessHandler::StartTestProcess(int test)
 {
-  std::chrono::system_clock::time_point stop_time = this->CTest->GetStopTime();
-  if (stop_time != std::chrono::system_clock::time_point() &&
-      stop_time <= std::chrono::system_clock::now()) {
-    cmCTestLog(this->CTest, ERROR_MESSAGE,
-               "The stop time has been passed. "
-               "Stopping all tests."
-                 << std::endl);
-    this->StopTimePassed = true;
-    return false;
-  }
-
   if (this->HaveAffinity && this->Properties[test]->WantAffinity) {
     size_t needProcessors = this->GetProcessorsUsed(test);
     if (needProcessors > this->ProcessorsAvailable.size()) {
@@ -199,6 +187,30 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
   return false;
 }
 
+bool cmCTestMultiProcessHandler::CheckStopTimePassed()
+{
+  if (!this->StopTimePassed) {
+    std::chrono::system_clock::time_point stop_time =
+      this->CTest->GetStopTime();
+    if (stop_time != std::chrono::system_clock::time_point() &&
+        stop_time <= std::chrono::system_clock::now()) {
+      this->SetStopTimePassed();
+    }
+  }
+  return this->StopTimePassed;
+}
+
+void cmCTestMultiProcessHandler::SetStopTimePassed()
+{
+  if (!this->StopTimePassed) {
+    cmCTestLog(this->CTest, ERROR_MESSAGE,
+               "The stop time has been passed. "
+               "Stopping all tests."
+                 << std::endl);
+    this->StopTimePassed = true;
+  }
+}
+
 void cmCTestMultiProcessHandler::LockResources(int index)
 {
   this->LockedResources.insert(
@@ -279,6 +291,10 @@ void cmCTestMultiProcessHandler::StartNextTests()
     return;
   }
 
+  if (this->CheckStopTimePassed()) {
+    return;
+  }
+
   size_t numToStart = 0;
 
   if (this->RunningCount < this->ParallelLevel) {
@@ -358,10 +374,6 @@ void cmCTestMultiProcessHandler::StartNextTests()
     }
 
     if (testLoadOk && processors <= numToStart && this->StartTest(test)) {
-      if (this->StopTimePassed) {
-        return;
-      }
-
       numToStart -= processors;
     } else if (numToStart == 0) {
       break;
@@ -424,8 +436,11 @@ void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
   auto properties = runner->GetTestProperties();
 
   bool testResult = runner->EndTest(this->Completed, this->Total, started);
+  if (runner->TimedOutForStopTime()) {
+    this->SetStopTimePassed();
+  }
   if (started) {
-    if (runner->StartAgain()) {
+    if (!this->StopTimePassed && runner->StartAgain()) {
       this->Completed--; // remove the completed test because run again
       return;
     }

Source/CTest/cmCTestMultiProcessHandler.h

@@ -113,6 +113,9 @@ protected:
   inline size_t GetProcessorsUsed(int index);
   std::string GetName(int index);
 
+  bool CheckStopTimePassed();
+  void SetStopTimePassed();
+
   void LockResources(int index);
   void UnlockResources(int index);
   // map from test number to set of depend tests
@@ -125,7 +128,7 @@ protected:
   size_t RunningCount;
   std::set<size_t> ProcessorsAvailable;
   size_t HaveAffinity;
-  bool StopTimePassed;
+  bool StopTimePassed = false;
   // list of test properties (indices concurrent to the test map)
   PropertiesMap Properties;
   std::map<int, bool> TestRunningMap;

Source/CTest/cmCTestRunTest.cxx

@@ -140,6 +140,9 @@ bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started)
   bool passed = true;
   cmProcess::State res =
     started ? this->TestProcess->GetProcessStatus() : cmProcess::State::Error;
+  if (res != cmProcess::State::Expired) {
+    this->TimeoutIsForStopTime = false;
+  }
   int retVal = this->TestProcess->GetExitValue();
   bool forceFail = false;
   bool skipped = false;
@@ -537,6 +540,7 @@ bool cmCTestRunTest::StartTest(size_t total)
 
   auto timeout = this->TestProperties->Timeout;
 
+  this->TimeoutIsForStopTime = false;
   std::chrono::system_clock::time_point stop_time = this->CTest->GetStopTime();
   if (stop_time != std::chrono::system_clock::time_point()) {
     std::chrono::duration<double> stop_timeout =
@@ -547,6 +551,7 @@ bool cmCTestRunTest::StartTest(size_t total)
     }
     if (timeout == std::chrono::duration<double>::zero() ||
         stop_timeout < timeout) {
+      this->TimeoutIsForStopTime = true;
       timeout = stop_timeout;
     }
   }

Source/CTest/cmCTestRunTest.h

@@ -80,6 +80,8 @@ public:
 
   void FinalizeTest();
 
+  bool TimedOutForStopTime() const { return this->TimeoutIsForStopTime; }
+
 private:
   bool NeedsToRerun();
   void DartProcessing();
@@ -92,6 +94,7 @@ private:
   void MemCheckPostProcess();
 
   cmCTestTestHandler::cmCTestTestProperties* TestProperties;
+  bool TimeoutIsForStopTime = false;
   // Pointer back to the "parent"; the handler that invoked this test run
   cmCTestTestHandler* TestHandler;
   cmCTest* CTest;