pjcj / Devel--Cover

Code coverage metrics for Perl
http://www.pjcj.net/perl.html
93 stars 89 forks source link

Marking a covered line uncoverable can make it count as uncovered #323

Closed perlpunk closed 1 year ago

perlpunk commented 1 year ago

In certain cases, lines that are covered, but marked as uncoverable, are counted as uncovered statements, but still are marked as green in the HTML report. Here is an example (t/uncoverable.t):

#!/usr/bin/perl
use strict;
use warnings;
use Test::More tests => 1;

my $num = 1;
diag 'negative' if $num < 0; # uncoverable statement
pass 'end';

When I run this I get 91.5% for uncoverable.t:

PERL5OPT=-MDevel::Cover=-coverage,statement prove -l t/uncoverable.t
cover -report html_minimal cover_db
--------------- ------ ------
File              stmt  total
--------------- ------ ------
/usr/bin/prove   100.0  100.0
t/uncoverable.t   91.6   91.6
Total             96.1   96.1
--------------- ------ ------

When removing the uncoverable comment, I get 100% coverage. Instead adding # uncoverable statement count:2 before that line, I also get 100%. It seems to happen only when the line (or at least one of the statements on the line, if there are more) is actually covered.

perlpunk commented 1 year ago

I had a closer look into the code, and this seems to be the actual wanted behaviour. https://github.com/pjcj/Devel--Cover/blob/main/lib/Devel/Cover/Statement.pm#L22

Having a statement marked as uncoverable and reporting it covered might seem like one should get rid of the marker again, but in our case we have statements that are randomly covered due to timing issues. Also it's not really helpful because the report doesn't tell us which on line that statement actually is.

So I assume this was made as a feature, but I would like to ask if we can get rid of it because of the above reasons. Or run the report with a certain commandline option saying "don't complain about covered uncoverable statements".

Maybe we can shortly talk about it at the summit in 2 weeks.

pjcj commented 1 year ago

I think you've narrowed down the problem correctly here. One problem is that the html_minimal report doesn't manage this correctly, as you've seen. Another report such as text or html_basic would show you where the error is. This is one reason why I use the html_basic report in cpancover, for example. I'd like to change the default but wonder whether that would cause more problems for people.

I'll look into making this behaviour optional.

pjcj commented 1 year ago

I have added the concept of ignore_covered_err which says that any code which is covered cannot be an error, whether that code is marked as uncoverable or not.

There are two ways to activate this:

  1. Globally, by using the -ignore_covered_err option to cover
  2. Individually by adding class:ignore_covered_err to the appropriate uncoverable directive

This is included in release 1.40 - I hope it helps!

perlpunk commented 1 year ago

Awesome, thanks a lot! I will try it out hopefully tomorrow

perlpunk commented 1 year ago

I tried it out and found that the command line option does not seem to work. We have:

-MDevel::Cover=-db,/path/cover_db,-ignore,...,-coverage,statement,-ignore_covered_err

But the individual mark class:ignore_covered_err does work, and I think we might want to prefer this anyway to still be able to find places where we can remove the uncoverable mark. Thanks!

edit: oh wait, it's meant as a commandline option to the cover tool, not an option during running the test. nevermind =)

pjcj commented 1 year ago

Sorry for not being clear. Yes, it's a reporting option because it just changes the way the results are shown rather than the coverage data collected, though the distinction is not always obvious. Glad it's working and thatnks for walking me though why it's needed.