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

Improve coverage heuristic in function_body_lines #210

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Mar 7, 2019

Improve coverage heuristic in function_body_lines

The purpose of function_body_lines is to find functions which are "dead" (= never executed by test suite), and mark them as code, because Julia's built-in coverage code does not do that; if we were to rely on it, such functions would be marked as non-code, and thus they do not show up in the overall code coverage percentage.

But our current heuristic is too aggressive in so far as that Julia also sometimes marks code lines as "is not code" in the middle of functions that are being executed (most likely due to inlining and other optimizations). If we mark those lines as code, they will show up as uncovered, with no way for the package author to change this.

With this commit, we refine our heuristic: we only apply it to functions for which Julia recorded no coverage at all, as before; but if even just one line in the function body has coverage information, we do not touch the code lines in it at all.

Some care must be taken to make sure this works right for nested functions.

Closes #188

@codecov-io
Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #210 into master will increase coverage by 2.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   80.18%   82.31%   +2.13%     
==========================================
  Files           6        6              
  Lines         333      328       -5     
==========================================
+ Hits          267      270       +3     
+ Misses         66       58       -8
Impacted Files Coverage Δ
src/Coverage.jl 89.32% <100%> (+2.52%) ⬆️
src/parser.jl 82.14% <100%> (+2.14%) ⬆️
src/coveralls.jl 60.34% <0%> (+1.02%) ⬆️
src/lcov.jl 88.46% <0%> (+1.66%) ⬆️
src/codecovio.jl 93.54% <0%> (+4.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c31888...eae1476. Read the comment docs.

@fingolfin
Copy link
Member Author

Nice, this seems to work, and thus improves coverage even for Coverage.jl; but it's not yet "perfect", e.g. https://codecov.io/gh/JuliaCI/Coverage.jl/src/f06e6fe198fa14e71e92eabcba51da77f5c25644/src/lcov.jl#L72 still gets marked as code; I am not quite sure why.

So even with this, there is probably still some need for PR #208, but hopefully less so.

@fingolfin fingolfin force-pushed the mh/tweak-function_body_lines branch from f06e6fe to 8a105a7 Compare March 7, 2019 14:48
The purpose of function_body_lines is to find functions which are "dead"
(= never executed by test suite), and mark them as code, because Julia's
built-in coverage code does not do that; if we were to rely on it, such
functions would be marked as non-code, and thus they do not show up in
the overall code coverage percentage.

But our current heuristic is too aggressive in so far as that Julia also
sometimes marks code lines as "is not code" in the middle of functions
that are being executed (most likely due to inlining and other
optimizations). If we mark those lines as code, they will show up as
uncovered, with no way for the package author to change this.

With this commit, we refine our heuristic: we only apply it to functions
for which Julia recorded no coverage at all, as before; but if even just
one line in the function body has coverage information, we do not touch
the code lines in it at all.

Some care must be taken to make sure this works right for nested
functions.
@ararslan
Copy link
Member

ararslan commented Mar 7, 2019

I don't understand this part of the code well enough to provide a particularly meaningful review, but for what it's worth, nothing looks blaringly wrong to me. 😛 Perhaps @vtjnash would be able to provide more insight?

@fingolfin fingolfin requested a review from vtjnash March 7, 2019 20:51
@fingolfin
Copy link
Member Author

Of course if something is wrong (blaringly or not), I'll work on fixing it (or at worst, we can revert the change again).

@davidanthoff
Copy link
Contributor

Is there any hope that at some point julia will just output the correct info here, so that Coverage.jl doesn't have to patch things up? Is there a julia issue tracking this?

@fingolfin
Copy link
Member Author

@davidanthoff as discussed on issue #187, the relevant Julia issue is
JuliaLang/julia#28192

@fingolfin fingolfin merged commit 79b067e into JuliaCI:master Mar 8, 2019
@fingolfin fingolfin deleted the mh/tweak-function_body_lines branch March 8, 2019 08:14
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