Coverage information does not seem to be right

[ronisbr]

Hi guys!

After the new version of Coverage, the coverage information does not seem to be right. Take a look, for example, at the function compose_rotation here:

https://coveralls.io/builds/19787299/source?filename=src/compose_rotations.jl#L53

It says that it is not tested at all. However, take a look at the test here:

https://github.com/JuliaSpace/ReferenceFrameRotations.jl/blob/6fd8162b51958ed809fe36c268eb11bd0a506a43/test/test_compose_rotations_quaternions.jl#L49

The last version of Coverage was saying that ReferenceFrameRotations.jl has a coverage of 100% and now it is 74%.

Notice that the same thing happened with Codecov.

[ronisbr]

By the way, I am also wondering why it is marking this line as it was not executed:

https://codecov.io/gh/JuliaSpace/ReferenceFrameRotations.jl/src/master/src/DCM.jl#L345

whereas the surrounding lines were executed.

[ronisbr]

Using --inline=no seems to fix the problem. However, the tests take ages to complete. Is there any other workaround?

[ronisbr]

And I could not use --inline=no in Travis. It is working only on local analysis. The command I used was:

julia --check-bounds=yes --color=yes --inline=no -e "VERSION >= v\"0.7.0-DEV.5183\" && using Pkg; Pkg.test(\"ReferenceFrameRotations\", coverage=true)"

[ararslan]

Sounds like JuliaLang/julia#28192

[ronisbr]

@ararslan yes, maybe. However, I started to see the problem today. The last coverage result, which was 2 days ago, was working perfectly.

[ararslan]

cc @fingolfin

[ararslan]

Yeah looking around at various PRs with coverage integration, I think we're going to need to revert the latest changes to this package unless we can come up with a quick fix. Something is very wrong.

[fingolfin]

The old coverage was "accidentally correct". The problem is that due to a bug in Coverage.jl, it reported lines that do not contain code, and lines that are not covered, the same way. You can see that in e.g. https://coveralls.io/builds/18398285/source?filename=src/compose_rotations.jl only one line is marked green, but no lines red; so that means it thinks that exactly that one line contains code. Which is of course wrong.

When I fixed this, the other lines of code started to be marked as "is code". But Julia still reports that there is no coverage for them, so now they get marked as "uncovered code".

That's unfortunate, but other than either enabling --inline=no, the only other fix I can think of is to improve how Julia reports code coverage. Specifically, it could try to attribute coverage to inlined functions (I think for C/C++, GCC somehow manages that quite well).

I do not see any way JuliaCoverage.jl can help with that, other than reverting the fix and returning to broken coverage reporting. Which will result in great coverage scores for everybody, with a lot of uncovered code not reported as such. I.e., pick what kind of error you prefer, first or second order :/.

[fingolfin]

@ararslan I am not sure anything is wrong in Coverage.jl, other than it not hiding the effects of JuliaLang/julia#28192 anymore.

That said, one could of course try to mixup things further, and e.g. not report macros as "code"; then the example @ronisbr reports for src/compose_rotations.jl would be "fixed". But not https://codecov.io/gh/JuliaSpace/ReferenceFrameRotations.jl/src/master/src/DCM.jl#L345

[ronisbr]

I understood, thanks @fingolfin . Indeed, running with --inline=no fixed the problem. However, I was not able to run this no Travis. When I tried to enable, it made no difference.

[fingolfin]

As a matter of fact, I wonder if any project using Coverage.jl with Julia 0.7 or 1.0 had a coverage score other than 100% prior to my fixes; I don't see how, at least. While that looks great, it is also completely useless...

[ronisbr]

I see. The local runs with inline=no showed 98% of coverage. I just need then to make this work in Travis somehow... ideas?

[fingolfin]

What did you try? I am looking at https://github.com/JuliaSpace/ReferenceFrameRotations.jl/blob/master/.travis.yml now. I assume you edited the four calls to Julia there to insert --inline=no? And that then did not help (tested how? via a PR to your own project?)

[KristofferC]

Currently, Pkg would have to be tweaked to run the tests with --inline=no.

[ronisbr]

Hi @fingolfin,

I modified the script in travis to execute the following command in my project:

julia --check-bounds=yes --color=yes --inline=no -e "VERSION >= v\"0.7.0-DEV.5183\" && using Pkg; Pkg.test(\"ReferenceFrameRotations\", coverage=true)"

I have reverted the commit because I saw no difference.

@KristofferC,

I understood that something inside Pkg must change to fix this. I have two questions:

1. Is there any workaround, since it works locally?
2. Should I open an issue at Julia project?

[KristofferC]

No super good workaround at the moment.

Please open an issue at Pkg.jl.

[ronisbr]

Done, should we close this one since it is not related to this package?

[tkoolen]

While I get the sentiment, I personally prefer accidentally correct over way off until the real issue is fixed in Base. It wasn't perfect, but it certainly was closer to the truth.

[fingolfin]

But always reporting 100% is only closer to the truth if you already know your coverage is above 50%. I'd also argue that it is always useless. You might as well turn off coverage completely.

[fingolfin]

I started a PR for Pkg here: JuliaLang/Pkg.jl#866

[tkoolen]

Thank you for the Pkg PR and also your efforts in maintaining this package.

Coverage 0.6.0 was not always reporting 100% for me, or for many other packages. Check out e.g.

https://codecov.io/github/JuliaArrays/StaticArrays.jl?branch=master

I'm just saying that stuff like https://codecov.io/gh/JuliaArrays/StaticArrays.jl/src/1683f798e0dcb8725768407053a45ae48434fed7/src/eigen.jl#L39 makes me completely distrust the current results. Let's see how much not inlining helps. I'm certainly not saying that the situation before was good though.

[ronisbr]

For ReferenceFrameRotations, when I tested the coverage off-line using inline=no, the results seem much more accurate. I could check this because the functions and the tests are simple.

[sbromberger]

It'd be great if some of you could test if this works for you as intended

ref: sbromberger/LightGraphs.jl#1163

[fingolfin]

I forgot to mention: of course to test with DISABLE_AMEND_COVERAGE_FROM_SRC right now, you'll have to make sure that my mh/DISABLE_AMEND_COVERAGE_FROM_SRC branch of Coverage.jl is installed, not that last release version, which of course does not yet have this experimental change... I'll try to either provide instructions for that, or perhaps we can just make a release with DISABLE_AMEND_COVERAGE_FROM_SRC in it, to make it easier for people to test this... @ararslan thoughts?

[ararslan]

Having the change on master with some documentation and a note clearly stating that it's experimental seems like a reasonable course of action to me.

[bramtayl]

set to yes to disable the heuristic which marks additional lines as code. While I still think this is problematic on its own

Can you give more details on this, @fingolfin ?

[fingolfin]

@bramtayl details on what? on why the heuristic is needed (and thus why disabling it is problematic)? This was extensively discussed in this issue already, and also in several other issues and PRs. In addition, I've added something about it to the README in PR #208.

[fingolfin]

I have just released Coverage.jl version 0.9.0. I hope many of you will see some improvement in coverage thanks to PR #210. I urge you to try that first. If you are still unhappy, you can try the DISABLE_AMEND_COVERAGE_FROM_SRC env var described above (and also described in README.md), to see if helps; but I'll repeat once more that this also risks missing completely uncovered functions, as those won't be counted towards coverage anymore.

[ronisbr]

@fingolfin Thanks! It is indeed much better on my tests.

[sbromberger]

yup. we're back up to 99.11% coverage now. Thanks!

[fingolfin]

Glad to hear this. And @sbromberger tested DISABLE_AMEND_COVERAGE_FROM_SRC, and you can see the bad thing that happens if our workaround heuristic is completely disabled using DISABLE_AMEND_COVERAGE_FROM_SRC: the coverage percentage goes up, because unused functions are ignored. Example: https://codecov.io/gh/JuliaGraphs/LightGraphs.jl/src/c376fd3c947b41f1eea0315a05fff6c069711cd0/src/Parallel/shortestpaths/johnson.jl#L40>

Without the option, and just Coverage.jl 0.9.0, the function is correctly marked as code that is not executed: https://codecov.io/gh/JuliaGraphs/LightGraphs.jl/src/master/src/Parallel/shortestpaths/johnson.jl#L40

[fingolfin]

I think we could close this now... @ararslan ?

[sbromberger]

Just a final question: what should we expect if we turn off DISABLE_AMEND_COVERAGE_FROM_SRC ?

[fingolfin]

I am not sure I understand the question... turn off the DISABLE flag? As in, not use it? And what do you want to know about that?!

[sbromberger]

ah, never mind. We tried both ways in LG and I concluded that we're going to not include the DISABLE flag. We get accurate and good results without it.

[ararslan]

I think we could close this now... @ararslan ?

I think you're in the best position to determine whether this is sufficiently resolved, so I trust your judgement on whether this should be closed.

[fingolfin]

I'll close it then; but of course anybody is free to open a new issue if they still see problems anywhere

[adamglos92]

I have still one small issue: one line (which is uncovered and next to another uncovered line) seems to be not considered, i.e. in coveralls it is not marked.
https://coveralls.io/builds/22042972/source?filename=src/qwdynamics/qwsearch/maximizing_function.jl#L65

[adamglos92]

Similarly one line which should be marked as covered is not marked
https://coveralls.io/builds/22042972/source?filename=src/qwmodels/szegedy/szegedy_stochastic.jl#L68

[fingolfin]

@adamglos92 Are you sure that line 65 in your first example is executed? Have you verified it, e.g. by printing or throwing an exception there? As to the second example: I only see a line of code that is not marked as code, so neither marked as covered nor as uncovered. The cause of this has been discussed extensively in this PR, and also is discussed in the README.md section on DISABLE_AMEND_COVERAGE_FROM_SRC.

[adamglos92]

@fingolfin It is not, but I think I get the idea now. So if in case there is multiple lines weren't executed, it is not promised that each is marked as uncovered. Can I expect that at least one (as in my case) is marked as uncovered?
Please ignore second example, the README.md clarifies it.