Refactor FileTest construction so that the test class is directly ava…

…ilable. (carbon-language#3035) This is a simplification of the construction, although somewhat limiting (it means that the caller can't register the same file multiple times, though I stopped doing that anyways since it was causing confusion). What this more importantly _allows_ is logic on the FileTestBase child itself that's not test-specific -- in particular, autoupdate functionality which wouldn't use RUN_ALL_TESTS. --------- Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
zygoloid · Aug 1, 2023 · eb05f61 · eb05f61
1 parent ce56226
commit eb05f61
Show file tree

Hide file tree

Showing 10 changed files with 46 additions and 99 deletions.
diff --git a/explorer/file_test.cpp b/explorer/file_test.cpp
@@ -3,7 +3,6 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 #include "absl/flags/flag.h"
-#include "common/check.h"
 #include "explorer/main.h"
 #include "testing/file_test/file_test_base.h"
 #include "testing/util/test_raw_ostream.h"
@@ -16,10 +15,9 @@ ABSL_FLAG(bool, trace, false,
 namespace Carbon::Testing {
 namespace {
 
-class ParseAndExecuteTestFile : public FileTestBase {
+class ExplorerFileTest : public FileTestBase {
  public:
-  explicit ParseAndExecuteTestFile(const std::filesystem::path& path)
-      : FileTestBase(path) {}
+  using FileTestBase::FileTestBase;
 
   auto RunWithFiles(const llvm::SmallVector<llvm::StringRef>& test_args,
                     const llvm::SmallVector<TestFile>& test_files,
@@ -89,12 +87,6 @@ class ParseAndExecuteTestFile : public FileTestBase {
 
 }  // namespace
 
-extern auto RegisterFileTests(
-    const llvm::SmallVector<std::filesystem::path>& paths) -> void {
-  ParseAndExecuteTestFile::RegisterTests(
-      "ParseAndExecuteTestFile", paths, [](const std::filesystem::path& path) {
-        return new ParseAndExecuteTestFile(path);
-      });
-}
+CARBON_FILE_TEST_FACTORY(ExplorerFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp
@@ -21,29 +21,11 @@ ABSL_FLAG(std::vector<std::string>, file_tests, {},
 
 namespace Carbon::Testing {
 
-// The length of the base directory.
-static int base_dir_len = 0;
-
 using ::testing::Eq;
 using ::testing::Matcher;
 using ::testing::MatchesRegex;
 using ::testing::StrEq;
 
-void FileTestBase::RegisterTests(
-    const char* fixture_label,
-    const llvm::SmallVector<std::filesystem::path>& paths,
-    std::function<FileTestBase*(const std::filesystem::path&)> factory) {
-  // Use RegisterTest instead of INSTANTIATE_TEST_CASE_P because of ordering
-  // issues between container initialization and test instantiation by
-  // InitGoogleTest.
-  for (const auto& path : paths) {
-    std::string test_name = path.string().substr(base_dir_len);
-    testing::RegisterTest(fixture_label, test_name.c_str(), nullptr,
-                          test_name.c_str(), __FILE__, __LINE__,
-                          [=]() { return factory(path); });
-  }
-}
-
 // Reads a file to string.
 static auto ReadFile(std::filesystem::path path) -> std::string {
   std::ifstream proto_file(path);
@@ -373,18 +355,20 @@ auto main(int argc, char** argv) -> int {
   CARBON_CHECK(target_dir.consume_front("/"));
   target_dir = target_dir.substr(0, target_dir.rfind(":"));
   std::string base_dir = working_dir.string() + target_dir.str() + "/";
-  Carbon::Testing::base_dir_len = base_dir.size();
+
+  auto test_factory = Carbon::Testing::GetFileTestFactory();
 
   // Register tests based on their absolute path.
-  llvm::SmallVector<std::filesystem::path> paths;
   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);
+    std::string test_name = path.string().substr(base_dir.size());
+    testing::RegisterTest(test_factory.name, test_name.c_str(), nullptr,
+                          test_name.c_str(), __FILE__, __LINE__,
+                          [=]() { return test_factory.factory_fn(path); });
   }
-  Carbon::Testing::RegisterFileTests(paths);
 
   return RUN_ALL_TESTS();
 }
diff --git a/testing/file_test/file_test_base.h b/testing/file_test/file_test_base.h
@@ -19,8 +19,8 @@
 
 namespace Carbon::Testing {
 
-// A framework for testing files. Children implement `RegisterTestFiles` with
-// calls to `RegisterTests` using a factory that constructs the child.
+// A framework for testing files. Children write
+// `CARBON_FILE_TEST_FACTORY(MyTest)` which is used to construct the tests.
 // `RunWithFiles` must also be implemented and will be called as part of
 // individual test executions. This framework includes a `main` implementation,
 // so users must not provide one.
@@ -94,23 +94,7 @@ class FileTestBase : public testing::Test {
     llvm::StringRef content;
   };
 
-  explicit FileTestBase(const std::filesystem::path& path) : path_(&path) {}
-
-  // Used by children to register tests with gtest.
-  static auto RegisterTests(
-      const char* fixture_label,
-      const llvm::SmallVector<std::filesystem::path>& paths,
-      std::function<FileTestBase*(const std::filesystem::path&)> factory)
-      -> void;
-
-  template <typename FileTestChildT>
-  static auto RegisterTests(
-      const char* fixture_label,
-      const llvm::SmallVector<std::filesystem::path>& paths) -> void {
-    RegisterTests(fixture_label, paths, [](const std::filesystem::path& path) {
-      return new FileTestChildT(path);
-    });
-  }
+  explicit FileTestBase(std::filesystem::path path) : path_(std::move(path)) {}
 
   // Implemented by children to run the test. Called by the TestBody
   // implementation, which will validate stdout and stderr. The return value
@@ -128,7 +112,7 @@ class FileTestBase : public testing::Test {
   auto TestBody() -> void final;
 
   // Returns the full path of the file being tested.
-  auto path() -> const std::filesystem::path& { return *path_; };
+  auto path() -> const std::filesystem::path& { return path_; };
 
  private:
   // Does replacements in ARGS for %s and %t.
@@ -148,12 +132,34 @@ class FileTestBase : public testing::Test {
   static auto TransformExpectation(int line_index, llvm::StringRef in)
       -> testing::Matcher<std::string>;
 
-  const std::filesystem::path* path_;
+  const std::filesystem::path path_;
+};
+
+// Aggregate a name and factory function for tests using this framework.
+struct FileTestFactory {
+  // The test fixture name.
+  const char* name;
+
+  // A factory function for tests.
+  std::function<FileTestBase*(const std::filesystem::path& path)> factory_fn;
 };
 
 // Must be implemented by the individual file_test to initialize tests.
-extern auto RegisterFileTests(
-    const llvm::SmallVector<std::filesystem::path>& paths) -> void;
+//
+// We can't use INSTANTIATE_TEST_CASE_P because of ordering issues between
+// container initialization and test instantiation by InitGoogleTest, but this
+// also allows us more flexibility in execution.
+//
+// The `CARBON_FILE_TEST_FACTOR` macro below provides a standard, convenient way
+// to implement this function.
+extern auto GetFileTestFactory() -> FileTestFactory;
+
+// Provides a standard GetFileTestFactory implementation.
+#define CARBON_FILE_TEST_FACTORY(Name)                                         \
+  auto GetFileTestFactory()->FileTestFactory {                                 \
+    return {(#Name),                                                           \
+            [](const std::filesystem::path& path) { return new Name(path); }}; \
+  }
 
 }  // namespace Carbon::Testing
 

diff --git a/testing/file_test/file_test_base_test.cpp b/testing/file_test/file_test_base_test.cpp
@@ -24,8 +24,7 @@ using ::testing::Matcher;
 
 class FileTestBaseTest : public FileTestBase {
  public:
-  explicit FileTestBaseTest(const std::filesystem::path& path)
-      : FileTestBase(path) {}
+  using FileTestBase::FileTestBase;
 
   static auto HasFilename(std::string filename) -> Matcher<TestFile> {
     return Field("filename", &TestFile::filename, Eq(filename));
@@ -92,9 +91,6 @@ class FileTestBaseTest : public FileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  FileTestBaseTest::RegisterTests<FileTestBaseTest>("FileTestBaseTest", paths);
-}
+CARBON_FILE_TEST_FACTORY(FileTestBaseTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/codegen/codegen_file_test.cpp b/toolchain/codegen/codegen_file_test.cpp
@@ -2,11 +2,9 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/driver_file_test_base.h"
 
 namespace Carbon::Testing {
@@ -23,9 +21,6 @@ class CodeGenFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  CodeGenFileTest::RegisterTests<CodeGenFileTest>("CodeGenFileTest", paths);
-}
+CARBON_FILE_TEST_FACTORY(CodeGenFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/driver/driver_file_test.cpp b/toolchain/driver/driver_file_test.cpp
@@ -2,7 +2,6 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
@@ -22,9 +21,6 @@ class DriverFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  DriverFileTest::RegisterTests<DriverFileTest>("DriverFileTest", paths);
-}
+CARBON_FILE_TEST_FACTORY(DriverFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/lexer/lexer_file_test.cpp b/toolchain/lexer/lexer_file_test.cpp
@@ -2,11 +2,9 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/driver_file_test_base.h"
 
 namespace Carbon::Testing {
@@ -23,9 +21,6 @@ class LexerFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  LexerFileTest::RegisterTests<LexerFileTest>("LexerFileTest", paths);
-}
+CARBON_FILE_TEST_FACTORY(LexerFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/lowering/lowering_file_test.cpp b/toolchain/lowering/lowering_file_test.cpp
@@ -2,11 +2,9 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/driver_file_test_base.h"
 
 namespace Carbon::Testing {
@@ -23,9 +21,6 @@ class LoweringFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  LoweringFileTest::RegisterTests<LoweringFileTest>("LoweringFileTest", paths);
-}
+CARBON_FILE_TEST_FACTORY(LoweringFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/parser/parse_tree_file_test.cpp b/toolchain/parser/parse_tree_file_test.cpp
@@ -2,11 +2,9 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/driver_file_test_base.h"
 
 namespace Carbon::Testing {
@@ -23,10 +21,6 @@ class ParseTreeFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  ParseTreeFileTest::RegisterTests<ParseTreeFileTest>("ParseTreeFileTest",
-                                                      paths);
-}
+CARBON_FILE_TEST_FACTORY(ParseTreeFileTest);
 
 }  // namespace Carbon::Testing
diff --git a/toolchain/semantics/semantics_file_test.cpp b/toolchain/semantics/semantics_file_test.cpp
@@ -2,11 +2,9 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include <filesystem>
 #include <string>
 
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/driver_file_test_base.h"
 
 namespace Carbon::Testing {
@@ -23,10 +21,6 @@ class SemanticsFileTest : public DriverFileTestBase {
 
 }  // namespace
 
-auto RegisterFileTests(const llvm::SmallVector<std::filesystem::path>& paths)
-    -> void {
-  SemanticsFileTest::RegisterTests<SemanticsFileTest>("SemanticsFileTest",
-                                                      paths);
-}
+CARBON_FILE_TEST_FACTORY(SemanticsFileTest);
 
 }  // namespace Carbon::Testing