Thread: 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
Attachment
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
Attachment
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