Thread: PXGS vs TAP tests
The recipe for running TAP tests in src/Makefile.global doesn't work for
the PGXS case. If you try it you get something like this:
andrew@emma:tests $ make PG_CONFIG=../inst.head.5701/bin/pg_config installcheck
rm -rf '/home/andrew/pgl/tests'/tmp_check
/usr/bin/mkdir -p '/home/andrew/pgl/tests'/tmp_check
cd ./ && TESTDIR='/home/andrew/pgl/tests' PATH="/home/andrew/pgl/inst.head.5701/bin:$PATH" PGPORT='65701' \
top_builddir='/home/andrew/pgl/tests//home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../..' \
PG_REGRESS='/home/andrew/pgl/tests//home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../../src/test/regress/pg_regress'
\
REGRESS_SHLIB='/src/test/regress/regress.so' \
/usr/bin/prove -I /home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../../src/test/perl/ -I ./
t/*.pl
Notice those bogus settings for top_builddir, PG_REGRESS and
REGRESS_SHLIB. The attached patch fixes this bug. With it you can get by
with a Makefile as simple as this for running TAP tests under PGXS:
TAP_TESTS = 1
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's
not clear to me why we need it in a TAP test recipe at all. Certainly
it's not installed anywhere in a standard install so it seems entirely
bogus for the PGXS case.
This seems like a bug fix that should be patched all the way back,
although I haven't yet investigated the back branches.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment
Andrew Dunstan <andrew@dunslane.net> writes:
> The recipe for running TAP tests in src/Makefile.global doesn't work for
> the PGXS case. If you try it you get something like this:
> ...
> Notice those bogus settings for top_builddir, PG_REGRESS and
> REGRESS_SHLIB. The attached patch fixes this bug.
OK, but does the 'prove_check' macro need similar adjustments?
> I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's
> not clear to me why we need it in a TAP test recipe at all.
After some digging in the git history, it looks like it's there because
of Noah's c09850992, which makes me wonder whether 017_shm.pl requires
it. If so, it'd make more sense perhaps for that one test script
to set up the environment variable than to have it cluttering every TAP
run.
(In any case, please don't push this till after beta2 is tagged.
We don't need possible test instability right now.)
regards, tom lane
On 6/20/21 10:45 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> The recipe for running TAP tests in src/Makefile.global doesn't work for >> the PGXS case. If you try it you get something like this: >> ... >> Notice those bogus settings for top_builddir, PG_REGRESS and >> REGRESS_SHLIB. The attached patch fixes this bug. > OK, but does the 'prove_check' macro need similar adjustments? No, PGXS doesn't support 'make check'. In the case of TAP tests it really doesn't matter - you're not going to be running against a started server anyway. > >> I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's >> not clear to me why we need it in a TAP test recipe at all. > After some digging in the git history, it looks like it's there because > of Noah's c09850992, which makes me wonder whether 017_shm.pl requires > it. If so, it'd make more sense perhaps for that one test script > to set up the environment variable than to have it cluttering every TAP > run. Yeah, I'll do some testing. > > (In any case, please don't push this till after beta2 is tagged. > We don't need possible test instability right now.) > > Yes, of course. cheers andre -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 6/20/21 10:56 AM, Andrew Dunstan wrote: > On 6/20/21 10:45 AM, Tom Lane wrote: > >>> I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's >>> not clear to me why we need it in a TAP test recipe at all. >> After some digging in the git history, it looks like it's there because >> of Noah's c09850992, which makes me wonder whether 017_shm.pl requires >> it. If so, it'd make more sense perhaps for that one test script >> to set up the environment variable than to have it cluttering every TAP >> run. > > Yeah, I'll do some testing. > > > Tests pass with the attached patch, which puts the setting in the Makefile for the recovery tests. The script itself doesn't need any changing. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
On Sun, Jun 20, 2021 at 01:24:04PM -0400, Andrew Dunstan wrote: > Tests pass with the attached patch, which puts the setting in the > Makefile for the recovery tests. The script itself doesn't need any > changing. +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) +export REGRESS_SHLIB It may be better to add a comment here explaning why REGRESS_SHLIB is required in this Makefile then? While on it, could we split those commands into multiple lines and reduce the noise of future diffs? Something as simple as that would make those prove commands easier to follow: +cd $(srcdir) && TESTDIR='$(CURDIR)' \ + $(with_temp_install) \ + PGPORT='6$(DEF_PGPORT)' \ + PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \ + REGRESS_SHLIB= '$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' \ + $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) There are other places where this could happen, but the TAP commands are particularly long. -- Michael
Attachment
On 6/21/21 8:23 PM, Michael Paquier wrote: > On Sun, Jun 20, 2021 at 01:24:04PM -0400, Andrew Dunstan wrote: >> Tests pass with the attached patch, which puts the setting in the >> Makefile for the recovery tests. The script itself doesn't need any >> changing. > +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) > +export REGRESS_SHLIB > It may be better to add a comment here explaning why REGRESS_SHLIB is > required in this Makefile then? > > While on it, could we split those commands into multiple lines and > reduce the noise of future diffs? Something as simple as that would > make those prove commands easier to follow: > +cd $(srcdir) && TESTDIR='$(CURDIR)' \ > + $(with_temp_install) \ > + PGPORT='6$(DEF_PGPORT)' \ > + PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \ > + REGRESS_SHLIB= '$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' \ > + $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) > > There are other places where this could happen, but the TAP commands > are particularly long. OK, done. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Jul 01, 2021 at 09:13:25AM -0400, Andrew Dunstan wrote: > OK, done. Thanks! -- Michael