From b393b32b4bb0bc830edc89df6262ad710cd0a3e2 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Thu, 26 Dec 2019 16:02:52 -0500 Subject: CTest: Improve error handling when reading resource spec file Fixes: #20079 --- Source/CTest/cmCTestResourceSpec.cxx | 79 +++++++++---- Source/CTest/cmCTestResourceSpec.h | 17 ++- Source/CTest/cmCTestTestHandler.cxx | 8 +- Tests/CMakeLib/testCTestResourceSpec.cxx | 122 ++++++++++++--------- .../CTestResourceAllocation/ctresalloc.cxx | 3 +- 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> 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 8e3ac22..c8bbb0b 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -513,9 +513,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 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 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; } -- cgit v0.12