message confusing for non-incrementing failures.

[exodist]

Some events cause failures, but do not bump the test count (and do not add an 'ok' line in TAP output). At the end this produces a confusing message:

# Looks like you failed 1 test of 5.

This even if the 5 ok lines were all a pass.

This message should be clarified, maybe an extra test state variable to track the types of failures will be needed.

[toddr]

t/nowarn.t .. 
ok 1 - test1 
ok 2 - test2
ok 3 - test3
# Unexpected warning: Intentional Warning here! at t/nowarn.t line 46.
ok 4 - test4
ok 5 - test5
1..5
# Looks like you failed 1 test of 5.
Dubious, test returned 1 (wstat 256, 0x100)
All 5 subtests passed 
Test Summary Report
-------------------
t/nowarn.t (Wstat: 256 Tests: 5 Failed: 0)
  Non-zero exit status: 1
Files=1, Tests=5,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.30 cusr  0.04 csys =  0.38 CPU)
Result: FAIL

[Leont]

That non-zero exit status is far more serious than the confusing message IMO

[autarch]

The non-zero status is correct here. The test really did fail. Indicating this via TAP is difficult because TAP is terrible.

[exodist]

Might make sense to add a final OK event for clarity. Obviously that effects test count, but nobody seemed to complain that Test::NoWarnings does that.

[autarch]

In that case isn't it easier to just make the Warning event increment the test count?

[petdance]

nobody seemed to complain that Test::NoWarnings does that.

We've just worked aroudn it. We just add one to the plan when we use Test::NoWarnings.

[exodist]

@autarch possibly, why don't you try it and see if it looks sane?

[autarch]

use strict;
use warnings;

use Test::More;
use Test2::Plugin::NoWarnings;

ok 1, 'foo';
warn 'wtf';
ok 2, 'bar';

done_testing();

The output with increments_count returning false:

$ prove -lv t/foo.t 
t/foo.t .. 
ok 1 - foo
# Unexpected warning: wtf at t/foo.t line 10.
ok 2 - bar
1..2
# Looks like you failed 1 test of 2.
Dubious, test returned 1 (wstat 256, 0x100)
All 2 subtests passed 

Test Summary Report
-------------------
t/foo.t (Wstat: 256 Tests: 2 Failed: 0)
  Non-zero exit status: 1
Files=1, Tests=2,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.03 cusr  0.01 csys =  0.08 CPU)
Result: FAIL

And with increments_count returning true:

$ prove -lv t/foo.t 
t/foo.t .. 
ok 1 - foo
not ok 2 - Unexpected warning: wtf at t/foo.t line 10.
ok 3 - bar
1..3
# Looks like you failed 1 test of 3.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

Test Summary Report
-------------------
t/foo.t (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=3,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.04 cusr  0.00 csys =  0.06 CPU)
Result: FAIL

[autarch]

I think bumping the count looks good. Messing with the test count is what test tools do, so I don't think it's unreasonable that using this plugin can do such a thing. The one issue is that it doesn't output a test result when there's no warnings, so you can't rely on a static test count.

[toddr]

Why wouldn't you want to emit a test at the end that says you got no warnings?

[autarch]

It's not so much about want as about how the plugin is implemented. That said, I'm not sure I want to ;)

It's just extra work for not much gain, IMO. The current implementation is really simple. Adding this extra test would make it more complicated.

[exodist]

In this case it would be extra tests when you already have a failure, so I don't think the test count / static plan issue is a big deal, the test count is only wrong on failure.

if it looks good then cool. I still think we need to correct this end of test message though, for other tools with similar effects.

[Leont]

The non-zero status is correct here. The test really did fail. Indicating this via TAP is difficult because TAP is terrible.

I can only agree TAP is terrible, but the return value not matching the number of not-ok's is just plain confusing.

[autarch]

I can only agree TAP is terrible, but the return value not matching the number of not-ok's is just plain confusing.

Oh, I see what you're saying. I thought you were saying that exiting non-zero was wrong.

[exodist]

5f39961 is a fix that should make this all a lot less confusing.

[exodist]

d0ea5eb updated

[ppisar]

I noticed that Scalar-Does tests fail since this commit https://rt.cpan.org/Public/Bug/Display.html?id=119030. This boils down to this code:

$ perl -Ilib -e 'use Test::More; use Test::NoWarnings; done_testing(1);'
1..1
ok 1 - no warnings
# All assertions passed, but errors were encountered.

It returns non-zero exit code. Is this because Test::NoWarnings shouldn't be used this way?

[Leont]

That doesn't look good to me

[exodist]

this does in fact bisect to 7aa8794

[exodist]

I have investigated. The problem is that the hub is finalized before the final event happens.

Example:

$ perl -Ilib -e 'use Test::More; use Test::NoWarnings; ok(1); done_testing(2)'
ok 1
1..2
ok 2 - no warnings

That is using the commit just before 7aa8794. Notice that the plan comes in just before the final event. Valid TAP requires the plan to be either before all the ok's, or after all the ok's, not in the middle.

7aa8794 did not cause the bug, it exposed it. Things were passing incorrectly before that commit.

[exodist]

Ah-Ha: https://rt.cpan.org/Public/Bug/Display.html?id=66485

Test::NoWarnings is known to not work with done_testing. The test in https://rt.cpan.org/Public/Bug/Display.html?id=119030 must have been written while Test2 was incorrectly passing.

The bug is not that the test now fails, the bug is that your test used to pass when it should not have. You need to switch away from using done_testing in that test. If you were to install the old Test::More from before the Test2 stuff was added your test would still be incorrect (but might pass by chance, try adding an ok and then it would surely fail)

[exodist]

I am going to re-close this ticket. The problem is that you are using Test::NoWarnings incorrectly. There was a Test2 bug as well, but it was already fixed which is why you are having a problem, you were relying on the bug.

[ppisar]

Thank you for the explanation.

[karenetheridge]

for future reference, Test::Warnings plays nicely with done_testing -- indeed that was one of the main reasons why it was written.

[Leont]

I am going to re-close this ticket. The problem is that you are using Test::NoWarnings incorrectly. There was a Test2 bug as well, but it was already fixed which is why you are having a problem, you were relying on the bug.

IMO Test2 should protect the user better from such a buggy usage. 255 is a reasonable return value if it really can't make sense of the situation anymore, but shouldn't it let this pass without explaining what's happening ("but errors were encountered." is less helpful than "but received results after finalization" or some such).

[exodist]

the silent pass was fixed, that is why we are having this discussion at all, so there is no action to be taken there.

as for the inconsistent end result, the problem is that both Test2 and Test::NoWarnings use END blocks. Test2 uses one to set the exit value and do its final protections. Test::NoWarnings uses one to send an event. END blocks are tricky, execution depends on load order. There is really not much more Test2 can do technically to help here.

[Leont]

There is really not much more Test2 can do technically to help here.

Yeah, I realized that right after posting, but my edit was too late.

I do think the diagnostic can be improved though.

[exodist]

I can make a hub warn or die if it recieves an event after finalization. That might help a little, but is not a bad idea even if it is of minimal use here.