[Perl-QA] Phalanx: What if full coverage isn't possible?

There's a whole set of these sort of problems.

sub new {
my $proto = shift;
my $class = ref $proto || $proto;

Devel::Cover wants you to consider the case where both ref $proto and $proto
are false. This is an (almost) impossible condition and certainly not an
interesting one to bother testing.

Then there are conditions that can't both occur in a single environment.

if( $^O eq 'MSWin32' ) {
do this;
}
else {
do that;
}

Unless you start combining the coverage runs from all hopolites. Which
might not be a bad thing. It certainly would help things like Class::DBI
which looks like it has piss poor coverage because it has so many optional
modules for testing.


Don't be mesmerized by 100% coverage. Devel::Cover isn't perfect and even
if it was there are uncoverable things. Even if you were to achieve 100%
coverage that doesn't guarantee anything. Coverage doesn't say anything
about the data being tested against, things going funny because the
environment changed, or an external utility (database).

Test coverage is a useful *heuristic* for test effectiveness. Like all
heuristics if you push it too far it falls apart. Get as close to 100% as
is useful and don't worry about the rest.

Indeed. Brian Marick wrote a great paper on this topic - "How to Misuse
Code Coverage", http://testing.com/writings/coverage.pdf.

* Pete Krawczyk [2004-07-09T13:10:52]
This came up just a few weeks ago, and I think the general feeling was
something like: cover everything, if possible; make everything
coverable, if practical; don't write stupid code to avoid "uncoverable"
situations.

So, if you know that branch isn't covered, but can't (really) be taken,
it's up to you whether that's a problem or not. Deming, Who Knows
Quality, would tell us to avoid arbitrary goals of 100% coverage...

There is some initial code in place in Devel::Cover to handle this
situation, but it is not well tested, not documented at all, and has no
(usable) interface. But you are welcome to play with it anyway ;-)

Should you wish to do so, I would suggest grepping he source for
"uncoverable", and looking at tests/uncoverable.t and
tests/.uncoverable. Also, google for perl-qa uncoverable to view
previous discussion of this topic.

.uncoverable is a file holding information about which parts of the
module are uncoverable and why. The format is lines of six columns:

1. md5sum of source file
2. type of coverage
3. line number
4. which criterion on that line (0 for first)
5. which part of the coverage (based on the internal representation)
6. reason

Michael Carman is looking at making this more usable. Columns one and
five are especially problematic at the moment. So for the time being
you might prefer to simply note which parts cannot be covered and why,
and later convert that to whatever system we ultimately use.

That's for you to decide. The lack of coverage serves to bring it to your
attention so you can ask (and answer) that very question. As others have said,
coverage is just a tool. The amount of coverage you get does not (by itself) say
anything at all about the correctness of your code.
I am, but I hit a roadblock with compound conditionals.

A little background for those unfamiliar with the guts of Devel::Cover: The
.uncoverable file captures analysis data at the same level of granularity as
Devel::Cover uses internally -- each individual operation.

e.g. the expression '$a && $b || $c' is actually seen as two completely separate
conditions:

L op R
-------------------------
1) $a && $b
2) $a && $b || $c

The truth table you see when you look at the condition coverage report (or
generate a "unified" report) is rather crudely and painfully calculated from
these primitives.

The problem is that for compound expressions, the unreachable paths are really
part of the composite expression and are best analyzed there. e.g. you can't
drive the "1 0 0" combination.

The "simple" solution is probably to add a new type of entry to .uncoverable for
composite expressions that tells us which row of a truth table can't be reached.

It occurred to me that if I'm missing a path in a composite expression I should
be missing coverage for one (or more) of the simple expressions that feed into
it. If I were really clever I could use that to propagate coverage analysis data
when building a truth table. (The reverse mapping -- from a truth table to the
primitive ops that build it -- is much harder.)

This got me thinking about something that had tickled my thoughts before. I'm
afraid that I have uncovered a serious flaw/limitation in Devel::Cover. Take the
expression '$a && $b || $c'
Its truth table is:

a b c | Z
---------
0) 0 X 0 | 0
1) 0 X 1 | 1
2) 1 0 0 | 0
3) 1 0 1 | 1
4) 1 1 X | 1

Suppose for the moment that all rows except row 2 (1 0 0) can be driven. The
following (contrived) example does this:

#!/usr/bin/perl
use strict;
use warnings;

for my $a (0 .. 1) {
for my $b (0 .. 1) {
for my $c (0 .. 1) {
foo($a, $b, $c);
}
}
}

sub foo {
my ($a, $b, $c) = @_;
$a = 0 unless ($b || $c); # Make '1 0 0' unreachable

print "$a $b $c ";
if ($a && $b || $c) {
print "T\n";
}
else {
print "F\n";
}
}

__END__

Now run that with -MDevel::Cover and generate a report. Shows full coverage,
doesn't it? The reason is that to Devel::Cover, the "if ($a && $b || $c)" line
has two *independent* conditions:

a b | Y Y c | Z
------- -------
0) 0 X | 0 0) 0 0 | 0
1) 1 0 | 0 1) 0 1 | 1
2) 1 1 | 1 2) 1 X | 1

Independently, all rows of both tables can be covered. What can't happen is
driving row 1 of table 1 at the same time as row 0 of table 2 (these are the
states that correspond to the "1 0 0" state in the composite table).

The truth table showing "1 0 0" as covered is my fault. I wrote the portion of
the reporting back end that builds the table from the simple operations. I
thought that I could calculate the coverage for a composite row from the
coverage of the simple rows that feed into it. I can see now that that
assumption was wrong.

Unfortunately, the information necessary to correct this isn't available.
Devel::Cover would have to either track condition coverage at an expression
level (instead of an operation level) or keep track of which combinations of
simple coverage were witnessed.

-mjc

{
local @INC=();
$@="";
eval { URI->new('badschme://') }
ok($!, "new dies when passed a badscheme and URI::_foreign is not"
."loadable");
}

Reached.

All unreachable code is either people misusing the term "unreachable", a
bug in Devel::Cover, or dead code that should be removed.

This is an example of the first, as is $class = ref($class) || $class.
Sure, it won't be reached by users doing sane things, but people do
insane things decently often. If the correct behavior in that case is
to die, make sure it dies. Notably, this code does not do the correct
thing:

package Foo;

sub new {
my $class=shift;
$class=ref($class)||$class;

bless [], $class;
}

eval { Foo::new(); }
is($!, "new dies when called as a function");

Of course, the author could make it do the right thing:

sub new {
my $class=shift;
$class=ref($class)||$class||__PACKAGE__;

bless [], $class;
}

Now coverage will be 100%, and the test will fail (bad test, or not
possible to write an omniscient test?). If coverage isn't 100% in this
case, then there's a bug in Devel::Cover (__PACKAGE__ should be
considered a constant).

An example of dead code that should be removed:

sub foo {
return 1 || foo();
}

...and even there, it's possible, in theory, for Devel::Cover to know
about that. In fact, the compiler will remove the foo() by itself, and
presumably Devel::Cover never notices the existence of the || in the source.
It may be possible to notice that you're running under Devel::Cover and
run the "look under every rock" tests only in that case. But if it's
your goal to cover everything and get that 100% there, then you need to
try harder before declaring something unreachable.

Note that this shouldn't be construed as saying that you should create
insane tests, just to get 100% coverage. My point is almost the exact
oppisate: that it's not reasonable to try for 100% conditional coverage.

-=- James Mastros

Actually this doesn't die, it does even worse, given this code:

package Foo;

sub new {
my $class=shift;
$class=ref($class)||$class;
bless [], $class;
}

package main;

my $foo = Foo::new();
print $foo;

The output is:

main=ARRAY(0x180035c)

The array gets blessed into the "main" package, which will of course
eventually die when you try and call a method from "main::" and it
can't find it.

I agree, 100% full coverage is many times an unreasonable goal.
Personally I am satisfied at 95% or above. That is not to say that I
wont try to squeeze out a little more coverage if I think it will
actually be useful to the tests. But if you are just covering to cover,
you are not necessarily improving the quality of the tests, which in
the end is the ultimate goal.

Steve

Here's a puzzle, then.

I just ran into a similar "problem" in POE::Driver::SysRW. For
portability I have a couple lines similar to

$! = 0 if $! == EAGAIN or $! == EWOULDBLOCK;

EAGAIN and EWOULDBLOCK are identical on most systems. In fact, one is
usually defined in terms of the other. They differ on a few platforms,
however, and it's important to check both.

The expression boils down to this on my test system:

A B dec $! == 35 or $! == 35
0 0 0 (green, tested)
0 1 0 (red, impossible to test)
1 X 1 (green, tested)
While I'd like 100% coverage, I've decided to live with 98.3% because of
these untestable (at least on my machine) conditions. It's nice to have
the red flags pointing at some silly code, and maybe an improvement will
come later.

I don't advocate that the improvement appear in Perl or Devel::Cover.
On the contrary, logic errors like this should be pointed out to make
better code.

Redefine EAGAIN and EWOULDBLOCK (they're just perl constants), and rerun
that code, after setting $!. (Your code probably isn't written so that
is in one callable place, but it could be.)

This is probably a good example of where it's too silly to force it to
test all the possibilities, but it is possible.

You could also consider it a bug in perl's optimizer, which should
notice that $a||$a == $a. (But you'd be wrong, as this isn't
neccessarly true if $a is magical, and $! is, in fact, magical.)

-=- James Mastros

I don't think it's silly at all - if "it's important to check both" then you
would want to have tests that cover situations which can occur on platforms
other than your own, otherwise you can't really be sure that you have
provided the logic you seek.

as you mention, local-style redefinition is just the solution for code like
this - I, for one, always get confused about operator precidence in cases
just slightly more complex than the one above, and prefer to use tests (and
parens :) to make sure I got it right.

--Geoff