Обсуждение: Writing new unit tests with PostgresNode
Hi all
(BTW, the ssl tests don't seem to use a bunch of the facilities provided by PostgresNode, instead rolling their own, so they don't serve as a good example.)
--
I've been taking a look at the Perl test infrastructure ( src/test/perl ) for writing multi-node tests, starting with PostgresNode.pm and I have a few comments based on my first approach to the code "cold".
I think a README in src/test/perl/ would be very helpful. It's currently not at all clear how to go about using the new test infrastructure when first looking at it, particularly adding new test suites. Also a summary of available test facilities and some short examples or pointers to where they can be found ( src/bin/, src/test/ssl, etc).
The docs at http://www.postgresql.org/docs/devel/static/regress-tap.html mention the use of tap for client programs but don't have anything on writing/using the framework. It doesn't seem to be very close to / compatible with http://pgtap.org/ unless I'm missing something obvious.
I want to add tests for failover slots but I'm not sure where to put them or how to set up the test skeleton. So I went looking, and I could use confirmation that the below is roughly right so I can write it up for some quickstart docs.
It *looks* like one needs to create a Makefile with:
subdir = src/test/mytests
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
check:
$(prove_check)
clean:
rm -rf ./tmp_check
Then create a subdir src/test/mytests/t and in it put files named 001_sometest.pl etc. Each of which should import PostgresNode and generally then create new instances indirectly via the PostgresNode::get_new_node function rather than by using the constructor. Then init and start the server and run tests. Like:
use strict;
use warnings;
use PostgresNode;
use TestLib;
use Test::More tests => 1;
diag 'Setting up node';
my $node = get_new_node('master');
$node->init;
$node->start;
my $ret = $node->psql('postgres', 'SELECT 1;');
is($ret, '1', 'simple SELECT');
$node->teardown_node;
Knowledge of perl's Test::More is required since most of the suite is built on it.
The suite should be added to src/test/Makefile's ALWAYS_SUBDIRS entry.
Sound about right? I can tidy that up a bit and turn it into a README and add a reference to that to the public tap docs to tell users where to go if they want to write more tests.
I don't know how many suites we'll want - whether it'll be desirable to have a few suites with lots of tests or to have lots of suites with just a few tests. I'm planning on starting by adding a suite named 'replication' and putting some tests for failover slots in there. Reasonable?
(BTW, the ssl tests don't seem to use a bunch of the facilities provided by PostgresNode, instead rolling their own, so they don't serve as a good example.)
On Mon, Feb 22, 2016 at 2:19 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > I've been taking a look at the Perl test infrastructure ( src/test/perl ) > for writing multi-node tests, starting with PostgresNode.pm and I have a few > comments based on my first approach to the code "cold". > > I think a README in src/test/perl/ would be very helpful. It's currently not > at all clear how to go about using the new test infrastructure when first > looking at it, particularly adding new test suites. Also a summary of > available test facilities and some short examples or pointers to where they > can be found ( src/bin/, src/test/ssl, etc). +1. I will be happy to contribute into that. > The docs at http://www.postgresql.org/docs/devel/static/regress-tap.html > mention the use of tap for client programs but don't have anything on > writing/using the framework. It doesn't seem to be very close to / > compatible with http://pgtap.org/ unless I'm missing something obvious. > > I want to add tests for failover slots but I'm not sure where to put them or > how to set up the test skeleton. So I went looking, and I could use > confirmation that the below is roughly right so I can write it up for some > quickstart docs. src/test/modules > It *looks* like one needs to create a Makefile with: > > subdir = src/test/mytests > top_builddir = ../../.. > include $(top_builddir)/src/Makefile.global > > check: > $(prove_check) > > clean: > rm -rf ./tmp_check Yep. > Then create a subdir src/test/mytests/t and in it put files named > 001_sometest.pl etc. Each of which should import PostgresNode and generally > then create new instances indirectly via the PostgresNode::get_new_node > function rather than by using the constructor. Then init and start the > server and run tests. Like: A couple of other things that I am aware of: - including strict and warnings is as well mandatory to ensure that test grammar is sane, and the tests written should have no warnings. - All the core routines used should be compatible down to perl 5.8.8. - The code produced should be sanitized with perltidy before commit. > use strict; > use warnings; > use PostgresNode; > use TestLib; > use Test::More tests => 1; > > diag 'Setting up node'; > my $node = get_new_node('master'); > $node->init; > $node->start; > > my $ret = $node->psql('postgres', 'SELECT 1;'); > is($ret, '1', 'simple SELECT'); > > $node->teardown_node; Teardown is not mandatory at the end, though I would recommend having it, PostgresNode::DESTROY enforcing the nodes to stop at the end of a test, and the temporary paths are unconditionally removed (those should not be removed in case of a failure honestly). > Knowledge of perl's Test::More is required since most of the suite is built > on it. > > The suite should be added to src/test/Makefile's ALWAYS_SUBDIRS entry. SUBDIRS is more suited for tests that do not represent a security risk. The ssl suite is part of ALWAYS_SUBDIRS because it enforces the use of 127.0.0.1. > Sound about right? I can tidy that up a bit and turn it into a README and > add a reference to that to the public tap docs to tell users where to go if > they want to write more tests. Yes, please. > I don't know how many suites we'll want - whether it'll be desirable to have > a few suites with lots of tests or to have lots of suites with just a few > tests. I'm planning on starting by adding a suite named 'replication' and > putting some tests for failover slots in there. Reasonable? It seems to me that the failover slot tests should be part of the recovery test suite I have proposed already. Those are located in src/test/recovery, introducing as well a set of routines in PostgresNode.pm that allows one to pass options to PostgresNode::init to enable archive or streaming on a given node. I would believe that any replication suite is going to need that, and I have done a bunch of legwork to make sure that this works on Windows as well. > (BTW, the ssl tests don't seem to use a bunch of the facilities provided by > PostgresNode, instead rolling their own, so they don't serve as a good > example.) Yep. -- Michael
On 22 February 2016 at 14:30, Michael Paquier <michael.paquier@gmail.com> wrote:
+1. I will be happy to contribute into that.
Thanks. Hopefully what I wrote will be useful as a starting point.
As I read through the modules I'll write some draft Perldoc too.
> The docs at http://www.postgresql.org/docs/devel/static/regress-tap.html
> mention the use of tap for client programs but don't have anything on
> writing/using the framework. It doesn't seem to be very close to /
> compatible with http://pgtap.org/ unless I'm missing something obvious.
>
> I want to add tests for failover slots but I'm not sure where to put them or
> how to set up the test skeleton.
src/test/modules
Could use a README to explain what the directory is for.
It looks like it contains a bunch of the extensions that used to be in contrib/ . So that'd be where I'd want to put any extension needed as part of failover slots testing, but not the Perl test control code for setting up the nodes, etc. This testing cannot usefully be done with pg_regress and the part that can be is already added to contrib/test_decoding.
A couple of other things that I am aware of:
- including strict and warnings is as well mandatory to ensure that
test grammar is sane, and the tests written should have no warnings.
Yep, did that.
- All the core routines used should be compatible down to perl 5.8.8.
Ugh. So not just Perl, ancient perl.
I don't suppose Perl offers any kind of "compatible(5.8.8)" statement or anything? Do I have to compile a ten-year-old Perl and its dependencies to work on PostgreSQL tests? http://search.cpan.org/dist/Perl-MinimumVersion/lib/Perl/MinimumVersion.pm looks useful; do you think it's reasonable for code that passes that check to just be thrown at the buildfarm?
- The code produced should be sanitized with perltidy before commit.
Makes sense. Keeps things consistent.
> The suite should be added to src/test/Makefile's ALWAYS_SUBDIRS entry.
SUBDIRS is more suited for tests that do not represent a security
risk. The ssl suite is part of ALWAYS_SUBDIRS because it enforces the
use of 127.0.0.1.
Ok, that makes sense.
> Sound about right? I can tidy that up a bit and turn it into a README and
> add a reference to that to the public tap docs to tell users where to go if
> they want to write more tests.
Yes, please.
Will do that now.
> I don't know how many suites we'll want - whether it'll be desirable to have
> a few suites with lots of tests or to have lots of suites with just a few
> tests. I'm planning on starting by adding a suite named 'replication' and
> putting some tests for failover slots in there. Reasonable?
It seems to me that the failover slot tests should be part of the
recovery test suite I have proposed already. Those are located in
src/test/recovery, introducing as well a set of routines in
PostgresNode.pm that allows one to pass options to PostgresNode::init
to enable archive or streaming on a given node. I would believe that
any replication suite is going to need that, and I have done a bunch
of legwork to make sure that this works on Windows as well.
Not committed yet, I see. That's https://commitfest.postgresql.org/9/438/ right?
I'd rather not duplicate your work there, so I should build on that. Is there a public git tree for that?
On Mon, Feb 22, 2016 at 4:24 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 22 February 2016 at 14:30, Michael Paquier <michael.paquier@gmail.com> > wrote: >> - All the core routines used should be compatible down to perl 5.8.8. > > Ugh. So not just Perl, ancient perl. > > I don't suppose Perl offers any kind of "compatible(5.8.8)" statement or > anything? Do I have to compile a ten-year-old Perl and its dependencies to > work on PostgreSQL tests? > http://search.cpan.org/dist/Perl-MinimumVersion/lib/Perl/MinimumVersion.pm > looks useful; do you think it's reasonable for code that passes that check > to just be thrown at the buildfarm? No, I don't use those old versions :) Module::CoreList->first_release is one way to do that for a module: $ perl -MModule::CoreList -e ' print Module::CoreList->first_release('Test::More'), "\n";' 5.006002 Last time I was dealing with that I had as well a look at http://perldoc.perl.org/, which was quite helpful by browsing through each version. >> > Sound about right? I can tidy that up a bit and turn it into a README >> > and >> > add a reference to that to the public tap docs to tell users where to go >> > if >> > they want to write more tests. >> >> Yes, please. > > Will do that now. This is definitely independent from the efforts of the other patches. >> > I don't know how many suites we'll want - whether it'll be desirable to >> > have >> > a few suites with lots of tests or to have lots of suites with just a >> > few >> > tests. I'm planning on starting by adding a suite named 'replication' >> > and >> > putting some tests for failover slots in there. Reasonable? >> >> It seems to me that the failover slot tests should be part of the >> recovery test suite I have proposed already. Those are located in >> src/test/recovery, introducing as well a set of routines in >> PostgresNode.pm that allows one to pass options to PostgresNode::init >> to enable archive or streaming on a given node. I would believe that >> any replication suite is going to need that, and I have done a bunch >> of legwork to make sure that this works on Windows as well. > > > Not committed yet, I see. That's https://commitfest.postgresql.org/9/438/ > right? Yeah... That's life. > I'd rather not duplicate your work there, so I should build on that. Is > there a public git tree for that? The latest set of patches is here: https://github.com/michaelpq/postgres/commits/recovery_regression Those are the ones I sent a couple of days back on -hackers. Hopefully those will get integrated into the core code. -- Michael
> >> - All the core routines used should be compatible down to perl 5.8.8. >> > > Ugh. So not just Perl, ancient perl. > > I don't suppose Perl offers any kind of "compatible(5.8.8)" statement > or > anything? Do I have to compile a ten-year-old Perl and its dependencies > to > work on PostgreSQL tests? > http://search.cpan.org/dist/Perl-MinimumVersion/lib/Perl/MinimumVersion.pm > looks useful; do you think it's reasonable for code that passes that > check > to just be thrown at the buildfarm? I think what's needed is: use 5.008_008; ( see: http://perldoc.perl.org/functions/use.html )
At 2016-02-22 07:45:37 +0000, er@xs4all.nl wrote: > > I think what's needed is: > > use 5.008_008; That is meant to fail if the code is run on a version of Perl older than 5.8.8, not fail if the code uses language features not present in 5.8.8. It is little help for those trying to maintain backwards compatibility. -- Abhijit
On 22 February 2016 at 15:41, Michael Paquier <michael.paquier@gmail.com> wrote:
- Facilities to make it easier to set a master up as replication-enabled (your patch does this, but it should use wal_level = 'logical' by default IMO; also I think setting autovacuum=off is very wrong and will mask problems)
>> > Sound about right? I can tidy that up a bit and turn it into a README
>> > and
>> > add a reference to that to the public tap docs to tell users where to go
>> > if
>> > they want to write more tests.
>>
>> Yes, please.
>
> Will do that now.
This is definitely independent from the efforts of the other patches.
Done.
I got a bit carried away and added:
- src/test/perl/README
- src/test/README
- src/test/modules/README
- POD for src/test/perl/PostgresNode.pm
- $node->info() function (that just returns what dump_info() printed, but as a string
- $node->backup(...) support for passing -R to pg_basebackup
- $node->backup(...) support for passing -X stream to pg_basebackup
- src/test/example_suite/ with some simple demo tests
I found that I was writing documentation for how to write tests that'd bitrot quickly and landed up writing a demo/sample test that can be run as part of 'make check' with --enable-tap-tests instead. Hopefully it's not overkill.
LMK if you think it's too much and I can trim out what's unwanted.
In the process I noticed a few helpers I think should be added to PostgresNode. I haven't added them since I didn't want this to turn into a big patch when it was meant to just be 'add a README', but the main things are:
- promote (you've done this)
- Facilities to make it easier to set a master up as replication-enabled (your patch does this, but it should use wal_level = 'logical' by default IMO; also I think setting autovacuum=off is very wrong and will mask problems)
- wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given LSN
- wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with name $appname (or any standby, if unspecified) passes $lsn for $col, where $col can be 'sent', 'write', 'flush' or 'replay'
> Not committed yet, I see. That's https://commitfest.postgresql.org/9/438/
> right?
Yeah... That's life.
I'll comment separately on that thread.
Er, the patch is attached this time.
On 22 February 2016 at 18:54, Craig Ringer <craig@2ndquadrant.com> wrote:
On 22 February 2016 at 15:41, Michael Paquier <michael.paquier@gmail.com> wrote:
>> > Sound about right? I can tidy that up a bit and turn it into a README
>> > and
>> > add a reference to that to the public tap docs to tell users where to go
>> > if
>> > they want to write more tests.
>>
>> Yes, please.
>
> Will do that now.
This is definitely independent from the efforts of the other patches.Done.I got a bit carried away and added:- src/test/perl/README- src/test/README- src/test/modules/README- POD for src/test/perl/PostgresNode.pm- $node->info() function (that just returns what dump_info() printed, but as a string- $node->backup(...) support for passing -R to pg_basebackup- $node->backup(...) support for passing -X stream to pg_basebackup- src/test/example_suite/ with some simple demo testsI found that I was writing documentation for how to write tests that'd bitrot quickly and landed up writing a demo/sample test that can be run as part of 'make check' with --enable-tap-tests instead. Hopefully it's not overkill.LMK if you think it's too much and I can trim out what's unwanted.In the process I noticed a few helpers I think should be added to PostgresNode. I haven't added them since I didn't want this to turn into a big patch when it was meant to just be 'add a README', but the main things are:- promote (you've done this)
- Facilities to make it easier to set a master up as replication-enabled (your patch does this, but it should use wal_level = 'logical' by default IMO; also I think setting autovacuum=off is very wrong and will mask problems)- wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given LSN- wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with name $appname (or any standby, if unspecified) passes $lsn for $col, where $col can be 'sent', 'write', 'flush' or 'replay'> Not committed yet, I see. That's https://commitfest.postgresql.org/9/438/
> right?
Yeah... That's life.I'll comment separately on that thread.--
Вложения
On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > Er, the patch is attached this time. top_builddir = ../..include $(top_builddir)/src/Makefile.global -SUBDIRS = regress isolation modules +SUBDIRS = regress isolation modules example_suite I have no problem with a test suite to be included in SUBDIRS, but if that's a template test it would be better if added in ALWAYS_SUBDIRS. I think as well that having it in src/test/perl/example_suite would make more sense. --- /dev/null +++ b/src/test/README @@ -0,0 +1,37 @@ +This directory contains a variety of test infrastructure as well as some of the +tests in PostgreSQL. Not all tests are here - in particular, there are more in +individual contrib/ modules and in src/bin. The usual format for README files is more like that: path/to/README Nice title ======== Text, blah. I think that it would be better to stick with the common style in place. Except that this file is definitely a good addition. + use strict; + use warnings; + use 5.8.8; + use PostgresNode; + use TestLib; I am -1 for including a forced version number in the list of inclusions. We have been careful about not using routines that are not supported with perl < 5.8.8. Let's continue like that. Simply mentioning in the README this minimal requirement is enough IMO. + +=pod + +=head2 Set up a node pod format... Do we really want that? Considering that those modules are only aimed at being dedicated for in-core testing, I would say no. diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore new file mode 100644 index 0000000..6628455 --- /dev/null +++ b/src/test/mb/.gitignore @@ -0,0 +1 @@ +/results/ This should be a separate patch. --- /dev/null +++ b/src/test/modules/README @@ -0,0 +1,10 @@ +src/test/modules contains PostgreSQL extensions that are primarily or entirely +intended for testing PostgreSQL and/or to serve as example code. The extensions +here aren't intended to be installed in a production server and aren't useful +for "real work". Header format should be reworked here as well. This patch is going a bit more far than I expected first, I thought that this would be only a set of README files added into core. And it is not completely clear what we would gain by using perlpod in the case of those in-core modules. -- Michael
On 22 February 2016 at 20:21, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Er, the patch is attached this time.
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-SUBDIRS = regress isolation modules
+SUBDIRS = regress isolation modules example_suite
I have no problem with a test suite to be included in SUBDIRS, but if
that's a template test it would be better if added in ALWAYS_SUBDIRS.
Fair enough. My thinking in adding it to SUBDIRS is that it's trivial enough to cost practically nothing, and things that aren't run by default tend to bitrot.
I think as well that having it in src/test/perl/example_suite would
make more sense.
Yeah, it probably would.
--- /dev/null
+++ b/src/test/README
@@ -0,0 +1,37 @@
+This directory contains a variety of test infrastructure as well as some of the
+tests in PostgreSQL. Not all tests are here - in particular, there are more in
+individual contrib/ modules and in src/bin.
The usual format for README files is more like that:
Can do.
+ use strict;
+ use warnings;
+ use 5.8.8;
+ use PostgresNode;
+ use TestLib;
I am -1 for including a forced version number in the list of
inclusions. We have been careful about not using routines that are not
supported with perl < 5.8.8. Let's continue like that. Simply
mentioning in the README this minimal requirement is enough IMO.
Ok, no complaint from me.
+
+=pod
+
+=head2 Set up a node
pod format... Do we really want that? Considering that those modules
are only aimed at being dedicated for in-core testing, I would say no.
If it's plain comments you have to scan through massive piles of verbose Perl to find what you want. If it's pod you can just perldoc /path/to/module it and get a nice summary of the functions etc.
If these are intended to become usable facilities for people to write tests with then I think it's important that the docs be reasonably accessible. You don't write Test::More tests by reading Test/More.pm, you read its perldoc. Same deal.
SGML ghastly to write and would separate the test docs from the tests themselves, so I think it's pretty much the worst of all worlds.
So. Yes. I do think pod is desirable. Plus anything but the most extremely primitive editor highlights it nicely and it's trivial to write even if, like me, you haven't really used Perl in five years.
Have a look at
perldoc src/test/perl/PostgresNode.pm
(and imagine there was a SYNOPSIS and EXAMPLES section, which there isn't yet). I think that's a *lot* more readable than dredging through the sources, and it's only going to get more so as the functionality grows.
diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore
new file mode 100644
index 0000000..6628455
--- /dev/null
+++ b/src/test/mb/.gitignore
@@ -0,0 +1 @@
+/results/
This should be a separate patch.
Eh, sure.
This patch is going a bit more far than I expected first, I thought
that this would be only a set of README files added into core.
Yeah, as I said I realised while writing the "how to write tests" and "how to write modules" parts that I'd basically be writing code-in-a-README that was guaranteed to bitrot into uselessness since it couldn't be run and tested automatically. A sample module with some comments/pod describing what it does is a much, much more useful way to achieve that end and the README can just refer to it.
And it
is not completely clear what we would gain by using perlpod in the
case of those in-core modules
As above, readability when you're authoring tests not hacking on the test framework.
I really don't like writing Perl. The last thing I want to do is read through reams of Perl to find out if TestLib or PostgresNode have some functionality I need in a test or if I need to add it / do it in my own code. I certainly don't want to be doing that repeatedly when I'm just trying to look up the signature for methods as I write tests.
It might make more sense if you think of it as infrastructure that lets people write tests, not the end product its self. In an ideal world nobody would have to look at it, they'd just use it. Do you read pg_regress.c when you're writing regression tests? Nope, you read the README and some examples. Same deal.
Craig Ringer wrote: > > +=pod > > + > > +=head2 Set up a node > > pod format... Do we really want that? Considering that those modules > > are only aimed at being dedicated for in-core testing, I would say no. > > If it's plain comments you have to scan through massive piles of verbose > Perl to find what you want. If it's pod you can just perldoc > /path/to/module it and get a nice summary of the functions etc. > > If these are intended to become usable facilities for people to write tests > with then I think it's important that the docs be reasonably accessible. Yes, I think adding POD here is a good idea. I considered doing it myself back when I was messing with PostgresNode ... -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 23, 2016 at 12:29 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Craig Ringer wrote: > >> > +=pod >> > + >> > +=head2 Set up a node >> > pod format... Do we really want that? Considering that those modules >> > are only aimed at being dedicated for in-core testing, I would say no. >> >> If it's plain comments you have to scan through massive piles of verbose >> Perl to find what you want. If it's pod you can just perldoc >> /path/to/module it and get a nice summary of the functions etc. >> >> If these are intended to become usable facilities for people to write tests >> with then I think it's important that the docs be reasonably accessible. > > Yes, I think adding POD here is a good idea. I considered doing it > myself back when I was messing with PostgresNode ... OK, withdrawal from here. If there are patches to add that to the existing tests, I'll review them, and rebase what I have depending on what gets in first. Could a proper patch split be done please? -- Michael
On 23 February 2016 at 09:52, Michael Paquier <michael.paquier@gmail.com> wrote:
I've removed the example suite in favour of adding a SYNOPSIS section to PostgresNode.pm and describing the rest in the README. It won't be necessary once your replication tests go in, they'll be a perfectly adequate example.On Tue, Feb 23, 2016 at 12:29 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Craig Ringer wrote:
>
>> > +=pod
>> > +
>> > +=head2 Set up a node
>> > pod format... Do we really want that? Considering that those modules
>> > are only aimed at being dedicated for in-core testing, I would say no.
>>
>> If it's plain comments you have to scan through massive piles of verbose
>> Perl to find what you want. If it's pod you can just perldoc
>> /path/to/module it and get a nice summary of the functions etc.
>>
>> If these are intended to become usable facilities for people to write tests
>> with then I think it's important that the docs be reasonably accessible.
>
> Yes, I think adding POD here is a good idea. I considered doing it
> myself back when I was messing with PostgresNode ...
OK, withdrawal from here. If there are patches to add that to the
existing tests, I'll review them, and rebase what I have depending on
what gets in first. Could a proper patch split be done please?
Just finished doing that. Thanks for taking a look at the first patch so quickly. I hope this one's better.
FWIW I think you were right that using pod for the actual test wasn't the best choice, I should've just used comments. I do think it's important for the modules to have structured docs.
I also cut out the changes to the backup method; I'll send a pull to add to your proposed replication patch instead.
--
Вложения
On 23 February 2016 at 21:39, Craig Ringer <craig@2ndquadrant.com> wrote:
Just finished doing that. Thanks for taking a look at the first patch so quickly. I hope this one's better.
CF entry created.
On Tue, Feb 23, 2016 at 10:39 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Just finished doing that. Thanks for taking a look at the first patch so
> quickly. I hope this one's better.
>
> FWIW I think you were right that using pod for the actual test wasn't the
> best choice, I should've just used comments. I do think it's important for
> the modules to have structured docs.
> I've removed the example suite in favour of adding a SYNOPSIS section to
> PostgresNode.pm and describing the rest in the README. It won't be necessary
> once your replication tests go in, they'll be a perfectly adequate example.
Software engineering is an adapt-and-survive or die universe, I
usually choose the former way of thinking on this ML. In short, I
don't mind rebasing on top of this stuff as needed or put efforts
where need be if the overall result is better.
> I also cut out the changes to the backup method; I'll send a pull to add to
> your proposed replication patch instead.
Thanks.
For patch 1:
+Not all these tests get run by "make check". Check src/test/Makefile to see
+which tests get run automatically.
Perhaps adding that those are listed under SUBDIRS, ALWAYS_SUBDIRS
meaning that those paths are not part of the main suite?
+examples/
+ demo programs for libpq that double as regression tests via "make check"
s/demo/demonstration
Nitpick: adding a dot at the end of the sentences describing each folder?
+ extensions used only or mainly for test purposes, generally not useful
+ or suitable for installing in production databases. Some of these have
+ their own tests, some are used by tests elsewhere.
I wouldn't say not useful, just not suitable.
+ infrastructure for Perl-based Test::More tests. There are no actual tests
+ here, the code is used by other tests in src/bin/, contrib/ and in the
+ subdirectories of src/test.
Hm, those are called TAP tests (Test Anything Protocol) and not
Test::More tests, no?
+elsewhere in the test tree.
"test tree" or "code tree"?
Those two files are good additions btw.
For patch 2:
(Somebody more familiar than I would be better commenting on the
format that this patch proposes for PostgresNode.pm).
+ use PostgresNode;
+
+ my $node = get_new_node('mynode');
It would be good to add a comment like grab a new node and assign a
free port to it.
+It requires IPC::Run - install libperl-ipc-run (Debian/Ubuntu) or perl-IPC-Run
+(Fedora/RHEL).
Archlinux: perl-ipc-run. I would remove the package portion honestly,
this is really platform dependent and there are as many package names
as platforms, many.
+around Test::More functions to run commands with an envronment set up to
s/envronment/environment
+You should generally prefer to use get_new_node() instead since it takes care
+of finding port numbers, registering instances for cleanup, etc.
Is "you" something used a lot in perl module docs? This is not really
Postgres-like.
+=item $node->data_dir()
All those items can be listed without parenthesis.
+If archiving is enabled, WAL files go here.
Location of WAL segments when WAL archiving is enabled.
+=cut
+
+sub info
+{
Do you have a use case in mind for PostgresNode::info? If this is just
a doc patch I would suggest adding that as a separate patch if that's
really needed.
+ $self->host eq $test_pghost
+ or die "set_replication_conf only works with the default host";
Er, why? On Windows 127.0.0.1 is used all the time. On Linux local is
used, but a node can still connect to another node via replication by
using the connection string of the node it is connecting to.
+Create a hot backup with pg_basebackup in $node->backup_dir,
+including the transaction logs. xlogs are fetched at the
+end of the backup, not streamed.
s/xlogs/WAL data.
+You'll have to configure a suitable max_wal_senders on the
+target server since it isn't done by default.
My patch does that :) We could remove it.
+src/test/ssl, or should be added to one of the suites for an existing utility
Nitpick: dot at the end of a sentence.
+You should add the new suite to src/test/Makefile . See the comments there.
Nitpick: space-dot.
--
Michael
Thanks, pushed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services