Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve EXPECT_XXX and ASSERT_XXX output in case of failure. #2330

Conversation

digit-google
Copy link
Contributor

Ensure that binary comparisons (e.g. EXPECT_EQ) display the actual values of the left and right side operands, and not just their source expression, as in:

Failure on file.cc:10:  expected != actual
Left:  10
Right: 20

Also ensure that boolean comparisons (e.g. EXPECT_TRUE) display the expected and actual values too as in:

Failure on file.cc:10: vec.size()
Expected: false
Actual:   1

This uses a small amount of C++11 SFINAE, without needing to change any of the test macro invocations. If a value is not supported, then "" will be used instead.

@digit-google digit-google force-pushed the better-test-failure-outputs branch 2 times, most recently from b1fb7bb to d2c0dd1 Compare September 22, 2023 15:38
@digit-google
Copy link
Contributor Author

Oh, note that the AppVeyor build fails because of the <atlcore.h> include which is fixed by PR #2329 , and has nothing to do with this patch.

@digit-google digit-google force-pushed the better-test-failure-outputs branch 2 times, most recently from ac3b944 to 7b1d430 Compare October 1, 2023 16:07
@digit-google
Copy link
Contributor Author

I had to change the patch after realizing that the expressions were evaluated twice in case of failure, which would result in incorrect runtime behavior. For example, consider:

EXPECT_EQ(0, fclose(file))

Which does happen in the source code, the fclose() would be called twice. I tried solving this by using lambdas to capture the value once with a const reference.

Ensure that binary comparisons (e.g. EXPECT_EQ) display the
actual values of the left and right side operands, and not just
their source expression, as in:

```
// For failed EXPECT_EQ(expected, actual)
Failure on file.cc:10:  expected != actual
Left:  10
Right: 20
```

Also ensure that boolean comparisons (e.g. EXPECT_TRUE) display
the expected and actual values too as in:

```
// For failed EXPECT_FALSE(vec.size())
Failure on file.cc:10: vec.size()
Expected: false
Actual:   1
```

This uses a small amount of C++11 SFINAE, without needing
to change any of the test macro invocations. If a value is
not supported, then "<UNPRINTABLE>" will be used instead.

Note that pointer comparisons with NULL or 0 will now
create compiler warnings or errors. So introduce new
EXPECT_NULL() and EXPECT_NO_NULL() macros to deal with
these. EXPECT_TRUE() and EXPECT_FALSE() still work
properly with pointers though, but the error message
for EXPECT_NULL() / EXPECT_NOT_NULL() is slightly
clearer.
@digit-google
Copy link
Contributor Author

Mmm, there are too many subtle compilation issues here. I', going to close this PR for now until I can weed out everything, if that is possible. Sorry about the noise.

@jhasse
Copy link
Collaborator

jhasse commented Oct 5, 2023

IMHO #1562 is the way forward.

@digit-google
Copy link
Contributor Author

Thanks, I was not aware that this was still a thing. The latest version of my branch at digit-google@5e6a105 passes all tests now. I'll keep it as a local change in our own Ninja version for now.

Do you know if anyone is working on #1562 at the moment?

@jhasse
Copy link
Collaborator

jhasse commented Oct 5, 2023

I would continue working on it when #1583 gets fixed. No one is working on that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants