TAP test command_fails versus command_fails_like
Hi hackers,
Recently, while writing some new TAP tests my colleague inadvertently
called the command_fails() subroutine instead of command_fails_like()
subroutine. Their parameters are almost the same but
command_fails_like() also takes a pattern for checking in the logs.
Notice if too many parameters are passed to command_fails they get
silently ignored.
Because of the mistake, the tests were all bogus because they were no
longer testing error messages as was the intention. OTOH, because
command failure was expected those tests still would record a "pass"
despite the wrong subroutine being used, so there is no evidence that
anything is wrong.
I wondered if the command_fails() subroutine could have done more to
protect users from accidentally shooting themselves. My attached patch
does this by ensuring that no "extra" (unexpected) parameters are
being passed to command_fails(). It seems more foolproof.
Thoughts?
(make check-world passes).
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Help-prevent-users-from-calling-the-wrong-functio.patchapplication/octet-stream; name=v1-0001-Help-prevent-users-from-calling-the-wrong-functio.patchDownload+3-2
On Wed, Feb 12, 2025 at 10:36 AM Peter Smith <smithpb2250@gmail.com> wrote:
Hi hackers,
Recently, while writing some new TAP tests my colleague inadvertently
called the command_fails() subroutine instead of command_fails_like()
subroutine. Their parameters are almost the same but
command_fails_like() also takes a pattern for checking in the logs.
Notice if too many parameters are passed to command_fails they get
silently ignored.Because of the mistake, the tests were all bogus because they were no
longer testing error messages as was the intention. OTOH, because
command failure was expected those tests still would record a "pass"
despite the wrong subroutine being used, so there is no evidence that
anything is wrong.I wondered if the command_fails() subroutine could have done more to
protect users from accidentally shooting themselves. My attached patch
does this by ensuring that no "extra" (unexpected) parameters are
being passed to command_fails(). It seems more foolproof.
We will need to fix many perl functions this way but I think
command_fails and command_fails_like or any other pair of similarly
named functions needs better protection. Looking at
https://stackoverflow.com/questions/19234209/perl-subroutine-arguments,
I feel a construct like below is more readable and crisp.
die "Too many arguments for subroutine" unless @_ <= 1;
Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?
This won't eliminate cases where command_like is used instead of
command_like_safe, and vice-versa, where logic is slightly different.
So the question is how far do we go detecting such misuses?
--
Best Wishes,
Ashutosh Bapat
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
On Wed, Feb 12, 2025 at 10:36 AM Peter Smith <smithpb2250@gmail.com> wrote:
Hi hackers,
Recently, while writing some new TAP tests my colleague inadvertently
called the command_fails() subroutine instead of command_fails_like()
subroutine. Their parameters are almost the same but
command_fails_like() also takes a pattern for checking in the logs.
Notice if too many parameters are passed to command_fails they get
silently ignored.Because of the mistake, the tests were all bogus because they were no
longer testing error messages as was the intention. OTOH, because
command failure was expected those tests still would record a "pass"
despite the wrong subroutine being used, so there is no evidence that
anything is wrong.I wondered if the command_fails() subroutine could have done more to
protect users from accidentally shooting themselves. My attached patch
does this by ensuring that no "extra" (unexpected) parameters are
being passed to command_fails(). It seems more foolproof.We will need to fix many perl functions this way but I think
command_fails and command_fails_like or any other pair of similarly
named functions needs better protection. Looking at
https://stackoverflow.com/questions/19234209/perl-subroutine-arguments,
I feel a construct like below is more readable and crisp.die "Too many arguments for subroutine" unless @_ <= 1;
I would write that as `if @_ > 1`, but otherwise I agree. This is a
programmer error, not a test failure, so it should use die() (or even
croak(), to report the error from the caller's perspective), not is().
Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?
If we do this, we should do it across the board for
PostgreSQL::Test::Utils and ::Cluster at least. Once we bump the
minimum perl version to 5.20 or beyond we should switch to using
function signatures (https://perldoc.perl.org/perlsub#Signatures), which
gives us this checking for free.
This won't eliminate cases where command_like is used instead of
command_like_safe, and vice-versa, where logic is slightly different.
So the question is how far do we go detecting such misuses?
command_like and command_like_safe take exactly the same parameters, the
only difference is how they capture stdout/stderr, so there's no way to
tell which one the caller meant. What we can detect however is if
command_ok is called when someone meant command_like.
- imari
On 2025-02-12 We 8:58 AM, Dagfinn Ilmari Mannsåker wrote:
Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?If we do this, we should do it across the board for
PostgreSQL::Test::Utils and ::Cluster at least. Once we bump the
minimum perl version to 5.20 or beyond we should switch to using
function signatures (https://perldoc.perl.org/perlsub#Signatures), which
gives us this checking for free.
Is there any reason we can't move to 5.20? Are there any buildfarm
animals using such an old version? 5.20 is now almost 10 years old.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
On 2025-02-12 We 8:58 AM, Dagfinn Ilmari Mannsåker wrote:
Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?If we do this, we should do it across the board for
PostgreSQL::Test::Utils and ::Cluster at least. Once we bump the
minimum perl version to 5.20 or beyond we should switch to using
function signatures (https://perldoc.perl.org/perlsub#Signatures), which
gives us this checking for free.Is there any reason we can't move to 5.20? Are there any buildfarm
animals using such an old version? 5.20 is now almost 10 years old.
Red Hat Enterprise Linux 7 has Perl 5.16 and is on Extended Lifecycle
Support until 2028-06-30. I don't know how long other distros based on
that (e.g. CentOS, Scientific Linux) are supported, but I can see that
Amazon Linux 2 is almost out of support (2025-06-30).
cheers
andrew
- ilmari
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
Andrew Dunstan <andrew@dunslane.net> writes:
On 2025-02-12 We 8:58 AM, Dagfinn Ilmari Mannsåker wrote:
Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?If we do this, we should do it across the board for
PostgreSQL::Test::Utils and ::Cluster at least. Once we bump the
minimum perl version to 5.20 or beyond we should switch to using
function signatures (https://perldoc.perl.org/perlsub#Signatures), which
gives us this checking for free.Is there any reason we can't move to 5.20? Are there any buildfarm
animals using such an old version? 5.20 is now almost 10 years old.Red Hat Enterprise Linux 7 has Perl 5.16 and is on Extended Lifecycle
Support until 2028-06-30. I don't know how long other distros based on
that (e.g. CentOS, Scientific Linux) are supported, but I can see that
Amazon Linux 2 is almost out of support (2025-06-30).
Ah, I found the thread from when we bumped it from 5.8 to 5.14 in 2022,
which has a list of then-current perl versions in the buildfarm:
/messages/by-id/20220902190329.jxvdeehkbfnrfmtl@awork3.anarazel.de
- ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
Andrew Dunstan <andrew@dunslane.net> writes:
Is there any reason we can't move to 5.20? Are there any buildfarm
animals using such an old version? 5.20 is now almost 10 years old.
Red Hat Enterprise Linux 7 has Perl 5.16 and is on Extended Lifecycle
Support until 2028-06-30. I don't know how long other distros based on
that (e.g. CentOS, Scientific Linux) are supported, but I can see that
Amazon Linux 2 is almost out of support (2025-06-30).
The oldest perl versions I see in the buildfarm are
longfin | 2025-02-12 14:42:03 | configure: using perl 5.14.0
lapwing | 2025-02-10 16:24:03 | configure: using perl 5.14.2
arowana | 2025-02-12 04:08:19 | configure: using perl 5.16.3
boa | 2025-02-12 16:28:51 | configure: using perl 5.16.3
buri | 2025-02-11 21:11:11 | configure: using perl 5.16.3
dhole | 2025-02-12 06:08:04 | configure: using perl 5.16.3
rhinoceros | 2025-02-12 14:52:10 | configure: using perl 5.16.3
siskin | 2025-02-11 21:39:44 | configure: using perl 5.16.3
snakefly | 2025-02-10 01:00:04 | configure: using perl 5.16.3
shelduck | 2025-02-12 12:13:18 | configure: using perl 5.18.2
topminnow | 2025-01-27 08:22:18 | configure: using perl 5.20.2
batfish | 2025-02-12 15:38:00 | configure: using perl 5.22.1
cuon | 2025-02-12 05:08:03 | configure: using perl 5.22.1
ayu | 2025-02-12 13:15:26 | configure: using perl 5.24.1
chimaera | 2025-02-12 11:14:35 | configure: using perl 5.24.1
urocryon | 2025-02-12 02:07:16 | configure: using perl 5.24.1
(This scan did not account for meson-using animals, which I'm assuming
are generally newer.)
longfin is my own animal and is intentionally set up with the oldest
supported Perl version; I'd have no problem moving it to another
version if we decide to move that goalpost. lapwing I believe we've
already nagged the owner of to update (its flex is museum-grade too).
But it looks like moving past 5.16 would be more problematic.
regards, tom lane
On Thu, Feb 13, 2025 at 12:58 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
On Wed, Feb 12, 2025 at 10:36 AM Peter Smith <smithpb2250@gmail.com> wrote:
Hi hackers,
Recently, while writing some new TAP tests my colleague inadvertently
called the command_fails() subroutine instead of command_fails_like()
subroutine. Their parameters are almost the same but
command_fails_like() also takes a pattern for checking in the logs.
Notice if too many parameters are passed to command_fails they get
silently ignored.Because of the mistake, the tests were all bogus because they were no
longer testing error messages as was the intention. OTOH, because
command failure was expected those tests still would record a "pass"
despite the wrong subroutine being used, so there is no evidence that
anything is wrong.I wondered if the command_fails() subroutine could have done more to
protect users from accidentally shooting themselves. My attached patch
does this by ensuring that no "extra" (unexpected) parameters are
being passed to command_fails(). It seems more foolproof.We will need to fix many perl functions this way but I think
command_fails and command_fails_like or any other pair of similarly
named functions needs better protection. Looking at
https://stackoverflow.com/questions/19234209/perl-subroutine-arguments,
I feel a construct like below is more readable and crisp.die "Too many arguments for subroutine" unless @_ <= 1;
I would write that as `if @_ > 1`, but otherwise I agree. This is a
programmer error, not a test failure, so it should use die() (or even
croak(), to report the error from the caller's perspective), not is().Another question is whether command_fails and command_fails_like is
the only pair or there are more which need stricter checks?If we do this, we should do it across the board for
PostgreSQL::Test::Utils and ::Cluster at least.
Here is a v2 patch covering many more subroutines, using the syntax
that was suggested above.
make check-world passes.
======
Kind Regards,
Peter Smith.
Fujitsu Australia