TestLib::command_fails_like enhancement

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

TestLib::command_fails_like enhancement

Andrew Dunstan-8

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.

Some patches I will be submitting shortly rely on this enhancement.


cheers


andrew


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


testlib-command-fails-like-args-1.patch (922 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Michael Paquier-2
On Thu, Oct 31, 2019 at 01:02:58PM -0400, 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.

Why not.

> Some patches I will be submitting shortly rely on this enhancement.

Anything submitted yet or any examples?  I was just wondering in which
case it mattered.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Mark Dilger-4
In reply to this post by Andrew Dunstan-8


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.

--
Mark Dilger


Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Craig Ringer-3
On Fri, 8 Nov 2019 at 06:28, Mark Dilger <[hidden email]> 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. 


--
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise
Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Andrew Dunstan-8

On 11/8/19 1:16 AM, Craig Ringer wrote:

> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <[hidden email]
> <mailto:[hidden email]>> 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.


cheers


andrew


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



Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Mark Dilger-4


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 <[hidden email]
>> <mailto:[hidden email]>> 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.

--
Mark Dilger


Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Andrew Dunstan-8

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 <[hidden email]
>>> <mailto:[hidden email]>> 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@...>


cheers


andrew


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



Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Mark Dilger-4


On 11/8/19 9:22 AM, Andrew Dunstan wrote:
...
> This will need to be rewritten in light of the above, but see
> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@...>

Thanks for the reference.  Having read your motivating example, this new
review reverses some of what I suggested in the prior review.


In the existing TestLib.pm, there are eight occurrences of nearly
identical usages of IPC::Run scattered through similar functions:

run_command:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

check_pg_config:
     my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
       \$stdout, '2>', \$stderr
       or die "could not execute pg_config";

program_help_ok:
     my $result = IPC::Run::run [ $cmd, '--help' ], '>', \$stdout, '2>',
       \$stderr;

program_version_ok:
     my $result = IPC::Run::run [ $cmd, '--version' ], '>', \$stdout, '2>',
       \$stderr;

program_options_handling_ok:
     my $result = IPC::Run::run [ $cmd, '--not-a-valid-option' ], '>',
       \$stdout,
       '2>', \$stderr;

command_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

command_like_safe:
     my $result = IPC::Run::run $cmd, '>', $stdoutfile, '2>', $stderrfile;

command_fails_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr,
       @extra_ipcrun_opts;

One rational motive for designing TestLib with so much code duplication
is to make the tests that use the library easier to read:

   command_like_safe(foo);
   command_like(bar);
   command_fails_like(baz);

which is easier to understand than:

   command_like(foo, failure_mode => safe);
   command_like(bar);
   command_like(baz, failure => expected);

and so forth.

In line with that thinking, perhaps you should just create:

   command_fails_without_tty_like(foo)

and be done, or perhaps:

   command_fails_like(foo, tty => 'closed')

and still preserve some of the test readability.  Will anyone like the
readability of your tests if you have:

   command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ?

Admittedly, "foo", "bar", and "baz" above are shorthand notation for
things in practice that are already somewhat hard to read, as in:

   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');

but adding more to that cruft just makes it worse.  Right?

--
Mark Dilger


Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Andrew Dunstan-8

On 11/8/19 4:40 PM, Mark Dilger wrote:

>
>
> On 11/8/19 9:22 AM, Andrew Dunstan wrote:
> ...
>> This will need to be rewritten in light of the above, but see
>> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@...>
>>
>
> Thanks for the reference.  Having read your motivating example, this
> new review reverses some of what I suggested in the prior review.
>
>
> In the existing TestLib.pm, there are eight occurrences of nearly
> identical usages of IPC::Run scattered through similar functions:
>
>
[snip]


>
> One rational motive for designing TestLib with so much code
> duplication is to make the tests that use the library easier to read:
>
>   command_like_safe(foo);
>   command_like(bar);
>   command_fails_like(baz);
>
> which is easier to understand than:
>
>   command_like(foo, failure_mode => safe);
>   command_like(bar);
>   command_like(baz, failure => expected);
>
> and so forth.
>
> In line with that thinking, perhaps you should just create:
>
>   command_fails_without_tty_like(foo)
>
> and be done, or perhaps:
>
>   command_fails_like(foo, tty => 'closed')
>
> and still preserve some of the test readability.  Will anyone like the
> readability of your tests if you have:
>
>   command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ?
>
> Admittedly, "foo", "bar", and "baz" above are shorthand notation for
> things in practice that are already somewhat hard to read, as in:
>
>   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');
>
> but adding more to that cruft just makes it worse.  Right?
>

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.


cheers


andrew

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



Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Andrew Dunstan-8

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?


cheers


andrew


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



Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Mark Dilger-4


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


Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Andrew Dunstan-8

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



Reply | Threaded
Open this post in threaded view
|

Re: TestLib::command_fails_like enhancement

Mark Dilger-4


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