Skip to content

Conversation

mmatrosov
Copy link

@mmatrosov mmatrosov commented Jan 2, 2022

Description

The default console reporter is a good choice when tests are fast and quiet. In many projects that I worked with neither is true. A test might take up to several seconds and the component under test might log something to console. The default reporter does not work good in these cases. Either you cannot understand what is being run now, or the output is cluttered. This desire manifested itself in #1579. Also, seems like the default reporter cares too much about assertions - how many passed, how many failed. However, I've never felt any need in this info in my practice.

All these problems do not apply to the output of Google Test. That's why I decided to mimic its behavior in a new gtest reporter. Please see changes in reporters.md for details.

That's how the output looks for SelfTest:

The initial code for catch_reporter_gtest.cpp was copy-pasted from catch_reporter_console.cpp. They don't have much code duplication in HEAD though. You can check out the diff for the file excluding the first commit to see the duplicated code.

This PR is for v2.x branch, because I don't understand the state of v3. There are no stable releases for it, the last releaes was more than a year ago, and it is not available in conan-center. But I want to use this feature in the nearest future, that's why I am targeting v2.

GitHub Issues

Should solve #1579.

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #2343 (08bf579) into v2.x (e9e4117) will decrease coverage by 2.36%.
The diff coverage is 38.91%.

@@            Coverage Diff             @@
##             v2.x    #2343      +/-   ##
==========================================
- Coverage   90.10%   87.74%   -2.36%     
==========================================
  Files         113      114       +1     
  Lines        5038     5178     +140     
==========================================
+ Hits         4539     4543       +4     
- Misses        499      635     +136     

@mmatrosov
Copy link
Author

mmatrosov commented Jan 3, 2022

I am not sure how to resolve the coverage problems. Should I include the new reporter in the test? If yes, how do I do it?

If I don't add the new reporter to the test I can fix the "project" check by adding some tests for catch_string_manip.cpp, but I won't be able to fix the "patch" check I suppose.

@mmatrosov
Copy link
Author

@horenmar could you please take a look? Seems like the run of ctest should produce all the coverage results, but when I run it locally, it only uses the default and the "compact" reporters, even though I see how all existing reporters are covered in codecov: https://app.codecov.io/gh/catchorg/Catch2/compare/2343/tree/include/reporters
But I don't get how coverage for other reporters is collected.

@horenmar
Copy link
Member

Thanks for the effort, but I do not want to merge gtest format reporter. Nowadays I only want to merge reporters that implement a well documented format used by some other tools.

As to the coverage, reporters are primarily tested through ApprovalTests, which are implemented in scripts/approvalTests.py, and they have to be added manually.

@horenmar horenmar closed this Jan 29, 2022
@sketch34
Copy link

The default reporter does not work good in these cases. Either you cannot understand what is being run now, or the output is cluttered.

Agreed. I am running into this problem with two of my current projects as well.

@mmatrosov
Copy link
Author

Yeah. I am disappointed this change was not merged. We maintain it as a patch in our custom conan recipe for catch2.

@dengchj
Copy link

dengchj commented Aug 23, 2023

Yeah. I am disappointed this change was not merged. We maintain it as a patch in our custom conan recipe for catch2.

You shouldn't have just called gtest. If you change the name but the implementation is similar, it will probably be accepted :)

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.

4 participants