Re: Writing new unit tests with PostgresNode

On Tue, Feb 23, 2016 at 10:39 PM, Craig Ringer <craig(at)2ndquadrant(dot)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