Merge topic 'ctest-failure-error-reporting' into release-3.17

a5be3916ee CTest: Provide more detailed information on resource allocation error
f0df3ed5b9 Refactor: Provide more detailed error information from TryAllocateResources()
f1c34443b7 CTest: Improve error reporting with bad working directory for tests
1dec359422 Refactor: Require detail when calling cmCTestRunTest::StartFailure()

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

6 files changed, 82 insertions, 24 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index 37679b9..2192843 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -196,8 +196,40 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
   // working directory because FinishTestProcess() will try to unlock them
   this->LockResources(test);
 
-  if (!this->TestsHaveSufficientResources[test]) {
-    testRun->StartFailure("Insufficient resources");
+  if (!this->ResourceAllocationErrors[test].empty()) {
+    std::ostringstream e;
+    e << "Insufficient resources for test " << this->Properties[test]->Name
+      << ":\n\n";
+    for (auto const& it : this->ResourceAllocationErrors[test]) {
+      switch (it.second) {
+        case ResourceAllocationError::NoResourceType:
+          e << "  Test requested resources of type '" << it.first
+            << "' which does not exist\n";
+          break;
+
+        case ResourceAllocationError::InsufficientResources:
+          e << "  Test requested resources of type '" << it.first
+            << "' in the following amounts:\n";
+          for (auto const& group : this->Properties[test]->ResourceGroups) {
+            for (auto const& requirement : group) {
+              if (requirement.ResourceType == it.first) {
+                e << "    " << requirement.SlotsNeeded
+                  << (requirement.SlotsNeeded == 1 ? " slot\n" : " slots\n");
+              }
+            }
+          }
+          e << "  but only the following units were available:\n";
+          for (auto const& res :
+               this->ResourceAllocator.GetResources().at(it.first)) {
+            e << "    '" << res.first << "': " << res.second.Total
+              << (res.second.Total == 1 ? " slot\n" : " slots\n");
+          }
+          break;
+      }
+      e << "\n";
+    }
+    e << "Resource spec file:\n\n  " << this->TestHandler->ResourceSpecFile;
+    testRun->StartFailure(e.str(), "Insufficient resources");
     this->FinishTestProcess(testRun, false);
     return false;
   }
@@ -205,8 +237,9 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
   cmWorkingDirectory workdir(this->Properties[test]->Directory);
   if (workdir.Failed()) {
     testRun->StartFailure("Failed to change working directory to " +
-                          this->Properties[test]->Directory + " : " +
-                          std::strerror(workdir.GetLastResult()));
+                            this->Properties[test]->Directory + " : " +
+                            std::strerror(workdir.GetLastResult()),
+                          "Failed to change working directory");
   } else {
     if (testRun->StartTest(this->Completed, this->Total)) {
       // Ownership of 'testRun' has moved to another structure.
@@ -249,7 +282,8 @@ bool cmCTestMultiProcessHandler::AllocateResources(int index)
 
 bool cmCTestMultiProcessHandler::TryAllocateResources(
   int index,
-  std::map<std::string, std::vector<cmCTestBinPackerAllocation>>& allocations)
+  std::map<std::string, std::vector<cmCTestBinPackerAllocation>>& allocations,
+  std::map<std::string, ResourceAllocationError>* errors)
 {
   allocations.clear();
 
@@ -264,18 +298,28 @@ bool cmCTestMultiProcessHandler::TryAllocateResources(
     ++processIndex;
   }
 
+  bool result = true;
   auto const& availableResources = this->ResourceAllocator.GetResources();
   for (auto& it : allocations) {
     if (!availableResources.count(it.first)) {
-      return false;
-    }
-    if (!cmAllocateCTestResourcesRoundRobin(availableResources.at(it.first),
-                                            it.second)) {
-      return false;
+      if (errors) {
+        (*errors)[it.first] = ResourceAllocationError::NoResourceType;
+        result = false;
+      } else {
+        return false;
+      }
+    } else if (!cmAllocateCTestResourcesRoundRobin(
+                 availableResources.at(it.first), it.second)) {
+      if (errors) {
+        (*errors)[it.first] = ResourceAllocationError::InsufficientResources;
+        result = false;
+      } else {
+        return false;
+      }
     }
   }
 
-  return true;
+  return result;
 }
 
 void cmCTestMultiProcessHandler::DeallocateResources(int index)
@@ -316,11 +360,13 @@ bool cmCTestMultiProcessHandler::AllResourcesAvailable()
 
 void cmCTestMultiProcessHandler::CheckResourcesAvailable()
 {
-  for (auto test : this->SortedTests) {
-    std::map<std::string, std::vector<cmCTestBinPackerAllocation>> allocations;
-    this->TestsHaveSufficientResources[test] =
-      !this->TestHandler->UseResourceSpec ||
-      this->TryAllocateResources(test, allocations);
+  if (this->TestHandler->UseResourceSpec) {
+    for (auto test : this->SortedTests) {
+      std::map<std::string, std::vector<cmCTestBinPackerAllocation>>
+        allocations;
+      this->TryAllocateResources(test, allocations,
+                                 &this->ResourceAllocationErrors[test]);
+    }
   }
 }
 
@@ -407,7 +453,7 @@ bool cmCTestMultiProcessHandler::StartTest(int test)
   }
 
   // Allocate resources
-  if (this->TestsHaveSufficientResources[test] &&
+  if (this->ResourceAllocationErrors[test].empty() &&
       !this->AllocateResources(test)) {
     this->DeallocateResources(test);
     return false;
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index 4837401..5b429d4 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -143,11 +143,18 @@ protected:
   void LockResources(int index);
   void UnlockResources(int index);
 
+  enum class ResourceAllocationError
+  {
+    NoResourceType,
+    InsufficientResources,
+  };
+
   bool AllocateResources(int index);
   bool TryAllocateResources(
     int index,
     std::map<std::string, std::vector<cmCTestBinPackerAllocation>>&
-      allocations);
+      allocations,
+    std::map<std::string, ResourceAllocationError>* errors = nullptr);
   void DeallocateResources(int index);
   bool AllResourcesAvailable();
 
@@ -174,7 +181,8 @@ protected:
   std::map<int,
            std::vector<std::map<std::string, std::vector<ResourceAllocation>>>>
     AllocatedResources;
-  std::map<int, bool> TestsHaveSufficientResources;
+  std::map<int, std::map<std::string, ResourceAllocationError>>
+    ResourceAllocationErrors;
   cmCTestResourceAllocator ResourceAllocator;
   std::vector<cmCTestTestHandler::cmCTestTestResult>* TestResults;
   size_t ParallelLevel; // max number of process that can be run at once
diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index cc5de43..ec54960 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -324,8 +324,9 @@ bool cmCTestRunTest::StartAgain(size_t completed)
   cmWorkingDirectory workdir(this->TestProperties->Directory);
   if (workdir.Failed()) {
     this->StartFailure("Failed to change working directory to " +
-                       this->TestProperties->Directory + " : " +
-                       std::strerror(workdir.GetLastResult()));
+                         this->TestProperties->Directory + " : " +
+                         std::strerror(workdir.GetLastResult()),
+                       "Failed to change working directory");
     return true;
   }
 
@@ -381,7 +382,8 @@ void cmCTestRunTest::MemCheckPostProcess()
   handler->PostProcessTest(this->TestResult, this->Index);
 }
 
-void cmCTestRunTest::StartFailure(std::string const& output)
+void cmCTestRunTest::StartFailure(std::string const& output,
+                                  std::string const& detail)
 {
   // Still need to log the Start message so the test summary records our
   // attempt to start this test
@@ -404,7 +406,7 @@ void cmCTestRunTest::StartFailure(std::string const& output)
   this->TestResult.ExecutionTime = cmDuration::zero();
   this->TestResult.CompressOutput = false;
   this->TestResult.ReturnValue = -1;
-  this->TestResult.CompletionStatus = "Failed to start";
+  this->TestResult.CompletionStatus = detail;
   this->TestResult.Status = cmCTestTestHandler::NOT_RUN;
   this->TestResult.TestCount = this->TestProperties->Index;
   this->TestResult.Name = this->TestProperties->Name;
diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h
index 7eeaebd..4988839 100644
--- a/Source/CTest/cmCTestRunTest.h
+++ b/Source/CTest/cmCTestRunTest.h
@@ -76,7 +76,7 @@ public:
 
   bool StartAgain(size_t completed);
 
-  void StartFailure(std::string const& output);
+  void StartFailure(std::string const& output, std::string const& detail);
 
   cmCTest* GetCTest() const { return this->CTest; }
 
diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx
index 78c68be..4f324ea 100644
--- a/Source/CTest/cmCTestTestHandler.cxx
+++ b/Source/CTest/cmCTestTestHandler.cxx
@@ -548,6 +548,7 @@ bool cmCTestTestHandler::ProcessOptions()
   val = this->GetOption("ResourceSpecFile");
   if (val) {
     this->UseResourceSpec = true;
+    this->ResourceSpecFile = val;
     auto result = this->ResourceSpec.ReadFromJSONFile(val);
     if (result != cmCTestResourceSpec::ReadFileResult::READ_OK) {
       cmCTestLog(this->CTest, ERROR_MESSAGE,
diff --git a/Source/CTest/cmCTestTestHandler.h b/Source/CTest/cmCTestTestHandler.h
index 55237f9..b1c8755 100644
--- a/Source/CTest/cmCTestTestHandler.h
+++ b/Source/CTest/cmCTestTestHandler.h
@@ -338,6 +338,7 @@ private:
 
   bool UseResourceSpec;
   cmCTestResourceSpec ResourceSpec;
+  std::string ResourceSpecFile;
 
   void GenerateRegressionImages(cmXMLWriter& xml, const std::string& dart);
   cmsys::RegularExpression DartStuff1;