Re: TestLib::command_fails_like enhancement

On 11/25/19 5:08 AM, Andrew Dunstan wrote:
> 
> On 11/11/19 4:28 PM, Mark Dilger wrote:
>>
>>
>>>>>>
>>>>>
>>>>> 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.
> 
> 
> 
> Here's a patch for that. The pty stuff crashes and burns on my Windows
> test box, so there I just set stdin to an empty string via the usual
> pipe mechanism.

Ok, I've reviewed and tested this.  It works fine for me on Linux.  I
am not set up to test it on Windows.  I think it is ready to commit.

I have one remaining comment about the code, and this is just FYI.  I
won't quibble with you committing your patch as it currently stands.

You might consider changing the '\x04' literal to use a named control
character, both for readability and portability, as here:

+               use charnames ':full';
+               @no_stdin = ('