Re: TAP test command_fails versus command_fails_like

On Thu, Feb 13, 2025 at 12:58 AM Dagfinn Ilmari Mannsåker
<ilmari(at)ilmari(dot)org> wrote:
>
> Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> writes:
>
> > On Wed, Feb 12, 2025 at 10:36 AM Peter Smith <smithpb2250(at)gmail(dot)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