On 11/11/19 11:28 AM, Andrew Dunstan wrote:
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.
Ok. I think your proposal sounds fine.
--
Mark Dilger
