From f20a9dbc6e115eeb2b48288bfb27234c29f7417a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 1 Feb 2020 20:55:42 +0100 Subject: [PATCH] Fix significant bug with storing composed matchers Given that in the 2 or so years that matchers are thing nobody complained, it seems that people do not actually write this sort of code, and the possibility will be removed in v3. However, to avoid correctness bugs, we will have to support this weird code in v2. --- docs/deprecations.md | 28 +++++++++++++++++++ include/internal/catch_matchers.h | 14 ++++++---- .../Baselines/compact.sw.approved.txt | 2 ++ .../Baselines/console.std.approved.txt | 4 +-- .../Baselines/console.sw.approved.txt | 22 +++++++++++++-- .../SelfTest/Baselines/junit.sw.approved.txt | 3 +- .../Baselines/sonarqube.sw.approved.txt | 1 + .../SelfTest/Baselines/xml.sw.approved.txt | 23 +++++++++++++-- .../SelfTest/UsageTests/Matchers.tests.cpp | 10 +++++++ 9 files changed, 94 insertions(+), 13 deletions(-) diff --git a/docs/deprecations.md b/docs/deprecations.md index c2b178e2..39194ed7 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -45,6 +45,34 @@ There should be no reason to ever have an empty `SourceLineInfo`, so the method will be removed. +### Composing lvalues of already composed matchers + +Because a significant bug in this use case has persisted for 2+ years +without a bug report, and to simplify the implementation, code that +composes lvalues of composed matchers will not compile. That is, +this code will no longer work: + +```cpp + auto m1 = Contains("string"); + auto m2 = Contains("random"); + auto composed1 = m1 || m2; + auto m3 = Contains("different"); + auto composed2 = composed1 || m3; + REQUIRE_THAT(foo(), !composed1); + REQUIRE_THAT(foo(), composed2); +``` + +Instead you will have to write this: + +```cpp + auto m1 = Contains("string"); + auto m2 = Contains("random"); + auto m3 = Contains("different"); + REQUIRE_THAT(foo(), !(m1 || m2)); + REQUIRE_THAT(foo(), m1 || m2 || m3); +``` + + ## Planned changes diff --git a/include/internal/catch_matchers.h b/include/internal/catch_matchers.h index 5b69c4a8..518a6e0d 100644 --- a/include/internal/catch_matchers.h +++ b/include/internal/catch_matchers.h @@ -91,9 +91,10 @@ namespace Matchers { return description; } - MatchAllOf& operator && ( MatcherBase const& other ) { - m_matchers.push_back( &other ); - return *this; + MatchAllOf operator && ( MatcherBase const& other ) { + auto copy(*this); + copy.m_matchers.push_back( &other ); + return copy; } std::vector const*> m_matchers; @@ -124,9 +125,10 @@ namespace Matchers { return description; } - MatchAnyOf& operator || ( MatcherBase const& other ) { - m_matchers.push_back( &other ); - return *this; + MatchAnyOf operator || ( MatcherBase const& other ) { + auto copy(*this); + copy.m_matchers.push_back( &other ); + return copy; } std::vector const*> m_matchers; diff --git a/projects/SelfTest/Baselines/compact.sw.approved.txt b/projects/SelfTest/Baselines/compact.sw.approved.txt index 628c6bdf..c12ce081 100644 --- a/projects/SelfTest/Baselines/compact.sw.approved.txt +++ b/projects/SelfTest/Baselines/compact.sw.approved.txt @@ -312,6 +312,8 @@ Condition.tests.cpp:: passed: 4 == ul for: 4 == 4 Condition.tests.cpp:: passed: 5 == c for: 5 == 5 Condition.tests.cpp:: passed: 6 == uc for: 6 == 6 Condition.tests.cpp:: passed: (std::numeric_limits::max)() > ul for: 4294967295 (0x) > 4 +Matchers.tests.cpp:: passed: testStringForMatching2(), !composed1 for: "some completely different text that contains one common word" not ( contains: "string" or contains: "random" ) +Matchers.tests.cpp:: passed: testStringForMatching2(), composed2 for: "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" ) Matchers.tests.cpp:: failed: testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) for: "this string contains 'abc' as a substring" contains: "not there" (case insensitive) Matchers.tests.cpp:: failed: testStringForMatching(), Contains("STRING") for: "this string contains 'abc' as a substring" contains: "STRING" Generators.tests.cpp:: passed: elem % 2 == 1 for: 1 == 1 diff --git a/projects/SelfTest/Baselines/console.std.approved.txt b/projects/SelfTest/Baselines/console.std.approved.txt index 74e576a9..f62b2e48 100644 --- a/projects/SelfTest/Baselines/console.std.approved.txt +++ b/projects/SelfTest/Baselines/console.std.approved.txt @@ -1380,6 +1380,6 @@ due to unexpected exception with message: Why would you throw a std::string? =============================================================================== -test cases: 305 | 231 passed | 70 failed | 4 failed as expected -assertions: 1664 | 1512 passed | 131 failed | 21 failed as expected +test cases: 306 | 232 passed | 70 failed | 4 failed as expected +assertions: 1666 | 1514 passed | 131 failed | 21 failed as expected diff --git a/projects/SelfTest/Baselines/console.sw.approved.txt b/projects/SelfTest/Baselines/console.sw.approved.txt index 25a92320..68357ef0 100644 --- a/projects/SelfTest/Baselines/console.sw.approved.txt +++ b/projects/SelfTest/Baselines/console.sw.approved.txt @@ -2390,6 +2390,24 @@ Condition.tests.cpp:: PASSED: with expansion: 4294967295 (0x) > 4 +------------------------------------------------------------------------------- +Composed matchers are distinct +------------------------------------------------------------------------------- +Matchers.tests.cpp: +............................................................................... + +Matchers.tests.cpp:: PASSED: + REQUIRE_THAT( testStringForMatching2(), !composed1 ) +with expansion: + "some completely different text that contains one common word" not ( + contains: "string" or contains: "random" ) + +Matchers.tests.cpp:: PASSED: + REQUIRE_THAT( testStringForMatching2(), composed2 ) +with expansion: + "some completely different text that contains one common word" ( contains: + "string" or contains: "random" or contains: "different" ) + ------------------------------------------------------------------------------- Contains string matcher ------------------------------------------------------------------------------- @@ -13302,6 +13320,6 @@ Misc.tests.cpp: Misc.tests.cpp:: PASSED: =============================================================================== -test cases: 305 | 215 passed | 86 failed | 4 failed as expected -assertions: 1681 | 1512 passed | 148 failed | 21 failed as expected +test cases: 306 | 216 passed | 86 failed | 4 failed as expected +assertions: 1683 | 1514 passed | 148 failed | 21 failed as expected diff --git a/projects/SelfTest/Baselines/junit.sw.approved.txt b/projects/SelfTest/Baselines/junit.sw.approved.txt index e0b7c1b8..59119a25 100644 --- a/projects/SelfTest/Baselines/junit.sw.approved.txt +++ b/projects/SelfTest/Baselines/junit.sw.approved.txt @@ -1,7 +1,7 @@ - + @@ -356,6 +356,7 @@ Exception.tests.cpp: + FAILED: diff --git a/projects/SelfTest/Baselines/sonarqube.sw.approved.txt b/projects/SelfTest/Baselines/sonarqube.sw.approved.txt index d6f6b090..1d7e92bf 100644 --- a/projects/SelfTest/Baselines/sonarqube.sw.approved.txt +++ b/projects/SelfTest/Baselines/sonarqube.sw.approved.txt @@ -892,6 +892,7 @@ Exception.tests.cpp: + FAILED: diff --git a/projects/SelfTest/Baselines/xml.sw.approved.txt b/projects/SelfTest/Baselines/xml.sw.approved.txt index 6d06368e..1021e168 100644 --- a/projects/SelfTest/Baselines/xml.sw.approved.txt +++ b/projects/SelfTest/Baselines/xml.sw.approved.txt @@ -2832,6 +2832,25 @@ Nor would this + + + + testStringForMatching2(), !composed1 + + + "some completely different text that contains one common word" not ( contains: "string" or contains: "random" ) + + + + + testStringForMatching2(), composed2 + + + "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" ) + + + + @@ -15893,7 +15912,7 @@ loose text artifact - + - + diff --git a/projects/SelfTest/UsageTests/Matchers.tests.cpp b/projects/SelfTest/UsageTests/Matchers.tests.cpp index 80e0420f..52f2912d 100644 --- a/projects/SelfTest/UsageTests/Matchers.tests.cpp +++ b/projects/SelfTest/UsageTests/Matchers.tests.cpp @@ -552,6 +552,16 @@ namespace { namespace MatchersTests { REQUIRE_THROWS_MATCHES(throwsSpecialException(2), SpecialException, !Message("DerivedException::what")); REQUIRE_THROWS_MATCHES(throwsSpecialException(2), SpecialException, Message("SpecialException::what")); } + + TEST_CASE("Composed matchers are distinct", "[matchers][composed]") { + auto m1 = Contains("string"); + auto m2 = Contains("random"); + auto composed1 = m1 || m2; + auto m3 = Contains("different"); + auto composed2 = composed1 || m3; + REQUIRE_THAT(testStringForMatching2(), !composed1); + REQUIRE_THAT(testStringForMatching2(), composed2); + } } } // namespace MatchersTests