On 11/11/19 8:48 AM, Andrew Dunstan wrote:
>
> On 11/9/19 8:25 AM, Andrew Dunstan wrote:
>> 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.
>>
>>
>
> On further consideration, I'm wondering why we don't just
> unconditionally use a closed input pty for all these functions (except
> run_log). None of them use any input, and making the client worry about
> whether or not to close it seems something of an abstraction break.
> There would be no API change at all involved in this case, just a bit of
> extra cleanliness. Would need testing on Windows, I'll go and do that now.
>
>
> Thoughts?
That sounds a lot better than your previous patch.
PostgresNode.pm and TestLib.pm both invoke IPC::Run::run. If you change
all the invocations in TestLib to close input pty, should you do the
same for PostgresNode? I don't have a strong argument for doing so, but
it seems cleaner to have both libraries invoking commands under
identical conditions, such that if commands were borrowed from one
library and called from the other they would behave the same.
PostgresNode already uses TestLib, so perhaps setting up the environment
can be abstracted into something, perhaps TestLib::run, and then used
everywhere that IPC::Run::run currently is used.
--
Mark Dilger
