[Perlunit-devel] Test::Unit::Error superfluous?

[Adam S. <ad...@sp...>]

[I mainly need a reply from Piers on this.]

I've been investigating why PerlUnit occasionally dies in the middle
of a run, and it turned out to be something in the set_up method of
one of my test cases throwing an exception.  Currently
TestCase::run_base only traps exceptions thrown from running the test
method itself, not those thrown from running the set_up method.  It
struck me that the framework should be just as robust in the face of
run-time errors from calls to set_up() as to calls to test methods, so
I set about fixing this, and it raised some issues.  The code in
question is

  sub run_bare {
      my $self = shift;
      debug(ref($self) . "::run_bare() called\n");
      $self->set_up();
      try {
          $self->run_test();
          1;
      }
      catch Error::Simple with {
          # Something died, which throws an Error::Simple
          Test::Unit::Error->make_from_error_simple(shift, $self)->throw;
      }
      finally {
          # Only gets called if 'set_up' succeed
          $self->tear_down;
      };
  }

There are two things wrong with this.  The first I just described,
which is that exceptions from set_up() are not trapped, causing the
framework to die.  The second is that only Error::Simple exceptions
from run_test() are trapped, but I can't think of any reason why the
actual test shouldn't fail by throwing any kind of exception.  Given
that the only point of Test::Unit::Error is to encapsulate
Error::Simple exceptions thrown by the test, I'm wondering whether
it's even needed.  We could just not bother trapping any exceptions in
run_bare(), and change Test::Unit::Result::run_protected to treat
*any* exceptions other than Test::Unit::Failure as an error,
i.e. change

    try {
        &$closure();
    }
    catch Test::Unit::Failure with {
        $self->add_failure($test, shift);
    }
    catch Test::Unit::Error with {
        $self->add_error($test, shift);
    };

to 

    try {
        &$closure();
    }
    catch Test::Unit::Failure with {
        $self->add_failure($test, shift);
    }
    otherwise {
        $self->add_error($test, shift);
    };

Can anyone see anything wrong with this in principle?  Actually, I
just spotted something, which is the exception passed to add_error()
wouldn't be a Test::Unit::Exception any more, so it wouldn't have the
set_object() method, which is used for determining which test caused
the exception.  Any other problems?

I think what I'll do in the absence of any other suggestions is change
make_from_error_simple so that it encapsulates *any* exception within
a Test::Unit::Error, not just Error::Simple exceptions.

In the course of investigating all this, I also noticed that in
t/tlib/TestTest.pm, some tests (e.g. test_tear_down_setup_fails() and
test_run_and_tear_down_fails()), throw Test::Unit::Error exceptions
from within their inner classes' set_up() and tear_down() methods.  In
real-life scenarios, this would never happen, so these are unrealistic
tests.  I'm changing them to normal die() calls, but since they
originate from revision 1.1 of lib/Test/Unit/tests/TestTest.pm by
Christian, it'd be reassuring to hear someone else agreeing that I'm
doing the right thing.

Adam