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

Show Branch Coverage in stdout if enabled. #117

Closed
wants to merge 2 commits into from

Conversation

joshuapinter
Copy link

@joshuapinter joshuapinter commented Mar 3, 2022

Prior to this only Line Coverage is shown in the stdout.

Branch coverage is equally important, when it is enabled, so it should be shown alongside it to ensure adequate coverage of the code base.

Now it shows in a clean format, including covered branches, total branches and percent of covered branches. I also moved the percentages first and the coverage and total lines/branches afterwards in brackets. So it looks something like this:

Coverage report generated for (1/10), (10/10), (2/10), (3/10), (4/10), (5/10), (6/10), (7/10), (8/10), (9/10) to /Users/me/Development/cntral/coverage.
Line Coverage: 58.44% (7554 / 12927)
Branch Coverage: 50.43% (1868 / 3704)

When branch coverage is not enabled it simply omits that line, so it appears like this:

Coverage report generated for (1/10), (10/10), (2/10), (3/10), (4/10), (5/10), (6/10), (7/10), (8/10), (9/10) to /Users/me/Development/cntral/coverage.
Line Coverage: 58.44% (7554 / 12927)

@jeremyevans
Copy link
Contributor

This is basically the same pull request as #103, except it is a larger change to the output format. Personally, I'm fine with either pull request. Anything that gets the branch coverage output to stdout would be an improvement over the current behavior, which forces users to check the coverage html file to find the branch coverage.

@joshuapinter
Copy link
Author

Agreed. I didn't see that PR before this, otherwise, I would have bumped it up.

Personally, I prefer the organized layout of this PR with Line Coverage and Branch Coverage on separate lines. Not sure about #103, but we've been using this PR/fork in production for a few months now without any issue.

@joshuapinter
Copy link
Author

Any chance we can get this merged in? We've been using it in our CI for about a year with great results. To reiterate, I think it's a cleaner solution than the other PR that tries to accomplish the same thing. Thanks.

Prior to this only Line Coverage is shown in the stdout. 

Branch coverage is equally important, when it is enabled, so it should be shown alongside it to ensure adequate coverage of the code base.

Now it shows in a clean format, including covered branches, total branches and percent of covered branches. Something like this: 

```
Coverage report generated for (1/10), (10/10), (2/10), (3/10), (4/10), (5/10), (6/10), (7/10), (8/10), (9/10) to /Users/me/Development/cntral/coverage.
Line Coverage: 7554 / 12927 (58.44%)
Branch Coverage: 1868 / 3704 (50.43%)
```

When branch coverage is not enabled it simply omits that line, so it appears like this: 

```
Coverage report generated for (1/10), (10/10), (2/10), (3/10), (4/10), (5/10), (6/10), (7/10), (8/10), (9/10) to /Users/me/Development/cntral/coverage.
Line Coverage: 7554 / 12927 (58.44%)
```
And then covered and total counts in brackets afterwards. Percentage is most critical at a quick glance.
@amatsuda
Copy link
Member

@joshuapinter Just merged #103 which is very similar to this patch but without the "percentage first" change, because I'm trying not to make any design change which I have never done for this project. Please make another pull request for the second commit which is a separate topic from branch coverage, and get approval from other members.

Also, it seems that output_message method has to return output at the bottom of the method, or the message is going to be nil when branchable_result? is falsy.

Thank you for your patch and your patience!

@amatsuda amatsuda closed this Oct 29, 2023
@joshuapinter
Copy link
Author

@amatsuda No worries and thank you for merging that in. We can start using the main branch again.

I'll review the code to check for the issue you pointed out and do the format change as a separate PR (if I think it's still worthwhile).

Thanks again. 🙏

@joshuapinter
Copy link
Author

Hi @amatsuda, I've revised the code and made a new pull request for the formatting change here:

#125

You were right, too, I needed to put output at the end to ensure it was returned from the method if branch overage was not enabled. Doh.

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.

None yet

3 participants