[Perlunit-users] Test::Unit::TestCase->list_tests order, database testing, Re: @TESTS bug

[Matthew A. <mc...@us...>]

Anyone else care to join in?  I need another brain!

I'm tempted to cross-post to perlunit-devel...  maybe you could reply
about the patch/refactor to devel, and discuss the actual use of
decoupled test cases on user?


On Tue, Oct 26, 2004 at 02:17:28PM +0200, Jean-Louis Leroy wrote:

> I've found out a trivial bug in Test::Unit,

You're right that @TESTS doesn't behave as documented.  The rest of
the problem is more complex, but it makes sense to:

  correct the docs,

  ensure the users can do what they _need_ to do,

  hopefully give them a little nudge to help differentiate between
  wanting ordered tests and actually needing them.

> which btw solves this problem:
> http://sourceforge.net/mailarchive/forum.php?thread_id=3232369&forum_id=2441

Thanks for digging out the reference. 8-)


> Test::Unit::TestCase::list_tests does grab the testsub names from
> @TESTS as documented, but then sticks them in a hash, probably to
> remove duplicates:

The comments here are relevant,

# Returns a list of the tests run by this class and its superclasses.
# DO NOT OVERRIDE THIS UNLESS YOU KNOW WHAT YOU ARE DOING!
> sub list_tests {
>     my $class = ref($_[0]) || $_[0];
>     # ...
>     my %tests = map {$_ => ''} @tests;
>     return keys %tests;
> }

I don't know the exact reason for the shouty warning.  I think it's
justified but needs another 300 words of explanation.

Hmm, lots of things going on in this method.  Looking at the code and
pondering what's needed:


In the general case it makes sense to shuffle the order of the tests
methods: AIUI the design intention is that tests should be independent
of each other.  This includes side effects, previous return values and
so on.  Illustrations below.

I thought we had discussed the explicit use of rand() in shuffling the
tests, but can't find mention in Google.  rand() could produce
unnerving intermittent failures if there were linkage between tests,
but is better than tests that fail or not depending on what other test
methods are present, the names of the tests and the version of Perl.


The introspection (get_matching_methods calls) can only produce
duplicates when a test has been overridden from its superclasses.  It
makes sense to filter these out: the overridden methods can't be
called by name and when overriding a method one expects it to
disappear from the bottom layer.  I can't think why one might override
a test, but that's beside the point.

So class methods and superclass methods are shuffled together and
de-duped and this is fine so far.


When @TESTS is set, the superclasses' tests are _appended_ to the ones
listed in @TESTS, then shuffled.  Failing to include them now is
likely to confuse people so we'd better not change that.  But for the
shuffle, you could put the superclass tests first by including their
names in @TESTS at the front.  Having a method to get them might be
convenient.

Superclass test methods are added in an order depending on how they're
shuffled in their class and the inheritance order; unless you also
fixed the sort for these classes by whatever means.

Test names still need de-duping and your %seen will do this.  Or do
they?  Maybe you put the name of one test in @TESTS several times,
because you wanted it run several times?  This isn't how it was
intended to work, but TMTOWTDI says maybe it should be possible.  Most
people would still want supermethods de-duped.



So we have test ordering (by hash, random, as specified, sorted), and
the relative ordering of tests vs. inherited tests.  The only order
that can't easily be reconstructed is "as specified", so it makes
sense to preserve this where possible.

We don't want repeat runs of overriding tests, but may want repeat
runs explicitly in @TESTS.

We want to keep the current default of hash ordered tests.  Switching
to random ordering for the default needs wider discussion.

One solution would be a Test::Unit::OrderPreservedTestCase which
contains the current list_tests method, minus the %tests but plus a
de-dup filter on the inherited tests.  Then the Test::Unit::TestCase
inherits from the thing with the tedious name and does the %tests
thing.  This pushes the complications out into the already complicated
class structure.

Another solution would involve methods such as

  list_local_tests_ordered # returns @TESTS or introspects class

  list_super_tests_ordered # recurses on @ISA classes

  list_tests_ordered ..... # combines (@local, @super), de-duping
                           # @super but not @local

  list_tests               # get ordered list, shuffle by hash

Then we add good comments about why it's done like this and how to get
the most from it.

Folks determined to sort test names can override list_tests with a
pass-through method.

Folks wanting proper randomisation can override list_tests to shuffle
properly.

Default behaviour stays the same, except @TESTS works as documented.

Folks wishing to skip even numbered test methods after the shuffle can
do this, provided the thread on the other CPU shuffles with the same
random seed.


Any more?


> I experimented a fix in a subclass of T::U::TestCase:
> 
>   sub list_tests {
>     my $class = ref($_[0]) || $_[0];
>     # ...
>     my %seen;
>     return grep { !$seen{$_}++ } @tests;
>   }

I'm glad this will fix your problem.  I think we need something more
flexible for the default behaviour, and there may be better ways to
fix your problem depending on exactly what it is.


> I see that the latest release of the module dates back to 2002,

Yes.  There are a couple of changes since then in CVS, but nothing
exciting.

> and the latest message on this ML is one year old.

There's more recent activity on the perlunit-devel list, for small
values of 'activity'.

> Is the module still alive? Who's in charge?

Many developers have been quiet for a while, I can't speak for them.
I believe Piers has the CPAN upload password.

I am using perlunit again at (new) work and have some interest in at
least the bugfixes.  Of which this is one.

> If I take the time to make a proper patch, will it be incorporated
> and will a new version be released?

Yes and no.  We will happily accept patches, and future releases will
happen at some point.  Exactly who is "we" and when things happen
mostly depends on our round-tuit schedulers.

More importantly for your patch, I think the fix should either be to
the documentation, or a wider change to the code.  Will you stay for a
chat before leaving your patch?  8-)


The JUnit guys discuss test execution order starting at
  http://groups.yahoo.com/group/junit/message/4063

and ending with a removal of the OP's need to fix order at
  http://groups.yahoo.com/group/junit/message/4068


The Ant (build tool) guys have a go too,
  http://www.mail-archive.com/us...@an.../msg08101.html
but that's broken.  Ask Google for "run junit tests in a certain
order" while it's still in the cache, or try http://web.archive.org .


There's more about the design at
  http://www.martinfowler.com/bliki/JunitNewInstance.html

but not much discussion of the ordering.


A more concrete illustration involving databases, typed off the top of
my head so please excuse typos,

  package SortmeTest;
  use strict;
  use base 'Test::Unit::TestCase';

  our @TESTS = qw( test_insert test_select );
  # assume this works as documented

  # else do
  #   sub list_tests { return sort $_[0]->SUPER::list_tests }
  # just like the code tells you not to

  my $dbh;

  sub set_up {
    my $self = shift;
    $self->SUPER::set_up; # always a good idea

    $dbh ||= new DBI(gubbins); # ensure we have a database handle
  }

  sub test_insert {
    my $self = shift;

    my $expect = 1; # daft for a simple example, but a good pattern

    my $actual = $dbh->do("insert into tble (a, b) values (3, 42)");

    $self->assert_equals( $expect, $actual );
  }

  sub test_select {
    my $self = shift;

    my $rows = $dbh->selectall_arrayref("select b from tble where a = 3");
    $self->assert_equals(1, scalar @$rows); # check num. rows
    $self->assert_equals(42, $rows->[0]->[0]); # check the b value
  }

  END {
    if ($dbh) {
      $dbh->rollback; # so the insert doesn't fail tomorrow
      undef $dbh;
    }
  }

There are many places where you have to force the design to do
something odd:

 - global database handle

  Can't use an object instance variable because different tests have
  different objects.  May prefer not to have two database connections
  because it will be slower.  Can't use the transaction to do insert/
  select/rollback, except on one handle.

 - sorting the method execution order

  Arises from the dependence of test_select on the preceding
  test_insert.

  If you maintain the list of test methods independently of the method
  definitions, they can get out of step.  This will bite you.

 - the magic number 42 appears in two places

  Fine for a small constant, but what about a 70 character text
  constant?  Use another global?  Risk a typo?

 - use of END instead of tear_down

  If you run in the Tk test runner, this could be hours later since
  you'll be examining the test output before quitting perl.

  Suppose you're on Oracle and you have a unique index.  The
  uncommitted record will cause a subsequent run of the test suite to
  hang until you close the window on the first one.  (By default? I
  don't know about disabling the behaviour.)  Very confusing.

  Fixing this one by having a teardown which checks $self->name before
  deciding whether to rollback...  well this way lies further madness.

OK, so I have set up my straw man.  He won't mind being "mended" with
a small diff:

  package ImprovedTest;
  use strict;
  use base 'Test::Unit::TestCase';

  sub set_up {
    my $self = shift;
    $self->SUPER::set_up; # always a good idea

    $self->{dbh} = new DBI(gubbins);
  }

  sub dbh { $_[0]->{dbh} } # if you like accessors

  sub test_fortytwo {
    my $self = shift;
    my $num = 42;
    $self->part_insert($num);
    $self->part_select($num);
  }

  sub part_insert {
    my ($self, $val) = @_;

    my $expect = 1; # daft for a simple example, but a good pattern

    my $actual = $self->dbh->do("insert into tble (a, b) values (3, $val)");

    $self->assert_equals( $expect, $actual );
  }

  sub part_select {
    my ($self, $val) = @_;

    my $rows = $self->dbh->selectall_arrayref("select b from tble where a = 3");
    $self->assert_equals(1, scalar @$rows); # check num. rows
    $self->assert_equals($val, $rows->[0]->[0]); # check the b value
  }

  sub tear_down {
    my $self = shift;
    $self->SUPER::tear_down;
    my $dbh = $self->dbh;

    if ($dbh) {
      $dbh->rollback; # so the insert doesn't fail tomorrow
    }
  }

Does this example make things any clearer?

 - one dbh per test object = one dbh per insert/select/rollback

  We have isolation.

 - test suite runs your tests independently

  Currently in series but no particular order, but if you're on a
  multi-processor machine then maybe you prefer to run them in
  parallel?  Or with a cluster, on separate machines?

  These features are just a twinkle in the eye, now.  One day somebody
  with a three hour test suite will implement them because it would
  save time to do so...  and might be easier than optimising the
  tests.

 - change your magic constants with ease

  I can add a new test like this,

    sub test_elephant {
      my $self = shift;
      my $string = "elephant";
      $self->part_insert($string);
      $self->part_select($string);
    }

  when I decide that the tble shall also accept strings.

 - rollback happens ASAP, whether tests pass/fail/die

  In a general way, for any other test methods we declare.  This
  starts to look like a base class My::DatabaseTestCase if you have
  many groups of tests to write.

 - doesn't even try the select if the insert fails

  The failure exception or die takes control off to the framework and
  then to tear_down.  No point trying the select, you expect it to
  fail.

  If you want to ensure that the select _does_ fail when run without
  the insert, this is a separate test.

    sub test_selectfail {
      my $self = shift;

      $self->assert_raises('Test::Unit::Failure',
                           sub { $self->part_select("Elvis") });
    }

  It's probably a useful test, too: a fresh dbh, select, failure,
  catch failure and be happy with it.

I hope this gives you some ideas.  Sorry if it's too simple for you,
but I haven't seen the tests you already have.


Matthew  #8-)