Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

Hi,

On 2022-03-02 15:57:23 +0900, Michael Paquier wrote:
> Do others have an opinion about a backpatch of the bugfix? Nobody has
> complained about that since pg_upgrade exists, so I have just done the
> change on HEAD.

WFM.

> +++ b/src/bin/pg_upgrade/t/001_basic.pl
> @@ -0,0 +1,9 @@
> +use strict;
> +use warnings;
> +
> +use PostgreSQL::Test::Utils;
> +use Test::More tests => 8;

Outdated.

> +program_help_ok('pg_upgrade');
> +program_version_ok('pg_upgrade');
> +program_options_handling_ok('pg_upgrade');

Unrelated. But I kinda wish we'd do this in a saner manner than copying this
test into every binary. E.g. by ensuring that all tools installed in the temp
install are tested or such.

> +# The test of pg_upgrade consists in setting up an instance. This is the
> +# source instance used for the upgrade. Then a new and fresh instance is
> +# created, and is used as the target instance for the upgrade.

This seems a bit repetitive. Lots of "instance".

> Before
> +# running an upgrade, a logical dump of the old instance is taken, and a
> +# second logical dump of the new instance is taken after the upgrade.
> +# The upgrade test passes if there are no differences in these two dumps.
> +
> +# Testing upgrades with an older instance of PostgreSQL requires setting up
> +# two environment variables, as of:
> +# - "olddump", to point to a dump file that will be used to set
> +# up the old instance to upgrade from, the dump being restored in the
> +# old cluster.
> +# - "oldinstall", to point to the installation path of the old
> +# instance.
> +if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
> + || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))

Odd indentation. Spaces between parens?

> +$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);

I'd copy the comments from test.sh wrt --wal-segsize,
--allow-group-access.

Greetings,

Andres Freund