Re: pg_createsubscriber TAP test wrapping makes command options hard to read.

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Tue, Jan 21, 2025 at 10:31:01AM +1100, Peter Smith wrote:
>> I applied the v5* patches and ran make check-world. All passed OK.
>>
>> Your fat comma changes have greatly improved readability, particularly
>> in 040_createsubscriber (the file that caused me to start this
>> thread). Thanks!
>
> It is not an improvement in only 040_createsubscriber, all the places
> Dagfinn is touching here feel much better with this new syntax.

Thanks, I'm glad you agree.

> You have spent a lot of time on the change from short to long names
> and on the new convention grammar for the option/value duos, clearly.

I did it by grepping for the various functions that call programs, such
as command_((fails_)?like|ok) etc, and then reading the docs for each
command to find the corresponding long options. It got almost
meditative after a while, especially once I got some Emacs keyboard
macros dialled in.

The ones in src/bin/pg_resetwal/t/001_basic.pl I deliberatly left short
because the error messages refer to the short spelling even if you used
the long one.

> It took me some time to read through the patch to catch
> inconsistencies.
>
> - [ 'pg_verifybackup', '-s', $backup_path ],
> + [ 'pg_verifybackup', '--skip-checksum', $backup_path ],
> qr/backup successfully verified/,
> '-s skips checksumming');
>
> This one in pg_verifybackup should perhaps switch to the long option
> for the test description, like the rest.

Yeah, good point.

> The option should be named --skip-checksums, causing a test failure in
> the CI.

All the tests passed for me on both Debian and macOS, I guess their
getopts accept unambiguous abbreviations of options.

> -my @pg_basebackup_cmd = (@pg_basebackup_defs, '-U', 'backupuser', '-Xfetch');
> +my @pg_basebackup_cmd = (
> + @pg_basebackup_defs,
> + '--user' => 'backupuser',
>
> There was a second series of failures in the test
> basebackup_to_shell/t/001_basic.pl because of this part. The option
> name should be "--username" for pg_basebackup.

Yeah.

> With these two fixes, the CI is happy, which should hopefully be
> enough for the buildfarm. But we'll see..
>
> I'd like to apply what you have done here (planning to do a second
> lookup, FWIW). If anybody has objections, feel free to post that
> here.

Thanks again for reviewing this monster patch.

- ilmari