On 11/8/19 4:40 PM, Mark Dilger wrote:
>
>
> 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:
>
>
[snip]
>
> 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?
>
OK, I agree that we're getting rather baroque here. I could go with your
suggestion of YA function, or possibly a solution that simple passes any
extra arguments straight through to IPC::Run::run(), e.g.
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',
'<pty<', \$eof_in);
That means we're not future-proofing the function - we'll never be able
to add more arguments to it, but I'm not really certain that matters
anyway. I should note that perlcritic whines about subroutines with too
many arguments, so making provision for more seems unnecessary anyway.
I don't think this is worth spending a huge amount of time on, we've
already spent more time discussing it than it would take to implement
either solution.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
