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

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).

> I tried to get rid of teardown_node by having a DESTROY method for
> PostgresNode; that method would call "pg_ctl stop -m immediate". That
> would have been much cleaner. However, when a test fails this doesn't
> work sanely because the END block for File::Temp runs earlier than that
> DESTROY block, which means the datadir is already gone by the time
> pg_ctl stop runs, so the node stop doesn't work at all. (Perhaps we
> could fix this by noting postmaster's PID at start time, and then
> sending a signal directly instead of relying on pg_ctl).

You could disable File::Temp cleanup and handle cleanup yourself at the
desired time. (I haven't reviewed whether the goal of removing teardown_node
is otherwise good.)

> +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;

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);

> - 'pg_controldata with nonexistent directory fails');
> + 'pg_controldata with nonexistent directory fails');

perltidy will undo this whitespace-only change.

> --- 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.

> - '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.

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

s/cluster -a/clusterdb -a/

> -issues_sql_like(
> +$ENV{PGPORT} = $node->getPort();
> +
> +issues_sql_like($node,

perltidy will move $node to its own line.

> -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."

> + 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?

> @@ -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.

> +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.)

nm