Subject: Re: [PATCH 1/2] diff --no-index tests: add test for --exit-code

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sun, 21 Mar 2021 23:44:23 +0100

On Sun, Mar 21 2021, Junio C Hamano wrote:

> Ramsay Jones <[email-masked]> writes:
>
>> On 21/03/2021 12:39, Ævar Arnfjörð Bjarmason wrote:
>>> Add a test for --exit-code working with --no-index. There's no reason
>>> to suppose it wouldn't, but we weren't testing for it anywhere in our
>>> tests. Let's fix that blind spot.
>>> 
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
>>> ---
>>>  t/t4053-diff-no-index.sh | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
>>> index 0168946b639..9b7a8ebfd3f 100755
>>> --- a/t/t4053-diff-no-index.sh
>>> +++ b/t/t4053-diff-no-index.sh
>>> @@ -16,7 +16,12 @@ test_expect_success 'setup' '
>>>  	echo 1 >non/git/b
>>>  '
>>>  
>>> -test_expect_success 'git diff --no-index directories' '
>>> +test_expect_success 'git diff --no-index --exit-code' '
>>> +	git diff --no-index --exit-code a/1 non/git/a &&
>>> +	test_expect_code 1 git diff --no-index --exit-code a/1 a/2
>>> +'
>>> +
>>> +Test_expect_success 'git diff --no-index directories' '
>>
>> I assume that s/test/Test/ was not intended. ;-)
>
> ;-)  
>
> Love to see reviewers are more careful than submitters' shells that
> are too lenient to allow such a test to pass before such a patch
> gets submitted.

Sorry about that, and thanks Ramsey for spotting it.

FWIW I don't think there's any shell you can run the tests with that
would catch this. We won't run the tests at all with "set -e"[1], and we
don't use "set -e" in the tests themselves by convention.

So you'll see a notice about an unknown Test_expect_success function in
the output if you're not blind to it as I apparently was, but the the
test will succeed, CI will pass etc.

1. In this case the thing holding it back is the structure of the sanity
   check added in 2006f0adaee (t/test-lib: make sure Git has already
   been built, 2012-09-17), we'd proceed if it was part of the "if",
   hrm...