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

fix(coverage): Do not emit error on BRDA:0 entries #417

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

cwahbong
Copy link
Contributor

Coverage report generators in Coverage.py would generate BRDA:0 for exit branches.

Ref: https://github.com/nedbat/coveragepy/blob/5467e1f/coverage/lcovreport.py#L108-L112

Also small string fixes for error messages in BRDA.

Improves #362.

@cwahbong
Copy link
Contributor Author

I just saw other discussions about the misuse of the line number fields. Is it making sense for being tolerant the negative numbers or simply drop the invalid entries and maybe put a warning message somewhere, either pop up or on output window (I did not find the output window like in other plugin for this one) in case other coverage tools also misuse the line number field?

Coverage report generators in Coverage.py would generate BRDA:0 for
exit branches.

Ref: https://github.com/nedbat/coveragepy/blob/5467e1f/coverage/lcovreport.py#L108-L112

Also small string fixes for error messages in BRDA.

Improves bazel-contrib#362.
@vogelsgesang
Copy link
Collaborator

Is it making sense for being tolerant the negative numbers or simply drop the invalid entries and maybe put a warning message somewhere

Maybe. Let's keep it as is, and then potentially ignore also negative numbers in case we actually get reports of this being a real-world issue. (I prefer "fail early, fail hard" since this makes it much simpler to find potential issues/unexpected behavior).

@vogelsgesang vogelsgesang merged commit 7f62f0e into bazel-contrib:master Oct 16, 2024
1 check passed
@cwahbong cwahbong deleted the fix-coverage-brda branch October 20, 2024 12:50
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.

2 participants