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

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