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

Use console reporter's totals summary for compact reporter #2554

Merged
merged 1 commit into from Oct 25, 2022

Conversation

psalz
Copy link
Contributor

@psalz psalz commented Oct 24, 2022

This changes the compact reporter's summary of test run totals to use the same format as the console reporter. This means that while output is no longer on a single line (two instead), it now includes totals for failedButOk test cases and assertions, which were previously missing.

Fixes #878.

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #2554 (0dea420) into devel (a43f679) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 0dea420 differs from pull request most recent head 2e66a84. Consider uploading reports for the commit 2e66a84 to get more accurate results

@@            Coverage Diff             @@
##            devel    #2554      +/-   ##
==========================================
+ Coverage   91.15%   91.30%   +0.15%     
==========================================
  Files         185      185              
  Lines        7614     7596      -18     
==========================================
- Hits         6940     6935       -5     
+ Misses        674      661      -13     

std::size_t row,
std::ostream& stream,
ColourImpl& colour ) {
for ( auto col : cols ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a copy? From a quick glance I would say no, the code does not modify the SummaryColumn here.

(I know it is like this in the original code, but I am looking through it again here and hoo boy, it is a mess)

@horenmar
Copy link
Member

Looks good, with two drive-by fixes since you are already touching that code. Honestly you can ignore them if you want 🤷

@horenmar horenmar added the Tweak label Oct 24, 2022
@psalz
Copy link
Contributor Author

psalz commented Oct 25, 2022

Sure, no problem. I did a refactoring pass as a fixup commit, will squash once approved.


Speaking of messes, I originally planned to do a larger refactoring of how [!shouldfail] and [!mayfail] are being handled, as there are a few things that have bothered me for a while (e.g. the way a passing [!shouldfail] test case is "reported"), but I gave up after a couple of hours because I found the whole logic to be very confusing. For example that a failed assertion in a [!shouldfail] is not isOk because it doesn't have ResultDisposition::SuppressFail (which only seems to be used for CHECKED_IF/ELSE), or whatever is happening here. If I ever find some time I would like to revisit this again, though.

Copy link
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@horenmar
Copy link
Member

Yeah, shouldfail/mayfail is also a "fun" mess. It is what happens when the code is evolved over time for 10+ years 😃

This changes the compact reporter's summary of test run totals to use
the same format as the console reporter. This means that while output is
no longer on a single line (two instead), it now includes totals for
`failedButOk` test cases and assertions, which were previously missing.
@psalz psalz force-pushed the improve-compact-shouldfail-reporting branch from 0dea420 to 2e66a84 Compare October 25, 2022 15:46
@horenmar horenmar merged commit 6185d0c into catchorg:devel Oct 25, 2022
@psalz psalz deleted the improve-compact-shouldfail-reporting branch October 27, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compact reporter does not handle [!shouldfail] properly
2 participants