| 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 |
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__