On Thu, Feb 13, 2025 at 12:58 AM Dagfinn Ilmari Mannsåker <ilm...@ilmari.org> wrote: > > Ashutosh Bapat <ashutosh.bapat....@gmail.com> writes: > > > On Wed, Feb 12, 2025 at 10:36 AM Peter Smith <smithpb2...@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
v2-0001-Help-prevent-users-from-calling-the-wrong-functio.patch
Description: Binary data
