False positive test #30

Closed
raulh39 opened this Issue Jul 27, 2016 · 4 comments

Projects

None yet

2 participants

@raulh39
raulh39 commented Jul 27, 2016

This simple program:

#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
#include "doctest.h"

TEST_CASE("This is doctest") {
    CHECK(1==9 || 1==8);
}

Compiled with g++ 5.4.0 or clang++ 3.8.0 generates a program that, when executed, shows:

[doctest] doctest version is "1.0.0"
[doctest] run with "-dt-help" for options
===============================================================================
[doctest] test cases:    1 |    1 passed |    0 failed
[doctest] assertions:    1 |    1 passed |    0 failed

But obviously "1==9 || 1==8" is false.

(A similar program with Catch produces an error saying "STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison")

I don't known if this is a known limitation, but it is a disturbing one.

@onqtam
Owner
onqtam commented Jul 27, 2016 edited

This is planned to be fixed (the way Catch does it) in 1.1 - I will also limit the expression decomposition to only binary expressions the same way.

Unfortunately the release of 1.1 was delayed but it will come. Thanks for the report!

I think that in this particular case 9 || 1 gets evaluated first and the result is a bool, and then that bool is compared to 1 which results in a bool which is compared to 8 which gets converted to a bool true and so the test "passes". Adding parenthesis should fix this - like this: (1==9) || (1==8)

@raulh39
raulh39 commented Jul 28, 2016

That's great news!

This is happening because of the way ExpressionDecomposer() works, right?

I do not think that 9 || 1 gets evaluated first. It seems to me that doctest is expanding

CHECK(1==9 || 1==8);

as

ExpressionDecomposer() << 1==9 || 1==8;

Taking operator precedence, operator << it's the first to evaluate, producing an instance of Expression_lhs.
Then the first == is evaluated, and an instance of Result is generated. Call it "r".
So in the end, this is what the compiler sees:

r || 1==8;

As Result has a bool conversion operator that, in this case, gets evaluated to true, the whole expression is evaluated to true.

I see that this line

//template <typename R> STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison& operator||(const R&);

is commented out in the Result class. I imagine that in version 1.1 you are going to take out the comment and then the same result that in Catch will be obtained.

Ok, thank you for your great library.

@onqtam
Owner
onqtam commented Jul 28, 2016

actually you are right :D

@martinmoene martinmoene referenced this issue in martinmoene/lest Aug 7, 2016
Open

Introduce 'simplify expression' message? #37

@onqtam
Owner
onqtam commented Sep 13, 2016

I forgot this issue and I logged another one for the same thing. It should be done in the dev branch - only the reasonable expressions will be allowed and for anything else a static assert with a message will be used. Version 1.1 incoming in the next few days!

@onqtam onqtam closed this Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment