From d49a6654845491fcb2c1d05ef20648c5b7bed783 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Tue, 16 Jul 2024 05:22:04 -0700 Subject: [PATCH] Use matcher's description in AllOf if matcher has no explanation. PiperOrigin-RevId: 652798234 Change-Id: I8e92248a2d9faf2a5719fe220145ea563acc14ff --- googlemock/include/gmock/gmock-matchers.h | 44 +++++++------------ .../test/gmock-matchers-arithmetic_test.cc | 14 +++--- .../test/gmock-matchers-comparisons_test.cc | 4 +- 3 files changed, 23 insertions(+), 39 deletions(-) diff --git a/googlemock/include/gmock/gmock-matchers.h b/googlemock/include/gmock/gmock-matchers.h index 3daf617307..063ee6ca25 100644 --- a/googlemock/include/gmock/gmock-matchers.h +++ b/googlemock/include/gmock/gmock-matchers.h @@ -1300,48 +1300,34 @@ class AllOfMatcherImpl : public MatcherInterface { bool MatchAndExplain(const T& x, MatchResultListener* listener) const override { - // This method uses matcher's explanation when explaining the result. - // However, if matcher doesn't provide one, this method uses matcher's - // description. + // If either matcher1_ or matcher2_ doesn't match x, we only need + // to explain why one of them fails. std::string all_match_result; - for (const Matcher& matcher : matchers_) { + + for (size_t i = 0; i < matchers_.size(); ++i) { StringMatchResultListener slistener; - // Return explanation for first failed matcher. - if (!matcher.MatchAndExplain(x, &slistener)) { - const std::string explanation = slistener.str(); - if (!explanation.empty()) { - *listener << explanation; + if (matchers_[i].MatchAndExplain(x, &slistener)) { + if (all_match_result.empty()) { + all_match_result = slistener.str(); } else { - *listener << "which doesn't match (" << Describe(matcher) << ")"; + std::string result = slistener.str(); + if (!result.empty()) { + all_match_result += ", and "; + all_match_result += result; + } } - return false; - } - // Keep track of explanations in case all matchers succeed. - std::string explanation = slistener.str(); - if (explanation.empty()) { - explanation = Describe(matcher); - } - if (all_match_result.empty()) { - all_match_result = explanation; } else { - if (!explanation.empty()) { - all_match_result += ", and "; - all_match_result += explanation; - } + *listener << slistener.str(); + return false; } } + // Otherwise we need to explain why *both* of them match. *listener << all_match_result; return true; } private: - // Returns matcher description as a string. - std::string Describe(const Matcher& matcher) const { - StringMatchResultListener listener; - matcher.DescribeTo(listener.stream()); - return listener.str(); - } const std::vector> matchers_; }; diff --git a/googlemock/test/gmock-matchers-arithmetic_test.cc b/googlemock/test/gmock-matchers-arithmetic_test.cc index 7521c0df08..f176962855 100644 --- a/googlemock/test/gmock-matchers-arithmetic_test.cc +++ b/googlemock/test/gmock-matchers-arithmetic_test.cc @@ -559,9 +559,10 @@ TEST_P(AllOfTestP, ExplainsResult) { Matcher m; // Successful match. Both matchers need to explain. The second - // matcher doesn't give an explanation, so the matcher description is used. + // matcher doesn't give an explanation, so only the first matcher's + // explanation is printed. m = AllOf(GreaterThan(10), Lt(30)); - EXPECT_EQ("which is 15 more than 10, and is < 30", Explain(m, 25)); + EXPECT_EQ("which is 15 more than 10", Explain(m, 25)); // Successful match. Both matchers need to explain. m = AllOf(GreaterThan(10), GreaterThan(20)); @@ -571,9 +572,8 @@ TEST_P(AllOfTestP, ExplainsResult) { // Successful match. All matchers need to explain. The second // matcher doesn't given an explanation. m = AllOf(GreaterThan(10), Lt(30), GreaterThan(20)); - EXPECT_EQ( - "which is 15 more than 10, and is < 30, and which is 5 more than 20", - Explain(m, 25)); + EXPECT_EQ("which is 15 more than 10, and which is 5 more than 20", + Explain(m, 25)); // Successful match. All matchers need to explain. m = AllOf(GreaterThan(10), GreaterThan(20), GreaterThan(30)); @@ -588,10 +588,10 @@ TEST_P(AllOfTestP, ExplainsResult) { EXPECT_EQ("which is 5 less than 10", Explain(m, 5)); // Failed match. The second matcher, which failed, needs to - // explain. Since it doesn't given an explanation, the matcher text is + // explain. Since it doesn't given an explanation, nothing is // printed. m = AllOf(GreaterThan(10), Lt(30)); - EXPECT_EQ("which doesn't match (is < 30)", Explain(m, 40)); + EXPECT_EQ("", Explain(m, 40)); // Failed match. The second matcher, which failed, needs to // explain. diff --git a/googlemock/test/gmock-matchers-comparisons_test.cc b/googlemock/test/gmock-matchers-comparisons_test.cc index 9881155ac0..5b75b457be 100644 --- a/googlemock/test/gmock-matchers-comparisons_test.cc +++ b/googlemock/test/gmock-matchers-comparisons_test.cc @@ -2334,11 +2334,9 @@ TEST(ExplainMatchResultTest, AllOf_True_True) { EXPECT_EQ("which is 0 modulo 2, and which is 0 modulo 3", Explain(m, 6)); } -// Tests that when AllOf() succeeds, but matchers have no explanation, -// the matcher description is used. TEST(ExplainMatchResultTest, AllOf_True_True_2) { const Matcher m = AllOf(Ge(2), Le(3)); - EXPECT_EQ("is >= 2, and is <= 3", Explain(m, 2)); + EXPECT_EQ("", Explain(m, 2)); } INSTANTIATE_GTEST_MATCHER_TEST_P(ExplainmatcherResultTest);