Make FileTest run tests async by default (#4991)

This takes the mechanism currently used for autoupdate and expands it to
the regular tests (deliberately trying to unify logic for
test/autoupdate/dump to deliver consistent behavior). I'm seeing about a
85% reduction in test time, though results will vary based on test
system.

This does some small edits to test output to make it fit better with the
new flow. Note I'm stopping printing of the "here's how to run" on every
test by default, since it's autoupdated into file content by default.
However, it's still there for test failures.

---------

Co-authored-by: Geoff Romer <gromer@google.com>

explorer/file_test.cpp

@@ -19,8 +19,8 @@ namespace {
 class ExplorerFileTest : public FileTestBase {
  public:
   explicit ExplorerFileTest(llvm::StringRef /*exe_path*/,
-                            std::mutex* output_mutex, llvm::StringRef test_name)
-      : FileTestBase(output_mutex, test_name),
+                            llvm::StringRef test_name)
+      : FileTestBase(test_name),
         prelude_line_re_(R"(prelude.carbon:(\d+))"),
         timing_re_(R"((Time elapsed in \w+: )\d+(ms))") {
     CARBON_CHECK(prelude_line_re_.ok(), "{0}", prelude_line_re_.error());
@@ -96,6 +96,9 @@ class ExplorerFileTest : public FileTestBase {
     }
   }
 
+  // Cannot execute in parallel.
+  auto AllowParallelRun() const -> bool override { return false; }
+
  private:
   // Trace output is directly checked for a few tests.
   auto check_trace_output() -> bool {

testing/file_test/BUILD

@@ -48,6 +48,7 @@ cc_library(
         "//testing/base:file_helpers",
         "@abseil-cpp//absl/flags:flag",
         "@abseil-cpp//absl/flags:parse",
+        "@abseil-cpp//absl/strings",
         "@googletest//:gtest",
         "@llvm-project//llvm:Support",
     ],

testing/file_test/autoupdate_testdata.sh

@@ -5,6 +5,6 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 "$(dirname "$0")/../../scripts/run_bazel.py" \
-  run -c opt --experimental_convenience_symlinks=ignore \
+  run --experimental_convenience_symlinks=ignore \
   --ui_event_filters=-info,-stdout,-stderr,-finish \
   //testing/file_test:file_test_base_test -- --autoupdate "$@"

testing/file_test/file_test_base.cpp

@@ -2,6 +2,23 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+// Implementation-wise, this:
+//
+// - Uses the registered `FileTestFactory` to construct `FileTestBase`
+//   instances.
+// - Constructs a `FileTestCase` that wraps each `FileTestBase` instance to
+//   register with googletest, and to provide the actual `TestBody`.
+// - Using `FileTestEventListener`, runs tests in parallel prior to normal
+//   googletest execution.
+//   - This is required to support `--gtest_filter` and access `should_run`.
+//   - Runs each `FileTestBase` instance to cache the `TestFile` on
+//     `FileTestInfo`.
+//   - Determines whether autoupdate would make changes, autoupdating if
+//     requested.
+// - When googletest would normally execute the test, `FileTestCase::TestBody`
+//   instead uses the cached state on `FileTestInfo`.
+//   - This only occurs when neither autoupdating nor dumping output.
+
 #include "testing/file_test/file_test_base.h"
 
 #include <filesystem>
@@ -11,6 +28,8 @@
 
 #include "absl/flags/flag.h"
 #include "absl/flags/parse.h"
+#include "absl/strings/str_join.h"
+#include "absl/strings/str_split.h"
 #include "common/check.h"
 #include "common/error.h"
 #include "common/exe_path.h"
@@ -30,7 +49,8 @@
 
 ABSL_FLAG(std::vector<std::string>, file_tests, {},
           "A comma-separated list of repo-relative names of test files. "
-          "Overrides test_targets_file.");
+          "Similar to and overrides `--gtest_filter`, but doesn't require the "
+          "test class name to be known.");
 ABSL_FLAG(std::string, test_targets_file, "",
           "A path to a file containing repo-relative names of test files.");
 ABSL_FLAG(bool, autoupdate, false,
@@ -45,6 +65,39 @@ ABSL_FLAG(bool, dump_output, false,
 
 namespace Carbon::Testing {
 
+// Information for a test case.
+struct FileTestInfo {
+  // The name.
+  std::string test_name;
+
+  // A factory function for creating the test object.
+  std::function<auto()->FileTestBase*> factory_fn;
+
+  // gtest's information about the test.
+  ::testing::TestInfo* registered_test;
+
+  // The test result, set after running.
+  std::optional<ErrorOr<TestFile>> test_result;
+
+  // Whether running autoupdate would change (or when autoupdating, already
+  // changed) the test file. This may be true even if output passes test
+  // expectations.
+  bool autoupdate_differs = false;
+};
+
+// Adapts a `FileTestBase` instance to gtest for outputting results.
+class FileTestCase : public testing::Test {
+ public:
+  explicit FileTestCase(FileTestInfo* test_info) : test_info_(test_info) {}
+
+  // Runs a test and compares output. This keeps output split by line so that
+  // issues are a little easier to identify by the different line.
+  auto TestBody() -> void final;
+
+ private:
+  FileTestInfo* test_info_;
+};
+
 // Splits outputs to string_view because gtest handles string_view by default.
 static auto SplitOutput(llvm::StringRef output)
     -> llvm::SmallVector<std::string_view> {
@@ -144,28 +197,27 @@ static auto RunAutoupdater(FileTestBase* test_base, const TestFile& test_file,
       .Run(dry_run);
 }
 
-// Runs a test and compares output. This keeps output split by line so that
-// issues are a little easier to identify by the different line.
-auto FileTestBase::TestBody() -> void {
-  // Add a crash trace entry with the single-file test command.
-  std::string test_command = GetBazelCommand(BazelMode::Test, test_name_);
-  llvm::PrettyStackTraceString stack_trace_entry(test_command.c_str());
-  llvm::errs() << "\nTo test this file alone, run:\n  " << test_command
-               << "\n\n";
-
-  ErrorOr<TestFile> test_file =
-      ProcessTestFileAndRun(this, output_mutex_, /*dump_output=*/false,
-                            absl::GetFlag(FLAGS_autoupdate));
-  ASSERT_TRUE(test_file.ok()) << test_file.error();
-  auto test_filename = std::filesystem::path(test_name_.str()).filename();
+auto FileTestCase::TestBody() -> void {
+  if (absl::GetFlag(FLAGS_autoupdate) || absl::GetFlag(FLAGS_dump_output)) {
+    return;
+  }
+
+  CARBON_CHECK(test_info_->test_result,
+               "Expected test to be run prior to TestBody: {0}",
+               test_info_->test_name);
+
+  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.
-  if (test_file->run_result.per_file_success.empty()) {
-    CompareFailPrefix(test_filename.string(), test_file->run_result.success);
+  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) {
+         test_file.run_result.per_file_success) {
       CompareFailPrefix(filename, success);
       if (!success) {
         require_overall_failure = true;
@@ -173,76 +225,50 @@ auto FileTestBase::TestBody() -> void {
     }
 
     if (require_overall_failure) {
-      EXPECT_FALSE(test_file->run_result.success)
+      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);
+      CompareFailPrefix(test_filename.string(), test_file.run_result.success);
     }
   }
 
-  // Check results. Include a reminder of the autoupdate command for any
-  // stdout/stderr differences.
-  std::string update_message;
-  if (test_file->autoupdate_line_number) {
-    update_message = llvm::formatv(
-        "If these differences are expected, try the autoupdater:\n  {0}",
-        GetBazelCommand(BazelMode::Autoupdate, test_name_));
-  } else {
-    update_message =
-        "If these differences are expected, content must be updated manually.";
+  // Check results. Include a reminder for NOAUTOUPDATE tests.
+  std::unique_ptr<testing::ScopedTrace> scoped_trace;
+  if (!test_file.autoupdate_line_number) {
+    scoped_trace = std::make_unique<testing::ScopedTrace>(
+        __FILE__, __LINE__,
+        "This file is NOAUTOUPDATE, so expected differences require manual "
+        "updates.");
   }
-  SCOPED_TRACE(update_message);
-  if (test_file->check_subset) {
-    EXPECT_THAT(SplitOutput(test_file->actual_stdout),
-                IsSupersetOf(test_file->expected_stdout));
-    EXPECT_THAT(SplitOutput(test_file->actual_stderr),
-                IsSupersetOf(test_file->expected_stderr));
+  if (test_file.check_subset) {
+    EXPECT_THAT(SplitOutput(test_file.actual_stdout),
+                IsSupersetOf(test_file.expected_stdout));
+    EXPECT_THAT(SplitOutput(test_file.actual_stderr),
+                IsSupersetOf(test_file.expected_stderr));
 
   } else {
-    EXPECT_THAT(SplitOutput(test_file->actual_stdout),
-                ElementsAreArray(test_file->expected_stdout));
-    EXPECT_THAT(SplitOutput(test_file->actual_stderr),
-                ElementsAreArray(test_file->expected_stderr));
+    EXPECT_THAT(SplitOutput(test_file.actual_stdout),
+                ElementsAreArray(test_file.expected_stdout));
+    EXPECT_THAT(SplitOutput(test_file.actual_stderr),
+                ElementsAreArray(test_file.expected_stderr));
   }
 
-  // If there are no other test failures, check if autoupdate would make
-  // changes. We don't do this when there _are_ failures because the
-  // SCOPED_TRACE already contains the autoupdate reminder.
-  if (!HasFailure() && RunAutoupdater(this, *test_file, /*dry_run=*/true)) {
-    ADD_FAILURE() << "Autoupdate would make changes to the file content.";
+  if (HasFailure()) {
+    llvm::errs() << "\nTo test this file alone, run:\n  "
+                 << GetBazelCommand(BazelMode::Test, test_info_->test_name)
+                 << "\n\n";
+    if (test_file.autoupdate_line_number) {
+      llvm::errs() << "\nThis test is NOAUTOUPDATE.\n\n";
+    }
+  }
+  if (test_info_->autoupdate_differs) {
+    ADD_FAILURE() << "Autoupdate would make changes to the file content. Run:\n"
+                  << GetBazelCommand(BazelMode::Autoupdate,
+                                     test_info_->test_name);
   }
-}
-
-auto FileTestBase::Autoupdate() -> ErrorOr<bool> {
-  // Add a crash trace entry mentioning which file we're updating.
-  std::string stack_trace_string =
-      llvm::formatv("performing autoupdate for {0}", test_name_);
-  llvm::PrettyStackTraceString stack_trace_entry(stack_trace_string.c_str());
-
-  CARBON_ASSIGN_OR_RETURN(
-      TestFile test_file,
-      ProcessTestFileAndRun(this, output_mutex_, /*dump_output=*/false,
-                            absl::GetFlag(FLAGS_autoupdate)));
-  return RunAutoupdater(this, test_file, /*dry_run=*/false);
-}
-
-auto FileTestBase::DumpOutput() -> ErrorOr<Success> {
-  std::string banner(79, '=');
-  banner.append("\n");
-  llvm::errs() << banner << "= " << test_name_ << "\n";
-
-  CARBON_ASSIGN_OR_RETURN(
-      TestFile test_file,
-      ProcessTestFileAndRun(this, output_mutex_, /*dump_output=*/true,
-                            absl::GetFlag(FLAGS_autoupdate)));
-  llvm::errs() << banner << test_file.actual_stdout << banner
-               << "= Exit with success: "
-               << (test_file.run_result.success ? "true" : "false") << "\n"
-               << banner;
-  return Success();
 }
 
 auto FileTestBase::GetLineNumberReplacements(
@@ -254,48 +280,91 @@ auto FileTestBase::GetLineNumberReplacements(
            .line_formatv = R"({0})"}};
 }
 
-// Returns the tests to run.
-static auto GetTests() -> llvm::SmallVector<std::string> {
-  // Prefer a user-specified list if present.
-  auto specific_tests = absl::GetFlag(FLAGS_file_tests);
-  if (!specific_tests.empty()) {
-    return llvm::SmallVector<std::string>(specific_tests.begin(),
-                                          specific_tests.end());
+// If `--file_tests` is set, transform it into a `--gtest_filter`.
+static auto MaybeApplyFileTestsFlag(llvm::StringRef factory_name) -> void {
+  if (absl::GetFlag(FLAGS_file_tests).empty()) {
+    return;
+  }
+  RawStringOstream filter;
+  llvm::ListSeparator sep(":");
+  for (const auto& file : absl::GetFlag(FLAGS_file_tests)) {
+    filter << sep << factory_name << "." << file;
   }
+  absl::SetFlag(&FLAGS_gtest_filter, filter.TakeStr());
+}
 
-  // Extracts tests from the target file.
-  CARBON_CHECK(!absl::GetFlag(FLAGS_test_targets_file).empty(),
-               "Missing --test_targets_file.");
-  auto content = ReadFile(absl::GetFlag(FLAGS_test_targets_file));
-  CARBON_CHECK(content.ok(), "{0}", content.error());
-  llvm::SmallVector<std::string> all_tests;
-  for (llvm::StringRef file_ref : llvm::split(*content, "\n")) {
-    if (file_ref.empty()) {
-      continue;
-    }
-    all_tests.push_back(file_ref.str());
+// Loads tests from the manifest file, and registers them for execution. The
+// vector is taken as an output parameter so that the address of entries is
+// stable for the factory.
+static auto RegisterTests(FileTestFactory* test_factory,
+                          llvm::StringRef exe_path,
+                          llvm::SmallVectorImpl<FileTestInfo>& tests)
+    -> ErrorOr<Success> {
+  if (absl::GetFlag(FLAGS_test_targets_file).empty()) {
+    return Error("Missing --test_targets_file.");
+  }
+  CARBON_ASSIGN_OR_RETURN(auto test_manifest,
+                          ReadFile(absl::GetFlag(FLAGS_test_targets_file)));
+
+  // Prepare the vector first, so that the location of entries won't change.
+  for (const auto& test_name :
+       absl::StrSplit(test_manifest, "\n", absl::SkipEmpty())) {
+    tests.push_back({.test_name = std::string(test_name)});
+  }
+
+  // Amend entries with factory functions.
+  for (auto& test : tests) {
+    llvm::StringRef test_name = test.test_name;
+    test.factory_fn = [test_factory, exe_path, test_name]() {
+      return test_factory->factory_fn(exe_path, test_name);
+    };
+    test.registered_test = testing::RegisterTest(
+        test_factory->name, test_name.data(), nullptr, test_name.data(),
+        __FILE__, __LINE__, [&test]() { return new FileTestCase(&test); });
   }
-  return all_tests;
+  return Success();
 }
 
-// Runs autoupdate for the given tests. This is multi-threaded to try to get a
-// little extra speed.
-static auto RunAutoupdate(llvm::StringRef exe_path,
-                          llvm::ArrayRef<std::string> tests,
-                          FileTestFactory& test_factory) -> int {
+// Implements the parallel test execution through gtest's listener support.
+class FileTestEventListener : public testing::EmptyTestEventListener {
+ public:
+  explicit FileTestEventListener(llvm::MutableArrayRef<FileTestInfo> tests)
+      : tests_(tests) {}
+
+  // Runs test during start, after `should_run` is initialized. This is
+  // multi-threaded to get extra speed.
+  auto OnTestProgramStart(const testing::UnitTest& /*unit_test*/)
+      -> void override;
+
+ private:
+  llvm::MutableArrayRef<FileTestInfo> tests_;
+};
+
+auto FileTestEventListener::OnTestProgramStart(
+    const testing::UnitTest& /*unit_test*/) -> void {
   llvm::CrashRecoveryContext::Enable();
   llvm::DefaultThreadPool pool(
-      {.ThreadsRequested = absl::GetFlag(FLAGS_threads)});
+      {.ThreadsRequested = absl::GetFlag(FLAGS_dump_output)
+                               ? 1
+                               : absl::GetFlag(FLAGS_threads)});
+  if (!absl::GetFlag(FLAGS_dump_output)) {
+    llvm::errs() << "Running tests with " << pool.getMaxConcurrency()
+                 << " thread(s)\n";
+  }
 
   // Guard access to both `llvm::errs` and `crashed`.
-  std::mutex mutex;
   bool crashed = false;
+  std::mutex output_mutex;
+
+  for (auto& test : tests_) {
+    if (!test.registered_test->should_run()) {
+      continue;
+    }
 
-  for (const auto& test_name : tests) {
-    pool.async([&test_factory, &mutex, &exe_path, &crashed, test_name] {
+    pool.async([&output_mutex, &crashed, &test] {
       // If any thread crashed, don't try running more.
       {
-        std::unique_lock<std::mutex> lock(mutex);
+        std::unique_lock<std::mutex> lock(output_mutex);
         if (crashed) {
           return;
         }
@@ -307,19 +376,46 @@ static auto RunAutoupdate(llvm::StringRef exe_path,
       llvm::CrashRecoveryContext crc;
       crc.DumpStackAndCleanupOnFailure = true;
       bool thread_crashed = !crc.RunSafely([&] {
-        std::unique_ptr<FileTestBase> test(
-            test_factory.factory_fn(exe_path, &mutex, test_name));
-        auto result = test->Autoupdate();
+        std::unique_ptr<FileTestBase> test_instance(test.factory_fn());
+
+        // Add a crash trace entry with the single-file test command.
+        std::string test_command =
+            GetBazelCommand(BazelMode::Test, test.test_name);
+        llvm::PrettyStackTraceString stack_trace_entry(test_command.c_str());
+
+        if (absl::GetFlag(FLAGS_dump_output)) {
+          std::unique_lock<std::mutex> lock(output_mutex);
+          llvm::errs() << "\n--- Dumping: " << test.test_name << "\n\n";
+        }
+
+        test.test_result = ProcessTestFileAndRun(
+            test_instance.get(), &output_mutex,
+            absl::GetFlag(FLAGS_dump_output), absl::GetFlag(FLAGS_autoupdate));
+
+        if (!test.test_result->ok()) {
+          std::unique_lock<std::mutex> lock(output_mutex);
+          llvm::errs() << "\n" << test.test_result->error().message() << "\n";
+          return;
+        }
 
-        std::unique_lock<std::mutex> lock(mutex);
-        if (result.ok()) {
-          llvm::errs() << (*result ? "!" : ".");
+        test.autoupdate_differs =
+            RunAutoupdater(test_instance.get(), **test.test_result,
+                           /*dry_run=*/!absl::GetFlag(FLAGS_autoupdate));
+
+        std::unique_lock<std::mutex> lock(output_mutex);
+        if (absl::GetFlag(FLAGS_dump_output)) {
+          llvm::outs().flush();
+          const TestFile& test_file = **test.test_result;
+          llvm::errs() << "\n--- Exit with success: "
+                       << (test_file.run_result.success ? "true" : "false")
+                       << "\n--- Autoupdate differs: "
+                       << (test.autoupdate_differs ? "true" : "false") << "\n";
         } else {
-          llvm::errs() << "\n" << result.error().message() << "\n";
+          llvm::errs() << (test.autoupdate_differs ? "!" : ".");
         }
       });
       if (thread_crashed) {
-        std::unique_lock<std::mutex> lock(mutex);
+        std::unique_lock<std::mutex> lock(output_mutex);
         crashed = true;
       }
     });
@@ -332,23 +428,21 @@ static auto RunAutoupdate(llvm::StringRef exe_path,
     std::abort();
   }
   llvm::errs() << "\nDone!\n";
-  return EXIT_SUCCESS;
 }
 
 // Implements main() within the Carbon::Testing namespace for convenience.
-static auto Main(int argc, char** argv) -> int {
+static auto Main(int argc, char** argv) -> ErrorOr<int> {
   Carbon::InitLLVM init_llvm(argc, argv);
   testing::InitGoogleTest(&argc, argv);
   auto args = absl::ParseCommandLine(argc, argv);
 
   if (args.size() > 1) {
-    llvm::errs() << "Unexpected arguments:";
+    ErrorBuilder b;
+    b << "Unexpected arguments:";
     for (char* arg : llvm::ArrayRef(args).drop_front()) {
-      llvm::errs() << " ";
-      llvm::errs().write_escaped(arg);
+      b << " " << FormatEscaped(arg);
     }
-    llvm::errs() << "\n";
-    return EXIT_FAILURE;
+    return b;
   }
 
   std::string exe_path = FindExecutablePath(argv[0]);
@@ -358,51 +452,44 @@ static auto Main(int argc, char** argv) -> int {
   // on Windows, but POSIX requires it to be 0.
   if (std::error_code error =
           llvm::sys::Process::SafelyCloseFileDescriptor(0)) {
-    llvm::errs() << "Unable to close standard input: " << error.message()
-                 << "\n";
-    return EXIT_FAILURE;
+    return Error("Unable to close standard input: " + error.message());
   }
   if (std::error_code error =
           llvm::sys::Process::FixupStandardFileDescriptors()) {
-    llvm::errs() << "Unable to correct standard file descriptors: "
-                 << error.message() << "\n";
-    return EXIT_FAILURE;
+    return Error("Unable to correct standard file descriptors: " +
+                 error.message());
   }
   if (absl::GetFlag(FLAGS_autoupdate) && absl::GetFlag(FLAGS_dump_output)) {
-    llvm::errs() << "--autoupdate and --dump_output are mutually exclusive.\n";
-    return EXIT_FAILURE;
+    return Error("--autoupdate and --dump_output are mutually exclusive.");
   }
 
-  llvm::SmallVector<std::string> tests = GetTests();
   auto test_factory = GetFileTestFactory();
-  if (absl::GetFlag(FLAGS_autoupdate)) {
-    return RunAutoupdate(exe_path, tests, test_factory);
-  } else if (absl::GetFlag(FLAGS_dump_output)) {
-    for (const auto& test_name : tests) {
-      std::unique_ptr<FileTestBase> test(
-          test_factory.factory_fn(exe_path, nullptr, test_name));
-      auto result = test->DumpOutput();
-      if (!result.ok()) {
-        llvm::errs() << "\n" << result.error().message() << "\n";
-      }
-    }
-    llvm::errs() << "\nDone!\n";
-    return EXIT_SUCCESS;
-  } else {
-    for (const std::string& test_name : tests) {
-      testing::RegisterTest(
-          test_factory.name, test_name.c_str(), nullptr, test_name.c_str(),
-          __FILE__, __LINE__,
-          [&test_factory, &exe_path, test_name = test_name]() {
-            return test_factory.factory_fn(exe_path, nullptr, test_name);
-          });
-    }
-    return RUN_ALL_TESTS();
+
+  MaybeApplyFileTestsFlag(test_factory.name);
+
+  // Inline 0 entries because it will always be too large to store on the stack.
+  llvm::SmallVector<FileTestInfo, 0> tests;
+  CARBON_RETURN_IF_ERROR(RegisterTests(&test_factory, exe_path, tests));
+
+  testing::TestEventListeners& listeners =
+      testing::UnitTest::GetInstance()->listeners();
+  if (absl::GetFlag(FLAGS_autoupdate) || absl::GetFlag(FLAGS_dump_output)) {
+    // Suppress all of the default output.
+    delete listeners.Release(listeners.default_result_printer());
   }
+  // Use a listener to run tests in parallel.
+  listeners.Append(new FileTestEventListener(tests));
+
+  return RUN_ALL_TESTS();
 }
 
 }  // namespace Carbon::Testing
 
 auto main(int argc, char** argv) -> int {
-  return Carbon::Testing::Main(argc, argv);
+  if (auto result = Carbon::Testing::Main(argc, argv); result.ok()) {
+    return *result;
+  } else {
+    llvm::errs() << result.error() << "\n";
+    return EXIT_FAILURE;
+  }
 }

testing/file_test/file_test_base.h

@@ -23,7 +23,7 @@
 namespace Carbon::Testing {
 
 // A framework for testing files. See README.md for documentation.
-class FileTestBase : public testing::Test {
+class FileTestBase {
  public:
   // Provided for child class convenience.
   using LineNumberReplacement = FileTestAutoupdater::LineNumberReplacement;
@@ -52,8 +52,8 @@ class FileTestBase : public testing::Test {
     llvm::SmallVector<std::pair<std::string, bool>> per_file_success;
   };
 
-  explicit FileTestBase(std::mutex* output_mutex, llvm::StringRef test_name)
-      : output_mutex_(output_mutex), test_name_(test_name) {}
+  explicit FileTestBase(llvm::StringRef test_name) : test_name_(test_name) {}
+  virtual ~FileTestBase() = default;
 
   // Implemented by children to run the test. The framework will validate the
   // content written to `output_stream` and `error_stream`. Children should use
@@ -105,25 +105,10 @@ class FileTestBase : public testing::Test {
   // run.
   virtual auto AllowParallelRun() const -> bool { return true; }
 
-  // Runs a test and compares output. This keeps output split by line so that
-  // issues are a little easier to identify by the different line.
-  auto TestBody() -> void final;
-
-  // Runs the test and autoupdates checks. Returns true if updated.
-  auto Autoupdate() -> ErrorOr<bool>;
-
-  // Runs the test and dumps output.
-  auto DumpOutput() -> ErrorOr<Success>;
-
   // Returns the name of the test (relative to the repo root).
   auto test_name() const -> llvm::StringRef { return test_name_; }
 
  private:
-  // An optional mutex for output. If provided, it will be locked during `Run`
-  // when stderr/stdout are being captured (SET-CAPTURE-CONSOLE-OUTPUT), in
-  // order to avoid output conflicts.
-  std::mutex* output_mutex_;
-
   llvm::StringRef test_name_;
 };
 
@@ -132,11 +117,9 @@ struct FileTestFactory {
   // The test fixture name.
   const char* name;
 
-  // A factory function for tests. The output_mutex is optional; see
-  // `FileTestBase::output_mutex_`.
-  std::function<auto(llvm::StringRef exe_path, std::mutex* output_mutex,
-                     llvm::StringRef test_name)
-                    ->FileTestBase*>
+  // A factory function for tests.
+  std::function<
+      auto(llvm::StringRef exe_path, llvm::StringRef test_name)->FileTestBase*>
       factory_fn;
 };
 
@@ -151,12 +134,11 @@ struct FileTestFactory {
 extern auto GetFileTestFactory() -> FileTestFactory;
 
 // Provides a standard GetFileTestFactory implementation.
-#define CARBON_FILE_TEST_FACTORY(Name)                                    \
-  auto GetFileTestFactory() -> FileTestFactory {                          \
-    return {#Name, [](llvm::StringRef exe_path, std::mutex* output_mutex, \
-                      llvm::StringRef test_name) {                        \
-              return new Name(exe_path, output_mutex, test_name);         \
-            }};                                                           \
+#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);                          \
+            }};                                                              \
   }
 
 }  // namespace Carbon::Testing

testing/file_test/file_test_base_test.cpp

@@ -17,9 +17,8 @@ namespace {
 
 class FileTestBaseTest : public FileTestBase {
  public:
-  FileTestBaseTest(llvm::StringRef /*exe_path*/, std::mutex* output_mutex,
-                   llvm::StringRef test_name)
-      : FileTestBase(output_mutex, test_name) {}
+  FileTestBaseTest(llvm::StringRef /*exe_path*/, llvm::StringRef test_name)
+      : FileTestBase(test_name) {}
 
   auto Run(const llvm::SmallVector<llvm::StringRef>& test_args,
            llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>& fs,

testing/file_test/rules.bzl

@@ -9,6 +9,7 @@ a file which can be accessed as a list. This avoids long argument parsing.
 """
 
 load("@rules_cc//cc:defs.bzl", "cc_test")
+load("//bazel/cc_toolchains:defs.bzl", "cc_env")
 load("//bazel/manifest:defs.bzl", "manifest")
 
 def file_test(
@@ -50,6 +51,7 @@ def file_test(
             srcs = [prebuilt_binary],
             data = data,
             args = args,
+            env = cc_env(),
             **kwargs
         )
     else:
@@ -57,5 +59,6 @@ def file_test(
             name = name,
             data = data,
             args = args,
+            env = cc_env(),
             **kwargs
         )

toolchain/testing/BUILD

@@ -3,7 +3,6 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test")
-load("//bazel/cc_toolchains:defs.bzl", "cc_env")
 load("//testing/file_test:rules.bzl", "file_test")
 
 package(default_visibility = ["//visibility:public"])
@@ -55,7 +54,6 @@ file_test(
     size = "small",
     timeout = "moderate",  # Taking >60 seconds in GitHub actions
     srcs = ["file_test.cpp"],
-    env = cc_env(),
     tests = [":all_testdata"],
     deps = [
         "//common:all_llvm_targets",

toolchain/testing/file_test.cpp

@@ -23,7 +23,7 @@ namespace {
 // component subdirectories.
 class ToolchainFileTest : public FileTestBase {
  public:
-  explicit ToolchainFileTest(llvm::StringRef exe_path, std::mutex* output_mutex,
+  explicit ToolchainFileTest(llvm::StringRef exe_path,
                              llvm::StringRef test_name);
 
   // Adds a replacement for `core_package_dir`.
@@ -89,9 +89,8 @@ static auto GetComponent(llvm::StringRef test_name) -> llvm::StringRef {
 }
 
 ToolchainFileTest::ToolchainFileTest(llvm::StringRef exe_path,
-                                     std::mutex* output_mutex,
                                      llvm::StringRef test_name)
-    : FileTestBase(output_mutex, test_name),
+    : FileTestBase(test_name),
       component_(GetComponent(test_name)),
       installation_(InstallPaths::MakeForBazelRunfiles(exe_path)) {}