Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing tests counted but displayed as TODO with $TODO defined but false #812

Closed
dboehmer opened this issue Jun 20, 2018 · 3 comments
Closed

Comments

@dboehmer
Copy link

Test::More seems to behave inconsistently when $TODO has a defined but false value. A failing test is counted for the test summary but is displayed as TODO test. That is really confusing.

TODO, or not TODO: that is the question
William Shakespeare

The following test file illustrates the issue:

use strict;
use warnings;

use Test::More;

my @values = ( 
    undef,            # as expected
    "",               # false but defined -> inconsistent
    0,                # false but defined -> inconsistent
    0.0,              # false but defined -> inconsistent
    "0.0",            # true -> TODO
    "this is why",    # as expected
);

for my $value (@values) { 
  TODO: {
        local $TODO = $value;
        fail "foo";
    }
}

pass "bar";

done_testing;

Only the first and the two last failing tests are displayed consistently. The other 3 are both TODO and not-TODO:

$ perl todo.t 
not ok 1 - foo
#   Failed test 'foo'
#   at todo.t line 18.
not ok 2 - foo
#   Failed (TODO) test 'foo'
#   at todo.t line 18.
not ok 3 - foo
#   Failed (TODO) test 'foo'
#   at todo.t line 18.
not ok 4 - foo
#   Failed (TODO) test 'foo'
#   at todo.t line 18.
not ok 5 - foo # TODO 0.0
#   Failed (TODO) test 'foo'
#   at todo.t line 18.
not ok 6 - foo # TODO this is why
#   Failed (TODO) test 'foo'                                                                                        
#   at todo.t line 18.                                                                                              
ok 7 - bar                                                                                                          
1..7                                                                                                                
# Looks like you failed 4 tests of 7.

I am not sure whether $TODO defined but false should make a test TODO or not TODO. Most of the false values seem not to be a useful TODO message but having "" for no message at all might be desirable.

Unfortunately I also don't know enough about the guts of Test::More to provide a fix. I'd gladly do with some hints.

@exodist
Copy link
Member

exodist commented Nov 26, 2018

a defined but falsy $TODO making TODO effective is a legacy Test::More behavior, too late to debate the behavior without breaking things (Though if it were not too late I would be on the side of a false value disabling todo). However it is absolutely a bug that it displays the TODO but counts a failure, it should not count a failure.

@dboehmer
Copy link
Author

Thanks for picking this up!

Can you give me a hint where this is to be fixed or are you going to fix it yourself?

@exodist
Copy link
Member

exodist commented Nov 26, 2018

it is probably somewhere in Test2::API::Hub, if not there then maybe in Test2::Event::*. I will fix it when I have a moment, but probably not today.

exodist added a commit that referenced this issue Dec 11, 2018
    - Fix #814 Windows fork+test failure
    - Fix #819 Documentation updates
    - Fix #810 Verbose TAP newline regression
    - Fix #817 local $TODO bug
    - Fix #812 Another local $TODO bug
    - Fix #815 shm read warning
    - Merge doc fix PR's from magnolia-k (thanks!)
exodist added a commit that referenced this issue Apr 25, 2019
Previous fix for #812 was incorrect.
Fixes #829

The original issue is that a TODO = '' would result in a failing test,
but diagnostics that report TODO

MY original fix was to make TODO = '' set todo, which was incorrect

The correct fix was to make the diagnostics not incorrectly report a
TODO when it is set to ''.

To add consfusion, Test::Builder always ignores TODO = '', but does NOT
ignore TB->set_todo('').... That is a legacy behavior that must be
preserved :-(
exodist added a commit that referenced this issue Apr 25, 2019
    - Do not use threads::Shared in Test::Tester::Capture (#826)
    - Add missing version info to Info/Table
    - Fix event in global destruction bug (#827)
    - Proper fix for todo = '' (#812, #829)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants