Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 18 May 2022 18:20:08 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@gmail.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Andrew Dunstan <andrew@dunslane.net>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rewriting the test of pg_upgrade as a TAP test - take three -
 remastered set
Message-ID: <YoS6SFWXvI5zplXV@paquier.xyz>
References: <547429.1648734590@sss.pgh.pa.us>
 <YkZSgJ8tGy2tAWwf@paquier.xyz>
 <20220502042718.GB1565149@rfd.leadboat.com>
 <YniID7Lm9bZUoQXl@paquier.xyz>
 <YnsRiDW5tY8z0oCf@paquier.xyz>
 <20220511053255.GA2578591@rfd.leadboat.com>
 <YnyawnlheodeuFfS@paquier.xyz>
 <20220514082728.GE2684227@rfd.leadboat.com>
 <YoHhWD5vQzb2mmiF@paquier.xyz>
 <20220518080315.GC2820845@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="jFqksGfWApGEBN0Y"
Content-Disposition: inline
In-Reply-To: <20220518080315.GC2820845@rfd.leadboat.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YoS6SFWXvI5zplXV%40paquier.xyz>
Precedence: bulk


--jFqksGfWApGEBN0Y
Content-Type: multipart/mixed; boundary="v6+IxbfNT0H0yAFK"
Content-Disposition: inline


--v6+IxbfNT0H0yAFK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>>=20
>> 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.
>=20
> 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=3Dlongfin&d=
t=3D2022-05-18%2000%3A59%3A35&stg=3Dpg_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=3D5678: 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 runn=
ing
> 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, '');
>=20
> 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

--v6+IxbfNT0H0yAFK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="upgrade-tap-fixes-noah-3.patch"
Content-Transfer-Encoding: quoted-printable

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) =3D @_;
+	my ($node, $prefix, $from_char, $to_char, $suffix) =3D @_;
=20
-	my $dbname =3D '';
+	my $dbname =3D $prefix;
 	for my $i ($from_char .. $to_char)
 	{
 		next if $i =3D=3D 7 || $i =3D=3D 10 || $i =3D=3D 13;    # skip BEL, LF, =
and CR
 		$dbname =3D $dbname . sprintf('%c', $i);
 	}
=20
-	# Exercise backslashes adjacent to double quotes, a Windows special
-	# case.
-	$dbname =3D '\\"\\' . $dbname . '\\\\"\\\\\\';
+	$dbname .=3D $suffix;
 	$node->command_ok([ 'createdb', $dbname ]);
 }
=20
@@ -79,10 +77,12 @@ else
 {
 	# Default is to use pg_regress to set up the old instance.
=20
-	# 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, '');
=20
 	# Grab any regression options that may be passed down by caller.
 	my $extra_opts =3D $ENV{EXTRA_REGRESS_OPTS} || "";
@@ -99,7 +99,7 @@ else
=20
 	my $rc =3D
 	  system($ENV{PG_REGRESS}
-		  . "$extra_opts "
+		  . " $extra_opts "
 		  . "--dlpath=3D\"$dlpath\" "
 		  . "--bindir=3D "
 		  . "--host=3D"
@@ -121,6 +121,7 @@ else
 			print "=3D=3D=3D EOF =3D=3D=3D\n";
 		}
 	}
+	is($rc, 0, 'regression tests pass');
 }
=20
 # Before dumping, get rid of objects not existing or not supported in later

--v6+IxbfNT0H0yAFK--

--jFqksGfWApGEBN0Y
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmKEukgACgkQnvQgOdby
QH0OJQ/+Izp5tD9U1WgSuAUYKpdT3/uQV5GDWQ4ljTiCzdnO/4grF5I9adjcMo4x
dvt6XFjJoK4Pq/0O4N7c/VBPLcHEMBRKxBLOvdHCnULrz6Si8aAndNsxD0+nO0Lc
Aq2N9iZcFdctYIg/GvWnS+5z+2tgd2S2IxyE/03sdQwGlMvVUJr0tLO6AM1feCz9
GhcsfCRggIn7712+kHgWMqbY66mKJyNHr9MtS7jjGUI3YgTyHKg2teBjEBjBHfG5
Ux8TU6x7QAmc1yP5aqPULtEHxZQhXF/VniGv6fyRphAtdFpvSDN2ywt79J0LfRbf
/u9+BWkT+cfeFDQdXIEimrndW78AB7uu+JmElWJQifl8WS0WdC6tr+QLRc3Az4Ky
MKBaD5N7n19hLg5k0lrApR7CTRhzd6LeRWPQONwXL1RRGi7rEOVjp3ImmsS7kRhN
n6Xs/qmzXLv09Te8JMqdpblnFcbY1Xj/y7fTPzQeZcuvjF7lckva0nRglCCkVFlm
ZV3M4KeZUO5Lzz2MiZRDQpHntFaV5b1+NeRmm87DSOPomcQwZPA8VuOGj2POhnQY
zGcKkC2ITHEKNzd3jmUB6UjBSzsmSnrT8BtmN0q2suQDrPt5Z/4x9Hb8RWbdpqWh
yzYPgICcpeb1U3uwCeTH4IgP0aFWNcHoalTAcQ6BvxWXAoxQH3I=
=2UYA
-----END PGP SIGNATURE-----

--jFqksGfWApGEBN0Y--