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(at)paquier(dot)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(at)paquier(dot)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(at)paquier(dot)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