Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

[SZEDER Gábor]

On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:
> On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:
> 
> > In many test helper functions we verify that they were invoked with
> > sensible parameters, and call BUG() to abort the test script when the
> > parameters are buggy.  6a67c75948 (test-lib-functions: restrict
> > test_must_fail usage, 2020-07-07) added such a parameter verification
> > to 'test_must_fail', but it didn't report the error with BUG(), like
> > we usually do.
> 
> OK. I do not care all that much between BUG() and not-BUG here, since we
> are unlikely to have a test where test_must_fail returning 0 yields
> success. I guess the most interesting outcome is that we would notice a
> bug in a test_expect_failure block.

If I had managed to send a new version of this patch series in the
last 5 years :), then this would have caught the issue noted in:

  https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/

Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:
>> On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:
>> 
>> > In many test helper functions we verify that they were invoked with
>> > sensible parameters, and call BUG() to abort the test script when the
>> > parameters are buggy.  6a67c75948 (test-lib-functions: restrict
>> > test_must_fail usage, 2020-07-07) added such a parameter verification
>> > to 'test_must_fail', but it didn't report the error with BUG(), like
>> > we usually do.
>> 
>> OK. I do not care all that much between BUG() and not-BUG here, since we
>> are unlikely to have a test where test_must_fail returning 0 yields
>> success. I guess the most interesting outcome is that we would notice a
>> bug in a test_expect_failure block.
>
> If I had managed to send a new version of this patch series in the
> last 5 years :), then this would have caught the issue noted in:
>
>   https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/

I was wondering if we should remove "test_might_fail".  Its use case
is rather limited to very narrow cases, like

 * we want to kill something but it may have exited on its own

 * we want "git config --unset" but the variable may or may not be set

 * we want "git foo --abort" just in case we are in the middle of
   "git foo"

all of which is clearer with "|| :", and more importantly, the thing
whose "failure" is protected against the test framework declaring a
test failure is *not* what we are testing (these "config --unset"
are not about testing "git config", in other words).

So the extra ability test_must_fail and test_might_fail have that
they can detect uncontrolled death with non-zero exit status (aka
"crash") is not very interesting---it is more like "As we are
running a git command here, it would be better to catch than not
catch a segfault here as well", i.e., a nice to have item.

Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

[Jeff King]

On Tue, Apr 14, 2026 at 02:11:00PM -0700, Junio C Hamano wrote:

> I was wondering if we should remove "test_might_fail".  Its use case
> is rather limited to very narrow cases, like
> 
>  * we want to kill something but it may have exited on its own
> 
>  * we want "git config --unset" but the variable may or may not be set
> 
>  * we want "git foo --abort" just in case we are in the middle of
>    "git foo"
> 
> all of which is clearer with "|| :", and more importantly, the thing
> whose "failure" is protected against the test framework declaring a
> test failure is *not* what we are testing (these "config --unset"
> are not about testing "git config", in other words).
> 
> So the extra ability test_must_fail and test_might_fail have that
> they can detect uncontrolled death with non-zero exit status (aka
> "crash") is not very interesting---it is more like "As we are
> running a git command here, it would be better to catch than not
> catch a segfault here as well", i.e., a nice to have item.

I think the main value of both (but especially test_might_fail) is that
they slot naturally into &&-chains. I left a similar comment in that
other thread, but to expand a bit, if you do:

  false &&
  true || : &&
  echo everything ok

you will get "everything ok", even though step 1 failed. You need:

  false &&
  { true || : } &&
  echo everything ok

except that because it is shell you have to add an extra semicolon after
the ":". ;)

Syntax-complaints aside, I think it is a very easy thing for
contributors to get wrong. So I think test_might_fail has value, though
I do not care if it has a different name.

-Peff

Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think the main value of both (but especially test_might_fail) is that
> they slot naturally into &&-chains. I left a similar comment in that
> other thread, but to expand a bit, if you do:
>
>   false &&
>   true || : &&
>   echo everything ok
>
> you will get "everything ok", even though step 1 failed. You need:
>
>   false &&
>   { true || : } &&
>   echo everything ok
>
> except that because it is shell you have to add an extra semicolon after
> the ":". ;)
>
> Syntax-complaints aside, I think it is a very easy thing for
> contributors to get wrong. So I think test_might_fail has value, though
> I do not care if it has a different name.

Yup, I recall that I recently said that I hate that semicolon ;-)

Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'

[Jeff King]

On Tue, Apr 14, 2026 at 10:52:33PM +0200, SZEDER Gábor wrote:

> On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:
> > On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:
> > 
> > > In many test helper functions we verify that they were invoked with
> > > sensible parameters, and call BUG() to abort the test script when the
> > > parameters are buggy.  6a67c75948 (test-lib-functions: restrict
> > > test_must_fail usage, 2020-07-07) added such a parameter verification
> > > to 'test_must_fail', but it didn't report the error with BUG(), like
> > > we usually do.
> > 
> > OK. I do not care all that much between BUG() and not-BUG here, since we
> > are unlikely to have a test where test_must_fail returning 0 yields
> > success. I guess the most interesting outcome is that we would notice a
> > bug in a test_expect_failure block.
> 
> If I had managed to send a new version of this patch series in the
> last 5 years :), then this would have caught the issue noted in:
> 
>   https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/

Looking at that old thread, I do not see any reason you should not
re-send it (with or without the cosmetic fixups I suggested on top).

-Peff