Thread: Re: TAP test command_fails versus command_fails_like
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: https://postgr.es/m/20220902190329.jxvdeehkbfnrfmtl%40awork3.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