From f8794634c286810ee97544e1e4ea0cbd1555d66d 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 | 29 +++++++++++++++++++ src/catch2/catch_matchers.h | 14 +++++---- .../Baselines/automake.sw.approved.txt | 1 + .../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 + tests/SelfTest/Baselines/tap.sw.approved.txt | 6 +++- .../Baselines/teamcity.sw.approved.txt | 2 ++ tests/SelfTest/Baselines/xml.sw.approved.txt | 23 +++++++++++++-- tests/SelfTest/UsageTests/Matchers.tests.cpp | 10 +++++++ 12 files changed, 103 insertions(+), 14 deletions(-) diff --git a/docs/deprecations.md b/docs/deprecations.md index c416ad6d..ca277d7e 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -9,6 +9,35 @@ either of these is a breaking change, and thus will not happen until at least the next major release. +### 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 ### `CHECKED_IF` and `CHECKED_ELSE` diff --git a/src/catch2/catch_matchers.h b/src/catch2/catch_matchers.h index 78d9e84e..08b69584 100644 --- a/src/catch2/catch_matchers.h +++ b/src/catch2/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/tests/SelfTest/Baselines/automake.sw.approved.txt b/tests/SelfTest/Baselines/automake.sw.approved.txt index db7f4006..77e56433 100644 --- a/tests/SelfTest/Baselines/automake.sw.approved.txt +++ b/tests/SelfTest/Baselines/automake.sw.approved.txt @@ -99,6 +99,7 @@ Nor would this :test-result: PASS Comparisons between ints where one side is computed :test-result: PASS Comparisons between unsigned ints and negative signed ints match c++ standard behaviour :test-result: PASS Comparisons with int literals don't warn when mixing signed/ unsigned +:test-result: PASS Composed matchers are distinct :test-result: FAIL Contains string matcher :test-result: PASS Copy and then generate a range :test-result: FAIL Custom exceptions can be translated when testing for nothrow diff --git a/tests/SelfTest/Baselines/compact.sw.approved.txt b/tests/SelfTest/Baselines/compact.sw.approved.txt index 48007822..66c48408 100644 --- a/tests/SelfTest/Baselines/compact.sw.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.approved.txt @@ -344,6 +344,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/tests/SelfTest/Baselines/console.std.approved.txt b/tests/SelfTest/Baselines/console.std.approved.txt index 69ba4035..a0603ad8 100644 --- a/tests/SelfTest/Baselines/console.std.approved.txt +++ b/tests/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: 328 | 254 passed | 70 failed | 4 failed as expected -assertions: 1829 | 1677 passed | 131 failed | 21 failed as expected +test cases: 329 | 255 passed | 70 failed | 4 failed as expected +assertions: 1831 | 1679 passed | 131 failed | 21 failed as expected diff --git a/tests/SelfTest/Baselines/console.sw.approved.txt b/tests/SelfTest/Baselines/console.sw.approved.txt index d6285a81..10eada63 100644 --- a/tests/SelfTest/Baselines/console.sw.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.approved.txt @@ -2610,6 +2610,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 ------------------------------------------------------------------------------- @@ -14300,6 +14318,6 @@ Misc.tests.cpp: Misc.tests.cpp:: PASSED: =============================================================================== -test cases: 328 | 238 passed | 86 failed | 4 failed as expected -assertions: 1846 | 1677 passed | 148 failed | 21 failed as expected +test cases: 329 | 239 passed | 86 failed | 4 failed as expected +assertions: 1848 | 1679 passed | 148 failed | 21 failed as expected diff --git a/tests/SelfTest/Baselines/junit.sw.approved.txt b/tests/SelfTest/Baselines/junit.sw.approved.txt index 0be5cc07..b10151f1 100644 --- a/tests/SelfTest/Baselines/junit.sw.approved.txt +++ b/tests/SelfTest/Baselines/junit.sw.approved.txt @@ -1,7 +1,7 @@ - + @@ -364,6 +364,7 @@ Exception.tests.cpp: + FAILED: diff --git a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt index 07c512f0..1b446e04 100644 --- a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt +++ b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt @@ -923,6 +923,7 @@ Exception.tests.cpp: + FAILED: diff --git a/tests/SelfTest/Baselines/tap.sw.approved.txt b/tests/SelfTest/Baselines/tap.sw.approved.txt index b22d58c5..9e366479 100644 --- a/tests/SelfTest/Baselines/tap.sw.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.approved.txt @@ -686,6 +686,10 @@ ok {test-number} - 5 == c for: 5 == 5 ok {test-number} - 6 == uc for: 6 == 6 # Comparisons with int literals don't warn when mixing signed/ unsigned ok {test-number} - (std::numeric_limits::max)() > ul for: 4294967295 (0x) > 4 +# Composed matchers are distinct +ok {test-number} - testStringForMatching2(), !composed1 for: "some completely different text that contains one common word" not ( contains: "string" or contains: "random" ) +# Composed matchers are distinct +ok {test-number} - testStringForMatching2(), composed2 for: "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" ) # Contains string matcher not ok {test-number} - testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) for: "this string contains 'abc' as a substring" contains: "not there" (case insensitive) # Contains string matcher @@ -3684,5 +3688,5 @@ ok {test-number} - q3 == 23. for: 23.0 == 23.0 ok {test-number} - # xmlentitycheck ok {test-number} - -1..1838 +1..1840 diff --git a/tests/SelfTest/Baselines/teamcity.sw.approved.txt b/tests/SelfTest/Baselines/teamcity.sw.approved.txt index dbfb9835..e3d1ead2 100644 --- a/tests/SelfTest/Baselines/teamcity.sw.approved.txt +++ b/tests/SelfTest/Baselines/teamcity.sw.approved.txt @@ -231,6 +231,8 @@ Exception.tests.cpp:|nunexpected exception with message:|n "unexpe ##teamcity[testFinished name='Comparisons between unsigned ints and negative signed ints match c++ standard behaviour' duration="{duration}"] ##teamcity[testStarted name='Comparisons with int literals don|'t warn when mixing signed/ unsigned'] ##teamcity[testFinished name='Comparisons with int literals don|'t warn when mixing signed/ unsigned' duration="{duration}"] +##teamcity[testStarted name='Composed matchers are distinct'] +##teamcity[testFinished name='Composed matchers are distinct' duration="{duration}"] ##teamcity[testStarted name='Contains string matcher'] Matchers.tests.cpp:|nexpression failed|n CHECK_THAT( testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) )|nwith expansion:|n "this string contains |'abc|' as a substring" contains: "not there" (case insensitive)|n'] Matchers.tests.cpp:|nexpression failed|n CHECK_THAT( testStringForMatching(), Contains("STRING") )|nwith expansion:|n "this string contains |'abc|' as a substring" contains: "STRING"|n'] diff --git a/tests/SelfTest/Baselines/xml.sw.approved.txt b/tests/SelfTest/Baselines/xml.sw.approved.txt index d2d0ce54..63f6d760 100644 --- a/tests/SelfTest/Baselines/xml.sw.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.approved.txt @@ -3011,6 +3011,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" ) + + + + @@ -17139,7 +17158,7 @@ loose text artifact - + - + diff --git a/tests/SelfTest/UsageTests/Matchers.tests.cpp b/tests/SelfTest/UsageTests/Matchers.tests.cpp index e10e98ca..e5c28ef9 100644 --- a/tests/SelfTest/UsageTests/Matchers.tests.cpp +++ b/tests/SelfTest/UsageTests/Matchers.tests.cpp @@ -556,6 +556,16 @@ namespace { namespace MatchersTests { 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); + } + template struct EqualsRangeMatcher : Catch::MatcherGenericBase {