Re: Devel::Cover Problem: testing || for a default value.
Michael G Schwern writes: On Tue, Jul 12, 2005 at 07:45:35AM +, Smylers wrote: A good way of putting assumptions into code is with (Michael's excellent) Carp::Assert: assert $p || $q, 'Either $p or $q must be supplied' if DEBUG; While I get where you're going, I don't see how this could be made to work. Fair enough; I'm prepared to believe you on that. What, exactly, is Devel::Cover supposed to do with those assert statements? I've no idea how Devel::Cover cover manages to do what it does. I suppose I must've been thinking something along the lines of if it can react to a specially formatted comment (urgh!) then it can react to an assert statement. Smylers -- May God bless us with enough foolishness to believe that we can make a difference in this world, so that we can do what others claim cannot be done.
Re: Devel::Cover Problem: testing || for a default value.
Michael G Schwern writes:
my $foo = $p || $q is not boolean. I'm not even sure you can call
it pseudo-boolean without understanding the surrounding code. How
do you know that $q can never be false?
So we want some way of annotating the code which will let Devel::Cover
know that you're assuming that $p and $q won't both be false.
Devel::Cover wouldn't be the only beneficiary of having such an
assumption explicitly stated: other humans having to read the code might
appreciate it too.
I can see why having 'inline' directives specifically for Devel::Cover
could be seen as ugly, but if instead they are general-purpose
assumptions then it's obviously better to have them next to the code
that's relying on the assumption than in some other file.
A good way of putting assumptions into code is with (Michael's
excellent) Carp::Assert:
assert $p || $q, 'Either $p or $q must be supplied' if DEBUG;
That improves your code, makes it easier for other people who have to
deal with it, and should be enough to keep Devel::Cover happy.
The other examples in the ticket play out the same way:
bless {}, ref $class || $class;
$class being false would be quite the error.
Ditto, so you could make the assumption explicit with an assertion. Or
perhaps something like Params::Validate would be better.
Smylers
--
May God bless us with enough foolishness to believe that we can make a
difference in this world, so that we can do what others claim cannot be done.
Re: Devel::Cover Problem: testing || for a default value.
On Tue, Jul 12, 2005 at 07:45:35AM +, Smylers wrote: I can see why having 'inline' directives specifically for Devel::Cover could be seen as ugly, but if instead they are general-purpose assumptions then it's obviously better to have them next to the code that's relying on the assumption than in some other file. A good way of putting assumptions into code is with (Michael's excellent) Carp::Assert: assert $p || $q, 'Either $p or $q must be supplied' if DEBUG; That improves your code, makes it easier for other people who have to deal with it, and should be enough to keep Devel::Cover happy. While I get where you're going, I don't see how this could be made to work. What, exactly, is Devel::Cover supposed to do with those assert statements? -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern Ahh email, my old friend. Do you know that revenge is a dish that is best served cold? And it is very cold on the Internet!
Re: Devel::Cover Problem: testing || for a default value.
[EMAIL PROTECTED] wrote: I respectfully disagree. I think you're focusing too much on the low-level behavior of || returning one of its operands. That behavior makes Perl's syntax flexible and a little ambiguous. Because Perl doesn't make a distinction between assign with a default value and perform a boolean OR Devel::Cover it has to play it conservatively. You shouldn't shift the burden to somewhere else (where $foo is subsequently used) either, because you don't know how it will be used. It could be 1) a boolean in a condition 2) used in another $a = $b || $c type expression 3) an output that only appears in a print() statement ... In any of these cases, it's possible that $foo is really a boolean but by the method you proposed $foo would only be tested for taking both true and false values in the first one. I appreciate your point of view. My suggestion (as I stated in my original post) is conditional on being able to determine the context in which the || is used. If you read the documentation for Want.pm or perldoc perlguts (search for context propogation) there seems to be the possibility of doing this -- though I freely admit that I don't understand perlguts well enough to say. Regards, David Golden
Re: Devel::Cover Problem: testing || for a default value.
Michael G Schwern wrote:
The other examples in the ticket play out the same way:
bless {}, ref $class || $class;
I encountered the coverage problem inherent in this code in the
constructor of a module whose maintenance I recently assumed. (For that
matter, I could have encountered it in modules I myself wrote and still
maintain.) I eventually figured that there was no reasonable way to
make the red in the top line of the condition box go away. And since
that was the last remaining bit of uncovered code within the
constructor, I figured I should clean that up as well. So I re-coded
along these lines:
my $self = ref($class) ? bless( {}, ref($class) )
: bless( {}, $class );
More verbose. Less elegant. But more strictly precise. And 100%
testable with reasonable conditions.
jimk
Re: Devel::Cover Problem: testing || for a default value.
On Tue, Jul 12, 2005 at 10:44:02PM -0400, James E Keenan wrote:
The other examples in the ticket play out the same way:
bless {}, ref $class || $class;
I encountered the coverage problem inherent in this code in the
constructor of a module whose maintenance I recently assumed. (For that
matter, I could have encountered it in modules I myself wrote and still
maintain.) I eventually figured that there was no reasonable way to
make the red in the top line of the condition box go away. And since
that was the last remaining bit of uncovered code within the
constructor, I figured I should clean that up as well. So I re-coded
along these lines:
my $self = ref($class) ? bless( {}, ref($class) )
: bless( {}, $class );
More verbose. Less elegant. But more strictly precise. And 100%
testable with reasonable conditions.
I would have just eliminated the $obj-new option. In all my years of
programming Perl I've never used it nor have I recently seen it used.
Pure YAGNI.
bless {}, $class;
--
Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern
'All anyone gets in a mirror is themselves,' she said. 'But what you
gets in a good gumbo is everything.'
-- Witches Abroad by Terry Prachett
Devel::Cover Problem: testing || for a default value.
In HTML::Widgets::NavMenu I have the following code:
sub get_coords_while_skipping_skips
{
my $self = shift;
my $callback = shift;
my $coords = shift || $self-get_current_coords(); # line 571
Now when I run the tests with Devel::Cover it reports the following for line
571 in the Condition Coverage:
A B dec
0 0 0 # marked as red.
0 1 1
1 X 1
Now in order to eliminate the red I need to make sure both operands are False.
However, $self-get_current_coords() cannot be false, and that's not what I'm
using || for. It's not an if ( FOO || BAR ). It's instead something like
that:
my $coords = shift;
if (!$coords)
{
$coords = $self-get_current_coords();
}
Which would probably eliminate the problem, but is much less elegant.
I should also note that I have many
my @coords = @{shift || $self-get_current_coords};
statements which exhibit this problem.
So what should I do to eliminate it?
Thanks in advance,
Shlomi Fish
-
Shlomi Fish [EMAIL PROTECTED]
Homepage:http://www.shlomifish.org/
Tcl is LISP on drugs. Using strings instead of S-expressions for closures
is Evil with one of those gigantic E's you can find at the beginning of
paragraphs.
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 19:27:52 +0300, Shlomi Fish wrote:
my $coords = shift;
if (!$coords)
{
$coords = $self-get_current_coords();
}
A work around:
$coords = scalar(@_) ? shift : $self-get_current_coords();
I should also note that I have many
my @coords = @{shift || $self-get_current_coords};
statements which exhibit this problem.
So what should I do to eliminate it?
Maybe Just Nothing
The issue is that you can't special case get_current_coords to be
truish, as far as Devel::Cover is concerned - it might not be.
Any fix that could be thought up is inherently problematic.
Coverage reporting is not done for the pretty colors - a human reads
it, and says OK, this is logical, get_current_coords always returns
a true value. It's not a race for greens and percentages.
--
() Yuval Kogman [EMAIL PROTECTED] 0xEBD27418 perl hacker
/\ kung foo master: /me supports the ASCII Ribbon Campaign: neeyah!!!
pgpjRB6TPn3n2.pgp
Description: PGP signature
Re: Devel::Cover Problem: testing || for a default value.
2005/7/11, Yuval Kogman [EMAIL PROTECTED]: Coverage reporting is not done for the pretty colors - a human reads it, and says OK, this is logical, get_current_coords always returns a true value. It's not a race for greens and percentages. otherwise it should be called golf... :) -- Jérôme Fenal - jfenal AT gmail.com - http://fenal.org/ Paris.pm - http://paris.mongueurs.net/
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 07:38:57PM +0300, Yuval Kogman wrote: So what should I do to eliminate it? Maybe Just Nothing The issue is that you can't special case get_current_coords to be truish, as far as Devel::Cover is concerned - it might not be. Any fix that could be thought up is inherently problematic. Coverage reporting is not done for the pretty colors - a human reads it, and says OK, this is logical, get_current_coords always returns a true value. It's not a race for greens and percentages. While I agree coverage is not a race, I disagree that a human should have to disambiguate between real missing coverage and a false negative. At least not more than once. I'll make the same argument no broken windows argument here that I do about warnings and tests: elminate all warnings, even if they are dubious. Ensure all tests pass eliminating all false negatives. Do not leave any expected warnings or expected failures because this erodes the confidence in the test suite. Warnings and test failures fail to ring alarm bells. One expected warning leads to two. Then four. Then finally too many to remember which are expected and which are not and you ignore them all together. The Pragmatic Programmer does a good job with this argument. http://www.pragmaticprogrammer.com/ppbook/extracts/no_broken_windows.html So goes the same with coverage. Red should be a BAD color, something you do not want to see. You want to eliminate the red. But sometimes its a false negative. In that case there should be some way to tell the tool that it is, in fact, a false negative. Just like skipping tests, you store the fact that there is a false negative to make the red go away. Red remains a bad color and seeing it means something is wrong. The team doesn't have to remember which bits are expected to be uncovered and which are not. What's missing is a way to let Devel::Cover know that a bit of coverage is not necessary. The first way to do this which pops into my mind is a comment. my $foo = $bar || default(); # DC ignore X|0 Hey, Devel::Cover! Ignore the case where the right side of this logic is false. Ignored conditions would be green, but perhaps a slightly different shade of green so they can be spotted if you're looking for them. -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern 'All anyone gets in a mirror is themselves,' she said. 'But what you gets in a good gumbo is everything.' -- Witches Abroad by Terry Prachett
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 13:17:48 -0700, Michael G Schwern wrote: On Mon, Jul 11, 2005 at 07:38:57PM +0300, Yuval Kogman wrote: I'll make the same argument no broken windows argument here that I do about warnings and tests: elminate all warnings, even if they are dubious. Ensure all tests pass eliminating all false negatives. Do not leave any expected warnings or expected failures because this erodes the confidence in the test suite. Warnings and test failures fail to ring alarm bells. One expected warning leads to two. Then four. Then finally too many to remember which are expected and which are not and you ignore them all together. I think that's the main difference between Devel::Cover and test runs for me - i run Devel::Cover carefully, just a few times before each release (repeating until I decide it's enough... The number of runs is roughly $lines_of_code/50, i would guess) During normal development I don't bother with coverage runs. They take long and I never get to 100% anyway, so I don't look for constant feedback. Devel::Cover is, for me, a think-hard process, where I try to figure out if there is duplicate logic, or completely untouched portions, and resolve this issue normally not by adding tests, but by removing meat. Only the trivial cases are resolved quickly. Perfecting coverage reports is IMHO too much pushing a project to where it could be, rather than pulling it to where it ought to be - determining features, and writing code and tests based on these ideas. In short, I think that no red in a coverage report is too much effort for me to work on continually. It doesn't help me as much as simply using the code does, and if I have no use for the code, then why am I working on it in the first place? -- () Yuval Kogman [EMAIL PROTECTED] 0xEBD27418 perl hacker /\ kung foo master: /me climbs a brick wall with his fingers: neeyah! pgpRgUIITWju5.pgp Description: PGP signature
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 01:17:48PM -0700, Michael G Schwern wrote: On Mon, Jul 11, 2005 at 07:38:57PM +0300, Yuval Kogman wrote: So what should I do to eliminate it? Maybe Just Nothing The issue is that you can't special case get_current_coords to be truish, as far as Devel::Cover is concerned - it might not be. Any fix that could be thought up is inherently problematic. Coverage reporting is not done for the pretty colors - a human reads it, and says OK, this is logical, get_current_coords always returns a true value. It's not a race for greens and percentages. While I agree coverage is not a race, I disagree that a human should have to disambiguate between real missing coverage and a false negative. At least not more than once. Quite. I'll make the same argument no broken windows argument here that I do about warnings and tests: elminate all warnings, even if they are dubious. Ensure all tests pass eliminating all false negatives. Do not leave any expected warnings or expected failures because this erodes the confidence in the test suite. Warnings and test failures fail to ring alarm bells. One expected warning leads to two. Then four. Then finally too many to remember which are expected and which are not and you ignore them all together. The Pragmatic Programmer does a good job with this argument. http://www.pragmaticprogrammer.com/ppbook/extracts/no_broken_windows.html I agree with this completely. The rest of the book is pretty good too. So goes the same with coverage. Red should be a BAD color, something you do not want to see. You want to eliminate the red. But sometimes its a false negative. In that case there should be some way to tell the tool that it is, in fact, a false negative. Just like skipping tests, you store the fact that there is a false negative to make the red go away. Red remains a bad color and seeing it means something is wrong. The team doesn't have to remember which bits are expected to be uncovered and which are not. What's missing is a way to let Devel::Cover know that a bit of coverage is not necessary. I've called this concept uncoverable code, and if you grep the current Devel::Cover source code for uncoverable you'll find a bit of code designed to solve this problem. But it's not complete, so I've neither documented it nor advertised it. Things have proceeded much further in my development code and I'm hoping to have something working properly before YAPC::EU. (No promises though.) The first way to do this which pops into my mind is a comment. my $foo = $bar || default(); # DC ignore X|0 Hey, Devel::Cover! Ignore the case where the right side of this logic is false. I wasn't particularly happy with the idea of needing to change the source code just to satisfy some tool. I feel the same way about doing things to shut up lint, for example, or to satisfy some arbitrary metric. That's why I've initially stored information in a .uncoverable file. But everyone I've spoken to about this has either not worried about or actively preferred to annotate the source. Many of those have also admitted to adding pod and even tests inline, which tells me that I really should be ignoring their opinions, but I try to please my users anyway, and so I'll see what I can do. Ignored conditions would be green, but perhaps a slightly different shade of green so they can be spotted if you're looking for them. Yes, it should be possible to easily find this uncoverable code amongst the coverage. It should also be possible to note why the code is uncoverable. I've also found it useful to have different classes of uncoverable code. Why is it that my TODO list only gets longer? -- Paul Johnson - [EMAIL PROTECTED] http://www.pjcj.net
Re: Devel::Cover Problem: testing || for a default value.
Michael G Schwern wrote: What's missing is a way to let Devel::Cover know that a bit of coverage is not necessary. The first way to do this which pops into my mind is a comment. my $foo = $bar || default(); # DC ignore X|0 I posted an item about this usage of the || operator the Devel::Cover RT several months ago: http://rt.cpan.org/NoAuth/Bug.html?id=11304 My suggestion turns on the question of whether it's possible to differentiate the context between a true boolean context ( foo() if $p || $q ) and a pseudo-boolean context that is really part of an assignment ( my $foo = $p || $q ) or just a standalone statement ( $p || print $q ). Want.pm seems to imply that this might be possible, but I don't know the guts of Perl well enough. The concept I had was that *EXCEPT* in true boolean context, the $p || $q idiom is (I think) pretty much logically equivalent to a trinary operation $p ? $p : $q (ignoring potential side effects) and thus the truth table in this situation only needs to include the first operand, thus avoiding the false-alarm. Regards, David Golden
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 11:22:51PM +0200, Paul Johnson wrote: my $foo = $bar || default(); # DC ignore X|0 Hey, Devel::Cover! Ignore the case where the right side of this logic is false. I wasn't particularly happy with the idea of needing to change the source code just to satisfy some tool. I feel the same way about doing things to shut up lint, for example, or to satisfy some arbitrary metric. That's why I've initially stored information in a .uncoverable file. Guh! .uncoverable would presumably be line number based. That means every time you edit your source file you have to change all the lines in .uncoverable. *shudder* The only other scheme I can think of, pattern matching based on source code, is unreliable. Finally, as with tests and docs, the closer you put the meta-data to the real data the more likely it will be kept up to date. So inline Devel::Cover hints seem the way to go. Why is it that my TODO list only gets longer? It means your code is popular and people want to use it and everybody loves you!!! Don't call them TODO items, call them hug lines. ;) -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern Ahh email, my old friend. Do you know that revenge is a dish that is best served cold? And it is very cold on the Internet!
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 17:33:26 -0400, David Golden wrote: Want.pm seems to imply that this might be possible, but I don't know the guts of Perl well enough. Want looks into the opcode tree and looks for context thingies inside it, so it should be possible as far as I understand from reading the code. I don't think this is desirable, though. Several times i've assigned booleans expressions into a scalar, to diagnose them later, or to pass them into something else.. This becomes common if you tend to use closures or other functional programming like structures. It could be a convenient option, but I am more weary of false negatives than false positives in these kinds of scenarios. It's my firm belief that special cases must be noted as such, and not implicitly taken care of. Whether this is truly a special case, or whether it is just too late due to Perl's idiomatic nature is a harder question though. Personally I prefer explicit meta-comments, one for each || in the line. I think It's a bit reminiscent of Test::More's $TODO. -- () Yuval Kogman [EMAIL PROTECTED] 0xEBD27418 perl hacker /\ kung foo master: /me spreads pj3Ar using 0wnage: neeyah!!! pgpC6AACt8rrw.pgp Description: PGP signature
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 05:33:26PM -0400, David Golden wrote:
http://rt.cpan.org/NoAuth/Bug.html?id=11304
My suggestion turns on the question of whether it's possible to
differentiate the context between a true boolean context ( foo() if $p
|| $q ) and a pseudo-boolean context that is really part of an
assignment ( my $foo = $p || $q ) or just a standalone statement ( $p
|| print $q ).
my $foo = $p || $q is not boolean. I'm not even sure you can call it
pseudo-boolean without understanding the surrounding code. How do you
know that $q can never be false?
The other examples in the ticket play out the same way:
bless {}, ref $class || $class;
$class being false would be quite the error. Devel::Cover can't know that
in most cases you want to ignore the 0 || 0 case because you assume $class
to always be some true value and don't think its worth testing that it
might be false.
Want.pm seems to imply that this might be possible, but I don't know the
guts of Perl well enough. The concept I had was that *EXCEPT* in true
boolean context, the $p || $q idiom is (I think) pretty much logically
equivalent to a trinary operation $p ? $p : $q (ignoring potential
side effects) and thus the truth table in this situation only needs to
include the first operand, thus avoiding the false-alarm.
That assumption only works in void context.
# Don't care about the return value of die.
open FILE, $foo || die $!;
# DO care about what $q is
foo( $p || $q );
And the special ||= case:
$p ||= $q;
--
Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern
Reality is that which, when you stop believing in it, doesn't go away.
-- Phillip K. Dick
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 02:54:19PM -0700, Michael G Schwern wrote: On Mon, Jul 11, 2005 at 11:22:51PM +0200, Paul Johnson wrote: my $foo = $bar || default(); # DC ignore X|0 Hey, Devel::Cover! Ignore the case where the right side of this logic is false. I wasn't particularly happy with the idea of needing to change the source code just to satisfy some tool. I feel the same way about doing things to shut up lint, for example, or to satisfy some arbitrary metric. That's why I've initially stored information in a .uncoverable file. Guh! .uncoverable would presumably be line number based. That means every time you edit your source file you have to change all the lines in .uncoverable. *shudder* Now, would I do that?! It's actually based on the MD5 hash of the line which means you only need to change it when you have changed the line, which you would presumably (there's that word again) be doing anyway. It seemed to work quite well for me in the last project I did, where I tried to do test first development, keeping the coverage at 100%. But never fear, I'm comitted to supporting comments too. Who knows, I might even end up using them myself. The only other scheme I can think of, pattern matching based on source code, is unreliable. Right. Finally, as with tests and docs, the closer you put the meta-data to the real data the more likely it will be kept up to date. So inline Devel::Cover hints seem the way to go. Hmmm ;-) Why is it that my TODO list only gets longer? It means your code is popular and people want to use it and everybody loves you!!! Don't call them TODO items, call them hug lines. ;) $ svk mv TODO HUGS Do I now get to port the thing to Haskell? -- Paul Johnson - [EMAIL PROTECTED] http://www.pjcj.net
Re: Devel::Cover Problem: testing || for a default value.
Michael G Schwern wrote:
my $foo = $p || $q is not boolean. I'm not even sure you can call it
pseudo-boolean without understanding the surrounding code. How do you
know that $q can never be false?
The other examples in the ticket play out the same way:
bless {}, ref $class || $class;
$class being false would be quite the error. Devel::Cover can't know that
in most cases you want to ignore the 0 || 0 case because you assume $class
to always be some true value and don't think its worth testing that it
might be false.
I think this is a coverage vs correctness distinction. The idea that I
was trying to convey is that while these expressions use a boolean
operator for a shortcut, they aren't really about truth vs. falsity of
the overall expression, *except* when they are being used as part of a
conditional statement. From a coverage perspective, what should matter
in my $foo = $p || $q is that $foo takes on the values of both $p and
$q at some point during the test suite, not whether or not $foo takes on
both true and false values -- coverage of that condition should be
checked when $foo is used in another expression.
In the bless case -- you're right that the case of $class being false
may be of interest, but that's not what this common idiom actually
does. The code will blithely pass a false value to bless (with
potentially unexpected results depending on whether $class is 0, , or
undef). That failure is an example of where correctness can't be
validated by coverage -- where the error lies between the ears of the
programmer. :-) If $class were explictly tested, then Devel::Cover
would pick it up properly, such as in bless {}, ref $class || $class ||
die.
Want.pm seems to imply that this might be possible, but I don't know the
guts of Perl well enough. The concept I had was that *EXCEPT* in true
boolean context, the $p || $q idiom is (I think) pretty much logically
equivalent to a trinary operation $p ? $p : $q (ignoring potential
side effects) and thus the truth table in this situation only needs to
include the first operand, thus avoiding the false-alarm.
That assumption only works in void context.
# Don't care about the return value of die.
open FILE, $foo || die $!;
# DO care about what $q is
foo( $p || $q );
Following my logic above, here I'd argue that we *don't* care what $q is
from a coverage perspective, only that both $p and $q are passed to foo
at some point. However, if for foo() we have:
sub foo { my $val = shift; print Hi! if $val }
We now care about what $val is from a coverage standpoint, so we get the
same coverage testing that way and we pick up the case we want where $q
is false in the original call. Unfortunately, this doesn't work with
Devel::Cover when foo() is a built-in or subroutine imported from
another module. In such a case, one option could be to use foo( $p ||
$q || '' ) or, more precisely, foo( $p || $q || $q ) -- which are
awkward, but are actually forcing the program to evaluate $q as true or
false.
I guess that begs the question for coverage testers which case is more
common -- caring only that $p || $q takes on the value of $p and $q,
or caring that both $p and $q take on both true and false values.
Regards,
David Golden
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 07:43:24PM -0400, David Golden wrote: I think this is a coverage vs correctness distinction. The idea that I was trying to convey is that while these expressions use a boolean operator for a shortcut, they aren't really about truth vs. falsity of the overall expression, *except* when they are being used as part of a conditional statement. From a coverage perspective, what should matter in my $foo = $p || $q is that $foo takes on the values of both $p and $q at some point during the test suite, not whether or not $foo takes on both true and false values -- coverage of that condition should be checked when $foo is used in another expression. You're right, I was thinking about this problem backwards. -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern Ahh email, my old friend. Do you know that revenge is a dish that is best served cold? And it is very cold on the Internet!
Re: Devel::Cover Problem: testing || for a default value.
[EMAIL PROTECTED] wrote:
Michael G Schwern wrote:
you're right that the case of $class being false may be of interest,
but that's not what this common idiom actually does. The code will
blithely pass a false value to bless (with potentially unexpected
results depending on whether $class is 0, , or undef). That failure
is an example of where correctness can't be validated by coverage --
where the error lies between the ears of the programmer. :-) If
$class were explictly tested, then Devel::Cover would pick it up
properly, such as in bless {}, ref $class || $class || die.
I'd say this idiom is one of the ones I am most often affected by in the
work I do for the Kwalitee project - the my$class = ref$proto||$proto;
idiom in constructors. I usually do the following
1. Add code to handle the 'both false' case, similiar to
my $class = ref $proto || $proto;
warn 'wrong calling convention for Class::Constructor::new - try
Class::Constructor-new' and return unless $class;
2. Add a test that makes ref $proto || $proto false, and tidy up the
harness so the warning doesn't mess up the output
my @warn;
my $rc;
eval {
local $SIG{__WARN__} = sub {push @warn, @_};
$rc = Class::Constructor::new();
};
is(@warn, 1, 'warning on calling convention');
like(shift(@warn), qr(wrong calling convention for
Class::Constructor::new - try Class::Constructor-new at ), 'expect
message');
is($rc, undef, 'no object created');
Now D::C is happy, and the code is more robust - to me a win-win. Now
newbies who dont really know perl's OO conventions are gently steered to
the path to enlightenment, and everyone else is only penalised with a
very lightweight unless test.This seems OK to me, but I know opinions on
this cover a wide spectrum.
The only caveat is in regards to those psycho's who like to bless into
the '0' namespaceI believe '' and undef result in blessing into main::
--
Leif Eriksen
Snr Developer
http://www.hpa.com.au/
phone: +61 3 9217 5545
email: [EMAIL PROTECTED]
Re: Devel::Cover Problem: testing || for a default value.
Paul Johnson wrote: On Mon, Jul 11, 2005 at 02:54:19PM -0700, Michael G Schwern wrote: On Mon, Jul 11, 2005 at 11:22:51PM +0200, Paul Johnson wrote: my $foo = $bar || default(); # DC ignore X|0 Hey, Devel::Cover! Ignore the case where the right side of this logic is false. I wasn't particularly happy with the idea of needing to change the source code just to satisfy some tool. I feel the same way about doing things to shut up lint, for example, or to satisfy some arbitrary metric. That's why I've initially stored information in a .uncoverable file. Guh! .uncoverable would presumably be line number based. That means every time you edit your source file you have to change all the lines in .uncoverable. *shudder* Now, would I do that?! It's actually based on the MD5 hash of the line which means you only need to change it when you have changed the line, which you would presumably (there's that word again) be doing anyway. theres another way to approach it. 1. emit a text version of the coverage failures which a. has same info as html details - ie line# conditionals, code b. starts with a comment. 2. use that 'exact' format as your ignore directive a. except that # means its entirely ignored b. user removes # to cause report supression now, full power (and a bit of tedium) is in users hands. by just renaming the output to .uncoverable (or whatever), they get a do-nothing version thats up-to-date with their code, and thats easy to use to supress everything. You could call it a feature that line-changes invalidate supressions, since the code being tested has changed ;-) Adding pattern matching on line # is easy, and would make the .uncoverable file a bit more flexible, you probly want \Q \E around the code itself, with loud caveats about not doing intelligent src-code matching. Hey, its supression, it shouldnt be too easy. (We all know the chilling effect of jailing journalists! :-O ) And updating supression files shouldnt be a daily occurrence, more of a 'once per release, before management looks at the html' ;-) Why is it that my TODO list only gets longer? Ive been wanting to spend enough time studying the code that I grok the way it works, it would be a worthy pod contribution to your work. It should increase the approachability of the module, and improve odds that others would feel they had a chance of successfully completing the TODO items. The code has a steep grokking curve.
Re: Devel::Cover Problem: testing || for a default value.
On Tue, 2005-07-12 at 10:46 +1000, [EMAIL PROTECTED] wrote: I'd say this idiom is one of the ones I am most often affected by in the work I do for the Kwalitee project - the my$class = ref$proto||$proto; idiom in constructors. I usually do the following 1. Add code to handle the 'both false' case, similiar to my $class = ref $proto || $proto; warn 'wrong calling convention for Class::Constructor::new - try Class::Constructor-new' and return unless $class; Why not delete the code entirely? Do these classes *really* expect users to call them with anything besides Classname-new()? I find it baffling that the Perl documentation recommends that technique, but until recently it recommended calling UNIVERSAL::isa() directly too. -- c
Re: [Maybe Spam] Re: Devel::Cover Problem: testing || for a default value.
[EMAIL PROTECTED] wrote: On Tue, 2005-07-12 at 10:46 +1000, [EMAIL PROTECTED] wrote: 1. Add code to handle the 'both false' case, similiar to my $class = ref $proto || $proto; warn 'wrong calling convention for Class::Constructor::new - try Class::Constructor-new' and return unless $class; Why not delete the code entirely? Do these classes *really* expect users to call them with anything besides Classname-new()? I'd put it this way. 1. Classes that dont test for a valid package name in their constructor do not expect to be called in any way other than Classname-new(). 2. Those classes fail badly when a naive/inexperienced/drunk/whatever user uses the wrong convention 3. Classes that test for a valid package name dont suffer from 2. 4. That said, 2 doesnt happen very often. Whether the developer wants to protection when it does happen is up to the developer. Getting 100% coverage via D::C is another motivation. -- Leif Eriksen Snr Developer http://www.hpa.com.au/ phone: +61 3 9217 5545 email: [EMAIL PROTECTED]
Re: Devel::Cover Problem: testing || for a default value.
On Mon, Jul 11, 2005 at 08:28:01PM -0600, Jim Cromie wrote: 1. emit a text version of the coverage failures which a. has same info as html details - ie line# conditionals, code b. starts with a comment. 2. use that 'exact' format as your ignore directive a. except that # means its entirely ignored b. user removes # to cause report supression now, full power (and a bit of tedium) is in users hands. by just renaming the output to .uncoverable (or whatever), they get a do-nothing version thats up-to-date with their code, and thats easy to use to supress everything. You could call it a feature that line-changes invalidate supressions, since the code being tested has changed ;-) No, this is as silly as saying TODO tests should have to all be reencoded every time you edit the test. You're causing the programmer to have to redo a task every time the file is edited. Not only is this a collosal waste of time and likely to make the programmer just give up on coverage or worrying about keeping .uncoverable up to date but, as with any rote task performed by humans, its an invitation to mistakes. Adding pattern matching on line # is easy, and would make the .uncoverable file a bit more flexible, you probly want \Q \E around the code itself, with loud caveats about not doing intelligent src-code matching. Hey, its supression, it shouldnt be too easy. FALSE! Making supression hard will cause them to not want to supress. The end result being... what? That they write code in a style which Devel::Cover likes? Is that an improvement, coding to satisfy the quirks of your analysis tools? More likely they'll just give up on maintaining .uncoverable and live with the false negatives, an outcome we do not want. And updating supression files shouldnt be a daily occurrence, Yes it should. It should happen as the programmer spots the mis-coverage, otherwise they're going to forget. Coverage analysis, like testing, is part of the development process to be continually used to prevent and catch bugs early and speed development, not a just before release check. The attitude that this should be a hard thing reveals that one does not trust the programmer not to abuse the uncoverable option and to put road blocks in the way. This is a losing attitude. If you don't trust the programmer not to try and fool their analysis tools, if they haven't bought in to the process, if they're consciously trying to sabotogue the system, making .uncoverable a little difficult to use is the least of your worries. There's a bazillion other ways a renegade programmer can sabotoge their analysis tools. Making the tools harder to use will just lead to more resentment. In the end, you have to trust your programmer. Don't try to solve social issues with technology. -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern Just call me 'Moron Sugar'. http://www.somethingpositive.net/sp05182002.shtml
