test-class icon indicating copy to clipboard operation
test-class copied to clipboard

using test method return value as skip reason is problematic

Open exodist opened this issue 3 years ago • 1 comments

If you write a test method like this:

sub my_test(2) {
    ok(1);
    return "I will write the second test later"
}

then Test::Class will call $Test::Builder->skip("I will write the second test later"); once to make up for the shortcoming in the plan.

At first glance this seems useful. However there are a couple issues:

  1. This makes a plan useless. The plan is intended to make sure you run all the tests you expect. Test::Class completely defeats the purpose by auto-fixing it. For instance, you might add a 'return "the rest takes too long";' for debugging purposes while working on a method, then forget to take it out, Test::Class will happily use that message and yo umay never know you are skipping half your test method!

  2. If you do not add an intentional return then perl returns the result of the last statement. So if my return was missing from the first example, the skip reason would be "1" as returned by ok(1). Worse, you could call $self->extra_tests or similar, and that may return an object, which makes for a terrible skip reason, at will confuse people when they see ok 1 # SKIP: Foo::Bar(0x0000)

  3. Case 2 can break if it happens in the right Test2::IPC scenario as it tries to serialize the skip reason using Storable, the object could be too large (DBIx::ClasS anyone?) or can contain GLOB's or other things that Storable cannot serialize.

I am going to fix case 3 in Test::Builder. I am not sure what you should do about cases 1 and 2. case 1 is kind of locked in place because backcompat, so I guess you need a CAVEAT listed stating that Test::Class makes plans completely useless and you might as well not put any test-method plans. Case 2 probably needs a heuristic of some kind to see if the return value stringifies into something sane. Should also do the stringification of the result value $skip_reason = "$skip_reason" if nothing else to reduce the number of times huge objects get shoved into testing internals on Test::Builder versions that do nto have the fix I have not written yet.

exodist avatar May 19 '21 06:05 exodist