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

RFT (request for tips): tweaking .cov files, aka code_coverage output #7541

Closed
timholy opened this issue Jul 7, 2014 · 13 comments
Closed

RFT (request for tips): tweaking .cov files, aka code_coverage output #7541

timholy opened this issue Jul 7, 2014 · 13 comments

Comments

@timholy
Copy link
Member

timholy commented Jul 7, 2014

In these issues:
JuliaCI/Coverage.jl#4
JuliaCI/Coverage.jl#11
it was noticed that it's difficult to accurately calculate coverage, given the format of the .cov files. @IainNZ suggested that this is best fixed in julia, since its parsing is definitive.

I have a general idea of how to fix this, but I could use some help with the details:

  1. when code is expanded, for each file, create a list storing
struct {
    int firstlinenumber;
    int lastlinenumber;
    int iscompiled;
}

for each method in the file. The initial setting for iscompiled is false. Methods are stored in the order of appearance in the file.
2. Have coverageVisitLine, which gets called when functions are compiled, set iscompiled to true
3. In jl_write_coverage_data, when you get to the first line of the next method in the list, check iscompiled. If false, prepend 0 instead of - for each line of the function. This will even penalize comments, end statements, etc, but I think it's better to err in this direction---it's extra incentive to make sure all functions are tested.

The part I'm stuck on is finding the best place for step 1. My favorite candidate is here, but I'm not at all sure about this.

@mbauman
Copy link
Member

mbauman commented Jul 9, 2014

Another big part of the overestimation comes from the often-used x==1 && do_something() idiom. Perhaps short-circuiting lines should only count if you get to the end of the circuit?

@timholy
Copy link
Member Author

timholy commented Jul 9, 2014

True. Though when it's i < n || throw(BoundsError()) I'm not sure there's any reason to be particularly interested in triggering the BoundsError.

@hayd
Copy link
Member

hayd commented Jul 9, 2014

Similarly for ternary operator x == 1 ? do_something : do_something_else.

...you may want to check that the BoundsError is correctly triggered.

@StefanKarpinski
Copy link
Member

It seems to me that the most interesting coverage question is of all the branch points in your program, how many of the possible branches do you take? I'm not sure if this is equivalent to fraction of basic blocks covered. At that point, you can consider 2-grams of basic blocks, 3-grams of basic blocks, etc.

@timholy
Copy link
Member Author

timholy commented Jul 9, 2014

The branches question is indeed relevant, but we shouldn't let ourselves get distracted by it: browsing through the *.cov files, by far the bigger issue is the fact that uncompiled functions don't get counted against the coverage fraction. Untested lines should be shown in red, and "irrelevant" lines (comments, etc) are uncolored. The biggest problem is determining which lines are relevant. For example, here are two files that are (laughably) listed as having 100% coverage: https://coveralls.io/files/238719979, https://coveralls.io/files/238719982.

You can see this result again and again throughout the complete results.

@simonster
Copy link
Member

How will we stop functions that are tested but always inlined from counting against the coverage percentage?

@timholy
Copy link
Member Author

timholy commented Jul 9, 2014

Disable inlining when performing --code-coverage? #7464 (comment)

@JeffBezanson
Copy link
Member

Disabling inlining is really not realistic.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2014

If one cares, then I suppose the alternative would be to have an analysis tool that flags all functions that will be inlined. But if this is the only application, that seems a little excessive---this is just supposed to be a guide for helping people write more comprehensive tests, and that is something that already requires a little intelligence. I care less about what the actual coverage number is.

@mlhetland
Copy link
Contributor

If we do want to add branch coverage, Ned Batchelder's much-used coverage.py might be of interest. (I've had some issues with it complaining of uncovered branches that aren't really “there”—stuff that couldn't have been reached anyway. But that's why he has the # pragma comments.)

As for the inlining issue: It seems to me that that could be important. If one wants 100% coverage (which could be useful, if one wants to add a test for it, for example—or we want a command-line switch for ensuring 100% coverage, like dlang), the plethora of tiny functions that seem idiomatic to Julia would seriously mess with that, no? If disabling inlining is unrealistic—is it possible to record where a function is inlined, and work with that?

Or … if functions are going to be inlined, if we know that when we compile them (as in @timholy's suggestion, here), could we not just use that as a reason to not flag them as compiled? In a sense, they haven't been compiled as individual functions (from the perspective of coverage) anyway. Then we will "sort of" get 100% coverage if and only if we have executed all the code, including the inlined functions. (Not talking about branch coverage here.) The only issue is that we won't be able to count individual lines in the inlined functions, but that's perhaps less crucial, if they're going to be tiny anyway, in general?

@timholy
Copy link
Member Author

timholy commented Aug 8, 2014

Currently the - that gets written in front of the line means, as far as Coveralls.jl is concerned, "this line doesn't count." So a function that is always inlined won't prevent you from reaching something that Coveralls thinks is 100% coverage. The only lines that count against your total are those with a 0 out front. So currently I'd guess that the number reported for most packages are inflated, and the number reporting something > 0 is an underestimate.

Besides inlining, the other big ticket item is precompilation, which currently has the same effect. You can work around this by deleting sys.so before you run, but a better solution would be for --code-coverage=all and --track-allocation=all to temporarily disable it. A few minutes spent searching through the code did not reveal to me how that file gets loaded, so ATM I haven't figured out how one might go about doing that.

timholy referenced this issue in JuliaAttic/Color.jl Aug 27, 2014
@timholy
Copy link
Member Author

timholy commented Oct 23, 2014

For the inlining problem, perhaps we could have inference.jl tag that struct I mentioned up top.

@timholy
Copy link
Member Author

timholy commented Jan 6, 2015

In combination with other work in base (--inline=no, fixing the detection of base vs user code) and JuliaCI/Coverage.jl#36, this seems to basically be done.

@timholy timholy closed this as completed Jan 6, 2015
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

No branches or pull requests

7 participants