diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp index 4e3b961c1914..3edca91e1efe 100644 --- a/testing/file_test/file_test_base.cpp +++ b/testing/file_test/file_test_base.cpp @@ -93,6 +93,14 @@ struct FileTestInfo { // expectations. bool autoupdate_differs = false; + // Mismatches between `fail_` prefixes and test success, set after running. + // Autoupdate doesn't fix these; they require manual renaming. + llvm::SmallVector fail_prefix_problems; + + // Whether the test is NOAUTOUPDATE and its output doesn't match expectations. + // Like `fail_prefix_problems`, this requires a manual fix. + bool noautoupdate_differs = false; + // Time spent in the test total, including processing and autoupdate. std::chrono::milliseconds elapsed_ms = std::chrono::milliseconds(0); }; @@ -120,35 +128,51 @@ static auto SplitOutput(llvm::StringRef output) llvm::StringRef(output).split(lines, "\n"); return llvm::SmallVector(lines.begin(), lines.end()); } -// Verify that the success and `fail_` prefix use correspond. Separately handle -// both cases for clearer test failures. -static auto CompareFailPrefix(llvm::StringRef filename, bool success) -> void { - if (success) { - EXPECT_FALSE(filename.starts_with("fail_")) - << "`" << filename - << "` succeeded; if success is expected, remove the `fail_` " - "prefix."; - } else { - EXPECT_TRUE(filename.starts_with("fail_")) - << "`" << filename - << "` failed; if failure is expected, add the `fail_` prefix."; - } -} +// Checks that success and `fail_` prefix use correspond for the test file and +// its splits. Returns a description of each mismatch found. These can't be +// fixed by autoupdate, so they're reported separately from output mismatches. +static auto GetFailPrefixProblems(llvm::StringRef test_name, + const TestFile& test_file) + -> llvm::SmallVector { + llvm::SmallVector problems; + auto check = [&](llvm::StringRef name, llvm::StringRef split_desc, + bool success) { + bool has_prefix = name.starts_with("fail_"); + if (success && has_prefix) { + problems.push_back(llvm::formatv( + "`{0}`{1} succeeded; if success is expected, remove the `fail_` " + "prefix.", + test_name, split_desc)); + } else if (!success && !has_prefix) { + problems.push_back(llvm::formatv( + "`{0}`{1} failed; if failure is expected, add the `fail_` prefix.", + test_name, split_desc)); + } + }; -// Verify that the success and `fail_` prefix use correspond for a split within -// a test file. -static auto CompareFailPrefix(llvm::StringRef filename, llvm::StringRef split, - bool success) -> void { - if (success) { - EXPECT_FALSE(split.starts_with("fail_")) - << "`" << filename << "` split `" << split - << "` succeeded; if success is expected, remove the `fail_` " - "prefix."; - } else { - EXPECT_TRUE(split.starts_with("fail_")) - << "`" << filename << "` split `" << split - << "` failed; if failure is expected, add the `fail_` prefix."; + auto test_filename = std::filesystem::path(test_name.str()).filename(); + const auto& run_result = test_file.run_result; + bool require_overall_failure = false; + for (const auto& [split, success] : run_result.per_file_success) { + check(split, llvm::formatv(" split `{0}`", split).str(), success); + if (!success) { + require_overall_failure = true; + } } + + if (require_overall_failure) { + if (run_result.success) { + problems.push_back(llvm::formatv( + "`{0}` succeeded, but there is a per-file failure expectation, so " + "the overall result should have been a failure.", + test_name)); + } + } else { + // Individual files all succeeded (or there are no splits), so the prefix is + // enforced on the main test file. + check(test_filename.string(), "", run_result.success); + } + return problems; } // Returns the requested bazel command string for the given execution mode. @@ -217,6 +241,21 @@ static auto RunAutoupdater(FileTestBase* test_base, const TestFile& test_file, .Run(dry_run); } +// Returns whether the actual output matches the expected output. +static auto OutputMatchesExpectations(const TestFile& test_file) -> bool { + auto matches = [&](llvm::StringRef actual, + llvm::ArrayRef> expected) { + if (test_file.check_subset) { + return testing::Value(SplitOutput(actual), + testing::IsSupersetOf(expected)); + } + return testing::Value(SplitOutput(actual), + testing::ElementsAreArray(expected)); + }; + return matches(test_file.actual_stdout, test_file.expected_stdout) && + matches(test_file.actual_stderr, test_file.expected_stderr); +} + auto FileTestCase::TestBody() -> void { if (absl::GetFlag(FLAGS_autoupdate) || absl::GetFlag(FLAGS_dump_output)) { return; @@ -228,31 +267,11 @@ auto FileTestCase::TestBody() -> void { ASSERT_TRUE(test_info_->test_result->ok()) << test_info_->test_result->error(); - auto test_filename = std::filesystem::path(test_info_->test_name).filename(); // Check success/failure against `fail_` prefixes. TestFile& test_file = **(test_info_->test_result); - if (test_file.run_result.per_file_success.empty()) { - CompareFailPrefix(test_filename.string(), test_file.run_result.success); - } else { - bool require_overall_failure = false; - for (const auto& [filename, success] : - test_file.run_result.per_file_success) { - CompareFailPrefix(test_filename.string(), filename, success); - if (!success) { - require_overall_failure = true; - } - } - - if (require_overall_failure) { - EXPECT_FALSE(test_file.run_result.success) - << "There is a per-file failure expectation, so the overall result " - "should have been a failure."; - } else { - // Individual files all succeeded, so the prefix is enforced on the main - // test file. - CompareFailPrefix(test_filename.string(), test_file.run_result.success); - } + for (const auto& problem : test_info_->fail_prefix_problems) { + ADD_FAILURE() << problem; } // Check results. Include a reminder for NOAUTOUPDATE tests. @@ -463,6 +482,11 @@ static auto RunSingleTest(FileTestInfo& test, bool single_threaded, return true; } + test.fail_prefix_problems = + GetFailPrefixProblems(test.test_name, **test.test_result); + test.noautoupdate_differs = !(*test.test_result)->autoupdate_line_number && + !OutputMatchesExpectations(**test.test_result); + Timer autoupdate_timer; test.autoupdate_differs = RunAutoupdater(test_instance.get(), **test.test_result, @@ -478,7 +502,13 @@ static auto RunSingleTest(FileTestInfo& test, bool single_threaded, << "\n--- Autoupdate differs: " << (test.autoupdate_differs ? "true" : "false") << "\n"; } else { - llvm::errs() << (test.autoupdate_differs ? "!" : "."); + // `?` indicates a problem that autoupdate can't fix, `!` indicates that + // autoupdate made (or would make) changes, and `.` indicates no changes. + llvm::errs() << (!test.fail_prefix_problems.empty() || + test.noautoupdate_differs + ? "?" + : test.autoupdate_differs ? "!" + : "."); } return true; @@ -547,6 +577,31 @@ auto FileTestEventListener::OnTestProgramStart( << all_elapsed_ms.count() << " ms wall time, " << total_elapsed_ms.count() << " ms across threads\n"; + // When autoupdating, list problems that autoupdate couldn't fix. When not + // autoupdating, these are reported as test failures instead. + if (absl::GetFlag(FLAGS_autoupdate)) { + bool printed_header = false; + auto print_problem = [&](llvm::StringRef problem) { + if (!printed_header) { + llvm::errs() << "\nProblems that require manual fixes:\n"; + printed_header = true; + } + llvm::errs() << " - " << problem << "\n"; + }; + for (const auto& test : tests_) { + for (const auto& problem : test.fail_prefix_problems) { + print_problem(problem); + } + if (test.noautoupdate_differs) { + print_problem( + llvm::formatv("`{0}` is NOAUTOUPDATE and its output doesn't match " + "expectations.", + test.test_name) + .str()); + } + } + } + // When there are multiple tests, give additional timing details, particularly // slowest tests. auto print_slowest_tests = absl::GetFlag(FLAGS_print_slowest_tests); diff --git a/testing/file_test/test_file.cpp b/testing/file_test/test_file.cpp index d57116528ac9..b05c88bce8d8 100644 --- a/testing/file_test/test_file.cpp +++ b/testing/file_test/test_file.cpp @@ -893,8 +893,10 @@ static auto ProcessFileContent(llvm::StringRef filename, return Success(); } -auto ProcessTestFile(llvm::StringRef test_name, bool running_autoupdate) - -> ErrorOr { +// Implementation of `ProcessTestFile`, without special handling for +// NOAUTOUPDATE files during autoupdate. +static auto ProcessTestFileImpl(llvm::StringRef test_name, + bool running_autoupdate) -> ErrorOr { TestFile test_file; // Store the original content, to avoid a read when autoupdating. @@ -976,4 +978,16 @@ auto ProcessTestFile(llvm::StringRef test_name, bool running_autoupdate) return std::move(test_file); } +auto ProcessTestFile(llvm::StringRef test_name, bool running_autoupdate) + -> ErrorOr { + auto result = ProcessTestFileImpl(test_name, running_autoupdate); + + // Autoupdate won't modify a NOAUTOUPDATE file, so process it as normal. This + // builds the expectations, which lets us report mismatches to the user. + if (running_autoupdate && result.ok() && !result->autoupdate_line_number) { + return ProcessTestFileImpl(test_name, /*running_autoupdate=*/false); + } + return result; +} + } // namespace Carbon::Testing diff --git a/toolchain/driver/config_subcommand.cpp b/toolchain/driver/config_subcommand.cpp index d4e4bcd50bcb..3e8fa2243ea8 100644 --- a/toolchain/driver/config_subcommand.cpp +++ b/toolchain/driver/config_subcommand.cpp @@ -14,7 +14,6 @@ #include "clang/Lex/HeaderSearch.h" #include "common/check.h" #include "common/command_line.h" -#include "common/filesystem.h" #include "common/version.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/STLExtras.h" @@ -193,22 +192,25 @@ auto ConfigSubcommand::Run(DriverEnv& driver_env) -> DriverResult { {.key = "VERSION", .value = Version::String.str()}, }; - // Try to read the installation digest and include that. - auto read_result = Filesystem::Cwd().ReadFileToString( - driver_env.installation->digest_path()); - if (!read_result.ok()) { + // Try to read the installation digest and include that. This goes through the + // driver's filesystem so that tests get consistent results regardless of + // whether the digest happens to exist on the real filesystem. + auto read_result = driver_env.fs->getBufferForFile( + driver_env.installation->digest_path().native()); + if (!read_result) { CARBON_DIAGNOSTIC(ConfigFailedToReadDigest, Error, - "unable to read the installation's digest file: {0}", - std::string); + "unable to read the installation's digest file: {0}: {1}", + std::string, std::string); driver_env.emitter.Emit(ConfigFailedToReadDigest, - read_result.error().ToString()); + driver_env.installation->digest_path().native(), + read_result.getError().message()); // Remember that we encountered an error but continue to give a minimally // useful `config` output. result = false; } else { data.push_back({.key = "INSTALL_DIGEST", - .value = llvm::StringRef(*read_result).rtrim().str()}); + .value = (*read_result)->getBuffer().rtrim().str()}); } // Compute and print Clang's config entries if we can. This will have been diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index f54f65e9ed63..9e30f71370f5 100644 --- a/toolchain/driver/driver_test.cpp +++ b/toolchain/driver/driver_test.cpp @@ -311,6 +311,8 @@ TEST_F(DriverTest, LinkWithFlagLikeFiles) { } TEST_F(DriverTest, ConfigJson) { + // Use the real filesystem so that the installation digest can be read. + auto cleanup = ScopedTempWorkingDir(); EXPECT_TRUE(driver_.RunCommand({"config", "--json"}).success); EXPECT_THAT(test_error_stream_.TakeStr(), StrEq("")); diff --git a/toolchain/driver/testdata/fail_config.carbon b/toolchain/driver/testdata/fail_config.carbon index ea20ee3408d8..97531ffee919 100644 --- a/toolchain/driver/testdata/fail_config.carbon +++ b/toolchain/driver/testdata/fail_config.carbon @@ -6,9 +6,9 @@ // // NOAUTOUPDATE // TIP: To test this file alone, run: -// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/config.carbon +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/fail_config.carbon // TIP: To dump output, run: -// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/driver/testdata/config.carbon +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/driver/testdata/fail_config.carbon // We don't include the digest in `file_test` to avoid unnecessary dependencies. // CHECK:STDERR: error: unable to read the installation's digest file: {{.+}}