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.
This commit is contained in:
Martin Hořeňovský 2020-02-01 20:55:42 +01:00
parent d6b2a3793b
commit f8794634c2
No known key found for this signature in database
GPG Key ID: DE48307B8B0D381A
12 changed files with 103 additions and 14 deletions

View File

@ -9,6 +9,35 @@ either of these is a breaking change, and thus will not happen until
at least the next major release. 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 ## Planned changes
### `CHECKED_IF` and `CHECKED_ELSE` ### `CHECKED_IF` and `CHECKED_ELSE`

View File

@ -91,9 +91,10 @@ namespace Matchers {
return description; return description;
} }
MatchAllOf<ArgT>& operator && ( MatcherBase<ArgT> const& other ) { MatchAllOf<ArgT> operator && ( MatcherBase<ArgT> const& other ) {
m_matchers.push_back( &other ); auto copy(*this);
return *this; copy.m_matchers.push_back( &other );
return copy;
} }
std::vector<MatcherBase<ArgT> const*> m_matchers; std::vector<MatcherBase<ArgT> const*> m_matchers;
@ -124,9 +125,10 @@ namespace Matchers {
return description; return description;
} }
MatchAnyOf<ArgT>& operator || ( MatcherBase<ArgT> const& other ) { MatchAnyOf<ArgT> operator || ( MatcherBase<ArgT> const& other ) {
m_matchers.push_back( &other ); auto copy(*this);
return *this; copy.m_matchers.push_back( &other );
return copy;
} }
std::vector<MatcherBase<ArgT> const*> m_matchers; std::vector<MatcherBase<ArgT> const*> m_matchers;

View File

@ -99,6 +99,7 @@ Nor would this
:test-result: PASS Comparisons between ints where one side is computed :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 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 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: FAIL Contains string matcher
:test-result: PASS Copy and then generate a range :test-result: PASS Copy and then generate a range
:test-result: FAIL Custom exceptions can be translated when testing for nothrow :test-result: FAIL Custom exceptions can be translated when testing for nothrow

View File

@ -344,6 +344,8 @@ Condition.tests.cpp:<line number>: passed: 4 == ul for: 4 == 4
Condition.tests.cpp:<line number>: passed: 5 == c for: 5 == 5 Condition.tests.cpp:<line number>: passed: 5 == c for: 5 == 5
Condition.tests.cpp:<line number>: passed: 6 == uc for: 6 == 6 Condition.tests.cpp:<line number>: passed: 6 == uc for: 6 == 6
Condition.tests.cpp:<line number>: passed: (std::numeric_limits<uint32_t>::max)() > ul for: 4294967295 (0x<hex digits>) > 4 Condition.tests.cpp:<line number>: passed: (std::numeric_limits<uint32_t>::max)() > ul for: 4294967295 (0x<hex digits>) > 4
Matchers.tests.cpp:<line number>: passed: testStringForMatching2(), !composed1 for: "some completely different text that contains one common word" not ( contains: "string" or contains: "random" )
Matchers.tests.cpp:<line number>: passed: testStringForMatching2(), composed2 for: "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" )
Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) for: "this string contains 'abc' as a substring" contains: "not there" (case insensitive) Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) for: "this string contains 'abc' as a substring" contains: "not there" (case insensitive)
Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("STRING") for: "this string contains 'abc' as a substring" contains: "STRING" Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("STRING") for: "this string contains 'abc' as a substring" contains: "STRING"
Generators.tests.cpp:<line number>: passed: elem % 2 == 1 for: 1 == 1 Generators.tests.cpp:<line number>: passed: elem % 2 == 1 for: 1 == 1

View File

@ -1380,6 +1380,6 @@ due to unexpected exception with message:
Why would you throw a std::string? Why would you throw a std::string?
=============================================================================== ===============================================================================
test cases: 328 | 254 passed | 70 failed | 4 failed as expected test cases: 329 | 255 passed | 70 failed | 4 failed as expected
assertions: 1829 | 1677 passed | 131 failed | 21 failed as expected assertions: 1831 | 1679 passed | 131 failed | 21 failed as expected

View File

@ -2610,6 +2610,24 @@ Condition.tests.cpp:<line number>: PASSED:
with expansion: with expansion:
4294967295 (0x<hex digits>) > 4 4294967295 (0x<hex digits>) > 4
-------------------------------------------------------------------------------
Composed matchers are distinct
-------------------------------------------------------------------------------
Matchers.tests.cpp:<line number>
...............................................................................
Matchers.tests.cpp:<line number>: 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:<line number>: 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 Contains string matcher
------------------------------------------------------------------------------- -------------------------------------------------------------------------------
@ -14300,6 +14318,6 @@ Misc.tests.cpp:<line number>
Misc.tests.cpp:<line number>: PASSED: Misc.tests.cpp:<line number>: PASSED:
=============================================================================== ===============================================================================
test cases: 328 | 238 passed | 86 failed | 4 failed as expected test cases: 329 | 239 passed | 86 failed | 4 failed as expected
assertions: 1846 | 1677 passed | 148 failed | 21 failed as expected assertions: 1848 | 1679 passed | 148 failed | 21 failed as expected

View File

@ -1,7 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?> <?xml version="1.0" encoding="UTF-8"?>
<testsuitesloose text artifact <testsuitesloose text artifact
> >
<testsuite name="<exe-name>" errors="17" failures="132" tests="1847" hostname="tbd" time="{duration}" timestamp="{iso8601-timestamp}"> <testsuite name="<exe-name>" errors="17" failures="132" tests="1849" hostname="tbd" time="{duration}" timestamp="{iso8601-timestamp}">
<properties> <properties>
<property name="filters" value="~[!nonportable]~[!benchmark]~[approvals] *"/> <property name="filters" value="~[!nonportable]~[!benchmark]~[approvals] *"/>
<property name="random-seed" value="1"/> <property name="random-seed" value="1"/>
@ -364,6 +364,7 @@ Exception.tests.cpp:<line number>
<testcase classname="<exe-name>.global" name="Comparisons between ints where one side is computed" time="{duration}"/> <testcase classname="<exe-name>.global" name="Comparisons between ints where one side is computed" time="{duration}"/>
<testcase classname="<exe-name>.global" name="Comparisons between unsigned ints and negative signed ints match c++ standard behaviour" time="{duration}"/> <testcase classname="<exe-name>.global" name="Comparisons between unsigned ints and negative signed ints match c++ standard behaviour" time="{duration}"/>
<testcase classname="<exe-name>.global" name="Comparisons with int literals don't warn when mixing signed/ unsigned" time="{duration}"/> <testcase classname="<exe-name>.global" name="Comparisons with int literals don't warn when mixing signed/ unsigned" time="{duration}"/>
<testcase classname="<exe-name>.global" name="Composed matchers are distinct" time="{duration}"/>
<testcase classname="<exe-name>.global" name="Contains string matcher" time="{duration}"> <testcase classname="<exe-name>.global" name="Contains string matcher" time="{duration}">
<failure message="testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No)" type="CHECK_THAT"> <failure message="testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No)" type="CHECK_THAT">
FAILED: FAILED:

View File

@ -923,6 +923,7 @@ Exception.tests.cpp:<line number>
<testCase name="Combining only templated matchers" duration="{duration}"/> <testCase name="Combining only templated matchers" duration="{duration}"/>
<testCase name="Combining templated and concrete matchers" duration="{duration}"/> <testCase name="Combining templated and concrete matchers" duration="{duration}"/>
<testCase name="Combining templated matchers" duration="{duration}"/> <testCase name="Combining templated matchers" duration="{duration}"/>
<testCase name="Composed matchers are distinct" duration="{duration}"/>
<testCase name="Contains string matcher" duration="{duration}"> <testCase name="Contains string matcher" duration="{duration}">
<failure message="CHECK_THAT(testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No))"> <failure message="CHECK_THAT(testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No))">
FAILED: FAILED:

View File

@ -686,6 +686,10 @@ ok {test-number} - 5 == c for: 5 == 5
ok {test-number} - 6 == uc for: 6 == 6 ok {test-number} - 6 == uc for: 6 == 6
# Comparisons with int literals don't warn when mixing signed/ unsigned # Comparisons with int literals don't warn when mixing signed/ unsigned
ok {test-number} - (std::numeric_limits<uint32_t>::max)() > ul for: 4294967295 (0x<hex digits>) > 4 ok {test-number} - (std::numeric_limits<uint32_t>::max)() > ul for: 4294967295 (0x<hex digits>) > 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 # 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) 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 # Contains string matcher
@ -3684,5 +3688,5 @@ ok {test-number} - q3 == 23. for: 23.0 == 23.0
ok {test-number} - ok {test-number} -
# xmlentitycheck # xmlentitycheck
ok {test-number} - ok {test-number} -
1..1838 1..1840

View File

@ -231,6 +231,8 @@ Exception.tests.cpp:<line number>|nunexpected exception with message:|n "unexpe
##teamcity[testFinished name='Comparisons between unsigned ints and negative signed ints match c++ standard behaviour' duration="{duration}"] ##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[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[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'] ##teamcity[testStarted name='Contains string matcher']
Matchers.tests.cpp:<line number>|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:<line number>|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:<line number>|nexpression failed|n CHECK_THAT( testStringForMatching(), Contains("STRING") )|nwith expansion:|n "this string contains |'abc|' as a substring" contains: "STRING"|n'] Matchers.tests.cpp:<line number>|nexpression failed|n CHECK_THAT( testStringForMatching(), Contains("STRING") )|nwith expansion:|n "this string contains |'abc|' as a substring" contains: "STRING"|n']

View File

@ -3011,6 +3011,25 @@ Nor would this
</Expression> </Expression>
<OverallResult success="true"/> <OverallResult success="true"/>
</TestCase> </TestCase>
<TestCase name="Composed matchers are distinct" tags="[composed][matchers]" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" >
<Expression success="true" type="REQUIRE_THAT" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" >
<Original>
testStringForMatching2(), !composed1
</Original>
<Expanded>
"some completely different text that contains one common word" not ( contains: "string" or contains: "random" )
</Expanded>
</Expression>
<Expression success="true" type="REQUIRE_THAT" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" >
<Original>
testStringForMatching2(), composed2
</Original>
<Expanded>
"some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" )
</Expanded>
</Expression>
<OverallResult success="true"/>
</TestCase>
<TestCase name="Contains string matcher" tags="[.][failing][matchers]" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" > <TestCase name="Contains string matcher" tags="[.][failing][matchers]" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" >
<Expression success="false" type="CHECK_THAT" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" > <Expression success="false" type="CHECK_THAT" filename="tests/<exe-name>/UsageTests/Matchers.tests.cpp" >
<Original> <Original>
@ -17139,7 +17158,7 @@ loose text artifact
</Section> </Section>
<OverallResult success="true"/> <OverallResult success="true"/>
</TestCase> </TestCase>
<OverallResults successes="1677" failures="149" expectedFailures="21"/> <OverallResults successes="1679" failures="149" expectedFailures="21"/>
</Group> </Group>
<OverallResults successes="1677" failures="148" expectedFailures="21"/> <OverallResults successes="1679" failures="148" expectedFailures="21"/>
</Catch> </Catch>

View File

@ -556,6 +556,16 @@ namespace { namespace MatchersTests {
REQUIRE_THROWS_MATCHES(throwsSpecialException(2), SpecialException, Message("SpecialException::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);
}
template<typename Range> template<typename Range>
struct EqualsRangeMatcher : Catch::MatcherGenericBase { struct EqualsRangeMatcher : Catch::MatcherGenericBase {