Merge topic 'ctest-resource-allocation-spec-message'

b393b32b4b CTest: Improve error handling when reading resource spec file

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

5 files changed, 153 insertions, 76 deletions

diff --git a/Source/CTest/cmCTestResourceSpec.cxx b/Source/CTest/cmCTestResourceSpec.cxx
index 237a745..8f91efb 100644
--- a/Source/CTest/cmCTestResourceSpec.cxx
+++ b/Source/CTest/cmCTestResourceSpec.cxx
@@ -16,21 +16,22 @@
 static const cmsys::RegularExpression IdentifierRegex{ "^[a-z_][a-z0-9_]*$" };
 static const cmsys::RegularExpression IdRegex{ "^[a-z0-9_]+$" };
 
-bool cmCTestResourceSpec::ReadFromJSONFile(const std::string& filename)
+cmCTestResourceSpec::ReadFileResult cmCTestResourceSpec::ReadFromJSONFile(
+  const std::string& filename)
 {
   cmsys::ifstream fin(filename.c_str());
   if (!fin) {
-    return false;
+    return ReadFileResult::FILE_NOT_FOUND;
   }
 
   Json::Value root;
   Json::CharReaderBuilder builder;
   if (!Json::parseFromStream(builder, fin, &root, nullptr)) {
-    return false;
+    return ReadFileResult::JSON_PARSE_ERROR;
   }
 
   if (!root.isObject()) {
-    return false;
+    return ReadFileResult::INVALID_ROOT;
   }
 
   int majorVersion = 1;
@@ -39,42 +40,42 @@ bool cmCTestResourceSpec::ReadFromJSONFile(const std::string& filename)
     auto const& version = root["version"];
     if (version.isObject()) {
       if (!version.isMember("major") || !version.isMember("minor")) {
-        return false;
+        return ReadFileResult::INVALID_VERSION;
       }
       auto const& major = version["major"];
       auto const& minor = version["minor"];
       if (!major.isInt() || !minor.isInt()) {
-        return false;
+        return ReadFileResult::INVALID_VERSION;
       }
       majorVersion = major.asInt();
       minorVersion = minor.asInt();
     } else {
-      return false;
+      return ReadFileResult::INVALID_VERSION;
     }
   } else {
-    return false;
+    return ReadFileResult::NO_VERSION;
   }
 
   if (majorVersion != 1 || minorVersion != 0) {
-    return false;
+    return ReadFileResult::UNSUPPORTED_VERSION;
   }
 
   auto const& local = root["local"];
   if (!local.isArray()) {
-    return false;
+    return ReadFileResult::INVALID_SOCKET_SPEC;
   }
   if (local.size() > 1) {
-    return false;
+    return ReadFileResult::INVALID_SOCKET_SPEC;
   }
 
   if (local.empty()) {
     this->LocalSocket.Resources.clear();
-    return true;
+    return ReadFileResult::READ_OK;
   }
 
   auto const& localSocket = local[0];
   if (!localSocket.isObject()) {
-    return false;
+    return ReadFileResult::INVALID_SOCKET_SPEC;
   }
   std::map<std::string, std::vector<cmCTestResourceSpec::Resource>> resources;
   cmsys::RegularExpressionMatch match;
@@ -88,21 +89,21 @@ bool cmCTestResourceSpec::ReadFromJSONFile(const std::string& filename)
             cmCTestResourceSpec::Resource resource;
 
             if (!item.isMember("id")) {
-              return false;
+              return ReadFileResult::INVALID_RESOURCE;
             }
             auto const& id = item["id"];
             if (!id.isString()) {
-              return false;
+              return ReadFileResult::INVALID_RESOURCE;
             }
             resource.Id = id.asString();
             if (!IdRegex.find(resource.Id.c_str(), match)) {
-              return false;
+              return ReadFileResult::INVALID_RESOURCE;
             }
 
             if (item.isMember("slots")) {
               auto const& capacity = item["slots"];
               if (!capacity.isConvertibleTo(Json::uintValue)) {
-                return false;
+                return ReadFileResult::INVALID_RESOURCE;
               }
               resource.Capacity = capacity.asUInt();
             } else {
@@ -111,17 +112,55 @@ bool cmCTestResourceSpec::ReadFromJSONFile(const std::string& filename)
 
             r.push_back(resource);
           } else {
-            return false;
+            return ReadFileResult::INVALID_RESOURCE;
           }
         }
       } else {
-        return false;
+        return ReadFileResult::INVALID_RESOURCE_TYPE;
       }
     }
   }
 
   this->LocalSocket.Resources = std::move(resources);
-  return true;
+  return ReadFileResult::READ_OK;
+}
+
+const char* cmCTestResourceSpec::ResultToString(ReadFileResult result)
+{
+  switch (result) {
+    case ReadFileResult::READ_OK:
+      return "OK";
+
+    case ReadFileResult::FILE_NOT_FOUND:
+      return "File not found";
+
+    case ReadFileResult::JSON_PARSE_ERROR:
+      return "JSON parse error";
+
+    case ReadFileResult::INVALID_ROOT:
+      return "Invalid root object";
+
+    case ReadFileResult::NO_VERSION:
+      return "No version specified";
+
+    case ReadFileResult::INVALID_VERSION:
+      return "Invalid version object";
+
+    case ReadFileResult::UNSUPPORTED_VERSION:
+      return "Unsupported version";
+
+    case ReadFileResult::INVALID_SOCKET_SPEC:
+      return "Invalid socket object";
+
+    case ReadFileResult::INVALID_RESOURCE_TYPE:
+      return "Invalid resource type object";
+
+    case ReadFileResult::INVALID_RESOURCE:
+      return "Invalid resource object";
+
+    default:
+      return "Unknown";
+  }
 }
 
 bool cmCTestResourceSpec::operator==(const cmCTestResourceSpec& other) const
diff --git a/Source/CTest/cmCTestResourceSpec.h b/Source/CTest/cmCTestResourceSpec.h
index 4646db8..cb242c0 100644
--- a/Source/CTest/cmCTestResourceSpec.h
+++ b/Source/CTest/cmCTestResourceSpec.h
@@ -31,7 +31,22 @@ public:
 
   Socket LocalSocket;
 
-  bool ReadFromJSONFile(const std::string& filename);
+  enum class ReadFileResult
+  {
+    READ_OK,
+    FILE_NOT_FOUND,
+    JSON_PARSE_ERROR,
+    INVALID_ROOT,
+    NO_VERSION,
+    INVALID_VERSION,
+    UNSUPPORTED_VERSION,
+    INVALID_SOCKET_SPEC, // Can't be INVALID_SOCKET due to a Windows macro
+    INVALID_RESOURCE_TYPE,
+    INVALID_RESOURCE,
+  };
+
+  ReadFileResult ReadFromJSONFile(const std::string& filename);
+  static const char* ResultToString(ReadFileResult result);
 
   bool operator==(const cmCTestResourceSpec& other) const;
   bool operator!=(const cmCTestResourceSpec& other) const;
diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx
index 37c7154..e70bc5a 100644
--- a/Source/CTest/cmCTestTestHandler.cxx
+++ b/Source/CTest/cmCTestTestHandler.cxx
@@ -537,9 +537,13 @@ bool cmCTestTestHandler::ProcessOptions()
   val = this->GetOption("ResourceSpecFile");
   if (val) {
     this->UseResourceSpec = true;
-    if (!this->ResourceSpec.ReadFromJSONFile(val)) {
+    auto result = this->ResourceSpec.ReadFromJSONFile(val);
+    if (result != cmCTestResourceSpec::ReadFileResult::READ_OK) {
       cmCTestLog(this->CTest, ERROR_MESSAGE,
-                 "Could not read resource spec file: " << val << std::endl);
+                 "Could not read/parse resource spec file "
+                   << val << ": "
+                   << cmCTestResourceSpec::ResultToString(result)
+                   << std::endl);
       return false;
     }
   }
diff --git a/Tests/CMakeLib/testCTestResourceSpec.cxx b/Tests/CMakeLib/testCTestResourceSpec.cxx
index 99bee56..b49f8ff 100644
--- a/Tests/CMakeLib/testCTestResourceSpec.cxx
+++ b/Tests/CMakeLib/testCTestResourceSpec.cxx
@@ -7,71 +7,89 @@
 struct ExpectedSpec
 {
   std::string Path;
-  bool ParseResult;
+  cmCTestResourceSpec::ReadFileResult ParseResult;
   cmCTestResourceSpec Expected;
 };
 
 static const std::vector<ExpectedSpec> expectedResourceSpecs = {
-  /* clang-format off */
-  {"spec1.json", true, {{{
-    {"gpus", {
-      {"2", 4},
-      {"e", 1},
-    }},
-    {"threads", {
-    }},
-  }}}},
-  {"spec2.json", true, {}},
-  {"spec3.json", false, {}},
-  {"spec4.json", false, {}},
-  {"spec5.json", false, {}},
-  {"spec6.json", false, {}},
-  {"spec7.json", false, {}},
-  {"spec8.json", false, {}},
-  {"spec9.json", false, {}},
-  {"spec10.json", false, {}},
-  {"spec11.json", false, {}},
-  {"spec12.json", false, {}},
-  {"spec13.json", false, {}},
-  {"spec14.json", true, {}},
-  {"spec15.json", true, {}},
-  {"spec16.json", true, {}},
-  {"spec17.json", false, {}},
-  {"spec18.json", false, {}},
-  {"spec19.json", false, {}},
-  {"spec20.json", true, {}},
-  {"spec21.json", false, {}},
-  {"spec22.json", false, {}},
-  {"spec23.json", false, {}},
-  {"spec24.json", false, {}},
-  {"spec25.json", false, {}},
-  {"spec26.json", false, {}},
-  {"spec27.json", false, {}},
-  {"spec28.json", false, {}},
-  {"spec29.json", false, {}},
-  {"spec30.json", false, {}},
-  {"spec31.json", false, {}},
-  {"spec32.json", false, {}},
-  {"spec33.json", false, {}},
-  {"spec34.json", false, {}},
-  {"spec35.json", false, {}},
-  {"spec36.json", false, {}},
-  {"noexist.json", false, {}},
-  /* clang-format on */
+  { "spec1.json",
+    cmCTestResourceSpec::ReadFileResult::READ_OK,
+    { { {
+      { "gpus",
+        {
+          { "2", 4 },
+          { "e", 1 },
+        } },
+      { "threads", {} },
+    } } } },
+  { "spec2.json", cmCTestResourceSpec::ReadFileResult::READ_OK, {} },
+  { "spec3.json",
+    cmCTestResourceSpec::ReadFileResult::INVALID_SOCKET_SPEC,
+    {} },
+  { "spec4.json",
+    cmCTestResourceSpec::ReadFileResult::INVALID_SOCKET_SPEC,
+    {} },
+  { "spec5.json",
+    cmCTestResourceSpec::ReadFileResult::INVALID_SOCKET_SPEC,
+    {} },
+  { "spec6.json",
+    cmCTestResourceSpec::ReadFileResult::INVALID_SOCKET_SPEC,
+    {} },
+  { "spec7.json",
+    cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE_TYPE,
+    {} },
+  { "spec8.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec9.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec10.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec11.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec12.json", cmCTestResourceSpec::ReadFileResult::INVALID_ROOT, {} },
+  { "spec13.json", cmCTestResourceSpec::ReadFileResult::JSON_PARSE_ERROR, {} },
+  { "spec14.json", cmCTestResourceSpec::ReadFileResult::READ_OK, {} },
+  { "spec15.json", cmCTestResourceSpec::ReadFileResult::READ_OK, {} },
+  { "spec16.json", cmCTestResourceSpec::ReadFileResult::READ_OK, {} },
+  { "spec17.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec18.json", cmCTestResourceSpec::ReadFileResult::INVALID_RESOURCE, {} },
+  { "spec19.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec20.json", cmCTestResourceSpec::ReadFileResult::READ_OK, {} },
+  { "spec21.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec22.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec23.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec24.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec25.json",
+    cmCTestResourceSpec::ReadFileResult::UNSUPPORTED_VERSION,
+    {} },
+  { "spec26.json",
+    cmCTestResourceSpec::ReadFileResult::UNSUPPORTED_VERSION,
+    {} },
+  { "spec27.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec28.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec29.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec30.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec31.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec32.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec33.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec34.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec35.json", cmCTestResourceSpec::ReadFileResult::INVALID_VERSION, {} },
+  { "spec36.json", cmCTestResourceSpec::ReadFileResult::NO_VERSION, {} },
+  { "noexist.json", cmCTestResourceSpec::ReadFileResult::FILE_NOT_FOUND, {} },
 };
 
-static bool testSpec(const std::string& path, bool expectedResult,
+static bool testSpec(const std::string& path,
+                     cmCTestResourceSpec::ReadFileResult expectedResult,
                      const cmCTestResourceSpec& expected)
 {
   cmCTestResourceSpec actual;
-  bool result = actual.ReadFromJSONFile(path);
+  auto result = actual.ReadFromJSONFile(path);
   if (result != expectedResult) {
-    std::cout << "ReadFromJSONFile(\"" << path << "\") returned " << result
-              << ", should be " << expectedResult << std::endl;
+    std::cout << "ReadFromJSONFile(\"" << path << "\") returned \""
+              << cmCTestResourceSpec::ResultToString(result)
+              << "\", should be \""
+              << cmCTestResourceSpec::ResultToString(expectedResult) << "\""
+              << std::endl;
     return false;
   }
 
-  if (result && actual != expected) {
+  if (actual != expected) {
     std::cout << "ReadFromJSONFile(\"" << path
               << "\") did not give expected spec" << std::endl;
     return false;
diff --git a/Tests/RunCMake/CTestResourceAllocation/ctresalloc.cxx b/Tests/RunCMake/CTestResourceAllocation/ctresalloc.cxx
index 27644af..80db05e 100644
--- a/Tests/RunCMake/CTestResourceAllocation/ctresalloc.cxx
+++ b/Tests/RunCMake/CTestResourceAllocation/ctresalloc.cxx
@@ -285,7 +285,8 @@ static int doVerify(int argc, char const* const* argv)
   std::set<std::string> testNameSet(testNameList.begin(), testNameList.end());
 
   cmCTestResourceSpec spec;
-  if (!spec.ReadFromJSONFile(resFile)) {
+  if (spec.ReadFromJSONFile(resFile) !=
+      cmCTestResourceSpec::ReadFileResult::READ_OK) {
     std::cout << "Could not read resource spec " << resFile << std::endl;
     return 1;
   }