Introduce flag handling into file_test. (#3030)

I'm integrating absl flag support with a few thoughts here...

1. It simplifies the handling of files in file_test.
    - Removes the need for a separate subset target.
3. Looking forward, I'm planning to add a flag to allow for autoupdate
of golden files.
4. In explorer, there's been confusion about having file_test run tests
"twice" so hopefully it's clearer when it's a separate target with a
different flag on the target.

explorer/BUILD

@@ -40,23 +40,51 @@ cc_binary(
     ],
 )
 
-file_test(
-    name = "file_test",
+cc_library(
+    name = "file_test_common",
+    testonly = 1,
     srcs = ["file_test.cpp"],
-    # Bazel limits sharding to 50. We have lots of tests, so this should
-    # maximize parallelism.
-    shard_count = 50,
-    tests = glob([
-        "testdata/**/*.carbon",
-        "trace_testdata/**/*.carbon",
-    ]),
     deps = [
         "//explorer/parse_and_execute",
         "//testing/file_test:file_test_base",
         "//testing/util:test_raw_ostream",
+        "@com_google_absl//absl/flags:flag",
     ],
 )
 
+file_test(
+    name = "file_test",
+    size = "small",
+    shard_count = 20,
+    tests = glob([
+        "testdata/**/*.carbon",
+        "trace_testdata/**/*.carbon",
+    ]),
+    deps = [":file_test_common"],
+)
+
+file_test(
+    name = "file_test.trace",
+    size = "small",
+    args = ["--trace"],
+    shard_count = 30,
+    tests = glob(
+        # This omits trace_testdata because that's run with trace in the
+        # default mode.
+        ["testdata/**/*.carbon"],
+        exclude = [
+            # `limits` tests check for various limit conditions (such as an
+            # infinite loop). The tests collectively don't test tracing
+            # because it creates substantial additional overhead.
+            "testdata/limits/**",
+            # Expensive tests to trace.
+            "testdata/assoc_const/rewrite_large_type.carbon",
+            "testdata/linked_list/typed_linked_list.carbon",
+        ],
+    ),
+    deps = [":file_test_common"],
+)
+
 glob_sh_run(
     args = ["$(location //explorer)"],
     data = ["//explorer"],

explorer/file_test.cpp

@@ -2,45 +2,23 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+#include "absl/flags/flag.h"
 #include "explorer/parse_and_execute/parse_and_execute.h"
 #include "testing/file_test/file_test_base.h"
 #include "testing/util/test_raw_ostream.h"
 
+ABSL_FLAG(bool, trace, false,
+          "Set to true to run tests with tracing enabled, even if they don't "
+          "otherwise specify it. This does not result in checking trace output "
+          "contents; it essentially only verifies there's not a crash bug.");
+
 namespace Carbon::Testing {
 namespace {
 
 class ParseAndExecuteTestFile : public FileTestBase {
  public:
-  explicit ParseAndExecuteTestFile(const std::filesystem::path& path,
-                                   bool trace)
-      : FileTestBase(path), trace_(trace) {}
-
-  auto SetUp() -> void override {
-    std::string path_str = path().string();
-    llvm::StringRef path_ref = path_str;
-
-    if (path_ref.find("trace_testdata") != llvm::StringRef::npos) {
-      is_trace_test = true;
-    }
-
-    if (trace_) {
-      if (path_ref.find("/limits/") != llvm::StringRef::npos) {
-        GTEST_SKIP()
-            << "`limits` tests check for various limit conditions (such as an "
-               "infinite loop). The tests collectively don't test tracing "
-               "because it creates substantial additional overhead.";
-      } else if (path_ref.endswith(
-                     "testdata/assoc_const/rewrite_large_type.carbon") ||
-                 path_ref.endswith(
-                     "testdata/linked_list/typed_linked_list.carbon")) {
-        GTEST_SKIP() << "Expensive test to trace";
-      }
-    } else {
-      if (is_trace_test) {
-        GTEST_SKIP() << "`trace` tests only check for trace output.";
-      }
-    }
-  }
+  explicit ParseAndExecuteTestFile(const std::filesystem::path& path)
+      : FileTestBase(path) {}
 
   auto RunWithFiles(const llvm::SmallVector<llvm::StringRef>& test_args,
                     const llvm::SmallVector<TestFile>& test_files,
@@ -55,11 +33,16 @@ class ParseAndExecuteTestFile : public FileTestBase {
       return false;
     }
 
+    // Trace output is only checked for a few tests.
+    bool check_trace_output =
+        path().string().find("/trace_testdata/") != std::string::npos;
+
     // Capture trace streaming, but only when in debug mode.
     TraceStream trace_stream;
     TestRawOstream trace_stream_ostream;
-    if (trace_) {
-      trace_stream.set_stream(is_trace_test ? &stdout : &trace_stream_ostream);
+    if (check_trace_output || absl::GetFlag(FLAGS_trace)) {
+      trace_stream.set_stream(check_trace_output ? &stdout
+                                                 : &trace_stream_ostream);
       trace_stream.set_allowed_phases({ProgramPhase::All});
       trace_stream.set_allowed_file_kinds({FileKind::Main});
     }
@@ -84,7 +67,9 @@ class ParseAndExecuteTestFile : public FileTestBase {
 
     // Skip trace test check as they use stdout stream instead of
     // trace_stream_ostream
-    if (trace_ && !is_trace_test) {
+    if (absl::GetFlag(FLAGS_trace)) {
+      CARBON_CHECK(!check_trace_output)
+          << "trace tests should only be run in the default mode.";
       EXPECT_FALSE(trace_stream_ostream.TakeStr().empty())
           << "Tracing should always do something";
     }
@@ -95,10 +80,6 @@ class ParseAndExecuteTestFile : public FileTestBase {
   auto GetDefaultArgs() -> llvm::SmallVector<std::string> override {
     return {};
   }
-
- private:
-  bool trace_;
-  bool is_trace_test = false;
 };
 
 }  // namespace
@@ -107,13 +88,8 @@ extern auto RegisterFileTests(
     const llvm::SmallVector<std::filesystem::path>& paths) -> void {
   ParseAndExecuteTestFile::RegisterTests(
       "ParseAndExecuteTestFile", paths, [](const std::filesystem::path& path) {
-        return new ParseAndExecuteTestFile(path, /*trace=*/false);
+        return new ParseAndExecuteTestFile(path);
       });
-  ParseAndExecuteTestFile::RegisterTests("ParseAndExecuteTestFile.trace", paths,
-                                         [](const std::filesystem::path& path) {
-                                           return new ParseAndExecuteTestFile(
-                                               path, /*trace=*/true);
-                                         });
 }
 
 }  // namespace Carbon::Testing

scripts/fix_cc_deps.py

@@ -54,6 +54,10 @@ EXTERNAL_REPOS: Dict[str, ExternalRepo] = {
     ),
     # tools/cpp/runfiles:runfiles.h -> tools/cpp/runfiles/runfiles.h
     "@bazel_tools": ExternalRepo(lambda x: re.sub(":", "/", x), "...", None),
+    # absl/flags:flag.h -> absl/flags/flag.h
+    "@com_google_absl": ExternalRepo(
+        lambda x: re.sub(":", "/", x), "...", None
+    ),
 }
 
 # TODO: proto rules are aspect-based and their generated files don't show up in

testing/file_test/BUILD

@@ -14,6 +14,8 @@ cc_library(
     deps = [
         "//common:check",
         "//testing/util:test_raw_ostream",
+        "@com_google_absl//absl/flags:flag",
+        "@com_google_absl//absl/flags:parse",
         "@com_google_googletest//:gtest",
         "@llvm-project//llvm:Support",
     ],

testing/file_test/file_test_base.cpp

@@ -7,6 +7,8 @@
 #include <filesystem>
 #include <fstream>
 
+#include "absl/flags/flag.h"
+#include "absl/flags/parse.h"
 #include "common/check.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Twine.h"
@@ -14,12 +16,13 @@
 #include "llvm/Support/InitLLVM.h"
 #include "testing/util/test_raw_ostream.h"
 
+ABSL_FLAG(std::vector<std::string>, file_tests, {},
+          "A comma-separated list of tests for file_test infrastructure.");
+
 namespace Carbon::Testing {
 
 // The length of the base directory.
 static int base_dir_len = 0;
-// The name of the `.subset` target.
-static std::string* subset_target = nullptr;
 
 using ::testing::Eq;
 using ::testing::Matcher;
@@ -68,8 +71,11 @@ auto FileTestBase::TestBody() -> void {
   CARBON_CHECK(src_dir);
   std::string test_file = path().lexically_relative(
       std::filesystem::path(src_dir).append("carbon"));
-  llvm::errs() << "\nTo test this file alone, run:\n  bazel test "
-               << *subset_target << " --test_arg=" << test_file << "\n\n";
+  const char* target = getenv("TEST_TARGET");
+  CARBON_CHECK(target);
+  // This advice overrides the --file_tests flag provided by the file_test rule.
+  llvm::errs() << "\nTo test this file alone, run:\n  bazel test " << target
+               << " --test_arg=--file_tests=" << test_file << "\n\n";
 
   // Store the file so that test_files can use references to content.
   std::string test_content = ReadFile(path());
@@ -343,6 +349,7 @@ auto FileTestBase::TransformExpectation(int line_index, llvm::StringRef in)
 }  // namespace Carbon::Testing
 
 auto main(int argc, char** argv) -> int {
+  absl::ParseCommandLine(argc, argv);
   testing::InitGoogleTest(&argc, argv);
   llvm::setBugReportMsg(
       "Please report issues to "
@@ -350,23 +357,14 @@ auto main(int argc, char** argv) -> int {
       "crash backtrace.\n");
   llvm::InitLLVM init_llvm(argc, argv);
 
-  if (argc < 2) {
-    llvm::errs() << "At least one test file must be provided.\n";
+  if (argc > 1) {
+    llvm::errs() << "Unexpected arguments starting at: " << argv[1] << "\n";
     return EXIT_FAILURE;
   }
 
+  // Configure the base directory for test names.
   const char* target = getenv("TEST_TARGET");
   CARBON_CHECK(target != nullptr);
-
-  // Configure the name of the subset target.
-  std::string subset_target_storage = target;
-  static constexpr char SubsetSuffix[] = ".subset";
-  if (!llvm::StringRef(subset_target_storage).ends_with(SubsetSuffix)) {
-    subset_target_storage += SubsetSuffix;
-  }
-  Carbon::Testing::subset_target = &subset_target_storage;
-
-  // Configure the base directory for test names.
   llvm::StringRef target_dir = target;
   std::error_code ec;
   std::filesystem::path working_dir = std::filesystem::current_path(ec);
@@ -379,9 +377,9 @@ auto main(int argc, char** argv) -> int {
 
   // Register tests based on their absolute path.
   llvm::SmallVector<std::filesystem::path> paths;
-  for (int i = 1; i < argc; ++i) {
-    auto path = std::filesystem::absolute(argv[i], ec);
-    CARBON_CHECK(!ec) << argv[i] << ": " << ec.message();
+  for (const auto& file_test : absl::GetFlag(FLAGS_file_tests)) {
+    auto path = std::filesystem::absolute(file_test, ec);
+    CARBON_CHECK(!ec) << file_test << ": " << ec.message();
     CARBON_CHECK(llvm::StringRef(path.string()).starts_with(base_dir))
         << "\n  " << path << "\n  should start with\n  " << base_dir;
     paths.push_back(path);

testing/file_test/rules.bzl

@@ -4,9 +4,7 @@
 
 """Rules for building fuzz tests."""
 
-load("@bazel_skylib//rules:native_binary.bzl", "native_test")
-
-def file_test(name, srcs, deps, tests, shard_count = 1):
+def file_test(name, tests, data = [], args = [], **kwargs):
     """Generates tests using the file_test base.
 
     There will be one main test using `name` that can be sharded, and includes
@@ -15,26 +13,17 @@ def file_test(name, srcs, deps, tests, shard_count = 1):
 
     Args:
       name: The base name of the tests.
-      srcs: cc_test srcs.
-      deps: cc_test deps.
-      tests: The list of test files to use as data.
-      shard_count: The number of shards to use; defaults to 1.
+      tests: The list of test files to use as data, typically a glob.
+      data: Passed to cc_test.
+      args: Passed to cc_test.
+      **kwargs: Passed to cc_test.
     """
-    subset_name = "{0}.subset".format(name)
-
     native.cc_test(
         name = name,
-        srcs = srcs,
-        deps = deps,
-        data = tests,
-        args = ["$(location {0})".format(x) for x in tests],
-        shard_count = shard_count,
-    )
-
-    native_test(
-        name = subset_name,
-        src = name,
-        out = subset_name,
-        data = tests,
-        tags = ["manual"],
+        data = tests + data,
+        args = ["--file_tests=" + ",".join([
+            "$(location {0})".format(x)
+            for x in tests
+        ])] + args,
+        **kwargs
     )