{"thread":{"id":"55181","subject":"[PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","startedAt":"2021-02-21T19:26:20Z","lastAt":"2026-04-15T15:25:39Z","messageCount":13,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"417456","messageId":"20210221192512.3096291-2-szeder.dev@gmail.com","threadId":"55181","inReplyTo":"20210221192512.3096291-1-szeder.dev@gmail.com","subject":"[PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-02-21T19:25:12Z","receivedAt":"2021-02-21T19:26:20Z","isPatch":true,"body":"In many test helper functions we verify that they were invoked with\nsensible parameters, and call BUG() to abort the test script when the\nparameters are buggy.  6a67c75948 (test-lib-functions: restrict\ntest_must_fail usage, 2020-07-07) added such a parameter verification\nto 'test_must_fail', but it didn't report the error with BUG(), like\nwe usually do.\n\nAs discussed in detail in the previous patch, BUG() didn't really work\nin 'test_must_fail' back then, but it resolved those issues, so let's\nuse BUG() in this case as well.\n\nThe two tests checking that 'test_must_fail' recognizes invalid\nparameters need some updates:\n\n  - BUG() calls 'exit 1' to abort the test script, but we don't want\n    that to happen while testing 'test_must_fail' itself, so in those\n    tests we must invoke that function in a subshell.\n  - These tests check that 'test_must_fail' failed with the\n    appropriate error message, but BUG() sends its error message to a\n    different file descriptor, so update the redirection accordingly.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t0000-basic.sh        | 4 ++--\n t/test-lib-functions.sh | 3 +--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex a6e570d674..b9d5c6c404 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1315,12 +1315,12 @@ test_expect_success 'test_must_fail on a failing git command with env' '\n '\n \n test_expect_success 'test_must_fail rejects a non-git command' '\n-\t! test_must_fail grep ^$ notafile 2>err &&\n+\t! ( test_must_fail grep ^$ notafile ) 7>err &&\n \tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n '\n \n test_expect_success 'test_must_fail rejects a non-git command with env' '\n-\t! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&\n+\t! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err &&\n \tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex a40c1c5d83..cdbc59e4f0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -910,8 +910,7 @@ test_must_fail () {\n \tesac\n \tif ! test_must_fail_acceptable \"$@\"\n \tthen\n-\t\techo >&6 \"test_must_fail: only 'git' is allowed: $*\"\n-\t\treturn 1\n+\t\tBUG \"test_must_fail: only 'git' is allowed: $*\"\n \tfi\n \t\"$@\" 2>&6\n \texit_code=$?\n-- \n2.30.1.940.gce394404de\n\n"},{"id":"417457","messageId":"20210221192512.3096291-1-szeder.dev@gmail.com","threadId":"55181","inReplyTo":null,"subject":"[PATCH 1/2] tests: don't mess with fd 7 of test helper functions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-02-21T19:25:11Z","receivedAt":"2021-02-21T19:26:20Z","isPatch":true,"body":"In test helper functions exercising git commands, e.g.\n'test_must_fail', 'test_env' and friends, we can't access the test\nscript's original standard error, and, consequently, BUG() doesn't\nwork as expected.\n\nThe root of the issue stems from a5bf824f3b (t: prevent '-x' tracing\nfrom interfering with test helpers' stderr, 2018-02-25), where we\nstarted to use a couple of file descriptor duplications and\nredirections to separate the standard error of git commands exercised\nin test helper functions from the stderr containing the '-x' trace\noutput of said helper functions.  To achieve that the git command's\nstderr is redirected to the test helper function's fd 7, which was\npreviously duplicated from the helper's stderr.  Alas, fd 7 was not\nthe right choice for this purpose, because fd 7 is the original\nstandard error of the test script, and, consequently, we now can't\nsend error messages from within such test helper functions directly to\nthe test script's stderr.  Since BUG() does want to send its error\nmessage there it doesn't work as expected in such test helper\nfunctions, because:\n\n  - If the test helper's stderr were redirected to a file (as is often\n    the case e.g. with 'test_must_fail'), then the \"bug in the test\n    script\" error message would end up in that file.\n\n  - If the test script is invoked without any of the verbose options,\n    then that error message would get lost to /dev/null, leaving no\n    clues about why the test script aborted so suddenly.\n\nWe don't have any BUG() calls in such test helper functions yet, but\n6a67c75948 (test-lib-functions: restrict test_must_fail usage,\n2020-07-07) did start to verify the parameters of 'test_must_fail' and\nreport the error with:\n\n    echo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n    return 1\n\ninstead of BUG() that we usually use to bail out in case of bogus\nparameters.\n\nUse fd 6 instead of fd 7 for these '-x' tracing related duplications\nand redirections.  It is a better choice for this purpose, because fd\n6 is the test script's original standard input, and neither these test\nhelper functions not the git commands exercised by them should ever\nread from the test scipt's stdin, see 781f76b158 (test-lib: redirect\nstdin of tests, 2011-12-15).  Update the aforementioned error\nreporting in 'test_must_fail' to send the error message to fd 6 as\nwell; the next patch will update it to use BUG() instead.\n\nThe only two other functions that want to directly write to the test\nscript's stderr are 'test_pause' and 'debug', but they are not\naffected by this issue, because, being special \"interactive\" debug\naids, their fd 7 were not redirected in a5bf824f3b.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-terminal.sh       |  4 ++--\n t/test-lib-functions.sh | 26 +++++++++++++-------------\n 2 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/t/lib-terminal.sh b/t/lib-terminal.sh\nindex e3809dcead..454909c087 100644\n--- a/t/lib-terminal.sh\n+++ b/t/lib-terminal.sh\n@@ -9,8 +9,8 @@ test_terminal () {\n \t\techo >&4 \"test_terminal: need to declare TTY prerequisite\"\n \t\treturn 127\n \tfi\n-\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&7\n-} 7>&2 2>&4\n+\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&6\n+} 6>&2 2>&4\n \n test_lazy_prereq TTY '\n \ttest_have_prereq PERL &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 05dc2cc6be..a40c1c5d83 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -910,10 +910,10 @@ test_must_fail () {\n \tesac\n \tif ! test_must_fail_acceptable \"$@\"\n \tthen\n-\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n+\t\techo >&6 \"test_must_fail: only 'git' is allowed: $*\"\n \t\treturn 1\n \tfi\n-\t\"$@\" 2>&7\n+\t\"$@\" 2>&6\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n@@ -936,7 +936,7 @@ test_must_fail () {\n \t\treturn 1\n \tfi\n \treturn 0\n-} 7>&2 2>&4\n+} 6>&2 2>&4\n \n # Similar to test_must_fail, but tolerates success, too.  This is\n # meant to be used in contexts like:\n@@ -952,8 +952,8 @@ test_must_fail () {\n # Accepts the same options as test_must_fail.\n \n test_might_fail () {\n-\ttest_must_fail ok=success \"$@\" 2>&7\n-} 7>&2 2>&4\n+\ttest_must_fail ok=success \"$@\" 2>&6\n+} 6>&2 2>&4\n \n # Similar to test_must_fail and test_might_fail, but check that a\n # given command exited with a given exit code. Meant to be used as:\n@@ -965,7 +965,7 @@ test_might_fail () {\n test_expect_code () {\n \twant_code=$1\n \tshift\n-\t\"$@\" 2>&7\n+\t\"$@\" 2>&6\n \texit_code=$?\n \tif test $exit_code = $want_code\n \tthen\n@@ -974,7 +974,7 @@ test_expect_code () {\n \n \techo >&4 \"test_expect_code: command exited with $exit_code, we wanted $want_code $*\"\n \treturn 1\n-} 7>&2 2>&4\n+} 6>&2 2>&4\n \n # test_cmp is a helper function to compare actual and expected output.\n # You can use it like:\n@@ -1261,8 +1261,8 @@ test_write_lines () {\n }\n \n perl () {\n-\tcommand \"$PERL_PATH\" \"$@\" 2>&7\n-} 7>&2 2>&4\n+\tcommand \"$PERL_PATH\" \"$@\" 2>&6\n+} 6>&2 2>&4\n \n # Given the name of an environment variable with a bool value, normalize\n # its value to a 0 (true) or 1 (false or empty string) return code.\n@@ -1388,13 +1388,13 @@ test_env () {\n \t\t\t\tshift\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\t\"$@\" 2>&7\n+\t\t\t\t\"$@\" 2>&6\n \t\t\t\texit\n \t\t\t\t;;\n \t\t\tesac\n \t\tdone\n \t)\n-} 7>&2 2>&4\n+} 6>&2 2>&4\n \n # Returns true if the numeric exit code in \"$2\" represents the expected signal\n # in \"$1\". Signals should be given numerically.\n@@ -1436,9 +1436,9 @@ nongit () {\n \t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd non-repo &&\n-\t\t\"$@\" 2>&7\n+\t\t\"$@\" 2>&6\n \t)\n-} 7>&2 2>&4\n+} 6>&2 2>&4\n \n # convert function arguments or stdin (if not arguments given) to pktline\n # representation. If multiple arguments are given, they are separated by\n-- \n2.30.1.940.gce394404de\n\n"},{"id":"417459","messageId":"YDLVsjumwSXgEU7k@coredump.intra.peff.net","threadId":"55181","inReplyTo":"20210221192512.3096291-1-szeder.dev@gmail.com","subject":"Re: [PATCH 1/2] tests: don't mess with fd 7 of test helper functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-21T21:50:42Z","receivedAt":"2021-02-21T21:51:53Z","isPatch":true,"body":"On Sun, Feb 21, 2021 at 08:25:11PM +0100, SZEDER Gábor wrote:\n\n> The root of the issue stems from a5bf824f3b (t: prevent '-x' tracing\n> from interfering with test helpers' stderr, 2018-02-25), where we\n> started to use a couple of file descriptor duplications and\n> redirections to separate the standard error of git commands exercised\n> in test helper functions from the stderr containing the '-x' trace\n> output of said helper functions.  To achieve that the git command's\n> stderr is redirected to the test helper function's fd 7, which was\n> previously duplicated from the helper's stderr.  Alas, fd 7 was not\n> the right choice for this purpose, because fd 7 is the original\n> standard error of the test script, and, consequently, we now can't\n> send error messages from within such test helper functions directly to\n> the test script's stderr.  Since BUG() does want to send its error\n> message there it doesn't work as expected in such test helper\n> functions, because:\n> \n>   - If the test helper's stderr were redirected to a file (as is often\n>     the case e.g. with 'test_must_fail'), then the \"bug in the test\n>     script\" error message would end up in that file.\n> \n>   - If the test script is invoked without any of the verbose options,\n>     then that error message would get lost to /dev/null, leaving no\n>     clues about why the test script aborted so suddenly.\n\nMakes sense. Well explained.\n\n> Use fd 6 instead of fd 7 for these '-x' tracing related duplications\n> and redirections.  It is a better choice for this purpose, because fd\n> 6 is the test script's original standard input, and neither these test\n> helper functions not the git commands exercised by them should ever\n> read from the test scipt's stdin, see 781f76b158 (test-lib: redirect\n> stdin of tests, 2011-12-15).  Update the aforementioned error\n> reporting in 'test_must_fail' to send the error message to fd 6 as\n> well; the next patch will update it to use BUG() instead.\n\ns/scipt/script/ in the paragraph above.\n\nI agree that 6 is probably reasonable. I wonder if it is worth having a\nmaster comment describing the function of various descriptors within the\ntest suite, so that people know which ones are available for which\npurposes.  It is getting awfully crowded in that space. Sadly, I don't\nthink we can portably use numbers higher than 9 (bash is happy to, but\neven dash cannot).\n\nOf course people would have to know to look for said comment, which they\nmay not do. :)\n\n-Peff\n"},{"id":"417460","messageId":"YDLXf+OoJabrJTWu@coredump.intra.peff.net","threadId":"55181","inReplyTo":"20210221192512.3096291-2-szeder.dev@gmail.com","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-21T21:58:23Z","receivedAt":"2021-02-21T21:59:20Z","isPatch":true,"body":"On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:\n\n> In many test helper functions we verify that they were invoked with\n> sensible parameters, and call BUG() to abort the test script when the\n> parameters are buggy.  6a67c75948 (test-lib-functions: restrict\n> test_must_fail usage, 2020-07-07) added such a parameter verification\n> to 'test_must_fail', but it didn't report the error with BUG(), like\n> we usually do.\n\nOK. I do not care all that much between BUG() and not-BUG here, since we\nare unlikely to have a test where test_must_fail returning 0 yields\nsuccess. I guess the most interesting outcome is that we would notice a\nbug in a test_expect_failure block.\n\n> The two tests checking that 'test_must_fail' recognizes invalid\n> parameters need some updates:\n> \n>   - BUG() calls 'exit 1' to abort the test script, but we don't want\n>     that to happen while testing 'test_must_fail' itself, so in those\n>     tests we must invoke that function in a subshell.\n>   - These tests check that 'test_must_fail' failed with the\n>     appropriate error message, but BUG() sends its error message to a\n>     different file descriptor, so update the redirection accordingly.\n\nThis is a bit intimate with the magic 7 descriptor. I think it would be\ncleaner to trigger the bug in a sub-test. We do have helpers for that,\nlike:\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b9d5c6c404..b3fd740452 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1315,13 +1315,25 @@ test_expect_success 'test_must_fail on a failing git command with env' '\n '\n \n test_expect_success 'test_must_fail rejects a non-git command' '\n-\t! ( test_must_fail grep ^$ notafile ) 7>err &&\n-\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+\tcmd=\"grep ^$ notafile\" &&\n+\trun_sub_test_lib_test_err bug-fail-nongit \"fail nongit\" <<-EOF &&\n+\ttest_expect_success \"non-git command\" \"test_must_fail $cmd\"\n+\tEOF\n+\tcheck_sub_test_lib_test_err bug-fail-nongit <<-\\EOF_OUT 3<<-EOF_ERR\n+\tEOF_OUT\n+\t> error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd\n+\tEOF_ERR\n '\n \n test_expect_success 'test_must_fail rejects a non-git command with env' '\n-\t! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err &&\n-\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+\tcmd=\"env var1=a var2=b grep ^$ notafile\" &&\n+\trun_sub_test_lib_test_err bug-fail-env \"fail nongit with env\" <<-EOF &&\n+\ttest_expect_success \"non-git command with env\" \"test_must_fail $cmd\"\n+\tEOF\n+\tcheck_sub_test_lib_test_err bug-fail-env <<-\\EOF_OUT 3<<-EOF_ERR\n+\tEOF_OUT\n+\t> error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd\n+\tEOF_ERR\n '\n \n test_done\n\nThis is modeled after other similar tests. I find the use of\ncheck_sub_test_lib_test_err here a bit verbose, but I think we could\nalso easily do:\n\n  grep \"bug in the test.*only .git. is allowed\" bug-fail-nongit/err\n\nNote that there are some other cases which could likewise be converted\n(the one for test_bool_env, which I noticed when grepping for \"7>\" when\ninvestigating the first patch).\n\n>  test_expect_success 'test_must_fail rejects a non-git command' '\n> -\t! test_must_fail grep ^$ notafile 2>err &&\n> +\t! ( test_must_fail grep ^$ notafile ) 7>err &&\n>  \tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n>  '\n\nHoly double-quoting batman! I do think using $SQ or just \".\" (if using\ngrep) to match single-quotes makes things more readable. Obviously not\nsomething you're introducing, but perhaps worth addressing as we touch\nthis test.\n\n-Peff\n"},{"id":"417476","messageId":"xmqq8s7g809p.fsf@gitster.g","threadId":"55181","inReplyTo":"YDLVsjumwSXgEU7k@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] tests: don't mess with fd 7 of test helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-22T17:45:06Z","receivedAt":"2021-02-22T17:45:53Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> I agree that 6 is probably reasonable. I wonder if it is worth having a\n> master comment describing the function of various descriptors within the\n> test suite, so that people know which ones are available for which\n> purposes.  It is getting awfully crowded in that space.\n\nThanks for a review.\nI had the same impression.  I'll wait for a reroll.\n\n\n"},{"id":"417482","messageId":"YDQBxqTbuYgq1xV8@coredump.intra.peff.net","threadId":"55181","inReplyTo":"YDLXf+OoJabrJTWu@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-22T19:11:02Z","receivedAt":"2021-02-22T19:14:52Z","isPatch":true,"body":"On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:\n\n> This is a bit intimate with the magic 7 descriptor. I think it would be\n> cleaner to trigger the bug in a sub-test. We do have helpers for that,\n> like:\n> [...]\n> This is modeled after other similar tests. I find the use of\n> check_sub_test_lib_test_err here a bit verbose, but I think we could\n> also easily do:\n> \n>   grep \"bug in the test.*only .git. is allowed\" bug-fail-nongit/err\n\nIn case it helps, here is a patch which can go on top of yours that\nimplements my suggestion using the (IMHO) more readable grep.\n\nIt also adds \"test_done\" to the sub-test snippets, which my earlier\npatch did not include. That's not strictly necessary (we should error\nout before we even get there), but it makes the test more robust (we are\nsure that the BUG is what caused us to exit non-zero, not the missing\ntest_done).\n\n> Note that there are some other cases which could likewise be converted\n> (the one for test_bool_env, which I noticed when grepping for \"7>\" when\n> investigating the first patch).\n\nThat looks like the only other one, and I think is likewise worth\nconverting (which is in the patch below).\n\nI thought it first it was also a problem that test_bool_env does not use\nBUG to catch invalid values. But I think it is trying to make its\nmessage a bit less confusing when the user is the one who provided us\nwith the invalid value, such as setting GIT_TEST_GIT_DAEMON=nonsense. We\ndo still exit the test script immediately, though, which is the right\nthing.\n\nIt does use BUG to complain when test_bool_env didn't get two\nparameters, but we don't bother to test it. We could.\n\n-- >8 --\nSubject: [PATCH] t0000: put bug/error checks into a sub-test\n\nWhen checking whether test_bool_env and test_must_fail correctly trigger\nerrors or bugs, we run them in a subshell (to avoid their \"exit\" calls\nimpacting the greater script) with descriptor 7 redirected (to catch\ntheir direct-to-user output). Let's instead run them using our sub-test\nhelpers. That gives us a more accurate view of what a calling user sees,\nand avoids knowing details like the magic of descriptor 7.\n\nNote that I didn't use check_sub_test_lib_test_err here. We don't really\ncare what (if anything) is printed on stdout, as long as we see our\nexpected error on stderr. Plus using grep makes it easier to formulate\nthe expected text (e.g., using \".\" instead of tricky quoting).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis does end up with more lines, and certainly more processes, than the\noriginal. I do think the conceptual cleanliness is worth it, though.\n\n t/t0000-basic.sh | 50 ++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 36 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b9d5c6c404..e5c06d055b 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -981,20 +981,32 @@ test_expect_success 'test_bool_env' '\n \n \t\tenvvar=false &&\n \t\t! test_bool_env envvar true &&\n-\t\t! test_bool_env envvar false &&\n+\t\t! test_bool_env envvar false\n+\t)\n+'\n \n+test_expect_success 'test_bool_env invalid value' '\n+\trun_sub_test_lib_test_err invalid-bool-value \"invalid value\" <<-\\EOF &&\n+\ttest_expect_success \"invalid bool\" \"\n \t\tenvvar=invalid &&\n-\t\t# When encountering an invalid bool value, test_bool_env\n-\t\t# prints its error message to the original stderr of the\n-\t\t# test script, hence the redirection of fd 7, and aborts\n-\t\t# with \"exit 1\", hence the subshell.\n-\t\t! ( test_bool_env envvar true ) 7>err &&\n-\t\tgrep \"error: test_bool_env requires bool values\" err &&\n+\t\texport envvar &&\n+\t\ttest_bool_env envvar true\n+\t\"\n+\ttest_done\n+\tEOF\n+\tgrep \"error: test_bool_env requires bool values\" invalid-bool-value/err\n+'\n \n+test_expect_success 'test_bool_env invalid default' '\n+\trun_sub_test_lib_test_err invalid-bool-default \"invalid default\" <<-\\EOF &&\n+\ttest_expect_success \"invalid bool\" \"\n \t\tenvvar=true &&\n-\t\t! ( test_bool_env envvar invalid ) 7>err &&\n-\t\tgrep \"error: test_bool_env requires bool values\" err\n-\t)\n+\t\texport envvar &&\n+\t\ttest_bool_env envvar default\n+\t\"\n+\ttest_done\n+\tEOF\n+\tgrep \"error: test_bool_env requires bool values\" invalid-bool-default/err\n '\n \n ################################################################\n@@ -1315,13 +1327,23 @@ test_expect_success 'test_must_fail on a failing git command with env' '\n '\n \n test_expect_success 'test_must_fail rejects a non-git command' '\n-\t! ( test_must_fail grep ^$ notafile ) 7>err &&\n-\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+\trun_sub_test_lib_test_err bug-fail-nongit \"fail nongit\" <<-\\EOF &&\n+\ttest_expect_success \"non-git command\" \"\n+\t\ttest_must_fail grep ^$ notafile\n+\t\"\n+\ttest_done\n+\tEOF\n+\tgrep \"bug.*test_must_fail: only .git. is allowed\" bug-fail-nongit/err\n '\n \n test_expect_success 'test_must_fail rejects a non-git command with env' '\n-\t! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err &&\n-\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+\trun_sub_test_lib_test_err bug-fail-env \"fail nongit with env\" <<-\\EOF &&\n+\ttest_expect_success \"non-git command with env\" \"\n+\t\ttest_must_fail env var1=a var2=b grep ^$ notafile\n+\t\"\n+\ttest_done\n+\tEOF\n+\tgrep \"bug.*test_must_fail: only .git. is allowed\" bug-fail-env/err\n '\n \n test_done\n-- \n2.30.1.1033.gd525307ce1\n\n\n"},{"id":"417483","messageId":"YDQDX/zdGTI1HmJ9@coredump.intra.peff.net","threadId":"55181","inReplyTo":"YDQBxqTbuYgq1xV8@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-22T19:17:51Z","receivedAt":"2021-02-22T19:21:30Z","isPatch":true,"body":"On Mon, Feb 22, 2021 at 02:11:02PM -0500, Jeff King wrote:\n\n> It also adds \"test_done\" to the sub-test snippets, which my earlier\n> patch did not include. That's not strictly necessary (we should error\n> out before we even get there), but it makes the test more robust (we are\n> sure that the BUG is what caused us to exit non-zero, not the missing\n> test_done).\n\nI'm quite tempted to do this, as well (this is on top of what I just\nsent, but could easily be done independently by removing the hunks\ntouching the instances I just added):\n\n-- >8 --\nSubject: [PATCH] t0000: automatically add test_done to sub-test snippets\n\nOur sub-test helper already handles setting up test_description,\nincluding test-lib.sh, etc. Let's also automatically a test_done at the\nend, since it is easy to forget and essentially every test snippet will\nwant it.\n\nThe only test which _wouldn't_ want this is one that was specifically\ntrying to check the behavior when test_done is not run. We don't have\nsuch a test, but if we wanted one, it could just as easily run \"exit 0\"\nas part of its snippet (which is arguably more obvious and readable than\nleaving out the test_done, anyway).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0000-basic.sh | 46 +---------------------------------------------\n 1 file changed, 1 insertion(+), 45 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex e5c06d055b..5a9592dd10 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -92,6 +92,7 @@ _run_sub_test_lib_test_common () {\n \t\t. \"\\$TEST_DIRECTORY\"/test-lib.sh\n \t\tEOF\n \t\tcat >>\"$name.sh\" &&\n+\t\techo \"test_done\" >>\"$name.sh\" &&\n \t\texport TEST_DIRECTORY &&\n \t\tTEST_OUTPUT_DIRECTORY=$(pwd) &&\n \t\texport TEST_OUTPUT_DIRECTORY &&\n@@ -141,7 +142,6 @@ test_expect_success 'pretend we have a fully passing test suite' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test full-pass <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -158,7 +158,6 @@ test_expect_success 'pretend we have a partially passing test suite' '\n \ttest_expect_success \"passing test #1\" \"true\"\n \ttest_expect_success \"failing test #2\" \"false\"\n \ttest_expect_success \"passing test #3\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test partial-pass <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -174,7 +173,6 @@ test_expect_success 'pretend we have a known breakage' '\n \trun_sub_test_lib_test failing-todo \"A failing TODO test\" <<-\\EOF &&\n \ttest_expect_success \"passing test\" \"true\"\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test failing-todo <<-\\EOF\n \t> ok 1 - passing test\n@@ -188,7 +186,6 @@ test_expect_success 'pretend we have a known breakage' '\n test_expect_success 'pretend we have fixed a known breakage' '\n \trun_sub_test_lib_test passing-todo \"A passing TODO test\" <<-\\EOF &&\n \ttest_expect_failure \"pretend we have fixed a known breakage\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test passing-todo <<-\\EOF\n \t> ok 1 - pretend we have fixed a known breakage # TODO known breakage vanished\n@@ -203,7 +200,6 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n \ttest_expect_success \"pretend we have a passing test\" \"true\"\n \ttest_expect_failure \"pretend we have fixed another known breakage\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test partially-passing-todos <<-\\EOF\n \t> not ok 1 - pretend we have a known breakage # TODO known breakage\n@@ -222,7 +218,6 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' '\n \ttest_expect_success \"passing test\" \"true\"\n \ttest_expect_success \"failing test\" \"false\"\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test mixed-results1 <<-\\EOF\n \t> ok 1 - passing test\n@@ -248,7 +243,6 @@ test_expect_success 'pretend we have a mix of all possible results' '\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n \ttest_expect_failure \"pretend we have fixed a known breakage\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test mixed-results2 <<-\\EOF\n \t> ok 1 - passing test\n@@ -277,7 +271,6 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n \ttest_expect_success \"failing test\" false\n-\ttest_done\n \tEOF\n \tmv t1234-verbose/out t1234-verbose/out+ &&\n \tgrep -v \"^Initialized empty\" t1234-verbose/out+ >t1234-verbose/out &&\n@@ -305,7 +298,6 @@ test_expect_success 'test --verbose-only' '\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n \ttest_expect_success \"failing test\" false\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test t2345-verbose-only-2 <<-\\EOF\n \t> ok 1 - passing test\n@@ -330,7 +322,6 @@ test_expect_success 'GIT_SKIP_TESTS' '\n \t\tdo\n \t\t\ttest_expect_success \"passing test #$i\" \"true\"\n \t\tdone\n-\t\ttest_done\n \t\tEOF\n \t\tcheck_sub_test_lib_test git-skip-tests-basic <<-\\EOF\n \t\t> ok 1 - passing test #1\n@@ -351,7 +342,6 @@ test_expect_success 'GIT_SKIP_TESTS several tests' '\n \t\tdo\n \t\t\ttest_expect_success \"passing test #$i\" \"true\"\n \t\tdone\n-\t\ttest_done\n \t\tEOF\n \t\tcheck_sub_test_lib_test git-skip-tests-several <<-\\EOF\n \t\t> ok 1 - passing test #1\n@@ -375,7 +365,6 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' '\n \t\tdo\n \t\t\ttest_expect_success \"passing test #$i\" \"true\"\n \t\tdone\n-\t\ttest_done\n \t\tEOF\n \t\tcheck_sub_test_lib_test git-skip-tests-sh-pattern <<-\\EOF\n \t\t> ok 1 - passing test #1\n@@ -399,7 +388,6 @@ test_expect_success 'GIT_SKIP_TESTS entire suite' '\n \t\tdo\n \t\t\ttest_expect_success \"passing test #$i\" \"true\"\n \t\tdone\n-\t\ttest_done\n \t\tEOF\n \t\tcheck_sub_test_lib_test git-skip-tests-entire-suite <<-\\EOF\n \t\t> 1..0 # SKIP skip all tests in git\n@@ -416,7 +404,6 @@ test_expect_success 'GIT_SKIP_TESTS does not skip unmatched suite' '\n \t\tdo\n \t\t\ttest_expect_success \"passing test #$i\" \"true\"\n \t\tdone\n-\t\ttest_done\n \t\tEOF\n \t\tcheck_sub_test_lib_test git-skip-tests-unmatched-suite <<-\\EOF\n \t\t> ok 1 - passing test #1\n@@ -435,7 +422,6 @@ test_expect_success '--run basic' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-basic <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -456,7 +442,6 @@ test_expect_success '--run with a range' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-range <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -477,7 +462,6 @@ test_expect_success '--run with two ranges' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-two-ranges <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -498,7 +482,6 @@ test_expect_success '--run with a left open range' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-left-open-range <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -519,7 +502,6 @@ test_expect_success '--run with a right open range' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-right-open-range <<-\\EOF\n \t> ok 1 # skip passing test #1 (--run)\n@@ -540,7 +522,6 @@ test_expect_success '--run with basic negation' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-basic-neg <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -561,7 +542,6 @@ test_expect_success '--run with two negations' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-two-neg <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -582,7 +562,6 @@ test_expect_success '--run a range and negation' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-range-and-neg <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -603,7 +582,6 @@ test_expect_success '--run range negation' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-range-neg <<-\\EOF\n \t> ok 1 # skip passing test #1 (--run)\n@@ -625,7 +603,6 @@ test_expect_success '--run include, exclude and include' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-inc-neg-inc <<-\\EOF\n \t> ok 1 # skip passing test #1 (--run)\n@@ -647,7 +624,6 @@ test_expect_success '--run include, exclude and include, comma separated' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-inc-neg-inc-comma <<-\\EOF\n \t> ok 1 # skip passing test #1 (--run)\n@@ -669,7 +645,6 @@ test_expect_success '--run exclude and include' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-neg-inc <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -691,7 +666,6 @@ test_expect_success '--run empty selectors' '\n \tdo\n \t\ttest_expect_success \"passing test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-empty-sel <<-\\EOF\n \t> ok 1 - passing test #1\n@@ -714,7 +688,6 @@ test_expect_success '--run substring selector' '\n \tdo\n \t\ttest_expect_success \"other test #$i\" \"true\"\n \tdone\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test run-substring-selector <<-\\EOF\n \t> ok 1 - relevant test\n@@ -734,7 +707,6 @@ test_expect_success '--run keyword selection' '\n \t\t\"--run invalid range start\" \\\n \t\t--run=\"a-5\" <<-\\EOF &&\n \ttest_expect_success \"passing test #1\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test_err run-inv-range-start \\\n \t\t<<-\\EOF_OUT 3<<-EOF_ERR\n@@ -749,7 +721,6 @@ test_expect_success '--run invalid range end' '\n \t\t\"--run invalid range end\" \\\n \t\t--run=\"1-z\" <<-\\EOF &&\n \ttest_expect_success \"passing test #1\" \"true\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test_err run-inv-range-end \\\n \t\t<<-\\EOF_OUT 3<<-EOF_ERR\n@@ -773,8 +744,6 @@ test_expect_success 'tests respect prerequisites' '\n \ttest_expect_success HAVETHIS,HAVEIT \"multiple prereqs\" \"true\"\n \ttest_expect_success HAVEIT,DONTHAVEIT \"mixed prereqs (yes,no)\" \"false\"\n \ttest_expect_success DONTHAVEIT,HAVEIT \"mixed prereqs (no,yes)\" \"false\"\n-\n-\ttest_done\n \tEOF\n \n \tcheck_sub_test_lib_test prereqs <<-\\EOF\n@@ -799,8 +768,6 @@ test_expect_success 'tests respect lazy prerequisites' '\n \ttest_lazy_prereq LAZY_FALSE false\n \ttest_expect_success LAZY_FALSE \"lazy prereq not satisfied\" \"false\"\n \ttest_expect_success !LAZY_FALSE \"negative false prereq\" \"true\"\n-\n-\ttest_done\n \tEOF\n \n \tcheck_sub_test_lib_test lazy-prereqs <<-\\EOF\n@@ -828,8 +795,6 @@ test_expect_success 'nested lazy prerequisites' '\n \t\ttest_path_is_missing inner\n \t\"\n \ttest_expect_success NESTED_PREREQ \"evaluate nested prereq\" \"true\"\n-\n-\ttest_done\n \tEOF\n \n \tcheck_sub_test_lib_test nested-lazy <<-\\EOF\n@@ -845,8 +810,6 @@ test_expect_success 'lazy prereqs do not turn off tracing' '\n \ttest_lazy_prereq LAZY true\n \n \ttest_expect_success lazy \"test_have_prereq LAZY && echo trace\"\n-\n-\ttest_done\n \tEOF\n \n \tgrep \"echo trace\" lazy-prereq-and-tracing/err\n@@ -861,7 +824,6 @@ test_expect_success 'tests clean up after themselves' '\n \ttest_expect_success \"cleanup happened\" \"\n \t\ttest $clean = yes\n \t\"\n-\ttest_done\n \tEOF\n \n \tcheck_sub_test_lib_test cleanup <<-\\EOF\n@@ -883,7 +845,6 @@ test_expect_success 'tests clean up even on failures' '\n \ttest_expect_success \"failure to clean up causes the test to fail\" \"\n \t\ttest_when_finished \\\"(exit 2)\\\"\n \t\"\n-\ttest_done\n \tEOF\n \tcheck_sub_test_lib_test failing-cleanup <<-\\EOF\n \t> not ok 1 - tests clean up even after a failure\n@@ -912,7 +873,6 @@ test_expect_success 'test_atexit is run' '\n \t\t> ../../dont-clean-atexit &&\n \t\t(exit 1)\n \t\"\n-\ttest_done\n \tEOF\n \ttest_path_is_file dont-clean-atexit &&\n \ttest_path_is_missing clean-atexit &&\n@@ -992,7 +952,6 @@ test_expect_success 'test_bool_env invalid value' '\n \t\texport envvar &&\n \t\ttest_bool_env envvar true\n \t\"\n-\ttest_done\n \tEOF\n \tgrep \"error: test_bool_env requires bool values\" invalid-bool-value/err\n '\n@@ -1004,7 +963,6 @@ test_expect_success 'test_bool_env invalid default' '\n \t\texport envvar &&\n \t\ttest_bool_env envvar default\n \t\"\n-\ttest_done\n \tEOF\n \tgrep \"error: test_bool_env requires bool values\" invalid-bool-default/err\n '\n@@ -1331,7 +1289,6 @@ test_expect_success 'test_must_fail rejects a non-git command' '\n \ttest_expect_success \"non-git command\" \"\n \t\ttest_must_fail grep ^$ notafile\n \t\"\n-\ttest_done\n \tEOF\n \tgrep \"bug.*test_must_fail: only .git. is allowed\" bug-fail-nongit/err\n '\n@@ -1341,7 +1298,6 @@ test_expect_success 'test_must_fail rejects a non-git command with env' '\n \ttest_expect_success \"non-git command with env\" \"\n \t\ttest_must_fail env var1=a var2=b grep ^$ notafile\n \t\"\n-\ttest_done\n \tEOF\n \tgrep \"bug.*test_must_fail: only .git. is allowed\" bug-fail-env/err\n '\n-- \n2.30.1.1033.gd525307ce1\n\n"},{"id":"417493","messageId":"xmqqk0qz7twz.fsf@gitster.g","threadId":"55181","inReplyTo":"YDQDX/zdGTI1HmJ9@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-22T20:02:20Z","receivedAt":"2021-02-22T20:03:28Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> ... Let's also automatically a test_done at the\n> end, ...\n\ns/a test_done/add &/ probably.\n\n"},{"id":"541589","messageId":"ad6pEbnSKzUOkS2k@szeder.dev","threadId":"55181","inReplyTo":"YDLXf+OoJabrJTWu@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2026-04-14T20:52:33Z","receivedAt":"2026-04-14T20:52:36Z","isPatch":true,"body":"On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:\n> On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:\n> \n> > In many test helper functions we verify that they were invoked with\n> > sensible parameters, and call BUG() to abort the test script when the\n> > parameters are buggy.  6a67c75948 (test-lib-functions: restrict\n> > test_must_fail usage, 2020-07-07) added such a parameter verification\n> > to 'test_must_fail', but it didn't report the error with BUG(), like\n> > we usually do.\n> \n> OK. I do not care all that much between BUG() and not-BUG here, since we\n> are unlikely to have a test where test_must_fail returning 0 yields\n> success. I guess the most interesting outcome is that we would notice a\n> bug in a test_expect_failure block.\n\nIf I had managed to send a new version of this patch series in the\nlast 5 years :), then this would have caught the issue noted in:\n\n  https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/\n\n\n"},{"id":"541591","messageId":"xmqqv7dt8cyj.fsf@gitster.g","threadId":"55181","inReplyTo":"ad6pEbnSKzUOkS2k@szeder.dev","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-14T21:11:00Z","receivedAt":"2026-04-14T21:11:02Z","isPatch":true,"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:\n>> On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:\n>> \n>> > In many test helper functions we verify that they were invoked with\n>> > sensible parameters, and call BUG() to abort the test script when the\n>> > parameters are buggy.  6a67c75948 (test-lib-functions: restrict\n>> > test_must_fail usage, 2020-07-07) added such a parameter verification\n>> > to 'test_must_fail', but it didn't report the error with BUG(), like\n>> > we usually do.\n>> \n>> OK. I do not care all that much between BUG() and not-BUG here, since we\n>> are unlikely to have a test where test_must_fail returning 0 yields\n>> success. I guess the most interesting outcome is that we would notice a\n>> bug in a test_expect_failure block.\n>\n> If I had managed to send a new version of this patch series in the\n> last 5 years :), then this would have caught the issue noted in:\n>\n>   https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/\n\nI was wondering if we should remove \"test_might_fail\".  Its use case\nis rather limited to very narrow cases, like\n\n * we want to kill something but it may have exited on its own\n\n * we want \"git config --unset\" but the variable may or may not be set\n\n * we want \"git foo --abort\" just in case we are in the middle of\n   \"git foo\"\n\nall of which is clearer with \"|| :\", and more importantly, the thing\nwhose \"failure\" is protected against the test framework declaring a\ntest failure is *not* what we are testing (these \"config --unset\"\nare not about testing \"git config\", in other words).\n\nSo the extra ability test_must_fail and test_might_fail have that\nthey can detect uncontrolled death with non-zero exit status (aka\n\"crash\") is not very interesting---it is more like \"As we are\nrunning a git command here, it would be better to catch than not\ncatch a segfault here as well\", i.e., a nice to have item.\n\n"},{"id":"541600","messageId":"20260414221429.GA3475104@coredump.intra.peff.net","threadId":"55181","inReplyTo":"ad6pEbnSKzUOkS2k@szeder.dev","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-14T22:14:29Z","receivedAt":"2026-04-14T22:14:30Z","isPatch":true,"body":"On Tue, Apr 14, 2026 at 10:52:33PM +0200, SZEDER Gábor wrote:\n\n> On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:\n> > On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:\n> > \n> > > In many test helper functions we verify that they were invoked with\n> > > sensible parameters, and call BUG() to abort the test script when the\n> > > parameters are buggy.  6a67c75948 (test-lib-functions: restrict\n> > > test_must_fail usage, 2020-07-07) added such a parameter verification\n> > > to 'test_must_fail', but it didn't report the error with BUG(), like\n> > > we usually do.\n> > \n> > OK. I do not care all that much between BUG() and not-BUG here, since we\n> > are unlikely to have a test where test_must_fail returning 0 yields\n> > success. I guess the most interesting outcome is that we would notice a\n> > bug in a test_expect_failure block.\n> \n> If I had managed to send a new version of this patch series in the\n> last 5 years :), then this would have caught the issue noted in:\n> \n>   https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/\n\nLooking at that old thread, I do not see any reason you should not\nre-send it (with or without the cosmetic fixups I suggested on top).\n\n-Peff\n"},{"id":"541602","messageId":"20260414221807.GB3475104@coredump.intra.peff.net","threadId":"55181","inReplyTo":"xmqqv7dt8cyj.fsf@gitster.g","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-14T22:18:07Z","receivedAt":"2026-04-14T22:18:09Z","isPatch":true,"body":"On Tue, Apr 14, 2026 at 02:11:00PM -0700, Junio C Hamano wrote:\n\n> I was wondering if we should remove \"test_might_fail\".  Its use case\n> is rather limited to very narrow cases, like\n> \n>  * we want to kill something but it may have exited on its own\n> \n>  * we want \"git config --unset\" but the variable may or may not be set\n> \n>  * we want \"git foo --abort\" just in case we are in the middle of\n>    \"git foo\"\n> \n> all of which is clearer with \"|| :\", and more importantly, the thing\n> whose \"failure\" is protected against the test framework declaring a\n> test failure is *not* what we are testing (these \"config --unset\"\n> are not about testing \"git config\", in other words).\n> \n> So the extra ability test_must_fail and test_might_fail have that\n> they can detect uncontrolled death with non-zero exit status (aka\n> \"crash\") is not very interesting---it is more like \"As we are\n> running a git command here, it would be better to catch than not\n> catch a segfault here as well\", i.e., a nice to have item.\n\nI think the main value of both (but especially test_might_fail) is that\nthey slot naturally into &&-chains. I left a similar comment in that\nother thread, but to expand a bit, if you do:\n\n  false &&\n  true || : &&\n  echo everything ok\n\nyou will get \"everything ok\", even though step 1 failed. You need:\n\n  false &&\n  { true || : } &&\n  echo everything ok\n\nexcept that because it is shell you have to add an extra semicolon after\nthe \":\". ;)\n\nSyntax-complaints aside, I think it is a very easy thing for\ncontributors to get wrong. So I think test_might_fail has value, though\nI do not care if it has a different name.\n\n-Peff\n"},{"id":"541669","messageId":"xmqqeckg8cun.fsf@gitster.g","threadId":"55181","inReplyTo":"20260414221807.GB3475104@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-15T15:25:36Z","receivedAt":"2026-04-15T15:25:39Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> I think the main value of both (but especially test_might_fail) is that\n> they slot naturally into &&-chains. I left a similar comment in that\n> other thread, but to expand a bit, if you do:\n>\n>   false &&\n>   true || : &&\n>   echo everything ok\n>\n> you will get \"everything ok\", even though step 1 failed. You need:\n>\n>   false &&\n>   { true || : } &&\n>   echo everything ok\n>\n> except that because it is shell you have to add an extra semicolon after\n> the \":\". ;)\n>\n> Syntax-complaints aside, I think it is a very easy thing for\n> contributors to get wrong. So I think test_might_fail has value, though\n> I do not care if it has a different name.\n\nYup, I recall that I recently said that I hate that semicolon ;-)\n"}]}