From 0ecfec00a5b8b3fc9cb856d07849b4b635e0f263 Mon Sep 17 00:00:00 2001 From: Justin Date: Tue, 17 Sep 2024 18:34:21 -0600 Subject: [PATCH] Make testfile ids atomic and create ins and out files on demand --- include/testharness/TestHarness.h | 8 ++-- include/tests/TestFile.h | 11 ++++-- include/toolchain/Command.h | 6 +-- include/toolchain/ExecutionState.h | 6 ++- src/testharness/TestHarness.cpp | 2 +- src/tests/TestFile.cpp | 47 +++++++++------------- src/tests/TestParser.cpp | 62 ++++++++++++++++-------------- src/tests/TestRunning.cpp | 1 - src/toolchain/Command.cpp | 34 +++++++++------- src/toolchain/ToolChain.cpp | 4 +- 10 files changed, 95 insertions(+), 86 deletions(-) diff --git a/include/testharness/TestHarness.h b/include/testharness/TestHarness.h index 89d5ae11..40529b6c 100644 --- a/include/testharness/TestHarness.h +++ b/include/testharness/TestHarness.h @@ -33,8 +33,8 @@ class TestHarness { results() { // Create temporary dir for test and toolchain files - tmpPath = fs::path(cfg.getConfigDirPath() / "tmp"); - fs::create_directory(tmpPath); + testArtifactsPath = fs::path(cfg.getConfigDirPath() / ".test-artifacts"); + fs::create_directory(testArtifactsPath); // Find tests findTests(); @@ -62,8 +62,8 @@ class TestHarness { // let derived classes find tests. void findTests(); - // Create a local tmp path for every test run - fs::path tmpPath; + // Create a local tmp path for ephemeral test input, output and toolchain files + fs::path testArtifactsPath; private: // The results of the tests. diff --git a/include/tests/TestFile.h b/include/tests/TestFile.h index 95fdab8f..4c1723e1 100644 --- a/include/tests/TestFile.h +++ b/include/tests/TestFile.h @@ -8,6 +8,7 @@ #include #include #include +#include namespace fs = std::filesystem; @@ -27,10 +28,10 @@ class TestFile { public: TestFile() = delete; // construct Testfile from path to .test file. - TestFile(const fs::path& path, const fs::path& tmpPath); + TestFile(const fs::path& path, const fs::path& artifactDir); ~TestFile(); - uint64_t id; + uint64_t getId() const { return id; } // Test path getters const fs::path& getTestPath() const { return testPath; } @@ -54,10 +55,14 @@ class TestFile { friend class TestParser; +private: + static uint64_t generateId(); + protected: - static uint64_t nextId; + static std::atomic nextId; private: + uint64_t id; // Path for the test, ins and out files fs::path testPath; fs::path outPath; diff --git a/include/toolchain/Command.h b/include/toolchain/Command.h index 4ddfe392..a742d3c6 100644 --- a/include/toolchain/Command.h +++ b/include/toolchain/Command.h @@ -58,15 +58,13 @@ class Command { std::string name; fs::path exePath; fs::path runtimePath; + fs::path tmpPath; std::vector args; - + // Every command produces a file descriptor for each of these paths fs::path errPath; fs::path outPath; - // The command can supply a output file to use instead of stdout/err - std::optional outputFile; - // Uses runtime and uses input stream. bool usesRuntime, usesInStr; diff --git a/include/toolchain/ExecutionState.h b/include/toolchain/ExecutionState.h index 1de14f46..cfc836d9 100644 --- a/include/toolchain/ExecutionState.h +++ b/include/toolchain/ExecutionState.h @@ -16,8 +16,10 @@ class ExecutionInput { // Creates input to a subprocess execution. ExecutionInput(fs::path inputPath, fs::path inputStreamPath, fs::path testedExecutable, fs::path testedRuntime) - : inputPath(std::move(inputPath)), inputStreamPath(std::move(inputStreamPath)), - testedExecutable(std::move(testedExecutable)), testedRuntime(std::move(testedRuntime)) {} + : inputPath(std::move(inputPath)), + inputStreamPath(std::move(inputStreamPath)), + testedExecutable(std::move(testedExecutable)), + testedRuntime(std::move(testedRuntime)) {} // Gets input file. const fs::path& getInputFile() const { return inputPath; } diff --git a/src/testharness/TestHarness.cpp b/src/testharness/TestHarness.cpp index 8ee78732..694dd04d 100644 --- a/src/testharness/TestHarness.cpp +++ b/src/testharness/TestHarness.cpp @@ -146,7 +146,7 @@ bool hasTestFiles(const fs::path& path) { } void TestHarness::addTestFileToSubPackage(SubPackage& subPackage, const fs::path& file) { - auto testfile = std::make_unique(file, tmpPath); + auto testfile = std::make_unique(file, testArtifactsPath); TestParser parser(testfile.get()); diff --git a/src/tests/TestFile.cpp b/src/tests/TestFile.cpp index a32fc690..1129b032 100644 --- a/src/tests/TestFile.cpp +++ b/src/tests/TestFile.cpp @@ -1,6 +1,8 @@ #include "tests/TestFile.h" #include "tests/TestParser.h" +static uint64_t nextId = 0; + namespace { std::string stripFileExtension(const std::string& str) { @@ -12,53 +14,42 @@ std::string stripFileExtension(const std::string& str) { namespace tester { -uint64_t TestFile::nextId = 0; +// Initialize the static id to zero +std::atomic TestFile::nextId(0); + +uint64_t TestFile::generateId() { + return nextId.fetch_add(1, std::memory_order_relaxed); +} -TestFile::TestFile(const fs::path& path, const fs::path& tmpPath) - : testPath(path) { +TestFile::TestFile(const fs::path& path, const fs::path& artifactDir) + : id(generateId()), testPath(path) { - fs::path testDir = tmpPath / std::to_string(nextId); - setInsPath(testDir / "test.ins"); - setOutPath(testDir / "test.out"); + std::string testName = path.stem(); + setInsPath(artifactDir / fs::path(testName + std::to_string(id) + ".ins")); + setOutPath(artifactDir / fs::path(testName + std::to_string(id) + ".out")); try { // Create tmp directory if it doesn't exist - std::cout << "Attempting to create directory: " << testDir << std::endl; - if (!fs::exists(testDir)) { - if (!fs::create_directories(testDir)) { - throw std::runtime_error("Failed to create directory: " + testDir.string()); + if (!fs::exists(artifactDir)) { + if (!fs::create_directories(artifactDir)) { + throw std::runtime_error("Failed to create directory: " + artifactDir.string()); } } - // Create the temporary input and ouput files - std::ofstream createInsFile(insPath); - std::ofstream createOutFile(outPath); - if (!createInsFile) { - throw std::runtime_error("Failed to create input file: " + insPath.string()); - } - if (!createOutFile) { - throw std::runtime_error("Failed to create output file: " + outPath.string()); - } - createInsFile.close(); - createOutFile.close(); - } catch (const fs::filesystem_error& e) { throw std::runtime_error("Filesystem error: " + std::string(e.what())); - } catch (const std::exception& e) { - throw std::runtime_error("Error in TestFile constructor: " + std::string(e.what())); - } - nextId++; + } } TestFile::~TestFile() { + + std::cout << "Calling Destructor...\n"; if (fs::exists(insPath)) { // Remove temporary input stream file fs::remove(insPath); } if (fs::exists(outPath)) { // Remove the tenmporary testfile directory and the expected out - fs::path testfileDir = outPath.parent_path(); fs::remove(outPath); - fs::remove(testfileDir); } } diff --git a/src/tests/TestParser.cpp b/src/tests/TestParser.cpp index 4cda6012..ce81a720 100644 --- a/src/tests/TestParser.cpp +++ b/src/tests/TestParser.cpp @@ -14,6 +14,26 @@ bool fullyContains(const std::string& str, const std::string& substr) { return str.substr(pos, substr.length()) == substr; } +ParseError copyFile(const fs::path& from, const fs::path& to) { + + // Open the files to operate upon + std::ifstream sourceFile(from, std::ios::binary); + std::ofstream destFile(to, std::ios::binary); + + // Check for errors opening + if (!destFile || !sourceFile) { + return ParseError::FileError; + } + + // Write the contents and check for errors + destFile << sourceFile.rdbuf(); + if (sourceFile.fail() || destFile.fail()) { + return ParseError::FileError; + } + + return ParseError::NoError; +} + void TestParser::insLineToFile(fs::path filePath, std::string line, bool firstInsert) { // open in append mode since otherwise multi-line checks and inputs would // over-write themselves. @@ -99,31 +119,15 @@ ParseError TestParser::matchInputFileDirective(std::string& line) { if (foundInput) return ParseError::DirectiveConflict; - PathOrError inputFilePath = parsePathFromLine(line, Directive::INPUT_FILE); - if (std::holds_alternative(inputFilePath)) { - - // Extract the path from the variant - std::filesystem::path path = std::get(inputFilePath); - - // Open the supplied check file and read into - std::ifstream sourceFile(path, std::ios::binary); - std::ofstream destFile(testfile->getInsPath(), std::ios::binary); - if (!destFile) { - return ParseError::FileError; - } else if (!sourceFile) { - return ParseError::FileError; - } - - destFile << sourceFile.rdbuf(); - - if (sourceFile.fail() || destFile.fail()) { - return ParseError::FileError; - } - + PathOrError path = parsePathFromLine(line, Directive::INPUT_FILE); + if (std::holds_alternative(path)) { + // Copy the input file referenced into the testfiles ephemeral ins file + auto inputPath = std::get(path); + copyFile(inputPath, testfile->getInsPath()); foundInputFile = true; - return ParseError::NoError; } - return std::get(inputFilePath); + + return std::get(path); } /** @@ -137,14 +141,16 @@ ParseError TestParser::matchCheckFileDirective(std::string& line) { if (foundCheck) return ParseError::DirectiveConflict; - PathOrError pathOrError = parsePathFromLine(line, Directive::CHECK_FILE); - if (std::holds_alternative(pathOrError)) { - testfile->setOutPath(std::get(pathOrError)); - foundCheckFile = true; + PathOrError path = parsePathFromLine(line, Directive::CHECK_FILE); + if (std::holds_alternative(path)) { + // Copy the input file referenced into the testfiles ephemeral ins file + auto outputPath = std::get(path); + copyFile(outputPath, testfile->getOutPath()); + foundCheckFile= true; return ParseError::NoError; } - return std::get(pathOrError); + return std::get(path); } /** diff --git a/src/tests/TestRunning.cpp b/src/tests/TestRunning.cpp index a4ac1015..47a9acda 100644 --- a/src/tests/TestRunning.cpp +++ b/src/tests/TestRunning.cpp @@ -193,7 +193,6 @@ TestResult runTest(TestFile* test, const ToolChain& toolChain, const Config& cfg const fs::path testPath = test->getTestPath(); const fs::path expOutPath = test->getOutPath(); - const fs::path insPath = test->getInsPath(); fs::path genOutPath; std::string genErrorString, expErrorString, diffString; diff --git a/src/toolchain/Command.cpp b/src/toolchain/Command.cpp index 0eac32e4..9fbac551 100644 --- a/src/toolchain/Command.cpp +++ b/src/toolchain/Command.cpp @@ -105,8 +105,8 @@ void runCommand(std::promise& promise, std::atomic_bool& killVar, const std::string& output, const std::string& error, const std::string& runtime) { - - + + std::cerr << "Running Command: " << exe << std::endl; std::cerr << "Made out file: " << input << std::endl; std::cerr << "Made error file: " << output << std::endl; @@ -178,6 +178,9 @@ namespace tester { Command::Command(const JSON& step, int64_t timeout) : usesRuntime(false), usesInStr(false), timeout(timeout) { + + fs::path tmpPath = "./tmp"; + // Make sure the step has all of the values needed for construction. ensureContains(step, "stepName"); ensureContains(step, "executablePath"); @@ -188,20 +191,23 @@ Command::Command(const JSON& step, int64_t timeout) for (std::string arg : step["arguments"]) args.push_back(arg); - // If no output path is supplied by default, temporaries are created to capture stdout and stderr. - std::string output_name = std::string(step["stepName"]) + ".stdout"; - std::string error_name = std::string(step["stepName"]) + ".stderr"; - outPath = fs::temp_directory_path() / output_name; - errPath = fs::temp_directory_path() / error_name; - // Set the executable path std::string path = step["executablePath"]; exePath = fs::path(path); - // Allow override of stdout path - if (doesContain(step, "output")) - outputFile = fs::path(step["output"]); + // Allow override of stdout path with output property + if (doesContain(step, "output")) { + outPath = tmpPath / fs::path(step["output"]); + } else { + std::string output_name = std::string(step["stepName"]) + ".stdout"; + outPath = tmpPath / output_name; + } + std::cout << "Created commadn with outpath: " << outPath << std::endl; + // Always create a stderr path + std::string error_name = std::string(step["stepName"]) + ".stderr"; + errPath = tmpPath / error_name; + // Do we use an input stream file? if (doesContain(step, "usesInStr")) usesInStr = step["usesInStr"]; @@ -212,13 +218,12 @@ Command::Command(const JSON& step, int64_t timeout) // Do we allow errors? if (doesContain(step, "allowError")) - allowError = step["allowError"]; + allowError = step["allowError"]; } ExecutionOutput Command::execute(const ExecutionInput& ei) const { // Create our output context. - fs::path out = outputFile.has_value() ? *outputFile : outPath; - ExecutionOutput eo(out, errPath); + ExecutionOutput eo(outPath, errPath); // Always remove old output files so we know if a new one was created std::error_code ec; @@ -230,6 +235,7 @@ ExecutionOutput Command::execute(const ExecutionInput& ei) const { for (const std::string& arg : args) trueArgs.emplace_back(resolveArg(ei, eo, arg).string()); + std::cout << "OUT PATH: " << outPath << std::endl; // Get the runtime path and standard out file, the things used in setting up // the execution of the command. std::string runtimeStr = usesRuntime ? ei.getTestedRuntime().string() : ""; diff --git a/src/toolchain/ToolChain.cpp b/src/toolchain/ToolChain.cpp index 70441512..8cb90714 100644 --- a/src/toolchain/ToolChain.cpp +++ b/src/toolchain/ToolChain.cpp @@ -34,7 +34,9 @@ ExecutionOutput ToolChain::build(TestFile* test) const { eo.setIsErrorTest(true); return eo; } - + + // The input for the next command is the output of the previous command, along with + // the same input stream, tested executable and runtime, which is shared for all commands. ei = ExecutionInput(eo.getOutputFile(), ei.getInputStreamFile(), ei.getTestedExecutable(), ei.getTestedRuntime()); }