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
