[PATCH 0/8] running unit tests inside a test-lib script

[PATCH 0/8] running unit tests inside a test-lib script

[Jeff King]

I saw a unit-test failure in CI a while back, and much to my annoyance
it was quite hard to debug, because we don't report the unit-test output
at all!

The problem is that we feed the unit-test binary to prove, which eats the
TAP output it produces. Normal tests get around this by using
--verbose-log (and then CI dumps the log). But the unit-test script
silently ignores that option, and you get no feedback at all about which
test failed.

We could fix that, obviously, but I think it's a symptom of a larger
problem: we don't do the usual setup things that test-lib.sh does.
Another example I found in the real world is that we skip some
environment setup, like LSAN_OPTIONS, leading to worse output for some
leaks. I'd guess there are other cases, as well.

So the real goal of this series is patch 5, which just lets us wrap the
unit-test binary in a regular t0001-unit-tests.sh script. Patch 4 is the
test-lib.sh tweaks needed for this.

And naturally, the whole thing turned up a few bugs, which are fixed in
patches 1-3. But it also turned up a number of cleanup possibilities.
The diffstat is very satisfying, but note the final two which I've
marked as RFC.

This would also avoid the need for the unit-tests to ignore test-lib
options, like the patches we saw recently in:

  https://lore.kernel.org/git/20261026-ks-unit-tests-ignore-long-tests-v2-0-fdbbefbd19fb@gmail.com/

I didn't clean up the existing ignored ones here, but I think it would
probably be reasonable to do so.

  [1/8]: unit-tests: report the correct unexpected argument
  [2/8]: test-lib: drop unused test_external_has_tap variable
  [3/8]: test-lib: stop inventing a JUnit testcase for empty suites
  [4/8]: test-lib: allow an external program to supply TAP output
  [5/8]: t: run clar unit tests through test-lib.sh
  [6/8]: t: drop redundant unit-test invocations
  [7/8]: test-tool: drop the run-command testsuite mode
  [8/8]: test-lib: drop the shell JUnit XML reporting code

 Makefile                    |   1 -
 ci/run-test-slice.sh        |   5 --
 t/Makefile                  |  12 +--
 t/helper/meson.build        |   1 -
 t/helper/test-run-command.c | 173 ------------------------------------
 t/helper/test-tool.c        |   1 -
 t/helper/test-tool.h        |   1 -
 t/helper/test-xml-encode.c  |  80 -----------------
 t/meson.build               |   4 +-
 t/run-test.sh               |  18 ----
 t/t0011-unit-tests.sh       |  20 +++++
 t/test-lib-junit.sh         | 132 ---------------------------
 t/test-lib.sh               |  16 ++--
 t/unit-tests/unit-test.c    |   2 +-
 14 files changed, 33 insertions(+), 433 deletions(-)
 delete mode 100644 t/helper/test-xml-encode.c
 delete mode 100755 t/run-test.sh
 create mode 100755 t/t0011-unit-tests.sh
 delete mode 100644 t/test-lib-junit.sh

[PATCH 1/8] unit-tests: report the correct unexpected argument

[Jeff King]

We use PARSE_OPT_KEEP_ARGV0 when parsing the unit-test command line, so
argv[0] still contains the program name afterwards. If we find an extra
argument, though, we report argv[0] as the offending parameter. This makes
an invocation like "unit-tests foo" complain about "unit-tests" instead
of "foo".

Report argv[1] instead, which is the first argument we didn't consume.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/unit-tests/unit-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
index 752fb38fb3..8eda87856c 100644
--- a/t/unit-tests/unit-test.c
+++ b/t/unit-tests/unit-test.c
@@ -48,7 +48,7 @@ int cmd_main(int argc, const char **argv)
 	argc = parse_options(argc, argv, NULL, options,
 			     unit_test_usage, PARSE_OPT_KEEP_ARGV0);
 	if (argc > 1)
-		usagef(_("extra command line parameter '%s'"), argv[0]);
+		usagef(_("extra command line parameter '%s'"), argv[1]);
 
 	strvec_push(&args, argv[0]);
 	strvec_push(&args, "-t");
-- 
2.56.0.559.g7c1a1d8168

[PATCH 2/8] test-lib: drop unused test_external_has_tap variable

[Jeff King]

The test_external_has_tap variable was used by test_external to remember
whether an external test produced TAP output. But we dropped
test_external in 5beca49a0b (test-lib: simplify by removing
test_external, 2022-07-28). Drop this leftover initialization.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a893963f86..3d001b91da 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -771,8 +771,6 @@ test_success=0
 
 test_missing_prereq=
 
-test_external_has_tap=0
-
 die () {
 	code=$?
 	# This is responsible for running the atexit commands even when a
-- 
2.56.0.559.g7c1a1d8168

[PATCH 3/8] test-lib: stop inventing a JUnit testcase for empty suites

[Jeff King]

If a script finishes without reporting any test cases, the JUnit output
code tries to call write_junit_xml_testcase(). But that function went
away in 78d5e4cfb4 (tests: refactor --write-junit-xml code, 2022-05-21),
so we get a shell error instead. A script which sets skip_all and calls
test_done without running any tests is enough to trigger this. For
example, on Linux:

  $ ./t0051-windows-named-pipe.sh --write-junit-xml
  ./t0051-windows-named-pipe.sh: 97: write_junit_xml_testcase: not found
  1..0 # SKIP skipping Windows-specific tests

This is mostly an annoyance, but it gets even worse with the "set -e"
detector:

  $ GIT_TEST_USE_SET_E=1 ./t0051-windows-named-pipe.sh --write-junit-xml
  ./t0051-windows-named-pipe.sh: 97: write_junit_xml_testcase: not found
  FATAL: Unexpected exit with code 127

Before the refactoring in that commit, the fallback wrote a successful
testcase named "all tests skipped". We could resurrect that, but I'm not
sure it actually makes sense. We can just leave the suite empty if we
don't have any testcases to report.

This also prepares us for scripts which delegate TAP output to an
external program. We can't report those as JUnit XML (we don't know what
was in the TAP!), but it does prevent us from inventing fake results or
complaining about the missing function.

Signed-off-by: Jeff King <peff@peff.net>
---
I do propose dropping the JUnit support entirely in the final patch, but
this is the minimum needed to unblock the rest of the series in the
meantime (in case we do want to keep it).

 t/test-lib-junit.sh | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
index f4994dd9d3..03a7360198 100644
--- a/t/test-lib-junit.sh
+++ b/t/test-lib-junit.sh
@@ -86,17 +86,11 @@ finalize_test_case_output () {
 		date getnanos $junit_start)\""
 	write_junit_xml "$(printf '%s\n' \
 		"    <testcase $junit_attrs>" "$@" "    </testcase>")"
-	junit_have_testcase=t
 }
 
 finalize_test_output () {
 	if test -n "$junit_xml_path"
 	then
-		test -n "$junit_have_testcase" || {
-			junit_start=$(test-tool date getnanos)
-			write_junit_xml_testcase "all tests skipped"
-		}
-
 		# adjust the overall time
 		junit_time=$(test-tool date getnanos $junit_suite_start)
 		sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
@@ -114,7 +108,6 @@ write_junit_xml () {
 	case "$1" in
 	--truncate)
 		>"$junit_xml_path"
-		junit_have_testcase=
 		shift
 		;;
 	esac
-- 
2.56.0.559.g7c1a1d8168

[PATCH 4/8] test-lib: allow an external program to supply TAP output

[Jeff King]

A program which produces its own TAP output can't use test-lib.sh,
because our TAP header and test counts conflict.

We used to support this with test_external, but dropped it in 5beca49a0b
(test-lib: simplify by removing test_external, 2022-07-28). That
interface let you mix shell tests with external TAP, which required
special handling for the test counts. But we can bring back a much
simpler variant: tell test-lib.sh up front that it will be a thin
wrapper for an external TAP generator, and let it stay out of the way.

Now you might ask what is the point of using test-lib.sh at all if it's
not going to generate any output. And the point is: it does other useful
setup, like handling logging, setting certain environment variables, and
so on. The next commit will wrap the clar unit-tests which will bring
some real-world benefits.

The only changes we need here are avoiding the TAP header and the plan
count. Note that if we skip the whole script via GIT_SKIP_TESTS, though,
we still have to emit a count (since we wouldn't run the external
command at all).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3d001b91da..3754db0ea7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1277,8 +1277,11 @@ test_done () {
 
 		# Maybe print SKIP message
 		test -z "$skip_all" || skip_all="# SKIP $skip_all"
-		case "$test_count" in
-		0)
+		case "$test_external,$skip_all,$test_count" in
+		t,,*)
+			: nothing
+			;;
+		*,*,0)
 			say "1..$test_count${skip_all:+ $skip_all}"
 			;;
 		*)
@@ -1534,7 +1537,7 @@ then
 	BAIL_OUT 'You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory'
 fi
 
-if test -n "$HARNESS_ACTIVE"
+if test -n "$HARNESS_ACTIVE" && test -z "$test_external"
 then
 	say "TAP version 13"
 	say "pragma +strict"
-- 
2.56.0.559.g7c1a1d8168

[PATCH 5/8] t: run clar unit tests through test-lib.sh

[Jeff King]

We run the clar unit-test binary directly, without going through
test-lib.sh. This means it misses out on the setup done by the shell
tests, and is inconsistent with the rest of the test suite.

I've hit two specific cases in practice. One is that we accept but
ignore --verbose-log. This means that if unit-tests fail in CI (which
runs the tests under prove, eating their stdout), we never get to see
the failing test output at all!

Another is that test-lib.sh sets sanitizer options like LSAN_OPTIONS.
You can see the difference by creating an intentional leak like this:

  diff --git a/t/unit-tests/u-strbuf.c b/t/unit-tests/u-strbuf.c
  index caa5d78aa3..4d252902ed 100644
  --- a/t/unit-tests/u-strbuf.c
  +++ b/t/unit-tests/u-strbuf.c
  @@ -84,6 +84,8 @@ static void t_addstr(struct strbuf *buf, const void *data)
   	size_t orig_alloc = buf->alloc;
   	size_t orig_len = buf->len;

  +	write(2, strdup("foo"), 3);
  +
   	assert_sane_strbuf(buf);
   	strbuf_addstr(buf, text);
   	assert_sane_strbuf(buf);

Without fast_unwind_on_malloc=0, you get a fairly useless report. With
test-lib.sh's settings, we get the full stack trace.

Obviously we could port these features into the unit-test code. But by
wrapping them in a regular test script we get that for free, plus it
will let us get rid of some extra complexities in our prove invocations
(in subsequent patches).

We do have to forward any relevant options to the unit-test binary. We
should do that here in t0011 and not as a central test-lib.sh feature,
because the set of handled options may differ between external programs
(if we ever even get a second one). Right now we support --immediate and
nothing else. In theory we should be able to handle --run, but the
syntax is a bit different between the two systems. This is no worse than
the existing behavior, though, since run-test.sh only passes
GIT_TEST_OPTS to shell tests, not to the unit-test binary.

Likewise we do not support --write-junit-xml; the unit-tests will run
but the results won't be present in the XML output. It would be possible
to support this with some work, but for now we are not making anything
worse (the unit-tests are currently run separately and do not even
support the option).

One note on the implementation: we have to inform meson of the
dependency. Without that, running "meson test t0011-unit-tests" would
fail (but "meson compile" first would be OK, because it builds the unit
tests). The make system doesn't have the same problem, because it just
depends on "all" before testing (the meson equivalent would be depending
on "compile", but it is more fine-grained).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/meson.build         |  3 +++
 t/t0011-unit-tests.sh | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100755 t/t0011-unit-tests.sh

diff --git a/t/meson.build b/t/meson.build
index 11b3752f00..fc628ad2d1 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -76,6 +76,8 @@ clar_unit_tests = executable('unit-tests',
 )
 test('unit-tests', clar_unit_tests, kwargs: test_kwargs)
 
+test_dependencies += clar_unit_tests
+
 subdir('helper')
 
 integration_tests = [
@@ -90,6 +92,7 @@ integration_tests = [
   't0008-ignores.sh',
   't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
+  't0011-unit-tests.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
   't0014-alias.sh',
diff --git a/t/t0011-unit-tests.sh b/t/t0011-unit-tests.sh
new file mode 100755
index 0000000000..ac3d7ccda2
--- /dev/null
+++ b/t/t0011-unit-tests.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='run clar unit tests'
+test_external=t
+. ./test-lib.sh
+
+# We store the binary in different locations for make vs meson. Yuck.
+if test -d "$GIT_BUILD_DIR/t/unit-tests"
+then
+	UNIT_TESTS="$GIT_BUILD_DIR/t/unit-tests/bin/unit-tests"
+else
+	UNIT_TESTS="$GIT_BUILD_DIR/t/unit-tests"
+fi
+
+# Pass along any options that are meaningful to the unit-test binary here.
+"$UNIT_TESTS" \
+	${immediate:+-i} \
+	|| error "unit-test binary failed"
+
+test_done
-- 
2.56.0.559.g7c1a1d8168

[PATCH 6/8] t: drop redundant unit-test invocations

[Jeff King]

Now that t0011 runs the clar unit tests, running the binary separately
from the shell tests would execute them twice. Drop the separate Meson
test and the extra invocation in the first CI test slice, along with the
unit-tests-test-tool target it used. We can also drop UNIT_TESTS_NO_DIR,
which supplied the arguments for that target.

Likewise, stop passing the unit-test binaries to prove alongside the
shell tests. Since all of its inputs are now shell scripts, we can use
TEST_SHELL_PATH directly instead of run-test.sh, and pass GIT_TEST_OPTS
through prove's "::" separator. This leaves no users of run-test.sh, so
we can drop it, too.

You can still run the unit-tests directly if you want to. I've also left
"make unit-tests" intact, though I'm not sure if it is actually useful.

Signed-off-by: Jeff King <peff@peff.net>
---
 ci/run-test-slice.sh |  5 -----
 t/Makefile           | 12 ++----------
 t/meson.build        |  1 -
 t/run-test.sh        | 18 ------------------
 4 files changed, 2 insertions(+), 34 deletions(-)
 delete mode 100755 t/run-test.sh

diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index d0063efc42..fd66443626 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -10,9 +10,4 @@ TESTS=$(cd t && ./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh)
 group "Run tests" make --quiet -C t T="$(echo "$TESTS" | tr '\n' ' ')" ||
 handle_failed_tests
 
-# We only have one unit test at the moment, so run it in the first slice.
-if [ "$1" == "1" ] ; then
-	group "Run unit tests" make --quiet -C t unit-tests-test-tool
-fi
-
 check_unignored_build_artifacts
diff --git a/t/Makefile b/t/Makefile
index e3249879c4..1d2eca9ae4 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -54,7 +54,6 @@ UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X)
 UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
-UNIT_TESTS_NO_DIR = $(notdir $(UNIT_TESTS))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual
@@ -79,7 +78,7 @@ failed:
 
 prove: pre-clean $(TEST_LINT)
 	@echo "*** prove (shell & unit tests) ***"
-	@$(CHAINLINTSUPPRESS) TEST_OPTIONS='$(GIT_TEST_OPTS)' TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS)
+	@$(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
@@ -88,21 +87,14 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
+.PHONY: unit-tests unit-tests-raw unit-tests-prove
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)
 
 unit-tests-prove:
 	@echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
 
-unit-tests-test-tool:
-	@echo "*** test-tool - unit tests **"
-	( \
-		cd unit-tests/bin && \
-		../../helper/test-tool$X run-command testsuite $(UNIT_TESTS_NO_DIR)\
-	)
-
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
 
diff --git a/t/meson.build b/t/meson.build
index fc628ad2d1..4dc47824f5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -74,7 +74,6 @@ clar_unit_tests = executable('unit-tests',
   ),
   dependencies: [libgit_commonmain],
 )
-test('unit-tests', clar_unit_tests, kwargs: test_kwargs)
 
 test_dependencies += clar_unit_tests
 
diff --git a/t/run-test.sh b/t/run-test.sh
deleted file mode 100755
index 63328ac630..0000000000
--- a/t/run-test.sh
+++ /dev/null
@@ -1,18 +0,0 @@
-#!/bin/sh
-
-# A simple wrapper to run shell tests via TEST_SHELL_PATH,
-# or exec unit tests directly.
-
-case "$1" in
-*.sh)
-	if test -z "${TEST_SHELL_PATH}"
-	then
-		echo >&2 "ERROR: TEST_SHELL_PATH is empty or not set"
-		exit 1
-	fi
-	exec "${TEST_SHELL_PATH}" "$@" ${TEST_OPTIONS}
-	;;
-*)
-	exec "$@"
-	;;
-esac
-- 
2.56.0.559.g7c1a1d8168

[RFC/PATCH 7/8] test-tool: drop the run-command testsuite mode

[Jeff King]

The last commit removed the final in-tree caller of "test-tool
run-command testsuite"; it was used only for unit tests, which now
run through the normal suite.

One of the original uses for it was supporting Azure pipelines, but we
dropped that support in e39e332e50 (ci: remove stale code for Azure
Pipelines, 2025-01-10). So in theory it is not used anywhere anymore.

However, commit be5d88e112 (test-tool run-command: learn to run (parts
of) the testsuite, 2019-10-06) indicates that it may also be useful for
Visual Studio users to run the tests without make or prove. It's unclear
if that is still a useful mode or not (if it is, we should drop or
revert this patch).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-run-command.c | 173 ------------------------------------
 1 file changed, 173 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 4a56456894..137bdc9d54 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -15,9 +15,6 @@
 #include "strvec.h"
 #include "strbuf.h"
 #include "parse-options.h"
-#include "string-list.h"
-#include "thread-utils.h"
-#include "wildmatch.h"
 
 static int number_callbacks;
 static int parallel_next(struct child_process *cp,
@@ -103,174 +100,6 @@ static int test_stdin_pipe_feed(int hook_stdin_fd, void *cb UNUSED, void *task_c
 	return !(*lines_remaining);
 }
 
-struct testsuite {
-	struct string_list tests, failed;
-	int next;
-	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
-	const char *shell_path;
-};
-#define TESTSUITE_INIT { \
-	.tests = STRING_LIST_INIT_DUP, \
-	.failed = STRING_LIST_INIT_DUP, \
-}
-
-static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
-		     void **task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *test;
-	if (suite->next >= suite->tests.nr)
-		return 0;
-
-	test = suite->tests.items[suite->next++].string;
-	if (suite->shell_path)
-		strvec_push(&cp->args, suite->shell_path);
-	strvec_push(&cp->args, test);
-	if (suite->quiet)
-		strvec_push(&cp->args, "--quiet");
-	if (suite->immediate)
-		strvec_push(&cp->args, "-i");
-	if (suite->verbose)
-		strvec_push(&cp->args, "-v");
-	if (suite->verbose_log)
-		strvec_push(&cp->args, "-V");
-	if (suite->trace)
-		strvec_push(&cp->args, "-x");
-	if (suite->write_junit_xml)
-		strvec_push(&cp->args, "--write-junit-xml");
-
-	strbuf_addf(err, "Output of '%s':\n", test);
-	*task_cb = (void *)test;
-
-	return 1;
-}
-
-static int test_finished(int result, struct strbuf *err, void *cb,
-			 void *task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *name = (const char *)task_cb;
-
-	if (result)
-		string_list_append(&suite->failed, name);
-
-	strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
-
-	return 0;
-}
-
-static int test_failed(struct strbuf *out, void *cb, void *task_cb)
-{
-	struct testsuite *suite = cb;
-	const char *name = (const char *)task_cb;
-
-	string_list_append(&suite->failed, name);
-	strbuf_addf(out, "FAILED TO START: '%s'\n", name);
-
-	return 0;
-}
-
-static const char * const testsuite_usage[] = {
-	"test-run-command testsuite [<options>] [<pattern>...]",
-	NULL
-};
-
-static int testsuite(int argc, const char **argv)
-{
-	struct testsuite suite = TESTSUITE_INIT;
-	int max_jobs = 1, i, ret = 0;
-	DIR *dir;
-	struct dirent *d;
-	struct option options[] = {
-		OPT_BOOL('i', "immediate", &suite.immediate,
-			 "stop at first failed test case(s)"),
-		OPT_INTEGER('j', "jobs", &max_jobs, "run <N> jobs in parallel"),
-		OPT_BOOL('q', "quiet", &suite.quiet, "be terse"),
-		OPT_BOOL('v', "verbose", &suite.verbose, "be verbose"),
-		OPT_BOOL('V', "verbose-log", &suite.verbose_log,
-			 "be verbose, redirected to a file"),
-		OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
-		OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
-			 "write JUnit-style XML files"),
-		OPT_END()
-	};
-	struct run_process_parallel_opts opts = {
-		.get_next_task = next_test,
-		.start_failure = test_failed,
-		.feed_pipe = test_stdin_pipe_feed,
-		.task_finished = test_finished,
-		.data = &suite,
-	};
-	struct strbuf progpath = STRBUF_INIT;
-	size_t path_prefix_len;
-
-	argc = parse_options(argc, argv, NULL, options,
-			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
-
-	if (max_jobs <= 0)
-		max_jobs = online_cpus();
-
-	/*
-	 * If we run without a shell, execute the programs directly from CWD.
-	 */
-	suite.shell_path = getenv("TEST_SHELL_PATH");
-	if (!suite.shell_path)
-		strbuf_addstr(&progpath, "./");
-	path_prefix_len = progpath.len;
-
-	dir = opendir(".");
-	if (!dir)
-		die("Could not open the current directory");
-	while ((d = readdir(dir))) {
-		const char *p = d->d_name;
-
-		if (!strcmp(p, ".") || !strcmp(p, ".."))
-			continue;
-
-		/* No pattern: match all */
-		if (!argc) {
-			strbuf_setlen(&progpath, path_prefix_len);
-			strbuf_addstr(&progpath, p);
-			string_list_append(&suite.tests, progpath.buf);
-			continue;
-		}
-
-		for (i = 0; i < argc; i++)
-			if (!wildmatch(argv[i], p, 0)) {
-				strbuf_setlen(&progpath, path_prefix_len);
-				strbuf_addstr(&progpath, p);
-				string_list_append(&suite.tests, progpath.buf);
-				break;
-			}
-	}
-	closedir(dir);
-
-	if (!suite.tests.nr)
-		die("No tests match!");
-	if (max_jobs > suite.tests.nr)
-		max_jobs = suite.tests.nr;
-
-	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
-		(uintmax_t)suite.tests.nr, max_jobs);
-
-	opts.processes = max_jobs;
-	run_processes_parallel(&opts);
-
-	if (suite.failed.nr > 0) {
-		ret = 1;
-		fprintf(stderr, "%"PRIuMAX" tests failed:\n\n",
-			(uintmax_t)suite.failed.nr);
-		for (i = 0; i < suite.failed.nr; i++)
-			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
-	}
-
-	string_list_clear(&suite.tests, 0);
-	string_list_clear(&suite.failed, 0);
-	strbuf_release(&progpath);
-
-	return ret;
-}
-
 static uint64_t my_random_next = 1234;
 
 static uint64_t my_random(void)
@@ -448,8 +277,6 @@ int cmd__run_command(int argc, const char **argv)
 		.data = &proc,
 	};
 
-	if (argc > 1 && !strcmp(argv[1], "testsuite"))
-		return testsuite(argc - 1, argv + 1);
 	if (!strcmp(argv[1], "inherited-handle"))
 		return inherit_handle(argv[0]);
 	if (!strcmp(argv[1], "inherited-handle-child"))
-- 
2.56.0.559.g7c1a1d8168

[RFC/PATCH 8/8] test-lib: drop the shell JUnit XML reporting code

[Jeff King]

Our shell tests can produce JUnit XML with --write-junit-xml, but we
don't have any in-tree callers. The Azure Pipelines consumer went away,
with the remaining CI references removed in e39e332e50 (ci: remove stale
code for Azure Pipelines, 2025-01-10).

GitLab CI does still consume JUnit XML, but that's generated by Meson in
meson-logs/testlog.junit.xml, not by us directly.

It's possible that somebody is invoking the tests with --write-junit-xml
themselves, but we have no easy way of knowing. If so then we'd need to
drop/revert this patch. I tend to doubt it somewhat, though. We fixed a
bug earlier in the series that would cause several tests to fail when
run with "set -e", and we enable that feature automatically when the
test suite is run under recent versions of bash. So there's a reasonable
chance anybody using it would have been seeing test failures since
ffe8005b9d (t: detect errors outside of test cases, 2026-04-21).

Getting rid of the option lets us also drop the test-lib-junit.sh file
and the test-tool xml encoder. Note that the start_test_case_output and
finalize_test_output hooks in test-lib.sh aren't used by anybody, but
I've left them for now. In theory they're generally useful, and they
don't cost very much to support. The other hooks are still used for
GitHub workflow annotations.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile                   |   1 -
 t/helper/meson.build       |   1 -
 t/helper/test-tool.c       |   1 -
 t/helper/test-tool.h       |   1 -
 t/helper/test-xml-encode.c |  80 ------------------------
 t/test-lib-junit.sh        | 125 -------------------------------------
 t/test-lib.sh              |   5 +-
 7 files changed, 1 insertion(+), 213 deletions(-)
 delete mode 100644 t/helper/test-xml-encode.c
 delete mode 100644 t/test-lib-junit.sh

diff --git a/Makefile b/Makefile
index a96be506b5..20ccdb7fa7 100644
--- a/Makefile
+++ b/Makefile
@@ -879,7 +879,6 @@ TEST_BUILTINS_OBJS += test-userdiff.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
 TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
-TEST_BUILTINS_OBJS += test-xml-encode.o
 TEST_BUILTINS_OBJS += test-zlib.o
 
 # Do not add more tests here unless they have extra dependencies. Add
diff --git a/t/helper/meson.build b/t/helper/meson.build
index ae513b4cdc..3dcc4d5219 100644
--- a/t/helper/meson.build
+++ b/t/helper/meson.build
@@ -77,7 +77,6 @@ test_tool_sources = [
   'test-wildmatch.c',
   'test-windows-named-pipe.c',
   'test-write-cache.c',
-  'test-xml-encode.c',
   'test-zlib.c',
 ]
 
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b71a22b43b..b1be64da06 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -86,7 +86,6 @@ static struct test_cmd cmds[] = {
 	{ "trace2", cmd__trace2 },
 	{ "truncate", cmd__truncate },
 	{ "userdiff", cmd__userdiff },
-	{ "xml-encode", cmd__xml_encode },
 	{ "wildmatch", cmd__wildmatch },
 #ifdef GIT_WINDOWS_NATIVE
 	{ "windows-named-pipe", cmd__windows_named_pipe },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index f2885b33d5..17b72276a1 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -79,7 +79,6 @@ int cmd__synthesize(int argc, const char **argv);
 int cmd__trace2(int argc, const char **argv);
 int cmd__truncate(int argc, const char **argv);
 int cmd__userdiff(int argc, const char **argv);
-int cmd__xml_encode(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
 #ifdef GIT_WINDOWS_NATIVE
 int cmd__windows_named_pipe(int argc, const char **argv);
diff --git a/t/helper/test-xml-encode.c b/t/helper/test-xml-encode.c
deleted file mode 100644
index b2f330d1a4..0000000000
--- a/t/helper/test-xml-encode.c
+++ /dev/null
@@ -1,80 +0,0 @@
-#include "test-tool.h"
-
-static const char *utf8_replace_character = "&#xfffd;";
-
-/*
- * Encodes (possibly incorrect) UTF-8 on <stdin> to <stdout>, to be embedded
- * in an XML file.
- */
-int cmd__xml_encode(int argc UNUSED, const char **argv UNUSED)
-{
-	unsigned char buf[1024], tmp[4], *tmp2 = NULL;
-	ssize_t cur = 0, len = 1, remaining = 0;
-	unsigned char ch;
-
-	for (;;) {
-		if (++cur == len) {
-			len = xread(0, buf, sizeof(buf));
-			if (!len)
-				return 0;
-			if (len < 0)
-				die_errno("Could not read <stdin>");
-			cur = 0;
-		}
-		ch = buf[cur];
-
-		if (tmp2) {
-			if ((ch & 0xc0) != 0x80) {
-				fputs(utf8_replace_character, stdout);
-				tmp2 = NULL;
-				cur--;
-				continue;
-			}
-			*tmp2 = ch;
-			tmp2++;
-			if (--remaining == 0) {
-				fwrite(tmp, tmp2 - tmp, 1, stdout);
-				tmp2 = NULL;
-			}
-			continue;
-		}
-
-		if (!(ch & 0x80)) {
-			/* 0xxxxxxx */
-			if (ch == '&')
-				fputs("&amp;", stdout);
-			else if (ch == '\'')
-				fputs("&apos;", stdout);
-			else if (ch == '"')
-				fputs("&quot;", stdout);
-			else if (ch == '<')
-				fputs("&lt;", stdout);
-			else if (ch == '>')
-				fputs("&gt;", stdout);
-			else if (ch >= 0x20)
-				fputc(ch, stdout);
-			else if (ch == 0x09 || ch == 0x0a || ch == 0x0d)
-				fprintf(stdout, "&#x%02x;", ch);
-			else
-				fputs(utf8_replace_character, stdout);
-		} else if ((ch & 0xe0) == 0xc0) {
-			/* 110XXXXx 10xxxxxx */
-			tmp[0] = ch;
-			remaining = 1;
-			tmp2 = tmp + 1;
-		} else if ((ch & 0xf0) == 0xe0) {
-			/* 1110XXXX 10Xxxxxx 10xxxxxx */
-			tmp[0] = ch;
-			remaining = 2;
-			tmp2 = tmp + 1;
-		} else if ((ch & 0xf8) == 0xf0) {
-			/* 11110XXX 10XXxxxx 10xxxxxx 10xxxxxx */
-			tmp[0] = ch;
-			remaining = 3;
-			tmp2 = tmp + 1;
-		} else
-			fputs(utf8_replace_character, stdout);
-	}
-
-	return 0;
-}
diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
deleted file mode 100644
index 03a7360198..0000000000
--- a/t/test-lib-junit.sh
+++ /dev/null
@@ -1,125 +0,0 @@
-# Library of functions to format test scripts' output in JUnit XML
-# format, to support Git's test suite result to be presented in an
-# easily digestible way on Azure Pipelines.
-#
-# Copyright (c) 2022 Johannes Schindelin
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see https://www.gnu.org/licenses/ .
-#
-# The idea is for `test-lib.sh` to source this file when the user asks
-# for JUnit XML; these functions will then override (empty) functions
-# that are called at the appropriate times during the test runs.
-
-start_test_output () {
-	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
-	mkdir -p "$junit_xml_dir"
-	junit_xml_base=${1##*/}
-	junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
-	junit_attrs="name=\"${junit_xml_base%.sh}\""
-	junit_attrs="$junit_attrs timestamp=\"$(TZ=UTC \
-		date +%Y-%m-%dT%H:%M:%S)\""
-	write_junit_xml --truncate "<testsuites>" "  <testsuite $junit_attrs>"
-	junit_suite_start=$(test-tool date getnanos)
-	if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
-	then
-		GIT_TEST_TEE_OFFSET=0
-	fi
-}
-
-start_test_case_output () {
-	junit_start=$(test-tool date getnanos)
-}
-
-finalize_test_case_output () {
-	test_case_result=$1
-	shift
-	case "$test_case_result" in
-	ok)
-		set -- "$*"
-		;;
-	failure)
-		junit_insert="<failure message=\"not ok $test_count -"
-		junit_insert="$junit_insert $(xml_attr_encode --no-lf "$1")\">"
-		junit_insert="$junit_insert $(xml_attr_encode \
-			"$(if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
-			   then
-				test-tool path-utils skip-n-bytes \
-					"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
-			   else
-				printf '%s\n' "$@" | sed 1d
-			   fi)")"
-		junit_insert="$junit_insert</failure>"
-		if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
-		then
-			junit_insert="$junit_insert<system-err>$(xml_attr_encode \
-				"$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>"
-		fi
-		set -- "$1" "      $junit_insert"
-		;;
-	fixed)
-		set -- "$* (breakage fixed)"
-		;;
-	broken)
-		set -- "$* (known breakage)"
-		;;
-	skip)
-		message="$(xml_attr_encode --no-lf "$skipped_reason")"
-		set -- "$1" "      <skipped message=\"$message\" />"
-		;;
-	esac
-
-	junit_attrs="name=\"$(xml_attr_encode --no-lf "$this_test.$test_count $1")\""
-	shift
-	junit_attrs="$junit_attrs classname=\"$this_test\""
-	junit_attrs="$junit_attrs time=\"$(test-tool \
-		date getnanos $junit_start)\""
-	write_junit_xml "$(printf '%s\n' \
-		"    <testcase $junit_attrs>" "$@" "    </testcase>")"
-}
-
-finalize_test_output () {
-	if test -n "$junit_xml_path"
-	then
-		# adjust the overall time
-		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
-			-e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
-			-e '/^ *<\/testsuite/d' \
-			<"$junit_xml_path" >"$junit_xml_path.new"
-		mv "$junit_xml_path.new" "$junit_xml_path"
-
-		write_junit_xml "  </testsuite>" "</testsuites>"
-		write_junit_xml=
-	fi
-}
-
-write_junit_xml () {
-	case "$1" in
-	--truncate)
-		>"$junit_xml_path"
-		shift
-		;;
-	esac
-	printf '%s\n' "$@" >>"$junit_xml_path"
-}
-
-xml_attr_encode () {
-	if test "x$1" = "x--no-lf"
-	then
-		shift
-		printf '%s' "$*" | test-tool xml-encode
-	else
-		printf '%s\n' "$@" | test-tool xml-encode
-	fi
-}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3754db0ea7..c107fd687a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -195,7 +195,7 @@ mark_option_requires_arg () {
 	store_arg_to=$2
 }
 
-# These functions can be overridden e.g. to output JUnit XML
+# These functions can be overridden e.g. to add CI workflow annotations
 start_test_output () { :; }
 start_test_case_output () { :; }
 finalize_test_case_output () { :; }
@@ -260,9 +260,6 @@ parse_option () {
 		verbose_log=t
 		tee=t
 		;;
-	--write-junit-xml)
-		. "$TEST_DIRECTORY/test-lib-junit.sh"
-		;;
 	--github-workflow-markup)
 		. "$TEST_DIRECTORY/test-lib-github-workflow-markup.sh"
 		;;
-- 
2.56.0.559.g7c1a1d8168