Re: TAP tests are badly named - Mailing list pgsql-hackers

On Fri, Aug 14, 2015 at 09:31:43AM -0400, Andrew Dunstan wrote:
> On 08/14/2015 09:27 AM, Andrew Dunstan wrote:
> >The code
> >is rearranged a little, and an incorrect piece of code setting
> >$ENV{PERL5LIB} is fixed (in the case where it was previously empty it
> >would have added a spurious ";" and possibly caused a warning as well).

The PERL5LIB bit is clearly good, but please commit it separately.

> >Instead of looking everywhere in the tree for /t directories, the new
> >bincheck function only looks for them in src/bin.

Check.

> >And the tests would die
> >on the first failure, as we would also expect the checks run under "make"
> >to do.

"make" offers a choice.  If I had to settle for just one of its behaviors, I
would choose -k.  One can emulate "make -S" by killing "make -k" after the
first error, but there's no straightforward way to emulate "make -k" in terms
of "make -S".  Thus, I prefer the ancient vcregress decision in this area.  In
any event, a proposal to change it would need its own thread and a patch that
changes all targets facing this decision.

> >The effect is to remove the target "tapcheck" for which there is no "make"
> >equivalent, and replace it with the target "bincheck", which is the
> >equivalent of "make -C src/bin installcheck", which happens to be what the
> >buildfarm runs.

This "vcregress bincheck" differs from "make -C src/bin installcheck" by using
"check" semantics, and it differs from "make -C src/bin check" by skipping the
pg_upgrade test suite.  (I don't have any point in saying that beyond
clarifying the record.)

> -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

> +    # 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.