Merge topic 'ctest-timeout-zero'

0a5aeaf302 cmCTestRunTest: Consolidate test timeout selection logic
426e38cc10 cmCTestRunTest: Adopt decision for starting cmProcess timer
59336b29bd cmCTestRunTest: Remove unnecessary arguments to ForkProcess
07b5087ba7 Help: Document meaning of TIMEOUT test property with value 0
3edf7fbb41 ctest: Fix TIMEOUT test property with value 0 with --timeout flag
39a20a56dd Tests: Move `CTestTestZeroTimeout` into `RunCMake.CTestTimeout`
cd4038fe94 cmCTestTestHandler: Use in-class initialization of properties and results

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: buildbot <buildbot@kitware.com>
Merge-request: !8455

7 files changed, 98 insertions, 109 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index abd1aa67c9..44eccb2275 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -19,6 +19,7 @@
 #include <vector>
 
 #include <cm/memory>
+#include <cm/optional>
 #include <cmext/algorithm>
 
 #include <cm3p/json/value.h>
@@ -1095,9 +1096,9 @@ static Json::Value DumpCTestProperties(
     properties.append(
       DumpCTestProperty("SKIP_RETURN_CODE", testProperties.SkipReturnCode));
   }
-  if (testProperties.ExplicitTimeout) {
+  if (testProperties.Timeout) {
     properties.append(
-      DumpCTestProperty("TIMEOUT", testProperties.Timeout.count()));
+      DumpCTestProperty("TIMEOUT", testProperties.Timeout->count()));
   }
   if (!testProperties.TimeoutRegularExpressions.empty()) {
     properties.append(DumpCTestProperty(
diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index 5efe69f52d..cd2b23088a 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -14,12 +14,14 @@
 #include <utility>
 
 #include <cm/memory>
+#include <cm/optional>
 
 #include "cmsys/RegularExpression.hxx"
 
 #include "cmCTest.h"
 #include "cmCTestMemCheckHandler.h"
 #include "cmCTestMultiProcessHandler.h"
+#include "cmDuration.h"
 #include "cmProcess.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
@@ -30,11 +32,6 @@ cmCTestRunTest::cmCTestRunTest(cmCTestMultiProcessHandler& multiHandler)
 {
   this->CTest = multiHandler.CTest;
   this->TestHandler = multiHandler.TestHandler;
-  this->TestResult.ExecutionTime = cmDuration::zero();
-  this->TestResult.ReturnValue = 0;
-  this->TestResult.Status = cmCTestTestHandler::NOT_RUN;
-  this->TestResult.TestCount = 0;
-  this->TestResult.Properties = nullptr;
 }
 
 void cmCTestRunTest::CheckOutput(std::string const& line)
@@ -623,28 +620,7 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   }
   this->StartTime = this->CTest->CurrentTime();
 
-  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 =
-      (stop_time - std::chrono::system_clock::now()) % std::chrono::hours(24);
-
-    if (stop_timeout <= std::chrono::duration<double>::zero()) {
-      stop_timeout = std::chrono::duration<double>::zero();
-    }
-    if (timeout == std::chrono::duration<double>::zero() ||
-        stop_timeout < timeout) {
-      this->TimeoutIsForStopTime = true;
-      timeout = stop_timeout;
-    }
-  }
-
-  return this->ForkProcess(timeout, this->TestProperties->ExplicitTimeout,
-                           &this->TestProperties->Environment,
-                           &this->TestProperties->EnvironmentModification,
-                           &this->TestProperties->Affinity);
+  return this->ForkProcess();
 }
 
 void cmCTestRunTest::ComputeArguments()
@@ -742,46 +718,80 @@ void cmCTestRunTest::ParseOutputForMeasurements()
   }
 }
 
-bool cmCTestRunTest::ForkProcess(
-  cmDuration testTimeOut, bool explicitTimeout,
-  std::vector<std::string>* environment,
-  std::vector<std::string>* environment_modification,
-  std::vector<size_t>* affinity)
+bool cmCTestRunTest::ForkProcess()
 {
   this->TestProcess->SetId(this->Index);
   this->TestProcess->SetWorkingDirectory(this->TestProperties->Directory);
   this->TestProcess->SetCommand(this->ActualCommand);
   this->TestProcess->SetCommandArguments(this->Arguments);
 
-  // determine how much time we have
-  cmDuration timeout = this->CTest->GetRemainingTimeAllowed();
-  if (timeout != cmCTest::MaxDuration()) {
-    timeout -= std::chrono::minutes(2);
+  cm::optional<cmDuration> timeout;
+
+  // Check TIMEOUT test property.
+  if (this->TestProperties->Timeout &&
+      *this->TestProperties->Timeout >= cmDuration::zero()) {
+    timeout = this->TestProperties->Timeout;
+  }
+
+  // An explicit TIMEOUT=0 test property means "no timeout".
+  if (timeout && *timeout == std::chrono::duration<double>::zero()) {
+    timeout = cm::nullopt;
+  } else {
+    // Check --timeout.
+    if (!timeout && this->CTest->GetGlobalTimeout() > cmDuration::zero()) {
+      timeout = this->CTest->GetGlobalTimeout();
+    }
+
+    // Check CTEST_TEST_TIMEOUT.
+    cmDuration ctestTestTimeout = this->CTest->GetTimeOut();
+    if (ctestTestTimeout > cmDuration::zero() &&
+        (!timeout || ctestTestTimeout < *timeout)) {
+      timeout = ctestTestTimeout;
+    }
   }
-  if (this->CTest->GetTimeOut() > cmDuration::zero() &&
-      this->CTest->GetTimeOut() < timeout) {
-    timeout = this->CTest->GetTimeOut();
+
+  // Check CTEST_TIME_LIMIT.
+  cmDuration timeRemaining = this->CTest->GetRemainingTimeAllowed();
+  if (timeRemaining != cmCTest::MaxDuration()) {
+    // This two minute buffer is historical.
+    timeRemaining -= std::chrono::minutes(2);
   }
-  if (testTimeOut > cmDuration::zero() &&
-      testTimeOut < this->CTest->GetRemainingTimeAllowed()) {
-    timeout = testTimeOut;
+
+  // Check --stop-time.
+  std::chrono::system_clock::time_point stop_time = this->CTest->GetStopTime();
+  if (stop_time != std::chrono::system_clock::time_point()) {
+    cmDuration timeUntilStop =
+      (stop_time - std::chrono::system_clock::now()) % std::chrono::hours(24);
+    if (timeUntilStop < timeRemaining) {
+      timeRemaining = timeUntilStop;
+    }
   }
-  // always have at least 1 second if we got to here
-  if (timeout <= cmDuration::zero()) {
-    timeout = std::chrono::seconds(1);
+
+  // Enforce remaining time even over explicit TIMEOUT=0.
+  if (timeRemaining <= cmDuration::zero()) {
+    timeRemaining = cmDuration::zero();
   }
-  // handle timeout explicitly set to 0
-  if (testTimeOut == cmDuration::zero() && explicitTimeout) {
-    timeout = cmDuration::zero();
+  if (!timeout || timeRemaining < *timeout) {
+    this->TimeoutIsForStopTime = true;
+    timeout = timeRemaining;
   }
-  cmCTestOptionalLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
-                     this->Index << ": "
-                                 << "Test timeout computed to be: "
-                                 << cmDurationTo<unsigned int>(timeout)
-                                 << "\n",
-                     this->TestHandler->GetQuiet());
 
-  this->TestProcess->SetTimeout(timeout);
+  if (timeout) {
+    cmCTestOptionalLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
+                       this->Index << ": "
+                                   << "Test timeout computed to be: "
+                                   << cmDurationTo<unsigned int>(*timeout)
+                                   << "\n",
+                       this->TestHandler->GetQuiet());
+
+    this->TestProcess->SetTimeout(*timeout);
+  } else {
+    cmCTestOptionalLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
+                       this->Index
+                         << ": "
+                         << "Test timeout suppressed by TIMEOUT property.\n",
+                       this->TestHandler->GetQuiet());
+  }
 
   cmSystemTools::SaveRestoreEnvironment sre;
   std::ostringstream envMeasurement;
@@ -789,17 +799,17 @@ bool cmCTestRunTest::ForkProcess(
   // We split processing ENVIRONMENT and ENVIRONMENT_MODIFICATION into two
   // phases to ensure that MYVAR=reset: in the latter phase resets to the
   // former phase's settings, rather than to the original environment.
-  if (environment && !environment->empty()) {
+  if (!this->TestProperties->Environment.empty()) {
     cmSystemTools::EnvDiff diff;
-    diff.AppendEnv(*environment);
+    diff.AppendEnv(this->TestProperties->Environment);
     diff.ApplyToCurrentEnv(&envMeasurement);
   }
 
-  if (environment_modification && !environment_modification->empty()) {
+  if (!this->TestProperties->EnvironmentModification.empty()) {
     cmSystemTools::EnvDiff diff;
     bool env_ok = true;
 
-    for (auto const& envmod : *environment_modification) {
+    for (auto const& envmod : this->TestProperties->EnvironmentModification) {
       env_ok &= diff.ParseOperation(envmod);
     }
 
@@ -828,7 +838,7 @@ bool cmCTestRunTest::ForkProcess(
                                      1);
 
   return this->TestProcess->StartProcess(this->MultiTestHandler.Loop,
-                                         affinity);
+                                         &this->TestProperties->Affinity);
 }
 
 void cmCTestRunTest::SetupResourcesEnvironment(std::vector<std::string>* log)
diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h
index 7a97fa9295..6a507f4fc4 100644
--- a/Source/CTest/cmCTestRunTest.h
+++ b/Source/CTest/cmCTestRunTest.h
@@ -14,7 +14,6 @@
 #include "cmCTest.h"
 #include "cmCTestMultiProcessHandler.h"
 #include "cmCTestTestHandler.h"
-#include "cmDuration.h"
 #include "cmProcess.h"
 
 /** \class cmRunTest
@@ -110,10 +109,7 @@ private:
   bool NeedsToRepeat();
   void ParseOutputForMeasurements();
   void ExeNotFound(std::string exe);
-  bool ForkProcess(cmDuration testTimeOut, bool explicitTimeout,
-                   std::vector<std::string>* environment,
-                   std::vector<std::string>* environment_modification,
-                   std::vector<size_t>* affinity);
+  bool ForkProcess();
   void WriteLogOutputTop(size_t completed, size_t total);
   // Run post processing of the process output for MemCheck
   void MemCheckPostProcess();
diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx
index 3a1cb64708..7764f2b7c1 100644
--- a/Source/CTest/cmCTestTestHandler.cxx
+++ b/Source/CTest/cmCTestTestHandler.cxx
@@ -1379,11 +1379,6 @@ bool cmCTestTestHandler::ProcessDirectory(std::vector<std::string>& passed,
       p.Cost = static_cast<float>(rand());
     }
 
-    if (p.Timeout == cmDuration::zero() &&
-        this->CTest->GetGlobalTimeout() != cmDuration::zero()) {
-      p.Timeout = this->CTest->GetGlobalTimeout();
-    }
-
     if (!p.Depends.empty()) {
       for (std::string const& i : p.Depends) {
         for (cmCTestTestProperties const& it2 : this->TestList) {
@@ -2252,7 +2247,6 @@ bool cmCTestTestHandler::SetTestsProperties(
             rt.FixturesRequired.insert(lval.begin(), lval.end());
           } else if (key == "TIMEOUT"_s) {
             rt.Timeout = cmDuration(atof(val.c_str()));
-            rt.ExplicitTimeout = true;
           } else if (key == "COST"_s) {
             rt.Cost = static_cast<float>(atof(val.c_str()));
           } else if (key == "REQUIRED_FILES"_s) {
@@ -2431,17 +2425,6 @@ bool cmCTestTestHandler::AddTest(const std::vector<std::string>& args)
                      "Set test directory: " << test.Directory << std::endl,
                      this->Quiet);
 
-  test.IsInBasedOnREOptions = true;
-  test.WillFail = false;
-  test.Disabled = false;
-  test.RunSerial = false;
-  test.Timeout = cmDuration::zero();
-  test.ExplicitTimeout = false;
-  test.Cost = 0;
-  test.Processors = 1;
-  test.WantAffinity = false;
-  test.SkipReturnCode = -1;
-  test.PreviousRuns = 0;
   if (this->UseIncludeRegExpFlag &&
       (!this->IncludeTestsRegularExpression.find(testname) ||
        (!this->UseExcludeRegExpFirst &&
diff --git a/Source/CTest/cmCTestTestHandler.h b/Source/CTest/cmCTestTestHandler.h
index d0049da733..b7c0fafbde 100644
--- a/Source/CTest/cmCTestTestHandler.h
+++ b/Source/CTest/cmCTestTestHandler.h
@@ -14,6 +14,8 @@
 #include <utility>
 #include <vector>
 
+#include <cm/optional>
+
 #include "cmsys/RegularExpression.hxx"
 
 #include "cmCTest.h"
@@ -139,22 +141,21 @@ public:
     std::vector<std::pair<cmsys::RegularExpression, std::string>>
       TimeoutRegularExpressions;
     std::map<std::string, std::string> Measurements;
-    bool IsInBasedOnREOptions;
-    bool WillFail;
-    bool Disabled;
-    float Cost;
-    int PreviousRuns;
-    bool RunSerial;
-    cmDuration Timeout;
-    bool ExplicitTimeout;
+    bool IsInBasedOnREOptions = true;
+    bool WillFail = false;
+    bool Disabled = false;
+    float Cost = 0;
+    int PreviousRuns = 0;
+    bool RunSerial = false;
+    cm::optional<cmDuration> Timeout;
     cmDuration AlternateTimeout;
-    int Index;
+    int Index = 0;
     // Requested number of process slots
-    int Processors;
-    bool WantAffinity;
+    int Processors = 1;
+    bool WantAffinity = false;
     std::vector<size_t> Affinity;
     // return code of test which will mark test as "not run"
-    int SkipReturnCode;
+    int SkipReturnCode = -1;
     std::vector<std::string> Environment;
     std::vector<std::string> EnvironmentModification;
     std::vector<std::string> Labels;
@@ -175,17 +176,17 @@ public:
     std::string Reason;
     std::string FullCommandLine;
     std::string Environment;
-    cmDuration ExecutionTime;
-    std::int64_t ReturnValue;
-    int Status;
+    cmDuration ExecutionTime = cmDuration::zero();
+    std::int64_t ReturnValue = 0;
+    int Status = NOT_RUN;
     std::string ExceptionStatus;
     bool CompressOutput;
     std::string CompletionStatus;
     std::string CustomCompletionStatus;
     std::string Output;
     std::string TestMeasurementsOutput;
-    int TestCount;
-    cmCTestTestProperties* Properties;
+    int TestCount = 0;
+    cmCTestTestProperties* Properties = nullptr;
   };
 
   struct cmCTestTestResultLess
diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx
index e14a4e1d0c..780d626927 100644
--- a/Source/CTest/cmProcess.cxx
+++ b/Source/CTest/cmProcess.cxx
@@ -13,7 +13,6 @@
 
 #include "cmCTest.h"
 #include "cmCTestRunTest.h"
-#include "cmCTestTestHandler.h"
 #include "cmGetPipes.h"
 #include "cmStringAlgorithms.h"
 #if defined(_WIN32)
@@ -26,7 +25,6 @@ cmProcess::cmProcess(std::unique_ptr<cmCTestRunTest> runner)
   : Runner(std::move(runner))
   , Conv(cmProcessOutput::UTF8, CM_PROCESS_BUF_SIZE)
 {
-  this->Timeout = cmDuration::zero();
   this->TotalTime = cmDuration::zero();
   this->ExitValue = 0;
   this->Id = 0;
@@ -152,11 +150,9 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
 
 void cmProcess::StartTimer()
 {
-  auto* properties = this->Runner->GetTestProperties();
-  auto msec =
-    std::chrono::duration_cast<std::chrono::milliseconds>(this->Timeout);
-
-  if (msec != std::chrono::milliseconds(0) || !properties->ExplicitTimeout) {
+  if (this->Timeout) {
+    auto msec =
+      std::chrono::duration_cast<std::chrono::milliseconds>(*this->Timeout);
     this->Timer.start(&cmProcess::OnTimeoutCB,
                       static_cast<uint64_t>(msec.count()), 0);
   }
diff --git a/Source/CTest/cmProcess.h b/Source/CTest/cmProcess.h
index be030e4c55..15786873ec 100644
--- a/Source/CTest/cmProcess.h
+++ b/Source/CTest/cmProcess.h
@@ -12,6 +12,8 @@
 #include <utility>
 #include <vector>
 
+#include <cm/optional>
+
 #include <cm3p/uv.h>
 
 #include "cmDuration.h"
@@ -76,7 +78,7 @@ public:
   }
 
 private:
-  cmDuration Timeout;
+  cm::optional<cmDuration> Timeout;
   std::chrono::steady_clock::time_point StartTime;
   cmDuration TotalTime;
   bool ReadHandleClosed = false;