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

[Junio C Hamano <gitster@pobox.com>]

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.