Skip Menu |
 

This queue is for tickets about the Test-Harness CPAN distribution.

Report information
The Basics
Id: 33609
Status: resolved
Priority: 0/
Queue: Test-Harness

People
Owner: Nobody in particular
Requestors: jkeenan [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in:
  • 3.05
  • 3.06
  • 3.07
  • 3.08
  • 3.09
Fixed in: (no value)



Subject: Design flaw in prove and App::Prove
Andy(s) & company: Attached is a patch intended to remedy what I believe is a design flaw in Test-Harness-3*'s bin/prove and lib/App/Prove.pm. In Test-Harness-2, 'prove' was described as a simple wrapper around Test::Harness:runtests(). If you had need to use 'prove' inside another program, you could wrap it in a 'system' call and test its exit value. If, however, you did not want to make a system call at that point, you could swap in runtests() and simply follow it up with 'or die ...' instead of the 'and die ...' required after the system call. Actual use case: I was using a system call to 'prove' inside a module in the Parrot distribution. chromatic criticized this as limiting portability. I disagreed at first but was ultimately persuaded to use runtests(). In a revised version of that same module in the Parrot distribution (http://svn.perl.org/parrot/branches/tcif/lib/Parrot/Configure/Options/Test.pm at r26081), I am using the new version of 'prove' from Test-Harness-3.05+ which permits you to pass command-line arguments which follow a '::' to the underlying tests. if ( $self->get_run('run_configure_tests') ) { print "As you requested, we'll start with some tests of the configuration tools.\n\n"; my $optstr = $self->get_all_options(); system( qq{$self->{this_prove} @preconfiguration_tests :: $optstr} ) and die "Pre-configuration tests did not complete successfully; Configure.pl will not continue."; Anticipating the same objection as previously, I want to replace my use of 'prove' (contained in $self->{this_prove}) with the underlying code from App::Prove, which is described as the implementation of 'prove'. When I tried to do so, however, I discovered that I could not achieve the intended result. Subsituting $app->process_args() and $app->run, I found that I was always exiting the calling program (Parrot's Configure.pl) instead of proceeding to other computations. Eventually I realized that the problem was this sequence: 1. $app->run, when it DWIMs, internally calls: $self->_runtests( $self->_get_args, $self- Show quoted text
>_get_tests );
2. _runtests(), when it DWIMs, internally calls: $self->_exit( $aggregator->has_problems ? 1 : 0 ); 3. That _exit method is coded like this: sub _exit { exit( $_[1] || 0 ) } In other words, buried 3 levels down inside App::Prove there lies a call to what is ultimately the most powerful command in Perl 5: exit(). So App::Prove is saying, "Stop everything and return to the shell." It is *not*, in its current formulation, returning a value to the 'prove' executable and letting 'prove' handle the 'exit' call. As a result, anything using App::Prove directly is going to terminate by returning to the shell -- which means that I can't embed it in any other Perl program (at least not easily). Based on my experience in refactoring many of the Parrot configuration and build tools written in Perl 5, I believe it is not a good idea for Perl modules to contain direct 'exit' calls. The modules should return a value to their caller programs -- whether executables or test scripts -- and let the caller programs decide whether or not to exit or what value to exit with. And -- though this is not the main point at issue here -- I also recommend that Perl subroutines return a true value when good, intended things happen and return a false value (undef, 0 or q{}, depending on circumstances) when bad, unintended things happen. I believe that Perl's notion of truth and falsehood should percolate up to the top-level program and that only that program -- the one that is intended to speak to the shell -- should translate Perl truth into system truth. Accordingly, I have written a relatively small patch which eliminates the 'exit' call from lib/App/Prove.pm and has App::Prove::run() return a Perl true value to 'bin/prove' when it DWIMs. 'bin/prove' then handles the 'exit' call. One test file had to be fixed a bit to get it to pass under 'make test'. Please evaluate, as I would like to use this revised App::Prove in Parrot. Thank you very much. Jim Keenan
Subject: diff.app.prove.txt
Download diff.app.prove.txt
text/plain 1.6k
Index: t/proverun.t =================================================================== --- t/proverun.t (revision 74) +++ t/proverun.t (revision 75) @@ -61,12 +61,6 @@ return $self; } -sub _exit { - my $self = shift; - push @{ $self->{_log} }, [ '_exit', @_ ]; - die "Exited"; -} - sub get_log { my $self = shift; my @log = @{ $self->{_log} }; @@ -144,7 +138,8 @@ # Why does this make the output from the test spew out of # our STDOUT? eval { $app->run }; - like $@, qr{Exited}, "$name: exited via _exit()"; + is($@, q{}, + "run() returned true value -- prerequisite to successful 'prove'"); my @log = get_log(); Index: lib/App/Prove.pm =================================================================== --- lib/App/Prove.pm (revision 74) +++ lib/App/Prove.pm (revision 75) @@ -236,8 +236,6 @@ return; } -sub _exit { exit( $_[1] || 0 ) } - sub _help { my ( $self, $verbosity ) = @_; @@ -406,10 +404,8 @@ local $ENV{TEST_VERBOSE} = 1 if $self->verbose; - $self->_runtests( $self->_get_args, $self->_get_tests ); + return $self->_runtests( $self->_get_args, $self->_get_tests ); } - - return; } sub _get_tests { @@ -440,9 +436,7 @@ my $aggregator = $harness->runtests(@tests); - $self->_exit( $aggregator->has_problems ? 1 : 0 ); - - return; + $aggregator->has_problems ? 0 : 1; } sub _get_switches { Index: bin/prove =================================================================== --- bin/prove (revision 74) +++ bin/prove (revision 75) @@ -5,7 +5,7 @@ my $app = App::Prove->new; $app->process_args(@ARGV); -$app->run; +( $app->run ) ? exit(0) : exit(1); __END__
Download (untitled) / with headers
text/plain 301b
Hi Jim, I agree - it was a bad design choice. I've applied your patch with a couple of small changes. Unfortunately I've just released 3.10 tonight and I'd rather not release 3.11 so soon after - but it'll almost certainly be released within the next week or so. Thanks for the patch and feedback.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.