http://qs1969.pair.com?node_id=606564

I was going over our code coverage reports at work and noticed that if the following snippet of code was fully covered, we'd have 100% coverage for one of our modules (with the awkward name of SQL::Stripper):

if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) { ... }

In this case, I noticed the following for the SQL::Stripper code coverage:

    line !l  l&&!r  l&&r  condition
    94   51  0      2     @chunks > 1 and $chunks-2 =~ s/$comment$//

The number indicates how many times each condition is exercised. The exact code is this:

    @chunks > 1 && $chunks[-2] =~ s/$comment$//

(Note that code as Devel::Cover reports it is often subtly different, but semantically equivalent, because Devel::Cover runs off of the opcodes).

That code is more or less equivalent to:

   $left && $right

There are only three possibly paths through that (note the headings in the Devel::Cover output I listed above):

$left is false
!l
$left is true, $right is $false
l&&!r
$left and $right are $true
l&&r

Because of the way the SQL::Stripper code is structured, if $left is true, $right must be true because $right will always have a $comment to remove from the @chunks element. Thus, we have a code coverage condition that cannot be exercised (l&&!r), so the actual full snippet:

if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) { pop @chunks; }

Might be better written as this:

if ( @chunks > 1 ) { $chunks[-2] =~ s/$comment$//; # XXX remove comment indicator pop @chunks; # XXX remove the comment }

That will make the code coverage tests report 100% for SQL::Stripper so I made the change. Then I realized the problem. Prior to that snippet is the following code:

my $split = Data::Record->new( { split => $comment, unless => $RE{quoted}, chomp => 0, trim => 1, } ); my @chunks = $split->records($line_of_sql);

That says "for this individual line of SQL, split it on comments and store the sections in @chunks. Then, if we have more than one chunk (assuming only one comment per line), the last chunk is a comment we can discard and the previous chunk just needs the comment marker removed. However, by changing my code to get 100% code coverage, I've made the code more fragile. Previously, the we effectively had an assertion that there was a comment marker in our line of SQL:

if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) { pop @chunks; }

But the new code merely assumes we have that comment marker:

if ( @chunks > 1 ) { $chunks[-2] =~ s/$comment$//; # XXX remove comment indicator pop @chunks; # XXX remove the comment }

As a result, if someone alters the code prior to this, the more rigorous code -- which could not achieve 100% code coverage -- would have been safer to use. Using something like Carp::Assert or something similar might help here. It would make the code more robust and allow the 100% code coverage.

That's part of what I like about code coverage. Now that I'm making the above change, the code is a more accurate reflection of what's really happening, though sometimes you really have to sit down and think it through.

Cheers,
Ovid

New address of my CGI Course.

Replies are listed 'Best First'.
Re: When 100% Code Coverage Backfires
by xdg (Monsignor) on Mar 26, 2007 at 12:35 UTC
    by changing my code to get 100% code coverage

    Isn't the point of coverage to change your tests to increase coverage? Changing your logic for the sake of coverage seems backwards. (Note -- I'm not talking about changing layout of conditionals so that Devel::Cover can follow the logic.)

    What your example does demonstrate is how seeking 100% coverage can help uncover faulty assumptions in either the requirements or the underlying code.

    Using something like Carp::Assert or something similar might help here

    I like that for clarity of intent, but of course, it's only hiding the lack of coverage of the failure of the assertion. Your assert line of code will be "covered", but the underlying logic won't.

    Nevertheless, if I had to sum up your post, it would this:

    Conditionals or branches that can't be covered because they always should be true (or false) should be converted into assertions instead.

    And that does sound like good advice. ++

    -xdg

    Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

      Isn't the point of coverage to change your tests to increase coverage?

      Nope. The point is to produce a better product.

      Sometimes that'll be by spotting where you need more tests. Sometimes that'll be spotting where you have dead code, or illogical code, and fixing that. Sometimes that'll be leaving well alone.

      (spot the bloke on a hobby horse - sorry :-)

        Nope. The point is to produce a better product.

        Agreed. I think I phrased it poorly.

        What I meant to suggest is that changing code for the primary purpose of increasing coverage is probably not the best thing to be doing, particularly if it makes the resulting product worse. Fixing defects or cleaning up code smells uncovered while working to improve coverage is -- as you pointed out -- part of the point of doing coverage testing in the first place.

        Since I generally try to follow test-driven development, I aim for 100% at the start and decreasing coverage is a helpful sign that my coding has gotten ahead of my testing.

        -xdg

        Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

        ++

        I just removed some dead code earlier today that the code coverage spotted. It was along the lines of the following:

        sub foo { # bunch of code if ( $some_condition ) { # do a bunch of stuff return $something; } else { # do some other stuff croak($error); } # do some more stuff }

        That can be easy to miss in refactoring.

        Cheers,
        Ovid

        New address of my CGI Course.

Re: When 100% Code Coverage Backfires
by adrianh (Chancellor) on Mar 26, 2007 at 13:58 UTC

    I think I've annoyed Ovid with this link in the past, but for those who've not encountered it Brian Marick's How to Misuse Code Coverage is worth a read.

Re: When 100% Code Coverage Backfires
by agianni (Hermit) on Mar 26, 2007 at 13:12 UTC

    Generally when given this scenario, I have used Test::MockModule to override something like Data::Record->records() in your example with results that would fully exercise the code, regardless of whether it would otherwise be triggered in its 'natural state'.

    Correct me if I'm wrong, but that's a generally acceptable testing practice, isn't it?

    perl -e 'split//,q{john hurl, pest caretaker}and(map{print @_[$_]}(joi +n(q{},map{sprintf(qq{%010u},$_)}(2**2*307*4993,5*101*641*5261,7*59*79 +*36997,13*17*71*45131,3**2*67*89*167*181))=~/\d{2}/g));'

      Depends upon how one goes about it. I prefer to use Test::MockModule when I have something external I don't want to run or I have arguments I want to capture. In this case, it's reasonably to use it in addition to running the Data::Record code. However, that means that your mocked method would then be behaving in a way that it's impossible for the code to behave. I'm not sure how that's a win over providing asserts.

      Cheers,
      Ovid

      New address of my CGI Course.

Re: When 100% Code Coverage Backfires
by eric256 (Parson) on Mar 26, 2007 at 14:33 UTC

    I'm a bit confused. If you need the test, then there must be some way to reach it, if you don't then its not needed. I don't understand how you think you can never reach it AND still must have the condition. If you are afraid of them changing something earlier, then don't you need a test earlier in the code to make sure that it is a comment so that if it is changed that test will fail?


    ___________
    Eric Hodges

      Rather than put a comment in the code letting people know that the test might fail, it's better to have an assert in there which cannot be ignored. So in this case, I might have something like:

      my $split = Data::Record->new( { split => $comment, unless => $RE{quoted}, chomp => 0, trim => 1, } ); my @chunks = $split->records($line_of_sql); if ( @chunks > 1 ) { assert( $chunks[-2] =~ /$some_text/ ); $chunks[-2] =~ s/$some_text//; # XXX remove comment indicator pop @chunks; # XXX remove comment }

      With that, I never have to worry that someone might change the previous code and silently break the following code. Assertions should be used to assert that things that should never break in fact, do not break. They can take a bit to get used to, but they're very valuable. In this case, it's even better than my initial code because I don't have a silent failure if the chunk does not contain the code that it must contain at that point.

      Cheers,
      Ovid

      New address of my CGI Course.