Re: [PATCH 01/25] t: use test_might_fail() instead of manipulating exit code manually
- Date: Mon, 2 Jul 2018 16:58:13 -0400
- From: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
- Subject: Re: [PATCH 01/25] t: use test_might_fail() instead of manipulating exit code manually
On Mon, Jul 2, 2018 at 1:44 PM Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> > 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
(I'm not arguing against a follow-up patch, though, just thinking
aloud about its potential value.)