Re: Handling of REGRESS_OPTS in MSVC for regression tests

On 11/26/18 12:43 AM, Michael Paquier wrote:
> Hi all,
>
> As mentioned on this thread, I have been fighting a bit with the
> buildfarm when trying to introduce new PGXS options for isolation and
> TAP tests:
> https://www.postgresql.org/message-id/E1gR4D0-0002Yj-Jw@gemulon.postgresql.org
>
> However it happens that we have more issues than the buildfarm failures
> to begin with.  One of them it related to the way REGRESS_OPTS is
> defined and handled across the whole tree for MSVC.
>
> There are in the tree now a couple of paths using top_builddir or
> top_srcdir:
> $ git grep REGRESS_OPTS | grep top
> contrib/dblink/Makefile:REGRESS_OPTS =
> --dlpath=$(top_builddir)/src/test/regress
> contrib/pg_stat_statements/Makefile:REGRESS_OPTS = --temp-config
> $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
> src/test/modules/commit_ts/Makefile:REGRESS_OPTS =
> --temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
> src/test/modules/test_rls_hooks/Makefile:REGRESS_OPTS =
> --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
>
> Using top_builddir for dblink is for example fine as this needs to point
> out to the place where builds of pg_regress are present.  However when
> it comes to configuration files we should use top_builddir, and ought to
> use top_srcdir instead as VPATH would complain about things gone
> missing.  So the definition that we make out of it is correct.  However
> there is an issue with MSVC which thinks that REGRESS_OPTS should only
> include top_builddir.  This is actually fine per-se as all MSVC tests
> run with make-installcheck, and not make-check, however I think that we
> should allow top_srcdir to be switched on-the-fly as much as
> top_builddir as top_srcdir could be used for something else.
>
> Another way worse issue is that MSVC scripts ignore some configuration
> values because of the way values of REGRESS_OPTS are parsed.  This
> causes some sets of regression tests to never run on Windows.  For
> example, here is what the command generated for pg_stat_statements, and
> what happens with it:
> c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/pg_regress/pg_regress \
>    --bindir=c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/psql \
>    --dbname=contrib_regression --temp-config pg_stat_statements
> [...]
> ============== running regression test queries        ==============
>
> =====================
>   All 0 tests passed.
> =====================
>
> This causes the tests to pass, but to run nothing as test list is eaten
> out as an option value for --temp-config.  In this case, what happens is
> that --temp-config is set to "pg_stat_statements", leaving the test list
> empty.  The same cannot happen with test_decoding as the buildfarm uses
> a custom module to run its tests (still this module could go away with
> PGXS options to control isolation tests).
>
> Attached is a patch which makes MSVC scripts more intelligent by being
> able to replace top_srcdir as well by topdir when fetching options.
>
> Thanks to that, MSVC is able to run the tests correctly, by building a
> proper command.  I think that this is a bug that should be back-patched,
> because the tests just don't run, and we likely would miss regressions
> because of that.
>
> Unfortunately, if I were to commit that directly, the buildfarm would
> turn immediately to red for all Windows members, because when checking
> module and contrib tests an "installcheck" happens, and
> shared_preload_libraries is not set in this case.  When working on the
> switch to add isolation and TAP test support to PGXS, test_decoding has
> been failing because the installed server did not have wal_level =
> logical set up, which is one instance of that.  The buildfarm code
> installing the Postgres instance on which the tests are run should
> configure that properly I think, and one tweak that we could use for the
> time being is to bypass tests of modules which cannot work yet, so as
> the buildfarm keeps being green.  This way, this would not be a blocker
> for the integration of the new PGXS infra for TAP and isolation.  And
> those could be enabled back once the buildfarm code is able to set up a
> proper configuration.  The following modules require some extra
> configuration:
> - commit_ts
> - test_rls_hooks
> - pg_stat_statements
> - test_decoding (if its Makefile is rewritten)
> - snapshot_too_old (if its Makefile is rewritten)
>
> The buildfarm part requires Andrew Dunstan's input, and there is a bit
> to sort out for the rest, so input from all is welcome.  Please note
> that I would prefer if the tests which just cannot work yet are
> disabled until the needed buildfarm infrastructure is needed.  Another
> option could also be to switch contribcheck and modulescheck so as they
> use a temporary installation, but that's a can of worms waiting to
> explode as MSVC makes more complicated the search for initdb and such
> (see the way upgradecheck happens for example), and this makes the tests
> waaay longer to run.
>

It's not the buildfarm that is broken. Both contribcheck() and 
modulescheck() in vcregress.pl run in installcheck mode, i.e. with an 
already running database. That makes the tests run faster, because 
setting up and breaking down instances is expensive, especially on 
Windows. We've got far too profligate with that in recent years, to the 
extent that some of my Windows buildfarm animals take many hours to 
complete full runs these days.

Note that TAP tests are not a problem here. It's up to the TAP scripts 
to set up and break down postgres instances they need, with whatever 
config options are required, such as shared preload libraries. It's only 
modules using pg_regress that could be a problem.

A simple way to proceed might be to have vcregress.pl honor the 
NO_INSTALLCHECK flag. Currently this is only used by pg_stat_statements, 
but we could add it anywhere required. In vcregress,pl, I would probably 
do this by having fetchTests() check for the flag and return an empty 
set of tests if it's found, and in turn have subdircheck() do nothing if 
it has an empty set of tests.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services