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
