pgsql: Clean up after TAP tests in oid2name and vacuumlo.

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 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.

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

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