Re: TestLib::command_fails_like enhancement

On 11/8/19 9:22 AM, Andrew Dunstan wrote:
...
> This will need to be rewritten in light of the above, but see
> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@2ndQuadrant.com>

Thanks for the reference.  Having read your motivating example, this new 
review reverses some of what I suggested in the prior review.


In the existing TestLib.pm, there are eight occurrences of nearly 
identical usages of IPC::Run scattered through similar functions:

run_command:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

check_pg_config:
     my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
       \$stdout, '2>', \$stderr
       or die "could not execute pg_config";

program_help_ok:
     my $result = IPC::Run::run [ $cmd, '--help' ], '>', \$stdout, '2>',
       \$stderr;

program_version_ok:
     my $result = IPC::Run::run [ $cmd, '--version' ], '>', \$stdout, '2>',
       \$stderr;

program_options_handling_ok:
     my $result = IPC::Run::run [ $cmd, '--not-a-valid-option' ], '>',
       \$stdout,
       '2>', \$stderr;

command_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

command_like_safe:
     my $result = IPC::Run::run $cmd, '>', $stdoutfile, '2>', $stderrfile;

command_fails_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr,
       @extra_ipcrun_opts;

One rational motive for designing TestLib with so much code duplication 
is to make the tests that use the library easier to read:

   command_like_safe(foo);
   command_like(bar);
   command_fails_like(baz);

which is easier to understand than:

   command_like(foo, failure_mode => safe);
   command_like(bar);
   command_like(baz, failure => expected);

and so forth.

In line with that thinking, perhaps you should just create:

   command_fails_without_tty_like(foo)

and be done, or perhaps:

   command_fails_like(foo, tty => 'closed')

and still preserve some of the test readability.  Will anyone like the 
readability of your tests if you have:

   command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ?

Admittedly, "foo", "bar", and "baz" above are shorthand notation for 
things in practice that are already somewhat hard to read, as in:

   command_fails_like(
       [ 'pg_dump', 'qqq', 'abc' ],
       qr/\Qpg_dump: error: too many command-line arguments (first is 
"abc")\E/,
       'pg_dump: too many command-line arguments');

but adding more to that cruft just makes it worse.  Right?

-- 
Mark Dilger