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

[Michael Paquier]

On Wed, May 18, 2022 at 01:03:15AM -0700, Noah Misch wrote:
> On Mon, May 16, 2022 at 02:30:00PM +0900, Michael Paquier wrote:
>> 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

Yes, I saw that.  This did not bother me much as the TAP tests run in
isolation, but I am fine to stick to your option and silence these.

> 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

So you are using EXTRA_REGRESS_OPTS, then, and a space is missing from
the first argument of the command used to make that work properly.

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

Okay, added this one.

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

Hmm.  The CI has been passing for me with this name pattern in place,
as of https://github.com/michaelpq/postgres/tree/upgrade_tap_fixes.

Attached is an updated patch to address your concerns.
--
Michael

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8372a85e6e..86fe1b4d09 100644
--- 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, 'regression\\"\\', 1,  45,   '\\\\"\\\\\\');
+	generate_db($oldnode, 'regression',      46, 90,  '');
+	generate_db($oldnode, 'regression',      91, 127, '');
 
 	# Grab any regression options that may be passed down by caller.
 	my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
@@ -99,7 +99,7 @@ else
 
 	my $rc =
 	  system($ENV{PG_REGRESS}
-		  . "$extra_opts "
+		  . " $extra_opts "
 		  . "--dlpath=\"$dlpath\" "
 		  . "--bindir= "
 		  . "--host="
@@ -121,6 +121,7 @@ else
 			print "=== EOF ===\n";
 		}
 	}
+	is($rc, 0, 'regression tests pass');
 }
 
 # Before dumping, get rid of objects not existing or not supported in later

signature.asc
Description: PGP signature