Re: [PATCH 06/12] t: prepare execution of potentially failing commands for `set -e`

[Jeff King <peff@peff.net>]

On Thu, Apr 16, 2026 at 07:34:26AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm still concerned that this approach is going to create extra friction
> > for test writers down the road. This series needed to clean up several
> > spots to avoid false positives, and some of the spots were non-trivial.
> >
> > Now that was the accumulated cruft of 20 years of test-writing, so it's
> > not clear to me how often new test-writers will run into this. But when
> > they do, I worry that it may be hard to even figure out what is going
> > on.
> >
> > But I've said as much in earlier rounds, and I'm not sure Junio agrees.
> > So we can note my dissent in the captain's log, and I can reserve the
> > right to told-you-so later if need be. ;)
> 
> The alternatigve to allow us to be sloppy is alluring from the point
> of view of a test writer in me.  But do we have an easy/canned way
> to run tests and see the unexpected failures outside test_expect_foo
> while ignoring all the noises from passing tests?  Perhaps running
> tests (with and without prove) while redirecting the standard output
> stream to /dev/null or something?

I took a stab at this. It's easy to do it hackily, but it proved a
little trickier than I'd hoped to get something elegant.

My main goals were that you would still get good output to either the
terminal or the test-results/*.out files (so we can't just redirect
stderr for all runs), and that we wouldn't spend too much extra CPU (so
just running the tests an extra time with stderr redirected is right
out).

My first attempt was to teach test-lib.sh a mode where all of the test
snippets are noops, stderr goes to a file in test-results/, and at the
end of the script we complain if our stderr file is non-empty. And then
we have a "test-lint-stderr" make target that runs each script in the
special mode. That works, but:

  1. Quite a few scripts do stuff inside their test snippets that affect
     the environment, and then do stuff outside of a test snippet that
     depends on that. Some of it is questionable, like running git
     commands outside of test snippets to generate expected output. But
     there's reasonable stuff like t0012 generating the list of builtin
     inside a test snippet and expecting:

       while read builtin
       do
               test_expect_success "$builtin..." ...
       done <builtins

     So I think the test suite is a little too free-form for this kind
     of trickery.

  2. It's way too slow, anyway. Even with all of the snippets as noops,
     it still takes ~13s to run all of the scripts in parallel on my
     machine. The real test suite only takes ~90 seconds! There's
     probably low-hanging fruit to fix there, but it's still an order of
     magnitude off what I'd hope the cost would be.

So I think that's probably a dead end.

The next obvious choice is redirecting stderr via tee or similar, which
is easy-ish. We already do it for --verbose-log, but we combine it with
stdout there (and we turn on verbose mode for the tests, which is going
to generate a bunch of extra output).

I think there's probably a way to do this with an extra tee for stderr.
But actually...do we even need tee? The idea is that we are not
expecting any output here, so if there is any, we'd bail and show it
along with an error.

So I think we might able able to get by with just redirecting descriptor
2, and then making sure that test snippets stderr goes to the original
stderr (which we already save as descriptor 7). And we catch only errors
outside of the snippet.

Something like this seems to work:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f7868c537..6657d56e52 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -728,7 +728,7 @@ then
 	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
 elif test "$verbose" = "t"
 then
-	exec 4>&2 3>&2
+	exec 4>&7 3>&7
 else
 	exec 4>/dev/null 3>/dev/null
 fi
@@ -970,7 +970,7 @@ maybe_setup_verbose () {
 	test -z "$verbose_only" && return
 	if match_pattern_list $test_count "$verbose_only"
 	then
-		exec 4>&2 3>&2
+		exec 4>&7 3>&7
 		# Emit a delimiting blank line when going from
 		# non-verbose to verbose.  Within verbose mode the
 		# delimiter is printed by test_expect_*.  The choice
@@ -1052,7 +1052,7 @@ test_eval_ () {
 			test 1 = $trace_level_ && set +x
 			trace_level_=$(($trace_level_-1))
 		fi
-	} 2>/dev/null 4>&2
+	} 2>/dev/null 4>&7
 
 	if test "$test_eval_ret_" != 0 && want_trace
 	then
@@ -1234,6 +1234,14 @@ test_done () {
 		EOF
 	fi
 
+	if test -s "$TEST_RESULTS_BASE.stderr"
+	then
+		say_color >&5 error "FATAL: Unexpected output on stderr"
+		sed >&5 's/^/# /' "$TEST_RESULTS_BASE.stderr"
+		GIT_EXIT_OK=1
+		exit 1
+	fi
+
 	if test "$test_fixed" != 0
 	then
 		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
@@ -1982,3 +1990,9 @@ test_lazy_prereq FSMONITOR_DAEMON '
 	git version --build-options >output &&
 	grep "feature: fsmonitor--daemon" output
 '
+
+# Now that test-lib setup is done, direct our stderr to a file, as we want to
+# catch and complain if anything ends up here. We can always access the
+# original via descriptor 7.
+mkdir -p "$TEST_RESULTS_DIR"
+exec 2>"$TEST_RESULTS_BASE.stderr"


If I stick a "test_expect_foobar" into a test script, it is found:

  $ ./t0001-init.sh
  ok 1 - plain
  [...]
  ok 102 - re-init reads matching includeIf.onbranch
  FATAL: Unexpected output on stderr
  # ./t0001-init.sh: 983: test_expect_foobar: not found

And if I run scripts with "-v" or "-x", that output still goes to the
terminal and does not trigger the stderr checker. Likewise with
--verbose-log, the log gets the verbose bits.

It _almost_ passes a full "make test", but there's one lingering
problem: random bits of code which want to output to stderr and bail,
and use "echo >&2" to do so. But with our redirect, that stderr goes to
the log file. t0000-basic notices this, because it is testing the test
suite itself. And it notices that:

  ./t0000-basic.sh --run=bogus

should complain to stderr, but now doesn't (it's swallowed by our log
file). We can fix that with;

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6657d56e52..aa6415b308 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -902,13 +902,13 @@ match_test_selector_list () {
 				if expr "z${selector%%-*}" : "z[0-9]*[^0-9]" >/dev/null
 				then
 					echo "error: $operation: invalid non-numeric in range" \
-						"start: '$orig_selector'" >&2
+						"start: '$orig_selector'" >&7
 					exit 1
 				fi
 				if expr "z${selector#*-}" : "z[0-9]*[^0-9]" >/dev/null
 				then
 					echo "error: $operation: invalid non-numeric in range" \
-						"end: '$orig_selector'" >&2
+						"end: '$orig_selector'" >&7
 					exit 1
 				fi
 				;;

but the same problem doubtless exists elsewhere. We could fix that
globally like this:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6657d56e52..72d60d9dac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -762,6 +762,15 @@ die () {
 	# test script run with '--immediate' fails, or when the user hits
 	# ctrl-C, i.e. when 'test_done' is not invoked at all.
 	test_atexit_handler || code=$?
+
+	# Dump the stderr log if we saw anything, since otherwise random
+	# errors will never make it to the user when we do not have
+	# a clean exit.
+	if test -s "$TEST_RESULTS_BASE.stderr"
+	then
+		cat >&7 "$TEST_RESULTS_BASE.stderr"
+	fi
+
 	if test -n "$GIT_EXIT_OK"
 	then
 		exit $code

It's a little weird because the stderr message is delayed until we cat
it out, but it makes sense. If we are bailing immediately, then the time
between "echo" and "cat" is small. And if we are not bailing, then that
is exactly the kind of bug our stderr log is trying to catch (and we
will complain during test_done).

I am not quite sure if this is an elegant solution or a terrible hack.

-Peff