Re: Some questions on CDBI code

[prev] [thread] [next] [Date index for 2005/07/05]

On Tue, Jul 05, 2005 at 01:25:53PM +1000, leif.eriksen@xxx.xxx.xx wrote:
>    I'm helping with the Phalanx project, specifically the attempt to 
> write coverage tests for Class::DBI so that Devel::Cover reports 100% 
> coverage of all statements, branches, conditions and subroutines for all 
> the components (and dependencies) of CDBI - you can see how were going 
> at http://phalanx.kwiki.org/index.cgi?ClassDBIHoplites

Thanks for doing this! The test coverage is quite good at the minute (as
long as you can run it against multiple databases), but there's always
room for improvement!

> In Class::DBI::ColumnGrouper::primary(), the sub tries to detect if 
> you've called it in a non-array context and you have a multicolumn 
> primary key. (code is if (!wantarray && @cols > 1) ).
> If this is true, it calls Carp::confess, which of course dies. It then 
> tries to return the first of the primary key names - but it cant - cause 
> its died. And hence the line "return $cols[0];" is reported by 
> Devel::Cover as never being executed. So I'm thinking either 
> Carp::confess() is more severe than what the author actually intended, 
> or the author erronously coded to return - could somebody advise which 
> so I can make the appropriate change.

confess() is probably too severe. We probably want $self->_croak, which
could be overridden to just warn, so the fall through would be OK.

> In Class::DBI::Iterator::new(), there is no attempt to check the 
> validity of the class we a blessing into - hence it is possible to bless 
> into main:: if your not careful, and blessing into main:: is probably 
> not what you intend. Also the $them parameter is not checked to see that 
> it isa('Class::DBI') class, which is important later in next() - if it 
> isnt a 'Class::DBI' derived class, then the Class::DBI::construct() call 
> in next() will fail - it might be nicer to fail construction rather than 
> use. Both these issues are not that important in 'normal' use, there 
> just highlighted in the coverage testing. I'd suggest checking that 
> ref($me) || $me contains a package name before blessing, and that $class 
> has a isa() check done.

I'm not sure how we can check that we have a 'sensible' package name, as
'main' is an entirely valid package. It may not make a lot of sense to
give it a 'main', but I'd prefer to keep this duck-typed. As long as the
class we're given can merrily do the 'construct()' etc, I don't see why
we should impose restrictions on it.

> Lastly, I've spent a lot of time going cross-eyed at 
> Class::DBI::stringify_self(). Devel::Cover reveals several untested 
> paths at the line "return (ref($self) || $self) unless $self;" . I've 
> come to the conclusion that if called as a method (as intended), the '|| 
> $self' is superfluous, and can never be reached - hence I suggest it 
> becomes "return ref($self) unless $self;" - I can expand on the tests I 
> have written if you need proof. It basically comes down to that if $self 
> (from a method call) in bool context fails, then $self must be an object 
> and hence ref($self) returns a true value (unless we've been 
> particularly psychotic and bless into a package called '0'), hence '|| 
> $self' isnt requried. I cant get $self (from a method call) to be false 
> in bool context (through _undefined_primary) and not be a reference. You 
> can do something like "Class::DBI::stringify_self(0) - which is a 
> function call not a method call, and hence is too far removed from 
> normal usage - to get path coverage, but I dont think its justified.

This is one of the issues I came across when I was playing with
Devel::Cover. To get full coverage a lot of the time you need to call
methods as functions and make sure the code handles this. I've always
been undecided as to what to do here. In the general case I' rather add
unlikely code like "Class::DBI::stringify_self(0)" code to the tests to
get coverage than adding contorted checks to the code. In this specific
case I have no strong opinions.

Thanks again.

Tony

to extend
> 
> Do these sound reasonable ? They add nothing to the feature set of CDBI, 
> just janitor the code.
> 
> -- 
> Leif Eriksen
> Snr Developer
> http://www.hpa.com.au/
> phone: +61 3 9217 5545
> email: leif.eriksen@xxx.xxx.xx