From fbe917b9492a2b6e478bcbabd677b0122dad94ac Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 13 Mar 2026 14:39:21 -0700 Subject: [PATCH] Create a UnifiedDiffMatcher to make golden test failures easier to understand (#6897) Right now I think everyone has the habit of doing an autoupdate then using source control for a diff. This is offering an option of better diff output from the test. For example: ``` TEST: toolchain/driver/testdata/fail_flush_errors.carbon ! Ran 1 tests in 81 ms wall time, 8 ms across threads testing/file_test/file_test_base.cpp:264: Failure Value of: SplitOutput(test_file.actual_stderr) Expected: matches elements with union diff Actual: { "fail_flush_errors.carbon:22:3: error: name `undeclared1` not found [NameNotFound]", " undeclared1;", " ^~~~~~~~~~~", "", "fail_flush_errors.carbon:31:3: error: `Core.String` implicitly referenced here, but package `Core` not found [CoreNotFound]", " \"undec\\x6Cared2\";", " ^~~~~~~~~~~~~~~~", "", "fail_flush_errors.carbon:35:3: error: name `undeclared2` not found [NameNotFound]", " undeclared2;", " ^~~~~~~~~~~", "", "fail_flush_errors.carbon:43:3: error: name `undeclared3` not found [NameNotFound]", " undeclared3;", " ^~~~~~~~~~~", "", "" }, union diff (- expected, + actual): === diff in expected elements 0 to 2: + fail_flush_errors.carbon:22:3: error: name `undeclared1` not found [NameNotFound] undeclared1; ^~~~~~~~~~~ === diff in expected elements 4 to 9: "undec\x6Cared2"; ^~~~~~~~~~~~~~~~ + fail_flush_errors.carbon:35:3: error: name `undeclared2` not found [NameNotFound] undeclared2; ^~~~~~~~~~~ === diff end Stack trace: 0x55e476d29efd: Carbon::Testing::FileTestCase::TestBody() 0x55e476dbd1f2: testing::internal::HandleExceptionsInMethodIfSupported<>() 0x55e476dbcf57: testing::Test::Run() 0x55e476dbf0bf: testing::TestInfo::Run() ... Google Test internal frames ... To test this file alone, run: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/fail_flush_errors.carbon testing/file_test/file_test_base.cpp:277: Failure Failed Autoupdate would make changes to the file content. Run: bazel run //toolchain/testing:file_test -- --autoupdate --file_tests=toolchain/driver/testdata/fail_flush_errors.carbon Stack trace: 0x55e476d2a5f0: Carbon::Testing::FileTestCase::TestBody() 0x55e476dbd1f2: testing::internal::HandleExceptionsInMethodIfSupported<>() 0x55e476dbcf57: testing::Test::Run() 0x55e476dbf0bf: testing::TestInfo::Run() ... Google Test internal frames ... [ FAILED ] ToolchainFileTest.toolchain/driver/testdata/fail_flush_errors.carbon, where GetParam() = toolchain/driver/testdata/fail_flush_errors.carbon (93 ms) ``` Assisted-by: Google Antigravity with Gemini --- testing/base/BUILD | 23 ++ testing/base/unified_diff_matcher.h | 300 +++++++++++++++++++++ testing/base/unified_diff_matcher_test.cpp | 247 +++++++++++++++++ testing/file_test/BUILD | 1 + testing/file_test/file_test_base.cpp | 10 +- 5 files changed, 574 insertions(+), 7 deletions(-) create mode 100644 testing/base/unified_diff_matcher.h create mode 100644 testing/base/unified_diff_matcher_test.cpp diff --git a/testing/base/BUILD b/testing/base/BUILD index 7f907df3a050..85a9164e2c06 100644 --- a/testing/base/BUILD +++ b/testing/base/BUILD @@ -137,3 +137,26 @@ cc_test( "@llvm-project//llvm:Support", ], ) + +cc_library( + name = "unified_diff_matcher", + testonly = 1, + hdrs = ["unified_diff_matcher.h"], + deps = [ + "//common:check", + "@googletest//:gtest", + "@llvm-project//llvm:Support", + ], +) + +cc_test( + name = "unified_diff_matcher_test", + size = "small", + srcs = ["unified_diff_matcher_test.cpp"], + deps = [ + ":gtest_main", + ":unified_diff_matcher", + "@googletest//:gtest", + "@llvm-project//llvm:Support", + ], +) diff --git a/testing/base/unified_diff_matcher.h b/testing/base/unified_diff_matcher.h new file mode 100644 index 000000000000..6d399435b19e --- /dev/null +++ b/testing/base/unified_diff_matcher.h @@ -0,0 +1,300 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_TESTING_BASE_UNIFIED_DIFF_MATCHER_H_ +#define CARBON_TESTING_BASE_UNIFIED_DIFF_MATCHER_H_ + +#include + +#include +#include +#include +#include + +#include "common/check.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/Sequence.h" +#include "llvm/ADT/SmallVector.h" + +namespace Carbon::Testing { + +// Matcher that compares the elements of two containers and produces a unified +// diff on failure. +template +class UnifiedDiffMatcher { + public: + explicit UnifiedDiffMatcher(Container expected) + : expected_(std::move(expected)) {} + + // Matches `actual` against `expected_`. Returns true on a match; returns + // false and prints a unified diff to `listener` on a mismatch. + template + auto MatchAndExplain(const ActualContainer& actual, + testing::MatchResultListener* listener) const -> bool; + + auto DescribeTo(std::ostream* os) const -> void { + *os << "matches elements with unified diff"; + } + + auto DescribeNegationTo(std::ostream* os) const -> void { + *os << "does not match elements with unified diff"; + } + + private: + // A 2D array, stored contiguously. Rows correspond to `expected_`'s elements, + // and columns correspond to the actual container's elements. + template + class Table; + + // The result of a `Matches` check between an expected and actual element. + enum class MatchResult : uint8_t { Unknown, Matches, DoesNotMatch }; + + // Checks whether `actual_element` matches `expected_[expected_index]`. It + // first checks whether a cached result exists. If not, it evaluates the + // match and stores the result in `match_results`. + template + auto IsElementMatch(size_t expected_index, size_t actual_index, + const ActualElement& actual_element, + Table& match_results) const -> bool { + MatchResult cached_result = match_results.Get(expected_index, actual_index); + if (cached_result != MatchResult::Unknown) { + return cached_result == MatchResult::Matches; + } + bool is_match = + testing::MatcherCast(expected_[expected_index]) + .Matches(actual_element); + match_results.Set( + expected_index, actual_index, + is_match ? MatchResult::Matches : MatchResult::DoesNotMatch); + return is_match; + } + + // Returns true if every element in `expected_` matches the corresponding + // element in `actual`. Stores comparisons in `match_results`. + template + auto IsEqual(const ActualContainer& actual, + Table& match_results) const -> bool; + + // Populates `subsequences` with the longest common matching subsequences + // found when comparing `actual` and `expected_`. Stores comparisons in + // `match_results`. + template + auto GetLongestCommonSubsequences(const ActualContainer& actual, + Table& match_results, + Table& subsequences) const -> void; + + // Prints the unified diff. + template + auto PrintDiff(const ActualContainer& actual, + Table& match_results, + const Table& subsequences, + testing::MatchResultListener* listener) const -> void; + + // The expected elements. + Container expected_; +}; + +// Returns a polymorphic matcher that acts similarly to +// ElementsAreArray but produces a unified diff on failure. +template +auto ElementsAreArrayWithUnifiedDiff(Container expected) { + return testing::MakePolymorphicMatcher( + UnifiedDiffMatcher(std::move(expected))); +} + +// ----------------------------------------------------------------------------- +// Internal implementation details follow. +// ----------------------------------------------------------------------------- + +template +template +class UnifiedDiffMatcher::Table { + public: + // Constructs a table with dimensions of expected_size and actual_size, + // corresponding to the containers being compared. + Table(int expected_size, int actual_size, T default_value) + : actual_size_(actual_size), + data_(expected_size * actual_size, default_value) {} + + // Sets the value at the given expected_index and actual_index. + auto Set(int expected_index, int actual_index, T value) -> void { + data_[expected_index * actual_size_ + actual_index] = std::move(value); + } + + // Gets the value at the given expected_index and actual_index. + auto Get(int expected_index, int actual_index) const -> T { + return data_[expected_index * actual_size_ + actual_index]; + } + + private: + // The actual_size of the table. + int actual_size_; + // The contiguous data storage for the table. + llvm::SmallVector data_; +}; + +template +template +auto UnifiedDiffMatcher::MatchAndExplain( + const ActualContainer& actual, testing::MatchResultListener* listener) const + -> bool { + Table match_results(expected_.size(), std::size(actual), + MatchResult::Unknown); + + if (IsEqual(actual, match_results)) { + return true; + } + + if (listener->IsInterested()) { + Table subsequences(expected_.size() + 1, std::size(actual) + 1, 0); + GetLongestCommonSubsequences(actual, match_results, subsequences); + PrintDiff(actual, match_results, subsequences, listener); + } + return false; +} + +template +template +auto UnifiedDiffMatcher::IsEqual( + const ActualContainer& actual, Table& match_results) const + -> bool { + if (expected_.size() != std::size(actual)) { + return false; + } + + for (auto [i, actual_element] : llvm::enumerate(actual)) { + if (!IsElementMatch(i, i, actual_element, match_results)) { + return false; + } + } + return true; +} + +template +template +auto UnifiedDiffMatcher::GetLongestCommonSubsequences( + const ActualContainer& actual, Table& match_results, + Table& subsequences) const -> void { + for (auto expected_index : llvm::seq(expected_.size())) { + for (auto [actual_index, actual_element] : llvm::enumerate(actual)) { + int subsequence_value; + if (IsElementMatch(expected_index, actual_index, actual_element, + match_results)) { + // If the elements match, the LCS length increases by 1 relative to + // the prefixes where both elements are excluded. + subsequence_value = subsequences.Get(expected_index, actual_index) + 1; + } else { + // Otherwise, the LCS length is the maximum of the LCS lengths + // relative to the prefixes where one element is excluded. + subsequence_value = + std::max(subsequences.Get(expected_index, actual_index + 1), + subsequences.Get(expected_index + 1, actual_index)); + } + subsequences.Set(expected_index + 1, actual_index + 1, subsequence_value); + } + } +} + +template +template +auto UnifiedDiffMatcher::PrintDiff( + const ActualContainer& actual, Table& match_results, + const Table& subsequences, + testing::MatchResultListener* listener) const -> void { + // A line in the diff output. + struct DiffLine { + enum class Kind { Match, ActualOnly, ExpectedOnly }; + Kind kind; + // Only used for `Match` and `ActualOnly`. + const ActualContainer::value_type* actual_value; + int expected_index; + }; + + llvm::SmallVector diff; + // Reserve a quick upper bound of the size. + diff.reserve(expected_.size() + std::size(actual)); + + // Reconstruct the diff by backtracking from the end of the table. + int expected_index = expected_.size() - 1; + int actual_index = std::size(actual) - 1; + auto actual_it = std::end(actual) - 1; + while (expected_index >= 0 || actual_index >= 0) { + auto match_result = (expected_index >= 0 && actual_index >= 0) + ? match_results.Get(expected_index, actual_index) + : MatchResult::DoesNotMatch; + CARBON_CHECK(match_result != MatchResult::Unknown); + if (match_result == MatchResult::Matches) { + // The element is in both lists for the diff. + diff.push_back({.kind = DiffLine::Kind::Match, + .actual_value = &*actual_it, + .expected_index = expected_index}); + --expected_index; + --actual_index; + --actual_it; + } else if (actual_index >= 0 && + (expected_index < 0 || + subsequences.Get(expected_index + 1, actual_index) >= + subsequences.Get(expected_index, actual_index + 1))) { + // Dropping an element from `actual` preserves the LCS length, so treat it + // as an insertion. + diff.push_back({.kind = DiffLine::Kind::ActualOnly, + .actual_value = &*actual_it, + .expected_index = std::max(0, expected_index)}); + --actual_index; + --actual_it; + } else { + // Otherwise, treat it as a deletion from `expected`. + diff.push_back({.kind = DiffLine::Kind::ExpectedOnly, + .actual_value = nullptr, + .expected_index = expected_index}); + --expected_index; + } + } + + struct PrintRange { + int begin; + int end; + }; + llvm::SmallVector print_ranges; + + constexpr int ContextLines = 3; + for (auto [i, line] : + llvm::reverse(llvm::zip_equal(llvm::seq(diff.size()), diff))) { + if (line.kind != DiffLine::Kind::Match) { + PrintRange range = { + .begin = std::max(0, i - ContextLines), + .end = std::min(diff.size() - 1, i + ContextLines)}; + if (print_ranges.empty() || print_ranges.back().begin > range.end + 1) { + print_ranges.push_back(range); + } else { + // Merge diffs with overlapping context. + print_ranges.back().begin = range.begin; + } + } + } + + *listener << "unified diff (- expected, + actual):\n"; + for (const auto& range : print_ranges) { + *listener << "=== diff in expected elements " + << diff[range.end].expected_index + 1 << " to " + << diff[range.begin].expected_index + 1 << " (1-based index):\n"; + for (auto i : llvm::reverse(llvm::seq_inclusive(range.begin, range.end))) { + const auto& line = diff[i]; + if (line.kind == DiffLine::Kind::Match) { + *listener << " " << *line.actual_value << "\n"; + } else if (line.kind == DiffLine::Kind::ActualOnly) { + *listener << "+ " << *line.actual_value << "\n"; + } else { + *listener << "- "; + expected_[line.expected_index].DescribeTo(listener->stream()); + *listener << "\n"; + } + } + } + *listener << "=== diff end\n"; +} + +} // namespace Carbon::Testing + +#endif // CARBON_TESTING_BASE_UNIFIED_DIFF_MATCHER_H_ diff --git a/testing/base/unified_diff_matcher_test.cpp b/testing/base/unified_diff_matcher_test.cpp new file mode 100644 index 000000000000..797aa16548ca --- /dev/null +++ b/testing/base/unified_diff_matcher_test.cpp @@ -0,0 +1,247 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "testing/base/unified_diff_matcher.h" + +#include +#include + +#include + +#include "llvm/ADT/SmallVector.h" + +namespace Carbon::Testing { +namespace { + +using ::testing::Matcher; +using ::testing::StrEq; + +// Asserts that when expected does not match actual, the string +// representation of the produced diff equals expected_diff. +auto ExpectUnifiedDiff(const llvm::SmallVector& actual, + const llvm::SmallVector>& expected, + const std::string& expected_diff) -> void { + testing::StringMatchResultListener listener; + EXPECT_FALSE(testing::ExplainMatchResult( + ElementsAreArrayWithUnifiedDiff(expected), actual, &listener)); + EXPECT_THAT(listener.str(), testing::Eq(expected_diff)); +} + +TEST(UnifiedDiffMatcherTest, Matches) { + llvm::SmallVector actual = {"A", "B", "C"}; + llvm::SmallVector> expected = {StrEq("A"), StrEq("B"), + StrEq("C")}; + EXPECT_THAT(actual, ElementsAreArrayWithUnifiedDiff(expected)); +} + +TEST(UnifiedDiffMatcherTest, MismatchMissing) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 3 (1-based index): + A +- is equal to "B" + C +=== diff end +)"; + ExpectUnifiedDiff({"A", "C"}, {StrEq("A"), StrEq("B"), StrEq("C")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchExtra) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 2 (1-based index): + A ++ B + C +=== diff end +)"; + ExpectUnifiedDiff({"A", "B", "C"}, {StrEq("A"), StrEq("C")}, ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchBoth) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 2 (1-based index): + A +- is equal to "C" ++ B +=== diff end +)"; + ExpectUnifiedDiff({"A", "B"}, {StrEq("A"), StrEq("C")}, ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchMultiple) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 5 (1-based index): + A +- is equal to "B" ++ X + C +- is equal to "D" ++ Y + E +=== diff end +)"; + ExpectUnifiedDiff( + {"A", "X", "C", "Y", "E"}, + {StrEq("A"), StrEq("B"), StrEq("C"), StrEq("D"), StrEq("E")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchLongContext) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 2 to 8 (1-based index): + 1 + 2 + 3 +- is equal to "X" ++ 4 + 5 + 6 + 7 +=== diff end +)"; + ExpectUnifiedDiff({"0", "1", "2", "3", "4", "5", "6", "7", "8"}, + {StrEq("0"), StrEq("1"), StrEq("2"), StrEq("3"), StrEq("X"), + StrEq("5"), StrEq("6"), StrEq("7"), StrEq("8")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, Mismatch5LineContext) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 7 (1-based index): +- is equal to "X" ++ 0 + 1 + 2 + 3 + 4 + 5 +- is equal to "Y" ++ 6 +=== diff end +)"; + ExpectUnifiedDiff({"0", "1", "2", "3", "4", "5", "6"}, + {StrEq("X"), StrEq("1"), StrEq("2"), StrEq("3"), StrEq("4"), + StrEq("5"), StrEq("Y")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, Mismatch6LineContext) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 8 (1-based index): +- is equal to "X" ++ 0 + 1 + 2 + 3 + 4 + 5 + 6 +- is equal to "Y" ++ 7 +=== diff end +)"; + ExpectUnifiedDiff({"0", "1", "2", "3", "4", "5", "6", "7"}, + {StrEq("X"), StrEq("1"), StrEq("2"), StrEq("3"), StrEq("4"), + StrEq("5"), StrEq("6"), StrEq("Y")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, Mismatch7LineContext) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 4 (1-based index): +- is equal to "X" ++ 0 + 1 + 2 + 3 +=== diff in expected elements 6 to 9 (1-based index): + 5 + 6 + 7 +- is equal to "Y" ++ 8 +=== diff end +)"; + ExpectUnifiedDiff({"0", "1", "2", "3", "4", "5", "6", "7", "8"}, + {StrEq("X"), StrEq("1"), StrEq("2"), StrEq("3"), StrEq("4"), + StrEq("5"), StrEq("6"), StrEq("7"), StrEq("Y")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchEmptyExpected) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 1 (1-based index): ++ A +=== diff end +)"; + ExpectUnifiedDiff({"A"}, {}, ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchEmptyActual) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 1 (1-based index): +- is equal to "A" +=== diff end +)"; + ExpectUnifiedDiff({}, {StrEq("A")}, ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchLongDifference) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 4 (1-based index): + 1 +- is equal to "2" +- is equal to "3" ++ X ++ Y ++ Z + 4 +=== diff end +)"; + ExpectUnifiedDiff({"1", "X", "Y", "Z", "4"}, + {StrEq("1"), StrEq("2"), StrEq("3"), StrEq("4")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchGreedyResyncActualMissing) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 6 (1-based index): + 1 + 2 +- is equal to "3" ++ X ++ 7 + 4 + 5 + 6 +=== diff end +)"; + ExpectUnifiedDiff({"1", "2", "X", "7", "4", "5", "6", "7", "8", "9"}, + {StrEq("1"), StrEq("2"), StrEq("3"), StrEq("4"), StrEq("5"), + StrEq("6"), StrEq("7"), StrEq("8"), StrEq("9")}, + ExpectedDiff); +} + +TEST(UnifiedDiffMatcherTest, MismatchGreedyResyncExpectedMissing) { + constexpr char ExpectedDiff[] = R"(unified diff (- expected, + actual): +=== diff in expected elements 1 to 7 (1-based index): + 1 + 2 +- is equal to "X" +- is equal to "7" ++ 3 + 4 + 5 + 6 +=== diff end +)"; + ExpectUnifiedDiff( + {"1", "2", "3", "4", "5", "6", "7", "8", "9"}, + {StrEq("1"), StrEq("2"), StrEq("X"), StrEq("7"), StrEq("4"), StrEq("5"), + StrEq("6"), StrEq("7"), StrEq("8"), StrEq("9")}, + ExpectedDiff); +} + +} // namespace +} // namespace Carbon::Testing diff --git a/testing/file_test/BUILD b/testing/file_test/BUILD index 681dee33b22b..3ca35b141408 100644 --- a/testing/file_test/BUILD +++ b/testing/file_test/BUILD @@ -54,6 +54,7 @@ cc_library( "//common:raw_string_ostream", "//common:set", "//testing/base:file_helpers", + "//testing/base:unified_diff_matcher", "@abseil-cpp//absl/flags:flag", "@abseil-cpp//absl/flags:parse", "@abseil-cpp//absl/strings", diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp index 18f71a4481c9..a316d6d35f21 100644 --- a/testing/file_test/file_test_base.cpp +++ b/testing/file_test/file_test_base.cpp @@ -50,6 +50,7 @@ #include "llvm/Support/PrettyStackTrace.h" #include "llvm/Support/Process.h" #include "llvm/Support/ThreadPool.h" +#include "testing/base/unified_diff_matcher.h" #include "testing/file_test/autoupdate.h" #include "testing/file_test/run_test.h" #include "testing/file_test/test_file.h" @@ -119,7 +120,6 @@ static auto SplitOutput(llvm::StringRef output) llvm::StringRef(output).split(lines, "\n"); return llvm::SmallVector(lines.begin(), lines.end()); } - // Verify that the success and `fail_` prefix use correspond. Separately handle // both cases for clearer test failures. static auto CompareFailPrefix(llvm::StringRef filename, bool success) -> void { @@ -259,13 +259,9 @@ auto FileTestCase::TestBody() -> void { } else { EXPECT_THAT(SplitOutput(test_file.actual_stdout), - ElementsAreArray(test_file.expected_stdout)) - << "Actual text:\n" - << test_file.actual_stdout; + ElementsAreArrayWithUnifiedDiff(test_file.expected_stdout)); EXPECT_THAT(SplitOutput(test_file.actual_stderr), - ElementsAreArray(test_file.expected_stderr)) - << "Actual text:\n" - << test_file.actual_stderr; + ElementsAreArrayWithUnifiedDiff(test_file.expected_stderr)); } if (HasFailure()) {