Subtests and todos

[yanick]

Fair warning: the patch seems to be working, doesn't break the test suites of Test::More. But I'm not exactly fully aware of what I'm doing...

In all cases, TODOs in subtests don't percolate to the aggregate results. This patch addresses that. What I think is a sane aggregate behavior is in the comments in Test::Stream::Event::Subtest.

[exodist]

Let me make sure I understand your intentions:

If there is a todo inside a subtest, make sure the final result of the subtest is also todo.

What if there are multiple todos inside the subtest, and each has a different reason? Which message is used?

I think I understand what you are asking for. Mind telling me your motivation? Is something broken? Is there some metadata you are trying to build or process that this will clean up?

I have no strong feeling about this either way. But it does alter a fundamental behavior that has been in place as long as subtest have, so it needs to be justified.

This is not a 'no' to the feature. But I also cannot say 'yes' yet. At the very least I feel this change should come after the March release of the new Internals.

[yanick]

Hi!

First: no problem at all about the being careful about such a change. If anything, I'm feeling safer to have somebody sanity-check the whole thing. :-)

And you got the gist right. Succinctly, right now we have:

$ cat s.pl
use Test::More;

subtest 'suite with todo test failing' => sub {
    local $TODO = 'normal todo';
    fail;
};

subtest 'suite with todo test passing' => sub {
    local $TODO = 'normal todo';
    pass;
};

done_testing;

$ prove s.pl
s.pl .. ok
All tests successful.
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.04 cusr  0.00 csys =  0.06 CPU)
Result: PASS

The TODO state within a subtest are not reflected in the aggregate result. So, in this example, prove will not know that test #2 has TODO elements that have passed. That is what the patch fixes, by percolating up the TODO state in, hopefully, a sane way. The rules I'm using are:

• if any of the subtests have failed, the aggregate reports as a failure.
• If not, if there's at least 1 TODO item inside, the aggregate is labelled as a TODO
• If at least one TODO has failed, the aggregate is marked as a failure (but it'll be a TODO failure because of the previous bullet point)

Note that for the percolated TODO comes without a message, for the reason you mention (if there are many of them, which one we pick?). I suppose that knowing if there are TODOs is what is important -- once you know they are there, one can look at the TAP output in more details.

[haarg]

I think this is solving the issue on the wrong end. It should be the consumer (Test::Harness) that tracks this information, not the producer.

[exodist]

I think it would be nice to have this meta-info more easily obtained via the subtest event object, though it is also not hard now as demonstrated in the pull request. That said I agree with @haarg about the changes to the post-test information about the issue.

The point not addressed so far is making the subtest have the #todo postfix when a test inside it was todo. In my opinion that is a question for Test-Simple, and the result of the change could be significant and matter to someone.

Ultimately the result of such a change would no change the pass/fail stats for any downstream consumer. However this can have a huge ripple effect for any testing tool that uses Test::Builder::Tester. It has a significant effect on the output TAP, and anyone verifying that string would need to update things. I am unsure if there is a Test::Builder::Tester specific change we can make to correlate to the change and preserve compatibility.

[yanick]

I think this is solving the issue on the wrong end.

That's a very good point. And as it's one that is more a specification decision (arguments can be made for either side, really), I'll leave to my better peers the joy to decide which side of the fence should bother with those pesky TODOs. :-)