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