Re: TestLib::command_fails_like enhancement

On 11/11/19 1:27 PM, Mark Dilger wrote:
>
>
> 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.



I don't think we need to do that. In the case of the PostgresNode.pm
uses we know what the executable is, unlike the the TestLib.pm cases.
They are our own executables and we don't expect them to be doing
anything funky with /dev/tty.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services