Re: Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

On Mon, Nov 30, 2015 at 6:28 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> On Fri, Nov 27, 2015 at 07:53:10PM -0300, Alvaro Herrera wrote:
> > Michael Paquier wrote:
> > > The result of a couple of hours of hacking is attached:
> > > - 0001 is the refactoring adding PostgresNode and RecursiveCopy. I have
> > > also found that it is quite advantageous to move some of the routines that
> > > are synonyms of system() and the stuff used for logging into another
> > > low-level library that PostgresNode depends on, that I called TestBase in
> > > this patch.
>
> > Here's another version of this. I changed the packages a bit more. For
> > starters, I moved the routines around a bit; some of your choices seemed
> > more about keeping stuff where it was originally rather than moving it
> > to where it made sense. These are the routines in each module:
> >
> > TestBase: system_or_bail system_log run_log slurp_dir slurp_file
> > append_to_file
> >
> > TestLib: get_new_node teardown_node psql poll_query_until command_ok
> > command_fails command_exit_is program_help_ok program_version_ok
> > program_options_handling_ok command_like issues_sql_like
>
> The proposed code is short on guidance about when to put a function in TestLib
> versus TestBase. TestLib has no header comment. The TestBase header comment
> would permit, for example, command_ok() in that module. I would try instead
> keeping TestLib as the base module and moving into PostgresNode the functions
> that deal with PostgreSQL clusters (get_new_node teardown_node psql
> poll_query_until issues_sql_like).

PostgresNode is wanted to be a base representation of how of node is,
not of how to operate on it. The ways to perform the tests, which
works on a node, is wanted as a higher-level operation.

Logging and base configuration of a test set is a lower level of
operations than PostgresNode, because cluster nodes need actually to
perform system calls, some of those system calls like run_log allowing
to log in the centralized log file. I have tried to make the headers
of those modules more verbose, please see attached.

>
> > +my $node = get_new_node();
> > +# Initialize node without replication settings
> > +$node->initNode(0);
> > +$node->startNode();
> > +my $pgdata = $node->getDataDir();
> > +
> > +$ENV{PGPORT} = $node->getPort();
>
> Starting a value retrieval method name with "get" is not Perlish. The TAP
> suites currently follow "man perlstyle" in using underscored_lower_case method
> names. No PostgreSQL Perl code uses lowerFirstCamelCase, though some uses
> CamelCase. The word "Node" is redundant. Use this style:
>
> $node->init(0);
> $node->start;
> my $pgdata = $node->data_dir;
> $ENV{PGPORT} = $node->port;

I have switched the style this way.

> As a matter of opinion, I recommend giving "init" key/value arguments instead
> of the single Boolean argument. The method could easily need more options in
> the future, and this makes the call site self-documenting:
>
> $node->init(hba_permit_replication => 0);

Done.

>
> > - 'pg_controldata with nonexistent directory fails');
> > + 'pg_controldata with nonexistent directory fails');
>
> perltidy will undo this whitespace-only change.

Cleaned up.

>
> > --- a/src/bin/pg_rewind/t/001_basic.pl
> > +++ b/src/bin/pg_rewind/t/001_basic.pl
> > @@ -1,9 +1,11 @@
> > +# Basic pg_rewind test.
> > +
> > use strict;
> > use warnings;
> > -use TestLib;
> > -use Test::More tests => 8;
> >
> > use RewindTest;
> > +use TestLib;
> > +use Test::More tests => 8;
>
> Revert all changes to this file. Audit the rest of the patch for whitespace
> change unrelated to the subject.

Done.

>
>
> > - 'fails with nonexistent table');
> > + 'fails with nonexistent table');
>
> > -'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x';
> > + 'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x';
>
> perltidy will undo these whitespace-only changes.

Cleaned up.

>
> > +# cluster -a is not compatible with -d, hence enforce environment variables
>
> s/cluster -a/clusterdb -a/

Fixed.

>
> > -command_fails([ 'createuser', 'user1' ], 'fails if role already exists');
> > +command_fails([ 'createuser', 'user1' ],
> > + 'fails if role already exists');
>
> perltidy will undo this whitespace-only change.
>
> > @@ -0,0 +1,252 @@
> > +# PostgresNode, simple node representation for regression tests.
> > +#
> > +# Regression tests should use this basic class infrastructure to define nodes
> > +# that need used in the test modules/scripts.
> > +package PostgresNode;
>
> Consider just saying, "Class representing a data directory and postmaster."

OK, I have changed this description:
+# PostgresNode, class representing a data directory and postmaster.
+#
+# This contains a basic set of routines able to work on a PostgreSQL node,
+# allowing to start, stop, backup and initialize it with various options.

>
> > + my $self = {
> > + _port => undef,
> > + _host => undef,
> > + _basedir => undef,
> > + _applname => undef,
> > + _logfile => undef };
> > +
> > + # Set up each field
> > + $self->{_port} = $pgport;
> > + $self->{_host} = $pghost;
> > + $self->{_basedir} = TestBase::tempdir;
> > + $self->{_applname} = "node_$pgport";
> > + $self->{_logfile} = "$TestBase::log_path/node_$pgport.log";
>
> Why set fields to undef immediately before filling them?

Fixed.

>
> > @@ -0,0 +1,143 @@
> > +# Set of low-level routines dedicated to base tasks for regression tests, like
> > +# command execution and logging.
> > +#
> > +# This module should not depend on any other PostgreSQL regression test
> > +# modules.
> > +package TestBase;
>
> This is no mere set of routines. Just "use"-ing this module creates some
> directories and alters stdin/stdout/stderr.

I have updated the description of this file.

>
> > +BEGIN
> > +{
> > + $windows_os = $Config{osname} eq 'MSWin32' || $Config{osname} eq 'msys';
> > +
> > + # Determine output directories, and create them. The base path is the
> > + # TESTDIR environment variable, which is normally set by the invoking
> > + # Makefile.
> > + $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check";
> > + $log_path = "$tmp_check/log";
> > +
> > + mkdir $tmp_check;
> > + mkdir $log_path;
>
> Never mutate the filesystem in a BEGIN block, because "perl -c" runs BEGIN
> blocks. (Likewise for the BEGIN block this patch adds to TestLib.)

Hm. It seems to me that the whole block should be part of INIT then,
because the log file where STDERR and STDOUT is recaptured depends on
those to be created as well. By doing this change, please note that
compilation errors are not recaptured into the log file (thanks Andrew
for the pointers to perlmod).

I have as well updated pg_rewind tests to remove PGDATABASE. Patch to
address those issues is attached.
Regards,
--
Michael