Using standard testsuites

I recently agreed to take over DBD::CSV, as the previous maintainer admitted he had no time to work on it.

First things to do in these cases for me - right after creating a full-history git repository - is to browse through the source code, see if there are pieces of code marked with comments about TOD'd and possible failures. Right after that, I start applying perltidy (with my own .perltidyrc of course) and updating the docs and Change history as I go along. git commit is easy, so I leave a nice trail for myself in what I thing is improving the overall quality.

After code changes, of course I run the test suite, which in this case was rather weird. It was based on a system obviously not written to be just for this module, but to be some sort of drop-in test suite for all kinds off database modules.

That was only obscuring what was happening, so I removed all references to MySQL, Adabas, pNET and other unused pieces of the puzzle.

That cleared thing up in what was being tested, but it was still not the nice syntax I knew from Test::More, so a complete conversion was needed.

Starting with the basic test, I worked through the test files, and converted all test lines one-by-one to use Test::More, and after each chunk I re-ran the test suite to be sure I didn't break something.

One nice side-effect was that most of the global variables that were used in the old test suite could be removed. In this case the driver was always the CSV driver, so all conditional parts that assumed otherwise could be dropped.

At one point though, my (converted) tests suddenly started to fail. The old test looked like this:

    # Now try the explicit type settings
    Test($state or $cursor->bind_param(1, " 4", SQL_INTEGER()), 'bind 
+1')
        or DbiError($dbh->err, $dbh->errstr);
    Test($state or $cursor->bind_param(2, "Andreas König"), 'bind 2')
        or DbiError($dbh->err, $dbh->errstr);
    Test($state or $cursor->execute, 'execute binds')
        or DbiError($dbh->err, $dbh->errstr);
:
    Test($state or (($ref = $cursor->fetch)  &&  $id == 4  &&
                    $name eq 'Andreas König'))
        or printf("Query returned id = %s, name = %s, ref = %s, %d\n",
                  $id, $name, $ref, scalar(@$ref));
-->
t/40bindparam.t .... ok
[download]

and that passed all OK. So why should the new tests:

  # Now try the explicit type settings
  ok ($sth->bind_param (1, " 4", &SQL_INTEGER),   "bind 4 int");
  ok ($sth->bind_param (2, "Andreas König"),      "bind str");
  ok($sth->execute,                               "execute");
:
  ok ($sth->fetch,                                "fetch");
  is ($id,        4,                              "id   4");
  is ($name,      "Andreas König",                "name 4");
-->
t/42_bindparam.t ....... 2/40
#   Failed test 'id   4'
#   at t/42_bindparam.t line 75.
#          got: ' 4'
#     expected: '4'
# Looks like you failed 1 test of 40.
t/42_bindparam.t ....... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/40 subtests
[download]

suddenly fail?

I found out that it was a bug in DBD::File, where bind_param () just ignored the third (attributes) argument completely. I have a fix ready, but I am awaiting commit rights to the DBI repository.

The original test suite was written so complex to be versatile, that it did hide the real errors. Now how valuable is a test suite if it does not test what you think to test? It only adds to the confusion and to the time needed to debug when things go really really wrong.

In fact this is just a plea to use standard and proven test suites that all of us understand. They are probably a lot more reliable than whatever you come up with yourself, even if you are a seasoned programmer.

Enjoy, Have FUN! H.Merijn

Comment on Using standard testsuites Select or Download Code

Replies are listed 'Best First'.
Re: Using standard testsuites by Bloodnok (Vicar) on Apr 28, 2009 at 16:42 UTC
I wonder if the omission would have shown up had test coverage (utilising Devel::Cover) been generated ? I suspect it may have given some indication... Having said that, it has to be taken on trust that a used CPAN module passes the tests defined its own test suite (c/w say mod::Test::Unit - which tests a module and all its ancestors!!) - hence maybe there's a case for all CPAN modules to utilise/generate test coverage figures - thus imbuing more to the test results. After all, as you've demonstrated, what use is 100% pass rate if test(s) being run are flawed &/or incomplete ? A user level that continues to overstate my experience :-))	[reply]
Re^2: Using standard testsuites by DrHyde (Prior) on Apr 29, 2009 at 10:40 UTC
Devel::Cover is a fabulous thing, and has helped me find and squash obscure bugs that weren't covered by my tests, and I'm starting to add it to my distributions as and when I have to hack on them. But it's not a panacea. It will, for example, flag all sorts of tiny conditions that a conscientious programmer puts in for things like `open(...) \|\| die(...)` but really aren't worth bothering with testing. It will flag things that not only aren't worth bothering with, but actually can't be tested on your machine, such as patches that people sent you to make your code work on other platforms. Seperating the coverage wheat from the chaff is rather time-consuming.	[reply] [d/l]
Re^3: Using standard testsuites by Bloodnok (Vicar) on Apr 29, 2009 at 10:50 UTC
I agree, it [Devel::Cover] is most definitely not a panacea - I suggested it merely as a means to improve the end result - where the end result is determined as a fit-for-purpose test suite. Maybe it's just me, but I invariably test _all_ paths thro' the code - especially error paths in cases where exception classes are in use. Having said that, your point about untestable code is well made and indeed, taken. A user level that continues to overstate my experience :-))	[reply]
Re^2: Using standard testsuites by Tux (Canon) on Apr 28, 2009 at 16:54 UTC
Devel::Cover is certainly in the pen. One of the first things was to add that target to `Makefile.PL`, as I have for all my modules. `make cover` is almost a second nature by now :). In this case it would not have shown the error, as DBD::File is not part of DBD::CSV, but stowed away in the bowels of DBI. Enjoy, Have FUN! H.Merijn	[reply] [d/l] [select]
Re: Using standard testsuites by rir (Vicar) on Apr 29, 2009 at 17:23 UTC
It is in DBD::CSV's documentation that it does not handle types correctly. Your specific bug is mentioned. I believe it was a DBI design decision to not require stringent type handling of database drivers because that would have made it more difficult to create them and because lots of the backends are not strictly compliant. A CSV file doesn't not have an SQL compliant typing system. You may want to type all your fields; the next player may just want it to work. Be well, rir	[reply]
Re^2: Using standard testsuites by Tux (Canon) on Apr 29, 2009 at 19:42 UTC
I think that specifically binding types to parameters/variables - and accepting them in the calls - kinda promises the end-user that the types will be honored. In this case, the bug isn't really in DBD::CSV, but in DBD::File, and I have meanwhile commited a patch that applies basic types in these cases when they are requested. Unless speified in one way or another, DBD::CSV would of course not be able to set those types itself when reading/analyzing files. I'm now done with the conversion to Test::More, and found out that this process at least fixed one RT bug along the way. Devel::Cover by the way is a great tool to craft TODO lists :). Update: More important, related to the original post, it was tested for, so even the original author was expecting it to work, but the tests didn't test what it was supposed to test, and thus hiding the bug. Documented or not, this is very very unwanted behaviour. Enjoy, Have FUN! H.Merijn	[reply]
Re^3: Using standard testsuites by rir (Vicar) on Apr 30, 2009 at 20:32 UTC
binding types ... kinda promises ... the types will be honored. You are right but there are other aspects. If I pull a record, change someone's middle initial, then update the record; do I want a ' 4' to be corrected to a canonical '4'? Will all the other programs using my CSV file approve? ~~There~~ That is an argument for least change. When one tightens up on type handling, little problems start popping up. Often they keep popping up. Incidentally, the test changed: `SQL_INTEGER()` and `&SQL_INTEGER` can differ semantically. Be well, rir	[reply] [d/l] [select]
Re^4: Using standard testsuites by Tux (Canon) on Apr 30, 2009 at 20:38 UTC


Clear questions and runnable code get the best and fastest answer
	PerlMonks