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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 2, 2018, 20:58 UTC
Message-ID: <CAPig+cSpDi8o6s2dQoDqb5_fbZ1ROY0guPxptQ=6syK6r9Shcg@mail.gmail.com>
In-Reply-To: <CAGZ79kah+mFH0YZ1L0R+f=fRVku1mesigm86oHTHKRRFc3y+0A@mail.gmail.com>

On Mon, Jul 2, 2018 at 1:44 PM Stefan Beller <sbeller@google.com> wrote:

Show 16 quoted lines

> > diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> >  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.
>
> 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?).

I agree that such a change is out of the scope of this patch series.

But, do keep in mind that the exit code of git-diff doesn't indicate only "success" or "error". Various exit codes represent different conditions; for instance, 1 means differences were found, 0 means no differences, and -1 means error. The outcome of this test is based upon output generated by git-diff, so the test itself will fail overall if git-diff doesn't produce expected results. Also, there is an entire test script (t4017-diff-retval.sh) concerned with verifying git-diff exit code, so I'm not sure we need to worry about it too much here.

(I'm not arguing against a follow-up patch, though, just thinking aloud about its potential value.)