Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 29, 2020, 21:49 UTC
Message-ID: <20200429214906.GA12075@syl.local>
In-Reply-To: <xmqqblnaufyt.fsf_-_@gitster.c.googlers.com>

On Wed, Apr 29, 2020 at 02:42:02PM -0700, Junio C Hamano wrote:

Show 39 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 29.04.20 um 21:50 schrieb Taylor Blau:
> >> This comment has nothing to do with your series, but I am curious if you
> >> are planning on touching 'test_might_fail' at all. These can be useful
> >> for non-Git commands, too, such as 'test_might_fail umask 022' on
> >> systems that may or may not do something sensible with umask.
> >
> > When it's not a git command that might fail, the idiom is
> >
> > 	... &&
> > 	{ umask 022 || :; } &&
> > 	...
> >
> > -- Hannes
>
> I hoped to find this documented in t/README, but ended up writing
> this.  Overkill?  I dunno.
>
> -- >8 --
> Subject: [PATCH] t/README: document when not to use test_might_fail
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/README | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/README b/t/README
> index 13747f1344..870950c7d1 100644
> --- a/t/README
> +++ b/t/README
> @@ -875,7 +875,9 @@ library for your script to use.
>   - test_might_fail [<options>] <git-command>
>
>     Similar to test_must_fail, but tolerate success, too.  Use this
> -   instead of "<git-command> || :" to catch failures due to segv.
> +   instead of "<git-command> || :" to catch failures due to segv,
> +   but do use "{ <non-git-command> || :; }" to ignore a failure from
> +   a command that is not git.

Hmm. I say this as somebody who just re-rolled a series to add two 'test_might_fail umask 022' lines, so am a little disappointed to learn that this is not considered to be idiomatic.

To me this seems a little overkill, but it may not be on environments where an extra subshell incurred by 'test_might_fail' might be overly expensive.

Junio: do you want another reroll of that series? :/

>     Accepts the same options as test_must_fail.

Thanks, Taylor