From 3f24aa28b73b9b7047f2ff821643fa5c4b16156a Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 27 Jul 2023 16:36:21 -0700 Subject: [PATCH] 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 | 46 +++++++++++++++++---- explorer/file_test.cpp | 62 +++++++++------------------- scripts/fix_cc_deps.py | 4 ++ testing/file_test/BUILD | 2 + testing/file_test/file_test_base.cpp | 36 ++++++++-------- testing/file_test/rules.bzl | 33 +++++---------- 6 files changed, 90 insertions(+), 93 deletions(-) diff --git a/explorer/BUILD b/explorer/BUILD index ba3ae2f68b4c..dd205e89d9ee 100644 --- a/explorer/BUILD +++ b/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"], diff --git a/explorer/file_test.cpp b/explorer/file_test.cpp index 90eea291fe4e..69baa3141fab 100644 --- a/explorer/file_test.cpp +++ b/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& test_args, const llvm::SmallVector& 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 override { return {}; } - - private: - bool trace_; - bool is_trace_test = false; }; } // namespace @@ -107,13 +88,8 @@ extern auto RegisterFileTests( const llvm::SmallVector& 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 diff --git a/scripts/fix_cc_deps.py b/scripts/fix_cc_deps.py index 4467e3957798..63bb1c3ffa44 100755 --- a/scripts/fix_cc_deps.py +++ b/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 diff --git a/testing/file_test/BUILD b/testing/file_test/BUILD index 2ac22ff1601e..ad6d7f17c795 100644 --- a/testing/file_test/BUILD +++ b/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", ], diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp index 2a835f4777df..999cefe3082c 100644 --- a/testing/file_test/file_test_base.cpp +++ b/testing/file_test/file_test_base.cpp @@ -7,6 +7,8 @@ #include #include +#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, 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 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); diff --git a/testing/file_test/rules.bzl b/testing/file_test/rules.bzl index 8cfa914a1a37..030ffe65f26e 100644 --- a/testing/file_test/rules.bzl +++ b/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 )