Re: TestLib::command_fails_like enhancement

On 11/8/19 11:25 AM, Mark Dilger wrote:
>
>
> On 11/8/19 6:33 AM, Andrew Dunstan wrote:
>>
>> On 11/8/19 1:16 AM, Craig Ringer wrote:
>>> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter(at)gmail(dot)com
>>> <mailto:hornschnorter(at)gmail(dot)com>> wrote:
>>>
>>>
>>>
>>>      On 10/31/19 10:02 AM, Andrew Dunstan wrote:
>>>      >
>>>      > This small patch authored by my colleague Craig Ringer enhances
>>>      > Testlib's command_fails_like by allowing the passing of extra
>>>      keyword
>>>      > type arguments. The keyword initially recognized is
>>>      'extra_ipcrun_opts'.
>>>      > The value for this keyword needs to be an array, and is passed
>>>      through
>>>      > to the call to IPC::Run.
>>>
>>>      Hi Andrew, a few code review comments:
>>>
>>>      The POD documentation for this function should be updated to
>>>      include a
>>>      description of the %kwargs argument list.
>>>
>>>      Since command_fails_like is patterned on command_like, perhaps you
>>>      should make this change to both of them, even if you only
>>> originally
>>>      intend to use the new functionality in command_fails_like.  I'm
>>>      not sure
>>>      of this, though, since I haven't seen any example usage yet.
>>>
>>>      I'm vaguely bothered by having %kwargs gobble up the remaining
>>>      function
>>>      arguments, not because it isn't a perl-ish thing to do, but
>>>      because none
>>>      of the other functions in this module do anything similar.  The
>>>      function
>>>      check_mode_recursive takes an optional $ignore_list array
>>>      reference as
>>>      its last argument.  Perhaps command_fails_like could follow that
>>>      pattern
>>>      by taking an optional $kwargs hash reference.
>>>
>>>
>>> Yeah, that's probably sensible.
>>>
>>>
>>>
>>
>>
>> OK, I will rework it taking these comments into account. Thanks for the
>> comments Mark.
>
> I'd be happy to see the regression tests you are writing sooner than
> that, if you don't mind posting them.  It's hard to do a proper review
> for you without a better sense of where you are going with these changes.

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>

cheers

andrew

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