Thread: 9.6 TAP tests and extensions
Hi all While updating an extension for 9.6 I noticed that while the $(prove_check) definition is exposed for use by PGXS in Makefile.global, extensions can't actually use the TAP tests because we don't install the required Perl modules like PostgresNode.pm. I don't see any reason not to make this available to extension authors and doing so is harmless, so here's a small patch to install it. I think it's reasonable to add this to 9.6 even at this late stage; IMO it should've been installed from the beginning. They're only installed if --enable-tap-tests is set, since otherwise $(prove_check) will just error out with "TAP tests not enabled" anyway. Not having this in 9.6 will make it way harder for extension authors to benefit from the new TAP tooling. Another patch allows the isolation tester to be installed too, again so that extensions can use it. The final patch just adds a comment to src/test/Makefile to warn that src/Makefile doesn't call it directly, because I was confused as to why 'make -C src/test install' installed everything, but 'make install' did not. Sorry this is so late in the piece. It only came up when I switched from 10.0 dev/review to updating extensions for 9.6. But it's just adding installed files and I think it's worth doing. Another small patch (pending) will be needed because we look for pg_regress in the wrong place when running out-of-tree with $(prove_installcheck). Can't exec "/home/craig/projects/2Q/postgres-bdr-extension//home/craig/pg/96/lib/postgresql/pgxs/src/makefiles/../../src/test/regress/pg_regress": No such file or directory at /home/craig/pg/96/lib/postgresql/pgxs/src/makefiles/../../src/test/perl/TestLib.pm line 151. $(prove_check) won't be usable because it assumes a temp install in ./tmp_install but that's to be expected. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
This was wrong for out-of-tree builds, updated. Still pending fix for PG_REGRESS path when invoked using $(prove_check) from PGXS -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 13 September 2016 at 13:27, Craig Ringer <craig@2ndquadrant.com> wrote: > This was wrong for out-of-tree builds, updated. > > Still pending fix for PG_REGRESS path when invoked using > $(prove_check) from PGXS Looking further at this, I think a pgxs-specific patch to add support for prove tests and isolation tests would be best, and can be done separately. Possibly even snuck into a point release, but if not, at least extension authors can invoke prove in their own Makefile if the required modules get installed. It just needs an adaptation of the command used in the $(prove_check) definition. Extension makefiles run tests by listing the tests in REGRESS . Something similar would make sense for isolation checks. For prove, probably just a macro that can be invoked to enable prove tests in pgxs makefiles. I suggest that the above patches be applied to 9.6 and v10. Then for v10 I'll look at enhancing PGXS to make it easier to use isolation tests and prove tests; extensions that want to use them in 9.6 can just add something like: prove_check: rm -rf $(CURDIR)/tmp_check/log cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell pg_config --bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl .PHONY: prove_check to their Makefile , so it's not necessary to have PGXS support for this for it to be useful in 9.6. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 13 September 2016 at 14:36, Craig Ringer <craig@2ndquadrant.com> wrote: > > > prove_check: > rm -rf $(CURDIR)/tmp_check/log > cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell pg_config > --bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' > top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='pg_regress' > $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl > > .PHONY: prove_check Actually, that should be prove_check: rm -rf $(CURDIR)/tmp_check/log cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell $(PG_CONFIG) --bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(pgxsdir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl for a typical install. If PGXS defined that as a var that can be invoked with $(PGXS_PROVE) or something that'd be handy, but can be done later. It's trivial to do in an extension Makefile so long as the required files actually get installed. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer wrote: > I suggest that the above patches be applied to 9.6 and v10. Then for > v10 I don't object to patching 9.6 in this way, but kindly do not pollute this thread with future ideas on what to do on pg10, at least until the current release is sorted out. You'll only distract people from your present objective. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Craig Ringer <craig@2ndquadrant.com> writes:
> While updating an extension for 9.6 I noticed that while the
> $(prove_check) definition is exposed for use by PGXS in
> Makefile.global, extensions can't actually use the TAP tests because
> we don't install the required Perl modules like PostgresNode.pm.
> I don't see any reason not to make this available to extension authors
> and doing so is harmless, so here's a small patch to install it. I
> think it's reasonable to add this to 9.6 even at this late stage; IMO
> it should've been installed from the beginning.
Without taking a position on the merits of this patch per se, I'd like
to say that I find the argument for back-patching into 9.6 and not
further than that to be pretty dubious. $(prove_check) has been there
since 9.4, and in the past we've often regretted it when we failed
to back-patch TAP infrastructure fixes all the way back to 9.4.
Or to be concrete: how is an extension author, or more to the point an
extension Makefile, supposed to know whether it can use $(prove_check)?
How would this patch change that, and how would extension authors cope
with building against both patched and unpatched trees?
regards, tom lane
On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Craig Ringer <craig@2ndquadrant.com> writes: >> While updating an extension for 9.6 I noticed that while the >> $(prove_check) definition is exposed for use by PGXS in >> Makefile.global, extensions can't actually use the TAP tests because >> we don't install the required Perl modules like PostgresNode.pm. Whoops, I managed to misplace this thread. >> I don't see any reason not to make this available to extension authors >> and doing so is harmless, so here's a small patch to install it. I >> think it's reasonable to add this to 9.6 even at this late stage; IMO >> it should've been installed from the beginning. > > Without taking a position on the merits of this patch per se, I'd like > to say that I find the argument for back-patching into 9.6 and not > further than that to be pretty dubious. $(prove_check) has been there > since 9.4, and in the past we've often regretted it when we failed > to back-patch TAP infrastructure fixes all the way back to 9.4. No objection to backpatching, I just thought I'd be more intrusive to do that than just 9.6. Since 9.5 and older have more limited versions of PostgresNode which lack safe_psql, etc, I'm not sure it's very practical for extensions to bother running TAP tests on 9.4 and 9.5 anyway. I'd love to be able to, but unless we backport the new src/test/perl stuff and the changes to the rest of the TAP tests to make them work with it I don't see it being very useful. Since that's really not going to fly, I say this should just go in 9.6. Extension authors can just use: ifeq ($(MAJORVERSION),9.6) endif when defining their prove rules. Not beautiful, but when it wasn't really designed from the start to work with PGXS I don't see much alternative. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer <craig@2ndquadrant.com> writes:
> On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Without taking a position on the merits of this patch per se, I'd like
>> to say that I find the argument for back-patching into 9.6 and not
>> further than that to be pretty dubious. $(prove_check) has been there
>> since 9.4, and in the past we've often regretted it when we failed
>> to back-patch TAP infrastructure fixes all the way back to 9.4.
> No objection to backpatching, I just thought I'd be more intrusive to
> do that than just 9.6.
> Since 9.5 and older have more limited versions of PostgresNode which
> lack safe_psql, etc, I'm not sure it's very practical for extensions
> to bother running TAP tests on 9.4 and 9.5 anyway.
Certainly there are restrictions, but I'd imagine that every new release
will be adding features to the TAP test infrastructure for some time to
come. I think it's silly to claim that 9.6 is the first branch where
TAP testing is usable at all.
> Extension authors can just use:
> ifeq ($(MAJORVERSION),9.6)
> endif
> when defining their prove rules.
That will break as soon as 10 comes out. And numerical >= tests aren't
all that convenient in Make. It'd be much better if a test on whether
$(prove_check) is defined would be sufficient.
regards, tom lane
On 23 September 2016 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Without taking a position on the merits of this patch per se, I'd like
>>> to say that I find the argument for back-patching into 9.6 and not
>>> further than that to be pretty dubious. $(prove_check) has been there
>>> since 9.4, and in the past we've often regretted it when we failed
>>> to back-patch TAP infrastructure fixes all the way back to 9.4.
>
>> No objection to backpatching, I just thought I'd be more intrusive to
>> do that than just 9.6.
>
>> Since 9.5 and older have more limited versions of PostgresNode which
>> lack safe_psql, etc, I'm not sure it's very practical for extensions
>> to bother running TAP tests on 9.4 and 9.5 anyway.
>
> Certainly there are restrictions, but I'd imagine that every new release
> will be adding features to the TAP test infrastructure for some time to
> come. I think it's silly to claim that 9.6 is the first branch where
> TAP testing is usable at all.
OK.
I didn't intend to claim 9.6 is the first branch where it's usable,
just that the differences between 9.4/9.5 and 9.6 mean that supporting
both in extensions will likely be more pain than it is worth. Mainly
due to the safe_psql change in 2c83f435. It might've been good to
backport that, but it was pretty wide reaching and at the time I
wasn't thinking in terms of using TAP tests in extensions so it didn't
occur to me.
Extension Perl code can always use some adapter/glue code to handle
9.4 and 9.5 if they want, or error if they don't, so it's not a fatal
barrier.
Also, despite what I said upthread, there's no need for normal
PGXS-using extensions to define their $(prove_installcheck)
replacement. It works as-is. The problems I was having stemmed from
the fact that I was working with a pretty nonstandard Makefile without
realising that the changes were going to affect prove. $(prove_check)
isn't useful from extensions of course since it expects a temp install
and PGXS doesn't know how to make one, but $(prove_installcheck) does
the job fine.
It's thus sufficient to apply the patch to install the perl modules to
9.4, 9.5 and 9.6. Nothing else is needed. I've attached backports for
9.4 and 9.5.
If you were really keen, we could actually backport the new TAP code
to 9.4 and 9.5 pretty much wholesale. 9.4 and 9.5 don't have the psql
method, PostgresNode, etc, so there's nothing to break. But that's for
separate consideration.
>> Extension authors can just use:
>> ifeq ($(MAJORVERSION),9.6)
>> endif
>> when defining their prove rules.
>
> That will break as soon as 10 comes out. And numerical >= tests aren't
> all that convenient in Make. It'd be much better if a test on whether
> $(prove_check) is defined would be sufficient.
Fair enough. I forgot how much numeric range tests sucked in Make.
In that case we should backpatch the installation of the Perl modules
right back to 9.4. There's not even a need to test if
$(prove_installcheck) is defined then, just need a semicolon so it
evaluates to empty e.g.
prove_installcheck:
$(prove_installcheck) ;
check: prove_check
So ... patches attached. The 9.6 patch applies to head too.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Craig Ringer <craig@2ndquadrant.com> writes:
> On 23 September 2016 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Certainly there are restrictions, but I'd imagine that every new release
>> will be adding features to the TAP test infrastructure for some time to
>> come. I think it's silly to claim that 9.6 is the first branch where
>> TAP testing is usable at all.
> Also, despite what I said upthread, there's no need for normal
> PGXS-using extensions to define their $(prove_installcheck)
> replacement. It works as-is. The problems I was having stemmed from
> the fact that I was working with a pretty nonstandard Makefile without
> realising that the changes were going to affect prove. $(prove_check)
> isn't useful from extensions of course since it expects a temp install
> and PGXS doesn't know how to make one, but $(prove_installcheck) does
> the job fine.
> It's thus sufficient to apply the patch to install the perl modules to
> 9.4, 9.5 and 9.6. Nothing else is needed. I've attached backports for
> 9.4 and 9.5.
Pushed with cosmetic adjustments --- mostly, I followed the project
standard that makefiles are named Makefile. (We use a GNUmakefile
only in directories where it seems likely that non-developers would
invoke make by hand, and there's supposed to be a Makefile beside them
that throws an error whining about how you're not using GNU make.
I see no need for that in src/test/perl.)
Looking back over the thread, I see that you also proposed installing
isolationtester and pg_isolation_regress for the benefit of extensions.
I'm very much less excited about that idea. It'd be substantially more
dead weight in typical installations, and I'm not sure that it'd be useful
to common extensions, and I'm not eager to treat isolationtester's API
and behavior as something we need to hold stable for extension use.
regards, tom lane
<p dir="ltr"><p dir="ltr">On 24 Sep. 2016 04:04, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:.<br /> ><br /> > > <br /> > > It's thus sufficientto apply the patch to install the perl modules to<br /> > > 9.4, 9.5 and 9.6. Nothing else is needed. I'veattached backports for<br /> > > 9.4 and 9.5.<br /> ><br /> > Pushed with cosmetic adjustments --- <p dir="ltr">Thanks.<pdir="ltr">> Looking back over the thread, I see that you also proposed installing<br /> > isolationtesterand pg_isolation_regress for the benefit of extensions.<br /> > I'm very much less excited about that idea. It'd be substantially more<br /> > dead weight in typical installations, and I'm not sure that it'd be useful<br/> > to common extensions, and I'm not eager to treat isolationtester's API<br /> > and behavior as somethingwe need to hold stable for extension use.<p dir="ltr">Fine by me. I have no particular problem expecting those tobe installed explicitly by ext devs who want to use them.<br />
On 2016-09-23 16:04:32 -0400, Tom Lane wrote: > Looking back over the thread, I see that you also proposed installing > isolationtester and pg_isolation_regress for the benefit of extensions. > I'm very much less excited about that idea. It'd be substantially more > dead weight in typical installations, and I'm not sure that it'd be useful > to common extensions, and I'm not eager to treat isolationtester's API > and behavior as something we need to hold stable for extension use. FWIW, I'd be quite happy if it were installed. Running isolationtester when compiling extensions against distribution postgres packages would be quite useful. Andres
On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote: > On 2016-09-23 16:04:32 -0400, Tom Lane wrote: >> Looking back over the thread, I see that you also proposed installing >> isolationtester and pg_isolation_regress for the benefit of extensions. >> I'm very much less excited about that idea. It'd be substantially more >> dead weight in typical installations, and I'm not sure that it'd be useful >> to common extensions, and I'm not eager to treat isolationtester's API >> and behavior as something we need to hold stable for extension use. > > FWIW, I'd be quite happy if it were installed. Running isolationtester > when compiling extensions against distribution postgres packages would > be quite useful. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote: >> On 2016-09-23 16:04:32 -0400, Tom Lane wrote: >>> Looking back over the thread, I see that you also proposed installing >>> isolationtester and pg_isolation_regress for the benefit of extensions. >>> I'm very much less excited about that idea. It'd be substantially more >>> dead weight in typical installations, and I'm not sure that it'd be useful >>> to common extensions, and I'm not eager to treat isolationtester's API >>> and behavior as something we need to hold stable for extension use. >> >> FWIW, I'd be quite happy if it were installed. Running isolationtester >> when compiling extensions against distribution postgres packages would >> be quite useful. > > +1. Patch attached. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 14 November 2016 at 14:55, Craig Ringer <craig@2ndquadrant.com> wrote: > On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote: >>> On 2016-09-23 16:04:32 -0400, Tom Lane wrote: >>>> Looking back over the thread, I see that you also proposed installing >>>> isolationtester and pg_isolation_regress for the benefit of extensions. >>>> I'm very much less excited about that idea. It'd be substantially more >>>> dead weight in typical installations, and I'm not sure that it'd be useful >>>> to common extensions, and I'm not eager to treat isolationtester's API >>>> and behavior as something we need to hold stable for extension use. >>> >>> FWIW, I'd be quite happy if it were installed. Running isolationtester >>> when compiling extensions against distribution postgres packages would >>> be quite useful. >> >> +1. > > Patch attached. As Andres pointed out elsewhere this is only half the picture. It should really add PGXS support for ISOLATION_TESTS and bringing up the test harness too. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer wrote: > On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote: > >> On 2016-09-23 16:04:32 -0400, Tom Lane wrote: > >>> Looking back over the thread, I see that you also proposed installing > >>> isolationtester and pg_isolation_regress for the benefit of extensions. > >>> I'm very much less excited about that idea. It'd be substantially more > >>> dead weight in typical installations, and I'm not sure that it'd be useful > >>> to common extensions, and I'm not eager to treat isolationtester's API > >>> and behavior as something we need to hold stable for extension use. > >> > >> FWIW, I'd be quite happy if it were installed. Running isolationtester > >> when compiling extensions against distribution postgres packages would > >> be quite useful. > > > > +1. > > Patch attached. Hmm but this only installs isolationtester itself ... don't you need pg_isolation_regress too? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 25 November 2016 at 02:47, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Craig Ringer wrote: >> On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote: >> > On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote: >> >> On 2016-09-23 16:04:32 -0400, Tom Lane wrote: >> >>> Looking back over the thread, I see that you also proposed installing >> >>> isolationtester and pg_isolation_regress for the benefit of extensions. >> >>> I'm very much less excited about that idea. It'd be substantially more >> >>> dead weight in typical installations, and I'm not sure that it'd be useful >> >>> to common extensions, and I'm not eager to treat isolationtester's API >> >>> and behavior as something we need to hold stable for extension use. >> >> >> >> FWIW, I'd be quite happy if it were installed. Running isolationtester >> >> when compiling extensions against distribution postgres packages would >> >> be quite useful. >> > >> > +1. >> >> Patch attached. > > Hmm but this only installs isolationtester itself ... don't you need > pg_isolation_regress too? Yeah, as Andres pointed out offlist after I posted this. Meant to follow up but got side-tracked. It needs PGXS support to be properly useful. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services