From 302593ded511b49190bca0af8a6d50d18a4c2c14 Mon Sep 17 00:00:00 2001 From: davidot Date: Tue, 13 Sep 2022 23:53:45 +0200 Subject: [PATCH] test-test262: Port to Core::Stream and use TRY more The only complication here is that Core::Stream::File is not RefCounted meaning we have to use OwnPtr instead of RefPtr. Unfortunately we cannot propagate errors as some errors must be caught and dealt with as the runner can do anything (like stop at any moment or close pipes). --- Tests/LibJS/test-test262.cpp | 95 ++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 48 deletions(-) diff --git a/Tests/LibJS/test-test262.cpp b/Tests/LibJS/test-test262.cpp index 8dad872073..92b043d262 100644 --- a/Tests/LibJS/test-test262.cpp +++ b/Tests/LibJS/test-test262.cpp @@ -15,6 +15,8 @@ #include #include #include +#include +#include #include #include #include @@ -111,47 +113,34 @@ static constexpr StringView total_test_emoji = "🧪"sv; class Test262RunnerHandler { public: - static OwnPtr create(char const* const command[]) + static ErrorOr> create(StringView command, char const* const arguments[]) { - int write_pipe_fds[2]; - int read_pipe_fds[2]; - if (pipe2(write_pipe_fds, O_CLOEXEC) < 0) { - perror("pipe2"); - return nullptr; - } - - if (pipe2(read_pipe_fds, O_CLOEXEC) < 0) { - perror("pipe2"); - return nullptr; - } + auto write_pipe_fds = TRY(Core::System::pipe2(O_CLOEXEC)); + auto read_pipe_fds = TRY(Core::System::pipe2(O_CLOEXEC)); posix_spawn_file_actions_t file_actions; posix_spawn_file_actions_init(&file_actions); posix_spawn_file_actions_adddup2(&file_actions, write_pipe_fds[0], STDIN_FILENO); posix_spawn_file_actions_adddup2(&file_actions, read_pipe_fds[1], STDOUT_FILENO); - pid_t pid {}; - - if (posix_spawnp(&pid, command[0], &file_actions, nullptr, const_cast(command), environ) < 0) { - perror("posix_spawnp"); - return nullptr; - } + auto pid = TRY(Core::System::posix_spawnp(command, &file_actions, nullptr, const_cast(arguments), environ)); posix_spawn_file_actions_destroy(&file_actions); + ArmedScopeGuard runner_kill { [&pid] { kill(pid, SIGKILL); } }; - close(write_pipe_fds[0]); - close(read_pipe_fds[1]); + TRY(Core::System::close(write_pipe_fds[0])); + TRY(Core::System::close(read_pipe_fds[1])); - auto infile = Core::File::construct(); - infile->open(read_pipe_fds[0], Core::OpenMode::ReadOnly, Core::File::ShouldCloseFileDescriptor::Yes); + auto infile = TRY(Core::Stream::File::adopt_fd(read_pipe_fds[0], Core::Stream::OpenMode::Read)); - auto outfile = Core::File::construct(); - outfile->open(write_pipe_fds[1], Core::OpenMode::WriteOnly, Core::File::ShouldCloseFileDescriptor::Yes); + auto outfile = TRY(Core::Stream::File::adopt_fd(write_pipe_fds[1], Core::Stream::OpenMode::Write)); + + runner_kill.disarm(); return make(pid, move(infile), move(outfile)); } - Test262RunnerHandler(pid_t pid, NonnullRefPtr in_file, NonnullRefPtr out_file) + Test262RunnerHandler(pid_t pid, NonnullOwnPtr in_file, NonnullOwnPtr out_file) : m_pid(pid) , m_input(move(in_file)) , m_output(move(out_file)) @@ -162,17 +151,17 @@ public: { // It's possible the process dies before we can write all the tests // to the stdin. So make sure that we don't crash but just stop writing. - struct sigaction action_handler { }; + struct sigaction action_handler { + .sa_handler = SIG_IGN, .sa_mask = {}, .sa_flags = 0, + }; struct sigaction old_action_handler; - action_handler.sa_flags = 0; - action_handler.sa_handler = SIG_IGN; if (sigaction(SIGPIPE, &action_handler, &old_action_handler) < 0) { perror("sigaction"); return false; } for (String const& line : lines) { - if (!m_output->write(String::formatted("{}\n", line))) + if (!m_output->write_or_error(String::formatted("{}\n", line).bytes())) break; } @@ -188,7 +177,12 @@ public: String read_all() { - return String(m_input->read_all().bytes(), Chomp); + auto all_output_or_error = m_input->read_all(); + if (all_output_or_error.is_error()) { + warnln("Got error: {} while reading runner output", all_output_or_error.error()); + return ""sv; + } + return String(all_output_or_error.value().bytes(), Chomp); } enum class ProcessResult { @@ -199,23 +193,24 @@ public: Unknown, }; - ProcessResult status() + ErrorOr status() { if (m_pid == -1) return ProcessResult::Unknown; - int wstatus { 0 }; - int rc = waitpid(m_pid, &wstatus, WNOHANG); - if (rc == 0) { + m_output->close(); + + auto wait_result = TRY(Core::System::waitpid(m_pid, WNOHANG)); + if (wait_result.pid == 0) { // Attempt to kill it, since it has not finished yet somehow return ProcessResult::Running; } m_pid = -1; - if (WIFSIGNALED(wstatus) && WTERMSIG(wstatus) == SIGALRM) + if (WIFSIGNALED(wait_result.status) && WTERMSIG(wait_result.status) == SIGALRM) return ProcessResult::FailedFromTimeout; - if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) + if (WIFEXITED(wait_result.status) && WEXITSTATUS(wait_result.status) == 0) return ProcessResult::DoneWithZeroExitCode; return ProcessResult::Failed; @@ -223,11 +218,11 @@ public: public: pid_t m_pid; - NonnullRefPtr m_input; - NonnullRefPtr m_output; + NonnullOwnPtr m_input; + NonnullOwnPtr m_output; }; -static HashMap run_test_files(Span files, size_t offset, char const* const command[]) +static HashMap run_test_files(Span files, size_t offset, StringView command, char const* const arguments[]) { HashMap results {}; results.ensure_capacity(files.size()); @@ -239,11 +234,12 @@ static HashMap run_test_files(Span files, size_t off }; while (test_index < files.size()) { - auto runner_process = Test262RunnerHandler::create(command); - if (!runner_process) { + auto runner_process_or_error = Test262RunnerHandler::create(command, arguments); + if (runner_process_or_error.is_error()) { fail_all_after(); return results; } + auto& runner_process = runner_process_or_error.value(); if (!runner_process->write_lines(files.slice(test_index))) { fail_all_after(); @@ -251,9 +247,12 @@ static HashMap run_test_files(Span files, size_t off } String output = runner_process->read_all(); - auto status = runner_process->status(); - VERIFY(status != Test262RunnerHandler::ProcessResult::Running); - bool failed = status != Test262RunnerHandler::ProcessResult::DoneWithZeroExitCode; + auto status_or_error = runner_process->status(); + bool failed = false; + if (!status_or_error.is_error()) { + VERIFY(status_or_error.value() != Test262RunnerHandler::ProcessResult::Running); + failed = status_or_error.value() != Test262RunnerHandler::ProcessResult::DoneWithZeroExitCode; + } for (StringView line : output.split_view('\n')) { if (!line.starts_with("RESULT "sv)) @@ -286,7 +285,7 @@ static HashMap run_test_files(Span files, size_t off if (failed) { TestResult result = TestResult::ProcessError; - if (status == Test262RunnerHandler::ProcessResult::FailedFromTimeout) { + if (!status_or_error.is_error() && status_or_error.value() == Test262RunnerHandler::ProcessResult::FailedFromTimeout) { result = TestResult::TimeoutError; } // assume the last test failed, if by SIGALRM signal it's a timeout @@ -379,7 +378,7 @@ ErrorOr serenity_main(Main::Arguments arguments) while (index < paths.size()) { print_progress(); auto this_batch_size = min(batch_size, paths.size() - index); - auto batch_results = run_test_files(paths.span().slice(index, this_batch_size), index, raw_args.data()); + auto batch_results = run_test_files(paths.span().slice(index, this_batch_size), index, args[0], raw_args.data()); results.ensure_capacity(results.size() + batch_results.size()); for (auto& [key, value] : batch_results) { @@ -412,7 +411,7 @@ ErrorOr serenity_main(Main::Arguments arguments) void write_per_file(HashMap const& result_map, Vector const& paths, StringView per_file_name, double time_taken_in_ms) { - auto file_or_error = Core::File::open(per_file_name, Core::OpenMode::WriteOnly); + auto file_or_error = Core::Stream::File::open(per_file_name, Core::Stream::OpenMode::Write); if (file_or_error.is_error()) { warnln("Failed to open per file for writing at {}: {}", per_file_name, file_or_error.error().string_literal()); return; @@ -428,7 +427,7 @@ void write_per_file(HashMap const& result_map, Vectorwrite(complete_results.to_string())) + if (!file->write_or_error(complete_results.to_string().bytes())) warnln("Failed to write per-file"); file->close(); }