pgsql: Clean up after TAP tests in oid2name and vacuumlo.
Clean up after TAP tests in oid2name and vacuumlo.
Oversights in commits 1aaf532de and bfea331a5. Unlike the case for
traditional-style REGRESS tests, pgxs.mk doesn't have any builtin support
for TAP tests, so it doesn't realize it should remove tmp_check/.
Maybe we should build some actual pgxs infrastructure for TAP tests ...
but for the moment, just remove explicitly.
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/f30c6f523f9caa73c9ba6ebd82c8d29fe45866a3
Modified Files
--------------
contrib/oid2name/Makefile | 2 ++
contrib/vacuumlo/Makefile | 2 ++
2 files changed, 4 insertions(+)
On Tue, Sep 04, 2018 at 02:52:07PM +0000, Tom Lane wrote:
Clean up after TAP tests in oid2name and vacuumlo.
Oversights in commits 1aaf532de and bfea331a5. Unlike the case for
traditional-style REGRESS tests, pgxs.mk doesn't have any builtin support
for TAP tests, so it doesn't realize it should remove tmp_check/.
Maybe we should build some actual pgxs infrastructure for TAP tests ...
but for the moment, just remove explicitly.
Thanks for fixing this. I think that there is an argument for just
moving the cleanup of $(pg_regress_clean_files) in pgxs.mk out of the
REGRESS portion instead? I see little need for new infrastructure
here.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Sep 04, 2018 at 02:52:07PM +0000, Tom Lane wrote:
Oversights in commits 1aaf532de and bfea331a5. Unlike the case for
traditional-style REGRESS tests, pgxs.mk doesn't have any builtin support
for TAP tests, so it doesn't realize it should remove tmp_check/.
Maybe we should build some actual pgxs infrastructure for TAP tests ...
but for the moment, just remove explicitly.
Thanks for fixing this. I think that there is an argument for just
moving the cleanup of $(pg_regress_clean_files) in pgxs.mk out of the
REGRESS portion instead? I see little need for new infrastructure
here.
It's really accidental that $(pg_regress_clean_files) happens to be a
superset of what the TAP tests need to have cleaned; we shouldn't build
that assumption in further.
If we're gonna do anything here, I think it'd be better to invent some new
symbol like HAVE_TAP_TESTS for calling Makefiles to define, then move all
the support into pgxs.mk, including the prove_[install]check rules.
regards, tom lane
On Tue, Sep 04, 2018 at 01:41:53PM -0400, Tom Lane wrote:
It's really accidental that $(pg_regress_clean_files) happens to be a
superset of what the TAP tests need to have cleaned; we shouldn't build
that assumption in further.
Fine for me.
If we're gonna do anything here, I think it'd be better to invent some new
symbol like HAVE_TAP_TESTS for calling Makefiles to define, then move all
the support into pgxs.mk, including the prove_[install]check rules.
Okay, I don't want to create a dependency between REGRESS and
HAVE_TAP_TESTS either, but modules specifying both should be able to
trigger both regressions and tap tests. So I would be inclined to
create two new rules, say check-regress and installcheck-regress, which
are invoked if check is called, and trigger pg_regress stuff. Then add
on top of it the existing prove-check and prove-installcheck. What do
you think? check and installcheck become this way the centralized place
for all types of test suites.
This would clean up src/test/modules/test_pg_dump/Makefile as well.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
Okay, I don't want to create a dependency between REGRESS and
HAVE_TAP_TESTS either, but modules specifying both should be able to
trigger both regressions and tap tests.
Agreed ...
So I would be inclined to
create two new rules, say check-regress and installcheck-regress, which
are invoked if check is called, and trigger pg_regress stuff. Then add
on top of it the existing prove-check and prove-installcheck. What do
you think? check and installcheck become this way the centralized place
for all types of test suites.
Why would we invent a different target name? I was thinking something
roughly like
check: submake $(REGRESS_PREP)
ifdef REGRESS
$(pg_regress_check) $(REGRESS_OPTS) $(REGRESS)
endif
ifdef TAP_TESTS
$(prove_check)
endif
although getting it to print a useful response when neither symbol
is set would require complicating things a bit. Still, as long as
there's just one copy of this rule, messiness isn't a big problem.
regards, tom lane
On Tue, Sep 04, 2018 at 03:16:55PM -0400, Tom Lane wrote:
Why would we invent a different target name? I was thinking something
roughly likecheck: submake $(REGRESS_PREP)
ifdef REGRESS
$(pg_regress_check) $(REGRESS_OPTS) $(REGRESS)
endif
ifdef TAP_TESTS
$(prove_check)
endifalthough getting it to print a useful response when neither symbol
is set would require complicating things a bit. Still, as long as
there's just one copy of this rule, messiness isn't a big problem.
OK. I have dug into that, and finished with the attached. What do you
think? One thing is that the definition of submake is moving out of
REGRESS, and .PHONY gets defined.
--
Michael
Attachments:
tap-pgxs.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/oid2name/Makefile b/contrib/oid2name/Makefile
index 908e078714..361a80a7a1 100644
--- a/contrib/oid2name/Makefile
+++ b/contrib/oid2name/Makefile
@@ -6,11 +6,11 @@ PGAPPICON = win32
PROGRAM = oid2name
OBJS = oid2name.o $(WIN32RES)
+TAP_TESTS = 1
+
PG_CPPFLAGS = -I$(libpq_srcdir)
PG_LIBS_INTERNAL = $(libpq_pgport)
-EXTRA_CLEAN = tmp_check
-
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -21,9 +21,3 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
-
-check:
- $(prove_check)
-
-installcheck:
- $(prove_installcheck)
diff --git a/contrib/vacuumlo/Makefile b/contrib/vacuumlo/Makefile
index 5de506151e..3efcb46735 100644
--- a/contrib/vacuumlo/Makefile
+++ b/contrib/vacuumlo/Makefile
@@ -6,11 +6,11 @@ PGAPPICON = win32
PROGRAM = vacuumlo
OBJS = vacuumlo.o $(WIN32RES)
+TAP_TESTS = 1
+
PG_CPPFLAGS = -I$(libpq_srcdir)
PG_LIBS_INTERNAL = $(libpq_pgport)
-EXTRA_CLEAN = tmp_check
-
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -21,9 +21,3 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
-
-check:
- $(prove_check)
-
-installcheck:
- $(prove_installcheck)
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index d5731621e7..4759d09823 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1283,6 +1283,15 @@ include $(PGXS)
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>TAP_TESTS</varname></term>
+ <listitem>
+ <para>
+ switch defining if TAP tests need to be run
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><varname>NO_INSTALLCHECK</varname></term>
<listitem>
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 8deb356958..453b117dc5 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -44,6 +44,7 @@
# listed in MODULES or MODULE_big
# REGRESS -- list of regression test cases (without suffix)
# REGRESS_OPTS -- additional switches to pass to pg_regress
+# TAP_TESTS -- switch to enable TAP tests
# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
# tests require special configuration, or don't use pg_regress
# EXTRA_CLEAN -- extra files to remove in 'make clean'
@@ -305,6 +306,9 @@ ifeq ($(PORTNAME), win)
rm -f regress.def
endif
endif # REGRESS
+ifdef TAP_TESTS
+ rm -rf tmp_check/
+endif
ifdef MODULE_big
clean: clean-lib
@@ -339,6 +343,7 @@ $(test_files_build): $(abs_builddir)/%: $(srcdir)/%
$(MKDIR_P) $(dir $@)
ln -s $< $@
endif # VPATH
+endif # REGRESS
.PHONY: submake
submake:
@@ -346,21 +351,32 @@ ifndef PGXS
$(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
endif
-# against installed postmaster
+# Standard rules to run regression tests including multiple test suites.
+# Runs against an installed postmaster
ifndef NO_INSTALLCHECK
installcheck: submake $(REGRESS_PREP)
+ifdef REGRESS
$(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)
endif
+ifdef TAP_TESTS
+ $(prove_installcheck)
+endif
+endif # NO_INSTALLCHECK
+# Runs independently of any installation
ifdef PGXS
check:
@echo '"$(MAKE) check" is not supported.'
@echo 'Do "$(MAKE) install", then "$(MAKE) installcheck" instead.'
else
check: submake $(REGRESS_PREP)
+ifdef REGRESS
$(pg_regress_check) $(REGRESS_OPTS) $(REGRESS)
endif
-endif # REGRESS
+ifdef TAP_TESTS
+ $(prove_check)
+endif
+endif # PGXS
ifndef NO_TEMP_INSTALL
temp-install: EXTRA_INSTALL+=$(subdir)
diff --git a/src/test/modules/test_pg_dump/Makefile b/src/test/modules/test_pg_dump/Makefile
index c64b353707..6123b994f6 100644
--- a/src/test/modules/test_pg_dump/Makefile
+++ b/src/test/modules/test_pg_dump/Makefile
@@ -7,6 +7,7 @@ EXTENSION = test_pg_dump
DATA = test_pg_dump--1.0.sql
REGRESS = test_pg_dump
+TAP_TESTS = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
@@ -18,8 +19,3 @@ top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
-
-check: prove-check
-
-prove-check: | temp-install
- $(prove_check)
On 2018-Sep-04, Michael Paquier wrote:
OK. I have dug into that, and finished with the attached. What do you
think? One thing is that the definition of submake is moving out of
REGRESS, and .PHONY gets defined.
Should this be used in src/test/modules/{brin,test_commit_ts} also?
Why do you guys design Makefile rules in pgsql-committers?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 05, 2018 at 11:39:50AM -0300, Alvaro Herrera wrote:
Should this be used in src/test/modules/{brin,test_commit_ts} also?
Hmm, I have not thought those two ones. commit_ts relies on REGRESS to
be defined so as it does its cleanup. brin is more interesting, it
directly quotes that it needs to tweak its Makefile to do the cleanup,
and it uses isolation tests. Wouldn't it be more adapted to add a
second extra switch for isolation tests similar to REGRESS? We could
call it ISOLATION, and it would list the set of isolation tests you
could run from an extension. That would be useful for a lot of folks.
Thoughts? It would be better to start a new thread rather than posting
a new patch, but I'd rather take the temperature first.
Why do you guys design Makefile rules in pgsql-committers?
That was a bad idea, and a reaction to what Tom has committed for the
cleanup of the new TAP tests I have added previously.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Sep 05, 2018 at 11:39:50AM -0300, Alvaro Herrera wrote:
Should this be used in src/test/modules/{brin,test_commit_ts} also?
Hmm, I have not thought those two ones. commit_ts relies on REGRESS to
be defined so as it does its cleanup. brin is more interesting, it
directly quotes that it needs to tweak its Makefile to do the cleanup,
and it uses isolation tests. Wouldn't it be more adapted to add a
second extra switch for isolation tests similar to REGRESS? We could
call it ISOLATION, and it would list the set of isolation tests you
could run from an extension. That would be useful for a lot of folks.
Thoughts? It would be better to start a new thread rather than posting
a new patch, but I'd rather take the temperature first.
Yeah, if we're going to introduce infrastructure for TAP tests, it'd
make sense to do so for isolation tests as well.
regards, tom lane
On 2018-Sep-05, Michael Paquier wrote:
On Wed, Sep 05, 2018 at 11:39:50AM -0300, Alvaro Herrera wrote:
Should this be used in src/test/modules/{brin,test_commit_ts} also?
Hmm, I have not thought those two ones. commit_ts relies on REGRESS to
be defined so as it does its cleanup. brin is more interesting, it
directly quotes that it needs to tweak its Makefile to do the cleanup,
and it uses isolation tests. Wouldn't it be more adapted to add a
second extra switch for isolation tests similar to REGRESS? We could
call it ISOLATION, and it would list the set of isolation tests you
could run from an extension. That would be useful for a lot of folks.Thoughts?
Yeah, +1 for that.
It would be better to start a new thread rather than posting
a new patch, but I'd rather take the temperature first.
Slightly feverish, it appears.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 05, 2018 at 03:20:03PM -0300, Alvaro Herrera wrote:
On 2018-Sep-05, Michael Paquier wrote:
It would be better to start a new thread rather than posting
a new patch, but I'd rather take the temperature first.Slightly feverish, it appears.
As far as I can see, the proposal is not cold to death and could have a
future, so I'll begin a new thread with a more complete patch.
--
Michael
On Wed, Sep 05, 2018 at 01:05:42PM -0700, Michael Paquier wrote:
As far as I can see, the proposal is not cold to death and could have a
future, so I'll begin a new thread with a more complete patch.
Done. Let's move the discussion here then:
/messages/by-id/20180906014849.GG2726@paquier.xyz
--
Michael