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

On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> > Well, let's go ahead with it and see what happens. If it's too
> > much of a mess we can always revert.
>
> Okay, done after an extra round of self-review.

commit 322becb wrote:
> --- /dev/null
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl

> +# Generate a database with a name made of a range of ASCII characters.
> +sub generate_db
> +{
> + my ($node, $from_char, $to_char) = @_;
> +
> + my $dbname = '';
> + for my $i ($from_char .. $to_char)
> + {
> + next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
> + $dbname = $dbname . sprintf('%c', $i);
> + }
> + $node->run_log(
> + [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
> + );

Nothing checks the command result, so the test file passes even if each of
these createdb calls fails. Other run_log() calls in this file have the same
problem. This particular one should be command_ok() or similar.

--host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
because that call puts equivalent configuration in the environment. Other
calls in the file have the same redundant operands. (No other test file has
redundant --host or --port.)

> + # Grab any regression options that may be passed down by caller.
> + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";

Typo: s/_OPT/_OPTS/

> + my @extra_opts = split(/\s+/, $extra_opts_val);

src/test/recovery/t/027_stream_regress.pl and the makefiles treat
EXTRA_REGRESS_OPTS as a shell fragment. To be compatible, use the
src/test/recovery/t/027_stream_regress.pl approach. Affected usage patetrns
are not very important, but since the tree has code for it, you may as well
borrow that code. These examples witness the difference:

EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/bin/pg_upgrade check PROVE_TESTS=t/002_pg_upgrade.pl
# log has: /home/nm/src/pg/postgresql/src/bin/pg_upgrade/../../../src/test/regress/pg_regress: unrecognized option '--nosuc"'
EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/test/recovery check PROVE_TESTS=t/027_stream_regress.pl
# log has: /home/nm/src/pg/postgresql/src/test/recovery/../../../src/test/regress/pg_regress: unrecognized option '--nosuc h'

> --- a/src/bin/pg_upgrade/test.sh
> +++ /dev/null

> -# Create databases with names covering the ASCII bytes other than NUL, BEL,
> -# LF, or CR. BEL would ring the terminal bell in the course of this test, and
> -# it is not otherwise a special case. PostgreSQL doesn't support the rest.
> -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
> - if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
> -# Exercise backslashes adjacent to double quotes, a Windows special case.
> -dbname1='\"\'$dbname1'\\"\\\'

This rewrite dropped the exercise of backslashes adjacent to double quotes.