Mailing-List: contact test-dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: test-dev@httpd.apache.org
Message-ID: <3BE97DE0.4080903@stason.org>
Date: Thu, 08 Nov 2001 02:30:56 +0800
From: Stas Bekman <stas@stason.org>
Organization: Hope, Humanized
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.5) Gecko/20011012
MIME-Version: 1.0
To: test-dev@httpd.apache.org
Subject: Re: [patch take2] add test skipping reasoning
References: <Pine.LNX.4.21.0111070954050.27309-100000@localhost>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

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/