perlmeditation
Ovid
<p>I was going over our [dist://Devel::Cover|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 <tt>SQL::Stripper</tt>):</p>
<code>if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) {
...
}</code>
<p>In this case, I noticed the following for the SQL::Stripper code coverage:</p>
<pre><tt> line !l l&&!r l&&r condition
94 51 0 2 @chunks > 1 and $chunks[-2] =~ s/$comment$//</tt></pre>
<p>The number indicates how many times each condition is exercised. The exact code is this:</p>
<code> @chunks > 1 && $chunks[-2] =~ s/$comment$//</code>
<p>(Note that code as Devel::Cover reports it is often subtly different, but semantically equivalent, because Devel::Cover runs off of the opcodes).</p>
<p>That code is more or less equivalent to:</p>
<code> $left && $right</code>
<p>There are only three possibly paths through that (note the headings in the Devel::Cover output I listed above):</p>
<dl>
<dt>$left is false</dt>
<dd>!l</dd>
<dt>$left is true, $right is $false</dt>
<dd>l&&!r</dd>
<dt>$left and $right are $true</dt>
<dd>l&&r</dd>
</dl>
<p>Because of the way the <tt>SQL::Stripper</tt> 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:</p>
<code> if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) {
pop @chunks;
}</code>
<p>Might be better written as this:</p>
<code> if ( @chunks > 1 ) {
$chunks[-2] =~ s/$comment$//; # XXX remove comment indicator
pop @chunks; # XXX remove the comment
}</code>
<p>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:</p>
<code> my $split = Data::Record->new(
{
split => $comment,
unless => $RE{quoted},
chomp => 0,
trim => 1,
}
);
my @chunks = $split->records($line_of_sql);</code>
<p>That says "for this individual line of SQL, split it on comments and store the sections in <tt>@chunks</tt>. 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:</p>
<code> if ( @chunks > 1 && $chunks[-2] =~ s/$comment$// ) {
pop @chunks;
}</code>
<p>But the new code merely <em>assumes</em> we have that comment marker:</p>
<code> if ( @chunks > 1 ) {
$chunks[-2] =~ s/$comment$//; # XXX remove comment indicator
pop @chunks; # XXX remove the comment
}</code>
<p>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 [dist://Carp::Assert] or something similar might help here. It would make the code more robust and allow the 100% code coverage.</p>
<p>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.</p>
<!-- Node text goes above. Div tags should contain sig only -->
<div class="pmsig"><div class="pmsig-17000">
<p>Cheers,<br />
<a href="/index.pl?node=Ovid&lastnode_id=1072">Ovid</a></p>
<p><small>New address of <a href="http://users.easystreet.com/ovid/cgi_course/">my CGI Course</a>.</small></p>
</div></div>