From 66caff2c26b19ced1ff591b34a60ba7d75fe4aba Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 7 May 2025 15:28:13 -0700 Subject: [PATCH] Make the `file_test` binary work without custom environment variables. (#5442) Instead of crashing when run outside of `bazel`, make the toolchain's `file_test` binary work properly when no test-specific environment variables are set. This makes it a lot easier to run `file_test` under a debugger. There are two main changes here: - Don't crash if `$TEST_TMPDIR` is unset. Instead, fall back to LLVM's temporary directory (typically `$TMPDIR`). We already did this in some places in tests. We now do it in more places. - Don't fall back to a target label of `` in the reproduction commands if `$TEST_TARGET` is unset, because this causes all the tests to fail because their output doesn't match the expected output due to a differing bazel run command. Instead explicitly specify the target from the `FileTestBase`-derived class. Infrastructure for this has been added generally, but only rolled out to the toolchain `file_test` binary for now. --------- Co-authored-by: Jon Ross-Perkins --- testing/base/file_helpers.cpp | 16 ++++++------ testing/base/file_helpers.h | 4 +++ testing/file_test/BUILD | 1 + testing/file_test/autoupdate.cpp | 10 ++++---- testing/file_test/file_test_base.cpp | 37 +++++++++++++--------------- testing/file_test/file_test_base.h | 19 +++++++++++--- testing/file_test/run_test.cpp | 4 +-- toolchain/driver/driver_test.cpp | 4 +-- toolchain/testing/file_test.cpp | 6 +++++ 9 files changed, 61 insertions(+), 40 deletions(-) diff --git a/testing/base/file_helpers.cpp b/testing/base/file_helpers.cpp index 5246625b0d49..b15f6dfd04c6 100644 --- a/testing/base/file_helpers.cpp +++ b/testing/base/file_helpers.cpp @@ -14,6 +14,14 @@ namespace Carbon::Testing { +auto GetTempDirectory() -> std::filesystem::path { + if (char* tmpdir_env = getenv("TEST_TMPDIR"); tmpdir_env != nullptr) { + return tmpdir_env; + } else { + return std::filesystem::temp_directory_path(); + } +} + auto ReadFile(std::filesystem::path path) -> ErrorOr { std::ifstream file_stream(path); if (file_stream.fail()) { @@ -29,13 +37,7 @@ auto ReadFile(std::filesystem::path path) -> ErrorOr { auto WriteTestFile(llvm::StringRef name, llvm::StringRef contents) -> ErrorOr { - std::filesystem::path test_tmpdir; - if (char* tmpdir_env = getenv("TEST_TMPDIR"); tmpdir_env != nullptr) { - test_tmpdir = std::string(tmpdir_env); - } else { - test_tmpdir = std::filesystem::temp_directory_path(); - } - + std::filesystem::path test_tmpdir = GetTempDirectory(); const auto* unit_test = ::testing::UnitTest::GetInstance(); const auto* test_info = unit_test->current_test_info(); std::filesystem::path test_file = diff --git a/testing/base/file_helpers.h b/testing/base/file_helpers.h index 1f47082c17d6..0205d53b4c6c 100644 --- a/testing/base/file_helpers.h +++ b/testing/base/file_helpers.h @@ -12,6 +12,10 @@ namespace Carbon::Testing { +// Returns a directory that should be used to hold temporary files created by +// test execution. +auto GetTempDirectory() -> std::filesystem::path; + // Reads a file to string. auto ReadFile(std::filesystem::path path) -> ErrorOr; diff --git a/testing/file_test/BUILD b/testing/file_test/BUILD index 7d9c24896a96..2990b5ab7d91 100644 --- a/testing/file_test/BUILD +++ b/testing/file_test/BUILD @@ -19,6 +19,7 @@ cc_library( "//common:check", "//common:ostream", "//common:raw_string_ostream", + "//testing/base:file_helpers", "@abseil-cpp//absl/strings", "@abseil-cpp//absl/strings:string_view", "@llvm-project//llvm:Support", diff --git a/testing/file_test/autoupdate.cpp b/testing/file_test/autoupdate.cpp index da9028c846af..26472537ba36 100644 --- a/testing/file_test/autoupdate.cpp +++ b/testing/file_test/autoupdate.cpp @@ -16,6 +16,7 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/FormatVariadic.h" +#include "testing/base/file_helpers.h" namespace Carbon::Testing { @@ -149,10 +150,9 @@ auto FileTestAutoupdater::BuildCheckLines(llvm::StringRef output, file_to_number_map.insert({name, number}); } - // %t substitution means we may see TEST_TMPDIR in output. - char* tmpdir_env = getenv("TEST_TMPDIR"); - CARBON_CHECK(tmpdir_env != nullptr); - llvm::StringRef tmpdir = tmpdir_env; + // %t substitution means we may see the temporary directory's path in output. + std::filesystem::path tmpdir_path = GetTempDirectory(); + llvm::StringRef tmpdir = tmpdir_path.native(); llvm::SmallVector lines(llvm::split(output, '\n')); // It's typical that output ends with a newline, but we don't want to add a @@ -190,7 +190,7 @@ auto FileTestAutoupdater::BuildCheckLines(llvm::StringRef output, check_line.append("{{}}"); } - // Ignore TEST_TMPDIR in output. + // Ignore mentions of the temporary directory in output. if (auto pos = check_line.find(tmpdir); pos != std::string::npos) { check_line.replace(pos, tmpdir.size(), "{{.+}}"); } diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp index 8b0c2d2a6a02..521e75e51dba 100644 --- a/testing/file_test/file_test_base.cpp +++ b/testing/file_test/file_test_base.cpp @@ -78,7 +78,7 @@ struct FileTestInfo { std::string test_name; // A factory function for creating the test object. - std::functionFileTestBase*> factory_fn; + std::functionstd::unique_ptr> factory_fn; // gtest's information about the test. ::testing::TestInfo* registered_test; @@ -134,21 +134,11 @@ static auto CompareFailPrefix(llvm::StringRef filename, bool success) -> void { } } -// Modes for GetBazelCommand. -enum class BazelMode : uint8_t { - Autoupdate, - Dump, - Test, -}; - // Returns the requested bazel command string for the given execution mode. -static auto GetBazelCommand(BazelMode mode, llvm::StringRef test_name) - -> std::string { +auto FileTestBase::GetBazelCommand(BazelMode mode) -> std::string { RawStringOstream args; - - const char* target = getenv("TEST_TARGET"); args << "bazel " << ((mode == BazelMode::Test) ? "test" : "run") << " " - << (target ? target : "") << " "; + << GetBazelLabel() << " "; switch (mode) { case BazelMode::Autoupdate: @@ -165,10 +155,15 @@ static auto GetBazelCommand(BazelMode mode, llvm::StringRef test_name) } args << "--file_tests="; - args << test_name; + args << test_name(); return args.TakeStr(); } +auto FileTestBase::GetBazelLabel() -> std::string { + const char* target = getenv("TEST_TARGET"); + return target ? target : ""; +} + // Runs the FileTestAutoupdater, returning the result. static auto RunAutoupdater(FileTestBase* test_base, const TestFile& test_file, bool dry_run) -> bool { @@ -193,8 +188,8 @@ static auto RunAutoupdater(FileTestBase* test_base, const TestFile& test_file, return FileTestAutoupdater( std::filesystem::absolute(test_base->test_name().str()), - GetBazelCommand(BazelMode::Test, test_base->test_name()), - GetBazelCommand(BazelMode::Dump, test_base->test_name()), + test_base->GetBazelCommand(FileTestBase::BazelMode::Test), + test_base->GetBazelCommand(FileTestBase::BazelMode::Dump), test_file.input_content, filenames, *test_file.autoupdate_line_number, test_file.autoupdate_split, test_file.non_check_lines, test_file.actual_stdout, @@ -268,7 +263,8 @@ auto FileTestCase::TestBody() -> void { if (HasFailure()) { llvm::errs() << "\nTo test this file alone, run:\n " - << GetBazelCommand(BazelMode::Test, test_info_->test_name) + << test_info_->factory_fn()->GetBazelCommand( + FileTestBase::BazelMode::Test) << "\n\n"; if (!test_file.autoupdate_line_number) { llvm::errs() << "\nThis test is NOAUTOUPDATE.\n\n"; @@ -276,8 +272,8 @@ auto FileTestCase::TestBody() -> void { } if (test_info_->autoupdate_differs) { ADD_FAILURE() << "Autoupdate would make changes to the file content. Run:\n" - << GetBazelCommand(BazelMode::Autoupdate, - test_info_->test_name); + << test_info_->factory_fn()->GetBazelCommand( + FileTestBase::BazelMode::Autoupdate); } } @@ -372,7 +368,8 @@ static auto RunSingleTestHelper(FileTestInfo& test, FileTestBase& test_instance) -> void { Timer timer; // Add a crash trace entry with the single-file test command. - std::string test_command = GetBazelCommand(BazelMode::Test, test.test_name); + std::string test_command = + test_instance.GetBazelCommand(FileTestBase::BazelMode::Test); llvm::PrettyStackTraceString stack_trace_entry(test_command.c_str()); if (auto err = RunTestFile(test_instance, absl::GetFlag(FLAGS_dump_output), diff --git a/testing/file_test/file_test_base.h b/testing/file_test/file_test_base.h index 7782de5343a2..ffc36a4aed5a 100644 --- a/testing/file_test/file_test_base.h +++ b/testing/file_test/file_test_base.h @@ -108,6 +108,19 @@ class FileTestBase { virtual auto AllowParallelRun() const -> bool { return true; } // Returns the name of the test (relative to the repo root). + // Returns a bazel label that can be used to invoke this test. + virtual auto GetBazelLabel() -> std::string; + + // Modes for GetBazelCommand. + enum class BazelMode : uint8_t { + Autoupdate, + Dump, + Test, + }; + + // Returns the requested bazel command string for the given execution mode. + auto GetBazelCommand(BazelMode mode) -> std::string; + auto test_name() const -> llvm::StringRef { return test_name_; } private: @@ -120,8 +133,8 @@ struct FileTestFactory { const char* name; // A factory function for tests. - std::function< - auto(llvm::StringRef exe_path, llvm::StringRef test_name)->FileTestBase*> + std::functionstd::unique_ptr> factory_fn; }; @@ -139,7 +152,7 @@ extern auto GetFileTestFactory() -> FileTestFactory; #define CARBON_FILE_TEST_FACTORY(Name) \ auto GetFileTestFactory() -> FileTestFactory { \ return {#Name, [](llvm::StringRef exe_path, llvm::StringRef test_name) { \ - return new Name(exe_path, test_name); \ + return std::make_unique(exe_path, test_name); \ }}; \ } diff --git a/testing/file_test/run_test.cpp b/testing/file_test/run_test.cpp index 1426c23ceaa7..6015784ca711 100644 --- a/testing/file_test/run_test.cpp +++ b/testing/file_test/run_test.cpp @@ -14,6 +14,7 @@ #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/PrettyStackTrace.h" +#include "testing/base/file_helpers.h" #include "testing/file_test/file_test_base.h" #include "testing/file_test/test_file.h" @@ -62,8 +63,7 @@ static auto DoArgReplacements(llvm::SmallVector& test_args, break; } case 't': { - char* tmpdir = getenv("TEST_TMPDIR"); - CARBON_CHECK(tmpdir != nullptr); + std::filesystem::path tmpdir = GetTempDirectory(); it->replace(percent, 2, llvm::formatv("{0}/temp_file", tmpdir)); break; } diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index 683fd72f94c9..761e9cfea41c 100644 --- a/toolchain/driver/driver_test.cpp +++ b/toolchain/driver/driver_test.cpp @@ -38,9 +38,7 @@ class DriverTest : public testing::Test { InstallPaths::MakeForBazelRunfiles(Testing::GetExePath())), driver_(fs_, &installation_, /*input_stream=*/nullptr, &test_output_stream_, &test_error_stream_) { - char* tmpdir_env = getenv("TEST_TMPDIR"); - CARBON_CHECK(tmpdir_env != nullptr); - test_tmpdir_ = tmpdir_env; + test_tmpdir_ = Testing::GetTempDirectory(); } auto MakeTestFile(llvm::StringRef text, diff --git a/toolchain/testing/file_test.cpp b/toolchain/testing/file_test.cpp index f257d1abefa2..2fa78cc5ff20 100644 --- a/toolchain/testing/file_test.cpp +++ b/toolchain/testing/file_test.cpp @@ -62,6 +62,12 @@ class ToolchainFileTest : public FileTestBase { return component_ != "language_server"; } + // Force a fixed bazel label to avoid spurious test failures due to differing + // run commands when running file_test binary outside of bazel. + auto GetBazelLabel() -> std::string override { + return "//toolchain/testing:file_test"; + } + private: // Controls whether `Run()` includes the prelude. auto is_no_prelude() const -> bool {