Re: TAP tests are badly named

On 08/16/2015 02:23 PM, Noah Misch wrote:
>> -sub tapcheck
>> +sub tap_check
>>   {
>> -    InstallTemp();
>> +    die "Tap tests not enabled in configuration"
>> +      unless $config->{tap_tests};
> I endorse Heikki's argument for having omitted the configuration flag:
> http://www.postgresql.org/message-id/55B90161.5090506@iki.fi


That argument is not correct.  None of the tap tests can be run via make 
if --enable-tap-tests is not set. This doesn't just apply to the 
check-world target as Heikki asserted. Have a look at the definitions of 
prove_check and prove_installcheck in src/Makefile.global.in if you 
don't believe me.

>
>> +    # Reset those values, they may have been changed by another test.
>> +    # XXX is this true?
>> +    local %ENV = %ENV;
>>       $ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
>>       $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
>>   
>> +    $ENV{TESTDIR} = "$dir";
> The comment pertained only to the TESTDIR environment variable.  Adding the
> local %ENV is unnecessary.  I think you can just delete the comment; there's
> nothing noteworthy about setting a different TESTDIR value for each suite.
> The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, though
> keeping them here seems good for the benefit of future TAP targets.
>


The local decl is clearly needed. Otherwise repeated invocations of the 
function will result in repeated prepending of $topdir/src/test/perl to 
PERL5LIB. It's incredibly cheap anyway. And as you say, this is a much 
better place to do that than in bincheck. If you prefer, I could 
dispense with the local and instead only set to PERL5LIB conditionally. 
It's just a matter of style.

cheers

andrew