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.