| Message ID | xmqqblnaufyt.fsf_-_@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) | expand |
On Wed, Apr 29, 2020 at 02:42:02PM -0700, Junio C Hamano wrote: > 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
On Wed, Apr 29, 2020 at 5:42 PM Junio C Hamano <gitster@pobox.com> wrote: > Subject: [PATCH] t/README: document when not to use test_might_fail > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git 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. This seems like a good addition and perhaps may help reduce reviewer burden (not that this comes up very often, but I've recommended it here and there when reviewing patches). Here's a possible alternate wording: Similar to test_must_fail, but tolerate success, too. Use this instead of "<git-command> || :" to catch failures due to segv. To ignore failure of non-git commands, however, use "{ <non-git-command> || :; }".
Eric Sunshine <sunshine@sunshineco.com> writes: > This seems like a good addition and perhaps may help reduce reviewer > burden (not that this comes up very often, but I've recommended it > here and there when reviewing patches). Here's a possible alternate > wording: > > Similar to test_must_fail, but tolerate success, too. Use this > instead of "<git-command> || :" to catch failures due to segv. > To ignore failure of non-git commands, however, use > "{ <non-git-command> || :; }". That is certainly more succinct. "git grep -e '|| :' t/" gives some hits that are candidates to convert to test_might_fail, by the way.
Taylor Blau <me@ttaylorr.com> writes: > 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. It comes from the same principle as "we are not in the business of catching segv from system tools---don't use test_must_fail on non-git commands". Adopting the convention happened quite some time ago and that was why I checked if we failed to document it. What I wondered was if it is overkill to document the convention; if the convention was overkill is not a question at this point.
On Wed, Apr 29, 2020 at 03:10:50PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > 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. > > It comes from the same principle as "we are not in the business of > catching segv from system tools---don't use test_must_fail on > non-git commands". Adopting the convention happened quite some time > ago and that was why I checked if we failed to document it. > > What I wondered was if it is overkill to document the convention; if > the convention was overkill is not a question at this point. Ah, fair enough. Thanks for a patient explanation. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > 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. > ... > Junio: do you want another reroll of that series? :/ The one I saw and remember was two new umask calls protected in POSIXPERM prerequisite but without test-might-fail involved. Perhaps there is nothing to reroll? Or perhaps I am not checking my mailbox often enough?
On Wed, Apr 29, 2020 at 03:36:25PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > 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. > > ... > > Junio: do you want another reroll of that series? :/ > > The one I saw and remember was two new umask calls protected in POSIXPERM > prerequisite but without test-might-fail involved. > > Perhaps there is nothing to reroll? Or perhaps I am not checking my > mailbox often enough? You are checking your mailbox often enough, but unfortunately my memory isn't as good as I thought ;). You're right: those calls are in POSIXPERM-only tests, and don't have a 'test_might_fail' in front of them as such. That was easy ;). Thanks, Taylor
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. Accepts the same options as test_must_fail.