{"thread":{"id":"49857","subject":"[PATCH] tests: send \"bug in the test script\" errors to the script's stderr","startedAt":"2018-11-19T13:13:35Z","lastAt":"2018-11-20T12:10:03Z","messageCount":6,"participants":["SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"363634","messageId":"20181119131326.2435-1-szeder.dev@gmail.com","threadId":"49857","inReplyTo":null,"subject":"[PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-19T13:13:26Z","receivedAt":"2018-11-19T13:13:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the functions in our test library check that they were invoked\nproperly with conditions like this:\n\n  test \"$#\" = 2 ||\n  error \"bug in the test script: not 2 parameters to test-expect-success\"\n\nIf this particular condition is triggered, then 'error' will abort the\nwhole test script with a bold red error message [1] right away.\n\nHowever, under certain circumstances the test script will be aborted\ncompletely silently, namely if:\n\n  - a similar condition in a test helper function like\n    'test_line_count' is triggered,\n  - which is invoked from the test script's \"main\" shell [2],\n  - and the test script is run manually (i.e. './t1234-foo.sh' as\n    opposed to 'make t1234-foo.sh' or 'make test') [3]\n  - and without the '--verbose' option,\n\nbecause the error message is printed from within 'test_eval_', where\nstandard output is redirected either to /dev/null or to a log file.\nThe only indication that something is wrong is that not all tests in\nthe script are executed and at the end of the test script's output\nthere is no \"# passed all N tests\" message, which are subtle and can\neasily go unnoticed, as I had to experience myself.\n\nSend these \"bug in the test script\" error messages directly to the\ntest scripts standard error and thus to the terminal, so those bugs\nwill be much harder to overlook.  Instead of updating all ~20 such\n'error' calls with a redirection, let's add a BUG() function to\n'test-lib.sh', wrapping an 'error' call with the proper redirection\nand also including the common prefix of those error messages, and\nconvert all those call sites [4] to use this new BUG() function\ninstead.\n\n[1] That particular error message from 'test_expect_success' is\n    printed in color only when running with or without '--verbose';\n    with '--tee' or '--verbose-log' the error is printed without\n    color, but it is printed to the terminal nonetheless.\n\n[2] If such a condition is triggered in a subshell of a test, then\n    'error' won't be able to abort the whole test script, but only the\n    subshell, which in turn causes the test to fail in the usual way,\n    indicating loudly and clearly that something is wrong.\n\n[3] Well, 'error' aborts the test script the same way when run\n    manually or by 'make' or 'prove', but both 'make' and 'prove' pay\n    attention to the test script's exit status, and even a silently\n    aborted test script would then trigger those tools' usual\n    noticable error messages.\n\n[4] Strictly speaking, not all those 'error' calls need that\n    redirection to send their output to the terminal, see e.g.\n    'test_expect_success' in the opening example, but I think it's\n    better to be consistent.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/perf/perf-lib.sh      |  4 ++--\n t/t0001-init.sh         |  4 ++--\n t/t4013-diff-various.sh |  2 +-\n t/t5516-fetch-push.sh   |  2 +-\n t/t9902-completion.sh   |  2 +-\n t/test-lib-functions.sh | 25 ++++++++++++-------------\n t/test-lib.sh           | 10 +++++++---\n 7 files changed, 26 insertions(+), 23 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 11d1922cf5..2e33ab3ec3 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -82,7 +82,7 @@ test_perf_do_repo_symlink_config_ () {\n \n test_perf_create_repo_from () {\n \ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 parameters to test-create-repo\"\n+\tBUG \"not 2 parameters to test-create-repo\"\n \trepo=\"$1\"\n \tsource=\"$2\"\n \tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n@@ -184,7 +184,7 @@ test_wrapper_ () {\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n+\tBUG \"not 2 or 3 parameters to test-expect-success\"\n \texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 182da069f1..42a263cada 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -319,14 +319,14 @@ test_lazy_prereq GETCWD_IGNORES_PERMS '\n \tbase=GETCWD_TEST_BASE_DIR &&\n \tmkdir -p $base/dir &&\n \tchmod 100 $base ||\n-\terror \"bug in test script: cannot prepare $base\"\n+\tBUG \"cannot prepare $base\"\n \n \t(cd $base/dir && /bin/pwd -P)\n \tstatus=$?\n \n \tchmod 700 $base &&\n \trm -rf $base ||\n-\terror \"bug in test script: cannot clean $base\"\n+\tBUG \"cannot clean $base\"\n \treturn $status\n '\n \ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 73f7038253..7d985ff6b1 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -129,7 +129,7 @@ do\n \t\tcase \"$magic\" in\n \t\tnoellipses) ;;\n \t\t*)\n-\t\t\tdie \"bug in t4103: unknown magic $magic\" ;;\n+\t\t\tBUG \"unknown magic $magic\" ;;\n \t\tesac ;;\n \t*)\n \t\tcmd=\"$magic $cmd\" magic=\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 7316365a24..37e8e80893 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -95,7 +95,7 @@ mk_child() {\n \n check_push_result () {\n \ttest $# -ge 3 ||\n-\terror \"bug in the test script: check_push_result requires at least 3 parameters\"\n+\tBUG \"check_push_result requires at least 3 parameters\"\n \n \trepo_name=\"$1\"\n \tshift\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 175f83d704..b1f901170e 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1249,7 +1249,7 @@ test_expect_success 'teardown after ref completion' '\n \n test_path_completion ()\n {\n-\ttest $# = 2 || error \"bug in the test script: not 2 parameters to test_path_completion\"\n+\ttest $# = 2 || BUG \"not 2 parameters to test_path_completion\"\n \n \tlocal cur=\"$1\" expected=\"$2\"\n \techo \"$expected\" >expected &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex d158c8d0bf..18784f0152 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -418,14 +418,14 @@ test_declared_prereq () {\n test_verify_prereq () {\n \ttest -z \"$test_prereq\" ||\n \texpr >/dev/null \"$test_prereq\" : '[A-Z0-9_,!]*$' ||\n-\terror \"bug in the test script: '$test_prereq' does not look like a prereq\"\n+\tBUG \"'$test_prereq' does not look like a prereq\"\n }\n \n test_expect_failure () {\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-failure\"\n+\tBUG \"not 2 or 3 parameters to test-expect-failure\"\n \ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$@\"\n@@ -445,7 +445,7 @@ test_expect_success () {\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n+\tBUG \"not 2 or 3 parameters to test-expect-success\"\n \ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$@\"\n@@ -472,7 +472,7 @@ test_expect_success () {\n test_external () {\n \ttest \"$#\" = 4 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 3 ||\n-\terror >&5 \"bug in the test script: not 3 or 4 parameters to test_external\"\n+\tBUG \"not 3 or 4 parameters to test_external\"\n \tdescr=\"$1\"\n \tshift\n \ttest_verify_prereq\n@@ -613,7 +613,7 @@ test_path_is_missing () {\n test_line_count () {\n \tif test $# != 3\n \tthen\n-\t\terror \"bug in the test script: not 3 parameters to test_line_count\"\n+\t\tBUG \"not 3 parameters to test_line_count\"\n \telif ! test $(wc -l <\"$3\") \"$1\" \"$2\"\n \tthen\n \t\techo \"test_line_count: line count for $3 !$1 $2\"\n@@ -793,13 +793,12 @@ test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n \n \ttest -f \"$last_arg\" ||\n-\terror \"bug in the test script: test_i18ngrep requires a file\" \\\n-\t      \"to read as the last parameter\"\n+\tBUG \"test_i18ngrep requires a file to read as the last parameter\"\n \n \tif test $# -lt 2 ||\n \t   { test \"x!\" = \"x$1\" && test $# -lt 3 ; }\n \tthen\n-\t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n+\t\tBUG \"too few parameters to test_i18ngrep\"\n \tfi\n \n \tif test -n \"$GETTEXT_POISON\"\n@@ -871,7 +870,7 @@ test_seq () {\n \tcase $# in\n \t1)\tset 1 \"$@\" ;;\n \t2)\t;;\n-\t*)\terror \"bug in the test script: not 1 or 2 parameters to test_seq\" ;;\n+\t*)\tBUG \"not 1 or 2 parameters to test_seq\" ;;\n \tesac\n \ttest_seq_counter__=$1\n \twhile test \"$test_seq_counter__\" -le \"$2\"\n@@ -909,7 +908,7 @@ test_when_finished () {\n \t# doing so on Bash is better than nothing (the test will\n \t# silently pass on other shells).\n \ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n-\terror \"bug in test script: test_when_finished does nothing in a subshell\"\n+\tBUG \"test_when_finished does nothing in a subshell\"\n \ttest_cleanup=\"{ $*\n \t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n }\n@@ -918,7 +917,7 @@ test_when_finished () {\n # Usage: test_create_repo <directory>\n test_create_repo () {\n \ttest \"$#\" = 1 ||\n-\terror \"bug in the test script: not 1 parameter to test-create-repo\"\n+\tBUG \"not 1 parameter to test-create-repo\"\n \trepo=\"$1\"\n \tmkdir -p \"$repo\"\n \t(\n@@ -1230,7 +1229,7 @@ test_oid_cache () {\n \n \t\tif ! expr \"$k\" : '[a-z0-9][a-z0-9]*$' >/dev/null\n \t\tthen\n-\t\t\terror 'bug in the test script: bad hash algorithm'\n+\t\t\tBUG 'bad hash algorithm'\n \t\tfi &&\n \t\teval \"test_oid_${k}_$tag=\\\"\\$v\\\"\"\n \tdone\n@@ -1245,7 +1244,7 @@ test_oid () {\n \t# key-hash pair, so exit with an error.\n \tif eval \"test -z \\\"\\${$var+set}\\\"\"\n \tthen\n-\t\terror \"bug in the test script: undefined key '$1'\" >&2\n+\t\tBUG \"undefined key '$1'\"\n \tfi &&\n \teval \"printf '%s' \\\"\\${$var}\\\"\"\n }\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex aba66cafa2..8dd40a6f97 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -382,6 +382,10 @@ error () {\n \texit 1\n }\n \n+BUG () {\n+\terror >&7 \"bug in the test script: $*\"\n+}\n+\n say () {\n \tsay_color info \"$*\"\n }\n@@ -709,7 +713,7 @@ test_run_ () {\n \t\tif $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n \t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n \t\tthen\n-\t\t\terror \"bug in the test script: broken &&-chain or run-away HERE-DOC: $1\"\n+\t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n@@ -1209,7 +1213,7 @@ test_lazy_prereq SANITY '\n \tchmod -w SANETESTD.1 &&\n \tchmod -r SANETESTD.1/x &&\n \tchmod -rx SANETESTD.2 ||\n-\terror \"bug in test sript: cannot prepare SANETESTD\"\n+\tBUG \"cannot prepare SANETESTD\"\n \n \t! test -r SANETESTD.1/x &&\n \t! rm SANETESTD.1/x && ! test -f SANETESTD.2/x\n@@ -1217,7 +1221,7 @@ test_lazy_prereq SANITY '\n \n \tchmod +rwx SANETESTD.1 SANETESTD.2 &&\n \trm -rf SANETESTD.1 SANETESTD.2 ||\n-\terror \"bug in test sript: cannot clean SANETESTD\"\n+\tBUG \"cannot clean SANETESTD\"\n \treturn $status\n '\n \n-- \n2.20.0.rc0.134.gf0022f8e60\n\n"},{"id":"363663","messageId":"20181119194432.GA7330@sigill.intra.peff.net","threadId":"49857","inReplyTo":"20181119131326.2435-1-szeder.dev@gmail.com","subject":"Re: [PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-19T19:44:32Z","receivedAt":"2018-11-19T19:44:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:\n\n> Send these \"bug in the test script\" error messages directly to the\n> test scripts standard error and thus to the terminal, so those bugs\n> will be much harder to overlook.  Instead of updating all ~20 such\n> 'error' calls with a redirection, let's add a BUG() function to\n> 'test-lib.sh', wrapping an 'error' call with the proper redirection\n> and also including the common prefix of those error messages, and\n> convert all those call sites [4] to use this new BUG() function\n> instead.\n\nYes, I think this is an improvement.\n\n> +BUG () {\n> +\terror >&7 \"bug in the test script: $*\"\n> +}\n\nI naively expected this to go to >&4, but of course that is the\nconditional \"stderr or /dev/null, depending on --verbose\" descriptor. I\nhave a feeling that we could get rid of descriptors 5 and 7 in favor of\n3 and 4, if we did the conditional redirection when running each test,\ninstead of ahead of time.\n\nBut unless we are running out of descriptors, it's not worth the effort\n(it's debatable whether we are; 9be795fbce (t5615: avoid re-using\ndescriptor 4, 2017-12-08) made me nervous, but it's more about the\nspecial-ness of BASHE_XTRACEFD than anything).\n\nAnyway, that's all a tangent to your patch.\n\nI do notice that many of the existing \"FATAL:\" errors use descriptor 5,\nwhich goes to stdout. I'm not sure if those should actually be going to\nstderr (or if there's some TAP significance to those lines).\n\n-Peff\n"},{"id":"363683","messageId":"20181119233404.GW30222@szeder.dev","threadId":"49857","inReplyTo":"20181119194432.GA7330@sigill.intra.peff.net","subject":"Re: [PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-19T23:34:04Z","receivedAt":"2018-11-19T23:34:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 19, 2018 at 02:44:32PM -0500, Jeff King wrote:\n> On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:\n> \n> > Send these \"bug in the test script\" error messages directly to the\n> > test scripts standard error and thus to the terminal, so those bugs\n> > will be much harder to overlook.  Instead of updating all ~20 such\n> > 'error' calls with a redirection, let's add a BUG() function to\n> > 'test-lib.sh', wrapping an 'error' call with the proper redirection\n> > and also including the common prefix of those error messages, and\n> > convert all those call sites [4] to use this new BUG() function\n> > instead.\n> \n> Yes, I think this is an improvement.\n> \n> > +BUG () {\n> > +\terror >&7 \"bug in the test script: $*\"\n> > +}\n> \n> I naively expected this to go to >&4, but of course that is the\n> conditional \"stderr or /dev/null, depending on --verbose\" descriptor. \n\nThe first version of this patch did send the error message to fd 4 ;)\n\n> I do notice that many of the existing \"FATAL:\" errors use descriptor 5,\n> which goes to stdout. I'm not sure if those should actually be going to\n> stderr (or if there's some TAP significance to those lines).\n\nI chose to send these messages to standard error, because they are,\nwell, errors.  TAP only cares about stdout, but by aborting the test\nscript in BUG(), error() or die() we are already violating TAP anyway,\nso I don't think it matters whether we send \"bug in test script\" or\n\"FATAL\" errors to stdout or stderr.\n\nBTW, TAP understands \"Bail out!\" as bail out from the _entire_ test\nsuite, but that's not what we want here, I'd think.\n\nhttps://testanything.org/tap-specification.html#bail-out\n\n"},{"id":"363722","messageId":"20181120104528.GA26263@sigill.intra.peff.net","threadId":"49857","inReplyTo":"20181119233404.GW30222@szeder.dev","subject":"Re: [PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-20T10:45:28Z","receivedAt":"2018-11-20T10:45:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote:\n\n> > I do notice that many of the existing \"FATAL:\" errors use descriptor 5,\n> > which goes to stdout. I'm not sure if those should actually be going to\n> > stderr (or if there's some TAP significance to those lines).\n> \n> I chose to send these messages to standard error, because they are,\n> well, errors.  TAP only cares about stdout, but by aborting the test\n> script in BUG(), error() or die() we are already violating TAP anyway,\n> so I don't think it matters whether we send \"bug in test script\" or\n> \"FATAL\" errors to stdout or stderr.\n\nYeah, I agree it probably doesn't matter. I was mostly suggesting to\nmake the existing \">&5\" into \">&7\" for consistency. But I don't think\nthat needs to block your patch.\n\n> BTW, TAP understands \"Bail out!\" as bail out from the _entire_ test\n> suite, but that's not what we want here, I'd think.\n> \n> https://testanything.org/tap-specification.html#bail-out\n\nYes, I added the only existing \"Bail out!\" to test-lib.sh. :)\n\nI agree that's not what we want here. I actually think the current\nbehavior (to exit non-zero) does what we want. A TAP harness will\nrealize that's an error, but not block other scripts.\n\n-Peff\n"},{"id":"363723","messageId":"20181120105825.GB26263@sigill.intra.peff.net","threadId":"49857","inReplyTo":"20181120104528.GA26263@sigill.intra.peff.net","subject":"Re: [PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-20T10:58:25Z","receivedAt":"2018-11-20T10:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote:\n\n> On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote:\n> \n> > > I do notice that many of the existing \"FATAL:\" errors use descriptor 5,\n> > > which goes to stdout. I'm not sure if those should actually be going to\n> > > stderr (or if there's some TAP significance to those lines).\n> > \n> > I chose to send these messages to standard error, because they are,\n> > well, errors.  TAP only cares about stdout, but by aborting the test\n> > script in BUG(), error() or die() we are already violating TAP anyway,\n> > so I don't think it matters whether we send \"bug in test script\" or\n> > \"FATAL\" errors to stdout or stderr.\n> \n> Yeah, I agree it probably doesn't matter. I was mostly suggesting to\n> make the existing \">&5\" into \">&7\" for consistency. But I don't think\n> that needs to block your patch.\n\nBy the way, I did check to see whether this might help the situation\nwhere running under \"prove\" we see only \"Dubious...\" and not the actual\nerror() message produced by the test script. But no, it eats both stdout\nand stderr. You can sneak a line through by prepending it with \"#\", but\nonly if \"prove -o\" is used (and even then, it's hard to associate it\nwith a particular test when you're running many in parallel).\n\nSo I guess the status quo is not too bad: prove says \"dubious\", and then\nyou re-run the test with \"./t1234-foo.sh -v -i\" and you get to see the\nerror.\n\n-Peff\n"},{"id":"363730","messageId":"20181120120957.GY30222@szeder.dev","threadId":"49857","inReplyTo":"20181120105825.GB26263@sigill.intra.peff.net","subject":"Re: [PATCH] tests: send \"bug in the test script\" errors to the script's stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-20T12:09:57Z","receivedAt":"2018-11-20T12:10:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 20, 2018 at 05:58:25AM -0500, Jeff King wrote:\n> On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote:\n> \n> > On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote:\n> > \n> > > > I do notice that many of the existing \"FATAL:\" errors use descriptor 5,\n> > > > which goes to stdout. I'm not sure if those should actually be going to\n> > > > stderr (or if there's some TAP significance to those lines).\n> > > \n> > > I chose to send these messages to standard error, because they are,\n> > > well, errors.  TAP only cares about stdout, but by aborting the test\n> > > script in BUG(), error() or die() we are already violating TAP anyway,\n> > > so I don't think it matters whether we send \"bug in test script\" or\n> > > \"FATAL\" errors to stdout or stderr.\n> > \n> > Yeah, I agree it probably doesn't matter. I was mostly suggesting to\n> > make the existing \">&5\" into \">&7\" for consistency. But I don't think\n> > that needs to block your patch.\n> \n> By the way, I did check to see whether this might help the situation\n> where running under \"prove\" we see only \"Dubious...\" and not the actual\n> error() message produced by the test script. But no, it eats both stdout\n> and stderr. You can sneak a line through by prepending it with \"#\", but\n> only if \"prove -o\" is used (and even then, it's hard to associate it\n> with a particular test when you're running many in parallel).\n\nJust to be clear: I don't mind if in some combination of test\nharnesses and test options a \"bug in the test script\" message doesn't\nreach the terminal as long as I get a clearly visible error from\nsomewhere.\n\n> So I guess the status quo is not too bad: prove says \"dubious\", and then\n> you re-run the test with \"./t1234-foo.sh -v -i\" and you get to see the\n> error.\n\nAnd with '--verbose-log' the \"bug in the test script\" message goes to\nthe test's log as well, even when it has to go through fd 7 first, so\nif you use 'prove' and your GIT_TEST_OPTS includes '--verbose-log'\nthen you can just look at the log, there's no need to re-run the test.\n\n"}]}