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

Already on GitHub? Sign in to your account

subtest should auto-note at the beginning of the test #290

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

SineSwiper commented Apr 24, 2012

Currently, following the path of a subtest within direct mode (non-harness) can be a tad confusing. For example, take this:

ok 2 - MANIFEST
    1..8
    ok 1 - Standard stuff at the beginning
    ok 2 - NAME
    ok 3 - AUTHOR
    ok 4 - VERSION_FROM
    ok 5 - ABSTRACT_FROM
    not ok 6 - LICENSE
    #   Failed test 'LICENSE'
    #   at t\test-dist.t line 661.
    #   blah, blah, blah......

At first glance, you think "Hmpf, looks like something within MANIFEST failed.", but you'd be wrong. The subtest result doesn't give you the super description until AFTER the sub has completed. The change here will put in an auto-note to give a quick reference of the issue:

ok 2 - MANIFEST
# SUBTEST: Build.PL ==>
    1..8
    ok 1 - Standard stuff at the beginning
    ok 2 - NAME
    ok 3 - AUTHOR
    ok 4 - VERSION_FROM
    ok 5 - ABSTRACT_FROM
    not ok 6 - LICENSE
    #   Failed test 'LICENSE'
    #   at t\test-dist.t line 661.
    #   blah, blah, blah......

Now you know, without having to scroll past all of the Failed test debug data, that this subtest block is actually referencing Build.PL, not MANIFEST.

Owner

schwern commented Apr 26, 2012

I like the idea. I have some niggles about the formatting. I think it looks better like so:

ok 2 - MANIFEST
    # Subtest: Build.PL
    1..8
    ok 1 - Standard stuff at the beginning
    ok 2 - NAME

I'M NOT A FAN OF SHOUTING. Lining up the note with the subtest makes it clear that it goes with the subtest without ASCII art.

What do you think?

Contributor

SineSwiper commented Apr 26, 2012

I was thinking more of a for loop structure (begin -> indent -> end), but I think your idea does the trick.

I leave formatting up to you, as well as whether this belongs in Test::More or Test::Builder.

Contributor

doherty commented Apr 26, 2012

I actually agree with @SineSwiper on the indenting, but I agree with @schwern about the shouting and huge arrow. Split the difference?

If this goes in Test::Builder, it'll presumably become available to all testing modules built on Test::Builder that use subtests, right? If so, I consider that a good thing.

@SineSwiper @SineSwiper SineSwiper + SineSwiper Change subtest to auto-note at the beginning of the test
Fix t/subtest/*.t to work with new auto-note
Fix outergroup test in t/subtest/predicate.t
Fix Test::Builder::Tester::complaint to not output
   double-spaced $wanted
727075f
Contributor

SineSwiper commented Apr 26, 2012

Commit rebased with doherty's compromise, a fixed set of tests, and some minor test issues I found.

This seems unrelated. Why was it necessary?

Owner

SineSwiper replied Apr 27, 2012

Somewhat unrelated, but found during testing. This kind of output was especially annoying:

#   Failed test '1fail, wrongplan (2), todo [Reason] set via todo_start'
#   at t/subtest/todo.t line 79.
# STDOUT is:
# # Subtest: xxx
#     1..17
#     not ok 1 - failme
#     #   Failed test 'failme'
#     #   at t/subtest/todo.t line 140.
#     # Looks like you planned 17 tests but ran 1.
#     # Looks like you failed 1 test of 1 run.
# not ok 1 - xxx # TODO Reason
# #   Failed (TODO) test 'xxx'
# #   at t/subtest/todo.t line 71.
# not ok 2 - regular todo test # TODO Reason
# #   Failed (TODO) test 'regular todo test'
# #   at t/subtest/todo.t line 72.
#
# not:
#     1..17
#
#     not ok 1 - failme
#
#     #   Failed test 'failme'
#
#     #   at t/subtest/todo.t line 140.
#
#     # Looks like you planned 17 tests but ran 1.
#
#     # Looks like you failed 1 test of 1 run.
#
# not ok 1 - xxx # TODO Reason
#
# #   Failed (TODO) test 'xxx'
#
# #   at t/subtest/todo.t line 71.
#
# not ok 2 - regular todo test # TODO Reason
#
# #   Failed (TODO) test 'regular todo test'
#
# #   at t/subtest/todo.t line 72.
#
# as expected

I did keep the failures for a bit to test out the output after the \n removal, and it looked a lot better:

#   Failed test 'outergroup with internal barfoo_ok_2 failing line numbers'
#   at t/subtest/predicate.t line 164.
# STDERR is:
#         #   Failed test 'bar'
#         #   at t/subtest/predicate.t line 94.
#         # Looks like you failed 1 test of 2.
#     #   Failed test 'namehere'
#     #   at t/subtest/predicate.t line 161.
#     # Looks like you failed 1 test of 2.
# #   Failed test 'outergroup'
# #   at t/subtest/predicate.t line 162.
#
# not:
#         #   Failed test 'bar'
#         #   at t/subtest/predicate.t line 94.
#         # Looks like you failed 1 test of 2.
#     #   Failed test 'namehere'
#     #   at t/subtest/predicate.t line 161.
# #   Failed test 'outergroup'
# #   at t/subtest/predicate.t line 162.
#
# as expected
Contributor

SineSwiper commented May 3, 2012

I forget. Was everything kosher with this one?

Owner

schwern commented May 8, 2012

@SineSwiper There are two problems, neither is your fault.

Here's the tl;dr version: Changing the TAP format can cause problems with other test modules' tests, they often test the exact output. 1.5 has a mechanism to mitigate that. So if it's patched against the Test-Builder1.5 branch instead, I can put it in now without concern, but it's going to take a few months for there to be a stable 1.5 release. And there's no plans right now for another 0.9x release. Patching 1.5 should be easy, it's all in TB2::Formatter::TAP.

First is a compatibility concern. A lot of test modules test themselves by checking their own output (something we're working on moving away from). Any change to the TAP format risks breaking modules. Usually when we do that we try to make sure there's a way they can write their test so it works with versions before and after the change, so they're not forced to upgrade their Test::More dependency.

Fortunately, subtests are not used all that often, so the real impact is likely to be low. But I'm trying to keep 1.5 focused on just getting it stable and avoiding adding new features or changes which might cause more downstream havoc than we already have.

Second is that there's unlikely to be another stable release of the 0.9x series, there's nothing which really needs urgent release and I'd like to keep focused on 1.5. The patch will have to be rewritten for 1.5.

Fortunately 1.5's Test::Builder::Tester uses a TAP formatter which does not change. So if you patch the regular TAP formatter in 1.5, there will be little compatibility impact.

Contributor

SineSwiper commented May 8, 2012

I'm cool with putting this into 1.5. I'll submit a new pull request with that in it.

Fortunately, subtests are not used all that often, so the real impact is likely to be low.

Yeah, that's kinda what I'd like to change. The subtest command is a great feature, but it comes with some readability flaws that I'd like to correct: this request, #294, and the fact that the TAP::Harness frontend doesn't display the deeper subtests as progress numbers.

@SineSwiper SineSwiper closed this May 8, 2012

Owner

schwern commented May 17, 2012

@SineSwiper PS TAP::Harness currently has NO CONCEPT of subtests. They're totally ignored, treated as garbage. They'd be happy for a patch to add subtests to the grammar as a start, and then decide how to display them later.

Contributor

SineSwiper commented May 18, 2012

Oh, I know. That would be the third patch: making it say something like 1->4->7..8..9, etc. instead of 1/?

Owner

schwern commented May 18, 2012

Something like that, yeah.

@schwern schwern added a commit that referenced this pull request Apr 15, 2013

@schwern schwern Indent the subtest note with the subtest.
Discussed in #290.
5c1b28c

@schwern schwern referenced this pull request Apr 15, 2013

Merged

Subtest note at start #364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment