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

3 files changed, 81 insertions, 23 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;
     }
   }