Re: Writing new unit tests with PostgresNode - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: Writing new unit tests with PostgresNode |
Date | |
Msg-id | CAB7nPqQ8H7A0xYhKEMQUNq7Y9f90WQyX0NhZTM0a-eWWS4OqNQ@mail.gmail.com Whole thread Raw |
In response to | Re: Writing new unit tests with PostgresNode (Craig Ringer <craig@2ndquadrant.com>) |
List | pgsql-hackers |
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
pgsql-hackers by date: