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

[Piers C. <pdc...@bo...>]

Adam Spiers <ad...@sp...> writes:

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

Sorry for the delay.

> 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.

If you wrap things in a 'try' then it *already* turns any
straightforward die (ie, where you die with a string not an object)
into an Error::Simple. Frankly, I don't like the requirement to
capture the Error Simple and turn it into a Test::Unit::Error, but I
wanted T::U::Error's behaviour of adding the appropriate
object. For other 'typed' errors we really don't know enough about
them to be able to reliably turn them into Test::Unit::Error
objects. Maybe if you introduced a Test::Unit::Error::Typed, which can
then contain the original error...

As for exceptions thrown by setup, I took the view that trapping these
was a bad thing to do as a failure in setup code can be thought of as
a rather more fundamental error than an error in the test code itself,
and such a failure should probably halt the test process. I assume
that that was Christian's original view too, because I haven't changed
it any. 

However, looking at SUnit, it looks like the testrunner will also
catch exceptions thrown by the setup phase. I wonder if there's case
for wrapping the calls to setup in a 'try' and having that catch any
errors and wrap them in a Test::Unit::SetupError exception. Not sure
if that grabs us any extra info though.

>
> 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
>
> _______________________________________________________________
>
> Sponsored by:
> ThinkGeek at http://www.ThinkGeek.com/
> _______________________________________________
> Perlunit-devel mailing list
> Per...@li...
> https://lists.sourceforge.net/lists/listinfo/perlunit-devel
>
>

-- 
Piers

   "It is a truth universally acknowledged that a language in
    possession of a rich syntax must be in need of a rewrite."
         -- Jane Austen?