Bug #49080 for Module-Build: [PATCH] 'use_tap_harness' should die on test failure

Thu Aug 27 18:45:27 2009 dwheeler [...] cpan.org - Ticket created

Subject:	'use_tap_harness' should die on test failure
Date:	Thu, 27 Aug 2009 15:44:14 -0700
To:	bug-module-build [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

I'm using the "use_tap_harness" option in a project, and a colleague, who set up a pre-checkin hook to make sure that all tests pass, discovered that `./Build test` was exiting normally even when tests pass. Such is not the case when using Test::Harness, which `die`s on failure. TAP::Harness doesn't die, though; it's up to the caller to die. So the attached patch calls `exit 1` if the tests fail. I'd say that this is a bug, given that it means that the behavior of `./Build test` varies depending on whether or not one uses TAP::Harness. Best, David

Message body is not shown because sender requested not to inline it.

Thu Aug 27 22:14:57 2009 dagolden [...] cpan.org - Subject changed from ''use_tap_harness' should die on test failure' to '[PATCH] 'use_tap_harness' should die on test failure'

Thu Aug 27 22:14:57 2009 dagolden [...] cpan.org - Status changed from 'new' to 'open'

Thu Aug 27 22:14:57 2009 dagolden [...] cpan.org - Severity Normal added

Sun Aug 30 21:29:21 2009 dagolden [...] cpan.org - Correspondence added

Thank you for the bug report and patch. I took a different approach to fixing it, since "exit 1" can't be trapped in the usual way for testing. Instead, the full die() summary message of Test::Harness is emulated. I also added tests to confirm the non-zero exit code. Patched in trunk and will be in a future release.

Sun Aug 30 21:29:22 2009 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Sun Aug 30 23:15:24 2009 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49080] [PATCH] 'use_tap_harness' should die on test failure
Date:	Sun, 30 Aug 2009 20:14:56 -0700
To:	"bug-Module-Build [...] rt.cpan.org" <bug-Module-Build [...] rt.cpan.org>
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Aug 30, 2009, at 18:29, "David Golden via RT" <bug-Module-Build@rt.cpan.org Show quoted text

> wrote:

Show quoted text

> Thank you for the bug report and patch. I took a different approach > to > fixing it, since "exit 1" can't be trapped in the usual way for > testing. > Instead, the full die() summary message of Test::Harness is emulated. > I also added tests to confirm the non-zero exit code. > > Patched in trunk and will be in a future release.

I'm not sure that's the right approach, since TAP::Harness will have already output that information. It'll look redundant. I think that it needs to be a quiet exit, like Test::Harness does. David

Sun Aug 30 23:15:26 2009 The RT System itself - Status changed from 'patched' to 'open'

Mon Aug 31 00:44:41 2009 dagolden [...] cpan.org - Correspondence added

On Sun Aug 30 23:15:24 2009, DWHEELER wrote: Show quoted text

> I'm not sure that's the right approach, since TAP::Harness will have > already output that information. It'll look redundant. I think that it > needs to be a quiet exit, like Test::Harness does.

If you look at Test::Harness::runtests, you can see that it constructs a very specific die() message. Under TAP::Harness::runtests, the summary output is generated by the formatter, which may or may not send information to the console. It may have similar information, or it may not. In short, there is no way for Module::Build to know whether any invocation of TAP::Harness provides any useful information or not, so providing a summary -- even redundant -- that matches that produced by Test::Harness would seem to have the "least surprised" for users (including automated tools parsing test output). That's my take on it, but I'm open to further discussion. The only absolute I have is that it must die(), not exit(), so it can be caught in eval() or a signal handler. -- David G

Mon Aug 31 08:01:18 2009 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Mon Aug 31 14:56:49 2009 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49080] [PATCH] 'use_tap_harness' should die on test failure
Date:	Mon, 31 Aug 2009 11:56:22 -0700
To:	bug-Module-Build [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Aug 30, 2009, at 9:44 PM, David Golden via RT wrote: Show quoted text

> If you look at Test::Harness::runtests, you can see that it > constructs a > very specific die() message. Under TAP::Harness::runtests, the > summary > output is generated by the formatter, which may or may not send > information to the console. It may have similar information, or it > may > not. In short, there is no way for Module::Build to know whether any > invocation of TAP::Harness provides any useful information or not, so > providing a summary -- even redundant -- that matches that produced by > Test::Harness would seem to have the "least surprised" for users > (including automated tools parsing test output). > > That's my take on it, but I'm open to further discussion. > > The only absolute I have is that it must die(), not exit(), so it > can be > caught in eval() or a signal handler.

Hrm. Might be worth asking for opinions on perl-qa@perl.org and/or tapx-dev@hexten.net . Die'ing is fine, I agree with that, but I'm less sure about the output. I think that it's the responsibility of all formatters to output an appropriate summary, but I'm not positive. Best, David

Mon Aug 31 14:56:50 2009 The RT System itself - Status changed from 'patched' to 'open'

Tue Sep 01 22:51:48 2009 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Mon Dec 21 13:51:33 2009 dagolden [...] cpan.org - Status changed from 'patched' to 'resolved'

Mon Dec 21 13:51:33 2009 dagolden [...] cpan.org - Fixed in 0.36 added