Re: Phalanx: What if full coverage isn't possible?

[James Mastros]

Pete Krawczyk wrote:
Consider the following code:
    $impclass ||= implementor($scheme) ||
        do {
            require URI::_foreign;
            $impclass = 'URI::_foreign';
        };
That's in URI.pm, lines 54-58.
Devel::Cover treats that as a conditional.  So short of deleting
URI::_foreign, that do BLOCK is never going to return false.
{
  local @INC=();
  [EMAIL PROTECTED]"";
  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.

Is that a bug, then?  Or is it something else?  And how should I notate
that, keeping in mind the goals of Phalanx, so that it's clearly visible
that no test of that condition's failure can truly take place in a regular
"make test" run?
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