{"thread":{"id":"1473","subject":"[PATCH] Trapping exit in tests, using return for errors","startedAt":"2005-08-11T03:56:21Z","lastAt":"2005-08-11T16:00:40Z","messageCount":5,"participants":["Pavel Roskin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"7049","messageId":"1123732581.18644.37.camel@dv","threadId":"1473","inReplyTo":null,"subject":"[PATCH] Trapping exit in tests, using return for errors","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2005-08-11T03:56:21Z","receivedAt":"2005-08-11T03:56:21Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"Hello!\n\nI have noticed that \"make test\" fails without any explanations when the\n\"merge\" utility is missing.  I don't think tests should be silent in\ncase of failure.\n\nIt turned out that the particular test was using \"exit\" to interrupt the\ntest in case of an error.  This caused the whole test script to exit.\nNo further tests would be run even if \"--immediate\" wasn't specified.\nNo error message was printed.\n\nThis patch does following:\n\nAll instances of \"exit\", \"exit 1\" and \"(exit 1)\" in tests have been\nreplaced with \"return 1\".  In fact, \"(exit 1)\" had no effect.\n\nFile descriptor 5 is duplicated from file descriptor 1.  This is needed\nto print important error messages from tests.\n\nNew function test_run_() has been introduced.  Any \"return\" in the test\nwould merely cause that function to return without skipping calls to\ntest_failure_() and test_ok_().  The new function also traps \"exit\" and\ntreats it like a fatal error (in case somebody reintroduces \"exit\" in\nthe tests).\n\ntest_expect_failure() and test_expect_success() check both the result of\neval and the return value of test_run_().  If the later is not 0, it's\nalways a failure because it indicates the the test didn't complete.\n\nSigned-off-by: Pavel Roskin <proski@gnu.org>\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -100,7 +100,7 @@ test_expect_success \\\n      git-checkout-cache -u -f -q -a &&\n      git-update-cache --add yomin &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >4.out || exit\n+     git-ls-files --stage >4.out || return 1\n      diff -u M.out 4.out >4diff.out\n      compare_change 4diff.out expected &&\n      check_cache_at yomin clean'\n@@ -114,7 +114,7 @@ test_expect_success \\\n      git-update-cache --add yomin &&\n      echo yomin yomin >yomin &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >5.out || exit\n+     git-ls-files --stage >5.out || return 1\n      diff -u M.out 5.out >5diff.out\n      compare_change 5diff.out expected &&\n      check_cache_at yomin dirty'\n@@ -215,7 +215,7 @@ test_expect_success \\\n      echo nitfol nitfol >nitfol &&\n      git-update-cache --add nitfol &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >14.out || exit\n+     git-ls-files --stage >14.out || return 1\n      diff -u M.out 14.out >14diff.out\n      compare_change 14diff.out expected &&\n      check_cache_at nitfol clean'\n@@ -229,7 +229,7 @@ test_expect_success \\\n      git-update-cache --add nitfol &&\n      echo nitfol nitfol nitfol >nitfol &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >15.out || exit\n+     git-ls-files --stage >15.out || return 1\n      diff -u M.out 15.out >15diff.out\n      compare_change 15diff.out expected &&\n      check_cache_at nitfol dirty'\ndiff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh\n--- a/t/t1002-read-tree-m-u-2way.sh\n+++ b/t/t1002-read-tree-m-u-2way.sh\n@@ -73,7 +73,7 @@ test_expect_success \\\n     'rm -f .git/index &&\n      git-update-cache --add yomin &&\n      git-read-tree -m -u $treeH $treeM &&\n-     git-ls-files --stage >4.out || exit\n+     git-ls-files --stage >4.out || return 1\n      diff --unified=0 M.out 4.out >4diff.out\n      compare_change 4diff.out expected &&\n      check_cache_at yomin clean &&\n@@ -90,7 +90,7 @@ test_expect_success \\\n      git-update-cache --add yomin &&\n      echo yomin yomin >yomin &&\n      git-read-tree -m -u $treeH $treeM &&\n-     git-ls-files --stage >5.out || exit\n+     git-ls-files --stage >5.out || return 1\n      diff --unified=0 M.out 5.out >5diff.out\n      compare_change 5diff.out expected &&\n      check_cache_at yomin dirty &&\n@@ -192,7 +192,7 @@ test_expect_success \\\n      echo nitfol nitfol >nitfol &&\n      git-update-cache --add nitfol &&\n      git-read-tree -m -u $treeH $treeM &&\n-     git-ls-files --stage >14.out || exit\n+     git-ls-files --stage >14.out || return 1\n      diff --unified=0 M.out 14.out >14diff.out\n      compare_change 14diff.out expected &&\n      sum bozbar frotz >actual14.sum &&\n@@ -212,7 +212,7 @@ test_expect_success \\\n      git-update-cache --add nitfol &&\n      echo nitfol nitfol nitfol >nitfol &&\n      git-read-tree -m -u $treeH $treeM &&\n-     git-ls-files --stage >15.out || exit\n+     git-ls-files --stage >15.out || return 1\n      diff --unified=0 M.out 15.out >15diff.out\n      compare_change 15diff.out expected &&\n      check_cache_at nitfol dirty &&\ndiff --git a/t/t1005-read-tree-m-2way-emu23.sh b/t/t1005-read-tree-m-2way-emu23.sh\n--- a/t/t1005-read-tree-m-2way-emu23.sh\n+++ b/t/t1005-read-tree-m-2way-emu23.sh\n@@ -120,7 +120,7 @@ test_expect_success \\\n      git-checkout-cache -u -f -q -a &&\n      git-update-cache --add yomin &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >4.out || exit\n+     git-ls-files --stage >4.out || return 1\n      diff -u M.out 4.out >4diff.out\n      compare_change 4diff.out expected &&\n      check_cache_at yomin clean'\n@@ -136,7 +136,7 @@ test_expect_success \\\n      git-update-cache --add yomin &&\n      echo yomin yomin >yomin &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >5.out || exit\n+     git-ls-files --stage >5.out || return 1\n      diff -u M.out 5.out >5diff.out\n      compare_change 5diff.out expected &&\n      check_cache_at yomin dirty'\n@@ -241,7 +241,7 @@ test_expect_success \\\n      echo nitfol nitfol >nitfol &&\n      git-update-cache --add nitfol &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >14.out || exit\n+     git-ls-files --stage >14.out || return 1\n      diff -u M.out 14.out >14diff.out\n      compare_change 14diff.out expected &&\n      check_cache_at nitfol clean'\n@@ -255,7 +255,7 @@ test_expect_success \\\n      git-update-cache --add nitfol &&\n      echo nitfol nitfol nitfol >nitfol &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >15.out || exit\n+     git-ls-files --stage >15.out || return 1\n      diff -u M.out 15.out >15diff.out\n      compare_change 15diff.out expected &&\n      check_cache_at nitfol dirty'\n@@ -352,7 +352,7 @@ test_expect_success \\\n      sed -e \"s/such as/SUCH AS/\" bozbar-old >bozbar &&\n      git-update-cache --add bozbar &&\n      read_tree_twoway $treeH $treeM &&\n-     git-ls-files --stage >22.out || exit\n+     git-ls-files --stage >22.out || return 1\n      diff -u M.out 22.out >22diff.out\n      compare_change 22diff.out &&\n      check_cache_at bozbar clean'\ndiff --git a/t/t4002-diff-basic.sh b/t/t4002-diff-basic.sh\n--- a/t/t4002-diff-basic.sh\n+++ b/t/t4002-diff-basic.sh\n@@ -191,7 +191,7 @@ test_expect_success \\\n     'rm -fr Z [A-Z][A-Z] &&\n      git-read-tree $tree_A &&\n      git-checkout-cache -f -a &&\n-     git-read-tree -m $tree_O || (exit 1)\n+     git-read-tree -m $tree_O || return 1\n      git-update-cache --refresh >/dev/null ;# this can exit non-zero\n      git-diff-files >.test-a &&\n      cmp_diff_files_output .test-a .test-recursive-OA'\n@@ -201,7 +201,7 @@ test_expect_success \\\n     'rm -fr Z [A-Z][A-Z] &&\n      git-read-tree $tree_B &&\n      git-checkout-cache -f -a &&\n-     git-read-tree -m $tree_O || (exit 1)\n+     git-read-tree -m $tree_O || return 1\n      git-update-cache --refresh >/dev/null ;# this can exit non-zero\n      git-diff-files >.test-a &&\n      cmp_diff_files_output .test-a .test-recursive-OB'\n@@ -211,7 +211,7 @@ test_expect_success \\\n     'rm -fr Z [A-Z][A-Z] &&\n      git-read-tree $tree_B &&\n      git-checkout-cache -f -a &&\n-     git-read-tree -m $tree_A || (exit 1)\n+     git-read-tree -m $tree_A || return 1\n      git-update-cache --refresh >/dev/null ;# this can exit non-zero\n      git-diff-files >.test-a &&\n      cmp_diff_files_output .test-a .test-recursive-AB'\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -16,7 +16,7 @@ test_expect_success \\\n      for i in a b c\n      do\n \t     dd if=/dev/zero bs=4k count=1 | tr \"\\\\0\" $i >$i &&\n-\t     git-update-cache --add $i || exit\n+\t     git-update-cache --add $i || return 1\n      done &&\n      cat c >d && echo foo >>d && git-update-cache --add d &&\n      tree=`git-write-tree` &&\n@@ -29,7 +29,7 @@ test_expect_success \\\n \t while read object\n \t do\n \t    t=`git-cat-file -t $object` &&\n-\t    git-cat-file $t $object || exit 1\n+\t    git-cat-file $t $object || return 1\n \t done <obj-list\n      } >expect'\n \n@@ -58,7 +58,7 @@ test_expect_success \\\n      do\n          cmp $path ../.git/$path || {\n \t     echo $path differs.\n-\t     exit 1\n+\t     return 1\n \t }\n      done'\n cd $TRASH\n@@ -88,7 +88,7 @@ test_expect_success \\\n      do\n          cmp $path ../.git/$path || {\n \t     echo $path differs.\n-\t     exit 1\n+\t     return 1\n \t }\n      done'\n cd $TRASH\n@@ -106,7 +106,7 @@ test_expect_success \\\n \t while read object\n \t do\n \t    t=`git-cat-file -t $object` &&\n-\t    git-cat-file $t $object || exit 1\n+\t    git-cat-file $t $object || return 1\n \t done <obj-list\n     } >current &&\n     diff expect current'\n@@ -122,7 +122,7 @@ test_expect_success \\\n \t while read object\n \t do\n \t    t=`git-cat-file -t $object` &&\n-\t    git-cat-file $t $object || exit 1\n+\t    git-cat-file $t $object || return 1\n \t done <obj-list\n     } >current &&\n     diff expect current'\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -18,7 +18,7 @@ test_expect_success setup '\n \tdo\n \t    sleep 1 &&\n \t    commit=$(echo \"Commit #$i\" | git-commit-tree $tree -p $parent) &&\n-\t    parent=$commit || exit\n+\t    parent=$commit || return 1\n \tdone &&\n \techo \"$commit\" >.git/HEAD &&\n \tgit clone -l ./. victim &&\n@@ -31,7 +31,7 @@ test_expect_success setup '\n \tdo\n \t    sleep 1 &&\n \t    commit=$(echo \"Rebase #$i\" | git-commit-tree $tree -p $parent) &&\n-\t    parent=$commit || exit\n+\t    parent=$commit || return 1\n \tdone &&\n \techo \"$commit\" >.git/HEAD &&\n \techo Rebase &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -63,6 +63,7 @@ do\n \tesac\n done\n \n+exec 5>&1\n if test \"$verbose\" = \"t\"\n then\n \texec 4>&2 3>&1\n@@ -96,15 +97,24 @@ test_debug () {\n \ttest \"$debug\" == \"\" || eval \"$1\"\n }\n \n+test_run_ () {\n+\ttrap 'echo >&5 \"FATAL: Unexpected exit with code $?\"; exit 1' exit\n+\teval >&3 2>&4 \"$1\"\n+\teval_ret=\"$?\"\n+\ttrap - exit\n+\treturn 0\n+}\n+\n test_expect_failure () {\n \ttest \"$#\" == 2 ||\n \terror \"bug in the test script: not 2 parameters to test-expect-failure\"\n \tsay >&3 \"expecting failure: $2\"\n-\tif eval >&3 2>&4 \"$2\"\n+\ttest_run_ \"$2\"\n+\tif [ \"$?\" = 0 -a \"$eval_ret\" != 0 ]\n \tthen\n-\t\ttest_failure_ \"$@\"\n-\telse\n \t\ttest_ok_ \"$1\"\n+\telse\n+\t\ttest_failure_ \"$@\"\n \tfi\n }\n \n@@ -112,7 +122,8 @@ test_expect_success () {\n \ttest \"$#\" == 2 ||\n \terror \"bug in the test script: not 2 parameters to test-expect-success\"\n \tsay >&3 \"expecting success: $2\"\n-\tif eval >&3 2>&4 \"$2\"\n+\ttest_run_ \"$2\"\n+\tif [ \"$?\" = 0 -a \"$eval_ret\" = 0 ]\n \tthen\n \t\ttest_ok_ \"$1\"\n \telse\n\n\n-- \nRegards,\nPavel Roskin\n"},{"id":"7057","messageId":"7virydnh1o.fsf@assigned-by-dhcp.cox.net","threadId":"1473","inReplyTo":"1123732581.18644.37.camel@dv","subject":"Re: [PATCH] Trapping exit in tests, using return for errors","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-11T06:06:43Z","receivedAt":"2005-08-11T06:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pavel Roskin <proski@gnu.org> writes:\n\n> This patch does following:\n>\n> All instances of \"exit\", \"exit 1\" and \"(exit 1)\" in tests have been\n> replaced with \"return 1\".  In fact, \"(exit 1)\" had no effect.\n\nAre you sure about all of the above?\n\nYou are right about \"... || exit\" in the expect_success tests;\nthey are broken.  But '(exit 1)' or just saying 'false' at the\nend should have done the right thing.  Worse yet, return does\nnot seem to really work as expected, except if all you want to\ndo was to tell the test driver \"I failed\", in which case\n\"bloopl\" would work just as well.\n\nprompt$ cat k.sh\nwhat=\"$1\"\neval '\n        echo foo\n        case '$what' in\n        false)\n                (exit 1) ;;\n        exit)\n                exit 1 ;;\n        return)\n                return 1 ;;\n        esac\n'\necho \"status $?\"\nprompt$ bash k.sh exit\nfoo\nprompt$ bash k.sh false\nfoo\nstatus 1\nprompt$ bash k.sh return\nfoo\nk.sh: line 20: return: can only `return' from a function or sourced script\nstatus 1\nprompt$ \n"},{"id":"7059","messageId":"7vzmrpm1q9.fsf@assigned-by-dhcp.cox.net","threadId":"1473","inReplyTo":"7virydnh1o.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trapping exit in tests, using return for errors","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-11T06:22:54Z","receivedAt":"2005-08-11T06:22:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry, sent it out without finishing.  The worst is \"return\".\nWith ksh, ash, and dash, the script itself exits with status\ncode 1 (presumably you are trapping it with trap -- exit,\nthough).\n\nprompt$ bash k.sh exit\nfoo\nprompt$ bash k.sh false\nfoo\nstatus 1\nprompt$ bash k.sh return\nfoo\nk.sh: line 20: return: can only `return' from a function or sourced script\nstatus 1\nprompt$ ash k.sh exit\nfoo\nprompt$ ash k.sh false\nfoo\nstatus 1\nprompt$ ash k.sh return\nfoo\nprompt$ ksh k.sh exit\nfoo\nprompt$ ksh k.sh false\nfoo\nstatus 1\nprompt$ ksh k.sh return\nfoo\n"},{"id":"7060","messageId":"7vvf2dm1b8.fsf@assigned-by-dhcp.cox.net","threadId":"1473","inReplyTo":"7vzmrpm1q9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trapping exit in tests, using return for errors","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-11T06:31:55Z","receivedAt":"2005-08-11T06:31:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Sorry, sent it out without finishing.  The worst is \"return\".\n\nAh, my mistake.  You have the eval that can eval \"return\" in a\nfunction and let that \"return\" return from that function.\nCleverly done.\n\nThanks.\n"},{"id":"7062","messageId":"1123776040.2326.6.camel@dv","threadId":"1473","inReplyTo":"7vvf2dm1b8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trapping exit in tests, using return for errors","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2005-08-11T16:00:40Z","receivedAt":"2005-08-11T16:00:40Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"Hi, Junio!\n\nOn Wed, 2005-08-10 at 23:31 -0700, Junio C Hamano wrote:\n> Junio C Hamano <junkio@cox.net> writes:\n> \n> > Sorry, sent it out without finishing.  The worst is \"return\".\n> \n> Ah, my mistake.  You have the eval that can eval \"return\" in a\n> function and let that \"return\" return from that function.\n> Cleverly done.\n\nI'm glad you appreciate it.  One more fix on top of the last patch is\nneeded.\n\n\"return\" from a test would leave the exit trap set, which could cause a\nspurious error message if it's the last test in the script or\n--immediate is used.\n\nThe easiest solution would be to have a global trap that is set when\ntest-lib.sh is sourced and unset either by test_done(), error() or by\ntest_failure_() with --immediate.  This patch also depends on the patch\nthat adds test_done() the the scripts that don't have it.\n\nSigned-off-by: Pavel Roskin <proski@gnu.org>\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,6 +36,7 @@ unset SHA1_FILE_DIRECTORY\n \n error () {\n \techo \"* error: $*\"\n+\ttrap - exit\n \texit 1\n }\n \n@@ -74,6 +75,8 @@ fi\n test_failure=0\n test_count=0\n \n+trap 'echo >&5 \"FATAL: Unexpected exit with code $?\"; exit 1' exit\n+\n \n # You are not expected to call test_ok_ and test_failure_ directly, use\n # the text_expect_* functions instead.\n@@ -89,7 +92,7 @@ test_failure_ () {\n \tsay \"FAIL $test_count: $1\"\n \tshift\n \techo \"$@\" | sed -e 's/^/\t/'\n-\ttest \"$immediate\" == \"\" || exit 1\n+\ttest \"$immediate\" == \"\" || { trap - exit; exit 1; }\n }\n \n \n@@ -98,10 +101,8 @@ test_debug () {\n }\n \n test_run_ () {\n-\ttrap 'echo >&5 \"FATAL: Unexpected exit with code $?\"; exit 1' exit\n \teval >&3 2>&4 \"$1\"\n \teval_ret=\"$?\"\n-\ttrap - exit\n \treturn 0\n }\n \n@@ -132,6 +133,7 @@ test_expect_success () {\n }\n \n test_done () {\n+\ttrap - exit\n \tcase \"$test_failure\" in\n \t0)\t\n \t\t# We could:\n\n\n-- \nRegards,\nPavel Roskin\n"}]}