Re: [PATCHv4 02/15] t4017 (diff-retval): replace manual exit code check with test_expect_code

[Jonathan Nieder <jrnieder@gmail.com>]

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Sep 29, 2010 at 18:07, Junio C Hamano <gitster@pobox.com> wrote:
>> Elijah Newren <newren@gmail.com> writes:

>>> -test_expect_success 'git diff-tree HEAD^ HEAD' '
>>> +test_expect_code 1 'git diff-tree HEAD^ HEAD' '
>>>       git diff-tree --exit-code HEAD^ HEAD
>>> -     test $? = 1
>>>  '
>
> It also looks like this will pass for for all exit codes that *aren't*
> 1, because if $? != 1 +test_expect_code will get the exit code of
> 1.

You probably missed the - indicating that the "test $? = 1" was being
removed.

But using test_expect_code indeed has a similar problem to the one
you point out: it does not nicely discern between outcomes in a test
with multiple commands.  This shows up in some of the later tests in
this script (which is presumably why the original patch did not
touch them).

Maybe something like this would improve things?

-- 8< --
Subject: t4017 (diff-retval): factor out check_exit_status helper

When reporting a difference with --exit-code, diff is not only
supposed to fail but to fail with status 1.  Unfortunately, the shell
code to check for that has to amount to

	git diff --exit-code this that
	test $? = 1

which breaks &&-chaining if the script author is not cautious.

This test script already avoids trouble by putting such code in { }
blocks, but that is a bit clunky; better to notice the pattern and
give it its own function.

While at it, improve the output on failure when debugging with -v
by printing the expected and actual exit code (by using test_cmp).
The temporary files expect.status and actual.status used to
bring this about do not adversely affect the test because those
filenames are not tracked, hence not candidates for diffing.

Noticed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4017-diff-retval.sh |   93 ++++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 61 deletions(-)

diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 6158985..51a009b 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -4,6 +4,14 @@ test_description='Return value of diffs'
 
 . ./test-lib.sh
 
+check_exit_status () {
+	echo "$1" >expect.status
+	shift
+	"$@"
+	echo "$?" >actual.status
+	test_cmp expect.status actual.status
+}
+
 test_expect_success 'setup' '
 	echo "1 " >a &&
 	git add . &&
@@ -21,110 +29,81 @@ test_expect_success 'git diff --quiet -w  HEAD^^ HEAD^' '
 '
 
 test_expect_success 'git diff --quiet HEAD^^ HEAD^' '
-	test_must_fail git diff --quiet HEAD^^ HEAD^
+	check_exit_status 1 git diff --quiet HEAD^^ HEAD^
 '
 
 test_expect_success 'git diff --quiet -w  HEAD^ HEAD' '
-	test_must_fail git diff --quiet -w HEAD^ HEAD
+	check_exit_status 1 git diff --quiet -w HEAD^ HEAD
 '
 
 test_expect_success 'git diff-tree HEAD^ HEAD' '
-	git diff-tree --exit-code HEAD^ HEAD
-	test $? = 1
+	check_exit_status 1 git diff-tree --exit-code HEAD^ HEAD
 '
 test_expect_success 'git diff-tree HEAD^ HEAD -- a' '
 	git diff-tree --exit-code HEAD^ HEAD -- a
-	test $? = 0
 '
 test_expect_success 'git diff-tree HEAD^ HEAD -- b' '
-	git diff-tree --exit-code HEAD^ HEAD -- b
-	test $? = 1
+	check_exit_status 1 git diff-tree --exit-code HEAD^ HEAD -- b
 '
 test_expect_success 'echo HEAD | git diff-tree --stdin' '
-	echo $(git rev-parse HEAD) | git diff-tree --exit-code --stdin
-	test $? = 1
+	git rev-parse HEAD |
+	check_exit_status 1 git diff-tree --exit-code --stdin
 '
 test_expect_success 'git diff-tree HEAD HEAD' '
 	git diff-tree --exit-code HEAD HEAD
-	test $? = 0
 '
 test_expect_success 'git diff-files' '
 	git diff-files --exit-code
-	test $? = 0
 '
 test_expect_success 'git diff-index --cached HEAD' '
 	git diff-index --exit-code --cached HEAD
-	test $? = 0
 '
 test_expect_success 'git diff-index --cached HEAD^' '
-	git diff-index --exit-code --cached HEAD^
-	test $? = 1
+	check_exit_status 1 git diff-index --exit-code --cached HEAD^
 '
 test_expect_success 'git diff-index --cached HEAD^' '
 	echo text >>b &&
 	echo 3 >c &&
-	git add . && {
-		git diff-index --exit-code --cached HEAD^
-		test $? = 1
-	}
+	git add . &&
+	check_exit_status 1 git diff-index --exit-code --cached HEAD^
 '
 test_expect_success 'git diff-tree -Stext HEAD^ HEAD -- b' '
-	git commit -m "text in b" && {
-		git diff-tree -p --exit-code -Stext HEAD^ HEAD -- b
-		test $? = 1
-	}
+	git commit -m "text in b" &&
+	check_exit_status 1 git diff-tree -p --exit-code -Stext HEAD^ HEAD -- b
 '
 test_expect_success 'git diff-tree -Snot-found HEAD^ HEAD -- b' '
 	git diff-tree -p --exit-code -Snot-found HEAD^ HEAD -- b
-	test $? = 0
 '
 test_expect_success 'git diff-files' '
-	echo 3 >>c && {
-		git diff-files --exit-code
-		test $? = 1
-	}
+	echo 3 >>c &&
+	check_exit_status 1 git diff-files --exit-code
 '
 test_expect_success 'git diff-index --cached HEAD' '
-	git update-index c && {
-		git diff-index --exit-code --cached HEAD
-		test $? = 1
-	}
+	git update-index c &&
+	check_exit_status 1 git diff-index --exit-code --cached HEAD
 '
 
 test_expect_success '--check --exit-code returns 0 for no difference' '
-
 	git diff --check --exit-code
-
 '
 
 test_expect_success '--check --exit-code returns 1 for a clean difference' '
-
-	echo "good" > a &&
-	git diff --check --exit-code
-	test $? = 1
-
+	echo "good" >a &&
+	check_exit_status 1 git diff --check --exit-code
 '
 
 test_expect_success '--check --exit-code returns 3 for a dirty difference' '
-
 	echo "bad   " >> a &&
-	git diff --check --exit-code
-	test $? = 3
-
+	check_exit_status 3 git diff --check --exit-code
 '
 
 test_expect_success '--check with --no-pager returns 2 for dirty difference' '
-
-	git --no-pager diff --check
-	test $? = 2
-
+	check_exit_status 2 git --no-pager diff --check
 '
 
 test_expect_success 'check should test not just the last line' '
 	echo "" >>a &&
-	git --no-pager diff --check
-	test $? = 2
-
+	check_exit_status 2 git --no-pager diff --check
 '
 
 test_expect_success 'check detects leftover conflict markers' '
@@ -133,10 +112,8 @@ test_expect_success 'check detects leftover conflict markers' '
 	echo binary >>b &&
 	git commit -m "side" b &&
 	test_must_fail git merge master &&
-	git add b && (
-		git --no-pager diff --cached --check >test.out
-		test $? = 2
-	) &&
+	git add b &&
+	check_exit_status 2 git --no-pager diff --cached --check >test.out &&
 	test 3 = $(grep "conflict marker" test.out | wc -l) &&
 	git reset --hard
 '
@@ -146,19 +123,13 @@ test_expect_success 'check honors conflict marker length' '
 	echo ">>>>>>> boo" >>b &&
 	echo "======" >>a &&
 	git diff --check a &&
-	(
-		git diff --check b
-		test $? = 2
-	) &&
+	check_exit_status 2 git diff --check b &&
 	git reset --hard &&
 	echo ">>>>>>>> boo" >>b &&
 	echo "========" >>a &&
 	git diff --check &&
 	echo "b conflict-marker-size=8" >.gitattributes &&
-	(
-		git diff --check b
-		test $? = 2
-	) &&
+	check_exit_status 2 git diff --check b &&
 	git diff --check a &&
 	git reset --hard
 '
-- 
1.7.2.3