# New Ticket Created by James Keenan
# Please include the string: [perl #48633]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=48633 >
If at first you don't succeed ...
The patch attached refactors config/inter/progs.pm for the purpose of
increasing its testability.
As config/inter/progs.pm currently stands, its runstep() method does
two distinct things:
(1) It looks up basic values for compiler, linker, etc and/or
provides interactive prompts for the user to provide those values,
and then adds those values to the Parrot::Configure object.
(2) It then runs a basic test of the compiler using the values for
compiler, linker, etc., found in the Parrot::Configure object.
The tests which I wrote starting in June for this configuration step
class test runstep() as a whole and work most of the time for most
Parrot developers, notwithstanding the fact that my tests hard-coded
some values for responses to the prompts and then passed those values
on to the internal test_compiler() sub. But Andy Dougherty reported
failures which led me to try a couple of different ways of testing
this class.
The patch attached takes an approach which I have learned over the
past few months in writing tests for other step classes later in the
configuration sequence. That is to refactor the guts out of runstep
() into internal subroutines and then test those internal subs
directly. Doing so enables me to test the Perl 5 components of the
step classes -- which is my objective -- and to sidestep tests that
are essentially C components of those classes. So, in the patch
attached I've refactored the guts out of inter::progs::runstep() and
tested those guts directly and have completely sidestepped testing
what I can't test: the test_compiler() subroutine.
These tests are greatly facilitated by using
IO::CaptureOutput::capture() to capture verbose output, responses to
prompts, etc. Today we merged into trunk a branch in which Alan
Rocker and I were replacing previous instances of
Parrot::IO::Capture::Mini with IO::CaptureOutput.
Please review and try this out on different OSes. Once I can get
Andy D. to okay this, I'll apply it to trunk. Thank you very much.
kid51
Index: MANIFEST
===================================================================
--- MANIFEST (revision 23891)
+++ MANIFEST (working copy)
@@ -1,7 +1,7 @@
# ex: set ro:
# $Id$
#
-# generated by tools/dev/mk_manifest_and_skip.pl Fri Dec 14 15:44:44 2007 UT
+# generated by tools/dev/mk_manifest_and_skip.pl Sat Dec 15 00:19:22 2007 UT
#
# See tools/dev/install_files.pl for documentation on the
# format of this file.
@@ -3089,6 +3089,7 @@
t/configure/107-inter_progs-02.t []
t/configure/107-inter_progs-03.t []
t/configure/107-inter_progs-04.t []
+t/configure/107-inter_progs-05.t []
t/configure/108-inter_make.t []
t/configure/109-inter_lex-01.t []
t/configure/109-inter_lex-02.t []
Index: t/configure/107-inter_progs-03.t
===================================================================
--- t/configure/107-inter_progs-03.t (revision 23891)
+++ t/configure/107-inter_progs-03.t (working copy)
@@ -6,8 +6,7 @@
use strict;
use warnings;
-# Please leave as 'no_plan'; see 'BUG' in POD.
-use Test::More qw(no_plan); # tests => 23;
+use Test::More tests => 24;
use Carp;
use lib qw( lib t/configure/testlib );
use_ok('config::init::defaults');
@@ -18,6 +17,7 @@
use Parrot::Configure::Options qw( process_options );
use Parrot::Configure::Test qw( test_step_thru_runstep);
use Tie::Filehandle::Preempt::Stdin;
+use IO::CaptureOutput qw| capture |;
=for hints_for_testing Testing and refactoring of inter::progs should
entail understanding of issues discussed in the following RT tickets:
@@ -55,17 +55,17 @@
isa_ok( $step, $step_name );
ok( $step->description(), "$step_name has description" );
-my ( @prompts, $object );
+my @prompts;
foreach my $p (
qw|
- cc
- link
- ld
- ccflags
- linkflags
- ldflags
- libs
- cxx
+ cc
+ link
+ ld
+ ccflags
+ linkflags
+ ldflags
+ libs
+ cxx
|
)
{
@@ -73,17 +73,26 @@
}
push @prompts, q{n};
-$object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
+my $object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
can_ok( 'Tie::Filehandle::Preempt::Stdin', ('READLINE') );
isa_ok( $object, 'Tie::Filehandle::Preempt::Stdin' );
-{
- open STDOUT, '>', "/dev/null" or croak "Unable to open to myout";
- $ret = $step->runstep($conf);
- close STDOUT or croak "Unable to close after myout";
- ok( defined $ret, "$step_name runstep() returned defined value" );
-}
+my ($stdout, $debug, $debug_validity);
+capture( sub {
+ my $verbose = inter::progs::_get_verbose($conf);
+ my $ask = inter::progs::_prepare_for_interactivity($conf);
+ my $cc;
+ ($conf, $cc) = inter::progs::_get_programs($conf, $verbose, $ask);
+ $debug = inter::progs::_get_debug($conf, $ask);
+ $debug_validity = inter::progs::_is_debug_setting_valid($debug);
+}, \$stdout);
+ok( defined $debug_validity, "'debug_validity' set as expected" );
+capture( sub {
+ $conf = inter::progs::_set_debug_and_warn($conf, $debug);
+}, \$stdout);
+ok( defined $conf, "Components of $step_name runstep() tested okay" );
+
$object = undef;
untie *STDIN;
@@ -105,23 +114,6 @@
The tests in this file test subroutines exported by config::inter::progs.
-=head1 BUG
-
-This file tests the case where the C<--ask> option is specified and,
-hence, the user must respond to a prompt. The response to the prompt is
-supplied automatically via Tie::Filehandle::Preempt::Stdin. But that
-response is made via C<STDOUT>. A user generally would not like to see
-that output when running this test, say, via C<prove -v>. So the data
-written to C<STDOUT> must be captured rather than displayed.
-
-In other test files we can do that with Parrot::IO::Capture::Mini. We
-cannot do that here because there is extensive manipulation of C<STDOUT>
-deep inside the code being tested. The solution employed in this test
-successfully disposes of information printed to C<STDOUT>, but it
-confuses Test::Harness's count of tests run. This would cause the file
-as a whole to be reported as having failed, when in fact every single
-test passed. As a compromise, we run the file with C<no_plan>.
-
=head1 AUTHOR
James E Keenan
Index: t/configure/107-inter_progs-04.t
===================================================================
--- t/configure/107-inter_progs-04.t (revision 23891)
+++ t/configure/107-inter_progs-04.t (working copy)
@@ -55,17 +55,17 @@
isa_ok( $step, $step_name );
ok( $step->description(), "$step_name has description" );
-my ( @prompts, $object );
+my @prompts;
foreach my $p (
qw|
- cc
- link
- ld
- ccflags
- linkflags
- ldflags
- libs
- cxx
+ cc
+ link
+ ld
+ ccflags
+ linkflags
+ ldflags
+ libs
+ cxx
|
)
{
@@ -73,18 +73,15 @@
}
push @prompts, q{0};
-$object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
+my $object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
can_ok( 'Tie::Filehandle::Preempt::Stdin', ('READLINE') );
isa_ok( $object, 'Tie::Filehandle::Preempt::Stdin' );
-{
- my ($ret, $stdout);
- capture(
- sub { $ret = $step->runstep($conf); },
- \$stdout,
- );
- ok( !defined $ret, "$step_name runstep() returned defined value" );
-}
+my ($stdout, $rv);
+capture( sub {
+ $rv = $step->runstep($conf);
+}, \$stdout);
+ok( ! defined $rv, "$step_name runstep returned undef as expected" );
$object = undef;
untie *STDIN;
@@ -105,25 +102,10 @@
The files in this directory test functionality used by F<Configure.pl>.
-The tests in this file test subroutines exported by config::inter::progs.
+The tests in this file test config::inter::progs in the case where interactive
+configuration is requested and an inappropriate response to the debugging
+prompt is provided.
-=head1 BUG
-
-This file tests the case where the C<--ask> option is specified and,
-hence, the user must respond to a prompt. The response to the prompt is
-supplied automatically via Tie::Filehandle::Preempt::Stdin. But that
-response is made via C<STDOUT>. A user generally would not like to see
-that output when running this test, say, via C<prove -v>. So the data
-written to C<STDOUT> must be captured rather than displayed.
-
-In other test files we can do that with Parrot::IO::Capture::Mini. We
-cannot do that here because there is extensive manipulation of C<STDOUT>
-deep inside the code being tested. The solution employed in this test
-successfully disposes of information printed to C<STDOUT>, but it
-confuses Test::Harness's count of tests run. This would cause the file
-as a whole to be reported as having failed, when in fact every single
-test passed. As a compromise, we run the file with C<no_plan>.
-
=head1 AUTHOR
James E Keenan
Index: t/configure/107-inter_progs-01.t
===================================================================
--- t/configure/107-inter_progs-01.t (revision 23891)
+++ t/configure/107-inter_progs-01.t (working copy)
@@ -6,10 +6,8 @@
use strict;
use warnings;
-# Please leave as 'no_plan'; see 'BUG' in POD.
-use Test::More qw(no_plan); # tests => 24;
+use Test::More qw(no_plan); # tests => 21;
use Carp;
-use Data::Dumper;
use lib qw( lib t/configure/testlib );
use_ok('config::init::defaults');
use_ok('config::init::install');
@@ -19,6 +17,7 @@
use Parrot::Configure::Options qw( process_options );
use Parrot::Configure::Test qw( test_step_thru_runstep);
use Tie::Filehandle::Preempt::Stdin;
+use IO::CaptureOutput qw| capture |;
=for hints_for_testing Testing and refactoring of inter::progs should
entail understanding of issues discussed in the following RT tickets:
@@ -56,17 +55,17 @@
isa_ok( $step, $step_name );
ok( $step->description(), "$step_name has description" );
-my ( @prompts, $object );
+my @prompts;
foreach my $p (
qw|
- cc
- link
- ld
- ccflags
- linkflags
- ldflags
- libs
- cxx
+ cc
+ link
+ ld
+ ccflags
+ linkflags
+ ldflags
+ libs
+ cxx
|
)
{
@@ -74,17 +73,26 @@
}
push @prompts, q{y};
-$object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
+my $object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
can_ok( 'Tie::Filehandle::Preempt::Stdin', ('READLINE') );
isa_ok( $object, 'Tie::Filehandle::Preempt::Stdin' );
-{
- open STDOUT, '>', "/dev/null" or croak "Unable to open to myout";
- $ret = $step->runstep($conf);
- close STDOUT or croak "Unable to close after myout";
- ok( defined $ret, "$step_name runstep() returned defined value" );
-}
+my ($stdout, $debug, $debug_validity);
+capture( sub {
+ my $verbose = inter::progs::_get_verbose($conf);
+ my $ask = inter::progs::_prepare_for_interactivity($conf);
+ my $cc;
+ ($conf, $cc) = inter::progs::_get_programs($conf, $verbose, $ask);
+ $debug = inter::progs::_get_debug($conf, $ask);
+ $debug_validity = inter::progs::_is_debug_setting_valid($debug);
+}, \$stdout);
+ok( defined $debug_validity, "'debug_validity' set as expected" );
+capture( sub {
+ $conf = inter::progs::_set_debug_and_warn($conf, $debug);
+}, \$stdout);
+ok( defined $conf, "Components of $step_name runstep() tested okay" );
+
$object = undef;
untie *STDIN;
@@ -106,23 +114,6 @@
The tests in this file test subroutines exported by config::inter::progs.
-=head1 BUG
-
-This file tests the case where the C<--ask> option is specified and,
-hence, the user must respond to a prompt. The response to the prompt is
-supplied automatically via Tie::Filehandle::Preempt::Stdin. But that
-response is made via C<STDOUT>. A user generally would not like to see
-that output when running this test, say, via C<prove -v>. So the data
-written to C<STDOUT> must be captured rather than displayed.
-
-In other test files we can do that with Parrot::IO::Capture::Mini. We
-cannot do that here because there is extensive manipulation of C<STDOUT>
-deep inside the code being tested. The solution employed in this test
-successfully disposes of information printed to C<STDOUT>, but it
-confuses Test::Harness's count of tests run. This would cause the file
-as a whole to be reported as having failed, when in fact every single
-test passed. As a compromise, we run the file with C<no_plan>.
-
=head1 AUTHOR
James E Keenan
Index: t/configure/107-inter_progs-05.t
===================================================================
--- t/configure/107-inter_progs-05.t (revision 0)
+++ t/configure/107-inter_progs-05.t (revision 0)
@@ -0,0 +1,123 @@
+#! perl
+# Copyright (C) 2007, The Perl Foundation.
+# $Id: 107-inter_progs-05.t 23588 2007-12-08 14:31:02Z jkeenan $
+# 107-inter_progs-05.t
+
+use strict;
+use warnings;
+
+use Test::More tests => 23;
+use Carp;
+use lib qw( lib t/configure/testlib );
+use_ok('config::init::defaults');
+use_ok('config::init::install');
+use_ok('config::init::hints');
+use_ok('config::inter::progs');
+use Parrot::Configure;
+use Parrot::Configure::Options qw( process_options );
+use Parrot::Configure::Test qw( test_step_thru_runstep);
+use IO::CaptureOutput qw| capture |;
+
+=for hints_for_testing Testing and refactoring of inter::progs should
+entail understanding of issues discussed in the following RT tickets:
+http://rt.perl.org/rt3/Ticket/Display.html?id=43174;
+http://rt.perl.org/rt3/Ticket/Display.html?id=43173; and
+http://rt.perl.org/rt3/Ticket/Display.html?id=41168. You will have to
+determine a way to test a user response to a prompt.
+
+=cut
+
+my $args = process_options(
+ {
+ argv => [ ],
+ mode => q{configure},
+ }
+);
+
+my $conf = Parrot::Configure->new;
+
+test_step_thru_runstep( $conf, q{init::defaults}, $args );
+test_step_thru_runstep( $conf, q{init::install}, $args );
+test_step_thru_runstep( $conf, q{init::hints}, $args );
+
+my ( $task, $step_name, $step, $ret );
+my $pkg = q{inter::progs};
+
+$conf->add_steps($pkg);
+$conf->options->set( %{$args} );
+
+$task = $conf->steps->[3];
+$step_name = $task->step;
+
+$step = $step_name->new();
+ok( defined $step, "$step_name constructor returned defined value" );
+isa_ok( $step, $step_name );
+ok( $step->description(), "$step_name has description" );
+
+my ($stdout, $debug, $debug_validity);
+
+# Am setting 'verbose' here rather than above in process_options() so that the
+# steps run by test_step_thru_runstep() do not run verbosely. This hack may
not
+# be needed once we get rid of test_step_thru_runstep().
+$conf->options->set('verbose' => 1);
+
+# Setting below is solely to boost coverage. Since we are not running
+# inter::progs::test_compiler(), this will be okay.
+if ( $conf->data->get_p5('OSNAME') =~ /VMS|MSWin/ ) {
+ $conf->data->set_p5( OSNAME => q{linux} );
+} else {
+ $conf->data->set_p5( OSNAME => q{MSWin} );
+}
+
+my $verbose;
+capture( sub {
+ $verbose = inter::progs::_get_verbose($conf);
+ my $ask = inter::progs::_prepare_for_interactivity($conf);
+ my $cc;
+ ($conf, $cc) = inter::progs::_get_programs($conf, $verbose, $ask);
+ $debug = inter::progs::_get_debug($conf, $ask);
+ $debug_validity = inter::progs::_is_debug_setting_valid($debug);
+}, \$stdout);
+ok( defined $debug_validity, "'debug_validity' set as expected" );
+ok($verbose, "'verbose' set as expected");
+
+capture( sub {
+ $conf = inter::progs::_set_debug_and_warn($conf, $debug);
+}, \$stdout);
+ok( defined $conf, "Components of $step_name runstep() tested okay" );
+
+pass("Completed all tests in $0");
+
+################### DOCUMENTATION ###################
+
+=head1 NAME
+
+107-inter_progs-05.t - test config::inter::progs
+
+=head1 SYNOPSIS
+
+ % prove t/configure/107-inter_progs-05.t
+
+=head1 DESCRIPTION
+
+The files in this directory test functionality used by F<Configure.pl>.
+
+The tests in this file test config::inter::progs when interactive
+configuration is not requested, but verbose output is.
+
+=head1 AUTHOR
+
+James E Keenan
+
+=head1 SEE ALSO
+
+config::inter::progs, F<Configure.pl>.
+
+=cut
+
+# Local Variables:
+# mode: cperl
+# cperl-indent-level: 4
+# fill-column: 100
+# End:
+# vim: expandtab shiftwidth=4:
Property changes on: t/configure/107-inter_progs-05.t
___________________________________________________________________
Name: svn:mime-type
+ text/plain
Name: svn:keywords
+ Author Date Id Revision
Name: svn:eol-style
+ native
Index: t/configure/107-inter_progs-02.t
===================================================================
--- t/configure/107-inter_progs-02.t (revision 23891)
+++ t/configure/107-inter_progs-02.t (working copy)
@@ -6,8 +6,7 @@
use strict;
use warnings;
-# Please leave as 'no_plan'; see 'BUG' in POD.
-use Test::More qw(no_plan); # tests => 23;
+use Test::More tests => 24;
use Carp;
use lib qw( lib t/configure/testlib );
use_ok('config::init::defaults');
@@ -18,6 +17,7 @@
use Parrot::Configure::Options qw( process_options );
use Parrot::Configure::Test qw( test_step_thru_runstep);
use Tie::Filehandle::Preempt::Stdin;
+use IO::CaptureOutput qw| capture |;
=for hints_for_testing Testing and refactoring of inter::progs should
entail understanding of issues discussed in the following RT tickets:
@@ -55,17 +55,17 @@
isa_ok( $step, $step_name );
ok( $step->description(), "$step_name has description" );
-my ( @prompts, $object );
+my @prompts;
foreach my $p (
qw|
- cc
- link
- ld
- ccflags
- linkflags
- ldflags
- libs
- cxx
+ cc
+ link
+ ld
+ ccflags
+ linkflags
+ ldflags
+ libs
+ cxx
|
)
{
@@ -73,20 +73,28 @@
}
push @prompts, q{y};
-$object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
+my $object = tie *STDIN, 'Tie::Filehandle::Preempt::Stdin', @prompts;
can_ok( 'Tie::Filehandle::Preempt::Stdin', ('READLINE') );
isa_ok( $object, 'Tie::Filehandle::Preempt::Stdin' );
-{
- open STDOUT, '>', "/dev/null" or croak "Unable to open to myout";
- $ret = $step->runstep($conf);
- close STDOUT or croak "Unable to close after myout";
- ok( defined $ret, "$step_name runstep() returned defined value" );
-}
+my ($stdout, $debug, $debug_validity);
+capture( sub {
+ my $verbose = inter::progs::_get_verbose($conf);
+ my $ask = inter::progs::_prepare_for_interactivity($conf);
+ my $cc;
+ ($conf, $cc) = inter::progs::_get_programs($conf, $verbose, $ask);
+ $debug = inter::progs::_get_debug($conf, $ask);
+ $debug_validity = inter::progs::_is_debug_setting_valid($debug);
+}, \$stdout);
+ok( defined $debug_validity, "'debug_validity' set as expected" );
+capture( sub {
+ $conf = inter::progs::_set_debug_and_warn($conf, $debug);
+}, \$stdout);
+ok( defined $conf, "Components of $step_name runstep() tested okay" );
+
$object = undef;
untie *STDIN;
-
pass("Completed all tests in $0");
################### DOCUMENTATION ###################
@@ -105,23 +113,6 @@
The tests in this file test subroutines exported by config::inter::progs.
-=head1 BUG
-
-This file tests the case where the C<--ask> option is specified and,
-hence, the user must respond to a prompt. The response to the prompt is
-supplied automatically via Tie::Filehandle::Preempt::Stdin. But that
-response is made via C<STDOUT>. A user generally would not like to see
-that output when running this test, say, via C<prove -v>. So the data
-written to C<STDOUT> must be captured rather than displayed.
-
-In other test files we can do that with Parrot::IO::Capture::Mini. We
-cannot do that here because there is extensive manipulation of C<STDOUT>
-deep inside the code being tested. The solution employed in this test
-successfully disposes of information printed to C<STDOUT>, but it
-confuses Test::Harness's count of tests run. This would cause the file
-as a whole to be reported as having failed, when in fact every single
-test passed. As a compromise, we run the file with C<no_plan>.
-
=head1 AUTHOR
James E Keenan
Index: config/inter/progs.pm
===================================================================
--- config/inter/progs.pm (revision 23891)
+++ config/inter/progs.pm (working copy)
@@ -35,11 +35,37 @@
sub runstep {
my ( $self, $conf ) = @_;
+ my $verbose = _get_verbose($conf);
+
+ my $ask = _prepare_for_interactivity($conf);
+
+ my $cc;
+ ($conf, $cc) = _get_programs($conf, $verbose, $ask);
+
+ my $debug = _get_debug($conf, $ask);
+
+ my $debug_validity = _is_debug_setting_valid($debug);
+ return unless defined $debug_validity;
+
+ $conf = _set_debug_and_warn($conf, $debug);
+
+ # Beware! Inside test_compiler(), cc_build() and cc_run() both silently
+ # reference the Parrot::Configure object ($conf) at its current state.
+ # Cage cleaner alert: cf RT .
+ test_compiler($cc);
+
+ return 1;
+}
+
+sub _get_verbose {
+ my $conf = shift;
my $verbose = $conf->options->get('verbose');
print "\n" if $verbose;
+ return $verbose;
+}
- my ( $cc, $cxx, $link, $ld, $ccflags, $ccwarn, $linkflags, $ldflags,
$libs, $lex, $yacc );
-
+sub _prepare_for_interactivity {
+ my $conf = shift;
my $ask = $conf->options->get('ask');
if ($ask) {
print <<'END';
@@ -53,10 +79,14 @@
END
}
+ return $ask;
+}
+sub _get_programs {
+ my ($conf, $verbose, $ask) = @_;
# Set each variable individually so that hints files can use them as
# triggers to help pick the correct defaults for later answers.
-
+ my ( $cc, $cxx, $link, $ld, $ccflags, $linkflags, $ldflags, $libs, $lex,
$yacc );
$cc = integrate( $conf->data->get('cc'), $conf->options->get('cc') );
$cc = prompt( "What C compiler do you want to use?", $cc )
if $ask;
@@ -106,15 +136,25 @@
$cxx = integrate( $conf->data->get('cxx'), $conf->options->get('cxx') );
$cxx = prompt( "What C++ compiler do you want to use?", $cxx ) if $ask;
$conf->data->set( cxx => $cxx );
+ return ($conf, $cc);
+}
+sub _get_debug {
+ my ($conf, $ask) = @_;
my $debug = 'n';
$debug = 'y' if $conf->options->get('debugging');
$debug = prompt( "Do you want a debugging build of Parrot?", $debug )
if $ask;
- unless ( $debug =~ /^[yn]$/i ) {
- return;
- }
+ return $debug;
+}
+sub _is_debug_setting_valid {
+ my $debug = shift;
+ ( $debug =~ /^[yn]$/i ) ? return 1 : return;
+}
+
+sub _set_debug_and_warn {
+ my ($conf, $debug) = @_;
if ( $debug =~ /n/i ) {
$conf->data->set(
cc_debug => '',
@@ -124,12 +164,9 @@
}
# This one isn't prompted for above. I don't know why.
- $ccwarn = integrate( $conf->data->get('ccwarn'),
$conf->options->get('ccwarn') );
+ my $ccwarn = integrate( $conf->data->get('ccwarn'),
$conf->options->get('ccwarn') );
$conf->data->set( ccwarn => $ccwarn );
-
- test_compiler($cc);
-
- return 1;
+ return $conf;
}
sub test_compiler {
