Re: Rewriting the test of pg_upgrade as a TAP test - take two

Hi,

On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:
> Another topic that I would like to discuss is how this interface is fit
> for the buildfarm code.  After hacking my stuff, I have looked at the
> buildfarm code to notice that things like TestUpgradeXversion.pm do
> *not* make use of test.sh, and that what I hacked is much similar to
> what the buildfarm code is doing, which is itself roughly a copy of what
> test.sh does.  Andrew, your feedback would be important here.

Andrew, any comments?



> From 1294bb18426cea5c0764d0d07846fee291502b73 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Wed, 24 Jan 2018 16:19:53 +0900
> Subject: [PATCH 1/3] Refactor PostgresNode.pm so as nodes can use custom
>  binary path
> 
> This is a requirement for having a proper TAP test suite for pg_upgrade
> where users have the possibility to manipulate nodes which use different
> set of binaries during the upgrade processes.

Seems reasonable.

> +    # Find binary directory for this new node.
> +    if (!defined($bindir))
> +    {
> +        # If caller has not defined the binary directory, find it
> +        # from pg_config defined in this environment's PATH.
> +        my ($stdout, $stderr);
> +        my $result = IPC::Run::run [ 'pg_config', '--bindir' ], '>',
> +            \$stdout, '2>', \$stderr
> +            or die "could not execute pg_config";
> +        chomp($stdout);
> +        $bindir = $stdout;
> +    }
> +

This isn't really required, we could just assume things are on PATH if
bindir isn't specified.  Don't have strong feelings about it.



> From df74a70b886bb998ac37195b4d3067c41a012738 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Wed, 24 Jan 2018 16:22:00 +0900
> Subject: [PATCH 2/3] Add basic TAP test setup for pg_upgrade
> 
> The plan is to convert the current pg_upgrade test to the TAP
> framework.  This commit just puts a basic TAP test in place so that we
> can see how the build farm behaves, since the build farm client has some
> special knowledge of the pg_upgrade tests.

Not sure I see the point of keeping this separate, but whatever...


> From f903e01bbb65f1e38cd74b01ee99c4526e4c3b7f Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Fri, 26 Jan 2018 16:37:42 +0900
> Subject: [PATCH 3/3] Replace pg_upgrade's test.sh by a TAP test
> 
> This replacement still allows tests across major versions, though the
> interface to reach that has been changed a bit. See TESTING for more
> details on the matter.

> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> @@ -0,0 +1,213 @@
> +# Set of tests for pg_upgrade.
> +use strict;
> +use warnings;
> +use Cwd;
> +use Config;
> +use File::Basename;
> +use File::Copy;
> +use IPC::Run;
> +use PostgresNode;
> +use TestLib;
> +use Test::More tests => 4;
> +
> +# Generate a database with a name made of a range of ASCII characters.
> +sub generate_db
> +{

s/_/_random_/?


> +# From now on,

Odd phrasing imo.

>

> +elsif (defined($ENV{oldsrc}) &&
> +    defined($ENV{oldbindir}) &&
> +    defined($ENV{oldlibdir}))
> +{
> +    # A run is wanted on an old version as base.
> +    $oldsrc = $ENV{oldsrc};
> +    $oldbindir = $ENV{oldbindir};
> +    $oldlibdir = $ENV{oldlibdir};
> +    # FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql"
> +    # causes the regression tests to pass but pg_upgrade to fail afterwards.

Planning to fix it?


> +# Install manually regress.so, this got forgotten in the process.
> +copy "$oldsrc/src/test/regress/regress.so",
> +    "$oldlibdir/postgresql/regress.so"
> +    unless (-e "$oldlibdir/regress.so");

Weird comment again.


> +# Run regression tests on the old instance, using the binaries of this
> +# instance. At the same time create a tablespace path needed for the
> +# tests, similarly to what "make check" creates.

What does "using binaries of this instance" mean? And why?

> +chdir dirname($regress_bin);
> +rmdir "testtablespace";
> +mkdir "testtablespace";
> +$oldnode->run_log([ "$oldbindir/createdb", '--port', $oldnode->port,
> +                    'regression' ]);

> +$oldnode->command_ok([$regress_bin, '--schedule', './serial_schedule',
> +        '--dlpath', "$dlpath", '--bindir', $oldnode->bin_dir,
> +        '--use-existing', '--port', $oldnode->port],
> +        'regression test run on old instance');


> +# Before dumping, get rid of objects not existing in later versions. This
> +# depends on the version of the old server used, and matters only if the
> +# old and new source paths
> +my $oldpgversion;
> +($result, $oldpgversion, $stderr) =
> +    $oldnode->psql('postgres', qq[SHOW server_version_num;]);
> +my $fix_sql;
> +if ($newsrc ne $oldsrc)
> +{
> +    if ($oldpgversion <= 80400)
> +    {
> +        $fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);";
> +    }
> +    else
> +    {
> +        $fix_sql = "DROP FUNCTION public.oldstyle_length(integer, text);";
> +    }
> +    $oldnode->psql('postgres', $fix_sql);
> +}

I know you copied this, but what?


> +# Take a dump before performing the upgrade as a base comparison. Note
> +# that we need to use pg_dumpall from PATH here.

Whe do we need to?

Greetings,

Andres Freund