PXGS vs TAP tests

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

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

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