From d0b2b7bc4141de845956d99d5517cddb2a0eb6b5 Mon Sep 17 00:00:00 2001
From: Jon Ross-Perkins <jperkins@google.com>
Date: Thu, 14 Sep 2023 12:48:29 -0700
Subject: [PATCH] When running FileTests, verify that autoupdate wouldn't make
 changes. (#3232)

Trying to more proactively catch when autoupdate is missed. Most of the
execution time of these tests should be in running the program under
test, not processing output, so this should have marginal overhead in
order to produce a useful reminder.
---
 testing/file_test/autoupdate.cpp     |  8 +++--
 testing/file_test/autoupdate.h       |  5 +--
 testing/file_test/file_test_base.cpp | 50 ++++++++++++++++++++--------
 testing/file_test/file_test_base.h   |  3 ++
 4 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/testing/file_test/autoupdate.cpp b/testing/file_test/autoupdate.cpp
index 96e3cccc03aa..a7a441da896a 100644
--- a/testing/file_test/autoupdate.cpp
+++ b/testing/file_test/autoupdate.cpp
@@ -258,7 +258,7 @@ auto FileTestAutoupdater::StartSplitFile() -> void {
   ++non_check_line_;
 }
 
-auto FileTestAutoupdater::Run() -> bool {
+auto FileTestAutoupdater::Run(bool dry_run) -> bool {
   bool any_attached_stdout_lines = std::any_of(
       stdout_.lines.begin(), stdout_.lines.end(),
       [&](const CheckLine& line) { return line.line_number() != -1; });
@@ -330,8 +330,10 @@ auto FileTestAutoupdater::Run() -> bool {
   if (new_content == input_content_) {
     return false;
   }
-  std::ofstream out(file_test_path_);
-  out << new_content;
+  if (!dry_run) {
+    std::ofstream out(file_test_path_);
+    out << new_content;
+  }
   return true;
 }
 
diff --git a/testing/file_test/autoupdate.h b/testing/file_test/autoupdate.h
index 2d9971cc0e4e..173c6b8e0c9a 100644
--- a/testing/file_test/autoupdate.h
+++ b/testing/file_test/autoupdate.h
@@ -65,8 +65,9 @@ class FileTestAutoupdater {
     }
   }
 
-  // Automatically updates CHECKs in the provided file. Returns true if updated.
-  auto Run() -> bool;
+  // Automatically updates CHECKs in the provided file when dry_run=false.
+  // Returns true if generated file content differs from actual file content.
+  auto Run(bool dry_run) -> bool;
 
  private:
   // The file and line number that a CHECK line refers to, and the
diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp
index e020132d61a4..c5eb7f3783ab 100644
--- a/testing/file_test/file_test_base.cpp
+++ b/testing/file_test/file_test_base.cpp
@@ -80,7 +80,19 @@ auto FileTestBase::TestBody() -> void {
       << "Tests should be prefixed with `fail_` if and only if running them "
          "is expected to fail.";
 
-  // Check results.
+  // Check results. Include a reminder of the autoupdate command for any
+  // stdout/stderr differences.
+  std::string update_message;
+  if (context.autoupdate_line_number) {
+    update_message = llvm::formatv(
+        "If these differences are expected, try the autoupdater:\n"
+        "\tbazel run {0} -- --autoupdate --file_tests={1}",
+        target, test_name_);
+  } else {
+    update_message =
+        "If these differences are expected, content must be updated manually.";
+  }
+  SCOPED_TRACE(update_message);
   if (context.check_subset) {
     EXPECT_THAT(SplitOutput(context.stdout),
                 IsSupersetOf(context.expected_stdout));
@@ -93,19 +105,17 @@ auto FileTestBase::TestBody() -> void {
     EXPECT_THAT(SplitOutput(context.stderr),
                 ElementsAreArray(context.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(context, /*dry_run=*/true)) {
+    ADD_FAILURE() << "Autoupdate would make changes to the file content.";
+  }
 }
 
-auto FileTestBase::Autoupdate() -> ErrorOr<bool> {
-  // Add a crash trace entry mentioning which file we're updating.
-  llvm::PrettyStackTraceFormat stack_trace_entry("performing autoupdate for %s",
-                                                 test_name_);
-
-  TestContext context;
-  auto run_result = ProcessTestFileAndRun(context);
-  if (!run_result.ok()) {
-    return ErrorBuilder() << "Error updating " << test_name_ << ": "
-                          << run_result.error();
-  }
+auto FileTestBase::RunAutoupdater(const TestContext& context, bool dry_run)
+    -> bool {
   if (!context.autoupdate_line_number) {
     return false;
   }
@@ -132,7 +142,21 @@ auto FileTestBase::Autoupdate() -> ErrorOr<bool> {
              GetDefaultFileRE(expected_filenames),
              GetLineNumberReplacements(expected_filenames),
              [&](std::string& line) { DoExtraCheckReplacements(line); })
-      .Run();
+      .Run(dry_run);
+}
+
+auto FileTestBase::Autoupdate() -> ErrorOr<bool> {
+  // Add a crash trace entry mentioning which file we're updating.
+  llvm::PrettyStackTraceFormat stack_trace_entry("performing autoupdate for %s",
+                                                 test_name_);
+
+  TestContext context;
+  auto run_result = ProcessTestFileAndRun(context);
+  if (!run_result.ok()) {
+    return ErrorBuilder() << "Error updating " << test_name_ << ": "
+                          << run_result.error();
+  }
+  return RunAutoupdater(context, /*dry_run=*/false);
 }
 
 auto FileTestBase::GetLineNumberReplacements(
diff --git a/testing/file_test/file_test_base.h b/testing/file_test/file_test_base.h
index 37fd9fb088ed..9691c2c1ccee 100644
--- a/testing/file_test/file_test_base.h
+++ b/testing/file_test/file_test_base.h
@@ -151,6 +151,9 @@ class FileTestBase : public testing::Test {
   static auto TransformExpectation(int line_index, llvm::StringRef in)
       -> ErrorOr<testing::Matcher<std::string>>;
 
+  // Runs the FileTestAutoupdater, returning the result.
+  auto RunAutoupdater(const TestContext& context, bool dry_run) -> bool;
+
   llvm::StringRef test_name_;
 };