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

On Mon, May 16, 2022 at 02:30:00PM +0900, Michael Paquier wrote:
> On Sat, May 14, 2022 at 01:27:28AM -0700, Noah Misch wrote:
> > Here, I requested the rationale for the differences you had just described.
> > You made a choice to stop testing one list of database names and start testing
> > a different list of database names. Why?
>
> Because the shape of the new names does not change the test coverage
> ("regression" prefix or the addition of the double quotes with
> backslashes for all the database names), while keeping the code a bit
> simpler. If you think that the older names are more adapted, I have
> no objections to use them, FWIW, which is something like the patch
> attached would achieve.
>
> This uses the same convention as vcregress.pl before 322becb, but not
> the one of test.sh where "regression" was appended to the database
> names.

I would have picked the test.sh names, both because test.sh was the senior
implementation and because doing so avoids warnings under
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS. See the warnings here:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2022-05-18%2000%3A59%3A35&stg=pg_upgrade-check

More-notable line from that same log:
sh: /Users/buildfarm/bf-data/HEAD/pgsql.build/src/bin/pg_upgrade/../../../src/test/regress/pg_regress--port=5678: No such file or directory

Commit 7dd3ee5 adopted much of the 027_stream_regress.pl approach to running
pg_regress, but it didn't grab the "is($rc, 0, 'regression tests pass')"
needed to make defects like that report a failure.

> --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> @@ -13,18 +13,16 @@ use Test::More;
> # Generate a database with a name made of a range of ASCII characters.
> sub generate_db
> {
> - my ($node, $from_char, $to_char) = @_;
> + my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
>
> - my $dbname = '';
> + my $dbname = $prefix;
> 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);
> }
>
> - # Exercise backslashes adjacent to double quotes, a Windows special
> - # case.
> - $dbname = '\\"\\' . $dbname . '\\\\"\\\\\\';
> + $dbname .= $suffix;
> $node->command_ok([ 'createdb', $dbname ]);
> }
>
> @@ -79,10 +77,12 @@ else
> {
> # Default is to use pg_regress to set up the old instance.
>
> - # Create databases with names covering most ASCII bytes
> - generate_db($oldnode, 1, 45);
> - generate_db($oldnode, 46, 90);
> - generate_db($oldnode, 91, 127);
> + # Create databases with names covering most ASCII bytes. The
> + # first name exercises backslashes adjacent to double quotes, a
> + # Windows special case.
> + generate_db($oldnode, "\\\"\\", 1, 45, "\\\\\"\\\\\\");
> + generate_db($oldnode, '', 46, 90, '');
> + generate_db($oldnode, '', 91, 127, '');

Does this pass on Windows? I'm 65% confident that released IPC::Run can't
handle this input due to https://github.com/toddr/IPC-Run/issues/142. If it's
passing for you on Windows, then disregard.