From f626ec47020aec8baa5d2a6a8546d53a6b3db8af Mon Sep 17 00:00:00 2001 From: "Manikandan K. S." Date: Sun, 26 Jul 2026 00:51:45 +0530 Subject: [PATCH] fix: avoid double invocation of side-effecting matchers in PredicateFormatterFromMatcher EXPECT_THAT with Throws() previously invoked the callable twice on failure: once in Matches() and again in MatchPrintAndExplain() when formatting the diagnostic message. This caused issues with stateful or side-effecting callables. Fix: call MatchAndExplain() once with an interested listener, capture the result and explanation, then manually format the failure message using UniversalPrint() + type info + listener explanation. BREAKING CHANGE: PredicateFormatterFromMatcher no longer detects flaky matchers that change their result based on IsInterested(). This only affects pathological matchers; real matchers return consistent results regardless of listener interest. Fixes #4073 --- googlemock/include/gmock/gmock-matchers.h | 25 +++++++++++-------- .../test/gmock-matchers-containers_test.cc | 20 +++------------ googlemock/test/gmock-matchers-misc_test.cc | 13 ++++++++++ 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/googlemock/include/gmock/gmock-matchers.h b/googlemock/include/gmock/gmock-matchers.h index 603a5029f..0c195373f 100644 --- a/googlemock/include/gmock/gmock-matchers.h +++ b/googlemock/include/gmock/gmock-matchers.h @@ -1719,9 +1719,13 @@ class [[nodiscard]] PredicateFormatterFromMatcher { // potentially unsafe downcasting of the matcher argument. const Matcher matcher = SafeMatcherCast(matcher_); - // The expected path here is that the matcher should match (i.e. that most - // tests pass) so optimize for this case. - if (matcher.Matches(x)) { + // Capture the explanation during the initial match attempt so that + // side-effecting matchers (like Throws) are not invoked a second time + // when formatting the failure message. + StringMatchResultListener listener; + const bool matches = matcher.MatchAndExplain(x, &listener); + + if (matches) { return AssertionSuccess(); } @@ -1729,14 +1733,15 @@ class [[nodiscard]] PredicateFormatterFromMatcher { ss << "Value of: " << value_text << "\n" << "Expected: "; matcher.DescribeTo(&ss); - - // Rerun the matcher to "PrintAndExplain" the failure. - StringMatchResultListener listener; - if (MatchPrintAndExplain(x, matcher, &listener)) { - ss << "\n The matcher failed on the initial attempt; but passed when " - "rerun to generate the explanation."; + ss << "\n Actual: "; + UniversalPrint(x, &ss); +#if GTEST_HAS_RTTI + { + const std::string& type_name = GetTypeName(); + if (IsReadableTypeName(type_name)) ss << " (of type " << type_name << ")"; } - ss << "\n Actual: " << listener.str(); +#endif + PrintIfNotEmpty(listener.str(), &ss); return AssertionFailure() << ss.str(); } diff --git a/googlemock/test/gmock-matchers-containers_test.cc b/googlemock/test/gmock-matchers-containers_test.cc index 697509f71..c05ff09ca 100644 --- a/googlemock/test/gmock-matchers-containers_test.cc +++ b/googlemock/test/gmock-matchers-containers_test.cc @@ -2805,9 +2805,6 @@ class PredicateFormatterFromMatcherTest : public ::testing::Test { protected: enum Behavior { kInitialSuccess, kAlwaysFail, kFlaky }; - // A matcher that can return different results when used multiple times on the - // same input. No real matcher should do this; but this lets us test that we - // detect such behavior and fail appropriately. class MockMatcher : public MatcherInterface { public: bool MatchAndExplain(Behavior behavior, @@ -2815,20 +2812,11 @@ class PredicateFormatterFromMatcherTest : public ::testing::Test { *listener << "[MatchAndExplain]"; switch (behavior) { case kInitialSuccess: - // The first call to MatchAndExplain should use a "not interested" - // listener; so this is expected to return |true|. There should be no - // subsequent calls. - return !listener->IsInterested(); - + return true; case kAlwaysFail: return false; - case kFlaky: - // The first call to MatchAndExplain should use a "not interested" - // listener; so this will return |false|. Subsequent calls should have - // an "interested" listener; so this will return |true|, thus - // simulating a flaky matcher. - return listener->IsInterested(); + return false; } GTEST_LOG_(FATAL) << "This should never be reached"; @@ -2867,13 +2855,11 @@ TEST_F(PredicateFormatterFromMatcherTest, NoShortCircuitOnFailure) { EXPECT_EQ(expect, result.message()); } -TEST_F(PredicateFormatterFromMatcherTest, DetectsFlakyShortCircuit) { +TEST_F(PredicateFormatterFromMatcherTest, AlwaysFails) { AssertionResult result = RunPredicateFormatter(kFlaky); EXPECT_FALSE(result); // Implicit cast to bool. std::string expect = "Value of: dummy-name\nExpected: [DescribeTo]\n" - " The matcher failed on the initial attempt; but passed when rerun to " - "generate the explanation.\n" " Actual: 2" + OfType(internal::GetTypeName()) + ", [MatchAndExplain]"; EXPECT_EQ(expect, result.message()); diff --git a/googlemock/test/gmock-matchers-misc_test.cc b/googlemock/test/gmock-matchers-misc_test.cc index 16d62be24..18ffaa809 100644 --- a/googlemock/test/gmock-matchers-misc_test.cc +++ b/googlemock/test/gmock-matchers-misc_test.cc @@ -1754,6 +1754,19 @@ TEST(ThrowsTest, CallableExecutedExactlyOnce) { EXPECT_EQ(a, 4u); } +TEST(ThrowsTest, EvaluatesCallableOnlyOnceOnFailure) { + int call_count = 0; + auto stateful_lambda = [&call_count]() { + ++call_count; + }; + + EXPECT_NONFATAL_FAILURE( + EXPECT_THAT(stateful_lambda, Throws()), + "does not throw any exception"); + + EXPECT_EQ(call_count, 1); +} + TEST(ThrowsTest, Describe) { Matcher> matcher = Throws(); std::stringstream ss;