You are viewing a plain text version of this content. The canonical link for it is here.
Posted to test-dev@httpd.apache.org by Stas Bekman <st...@stason.org> on 2001/11/07 08:30:44 UTC
[patch take2] add test skipping reasoning
this patch:
- prints the reason for the skipped test
I didn't want to complicate things, so I've changed the definition of what
a condition function should return to be:
if (true)
return 1;
else
return the reason as a string different from 1;
issues:
- Doug has mentioned that "missing foo" doesn't help much for c modules
because it doesn't explain the real reason, which can be:
o apxs is not available
o the module requires 2.0
o else
solution:
- first let's integrate this patch.
- second I suggest splitting have_module into have_module_c and
have_module_perl, or leave have_module as is for 'mod_*.c' but do add
have_module_perl.
consider:
plan ..., have_module 'constant';
for constant.pm. this will falsely satisfy the requirement with what we
have now if there is mod_constant.c and it's compiled, but constant.pm is
not available. There is no requirement for Perl modules to start with
uppercase letter.
- third IMHO tests shouldn't care about why their requirement is not
satisfied, thefore we shouldn't try to make them set the reason.
have_module() should figure out why some mod_*.c is not there. But that's
a next step and has nothing to do with this patch.
Index: Apache-Test/lib/Apache/Test.pm
===================================================================
RCS file: /home/cvs/httpd-test/perl-framework/Apache-Test/lib/Apache/Test.pm,v
retrieving revision 1.27
diff -u -r1.27 Test.pm
--- Apache-Test/lib/Apache/Test.pm 2001/10/20 10:35:33 1.27
+++ Apache-Test/lib/Apache/Test.pm 2001/11/07 06:50:34
@@ -67,10 +67,26 @@
test_pm_refresh();
}
-#caller will need to have required Apache::TestRequest
-*have_http11 = \&Apache::TestRequest::install_http11;
-*have_lwp = \&Apache::TestRequest::has_lwp;
+sub have_http11 {
+ require Apache::TestRequest;
+ if (Apache::TestRequest::install_http11()) {
+ return 1;
+ }
+ else {
+ return "LWP cannot handle HTTP 1.1";
+ }
+}
+sub have_lwp {
+ require Apache::TestRequest;
+ if (Apache::TestRequest::has_lwp()) {
+ return 1;
+ }
+ else {
+ return "must have LWP installed";
+ }
+}
+
sub plan {
init_test_pm(shift) if ref $_[0];
@@ -80,24 +96,31 @@
if (@_ % 2) {
my $condition = pop @_;
my $ref = ref $condition;
- my $meets_condition = 0;
+ my $status;
if ($ref) {
if ($ref eq 'CODE') {
#plan tests $n, \&has_lwp
- $meets_condition = $condition->();
+ $status = $condition->();
}
elsif ($ref eq 'ARRAY') {
#plan tests $n, [qw(php4 rewrite)];
- $meets_condition = have_module($condition);
+ $status = have_module($condition);
+ }
+ else {
+ die "don't know how to handle a condition of type $ref";
}
}
else {
- # we have the verdict already: true/false
- $meets_condition = $condition ? 1 : 0;
+ # we have the verdict already: 1 or reason
+ $status = $condition;
}
+
+ # this shouldn't happen, must be a broken test
+ $status = 'fix me' unless defined $status;
- unless ($meets_condition) {
- print "1..0\n";
+ # tryint to emulate a dual variable (ala errno)
+ unless (length($status) == 1 and $status == 1) {
+ print "1..0 # skipped: $status \n";
exit; #XXX: Apache->exit
}
}
@@ -119,20 +142,25 @@
die "bogus module name $_" unless /^[\w:.]+$/;
eval "require $_";
#print $@ if $@;
- return 0 if $@;
+ return "cannot find $_" if $@;
}
return 1;
}
sub have_cgi {
- [have_module('cgi') || have_module('cgid')];
+ have_module('cgi') || have_module('cgid');
}
sub have_apache {
my $version = shift;
my $cfg = Apache::Test::config();
- $cfg->{server}->{rev} == $version;
+ if ($cfg->{server}->{rev} == $version) {
+ return 1;
+ }
+ else {
+ return "need apache $version, but this is $cfg->{server}->{rev}";
+ }
}
sub have_perl {
@@ -141,7 +169,7 @@
for my $key ($thing, "use$thing") {
return 1 if $Config{$key} and $Config{$key} eq 'define';
}
- return 0;
+ return "Perl was built with neither $thing nor use$thing";
}
package Apache::TestToString;
Index: t/apache/byterange.t
===================================================================
RCS file: /home/cvs/httpd-test/perl-framework/t/apache/byterange.t,v
retrieving revision 1.2
diff -u -r1.2 byterange.t
--- t/apache/byterange.t 2001/09/10 17:12:37 1.2
+++ t/apache/byterange.t 2001/11/07 06:50:34
@@ -25,7 +25,8 @@
my %other_files;
-plan tests => @pods + keys(%other_files), sub { $perlpod };
+plan tests => @pods + keys(%other_files),
+ sub { $perlpod ? 1 : "dir $vars->{perlpod} doesn't exist"};
for my $url (keys %other_files) {
verify($url, $other_files{$url});
Index: t/apache/getfile.t
===================================================================
RCS file: /home/cvs/httpd-test/perl-framework/t/apache/getfile.t,v
retrieving revision 1.5
diff -u -r1.5 getfile.t
--- t/apache/getfile.t 2001/09/10 17:12:37 1.5
+++ t/apache/getfile.t 2001/11/07 06:50:34
@@ -20,7 +20,8 @@
("/getfiles-binary-$_", $vars->{$_})
} qw(httpd perl);
-plan tests => @pods + keys(%other_files), sub { $perlpod };
+plan tests => @pods + keys(%other_files),
+ sub { $perlpod ? 1 : "dir $vars->{perlpod} doesn't exist"};
my $location = "/getfiles-perl-pod";
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [patch take2] add test skipping reasoning
Posted by Doug MacEachern <do...@covalent.net>.
On Thu, 8 Nov 2001, Stas Bekman wrote:
> So what do you suggest to write as a reason when mod_foo is not
> available? As you've mentioned before there could be many reasons (apxs
> undef'ed, v2 vs v1, etc) and they are definitely not the job of the test
> to know.
you can do this:
my $reason = $config->{cmodules_disabled}->{'mod_foo.c'} ||
'mod_foo.c is not installed';
if mod_foo.c is one in c-modules/ the reason will either be 'version x
required' or 'no apxs configured'
> what's the point of 'require mod_foo'?
nothing does that. it does fall through to 'require foo', i don't see the
harm in that. maybe one does want to check for a "pragma" module that is
only on CPAN: have_module('enum') will work. i don't see any conflict
between perl names and apache mod_ names. but we can worry about that
later.
> that runs the $condition twice. How about:
>
> my $condition = sub { $perlpod };
> plan tests => @pods + keys(%other_files), skip_unless($condition, "reason");
yeah, that's better.
Re: [patch take2] add test skipping reasoning
Posted by Stas Bekman <st...@stason.org>.
Doug MacEachern wrote:
> On Thu, 8 Nov 2001, Stas Bekman wrote:
>
>
>>that won't work. The condition argument must return a single value, or
>>it'll mess up the magic we have at the end of %args Test::plan expects.
>>
>
> right. there's got to be a better solution to this than have to
> check if the return value length == 1.
we could cook an errno like tied dual variable, which will test
true/false on numerical test and give the string on "" op. Or would that
be too much of an overkill?
>>Originally I've suggested to return a ref to a hash, since it wasn't
>>used yet. {$meets_condition => "failure reason"}.
>>
>
> that would still break expr if have_module(...);
that's correct, I was thinking only about plan(). So it's not good.
>>can you please explain why do you want to mix the two? What similarity
>>the two types have?
>>
>
> like in t/ssl/all.t:
> plan tests => 1, [$vars->{ssl_module_name}, qw(LWP::Protocol::https)];
>
> and t/modules/dav.t:
> plan tests => 14, [qw(dav HTTP::DAV)];
>
> don't want to have to write:
> plan tests => 1, [have_module_c($vars->{ssl_module_name}),
> have_module_perl('LWP::Protocol::https')];
>
> plan tests => 14, [have_module_c('dav'), have_module_perl('HTTP::DAV')];
Now I understand, thanks.
So what do you suggest to write as a reason when mod_foo is not
available? As you've mentioned before there could be many reasons (apxs
undef'ed, v2 vs v1, etc) and they are definitely not the job of the test
to know.
In any case as you say the Perl modules aren't expected to start from
lowercase letter, therefore it should be at least
if (/^a...
c case
else
perl case
what's the point of 'require mod_foo'?
>>I guess I don't understand you. What you have suggested earlier is to
>>add a special variable, so tests can set it if they have a reason to fail.
>>
>>First this requires changing tests to set the reason.
>>
>
> either way, as you've shown, requires a change to the test to set the
> reason. doesn't have to be a special variable, the point in the message
> was simply that tests will need to be able to describe reasons why tests
> are being skipped.
Oh, of course, I absolutely agree here.
>>2. have a special class variable:
>>$Test::FailureReason which can be set by any of have_foo functions on
>>failure and allows the test writer to set it as well.
>> This will allow us to keep tests simple and let have_foo subs to
>>report the reason, without taking away the flexibility of being able to
>>set this variable from within the test.
>>
>
> since the majority of tests will only skip tests based on have_foo not
> being available, how about this. an @Apache::Test::SkipReasons array,
> where have_foo inside will push @SkipReasons, "..."; if foo is not
> available. then for the few special cases such as t/apache/getfile.t:
>
> my $condition = sub { $perlpod };
>
> Apache::Test::skip_reason("no .pod files in $vars->{perlpod}",
> $condition);
>
> plan tests => @pods + keys(%other_files), $condition;
that runs the $condition twice. How about:
my $condition = sub { $perlpod };
plan tests => @pods + keys(%other_files), skip_unless($condition, "reason");
> then skip_reason does this:
>
> while (my($reason, $condition) = splice @_, 0, 2) {
> push @SkipReasons, $reason unless $condition->();
> }
and skip_unless() will take care of pushing the reason into @SkipReasons
if the condition has failed. This doesn't expose yet another sub into
the tests.
> plan() in this case will see that $condition->() fails and the
> reason(s) will already be present in @SkipReasons.
in my suggested case skip_unless() returns true/false, no need to rerun
the condition.
> i just don't want to loose the current functionality of:
>
> plan tests => $n, [qw(foo Bar::Baz)];
>
> and
>
> if (have_module('Foo::Bar')) {
> ...
> }
sure, I understand now.
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [patch take2] add test skipping reasoning
Posted by Doug MacEachern <do...@covalent.net>.
On Thu, 8 Nov 2001, Stas Bekman wrote:
> that won't work. The condition argument must return a single value, or
> it'll mess up the magic we have at the end of %args Test::plan expects.
right. there's got to be a better solution to this than have to
check if the return value length == 1.
> Originally I've suggested to return a ref to a hash, since it wasn't
> used yet. {$meets_condition => "failure reason"}.
that would still break expr if have_module(...);
> can you please explain why do you want to mix the two? What similarity
> the two types have?
like in t/ssl/all.t:
plan tests => 1, [$vars->{ssl_module_name}, qw(LWP::Protocol::https)];
and t/modules/dav.t:
plan tests => 14, [qw(dav HTTP::DAV)];
don't want to have to write:
plan tests => 1, [have_module_c($vars->{ssl_module_name}),
have_module_perl('LWP::Protocol::https')];
plan tests => 14, [have_module_c('dav'), have_module_perl('HTTP::DAV')];
> I guess I don't understand you. What you have suggested earlier is to
> add a special variable, so tests can set it if they have a reason to fail.
>
> First this requires changing tests to set the reason.
either way, as you've shown, requires a change to the test to set the
reason. doesn't have to be a special variable, the point in the message
was simply that tests will need to be able to describe reasons why tests
are being skipped.
> 2. have a special class variable:
> $Test::FailureReason which can be set by any of have_foo functions on
> failure and allows the test writer to set it as well.
> This will allow us to keep tests simple and let have_foo subs to
> report the reason, without taking away the flexibility of being able to
> set this variable from within the test.
since the majority of tests will only skip tests based on have_foo not
being available, how about this. an @Apache::Test::SkipReasons array,
where have_foo inside will push @SkipReasons, "..."; if foo is not
available. then for the few special cases such as t/apache/getfile.t:
my $condition = sub { $perlpod };
Apache::Test::skip_reason("no .pod files in $vars->{perlpod}",
$condition);
plan tests => @pods + keys(%other_files), $condition;
then skip_reason does this:
while (my($reason, $condition) = splice @_, 0, 2) {
push @SkipReasons, $reason unless $condition->();
}
plan() in this case will see that $condition->() fails and the
reason(s) will already be present in @SkipReasons.
i just don't want to loose the current functionality of:
plan tests => $n, [qw(foo Bar::Baz)];
and
if (have_module('Foo::Bar')) {
...
}
Re: [patch take2] add test skipping reasoning
Posted by Stas Bekman <st...@stason.org>.
Doug MacEachern wrote:
> On Wed, 7 Nov 2001, Stas Bekman wrote:
>
>
>>this patch:
>>- prints the reason for the skipped test
>>
>>I didn't want to complicate things, so I've changed the definition of what
>>a condition function should return to be:
>>
>> if (true)
>> return 1;
>> else
>> return the reason as a string different from 1;
>>
>
> please don't do that. just return 2 values if something wants to know the
> reason, something like:
>
> my $available = eval { require $module };
> my $reason = $available ? "ok" : "$module is not installed";
>
> return wantarray ? ($available, $reason) : $available;
>
> or have it always return a list:
> return ($reason, $available);
that won't work. The condition argument must return a single value, or
it'll mess up the magic we have at the end of %args Test::plan expects.
Originally I've suggested to return a ref to a hash, since it wasn't
used yet. {$meets_condition => "failure reason"}.
> and:
> my $ok = have_foo();
>
> will get the value of available.
>
> that way expr if have_foo; can still be used without having to check the
> length of the return value.
>
>
>>solution:
>>- first let's integrate this patch.
>>- second I suggest splitting have_module into have_module_c and
>>have_module_perl, or leave have_module as is for 'mod_*.c' but do add
>>have_module_perl.
>>
>>consider:
>>
>> plan ..., have_module 'constant';
>>
>>for constant.pm. this will falsely satisfy the requirement with what we
>>have now if there is mod_constant.c and it's compiled, but constant.pm is
>>not available. There is no requirement for Perl modules to start with
>>uppercase letter.
>>
>
> only Perl pragma modules start with a lowercase letter. i don't see this
> ever being a problem and would much rather continue to be able to mix Perl
> and C modules in a single have_module(...) call. if you really see
> the need, just add support to have_module so you can say
> have_module('constant.pm') so it will only check for a Perl module.
can you please explain why do you want to mix the two? What similarity
the two types have?
>>- third IMHO tests shouldn't care about why their requirement is not
>>satisfied, thefore we shouldn't try to make them set the reason.
>>
>
> then why does your patch do that? for something like a module, i would
> agree, but there will be cases such as this part of you patch below where
> only the test will know the reason why the it is skipping.
this is a special example where the patch doesn't demand its requirement
from the core system and therefore has to handle it by itself. And this
is fine, we want the non-special cases to be handled by the core system,
while allowing to have special cases.
>>--- t/apache/byterange.t 2001/09/10 17:12:37 1.2
>>+++ t/apache/byterange.t 2001/11/07 06:50:34
>>@@ -25,7 +25,8 @@
>>
>> my %other_files;
>>
>>-plan tests => @pods + keys(%other_files), sub { $perlpod };
>>+plan tests => @pods + keys(%other_files),
>>+ sub { $perlpod ? 1 : "dir $vars->{perlpod} doesn't exist"};
I guess I don't understand you. What you have suggested earlier is to
add a special variable, so tests can set it if they have a reason to fail.
First this requires changing tests to set the reason.
Second what's the point of the plan() extension magic then? If the test
says:
plan %foo, [qw(mod_a mod_b perl_mod)];
How the test can set the reason if it doesn't know which of the three
modules above will fail? unless it runs these before calling plan().
Currently I see two solutions:
1. per my suggestion, each have_foo returns:
{$meets_condition => "failure reason"}.
2. have a special class variable:
$Test::FailureReason which can be set by any of have_foo functions on
failure and allows the test writer to set it as well.
This will allow us to keep tests simple and let have_foo subs to
report the reason, without taking away the flexibility of being able to
set this variable from within the test.
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [patch take2] add test skipping reasoning
Posted by Doug MacEachern <do...@covalent.net>.
On Wed, 7 Nov 2001, Stas Bekman wrote:
> this patch:
> - prints the reason for the skipped test
>
> I didn't want to complicate things, so I've changed the definition of what
> a condition function should return to be:
>
> if (true)
> return 1;
> else
> return the reason as a string different from 1;
please don't do that. just return 2 values if something wants to know the
reason, something like:
my $available = eval { require $module };
my $reason = $available ? "ok" : "$module is not installed";
return wantarray ? ($available, $reason) : $available;
or have it always return a list:
return ($reason, $available);
and:
my $ok = have_foo();
will get the value of available.
that way expr if have_foo; can still be used without having to check the
length of the return value.
> solution:
> - first let's integrate this patch.
> - second I suggest splitting have_module into have_module_c and
> have_module_perl, or leave have_module as is for 'mod_*.c' but do add
> have_module_perl.
>
> consider:
>
> plan ..., have_module 'constant';
>
> for constant.pm. this will falsely satisfy the requirement with what we
> have now if there is mod_constant.c and it's compiled, but constant.pm is
> not available. There is no requirement for Perl modules to start with
> uppercase letter.
only Perl pragma modules start with a lowercase letter. i don't see this
ever being a problem and would much rather continue to be able to mix Perl
and C modules in a single have_module(...) call. if you really see
the need, just add support to have_module so you can say
have_module('constant.pm') so it will only check for a Perl module.
> - third IMHO tests shouldn't care about why their requirement is not
> satisfied, thefore we shouldn't try to make them set the reason.
then why does your patch do that? for something like a module, i would
agree, but there will be cases such as this part of you patch below where
only the test will know the reason why the it is skipping.
> --- t/apache/byterange.t 2001/09/10 17:12:37 1.2
> +++ t/apache/byterange.t 2001/11/07 06:50:34
> @@ -25,7 +25,8 @@
>
> my %other_files;
>
> -plan tests => @pods + keys(%other_files), sub { $perlpod };
> +plan tests => @pods + keys(%other_files),
> + sub { $perlpod ? 1 : "dir $vars->{perlpod} doesn't exist"};