Horton

Thread

Commits

  1. Fix prove_installcheck to use correct paths when used with PGXS

  1. PXGS vs TAP tests

    Andrew Dunstan <andrew@dunslane.net> — 2021-06-20T13:44:30Z

    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

  2. Re: PXGS vs TAP tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-20T14:45:21Z

    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

  3. Re: PXGS vs TAP tests

    Andrew Dunstan <andrew@dunslane.net> — 2021-06-20T14:56:40Z

    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

  4. Re: PXGS vs TAP tests

    Andrew Dunstan <andrew@dunslane.net> — 2021-06-20T17:24:04Z

    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

  5. Re: PXGS vs TAP tests

    Michael Paquier <michael@paquier.xyz> — 2021-06-22T00:23:42Z

    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

  6. Re: PXGS vs TAP tests

    Andrew Dunstan <andrew@dunslane.net> — 2021-07-01T13:13:25Z

    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

  7. Re: PXGS vs TAP tests

    Michael Paquier <michael@paquier.xyz> — 2021-07-02T00:10:30Z

    On Thu, Jul 01, 2021 at 09:13:25AM -0400, Andrew Dunstan wrote:
> OK, done.

Thanks!
--
Michael