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-9fa34914a...@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
