[PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Stefan Beller]

The `test_must_fail` should only be used to indicate a git command is
failing. `test_cmp` is not a git command, such that it doesn't need the
special treatment of `test_must_fail` (which e.g. includes checking for
segfault)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t3504-cherry-pick-rerere.sh         | 2 +-
 t/t5512-ls-remote.sh                  | 2 +-
 t/t5612-clone-refspec.sh              | 2 +-
 t/t7508-status.sh                     | 2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..c31141c471 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_config rerere.enabled false &&
 	test_must_fail git cherry-pick master &&
-	test_must_fail test_cmp expect foo
+	! test_cmp expect foo
 '
 
 test_done
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c9226..7178b917ce 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote specified' '
 
 test_expect_success 'suppress "From <url>" with -q' '
 	git ls-remote -q 2>actual_err &&
-	test_must_fail test_cmp exp_err actual_err
+	! test_cmp exp_err actual_err
 '
 
 test_expect_success 'use branch.<name>.remote if possible' '
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index fac5a73851..5f9ad51929 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-	test_must_fail test_cmp expect actual &&
+	! test_cmp expect actual &&
 	test_line_count = 2 actual
 '
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 93f162a4f7..1644866571 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
 test_expect_success '"status.branch=true" different from "--no-branch"' '
 	git status -s --no-branch  >expected_nobranch &&
 	git -c status.branch=true status -s >actual &&
-	test_must_fail test_cmp expected_nobranch actual
+	! test_cmp expected_nobranch actual
 '
 
 test_expect_success '"status.branch=true" weaker than "--no-branch"' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index d8464d4218..5cd6b40432 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
 		echo 1 >> file &&
 		svn_cmd commit -m "changing file" &&
 		svn_cmd up &&
-		test_must_fail test_cmp auto_updated_file au_file_saved
+		! test_cmp auto_updated_file au_file_saved
 	)
 '
 
-- 
2.14.0.rc0.3.g6c2e499285

[PATCH 2/2] tests: fix diff order arguments in test_cmp

[Stefan Beller]

Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1004-read-tree-m-u-wf.sh          |  2 +-
 t/t4015-diff-whitespace.sh           |  4 ++--
 t/t4205-log-pretty-formats.sh        |  2 +-
 t/t6007-rev-list-cherry-pick-file.sh | 32 ++++++++++++++++----------------
 t/t6013-rev-list-reverse-parents.sh  |  4 ++--
 t/t7001-mv.sh                        |  2 +-
 t/t7005-editor.sh                    |  6 +++---
 t/t7102-reset.sh                     |  4 ++--
 t/t7201-co.sh                        |  4 ++--
 t/t7400-submodule-basic.sh           |  2 +-
 t/t7405-submodule-merge.sh           |  2 +-
 t/t7506-status-submodule.sh          |  4 ++--
 t/t7600-merge.sh                     |  6 +++---
 t/t7610-mergetool.sh                 |  4 ++--
 t/t9001-send-email.sh                |  6 +++---
 15 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index c70cf42300..c7ce5d8bb5 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -218,7 +218,7 @@ test_expect_success 'D/F' '
 		echo "100644 $a 2	subdir/file2"
 		echo "100644 $b 3	subdir/file2/another"
 	) >expect &&
-	test_cmp actual expect
+	test_cmp expect actual
 
 '
 
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 12d182dc1b..1709e4578b 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -155,7 +155,7 @@ test_expect_success 'ignore-blank-lines: only new lines' '
 " >x &&
 	git diff --ignore-blank-lines >out &&
 	>expect &&
-	test_cmp out expect
+	test_cmp expect out
 '
 
 test_expect_success 'ignore-blank-lines: only new lines with space' '
@@ -165,7 +165,7 @@ test_expect_success 'ignore-blank-lines: only new lines with space' '
  " >x &&
 	git diff -w --ignore-blank-lines >out &&
 	>expect &&
-	test_cmp out expect
+	test_cmp expect out
 '
 
 test_expect_success 'ignore-blank-lines: after change' '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f530102..42f584f8b3 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 test_expect_success ':only and :unfold work together' '
 	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
 	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
-	test_cmp actual reverse &&
+	test_cmp reverse actual &&
 	{
 		grep -v patch.description <trailers | unfold &&
 		echo
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 2959745196..f0268372d2 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -57,7 +57,7 @@ test_expect_success '--left-right' '
 	git rev-list --left-right B...C > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 test_expect_success '--count' '
@@ -77,14 +77,14 @@ test_expect_success '--cherry-pick bar does not come up empty' '
 	git rev-list --left-right --cherry-pick B...C -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 test_expect_success 'bar does not come up empty' '
 	git rev-list --left-right B...C -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -96,14 +96,14 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev multiple --refs combine inclusive' '
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
 	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
 		<actual >actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -115,7 +115,7 @@ test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
 	git name-rev --stdin --name-only --refs="*tags/F" \
 		<actual >actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -127,14 +127,14 @@ test_expect_success 'name-rev --exclude excludes matched patterns' '
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
 		<actual >actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev --no-refs clears the refs list' '
 	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
 	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
 		<expect >actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 cat >expect <<EOF
@@ -148,7 +148,7 @@ test_expect_success '--cherry-mark' '
 	git rev-list --cherry-mark F...E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -162,7 +162,7 @@ test_expect_success '--cherry-mark --left-right' '
 	git rev-list --cherry-mark --left-right F...E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -173,14 +173,14 @@ test_expect_success '--cherry-pick --right-only' '
 	git rev-list --cherry-pick --right-only F...E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 test_expect_success '--cherry-pick --left-only' '
 	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -192,7 +192,7 @@ test_expect_success '--cherry' '
 	git rev-list --cherry F...E -- bar > actual &&
 	git name-rev --stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
-	test_cmp actual.named expect
+	test_cmp expect actual.named
 '
 
 cat >expect <<EOF
@@ -201,7 +201,7 @@ EOF
 
 test_expect_success '--cherry --count' '
 	git rev-list --cherry --count F...E -- bar > actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 cat >expect <<EOF
@@ -210,7 +210,7 @@ EOF
 
 test_expect_success '--cherry-mark --count' '
 	git rev-list --cherry-mark --count F...E -- bar > actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 cat >expect <<EOF
@@ -219,7 +219,7 @@ EOF
 
 test_expect_success '--cherry-mark --left-right --count' '
 	git rev-list --cherry-mark --left-right --count F...E -- bar > actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 test_expect_success '--cherry-pick with independent, but identical branches' '
diff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh
index 59fc2f06e0..89458d370f 100755
--- a/t/t6013-rev-list-reverse-parents.sh
+++ b/t/t6013-rev-list-reverse-parents.sh
@@ -28,7 +28,7 @@ test_expect_success '--reverse --parents --full-history combines correctly' '
 		perl -e "print reverse <>" > expected &&
 	git rev-list --reverse --parents --full-history master -- foo \
 		> actual &&
-	test_cmp actual expected
+	test_cmp expected actual
 	'
 
 test_expect_success '--boundary does too' '
@@ -36,7 +36,7 @@ test_expect_success '--boundary does too' '
 		perl -e "print reverse <>" > expected &&
 	git rev-list --boundary --reverse --parents --full-history \
 		master ^root -- foo > actual &&
-	test_cmp actual expected
+	test_cmp expected actual
 	'
 
 test_done
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index cbc5fb37fe..f5929c46f3 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -488,7 +488,7 @@ test_expect_success 'moving a submodule in nested directories' '
 		git config -f ../.gitmodules submodule.deep/directory/hierarchy/sub.path >../actual &&
 		echo "directory/hierarchy/sub" >../expect
 	) &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 test_expect_failure 'moving nested submodules' '
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 1b530b5022..29e5043b94 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -38,7 +38,7 @@ test_expect_success setup '
 	test_commit "$msg" &&
 	echo "$msg" >expect &&
 	git show -s --format=%s > actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 
 '
 
@@ -85,7 +85,7 @@ do
 		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp actual expect
+		test_cmp expect actual
 	'
 done
 
@@ -107,7 +107,7 @@ do
 		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp actual expect
+		test_cmp expect actual
 	'
 done
 
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 86f23be34a..95653a08ca 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -428,9 +428,9 @@ test_expect_success 'test --mixed <paths>' '
 	git reset HEAD -- file1 file2 file3 &&
 	test_must_fail git diff --quiet &&
 	git diff > output &&
-	test_cmp output expect &&
+	test_cmp expect output &&
 	git diff --cached > output &&
-	test_cmp output cached_expect
+	test_cmp cached_expect output
 '
 
 test_expect_success 'test resetting the index at give paths' '
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index d4b217b0ee..76c223c967 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -187,7 +187,7 @@ test_expect_success 'format of merge conflict from checkout -m' '
 	d
 	>>>>>>> local
 	EOF
-	test_cmp two expect
+	test_cmp expect two
 '
 
 test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
@@ -213,7 +213,7 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
 	d
 	>>>>>>> local
 	EOF
-	test_cmp two expect
+	test_cmp expect two
 '
 
 test_expect_success 'switch to another branch while carrying a deletion' '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 6f8337ffb5..a39e69a3eb 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1211,7 +1211,7 @@ test_expect_success 'clone --recurse-submodules with a pathspec works' '
 
 	git clone --recurse-submodules="sub0" multisuper multisuper_clone &&
 	git -C multisuper_clone submodule status |cut -c1,43- >actual &&
-	test_cmp actual expected
+	test_cmp expected actual
 '
 
 test_expect_success 'clone with multiple --recurse-submodules options' '
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 0d5b42a25b..7bfb2f498d 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -119,7 +119,7 @@ test_expect_success 'merge with one side as a fast-forward of the other' '
 	 git ls-tree test-forward sub | cut -f1 | cut -f3 -d" " > actual &&
 	 (cd sub &&
 	  git rev-parse sub-d > ../expect) &&
-	 test_cmp actual expect)
+	 test_cmp expect actual)
 '
 
 test_expect_success 'merging should conflict for non fast-forward' '
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 055c90736e..9edf6572ed 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -306,7 +306,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '
 		cd super &&
 		git diff >../diff_actual 2>&1
 	) &&
-	test_cmp diff_actual diff_expect
+	test_cmp diff_expect diff_actual
 '
 
 test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
@@ -314,7 +314,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 		cd super &&
 		git diff --submodule >../diff_submodule_actual 2>&1
 	) &&
-	test_cmp diff_submodule_actual diff_submodule_expect
+	test_cmp diff_submodule_expect diff_submodule_actual
 '
 
 # We'll setup different cases for further testing:
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 80194b79f9..dfde6a675a 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -697,7 +697,7 @@ test_expect_success 'merge --no-ff --edit' '
 	git cat-file commit HEAD >raw &&
 	grep "work done on the side branch" raw &&
 	sed "1,/^$/d" >actual raw &&
-	test_cmp actual expected
+	test_cmp expected actual
 '
 
 test_expect_success GPG 'merge --ff-only tag' '
@@ -709,7 +709,7 @@ test_expect_success GPG 'merge --ff-only tag' '
 	git merge --ff-only signed &&
 	git rev-parse signed^0 >expect &&
 	git rev-parse HEAD >actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 test_expect_success GPG 'merge --no-edit tag should skip editor' '
@@ -721,7 +721,7 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	EDITOR=false git merge --no-edit signed &&
 	git rev-parse signed^0 >expect &&
 	git rev-parse HEAD^2 >actual &&
-	test_cmp actual expect
+	test_cmp expect actual
 '
 
 test_expect_success 'set up mod-256 conflict scenario' '
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 381b7df452..1a430b9c40 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -621,7 +621,7 @@ test_expect_success 'file with no base' '
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool mybase -- both &&
 	>expected &&
-	test_cmp both expected
+	test_cmp expected both
 '
 
 test_expect_success 'custom commands override built-ins' '
@@ -632,7 +632,7 @@ test_expect_success 'custom commands override built-ins' '
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool defaults -- both &&
 	echo master both added >expected &&
-	test_cmp both expected
+	test_cmp expected both
 '
 
 test_expect_success 'filenames seen by tools start with ./' '
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index f30980895c..4d261c2a9c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1266,7 +1266,7 @@ test_expect_success $PREREQ 'asks about and fixes 8bit encodings' '
 	grep email-using-8bit stdout &&
 	grep "Which 8bit encoding" stdout &&
 	egrep "Content|MIME" msgtxt1 >actual &&
-	test_cmp actual content-type-decl
+	test_cmp content-type-decl actual
 '
 
 test_expect_success $PREREQ 'sendemail.8bitEncoding works' '
@@ -1277,7 +1277,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' '
 			--smtp-server="$(pwd)/fake.sendmail" \
 			email-using-8bit >stdout &&
 	egrep "Content|MIME" msgtxt1 >actual &&
-	test_cmp actual content-type-decl
+	test_cmp content-type-decl actual
 '
 
 test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '
@@ -1289,7 +1289,7 @@ test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
 	egrep "Content|MIME" msgtxt1 >actual &&
-	test_cmp actual content-type-decl
+	test_cmp content-type-decl actual
 '
 
 test_expect_success $PREREQ 'setup expect' '
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Jonathan Nieder]

Stefan Beller wrote:

> Fix the argument order for test_cmp. When given the expected
> result first the diff shows the actual output with '+' and the
> expectation with '-', which is the convention for our tests.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Yes, this should make the output from failing tests easier to take in
at a glance.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

How did you find these?  E.g. is there a grep pattern that reviewers
can use to repeat your results?

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Stefan Beller]

On Fri, Oct 6, 2017 at 3:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Stefan Beller wrote:
>
>> Fix the argument order for test_cmp. When given the expected
>> result first the diff shows the actual output with '+' and the
>> expectation with '-', which is the convention for our tests.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> Yes, this should make the output from failing tests easier to take in
> at a glance.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> How did you find these?  E.g. is there a grep pattern that reviewers
> can use to repeat your results?

The grunge work was done via scrolling through

git -C t grep test_cmp

However it occurred to me that checking for the completeness could be done
via

  git -C t grep test_cmp | \
    awk '{$1=""; print }  | \ # remove file name from output
    sort | uniq

There are some cases that are still pretty obvious what the
"actual" and the "expect" is, but they are not included in this patch
as the code (and file names) looked hairy.

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> How did you find these?  E.g. is there a grep pattern that reviewers
>> can use to repeat your results?
>
> The grunge work was done via scrolling through
>
> git -C t grep test_cmp
>
> However it occurred to me that checking for the completeness could be done
> via
>
>   git -C t grep test_cmp | \
>     awk '{$1=""; print }  | \ # remove file name from output
>     sort | uniq

Just like grep's GNUism, you can use -h to lose awk, i.e.

	git grep -h -e test_cmp t/ | sort -u

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index ec5f530102..42f584f8b3 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
>  test_expect_success ':only and :unfold work together' '
>  	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
>  	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
> -	test_cmp actual reverse &&
> +	test_cmp reverse actual &&
>  	{
>  		grep -v patch.description <trailers | unfold &&
>  		echo

This test is trying to see that giving the two modifers in any order
produces identical results, so the result of swaping is no more or
no less correct than the original, because what this test calls
"actual" is no less authoritative result than what is in "reverse"
at this point, which is the reason why we prefer "expect actual" in
the typical use of test_cmp.

Renaming <actual, reverse> to (rather meaningless) <one, two> or
<only-unfold, unfold-only> might make the intention a bit clearer.

THanks.

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jeff King]

On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:

> The `test_must_fail` should only be used to indicate a git command is
> failing. `test_cmp` is not a git command, such that it doesn't need the
> special treatment of `test_must_fail` (which e.g. includes checking for
> segfault)

Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
And your patch is obviously an improvement, but I have to wonder if some
of these make any sense.

If we're expecting some outcome, then it's reasonable to say:

  1. The output should look exactly like this. (test_cmp)

  2. The output should look something like this. (grep)

  3. The output should _not_ mention this (! grep)

But "the output should not look exactly like this" doesn't seem very
robust. It's likely to give a false success due to small changes (or
translations), or even bugs in the script.

For instance:

> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..c31141c471 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
>  test_expect_success 'cherry-pick conflict without rerere' '
>  	test_config rerere.enabled false &&
>  	test_must_fail git cherry-pick master &&
> -	test_must_fail test_cmp expect foo
> +	! test_cmp expect foo
>  '

Running ./t3504 with "-v" (with or without your patch) shows:

  --- expect	2017-10-06 19:14:43.677840120 +0000
  +++ foo	2017-10-06 19:14:43.705840120 +0000
  @@ -1 +1 @@
  -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue
  +foo-dev

Which just seems like a bug.  Did the original author mean foo-expect?
It's hard to tell, as we are just reusing expectations from previous
tests.

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 02106c9226..7178b917ce 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote specified' '
>  
>  test_expect_success 'suppress "From <url>" with -q' '
>  	git ls-remote -q 2>actual_err &&
> -	test_must_fail test_cmp exp_err actual_err
> +	! test_cmp exp_err actual_err
>  '

This one seems like "test_18ngrep ! ^From" would be more appropriate. Or
even "test_must_be_empty".

> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
> index fac5a73851..5f9ad51929 100755
> --- a/t/t5612-clone-refspec.sh
> +++ b/t/t5612-clone-refspec.sh
> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '
>  		git for-each-ref refs/tags >../actual
>  	) &&
>  	git for-each-ref refs/tags >expect &&
> -	test_must_fail test_cmp expect actual &&
> +	! test_cmp expect actual &&
>  	test_line_count = 2 actual

Here we check that no updates happened due to a fetch because we see
that the tags in the fetched repo do not match the tags in the parent
repo. That actually seems pretty legitimate. But I think:

  git for-each-ref refs/tags >before
  git fetch
  git for-each-ref refs/tags >after
  test_cmp before after

would be more straightforward.

> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index 93f162a4f7..1644866571 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
>  test_expect_success '"status.branch=true" different from "--no-branch"' '
>  	git status -s --no-branch  >expected_nobranch &&
>  	git -c status.branch=true status -s >actual &&
> -	test_must_fail test_cmp expected_nobranch actual
> +	! test_cmp expected_nobranch actual
>  '

Shouldn't this be comparing it positively to the output with "--branch"?

>  test_expect_success '"status.branch=true" weaker than "--no-branch"' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index d8464d4218..5cd6b40432 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
>  		echo 1 >> file &&
>  		svn_cmd commit -m "changing file" &&
>  		svn_cmd up &&
> -		test_must_fail test_cmp auto_updated_file au_file_saved
> +		! test_cmp auto_updated_file au_file_saved
>  	)
>  '

This one looked complicated, so I leave it as an exercise for the
reader. :)

-Peff

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Stefan Beller]

On Fri, Oct 6, 2017 at 12:22 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:
>
>> The `test_must_fail` should only be used to indicate a git command is
>> failing. `test_cmp` is not a git command, such that it doesn't need the
>> special treatment of `test_must_fail` (which e.g. includes checking for
>> segfault)
>
> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.

Thanks for actually thinking about the problem. I just searched and replaced
the combination of test_must_fail and test_cmp mechanically after I noticed
one such occurrence whilst preparing the next patch (order of test_cmp args).

>
> If we're expecting some outcome, then it's reasonable to say:
>
>   1. The output should look exactly like this. (test_cmp)
>
>   2. The output should look something like this. (grep)
>
>   3. The output should _not_ mention this (! grep)
>
> But "the output should not look exactly like this" doesn't seem very
> robust. It's likely to give a false success due to small changes (or
> translations), or even bugs in the script.

I agree that the case "should not look like exactly this" is most likely
indicating a weak test.


> Running ./t3504 with "-v" (with or without your patch) shows:
>
>   --- expect    2017-10-06 19:14:43.677840120 +0000
>   +++ foo       2017-10-06 19:14:43.705840120 +0000
>   @@ -1 +1 @@
>   -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue
>   +foo-dev
>
> Which just seems like a bug.  Did the original author mean foo-expect?

This was originally written by a non regular in 2008. I don't think
they remember even if we'd ask.

I think we'd want to not resolve it to foo-dev here,
(so ideally we'd test for

<<<<
foo
====
foo-dev
>>>>

but just testing that we do not blindly resolve to foo seems ok-ish)

Thanks for spotting this!

> It's hard to tell, as we are just reusing expectations from previous
> tests.
>
>> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
>> index 02106c9226..7178b917ce 100755
>> --- a/t/t5512-ls-remote.sh
>> +++ b/t/t5512-ls-remote.sh
>> @@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote specified' '
>>
>>  test_expect_success 'suppress "From <url>" with -q' '
>>       git ls-remote -q 2>actual_err &&
>> -     test_must_fail test_cmp exp_err actual_err
>> +     ! test_cmp exp_err actual_err
>>  '
>
> This one seems like "test_18ngrep ! ^From" would be more appropriate. Or
> even "test_must_be_empty".

Going by the test title, I agree.

>> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
>> index fac5a73851..5f9ad51929 100755
>> --- a/t/t5612-clone-refspec.sh
>> +++ b/t/t5612-clone-refspec.sh
>> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '
>>               git for-each-ref refs/tags >../actual
>>       ) &&
>>       git for-each-ref refs/tags >expect &&
>> -     test_must_fail test_cmp expect actual &&
>> +     ! test_cmp expect actual &&
>>       test_line_count = 2 actual
>
> Here we check that no updates happened due to a fetch because we see
> that the tags in the fetched repo do not match the tags in the parent
> repo. That actually seems pretty legitimate. But I think:
>
>   git for-each-ref refs/tags >before
>   git fetch
>   git for-each-ref refs/tags >after
>   test_cmp before after
>
> would be more straightforward.
>
>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
>> index 93f162a4f7..1644866571 100755
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
>>  test_expect_success '"status.branch=true" different from "--no-branch"' '
>>       git status -s --no-branch  >expected_nobranch &&
>>       git -c status.branch=true status -s >actual &&
>> -     test_must_fail test_cmp expected_nobranch actual
>> +     ! test_cmp expected_nobranch actual
>>  '
>
> Shouldn't this be comparing it positively to the output with "--branch"?
>
>>  test_expect_success '"status.branch=true" weaker than "--no-branch"' '
>> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
>> index d8464d4218..5cd6b40432 100755
>> --- a/t/t9164-git-svn-dcommit-concurrent.sh
>> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
>> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
>>               echo 1 >> file &&
>>               svn_cmd commit -m "changing file" &&
>>               svn_cmd up &&
>> -             test_must_fail test_cmp auto_updated_file au_file_saved
>> +             ! test_cmp auto_updated_file au_file_saved
>>       )
>>  '
>
> This one looked complicated, so I leave it as an exercise for the
> reader. :)

eh, I was hoping to not stirr up a controversy, but treating this as a
drive-by patch "making the tests a better place, one tiny step at a time".

Thanks,
Stefan

>
> -Peff

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:

>> The `test_must_fail` should only be used to indicate a git command is
>> failing. `test_cmp` is not a git command, such that it doesn't need the
>> special treatment of `test_must_fail` (which e.g. includes checking for
>> segfault)
>
> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.

Just for the record: I agree with all the above, and my Reviewed-by
still stands.

Thanks for looking closer.  I wonder if there's a systematic way to
avoid this kind of weak test that can bitrot and still pass too easily
--- e.g. should we have a test_must_differ command with an explanation
of why it should usually be avoided?  Would a lint check that bans
this kind of construct completely be going too far?

Jonathan

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.
>
> If we're expecting some outcome, then it's reasonable to say:
>
>   1. The output should look exactly like this. (test_cmp)
>
>   2. The output should look something like this. (grep)
>
>   3. The output should _not_ mention this (! grep)
>
> But "the output should not look exactly like this" doesn't seem very
> robust. It's likely to give a false success due to small changes (or
> translations), or even bugs in the script.

Yeah, thanks for stepping back and looking at it from higher level.

$ git grep -e 'test_must_fail test_cmp' -e '! test_cmp' t/

gives 21 hits, in addition to the 5 Stefan identified, and it would
be a promising hunt to go through each one of them to see if they
make sense.

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jonathan Nieder]

Stefan Beller wrote:

> The `test_must_fail` should only be used to indicate a git command is
> failing. `test_cmp` is not a git command, such that it doesn't need the
> special treatment of `test_must_fail` (which e.g. includes checking for
> segfault).
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t3504-cherry-pick-rerere.sh         | 2 +-
>  t/t5512-ls-remote.sh                  | 2 +-
>  t/t5612-clone-refspec.sh              | 2 +-
>  t/t7508-status.sh                     | 2 +-
>  t/t9164-git-svn-dcommit-concurrent.sh | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)

Thanks.  I agree that this is more readable, and it matches the advice
in t/README:

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I wonder if it would be useful to have a nod to that advice in the
docstring in t/test-lib-functions.sh, too.