Обсуждение: 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 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