Comment lines in unloaded files are marked as uncovered

[kmewhort]

In version 0.10, the additional lines on mult-iline statements, as well as comments, were ignored -- which seems like the correct behaviour. In version 0.11, these lines are marked as not covered.

Before and after:

[xaviershay]

yeah that's weird, I wouldn't have expected that to change. I'll have to go back through commits to figure it out.

[bf4]

@kmewhort Would you like to submit a failing test for us? It should be pretty easy to identify it with a simple git bisect.

Also, which Ruby are you on, and which profile/config are you running?

(Does changing the method invocation . to be trailing make a difference?)

[kmewhort]

I was having some trouble reproducing this in a simplified failure-case spec -- and it turns out the issue is a bit more subtle than the one I first stated. The actual issue is that files with 0% coverage report ALL lines as missed, not just the relevant lines.

In my particular case, my specs hit 100% coverage for file_a.rb, and simplecov correctly reports the relevant lines as hit (reporting nothing for the comment lines and multi-lines). Then, my cucumber test suite hits 0% coverage of file_a.rb, and -- problematically -- simplecov marks ALL the lines as missed, including the comment lines and multilines. The results I showed in the images above were actually the result of an lcov merge between these two test suites.

Looks like the likely culprit is 81e2431.

[xaviershay]

Thanks @kmewhort , the "double run" scenario sounds like enough for us to track it down. @hugopeixoto any thoughts on how we might address this?

[hugopeixoto]

If someone needs a quick workaround, setting track_files nil in their simplecov configuration code disables the unloaded files feature.

It seems that it is the merging process that's causing the problem. The coverage from each run probably look something like this:

spec: [nil, 1, 1, 1, nil, nil, nil, nil, nil, 1, nil]
cucumber: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

When merging these two coverage arrays, the nils have the lowest precedence, so the final coverage is

[0, 1, 1, 1, 0, 0, 0, 0, 0, 1, 0]

I'm not sure if there's a reason why nil is overridden. Since nil values represent uncoverable lines, they're always in the same place. Could this be changed? I changed merging to give nil the highest precedence, and the only tests that break are two in spec/merge_helpers_spec.rb that test explicitly for this.

An alternative would be to change the json format to explicitly mention unloaded files, but that might be a bit trickier.

I'm submitting a PR with a test case for this, and I'll add a commit with the nil precedence changed so you can try this out.

[kmewhort]

Merging {0,nil} to nil seems like maybe a bit of a workaround, without resolving the primary issue here. This would fix merges using the merge_helper...but not, for example, merges by an external merge tool.

I think the primary issue here is that a DA line in an lcov file is only supposed to report executable lines, but right now, for uncovered files, all the lines are being listed.

Is there any reason that the same filter used for covered files couldn't be run against these untracked files (so as to not report comment lines)? Would this be too much of a performance hit?

[hugopeixoto]

@kmewhort the problem is that there's no explicit filter.
It's the ruby interpreter that does all that work. simplecov just calls ruby coverage module, which returns the array filled with nils. If the file is not loaded, the coverage module will not know about it, and returns no information for that file.

[kmewhort]

Ah, I see. Darn, looks like there's no easy way to accurately report only the executable lines in these uncovered files then.

[ksmurphy]

I think I ran into this exact problem, and the fix was actually rather simple.

See this: #444

Disclaimer: I have not tested this except in my own environment and on my own project. My truth-table work gives me confidence that this fix indeed works, but I might be overlooking something.

[hugopeixoto]

@ksmurphy That sounds like what my proposed PR actually does.

[ksmurphy]

The original logic would overwrite a nil with a 0 unless both result arrays had nils, resulting in red. But since a nil signifies a no-op line, a nil should always be replaced with a nil. Thus changing '&&' to '||' fixed it for me.

I'm not sure what was in your PR, but did you make the same minor code change?

[rdunlop]

Is this affected by #441 ?

[PragTob]

Is this still an issue for anyone?

Need to retest this and see if it was/is fixed (which I hope)

[PragTob]

Closing due to missing feedback