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

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

Closed
wants to merge 1 commit into from

Conversation

SineSwiper
Copy link
Contributor

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.

@schwern
Copy link
Contributor

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?

@SineSwiper
Copy link
Contributor Author

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.

@doherty
Copy link
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.

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
@SineSwiper
Copy link
Contributor Author

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

@SineSwiper
Copy link
Contributor Author

I forget. Was everything kosher with this one?

@schwern
Copy link
Contributor

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.

@SineSwiper
Copy link
Contributor Author

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
@schwern
Copy link
Contributor

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.

@SineSwiper
Copy link
Contributor Author

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

@schwern
Copy link
Contributor

schwern commented May 18, 2012

Something like that, yeah.

schwern added a commit that referenced this pull request Apr 15, 2013
@schwern schwern mentioned this pull request Apr 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants