On 12/9/13 5:03 AM, Francesc Guasch wrote:
On Fri, Dec 06, 2013 at 07:01:32PM -0500, James E Keenan wrote:
It's not a big deal, I add a Makefile.PL. I must admit I struggled with Dist::Zilla myself when I had to install my own module in other servers.
Today I found a few minutes to checkout the latest version of Test-SQL-Data from https://github.com/frankiejol/Test-SQL-Data. Thanks for adding the Makefile.PL. What follows reflects my own opinions and should not be taken as a definitive evaluation. I pay a lot of attention to the degree to which a CPAN distribution's test suite exercises its source code. I use Devel::Cover to study that. So the first thing I did was to say: perl Makefile.PL && make && cover -delete && \ HARNESS_PERL_SWITCHES=-MDevel::Cover make test && \ cover -summary $ cover -summary Reading database from /Users/jkeenan/gitwork/Test-SQL-Data/cover_db [trimmed] -------------------- ------ ------ ------ ------ ------ File stmt bran cond sub -------------------- ------ ------ ------ ------ ------ blib/lib/Test/SQL/Data.pm 97.4 71.0 66.7 100.0 -------------------- ------ ------ ------ ------ ------ That's *very* impressive test coverage right out-of-the-box! However, ... I then turned to study the source code. I think what you have written is, fundamentally, *not* something that belongs in the Test::* namespace on CPAN. That's because what your module does is to provide methods for *initializing a database in preparation for testing and debugging during development*. Your module does not provide new functions that work on the Test::Builder framework. Let me elaborate a bit on that. Test::Builder is a module which provides a singleton object which on which basic test methods are called and which keeps track of the status of tests. (See: perldoc Test::Builder) You and I will almost never use Test::Builder directly. Instead, we will use modules like Test::Simple, Test::More, Test::Class, Test::XPath, etc., which (a) inherit that Test::Builder singleton object; (b) provide functions which wrap around (and cleverly disguise) Test::Builder's methods. So the following functions: Test::Simple::ok() Test::More::ok() Test::More::is() Test::More::like() ... ... are all working with the same object. Broadly speaking, all such functions are expected to evaluate an expression and either (a) say whether it's "ok" -- Perl-true -- or not; or (b) compare the value we "got" with some value we "expected". All such functions are then expected to take an argument which is the "name" or human-readable "description" of an individual test. These functions produce output according to the Test Anything Protocol (TAP) so that the results can be aggregated by distributions like Test::Harness and TAP::Harness and utilities like 'prove'. My hunch -- I'll let others corroborate this -- is that you don't need any of that in Test::SQL::Data. My recommendations are that you: * Remove the 'use Test::More;' from the module. * Remove the few calls of Test::More::ok() from inside your _check_count() and match_table(). * Rename the module to something not beginning with "Test::". * In your test suite, use basic Test::More functions like ok(), is() and like() to test the *return values* of functions like _check_count and match_table(). What you will then be left with is a module which focuses on methods for preparing a database for use during the kind of testing and debugging one does during a software development cycle. What that module should be named and how well your module serves its intended purpose is a discussion I'll leave to others. HTH Thank you very much. Jim Keenan
