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

TODO is misbehaving? #829

Closed
karenetheridge opened this issue Apr 18, 2019 · 12 comments

Comments

Projects
None yet
2 participants
@karenetheridge
Copy link
Member

commented Apr 18, 2019

t/Legacy/todo.txt (near the bottom) seems to suggest that when $TODO` is set to the empty string, "in todo" should still be false. And yet, that doesn't match observed behaviour, e.g.:

use strict;
use warnings;
use Test::More;

local $TODO = '';

pass('pass');
ok(1, 'ok');
is(2,2, 'is');
my $t = bless {}, 'Test::More';
isa_ok($t, 'Test::More', 'isa_ok');

note "### in_todo is ",(Test::Builder->new->in_todo//'<undef>');

done_testing;

prints:

ok 1 - pass # TODO
ok 2 - ok # TODO
ok 3 - is # TODO
ok 4 - 'isa_ok' isa 'Test::More' # TODO
# ### in_todo is 0
1..4

(I found this while looking into why Module-Metadata's tests have become noisy in blead: see Perl-Toolchain-Gang/Module-Metadata#31)

@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

A git bisect resulted in this being the relevant commit:

commit 9c269ff6e2b8a7be223ef1df7a254a09ec4dad2d
Author: Chad Granum <exodist7@gmail.com>
Date:   Tue Dec 11 09:43:25 2018 -0800

    Fix a bug in handling $TODO
    
    Most places honored a defined but false $TODO setting. One place did not
    honor it. This has been corrected.
    
    Fixes #812
@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Also seen on travis: $TODO=''->true in version 1.302162, $TODO=''->false in 1.302133.

@exodist

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Please install the old version of Test-Simple before Test2 and double check how it behaved then. The original behavior did use an empty string as a valid todo value if I recall. In Test2 I initially made some places use an empty string as not-todo and it broke some old tests on cpan, so I fixed it. Your tests that are now noisy probably used to be noisy, or they were modified while the bug was in place making them newly noisy.

If I am wrong, and they are not noisy in legacy Test::Builder let me know and I will look into it.

@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Your tests that are now noisy probably used to be noisy, or they were modified while the bug was in place making them newly noisy.

Sorry, negative on both these counts.

At tag v0.99, the code I pasted above produces:

ok 1 - pass
ok 2 - ok
ok 3 - is
ok 4 - 'isa_ok' isa 'Test::More'
# ### in_todo is <undef>
1..4

(that is, the tests are not considered TODO). This is also the case for all tags up to and including v1.001006. I also checked v1.302015, v1.302080, v1.302135 and v1.302141, all of which also treat the empty string as not-TODO. It is release v1.302142 where this changes, which is the first release that contains 9c269ff.

@exodist

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Ok, I will look into it, thanks for the report. I will probably get to it next week at the PTS.

@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

The timing is unfortunate, as April 20 is when the blead code freeze starts, in advance of the 5.30 release. If there is an issue in Test::More, it would be ideal to have that in 5.30.

@exodist

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Unfortunately I can guarentee I have no time to look at this as deep as needed before the pts. I am absolutely swamped right now.

If you or someone else wants to take a stab at this and make a PR I can merge it and run my downstream tests to verify it does not break cpan. So with a PR I can verify and release, but I am not in a position where I can actually take time to figure out where the problem is and make a fix right now.

(Downstream tests are a single command and a lot of waiting, so doea not take enough time to be a blocker)

@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

I bisected down to the source commit above. Reverting that commit is an option.

@exodist

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

That commit was introduced to fix a bug. Reverting it would just be swapping which bug we have in cpan, making any version checks to work around the bug twice as complex.

What I need to do is go back to that first bug, and look in depth at this bug, and see what solution eliminates both bugs.

@exodist exodist closed this in ba69085 Apr 25, 2019

exodist added a commit that referenced this issue Apr 25, 2019

v1.302163
    - 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)
@exodist

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@karenetheridge I have pushed a TRIAL release of Test-Simple that fixes this and several other issues. I will push a stable version sometime during the PTS after cpan-testers has had some time to generate reports.

@karenetheridge

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

yay, thanks!

@exodist

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Released to CPAN in a non-trial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.