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

Add Branch Coverage data for ProfData coverage files #477

Merged
merged 8 commits into from
Mar 8, 2021

Conversation

hborawski
Copy link
Contributor

Using the lcov json data segments, populate branch_coverage_data for profdata coverage files

This allows the cobertura report to correctly calculate branch coverage

Also added branch coverage to the HTML output by using the segments to determine column ranges that were not hit, so null coalesecing operations, or other single line branches can be determined as missed coverage.

Please let me know if there are any updates required for this PR, it's been quite some time since using ruby, so I'm sure the code can be improved.

I made sure the existing tests pass, and i added some small profdata tests to check that the branch data populates as expected.

I've also tested this locally against a real project and compared output to Xcode, to see how it works on a larger codebase.

Example of a covered line with an uncovered branch:
Screen Shot 2021-02-23 at 2 52 52 PM

@coveralls
Copy link

coveralls commented Feb 23, 2021

Coverage Status

Coverage increased (+0.6%) to 96.176% when pulling d3c0ff6 on hborawski:profdata-branch-coverage into f5dfc24 on SlatherOrg:master.

Copy link
Contributor

@ksuther ksuther left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I made some incredibly nitpicky suggestions just to match the existing code style.

I'm by no means a Ruby pro either, so if anyone else has any suggestions feel free to chime in. Otherwise if it's working on your code base and the tests are passing I'm happy to merge this in and cut a release.

assets/slather.css Outdated Show resolved Hide resolved
lib/slather/coverage_service/html_output.rb Outdated Show resolved Hide resolved
lib/slather/coverage_service/html_output.rb Outdated Show resolved Hide resolved
lib/slather/coverage_service/html_output.rb Outdated Show resolved Hide resolved
lib/slather/coverage_service/html_output.rb Outdated Show resolved Hide resolved
lib/slather/profdata_coverage_file.rb Outdated Show resolved Hide resolved
lib/slather/profdata_coverage_file.rb Outdated Show resolved Hide resolved
lib/slather/project.rb Outdated Show resolved Hide resolved
lib/slather/project.rb Outdated Show resolved Hide resolved
lib/slather/project.rb Outdated Show resolved Hide resolved
@hborawski
Copy link
Contributor Author

Updated the code to use snake case for all the variables, didn't even realize the rest of the codebase was snake 🤦

@ksuther ksuther merged commit 743288b into SlatherOrg:master Mar 8, 2021
@ksuther
Copy link
Contributor

ksuther commented Mar 8, 2021

Merged, thanks for the contribution! I'll make a release to include this.

@hborawski
Copy link
Contributor Author

Happy to help, and excited to get all my projects updated with this!

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