Re: [PATCH 01/25] t: use test_might_fail() instead of manipulating exit code manually

From: Stefan Beller <sbeller@google.com>
Date: Jul 2, 2018, 17:44 UTC
Message-ID: <CAGZ79kah+mFH0YZ1L0R+f=fRVku1mesigm86oHTHKRRFc3y+0A@mail.gmail.com>
In-Reply-To: <20180702002405.3042-2-sunshine@sunshineco.com>

Show 14 quoted lines

> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> index 0a8af76aab..6579c81216 100755
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -102,10 +102,8 @@ test_expect_success 'apply binary patch' '
>
>  test_expect_success 'diff --no-index with binary creation' '
>         echo Q | q_to_nul >binary &&
> -       (: hide error code from diff, which just indicates differences
> -        git diff --binary --no-index /dev/null binary >current ||
> -        true
> -       ) &&
> +       # hide error code from diff, which just indicates differences
> +       test_might_fail git diff --binary --no-index /dev/null binary >current &&

I am not sure why we need to be non-deterministic here, i.e. I would rather test for success or non-success error code and not just *any* error code.

This code was introduced in 71b989e7dd1 (fix bogus "diff --git" header from "diff --no-index", 2008-10-05) whereas the test_must_fail was introduced in 74359821020 (tests: introduce test_must_fail, 2008-02-28). However this code was authored without Junios involvement (he was AFK 2008-09-23..2008-10-08), so maybe test_must_fail was not so popular back then?

While I think this patch is a strict improvement for the test suite, I do wonder if we can tighten the exit code check here (maybe in a follow up series?).

Thanks, Stefan