{"thread":{"id":"46930","subject":"[PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","startedAt":"2017-10-06T19:00:18Z","lastAt":"2017-10-07T02:29:26Z","messageCount":11,"participants":["Stefan Beller","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"329915","messageId":"20171006190006.19623-1-sbeller@google.com","threadId":"46930","inReplyTo":null,"subject":"[PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-06T19:00:05Z","receivedAt":"2017-10-06T19:00:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `test_must_fail` should only be used to indicate a git command is\nfailing. `test_cmp` is not a git command, such that it doesn't need the\nspecial treatment of `test_must_fail` (which e.g. includes checking for\nsegfault)\n\nSigned-off-by: Stefan Beller \n---\n t/t3504-cherry-pick-rerere.sh | 2 +-\n t/t5512-ls-remote.sh | 2 +-\n t/t5612-clone-refspec.sh | 2 +-\n t/t7508-status.sh | 2 +-\n t/t9164-git-svn-dcommit-concurrent.sh | 2 +-\n 5 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\nindex a267b2d144..c31141c471 100755\n--- a/t/t3504-cherry-pick-rerere.sh\n+++ b/t/t3504-cherry-pick-rerere.sh\n@@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n test_expect_success 'cherry-pick conflict without rerere' '\n \ttest_config rerere.enabled false &&\n \ttest_must_fail git cherry-pick master &&\n-\ttest_must_fail test_cmp expect foo\n+\t! test_cmp expect foo\n '\n \n test_done\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 02106c9226..7178b917ce 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -56,7 +56,7 @@ test_expect_success 'use \"origin\" when no remote specified' '\n \n test_expect_success 'suppress \"From \" with -q' '\n \tgit ls-remote -q 2>actual_err &&\n-\ttest_must_fail test_cmp exp_err actual_err\n+\t! test_cmp exp_err actual_err\n '\n \n test_expect_success 'use branch..remote if possible' '\ndiff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh\nindex fac5a73851..5f9ad51929 100755\n--- a/t/t5612-clone-refspec.sh\n+++ b/t/t5612-clone-refspec.sh\n@@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '\n \t\tgit for-each-ref refs/tags >../actual\n \t) &&\n \tgit for-each-ref refs/tags >expect &&\n-\ttest_must_fail test_cmp expect actual &&\n+\t! test_cmp expect actual &&\n \ttest_line_count = 2 actual\n '\n \ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 93f162a4f7..1644866571 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1532,7 +1532,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n \tgit status -s --no-branch >expected_nobranch &&\n \tgit -c status.branch=true status -s >actual &&\n-\ttest_must_fail test_cmp expected_nobranch actual\n+\t! test_cmp expected_nobranch actual\n '\n \n test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\ndiff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\nindex d8464d4218..5cd6b40432 100755\n--- a/t/t9164-git-svn-dcommit-concurrent.sh\n+++ b/t/t9164-git-svn-dcommit-concurrent.sh\n@@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n \t\techo 1 >> file &&\n \t\tsvn_cmd commit -m \"changing file\" &&\n \t\tsvn_cmd up &&\n-\t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n+\t\t! test_cmp auto_updated_file au_file_saved\n \t)\n '\n \n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329916","messageId":"20171006190006.19623-2-sbeller@google.com","threadId":"46930","inReplyTo":"20171006190006.19623-1-sbeller@google.com","subject":"[PATCH 2/2] tests: fix diff order arguments in test_cmp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-06T19:00:06Z","receivedAt":"2017-10-06T19:00:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Fix the argument order for test_cmp. When given the expected\nresult first the diff shows the actual output with '+' and the\nexpectation with '-', which is the convention for our tests.\n\nSigned-off-by: Stefan Beller \n---\n t/t1004-read-tree-m-u-wf.sh | 2 +-\n t/t4015-diff-whitespace.sh | 4 ++--\n t/t4205-log-pretty-formats.sh | 2 +-\n t/t6007-rev-list-cherry-pick-file.sh | 32 ++++++++++++++++----------------\n t/t6013-rev-list-reverse-parents.sh | 4 ++--\n t/t7001-mv.sh | 2 +-\n t/t7005-editor.sh | 6 +++---\n t/t7102-reset.sh | 4 ++--\n t/t7201-co.sh | 4 ++--\n t/t7400-submodule-basic.sh | 2 +-\n t/t7405-submodule-merge.sh | 2 +-\n t/t7506-status-submodule.sh | 4 ++--\n t/t7600-merge.sh | 6 +++---\n t/t7610-mergetool.sh | 4 ++--\n t/t9001-send-email.sh | 6 +++---\n 15 files changed, 42 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh\nindex c70cf42300..c7ce5d8bb5 100755\n--- a/t/t1004-read-tree-m-u-wf.sh\n+++ b/t/t1004-read-tree-m-u-wf.sh\n@@ -218,7 +218,7 @@ test_expect_success 'D/F' '\n \t\techo \"100644 $a 2\tsubdir/file2\"\n \t\techo \"100644 $b 3\tsubdir/file2/another\"\n \t) >expect &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n \n '\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 12d182dc1b..1709e4578b 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -155,7 +155,7 @@ test_expect_success 'ignore-blank-lines: only new lines' '\n \" >x &&\n \tgit diff --ignore-blank-lines >out &&\n \t>expect &&\n-\ttest_cmp out expect\n+\ttest_cmp expect out\n '\n \n test_expect_success 'ignore-blank-lines: only new lines with space' '\n@@ -165,7 +165,7 @@ test_expect_success 'ignore-blank-lines: only new lines with space' '\n \" >x &&\n \tgit diff -w --ignore-blank-lines >out &&\n \t>expect &&\n-\ttest_cmp out expect\n+\ttest_cmp expect out\n '\n \n test_expect_success 'ignore-blank-lines: after change' '\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex ec5f530102..42f584f8b3 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '\n test_expect_success ':only and :unfold work together' '\n \tgit log --no-walk --pretty=\"%(trailers:only:unfold)\" >actual &&\n \tgit log --no-walk --pretty=\"%(trailers:unfold:only)\" >reverse &&\n-\ttest_cmp actual reverse &&\n+\ttest_cmp reverse actual &&\n \t{\n \t\tgrep -v patch.description actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n test_expect_success '--count' '\n@@ -77,14 +77,14 @@ test_expect_success '--cherry-pick bar does not come up empty' '\n \tgit rev-list --left-right --cherry-pick B...C -- bar > actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n test_expect_success 'bar does not come up empty' '\n \tgit rev-list --left-right B...C -- bar > actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect < actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n test_expect_success 'name-rev multiple --refs combine inclusive' '\n \tgit rev-list --left-right --cherry-pick F...E -- bar >actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/F\" --refs=\"*tags/E\" \\\n \t\tactual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect <actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/F\" \\\n \t\tactual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect <actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" --exclude=\"*E\" \\\n \t\tactual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n test_expect_success 'name-rev --no-refs clears the refs list' '\n \tgit rev-list --left-right --cherry-pick F...E -- bar >expect &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/F\" --refs=\"*tags/E\" --no-refs --refs=\"*tags/G\" \\\n \t\tactual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n cat >expect < actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect < actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect < actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n test_expect_success '--cherry-pick --left-only' '\n \tgit rev-list --cherry-pick --left-only E...F -- bar > actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect < actual &&\n \tgit name-rev --stdin --name-only --refs=\"*tags/*\" \\\n \t\t< actual > actual.named &&\n-\ttest_cmp actual.named expect\n+\ttest_cmp expect actual.named\n '\n \n cat >expect < actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n cat >expect < actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n cat >expect < actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--cherry-pick with independent, but identical branches' '\ndiff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh\nindex 59fc2f06e0..89458d370f 100755\n--- a/t/t6013-rev-list-reverse-parents.sh\n+++ b/t/t6013-rev-list-reverse-parents.sh\n@@ -28,7 +28,7 @@ test_expect_success '--reverse --parents --full-history combines correctly' '\n \t\tperl -e \"print reverse <>\" > expected &&\n \tgit rev-list --reverse --parents --full-history master -- foo \\\n \t\t> actual &&\n-\ttest_cmp actual expected\n+\ttest_cmp expected actual\n \t'\n \n test_expect_success '--boundary does too' '\n@@ -36,7 +36,7 @@ test_expect_success '--boundary does too' '\n \t\tperl -e \"print reverse <>\" > expected &&\n \tgit rev-list --boundary --reverse --parents --full-history \\\n \t\tmaster ^root -- foo > actual &&\n-\ttest_cmp actual expected\n+\ttest_cmp expected actual\n \t'\n \n test_done\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex cbc5fb37fe..f5929c46f3 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -488,7 +488,7 @@ test_expect_success 'moving a submodule in nested directories' '\n \t\tgit config -f ../.gitmodules submodule.deep/directory/hierarchy/sub.path >../actual &&\n \t\techo \"directory/hierarchy/sub\" >../expect\n \t) &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n test_expect_failure 'moving nested submodules' '\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 1b530b5022..29e5043b94 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -38,7 +38,7 @@ test_expect_success setup '\n \ttest_commit \"$msg\" &&\n \techo \"$msg\" >expect &&\n \tgit show -s --format=%s > actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n \n '\n \n@@ -85,7 +85,7 @@ do\n \t\tgit --exec-path=. commit --amend &&\n \t\tgit show -s --pretty=oneline |\n \t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp actual expect\n+\t\ttest_cmp expect actual\n \t'\n done\n \n@@ -107,7 +107,7 @@ do\n \t\tgit --exec-path=. commit --amend &&\n \t\tgit show -s --pretty=oneline |\n \t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp actual expect\n+\t\ttest_cmp expect actual\n \t'\n done\n \ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 86f23be34a..95653a08ca 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -428,9 +428,9 @@ test_expect_success 'test --mixed ' '\n \tgit reset HEAD -- file1 file2 file3 &&\n \ttest_must_fail git diff --quiet &&\n \tgit diff > output &&\n-\ttest_cmp output expect &&\n+\ttest_cmp expect output &&\n \tgit diff --cached > output &&\n-\ttest_cmp output cached_expect\n+\ttest_cmp cached_expect output\n '\n \n test_expect_success 'test resetting the index at give paths' '\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex d4b217b0ee..76c223c967 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -187,7 +187,7 @@ test_expect_success 'format of merge conflict from checkout -m' '\n \td\n \t>>>>>>> local\n \tEOF\n-\ttest_cmp two expect\n+\ttest_cmp expect two\n '\n \n test_expect_success 'checkout --merge --conflict=diff3 ' '\n@@ -213,7 +213,7 @@ test_expect_success 'checkout --merge --conflict=diff3 ' '\n \td\n \t>>>>>>> local\n \tEOF\n-\ttest_cmp two expect\n+\ttest_cmp expect two\n '\n \n test_expect_success 'switch to another branch while carrying a deletion' '\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 6f8337ffb5..a39e69a3eb 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1211,7 +1211,7 @@ test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \n \tgit clone --recurse-submodules=\"sub0\" multisuper multisuper_clone &&\n \tgit -C multisuper_clone submodule status |cut -c1,43- >actual &&\n-\ttest_cmp actual expected\n+\ttest_cmp expected actual\n '\n \n test_expect_success 'clone with multiple --recurse-submodules options' '\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 0d5b42a25b..7bfb2f498d 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -119,7 +119,7 @@ test_expect_success 'merge with one side as a fast-forward of the other' '\n \t git ls-tree test-forward sub | cut -f1 | cut -f3 -d\" \" > actual &&\n \t (cd sub &&\n \t git rev-parse sub-d > ../expect) &&\n-\t test_cmp actual expect)\n+\t test_cmp expect actual)\n '\n \n test_expect_success 'merging should conflict for non fast-forward' '\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 055c90736e..9edf6572ed 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -306,7 +306,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '\n \t\tcd super &&\n \t\tgit diff >../diff_actual 2>&1\n \t) &&\n-\ttest_cmp diff_actual diff_expect\n+\ttest_cmp diff_expect diff_actual\n '\n \n test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n@@ -314,7 +314,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \t\tcd super &&\n \t\tgit diff --submodule >../diff_submodule_actual 2>&1\n \t) &&\n-\ttest_cmp diff_submodule_actual diff_submodule_expect\n+\ttest_cmp diff_submodule_expect diff_submodule_actual\n '\n \n # We'll setup different cases for further testing:\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 80194b79f9..dfde6a675a 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -697,7 +697,7 @@ test_expect_success 'merge --no-ff --edit' '\n \tgit cat-file commit HEAD >raw &&\n \tgrep \"work done on the side branch\" raw &&\n \tsed \"1,/^$/d\" >actual raw &&\n-\ttest_cmp actual expected\n+\ttest_cmp expected actual\n '\n \n test_expect_success GPG 'merge --ff-only tag' '\n@@ -709,7 +709,7 @@ test_expect_success GPG 'merge --ff-only tag' '\n \tgit merge --ff-only signed &&\n \tgit rev-parse signed^0 >expect &&\n \tgit rev-parse HEAD >actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n test_expect_success GPG 'merge --no-edit tag should skip editor' '\n@@ -721,7 +721,7 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \tEDITOR=false git merge --no-edit signed &&\n \tgit rev-parse signed^0 >expect &&\n \tgit rev-parse HEAD^2 >actual &&\n-\ttest_cmp actual expect\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'set up mod-256 conflict scenario' '\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 381b7df452..1a430b9c40 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -621,7 +621,7 @@ test_expect_success 'file with no base' '\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool mybase -- both &&\n \t>expected &&\n-\ttest_cmp both expected\n+\ttest_cmp expected both\n '\n \n test_expect_success 'custom commands override built-ins' '\n@@ -632,7 +632,7 @@ test_expect_success 'custom commands override built-ins' '\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool defaults -- both &&\n \techo master both added >expected &&\n-\ttest_cmp both expected\n+\ttest_cmp expected both\n '\n \n test_expect_success 'filenames seen by tools start with ./' '\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex f30980895c..4d261c2a9c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1266,7 +1266,7 @@ test_expect_success $PREREQ 'asks about and fixes 8bit encodings' '\n \tgrep email-using-8bit stdout &&\n \tgrep \"Which 8bit encoding\" stdout &&\n \tegrep \"Content|MIME\" msgtxt1 >actual &&\n-\ttest_cmp actual content-type-decl\n+\ttest_cmp content-type-decl actual\n '\n \n test_expect_success $PREREQ 'sendemail.8bitEncoding works' '\n@@ -1277,7 +1277,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' '\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\temail-using-8bit >stdout &&\n \tegrep \"Content|MIME\" msgtxt1 >actual &&\n-\ttest_cmp actual content-type-decl\n+\ttest_cmp content-type-decl actual\n '\n \n test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '\n@@ -1289,7 +1289,7 @@ test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '\n \t\t\t--8bit-encoding=UTF-8 \\\n \t\t\temail-using-8bit >stdout &&\n \tegrep \"Content|MIME\" msgtxt1 >actual &&\n-\ttest_cmp actual content-type-decl\n+\ttest_cmp content-type-decl actual\n '\n \n test_expect_success $PREREQ 'setup expect' '\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329917","messageId":"20171006192252.3a27enzim5yurhe5@sigill.intra.peff.net","threadId":"46930","inReplyTo":"20171006190006.19623-1-sbeller@google.com","subject":"Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T19:22:52Z","receivedAt":"2017-10-06T19:23:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:\n\n> The `test_must_fail` should only be used to indicate a git command is\n> failing. `test_cmp` is not a git command, such that it doesn't need the\n> special treatment of `test_must_fail` (which e.g. includes checking for\n> segfault)\n\nHmph. \"test_must_fail test_cmp\" is a weird thing for somebody to write.\nAnd your patch is obviously an improvement, but I have to wonder if some\nof these make any sense.\n\nIf we're expecting some outcome, then it's reasonable to say:\n\n 1. The output should look exactly like this. (test_cmp)\n\n 2. The output should look something like this. (grep)\n\n 3. The output should _not_ mention this (! grep)\n\nBut \"the output should not look exactly like this\" doesn't seem very\nrobust. It's likely to give a false success due to small changes (or\ntranslations), or even bugs in the script.\n\nFor instance:\n\n> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\n> index a267b2d144..c31141c471 100755\n> --- a/t/t3504-cherry-pick-rerere.sh\n> +++ b/t/t3504-cherry-pick-rerere.sh\n> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n> test_expect_success 'cherry-pick conflict without rerere' '\n> \ttest_config rerere.enabled false &&\n> \ttest_must_fail git cherry-pick master &&\n> -\ttest_must_fail test_cmp expect foo\n> +\t! test_cmp expect foo\n> '\n\nRunning ./t3504 with \"-v\" (with or without your patch) shows:\n\n --- expect\t2017-10-06 19:14:43.677840120 +0000\n +++ foo\t2017-10-06 19:14:43.705840120 +0000\n @@ -1 +1 @@\n -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue\n +foo-dev\n\nWhich just seems like a bug. Did the original author mean foo-expect?\nIt's hard to tell, as we are just reusing expectations from previous\ntests.\n\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 02106c9226..7178b917ce 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -56,7 +56,7 @@ test_expect_success 'use \"origin\" when no remote specified' '\n> \n> test_expect_success 'suppress \"From \" with -q' '\n> \tgit ls-remote -q 2>actual_err &&\n> -\ttest_must_fail test_cmp exp_err actual_err\n> +\t! test_cmp exp_err actual_err\n> '\n\nThis one seems like \"test_18ngrep ! ^From\" would be more appropriate. Or\neven \"test_must_be_empty\".\n\n> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh\n> index fac5a73851..5f9ad51929 100755\n> --- a/t/t5612-clone-refspec.sh\n> +++ b/t/t5612-clone-refspec.sh\n> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '\n> \t\tgit for-each-ref refs/tags >../actual\n> \t) &&\n> \tgit for-each-ref refs/tags >expect &&\n> -\ttest_must_fail test_cmp expect actual &&\n> +\t! test_cmp expect actual &&\n> \ttest_line_count = 2 actual\n\nHere we check that no updates happened due to a fetch because we see\nthat the tags in the fetched repo do not match the tags in the parent\nrepo. That actually seems pretty legitimate. But I think:\n\n git for-each-ref refs/tags >before\n git fetch\n git for-each-ref refs/tags >after\n test_cmp before after\n\nwould be more straightforward.\n\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index 93f162a4f7..1644866571 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -1532,7 +1532,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n> test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n> \tgit status -s --no-branch >expected_nobranch &&\n> \tgit -c status.branch=true status -s >actual &&\n> -\ttest_must_fail test_cmp expected_nobranch actual\n> +\t! test_cmp expected_nobranch actual\n> '\n\nShouldn't this be comparing it positively to the output with \"--branch\"?\n\n> test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\n> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n> index d8464d4218..5cd6b40432 100755\n> --- a/t/t9164-git-svn-dcommit-concurrent.sh\n> +++ b/t/t9164-git-svn-dcommit-concurrent.sh\n> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n> \t\techo 1 >> file &&\n> \t\tsvn_cmd commit -m \"changing file\" &&\n> \t\tsvn_cmd up &&\n> -\t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n> +\t\t! test_cmp auto_updated_file au_file_saved\n> \t)\n> '\n\nThis one looked complicated, so I leave it as an exercise for the\nreader. :)\n\n-Peff\n"},{"id":"329920","messageId":"CAGZ79kb4N2442HNWJuviATpOwiQ-qoGOgK+6Ys2JQMuS_kvjzg@mail.gmail.com","threadId":"46930","inReplyTo":"20171006192252.3a27enzim5yurhe5@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-06T19:41:22Z","receivedAt":"2017-10-06T19:41:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 6, 2017 at 12:22 PM, Jeff King wrote:\n> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:\n>\n>> The `test_must_fail` should only be used to indicate a git command is\n>> failing. `test_cmp` is not a git command, such that it doesn't need the\n>> special treatment of `test_must_fail` (which e.g. includes checking for\n>> segfault)\n>\n> Hmph. \"test_must_fail test_cmp\" is a weird thing for somebody to write.\n> And your patch is obviously an improvement, but I have to wonder if some\n> of these make any sense.\n\nThanks for actually thinking about the problem. I just searched and replaced\nthe combination of test_must_fail and test_cmp mechanically after I noticed\none such occurrence whilst preparing the next patch (order of test_cmp args).\n\n>\n> If we're expecting some outcome, then it's reasonable to say:\n>\n> 1. The output should look exactly like this. (test_cmp)\n>\n> 2. The output should look something like this. (grep)\n>\n> 3. The output should _not_ mention this (! grep)\n>\n> But \"the output should not look exactly like this\" doesn't seem very\n> robust. It's likely to give a false success due to small changes (or\n> translations), or even bugs in the script.\n\nI agree that the case \"should not look like exactly this\" is most likely\nindicating a weak test.\n\n\n> Running ./t3504 with \"-v\" (with or without your patch) shows:\n>\n> --- expect 2017-10-06 19:14:43.677840120 +0000\n> +++ foo 2017-10-06 19:14:43.705840120 +0000\n> @@ -1 +1 @@\n> -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue\n> +foo-dev\n>\n> Which just seems like a bug. Did the original author mean foo-expect?\n\nThis was originally written by a non regular in 2008. I don't think\nthey remember even if we'd ask.\n\nI think we'd want to not resolve it to foo-dev here,\n(so ideally we'd test for\n\n<<<<\nfoo\n====\nfoo-dev\n>>>>\n\nbut just testing that we do not blindly resolve to foo seems ok-ish)\n\nThanks for spotting this!\n\n> It's hard to tell, as we are just reusing expectations from previous\n> tests.\n>\n>> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n>> index 02106c9226..7178b917ce 100755\n>> --- a/t/t5512-ls-remote.sh\n>> +++ b/t/t5512-ls-remote.sh\n>> @@ -56,7 +56,7 @@ test_expect_success 'use \"origin\" when no remote specified' '\n>>\n>> test_expect_success 'suppress \"From \" with -q' '\n>> git ls-remote -q 2>actual_err &&\n>> - test_must_fail test_cmp exp_err actual_err\n>> + ! test_cmp exp_err actual_err\n>> '\n>\n> This one seems like \"test_18ngrep ! ^From\" would be more appropriate. Or\n> even \"test_must_be_empty\".\n\nGoing by the test title, I agree.\n\n>> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh\n>> index fac5a73851..5f9ad51929 100755\n>> --- a/t/t5612-clone-refspec.sh\n>> +++ b/t/t5612-clone-refspec.sh\n>> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '\n>> git for-each-ref refs/tags >../actual\n>> ) &&\n>> git for-each-ref refs/tags >expect &&\n>> - test_must_fail test_cmp expect actual &&\n>> + ! test_cmp expect actual &&\n>> test_line_count = 2 actual\n>\n> Here we check that no updates happened due to a fetch because we see\n> that the tags in the fetched repo do not match the tags in the parent\n> repo. That actually seems pretty legitimate. But I think:\n>\n> git for-each-ref refs/tags >before\n> git fetch\n> git for-each-ref refs/tags >after\n> test_cmp before after\n>\n> would be more straightforward.\n>\n>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n>> index 93f162a4f7..1644866571 100755\n>> --- a/t/t7508-status.sh\n>> +++ b/t/t7508-status.sh\n>> @@ -1532,7 +1532,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n>> test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n>> git status -s --no-branch >expected_nobranch &&\n>> git -c status.branch=true status -s >actual &&\n>> - test_must_fail test_cmp expected_nobranch actual\n>> + ! test_cmp expected_nobranch actual\n>> '\n>\n> Shouldn't this be comparing it positively to the output with \"--branch\"?\n>\n>> test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\n>> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n>> index d8464d4218..5cd6b40432 100755\n>> --- a/t/t9164-git-svn-dcommit-concurrent.sh\n>> +++ b/t/t9164-git-svn-dcommit-concurrent.sh\n>> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n>> echo 1 >> file &&\n>> svn_cmd commit -m \"changing file\" &&\n>> svn_cmd up &&\n>> - test_must_fail test_cmp auto_updated_file au_file_saved\n>> + ! test_cmp auto_updated_file au_file_saved\n>> )\n>> '\n>\n> This one looked complicated, so I leave it as an exercise for the\n> reader. :)\n\neh, I was hoping to not stirr up a controversy, but treating this as a\ndrive-by patch \"making the tests a better place, one tiny step at a time\".\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"329936","messageId":"20171006202108.GX19555@aiede.mtv.corp.google.com","threadId":"46930","inReplyTo":"20171006190006.19623-1-sbeller@google.com","subject":"Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-06T20:21:08Z","receivedAt":"2017-10-06T20:21:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> The `test_must_fail` should only be used to indicate a git command is\n> failing. `test_cmp` is not a git command, such that it doesn't need the\n> special treatment of `test_must_fail` (which e.g. includes checking for\n> segfault).\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/t3504-cherry-pick-rerere.sh | 2 +-\n> t/t5512-ls-remote.sh | 2 +-\n> t/t5612-clone-refspec.sh | 2 +-\n> t/t7508-status.sh | 2 +-\n> t/t9164-git-svn-dcommit-concurrent.sh | 2 +-\n> 5 files changed, 5 insertions(+), 5 deletions(-)\n\nThanks. I agree that this is more readable, and it matches the advice\nin t/README:\n\n On the other hand, don't use test_must_fail for running regular\n platform commands; just use '! cmd'. We are not in the business\n of verifying that the world given to us sanely works.\n\nReviewed-by: Jonathan Nieder \n\nI wonder if it would be useful to have a nod to that advice in the\ndocstring in t/test-lib-functions.sh, too.\n"},{"id":"329940","messageId":"20171006215926.GY19555@aiede.mtv.corp.google.com","threadId":"46930","inReplyTo":"20171006192252.3a27enzim5yurhe5@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-06T21:59:26Z","receivedAt":"2017-10-06T21:59:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:\n\n>> The `test_must_fail` should only be used to indicate a git command is\n>> failing. `test_cmp` is not a git command, such that it doesn't need the\n>> special treatment of `test_must_fail` (which e.g. includes checking for\n>> segfault)\n>\n> Hmph. \"test_must_fail test_cmp\" is a weird thing for somebody to write.\n> And your patch is obviously an improvement, but I have to wonder if some\n> of these make any sense.\n\nJust for the record: I agree with all the above, and my Reviewed-by\nstill stands.\n\nThanks for looking closer. I wonder if there's a systematic way to\navoid this kind of weak test that can bitrot and still pass too easily\n--- e.g. should we have a test_must_differ command with an explanation\nof why it should usually be avoided? Would a lint check that bans\nthis kind of construct completely be going too far?\n\nJonathan\n"},{"id":"329941","messageId":"20171006220101.GZ19555@aiede.mtv.corp.google.com","threadId":"46930","inReplyTo":"20171006190006.19623-2-sbeller@google.com","subject":"Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-06T22:01:01Z","receivedAt":"2017-10-06T22:01:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Fix the argument order for test_cmp. When given the expected\n> result first the diff shows the actual output with '+' and the\n> expectation with '-', which is the convention for our tests.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nYes, this should make the output from failing tests easier to take in\nat a glance.\n\nReviewed-by: Jonathan Nieder \n\nHow did you find these? E.g. is there a grep pattern that reviewers\ncan use to repeat your results?\n"},{"id":"329942","messageId":"CAGZ79kZhF=-baqPPqTCsbdyybOTV+SdvTqYiRbvsvB1epqpkZQ@mail.gmail.com","threadId":"46930","inReplyTo":"20171006220101.GZ19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-06T22:10:27Z","receivedAt":"2017-10-06T22:10:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 6, 2017 at 3:01 PM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>\n>> Fix the argument order for test_cmp. When given the expected\n>> result first the diff shows the actual output with '+' and the\n>> expectation with '-', which is the convention for our tests.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>\n> Yes, this should make the output from failing tests easier to take in\n> at a glance.\n>\n> Reviewed-by: Jonathan Nieder \n>\n> How did you find these? E.g. is there a grep pattern that reviewers\n> can use to repeat your results?\n\nThe grunge work was done via scrolling through\n\ngit -C t grep test_cmp\n\nHowever it occurred to me that checking for the completeness could be done\nvia\n\n git -C t grep test_cmp | \\\n awk '{$1=\"\"; print } | \\ # remove file name from output\n sort | uniq\n\nThere are some cases that are still pretty obvious what the\n\"actual\" and the \"expect\" is, but they are not included in this patch\nas the code (and file names) looked hairy.\n"},{"id":"329954","messageId":"xmqqefqfvjpb.fsf@gitster.mtv.corp.google.com","threadId":"46930","inReplyTo":"CAGZ79kZhF=-baqPPqTCsbdyybOTV+SdvTqYiRbvsvB1epqpkZQ@mail.gmail.com","subject":"Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T01:18:56Z","receivedAt":"2017-10-07T01:19:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> How did you find these? E.g. is there a grep pattern that reviewers\n>> can use to repeat your results?\n>\n> The grunge work was done via scrolling through\n>\n> git -C t grep test_cmp\n>\n> However it occurred to me that checking for the completeness could be done\n> via\n>\n> git -C t grep test_cmp | \\\n> awk '{$1=\"\"; print } | \\ # remove file name from output\n> sort | uniq\n\nJust like grep's GNUism, you can use -h to lose awk, i.e.\n\n\tgit grep -h -e test_cmp t/ | sort -u\n\n"},{"id":"329957","messageId":"xmqq1smfvh8c.fsf@gitster.mtv.corp.google.com","threadId":"46930","inReplyTo":"20171006192252.3a27enzim5yurhe5@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T02:12:19Z","receivedAt":"2017-10-07T02:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmph. \"test_must_fail test_cmp\" is a weird thing for somebody to write.\n> And your patch is obviously an improvement, but I have to wonder if some\n> of these make any sense.\n>\n> If we're expecting some outcome, then it's reasonable to say:\n>\n> 1. The output should look exactly like this. (test_cmp)\n>\n> 2. The output should look something like this. (grep)\n>\n> 3. The output should _not_ mention this (! grep)\n>\n> But \"the output should not look exactly like this\" doesn't seem very\n> robust. It's likely to give a false success due to small changes (or\n> translations), or even bugs in the script.\n\nYeah, thanks for stepping back and looking at it from higher level.\n\n$ git grep -e 'test_must_fail test_cmp' -e '! test_cmp' t/\n\ngives 21 hits, in addition to the 5 Stefan identified, and it would\nbe a promising hunt to go through each one of them to see if they\nmake sense.\n\n"},{"id":"329958","messageId":"xmqqwp47u1vk.fsf@gitster.mtv.corp.google.com","threadId":"46930","inReplyTo":"20171006190006.19623-2-sbeller@google.com","subject":"Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T02:29:19Z","receivedAt":"2017-10-07T02:29:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index ec5f530102..42f584f8b3 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '\n> test_expect_success ':only and :unfold work together' '\n> \tgit log --no-walk --pretty=\"%(trailers:only:unfold)\" >actual &&\n> \tgit log --no-walk --pretty=\"%(trailers:unfold:only)\" >reverse &&\n> -\ttest_cmp actual reverse &&\n> +\ttest_cmp reverse actual &&\n> \t{\n> \t\tgrep -v patch.description \t\techo\n\nThis test is trying to see that giving the two modifers in any order\nproduces identical results, so the result of swaping is no more or\nno less correct than the original, because what this test calls\n\"actual\" is no less authoritative result than what is in \"reverse\"\nat this point, which is the reason why we prefer \"expect actual\" in\nthe typical use of test_cmp.\n\nRenaming to (rather meaningless) or\n might make the intention a bit clearer.\n\nTHanks.\n"}]}