Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

On 11/25/19 1:56 PM, Mark Dilger wrote:
>
>
> 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 = ('
> The only character set I can find where this matters is EBCDIC, in
> which the EOT character is 55 rather than 4.  Since EBCDIC does not
> occur in the list of supported character sets for postgres, per the
> docs section 23.3.1, I don't suppose it matters too much.  Nor can
> I test how this works on EBCDIC, so I'm mostly guessing that perl
> would do the right thing there.  But, at least to my eyes, it is
> more immediately clear what the code is doing when the control
> character name is spelled out.
>
>


Agreed, I'll do it that way. This is quite timely, as I just finished
reworking the patch that relies on it. Thanks for the review.


cheers


andrew


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

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

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 = ('

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

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.


cheers


andrew


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

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a377cdb226..5ff138360c 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -87,6 +87,8 @@ our @EXPORT = qw(
 
 our ($windows_os, $tmp_check, $log_path, $test_logfile);
 
+my @no_stdin;
+
 BEGIN
 {
 
@@ -178,6 +180,20 @@ INIT
 	autoflush STDOUT 1;
 	autoflush STDERR 1;
 	autoflush $testlog 1;
+
+	# Settings to close stdin for certain commands.
+	# On non-Windows, use a pseudo-terminal, so that libraries like openssl
+	# which open the tty if they think stdin isn't one for a password
+	# don't block. Windows doesn't have ptys, so just provide an empty
+	# string for stdin.
+	if ($windows_os)
+	{
+		@no_stdin = ('<', \"");
+	}
+	else
+	{
+		@no_stdin = ('', \$stdout, '2>', \$stderr;
+	my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr, @no_stdin;
 	chomp($stdout);
 	chomp($stderr);
 	return ($stdout, $stderr);
@@ -576,7 +592,7 @@ sub check_pg_config
 	my ($regexp) = @_;
 	my ($stdout, $stderr);
 	my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
-	  \$stdout, '2>', \$stderr
+	  \$stdout, '2>', \$stderr, @no_stdin
 	  or die "could not execute pg_config";
 	chomp($stdout);
 	$stdout =~ s/\r$//;
@@ -673,7 +689,7 @@ sub program_help_ok
 	my ($stdout, $stderr);
 	print("# Running: $cmd --help\n");
 	my $result = IPC::Run::run [ $cmd, '--help' ], '>', \$stdout, '2>',
-	  \$stderr;
+	  \$stderr, @no_stdin;
 	ok($result, "$cmd --help exit code 0");
 	isnt($stdout, '', "$cmd --help goes to stdout");
 	is($stderr, '', "$cmd --help nothing to stderr");
@@ -695,7 +711,7 @@ sub program_version_ok
 	my ($stdout, $stderr);
 	print("# Running: $cmd --version\n");
 	my $result = IPC::Run::run [ $cmd, '--version' ], '>', \$stdout, '2>',
-	  \$stderr;
+	  \$stderr, @no_stdin;
 	ok($result, "$cmd --version exit code 0");
 	isnt($stdout, '', "$cmd --version goes to stdout");
 	is($stderr, '', "$cmd --version nothing to stderr");
@@ -718,8 +734,7 @@ sub program_options_handling_ok
 	my ($stdout, $stderr);
 	print("# Running: $cmd --not-a-valid-option\n");
 	my $result = IPC::Run::run [ $cmd, '--not-a-valid-option' ], '>',
-	  \$stdout,
-	  '2>', \$stderr;
+	  \$stdout, '2>', \$stderr, @no_stdin;
 	ok(!$result, "$cmd with invalid option nonzero exit code");
 	isnt($stderr, '', "$cmd with invalid option prints error message");
 	return;
@@ -740,7 +755,7 @@ sub command_like
 	my ($cmd, $expected_stdout, $test_name) = @_;
 	my ($stdout, $stderr);
 	print("# Running: " . join(" ", @{$cmd}) . "\n");
-	my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;
+	my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr, @no_stdin;
 	ok($result, "$test_name: exit code 0");
 	is($stderr, '', "$test_name: no stderr");
 	like($stdout, $expected_stdout, "$test_name: matches");
@@ -769,7 +784,8 @@ sub command_like_safe
 	my $stdoutfile = File::Temp->new();
 	my $stderrfile = File::Temp->new();
 	print("# Running: " . join(" ", @{$cmd}) . "\n");
-	my $result = IPC::Run::run $cmd, '>', $stdoutfile, '2>', $stderrfile;
+	my $result = IPC::Run::run $cmd, '>', $stdoutfile, '2>', $stderrfile,
+	  

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

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',
    '

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

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

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',
>>>    '>>
>>> 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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

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',
   '

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

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

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',
>   '
> 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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

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
>> 
>>
>
> 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 => ['
> 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',
  'https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

On 11/8/19 9:22 AM, Andrew Dunstan wrote:
...

This will need to be rewritten in light of the above, but see



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 => ['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

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

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 >> > 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



cheers


andrew


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

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

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 mailto:hornschnor...@gmail.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.


--
Mark Dilger

Re: TestLib::command_fails_like enhancement

[Andrew Dunstan]

On 11/8/19 1:16 AM, Craig Ringer wrote:
> On Fri, 8 Nov 2019 at 06:28, Mark Dilger  > 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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: TestLib::command_fails_like enhancement

[Craig Ringer]

On Fri, 8 Nov 2019 at 06:28, Mark Dilger  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

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

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

Re: TestLib::command_fails_like enhancement

[Michael Paquier]

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
Description: PGP signature

TestLib::command_fails_like enhancement

[Andrew Dunstan]

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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 905d0d178f..5264111d00 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -771,10 +771,16 @@ the given regular expression.
 sub command_fails_like
 {
 	local $Test::Builder::Level = $Test::Builder::Level + 1;
-	my ($cmd, $expected_stderr, $test_name) = @_;
+	my ($cmd, $expected_stderr, $test_name, %kwargs) = @_;
+	my @extra_ipcrun_opts = ();
+	if (defined($kwargs{'extra_ipcrun_opts'}))
+	{
+		push(@extra_ipcrun_opts, @{$kwargs{'extra_ipcrun_opts'}});
+	}
 	my ($stdout, $stderr);
 	print("# Running: " . join(" ", @{$cmd}) . "\n");
-	my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;
+	my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr,
+	  @extra_ipcrun_opts;
 	ok(!$result, "$test_name: exit code not 0");
 	like($stderr, $expected_stderr, "$test_name: matches");
 	return;