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