test-tool: add unit test suite runner

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small
cleanups: we move t-basic out of the unit-tests directory, and add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process.

Some known remaining bits of work:
* We need to filter out cmake *.pdb files when running with test-tool.
* We should investigate switching the Windows CI to use test-tool instead of prove.
* We should determine whether it is confusing or otherwise harmful to people's workflow to have the unit tests run in parallel with shell tests when using prove as the default test target.

[1] https://lore.kernel.org/git/[email protected]/

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (3):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool

 Makefile                          | 19 +++++++++++----
 t/Makefile                        | 13 +++++++---
 t/helper/test-run-command.c       | 40 ++++++++++++++++++++++++-------
 t/run-test.sh                     | 13 ++++++++++
 t/t0080-unit-test-output.sh       | 24 +++++++++----------
 t/t0080/.gitignore                |  1 +
 t/{unit-tests => t0080}/t-basic.c |  2 +-
 7 files changed, 83 insertions(+), 29 deletions(-)
 create mode 100755 t/run-test.sh
 create mode 100644 t/t0080/.gitignore
 rename t/{unit-tests => t0080}/t-basic.c (98%)

base-commit: a26002b62827b89a19b1084bd75d9371d565d03c

2.43.0.381.gb435a96ce8-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/t0080/t-basic.c and adjust Makefiles and .gitignores as necessary.

This has the additional benefit that test harnesses seeking to run all unit tests can find them with a simple glob of "t/unit-tests/bin/t-*", with no exceptions needed. This will be important in a later patch where we add support for running the unit tests via a test-tool subcommand.

---
 Makefile                          | 17 +++++++++++++----
 t/Makefile                        |  2 +-
 t/t0080-unit-test-output.sh       | 24 ++++++++++++------------
 t/t0080/.gitignore                |  1 +
 t/{unit-tests => t0080}/t-basic.c |  2 +-
 5 files changed, 28 insertions(+), 18 deletions(-)
 create mode 100644 t/t0080/.gitignore
 rename t/{unit-tests => t0080}/t-basic.c (98%)

index 88ba7a3c51..ab32ec1101 100644
@@ -683,6 +683,7 @@ TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 UNIT_TEST_PROGRAMS =
+UNIT_TEST_HELPERS =
 UNIT_TEST_DIR = t/unit-tests
 UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
 

@@ -1339,10 +1340,12 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_HELPERS += t/t0080/t-basic
+UNIT_TEST_HELPER_PROGS = $(patsubst %,%$X,$(UNIT_TEST_HELPERS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_OBJS += $(patsubst %,%.o,$(UNIT_TEST_HELPERS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a

@@ -3189,7 +3192,9 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
+all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+
+all:: $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)

@@ -3620,7 +3625,7 @@ endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
-		$(UNIT_TEST_PROGS) $(MOFILES)
+		$(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS) $(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 	test -n "$(ARTIFACTS_DIRECTORY)"

@@ -3682,7 +3687,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) headless-git.o
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
-	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
+	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)

@@ -3869,6 +3874,10 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
+$(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
 unit-tests: $(UNIT_TEST_PROGS)

index b7a6fefe28..0bee7bc6ea 100644
@@ -42,7 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.pdb,$(wildcard unit-tests/bin/t-*)))
 
 # `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

index 961b54b06c..7431023d97 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/t0080/t-basic.c:76
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/t0080/t-basic.c:25
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/t0080/t-basic.c:30
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/t0080/t-basic.c:32
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/t0080/t-basic.c:48
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/t0080/t-basic.c:56
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/t0080/t-basic.c:61
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/t0080/t-basic.c:62
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/t0080/t-basic.c:63
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/t0080/t-basic.c:64
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/t0080/t-basic.c:91
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! "$GIT_BUILD_DIR"/t/t0080/t-basic >actual &&
 	test_cmp expect actual
 '
 

new file mode 100644
index 0000000000..1903542827
@@ -0,0 +1 @@
+/t-basic

similarity index 98%
rename from t/unit-tests/t-basic.c
rename to t/t0080/t-basic.c
index fda1ae59a6..83727221b1 100644
@@ -1,4 +1,4 @@
-#include "test-lib.h"
+#include "../unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

2.43.0.381.gb435a96ce8-goog

Up to this part was very understandable and agreeable, but I was surprised to see ...

... this conclusion. I somehow thought that t-basic part would be a good test-tool subcommand, as it is run from the suite of shell scripts for end-to-end testing CLI interaction.

Do we have any precedent to place programs placed under t/tXXXX/ and get them compiled?

And this motivation is very much understandable, and for that, as long as it is outside t/unit-tests/, it would be good. I just did not expect it to hang under t/t0080/, inviting more unit test pieces that are triggerable from the numberd shell scripts to hang under random t/t[0-9][0-9][0-9][0-9]/ directories.

Heh, I was about to ask the same thing.

In particular...

...no, I don't think we do. And quite often I exclude those directories when grepping around the code base, because there is often code there that is purely used as a data fixture. E.g., t4256 contains a copy of mailinfo.c which it uses as input for some of the tests. That code also happens to have out-of-bounds memory reads which we have since fixed in the real mailinfo.c, but of course "grep" finds them both. :)

So I would prefer a rule like "no buildable code in t/t[0-9]*". Barring that, maybe we could avoid using things that look too much like real Git code in our tests (though we sometimes do need fake code for things like funclines, and even that might end up creating false positives).

-Peff

Ack. I've moved this to a test-tool subcommand for V2, which I hope to send out soon.

Teach the testsuite runner in test-tool run-command testsuite how to run unit tests, by adding two new flags:

First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test binaries directly, rather than trying to interpret them as shell scripts.

Second "--(no-)require-shell-test-pattern" bypasses the check that the test filenames match the expected t####-*.sh pattern.

With these changes, you can now use test-tool to run the unit tests:

$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite --no-run-in-shell \
    --no-require-shell-test-pattern

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 40 +++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

index c0ed8722c8..2db7e1ef03 100644
@@ -64,11 +64,12 @@ static int task_finished(int result UNUSED,
 struct testsuite {
 	struct string_list tests, failed;
 	int next;
-	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml, run_in_shell;
 };
 #define TESTSUITE_INIT { \
 	.tests = STRING_LIST_INIT_DUP, \
 	.failed = STRING_LIST_INIT_DUP, \
+	.run_in_shell = 1, \
 }
 
 static int next_test(struct child_process *cp, struct strbuf *err, void *cb,

@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		return 0;
 
 	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
+	if (suite->run_in_shell)
+		strvec_push(&cp->args, "sh");
+	strvec_push(&cp->args, test);
 	if (suite->quiet)
 		strvec_push(&cp->args, "--quiet");
 	if (suite->immediate)

@@ -133,7 +136,7 @@ static const char * const testsuite_usage[] = {
 static int testsuite(int argc, const char **argv)
 {
 	struct testsuite suite = TESTSUITE_INIT;
-	int max_jobs = 1, i, ret = 0;
+	int max_jobs = 1, i, ret = 0, require_shell_test_pattern = 1;
 	DIR *dir;
 	struct dirent *d;
 	struct option options[] = {

@@ -147,6 +150,10 @@ static int testsuite(int argc, const char **argv)
 		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_BOOL(0, "run-in-shell", &suite.run_in_shell,
+			 "run programs in the suite via `sh`"),
+		OPT_BOOL(0, "require-shell-test-pattern", &require_shell_test_pattern,
+			 "require programs to match 't####-*.sh'"),
 		OPT_END()
 	};
 	struct run_process_parallel_opts opts = {

@@ -155,12 +162,21 @@ static int testsuite(int argc, const char **argv)
 		.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, we have to provide the relative path to
+	 * the executables.
+	 */
+	if (!suite.run_in_shell)
+		strbuf_addstr(&progpath, "./");
+	path_prefix_len = progpath.len;
 
 	dir = opendir(".");
 	if (!dir)

@@ -168,20 +184,27 @@ static int testsuite(int argc, const char **argv)
 	while ((d = readdir(dir))) {
 		const char *p = d->d_name;
 
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
+		if (!strcmp(p, ".") || !strcmp(p, ".."))
 			continue;
+		if (require_shell_test_pattern)
+			if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
+			    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
+			    !ends_with(p, ".sh"))
+				continue;
 
 		/* No pattern: match all */
 		if (!argc) {
-			string_list_append(&suite.tests, p);
+			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)) {
-				string_list_append(&suite.tests, p);
+				strbuf_setlen(&progpath, path_prefix_len);
+				strbuf_addstr(&progpath, p);
+				string_list_append(&suite.tests, progpath.buf);
 				break;
 			}
 	}

@@ -208,6 +231,7 @@ static int testsuite(int argc, const char **argv)
 
 	string_list_clear(&suite.tests, 0);
 	string_list_clear(&suite.failed, 0);
+	strbuf_release(&progpath);
 
 	return ret;
 }

2.43.0.381.gb435a96ce8-goog

Makes perfect sense.

This one I am not so sure. Do we still have situations where erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is problematic? IOW, do we need to keep it as conditional?

	... goes and looks ...

Ah, this variable/option is misnamed and that is what invited the above nonsense question out of me. The logic this option disables does not "require" (and error out if the requirement is not met); it is used in a loop over "ls *" and "filtering" files out that are not the numbered scripts.

But that confusion makes me wonder if non-script side of tests would also want some filtering in the longer run, even if the directory we feed to "test-tool run" happens to contain nothing but what we want to run right now. I wonder if we instead want a variable that holds a pattern used to match programs readdir() discovers and skip those that do not match the pattern? Its default value may be something like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give, say, "*" to pass everything, or something like that.

This makes me wonder why we want to do the readdir() loop ourselves. Instead of saying --no-require-shell-test-pattern there, wouldn't it be simpler to say "*" right there, and have testsuite() run the test programs named from the command line?

But that is orthogonal to the enhancement we have here.

This may be a stupid question, but do we even need the current "push 'sh' to the strvec"? If our executable shell scripts run just fine without, then this may not have to be conditional.

It is necessary for the same reason that the indirection provided by patch 4 is necessary: the test scripts are supposed to be run by TEST_SHELL_PATH, even if they may say "#!/bin/sh" in their header.

But the "testsuite" helper just hard-codes "sh", which is wrong. It's somewhat academic for Windows where "sh" is already bash, and that's the only reasonable thing anybody would set TEST_SHELL_PATH to anyway. But if the point is to be a drop-in replacement for the existing flow, it should be checking getenv("TEST_SHELL_PATH"). And perhaps the Makefile might need to correctly export the variable.

-Peff

It's speculation on my part, but I wonder if it has something to do with the number of shell tests? Google tells me that on Windows, the maximum command line length is 8191 characters. Which is actually a fair bit smaller than expanding the shell test list:

$ echo t????-*.sh | wc -c
25714

OK.

The original implementation and this series both still allow passing additional patterns on the command line. Only tests matching the hardcoded t[0-9][0-9][0-9][0-9]-*.sh pattern plus all the command-line patterns will be run. The new flag just bypasses the hardcoded pattern.

Given that V2 will already be changing in non-backwards-compatible ways, I think it makes the most sense to just delete the hardcoded pattern and require callers to provide it if they want it.

Actually, I misspoke here. The test must match the hardcoded pattern plus at least one of the command-line patterns (if any were provided).

Given that it sounds like the testsuite functionality is not really used at the moment, I still feel OK with changing the pattern matching logic here for V2.

In the previous commit, we added support in test-tool for running collections of unit tests. Now, add rules in t/Makefile for running in this way.

This new rule can be executed from the top-level Makefile via make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests, or by setting DEFAULT_UNIT_TEST_TARGET in config.mak.

NEEDS WORK: we need to exclude .pdb files generated by cmake [see 0df903d402 (unit-tests: do not mistake .pdb files for being executable, 2023-09-25)]

---
 Makefile   | 2 +-
 t/Makefile | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

index ab32ec1101..ce43ad2ae8 100644
@@ -3880,5 +3880,5 @@ $(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-L
 
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests

index 0bee7bc6ea..ad57ec0a41 100644
@@ -70,7 +70,7 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)

@@ -78,6 +78,13 @@ 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 run-command testsuite --no-run-in-shell --no-require-shell-test-pattern \
+	)
+
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
 

2.43.0.381.gb435a96ce8-goog

From: Jeff King <[email protected]>

Add a wrapper script to allow prove to run both shell tests and unit tests from a single invocation. This avoids issues around running prove twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so that errors can be spotted more quickly.

NEEDS WORK: as discussed in previous commits in this series, there's a desire to avoid prove specifically and (IIUC) unnecessary fork()/exec()ing in general on Windows. This change adds an extra exec() for each shell and unit test execution, will that be a problem for Windows?

[1] https://lore.kernel.org/git/[email protected]/

---
 t/Makefile    |  2 +-
 t/run-test.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100755 t/run-test.sh

index ad57ec0a41..0038a25e33 100644
@@ -61,7 +61,7 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):

new file mode 100755
index 0000000000..c29fef48dc
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+	exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac

2.43.0.381.gb435a96ce8-goog

;-)

I should have read this before I commented on that "--require is misnamed---what you really want is a custom filter" thing.

OK.

It is probably confusing especially given that the testsuite() thing does its own parallel spawning of tests while prove does its own. Is there a reason to choose unit-tests-test-tool Makefile target on prove-capable platforms (other than for testing the test-tool)?

Thanks for an entertaining read.

Please note: this series has been rebased onto jk/unit-tests-buildfix.

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in test-tool ... testsuite
* don't hardcode a test name filter in test-tool ... testsuite
* add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process

Some known remaining bits of work:
* We should investigate switching the Windows CI to use test-tool instead of prove. However, Windows CI seems broken on jk/unit-tests-buildfix, and I haven't had time to determine why.
* We should determine whether it is confusing or otherwise harmful to people's workflow to have the unit tests run in parallel with shell tests when using prove as the default test target.

[1] https://lore.kernel.org/git/[email protected]/

Changes in V2:
* Patch 1: move t-basic to a test-tool subcommand rather than a new executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in test-tool run-command testsuite
* New patch 3: remove the hardcoded filename filter in test-tool run-command testsuite
* Patch 4 (previously 2): simplified now that we no longer need to add any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using the unit test list built in the makefile in jk/unit-tests-buildfix.

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (5):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: get shell from env test-tool run-command testsuite: remove hardcoded filter test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool

 Makefile                                      |  6 ++--
 t/Makefile                                    | 15 +++++++---
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-run-command.c                   | 29 +++++++++++++++----
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/run-test.sh                                 | 13 +++++++++
 t/t0080-unit-test-output.sh                   | 24 +++++++--------
 8 files changed, 67 insertions(+), 27 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
 create mode 100755 t/run-test.sh

Range-diff against v1:

1:  a9f67ed703 < -:  ---------- t0080: turn t-basic unit test into a helper
-:  ---------- > 1:  da756b4bfb t0080: turn t-basic unit test into a helper
-:  ---------- > 2:  c8448406d7 test-tool run-command testsuite: get shell from env
-:  ---------- > 3:  e1b89ae93e test-tool run-command testsuite: remove hardcoded filter
2:  5ecbc976e6 ! 4:  b5665386b5 test-tool run-command testsuite: support unit tests

@@ Commit message
test-tool run-command testsuite: support unit tests

Teach the testsuite runner in test-tool run-command testsuite how to
- run unit tests, by adding two new flags:
+ run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
+ the programs directly from CWD, rather than defaulting to "sh" as an
+ interpreter.

- First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test
- binaries directly, rather than trying to interpret them as shell
- scripts.
-
- Second "--(no-)require-shell-test-pattern" bypasses the check that the
- test filenames match the expected t####-*.sh pattern.
-
- With these changes, you can now use test-tool to run the unit tests:
+ With this change, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
- $ ../../helper/test-tool run-command testsuite --no-run-in-shell \
- --no-require-shell-test-pattern
+ $ ../../helper/test-tool run-command testsuite

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

+ This again breaks backwards compatibility, as it is now required to set
+ TEST_SHELL_PATH properly for executing shell scripts, but again, as
+ noted in [2], there are no longer any such invocations in our codebase.
+
[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

## t/helper/test-run-command.c ##
-@@ t/helper/test-run-command.c: static int task_finished(int result UNUSED,
- struct testsuite {
- struct string_list tests, failed;
- int next;
-- int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
-+ int quiet, immediate, verbose, verbose_log, trace, write_junit_xml, run_in_shell;
- };
- #define TESTSUITE_INIT { \
- .tests = STRING_LIST_INIT_DUP, \
- .failed = STRING_LIST_INIT_DUP, \
-+ .run_in_shell = 1, \
- }
-
- static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
-@@ t/helper/test-run-command.c: static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
- return 0;
-
- test = suite->tests.items[suite->next++].string;
-- strvec_pushl(&cp->args, "sh", test, NULL);
-+ if (suite->run_in_shell)
-+ strvec_push(&cp->args, "sh");
-+ strvec_push(&cp->args, test);
- if (suite->quiet)
- strvec_push(&cp->args, "--quiet");
- if (suite->immediate)
-@@ t/helper/test-run-command.c: static const char * const testsuite_usage[] = {
- static int testsuite(int argc, const char **argv)
- {
- struct testsuite suite = TESTSUITE_INIT;
-- int max_jobs = 1, i, ret = 0;
-+ int max_jobs = 1, i, ret = 0, require_shell_test_pattern = 1;
- DIR *dir;
- struct dirent *d;
- struct option options[] = {
-@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
- 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_BOOL(0, "run-in-shell", &suite.run_in_shell,
-+ "run programs in the suite via sh"),
-+ OPT_BOOL(0, "require-shell-test-pattern", &require_shell_test_pattern,
-+ "require programs to match 't####-*.sh'"),
- OPT_END()
- };
- struct run_process_parallel_opts opts = {
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
.task_finished = test_finished,
.data = &suite,
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)

argc = parse_options(argc, argv, NULL, options,
testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
-
+@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
if (max_jobs <= 0)
max_jobs = online_cpus();
+
+ /*
+ * If we run without a shell, we have to provide the relative path to
+ * the executables.
+ */
-+ if (!suite.run_in_shell)
+ suite.shell_path = getenv("TEST_SHELL_PATH");
+ if (!suite.shell_path)
+- suite.shell_path = "sh";
+ strbuf_addstr(&progpath, "./");
+ path_prefix_len = progpath.len;

dir = opendir(".");
if (!dir)
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
- while ((d = readdir(dir))) {
- const char *p = d->d_name;
-
-- if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-- !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-- !ends_with(p, ".sh"))
-+ if (!strcmp(p, ".") || !strcmp(p, ".."))
- continue;
-+ if (require_shell_test_pattern)
-+ if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-+ !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-+ !ends_with(p, ".sh"))
-+ continue;

/* No pattern: match all */
if (!argc) {
3: 5b34c851cd ! 5: f2746703d5 unit tests: add rule for running with test-tool

    @@ Commit message
         `make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests`, or by
         setting DEFAULT_UNIT_TEST_TARGET in config.mak.
     
    -    NEEDS WORK: we need to exclude .pdb files generated by cmake [see
    -    0df903d402 (unit-tests: do not mistake `.pdb` files for being
    -    executable, 2023-09-25)]
    -
     
      ## Makefile ##
    -@@ Makefile: $(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-L
    +@@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
      
      .PHONY: build-unit-tests unit-tests
      build-unit-tests: $(UNIT_TEST_PROGS)
    @@ Makefile: $(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITL
      	$(MAKE) -C t/ unit-tests
     
      ## t/Makefile ##
    +@@ t/Makefile: CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
    + CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
    + UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
    + UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
    ++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
     @@ t/Makefile: $(T):
      $(UNIT_TESTS):
      	@echo "*** $@ ***"; $@
    @@ t/Makefile: unit-tests-raw: $(UNIT_TESTS)
     +	@echo "*** test-tool - unit tests **"
     +	( \
     +		cd unit-tests/bin && \
    -+		../../helper/test-tool run-command testsuite --no-run-in-shell --no-require-shell-test-pattern \
    ++		../../helper/test-tool$X run-command testsuite $(UNIT_TESTS_NO_DIR)\
     +	)
     +
      pre-clean:

4: c823265f0d = 6: cd7467a7bd t/Makefile: run unit tests alongside shell tests

base-commit: 799d449105dc1f6e77fa1ebaea4f6d8bdc6537cf

2.43.0.594.gd9cf4e227d-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/helper/test-example-tap.c and adjust Makefiles as necessary.

This has the additional benefit that test harnesses seeking to run all unit tests can find them with a simple glob of "t/unit-tests/bin/t-*", with no exceptions needed. This will be important in a later patch where we add support for running the unit tests via a test-tool subcommand.

---
 Makefile                                      |  4 ++--
 t/Makefile                                    |  3 +--
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/t0080-unit-test-output.sh                   | 24 +++++++++----------
 6 files changed, 20 insertions(+), 18 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)

index 23723367b8..ba55d817ee 100644
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
 TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o

@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype

@@ -3218,7 +3218,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

index 281f4c3534..1283c90c10 100644
@@ -44,8 +44,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
-UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 
 # `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

similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..21e4848e78 100644
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "t/unit-tests/test-lib.h"
+#include "test-tool.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

@@ -69,7 +70,7 @@ static void t_empty(void)
 	; /* empty */
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
 {
 	test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
 	TEST(t_res(1), "passing test and assertion return 1");

index 33b9501c21..bb5c04c9c0 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
+	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },

index b72f07ded9..38001bd1c6 100644
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);

index 961b54b06c..83b1e3b7f5 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/helper/test-example-tap.c:77
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/helper/test-example-tap.c:31
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/helper/test-example-tap.c:49
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/helper/test-example-tap.c:57
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/helper/test-example-tap.c:92
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! test-tool example-tap >actual &&
 	test_cmp expect actual
 '
 

2.43.0.594.gd9cf4e227d-goog

Fixed in V3.

Is this last paragraph still accurate? I think in this rebased version of the series, we'll continue to use $(UNIT_TESTS) derived from the source list rather than a glob in bin/.

Sensible.

Right, thanks for the catch. Removed in V3.

I see this line in the docs [1]: "As with wildcard expansion in rules, the results of the wildcard function are sorted". GNU Make has restored the sorted behavior of $(wildcard) since 2018 [2]. I'll leave the sort off for now, but if folks feel like we need to support older versions of make, I'll add it back.

[1] https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
[2] https://savannah.gnu.org/bugs/index.php?52076

Thanks for digging. I thought I was certain that woldcard is sorted and stable and was quite perplexed when I could not find the mention in a version of doc I had handy ("""This is Edition 0.75, last updated 19 January 2020, of 'The GNU Make Manual', for GNU 'make' version 4.3.""").

Likewise (mine is the latest version in Debian unstable). The change to sort comes from their[1] eedea52a, which was in GNU make 4.2.90. But the matching documentation change didn't happen until 5b993ae, which was 4.3.90 in late 2021. So that explains the mystery.

Those dates imply to me that we should keep the $(sort), though. Six years is not so long in distro timescales, especially given that Debian unstable is on a 4-year-old version. (And if we did want to get rid of it, certainly we should do so consistently across the Makefile in a separate patch).

-Peff

[1] commit ids are from https://git.savannah.gnu.org/git/make.git

I am stuck on 4.2.1 and cannot get to 4.3.90 any time soon. Can you want on this? It will take us out unless we can suppress the $(sort)

Sincerely,
Randall

Hi Randall,

I'm not sure I follow here. The change in 4.2.90 is that wildcard expansion becomes sorted by default again. So adding the $(sort) back shouldn't cause any problems in 4.2.1. Or did I misunderstand your point?

Makefile in a

becomes

problems in

I thought you were referring to a new ${sort} behaviour in 4.2.90. My bad.

Sounds sensible. The topic is expecting a reroll so I'll make sure I won't touch it until I see an update.

Thanks.

Makes sense, thanks for investigating. I've restored the $(sort) in V3.

When running tests through test-tool run-command testsuite, we currently hardcode sh as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead.

Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly.

When setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise default to sh.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index c0ed8722c8..a41a54d9cb 100644
@@ -65,6 +65,7 @@ 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, \

@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		return 0;
 
 	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
+	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)

@@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs <= 0)
 		max_jobs = online_cpus();
 
+	suite.shell_path = getenv("TEST_SHELL_PATH");
+	if (!suite.shell_path)
+		suite.shell_path = "sh";
+
 	dir = opendir(".");
 	if (!dir)
 		die("Could not open the current directory");

2.43.0.594.gd9cf4e227d-goog

That sounds nice in theory, but ...

... this done in the testsuite() function, doesn't suite.shell_path always gets some non-NULL value? Perhaps in a later step we will add a mechanism to allow suite.shell_path to be NULL when we know we are running an executable, or something?

Leaving readers in a bit of suspense may, especially in a series that is short like this, be a good technique to entice them to keep reading, perhaps, but anyway, if that is what is intended, a simple "this feature is not used in this step, but we will take advantage of it soon in a later step" would be a good idea.

Reworded in V3.

test-tool run-command testsuite currently assumes that it will only be running the shell test suite, and therefore filters out anything that does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".

Later in this series, we'll adapt test-tool run-command testsuite to also support unit tests, which do not follow the same naming conventions as the shell tests, so this hardcoded pattern is inconvenient.

Since testsuite also allows specifying patterns on the command-line, let's just remove this pattern. As noted in [1], there are no longer any uses of testsuite in our codebase, it should be OK to break backwards compatibility in this case. We also add a new filter to avoid trying to execute "." and "..", so that users who wish to execute every test in a directory can do so without specifying a pattern.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index a41a54d9cb..e6bd792274 100644
@@ -175,9 +175,7 @@ static int testsuite(int argc, const char **argv)
 	while ((d = readdir(dir))) {
 		const char *p = d->d_name;
 
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
+		if (!strcmp(p, ".") || !strcmp(p, ".."))
 			continue;
 
 		/* No pattern: match all */

2.43.0.594.gd9cf4e227d-goog

Makes sense to explain what future steps this prepares the codebase for like this.

As we discussed in Peff's Makefile change that enumerates "which are the unit-test programs?" Generally, $(wildcard) and readdir() to slurp everything in a directory, including stuff that is an untracked cruft, is not an excellent idea.

This is not an end-user facing program and we are in full control of its input (most notably, "which ones should we be running?"), I do not think it would be a huge issue, though.

Would you prefer if I remove the default behavior of "run everything in the CWD" and require passing in at least one filename filter?

No preference either way at all.

Teach the testsuite runner in test-tool run-command testsuite how to run unit tests: if TEST_SHELL_PATH is not set, assume that we're running the programs directly from CWD, rather than defaulting to "sh" as an interpreter.

With this change, you can now use test-tool to run the unit tests:

$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

This again breaks backwards compatibility, as it is now required to set TEST_SHELL_PATH properly for executing shell scripts, but again, as noted in [2], there are no longer any such invocations in our codebase.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

index e6bd792274..a0b8dc6fd7 100644
@@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
 		.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);

@@ -165,9 +167,14 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs <= 0)
 		max_jobs = online_cpus();
 
+	/*
+	 * If we run without a shell, we have to provide the relative path to
+	 * the executables.
+	 */
 	suite.shell_path = getenv("TEST_SHELL_PATH");
 	if (!suite.shell_path)
-		suite.shell_path = "sh";
+		strbuf_addstr(&progpath, "./");
+	path_prefix_len = progpath.len;
 
 	dir = opendir(".");
 	if (!dir)

@@ -180,13 +187,17 @@ static int testsuite(int argc, const char **argv)
 
 		/* No pattern: match all */
 		if (!argc) {
-			string_list_append(&suite.tests, p);
+			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)) {
-				string_list_append(&suite.tests, p);
+				strbuf_setlen(&progpath, path_prefix_len);
+				strbuf_addstr(&progpath, p);
+				string_list_append(&suite.tests, progpath.buf);
 				break;
 			}
 	}

@@ -213,6 +224,7 @@ static int testsuite(int argc, const char **argv)
 
 	string_list_clear(&suite.tests, 0);
 	string_list_clear(&suite.failed, 0);
+	strbuf_release(&progpath);
 
 	return ret;
 }

2.43.0.594.gd9cf4e227d-goog

Hi Josh

Judging from the last patch in this series it seems likely that we'll want to run unit tests and integration tests parallel. In which case it might be better to look at the filename extension to decide whether to sh as an interpreter so that we can avoid having to use a wrapper script. Then

    cd t
    helper/test-tool run-command testsuite 't[0-9]*.sh' 'unit-tests/bin/*'

would run the integration tests via "sh" and the unit-tests directly. We'd need to figure out how to look for tests in both directories as well though...

Best Wishes

Phillip

At the moment, I'm not planning on trying to make unit tests and shell tests run under the same test-tool process. If that is a valuable change, hopefully the Windows / CI experts can use this series as a starting point to make additional test-tool changes. However, I will probably not spend any further time on this area.

Hmph. It sounds more like "the run-command testsuite subcommand only runs programs in the current directory", not "assume" (which implies there is a way to override the assumption). Not that the limitation would hurt us in any way, though.

Reworded both of these in V3.

In the previous commit, we added support in test-tool for running collections of unit tests. Now, add rules in t/Makefile for running in this way.

This new rule can be executed from the top-level Makefile via make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests, or by setting DEFAULT_UNIT_TEST_TARGET in config.mak.

---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

index ba55d817ee..b0d1f04b4d 100644
@@ -3870,5 +3870,5 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests

index 1283c90c10..6e6316c29b 100644
@@ -45,6 +45,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+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

@@ -72,7 +73,7 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)

@@ -80,6 +81,13 @@ 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)'
 

2.43.0.594.gd9cf4e227d-goog

From: Jeff King <[email protected]>

Add a wrapper script to allow prove to run both shell tests and unit tests from a single invocation. This avoids issues around running prove twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so that errors can be spotted more quickly.

NEEDS WORK: as discussed in previous commits in this series, there's a desire to avoid prove specifically and (IIUC) unnecessary fork()/exec()ing in general on Windows. This change adds an extra exec() for each shell and unit test execution, will that be a problem for Windows?

[1] https://lore.kernel.org/git/[email protected]/

---
 t/Makefile    |  2 +-
 t/run-test.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100755 t/run-test.sh

index 6e6316c29b..6a67fc22d7 100644
@@ -64,7 +64,7 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):

new file mode 100755
index 0000000000..c29fef48dc
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+	exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac

2.43.0.594.gd9cf4e227d-goog

It does, but IMHO that is not likely to be a problem. It's once per top-level script (so ~1000), and each of those scripts spawns hundreds or thousands of sub-commands. I didn't do any measurements, though.

You can extend "prove" with extra perl modules so that it makes the distinction internally without the extra shell invocation. But when I tried to do it, I found it rather baroque and complicated (I can't remember if I succeeded but found it too gross, or just gave up halfway through trying).

Not if they share the same command-line options. If you use something like "--state=slow,save", then the first run will write the list of all tests to ".prove", and then the second will run every test mentioned in .prove (in addition to the unit-tests provided on the command-line).

You should be able to work around it by passing "--statefile". I _think_ it might be OK to just do that unconditionally. Something like:

  prove --exec $(TEST_SHELL_PATH $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
  prove --statefile=.prove-unit-tests $(GIT_PROVE_OPTS) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)

and then it's just a noop if GIT_PROVE_OPTS doesn't use --state. But I haven't played with it myself.

-Peff

I do not think it warrants such complexity. The wrapper script is fine.

Thanks to Dscho who figured this out, the jk/unit-tests-buildfix topic in my tree has been updated to pass "win test (n)" jobs.

I do not know much about "confusing" thing, but if the user allocates, say, 16 jobs to run tests in parallel, and one of them drives the "unit test suite runner" that wants to do its own parallelism, we'd easily end up busting the resource limit the end-user desires. It does not necessarily mean that we should limit the parallelism of "unit test suite runner" to 1 under prove, though.

The current prove helper script does not do any separate parallelism, so we are fine in that case, and the new test-tool runner does not currently support running both unit tests and shell tests in the same process, so we should be OK in either case.

The first one seems to have been resolved. The latter can happen while it cooks, I would presume. If there is no other comments, let's mark the topic for 'next'.

Thanks.

Please note: this series has once again been rebased onto the latest jk/unit-tests-buildfix.

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

[1] https://lore.kernel.org/git/[email protected]/

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in test-tool ... testsuite
* don't hardcode a test name filter in test-tool ... testsuite
* add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process

Changes in V3:
* Added new patch (#7) to use the new test-tool runner for unit tests in Windows CI.
* Restored the explicit sort call in t/Makefile, for backwards compatibility with older GNU Make versions.
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Fixed header include order in patch #1.
* Removed a paragraph in patch #1's commit message that is obsolete now that we're building the list of test files from the sources rather than by globbing.
* Added a note in patch #2 that setting a NULL suite.shell_path will be used in a later commit.
* Clarified up some sloppy wording in commit messages and comments in t/helper/test-run-command.c.

Changes in V2:
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Patch 1: move t-basic to a test-tool subcommand rather than a new executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in test-tool run-command testsuite
* New patch 3: remove the hardcoded filename filter in test-tool run-command testsuite
* Patch 4 (previously 2): simplified now that we no longer need to add any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using the unit test list built in the makefile in jk/unit-tests-buildfix.

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (6):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: get shell from env test-tool run-command testsuite: remove hardcoded filter test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool
ci: use test-tool as unit test runner on Windows

 Makefile                                      |  6 ++--
 ci/run-build-and-tests.sh                     |  2 --
 ci/run-test-slice.sh                          |  2 +-
 t/Makefile                                    | 14 ++++++++--
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-run-command.c                   | 28 +++++++++++++++----
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/run-test.sh                                 | 13 +++++++++
 t/t0080-unit-test-output.sh                   | 24 ++++++++--------
 10 files changed, 67 insertions(+), 29 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
 create mode 100755 t/run-test.sh

Range-diff against v2:
1: da756b4bfb ! 1: 6777451100 t0080: turn t-basic unit test into a helper

    @@ Commit message
         t0080-unit-test-output.sh, so let's move it to
         t/helper/test-example-tap.c and adjust Makefiles as necessary.
     
    -    This has the additional benefit that test harnesses seeking to run all
    -    unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
    -    with no exceptions needed. This will be important in a later patch where
    -    we add support for running the unit tests via a test-tool subcommand.
    -
     
      ## Makefile ##
    @@ Makefile: perf: all
      	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
     
      ## t/Makefile ##
    -@@ t/Makefile: TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
    - CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
    +@@ t/Makefile: CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
      CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
      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 = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
     -UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
    -+UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
    ++UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
      
      # `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
    @@ t/Makefile: TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
      ## t/unit-tests/t-basic.c => t/helper/test-example-tap.c ##
     @@
     -#include "test-lib.h"
    -+#include "t/unit-tests/test-lib.h"
     +#include "test-tool.h"
    ++#include "t/unit-tests/test-lib.h"
      
      /*
       * The purpose of this "unit test" is to verify a few invariants of the unit

2: c8448406d7 ! 2: 24f47f8fc7 test-tool run-command testsuite: get shell from env
@@ Commit message
Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test
- program directly.
+ program directly. We will use this feature in a later commit to enable
+ running binary executable unit tests.

- When setting up the struct testsuite in testsuite(), use the value
- of TEST_SHELL_PATH if it's set, otherwise default to sh.
+ However, for now when setting up the struct testsuite in testsuite(),
+ use the value of TEST_SHELL_PATH if it's set, otherwise keep the
+ original behavior by defaulting to sh.

[1] https://lore.kernel.org/git/[email protected]/

3:  e1b89ae93e = 3:  4a16a3ec24 test-tool run-command testsuite: remove hardcoded filter
4:  b5665386b5 ! 4:  abc9a7afe8 test-tool run-command testsuite: support unit tests

@@ Commit message
test-tool run-command testsuite: support unit tests

Teach the testsuite runner in test-tool run-command testsuite how to
- run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
- the programs directly from CWD, rather than defaulting to "sh" as an
- interpreter.
+ run unit tests: if TEST_SHELL_PATH is not set, run the programs directly
+ from CWD, rather than defaulting to "sh" as an interpreter.

With this change, you can now use test-tool to run the unit tests:
$ make
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
max_jobs = online_cpus();

+ /*
-+ * If we run without a shell, we have to provide the relative path to
-+ * the executables.
++ * If we run without a shell, execute the programs directly from CWD.
+ */
suite.shell_path = getenv("TEST_SHELL_PATH");
if (!suite.shell_path)
5: f2746703d5 ! 5: a8bbff2c6b unit tests: add rule for running with test-tool

    @@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_
      	$(MAKE) -C t/ unit-tests
     
      ## t/Makefile ##
    -@@ t/Makefile: CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
    - CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
    +@@ t/Makefile: CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
      UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
    - UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
    + UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
    + 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`)

6: cd7467a7bd ! 6: cfcc4bd427 t/Makefile: run unit tests alongside shell tests
@@ Commit message
twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so
- that errors can be spotted more quickly.
-
- NEEDS WORK: as discussed in previous commits in this series, there's a
- desire to avoid prove specifically and (IIUC) unnecessary
- fork()/exec()ing in general on Windows. This change adds an extra exec()
- for each shell and unit test execution, will that be a problem for
- Windows?
+ that errors can be spotted more quickly. Accordingly, we remove the
+ separate unit tests step for Linux CI. (We leave the Windows CI
+ unit-test step as-is, because the sharding scheme there involves
+ selecting specific test files rather than running make test.)

[1] https://lore.kernel.org/git/[email protected]/

+ ## ci/run-build-and-tests.sh ##
+@@ ci/run-build-and-tests.sh: if test -n "$run_tests"
+ then
+ group "Run tests" make test ||
+ handle_failed_tests
+- group "Run unit tests" \
+- make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
+ fi
+ check_unignored_build_artifacts
+
+
## t/Makefile ##
@@ t/Makefile: failed:
test -z "$$failed" || $(MAKE) $$failed
-: ---------- > 7: cbf37e0ddc ci: use test-tool as unit test runner on Windows

base-commit: 4904a4d08cc085716df12ce713ae7ee3d5ecb75a

2.44.0.rc0.258.g7320e95886-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/helper/test-example-tap.c and adjust Makefiles as necessary.

---
 Makefile                                      |  4 ++--
 t/Makefile                                    |  2 +-
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/t0080-unit-test-output.sh                   | 24 +++++++++----------
 6 files changed, 20 insertions(+), 17 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)

index 23723367b8..ba55d817ee 100644
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
 TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o

@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype

@@ -3218,7 +3218,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

index 2d95046f26..4861edafe6 100644
@@ -48,7 +48,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
 
 # `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

similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "test-tool.h"
+#include "t/unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

@@ -69,7 +70,7 @@ static void t_empty(void)
 	; /* empty */
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
 {
 	test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
 	TEST(t_res(1), "passing test and assertion return 1");

index 33b9501c21..bb5c04c9c0 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
+	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },

index b72f07ded9..38001bd1c6 100644
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);

index 961b54b06c..83b1e3b7f5 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/helper/test-example-tap.c:77
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/helper/test-example-tap.c:31
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/helper/test-example-tap.c:49
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/helper/test-example-tap.c:57
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/helper/test-example-tap.c:92
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! test-tool example-tap >actual &&
 	test_cmp expect actual
 '
 

2.44.0.rc0.258.g7320e95886-goog

When running tests through test-tool run-command testsuite, we currently hardcode sh as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead.

Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly. We will use this feature in a later commit to enable running binary executable unit tests.

However, for now when setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise keep the original behavior by defaulting to sh.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index c0ed8722c8..a41a54d9cb 100644
@@ -65,6 +65,7 @@ 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, \

@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		return 0;
 
 	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
+	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)

@@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs <= 0)
 		max_jobs = online_cpus();
 
+	suite.shell_path = getenv("TEST_SHELL_PATH");
+	if (!suite.shell_path)
+		suite.shell_path = "sh";
+
 	dir = opendir(".");
 	if (!dir)
 		die("Could not open the current directory");

2.44.0.rc0.258.g7320e95886-goog

test-tool run-command testsuite currently assumes that it will only be running the shell test suite, and therefore filters out anything that does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".

Later in this series, we'll adapt test-tool run-command testsuite to also support unit tests, which do not follow the same naming conventions as the shell tests, so this hardcoded pattern is inconvenient.

Since testsuite also allows specifying patterns on the command-line, let's just remove this pattern. As noted in [1], there are no longer any uses of testsuite in our codebase, it should be OK to break backwards compatibility in this case. We also add a new filter to avoid trying to execute "." and "..", so that users who wish to execute every test in a directory can do so without specifying a pattern.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index a41a54d9cb..e6bd792274 100644
@@ -175,9 +175,7 @@ static int testsuite(int argc, const char **argv)
 	while ((d = readdir(dir))) {
 		const char *p = d->d_name;
 
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
+		if (!strcmp(p, ".") || !strcmp(p, ".."))
 			continue;
 
 		/* No pattern: match all */

2.44.0.rc0.258.g7320e95886-goog

Teach the testsuite runner in test-tool run-command testsuite how to run unit tests: if TEST_SHELL_PATH is not set, run the programs directly from CWD, rather than defaulting to "sh" as an interpreter.

With this change, you can now use test-tool to run the unit tests:

$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

This again breaks backwards compatibility, as it is now required to set TEST_SHELL_PATH properly for executing shell scripts, but again, as noted in [2], there are no longer any such invocations in our codebase.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index e6bd792274..61eb1175fe 100644
@@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
 		.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);

@@ -165,9 +167,13 @@ static int testsuite(int argc, const char **argv)
 	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)
-		suite.shell_path = "sh";
+		strbuf_addstr(&progpath, "./");
+	path_prefix_len = progpath.len;
 
 	dir = opendir(".");
 	if (!dir)

@@ -180,13 +186,17 @@ static int testsuite(int argc, const char **argv)
 
 		/* No pattern: match all */
 		if (!argc) {
-			string_list_append(&suite.tests, p);
+			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)) {
-				string_list_append(&suite.tests, p);
+				strbuf_setlen(&progpath, path_prefix_len);
+				strbuf_addstr(&progpath, p);
+				string_list_append(&suite.tests, progpath.buf);
 				break;
 			}
 	}

@@ -213,6 +223,7 @@ static int testsuite(int argc, const char **argv)
 
 	string_list_clear(&suite.tests, 0);
 	string_list_clear(&suite.failed, 0);
+	strbuf_release(&progpath);
 
 	return ret;
 }

2.44.0.rc0.258.g7320e95886-goog

In the previous commit, we added support in test-tool for running collections of unit tests. Now, add rules in t/Makefile for running in this way.

This new rule can be executed from the top-level Makefile via make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests, or by setting DEFAULT_UNIT_TEST_TARGET in config.mak.

---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

index ba55d817ee..b0d1f04b4d 100644
@@ -3870,5 +3870,5 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests

index 4861edafe6..0ae04f1e42 100644
@@ -49,6 +49,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 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

@@ -76,7 +77,7 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)

@@ -84,6 +85,13 @@ 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)'
 

2.44.0.rc0.258.g7320e95886-goog

From: Jeff King <[email protected]>

Add a wrapper script to allow prove to run both shell tests and unit tests from a single invocation. This avoids issues around running prove twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so that errors can be spotted more quickly. Accordingly, we remove the separate unit tests step for Linux CI. (We leave the Windows CI unit-test step as-is, because the sharding scheme there involves selecting specific test files rather than running make test.)

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-build-and-tests.sh |  2 --
 t/Makefile                |  2 +-
 t/run-test.sh             | 13 +++++++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)
 create mode 100755 t/run-test.sh

index 7a1466b868..2528f25e31 100755
@@ -50,8 +50,6 @@ if test -n "$run_tests"
 then
 	group "Run tests" make test ||
 	handle_failed_tests
-	group "Run unit tests" \
-		make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
 fi
 check_unignored_build_artifacts
 

index 0ae04f1e42..0c739754d8 100644
@@ -68,7 +68,7 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):

new file mode 100755
index 0000000000..c29fef48dc
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+	exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac

2.44.0.rc0.258.g7320e95886-goog

Actually, I think the "manual prove outside of the Makefile" situation is worth keeping this. I know I sometimes copy commands from Makefiles and run them manually when debugging issues, so it could be annoying for folks if we remove the default here.

Hmm, by "manually running prove" I meant running:

prove t0001-init.sh

and so on. Or even "prove --state=failed" to re-run failed tests. But neither of those would even use this script, because there's no --exec option.

But it sounds like you mean literally cutting and pasting the "prove --exec" line from the Makefile. That seems to me like a weird thing to want to do, but OK, I'll try not to judge your workflow. ;)

But if you are worried about making debugging more confusing, it seems like silently defaulting to /bin/sh might make things worse. It is not necessarily what "make test" did, and complaining loudly might be more helpful than trying to run with an alternate shell.

I don't feel too strongly about it, though. I'd generally just run single tests as "./t0001-init.sh", which runs into the same issue. I've been working on Git long enough that I know it is one of the possible gotchas when a test failure does not reproduce. :)

-Peff

Alright, fixed in V4. We now set TEST_SHELL_PATH when running prove, and we error out in run-test.sh if TEST_SHELL_PATH is not set.

Although the previous commit changed t/Makefile to run unit tests alongside shell tests, the Windows CI still needs a separate unit-tests step due to how the test sharding works.

We want to avoid using prove as a test running on Windows due to performance issues [1], so use the new test-tool runner instead.

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ae8094382f..e167e646f7 100755
@@ -17,7 +17,7 @@ handle_failed_tests
 
 # We only have one unit test at the moment, so run it in the first slice
 if [ "$1" == "0" ] ; then
-	group "Run unit tests" make --quiet -C t unit-tests-prove
+	group "Run unit tests" make --quiet -C t unit-tests-test-tool
 fi
 
 check_unignored_build_artifacts

2.44.0.rc0.258.g7320e95886-goog

Folks, what's the status of this one? I just checked the RFC thread and this last one (more than a month ago) and the issues seems to have been addressed, but I prefer positive acks rather than "we've seen it already, take our silence as the sign of endorsement".

Thanks.

It is not a topic I am really interested in, so I was not following closely. But I think my earlier issues were addressed, and from a quick read now it looks OK to me. (I left one small comment, but I don't think it's a big deal either way).

-Peff

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

[1] https://lore.kernel.org/git/[email protected]/

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in test-tool ... testsuite
* don't hardcode a test name filter in test-tool ... testsuite
* add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process

Changes in V4:
* Set TEST_SHELL_PATH when running the prove test target, and error out of run-test.sh if TEST_SHELL_PATH is not set.

Changes in V3:
* Added new patch (#7) to use the new test-tool runner for unit tests in Windows CI.
* Restored the explicit sort call in t/Makefile, for backwards compatibility with older GNU Make versions.
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Fixed header include order in patch #1.
* Removed a paragraph in patch #1's commit message that is obsolete now that we're building the list of test files from the sources rather than by globbing.
* Added a note in patch #2 that setting a NULL suite.shell_path will be used in a later commit.
* Clarified up some sloppy wording in commit messages and comments in t/helper/test-run-command.c.

Changes in V2:
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Patch 1: move t-basic to a test-tool subcommand rather than a new executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in test-tool run-command testsuite
* New patch 3: remove the hardcoded filename filter in test-tool run-command testsuite
* Patch 4 (previously 2): simplified now that we no longer need to add any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using the unit test list built in the makefile in jk/unit-tests-buildfix.

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (6):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: get shell from env test-tool run-command testsuite: remove hardcoded filter test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool
ci: use test-tool as unit test runner on Windows

 Makefile                                      |  6 ++--
 ci/run-build-and-tests.sh                     |  2 --
 ci/run-test-slice.sh                          |  2 +-
 t/Makefile                                    | 14 ++++++++--
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-run-command.c                   | 28 +++++++++++++++----
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/run-test.sh                                 | 17 +++++++++++
 t/t0080-unit-test-output.sh                   | 24 ++++++++--------
 10 files changed, 71 insertions(+), 29 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
 create mode 100755 t/run-test.sh

Range-diff against v3:

1:  6777451100 = 1:  6777451100 t0080: turn t-basic unit test into a helper
2:  24f47f8fc7 = 2:  24f47f8fc7 test-tool run-command testsuite: get shell from env
3:  4a16a3ec24 = 3:  4a16a3ec24 test-tool run-command testsuite: remove hardcoded filter
4:  abc9a7afe8 = 4:  abc9a7afe8 test-tool run-command testsuite: support unit tests
5:  a8bbff2c6b = 5:  a8bbff2c6b unit tests: add rule for running with test-tool
6:  cfcc4bd427 ! 6:  0e32de1afe t/Makefile: run unit tests alongside shell tests
    @@ t/Makefile: failed:

      prove: pre-clean check-chainlint $(TEST_LINT)
     -  @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
    -+  @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
    ++  @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
        $(MAKE) clean-except-prove-cache

      $(T):
    @@ t/run-test.sh (new)
     +
     +case "$1" in
     +*.sh)
    -+  exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
    ++  if test -z "${TEST_SHELL_PATH+set}" ; then
    ++          echo "ERROR: TEST_SHELL_PATH is not set" >&2
    ++          exit 1
    ++  fi
    ++  exec ${TEST_SHELL_PATH} "$@"
     +  ;;
     +*)
     +  exec "$@"

7: cbf37e0ddc = 7: c562515293 ci: use test-tool as unit test runner on Windows

base-commit: 4904a4d08cc085716df12ce713ae7ee3d5ecb75a

2.44.0.769.g3c40516874-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/helper/test-example-tap.c and adjust Makefiles as necessary.

---
 Makefile                                      |  4 ++--
 t/Makefile                                    |  2 +-
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/t0080-unit-test-output.sh                   | 24 +++++++++----------
 6 files changed, 20 insertions(+), 17 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)

index 23723367b8..ba55d817ee 100644
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
 TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o

@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype

@@ -3218,7 +3218,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

index 2d95046f26..4861edafe6 100644
@@ -48,7 +48,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
 
 # `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

similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "test-tool.h"
+#include "t/unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

@@ -69,7 +70,7 @@ static void t_empty(void)
 	; /* empty */
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
 {
 	test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
 	TEST(t_res(1), "passing test and assertion return 1");

index 33b9501c21..bb5c04c9c0 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
+	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },

index b72f07ded9..38001bd1c6 100644
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);

index 961b54b06c..83b1e3b7f5 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/helper/test-example-tap.c:77
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/helper/test-example-tap.c:31
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/helper/test-example-tap.c:49
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/helper/test-example-tap.c:57
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/helper/test-example-tap.c:92
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! test-tool example-tap >actual &&
 	test_cmp expect actual
 '
 

2.44.0.769.g3c40516874-goog

When running tests through test-tool run-command testsuite, we currently hardcode sh as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead.

Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly. We will use this feature in a later commit to enable running binary executable unit tests.

However, for now when setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise keep the original behavior by defaulting to sh.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index c0ed8722c8..a41a54d9cb 100644
@@ -65,6 +65,7 @@ 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, \

@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		return 0;
 
 	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
+	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)

@@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs <= 0)
 		max_jobs = online_cpus();
 
+	suite.shell_path = getenv("TEST_SHELL_PATH");
+	if (!suite.shell_path)
+		suite.shell_path = "sh";
+
 	dir = opendir(".");
 	if (!dir)
 		die("Could not open the current directory");

2.44.0.769.g3c40516874-goog

test-tool run-command testsuite currently assumes that it will only be running the shell test suite, and therefore filters out anything that does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".

Later in this series, we'll adapt test-tool run-command testsuite to also support unit tests, which do not follow the same naming conventions as the shell tests, so this hardcoded pattern is inconvenient.

Since testsuite also allows specifying patterns on the command-line, let's just remove this pattern. As noted in [1], there are no longer any uses of testsuite in our codebase, it should be OK to break backwards compatibility in this case. We also add a new filter to avoid trying to execute "." and "..", so that users who wish to execute every test in a directory can do so without specifying a pattern.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index a41a54d9cb..e6bd792274 100644
@@ -175,9 +175,7 @@ static int testsuite(int argc, const char **argv)
 	while ((d = readdir(dir))) {
 		const char *p = d->d_name;
 
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
+		if (!strcmp(p, ".") || !strcmp(p, ".."))
 			continue;
 
 		/* No pattern: match all */

2.44.0.769.g3c40516874-goog

Teach the testsuite runner in test-tool run-command testsuite how to run unit tests: if TEST_SHELL_PATH is not set, run the programs directly from CWD, rather than defaulting to "sh" as an interpreter.

With this change, you can now use test-tool to run the unit tests:

$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

This again breaks backwards compatibility, as it is now required to set TEST_SHELL_PATH properly for executing shell scripts, but again, as noted in [2], there are no longer any such invocations in our codebase.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index e6bd792274..61eb1175fe 100644
@@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
 		.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);

@@ -165,9 +167,13 @@ static int testsuite(int argc, const char **argv)
 	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)
-		suite.shell_path = "sh";
+		strbuf_addstr(&progpath, "./");
+	path_prefix_len = progpath.len;
 
 	dir = opendir(".");
 	if (!dir)

@@ -180,13 +186,17 @@ static int testsuite(int argc, const char **argv)
 
 		/* No pattern: match all */
 		if (!argc) {
-			string_list_append(&suite.tests, p);
+			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)) {
-				string_list_append(&suite.tests, p);
+				strbuf_setlen(&progpath, path_prefix_len);
+				strbuf_addstr(&progpath, p);
+				string_list_append(&suite.tests, progpath.buf);
 				break;
 			}
 	}

@@ -213,6 +223,7 @@ static int testsuite(int argc, const char **argv)
 
 	string_list_clear(&suite.tests, 0);
 	string_list_clear(&suite.failed, 0);
+	strbuf_release(&progpath);
 
 	return ret;
 }

2.44.0.769.g3c40516874-goog

In the previous commit, we added support in test-tool for running collections of unit tests. Now, add rules in t/Makefile for running in this way.

This new rule can be executed from the top-level Makefile via make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests, or by setting DEFAULT_UNIT_TEST_TARGET in config.mak.

---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

index ba55d817ee..b0d1f04b4d 100644
@@ -3870,5 +3870,5 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests

index 4861edafe6..0ae04f1e42 100644
@@ -49,6 +49,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 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

@@ -76,7 +77,7 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)

@@ -84,6 +85,13 @@ 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)'
 

2.44.0.769.g3c40516874-goog

From: Jeff King <[email protected]>

Add a wrapper script to allow prove to run both shell tests and unit tests from a single invocation. This avoids issues around running prove twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so that errors can be spotted more quickly. Accordingly, we remove the separate unit tests step for Linux CI. (We leave the Windows CI unit-test step as-is, because the sharding scheme there involves selecting specific test files rather than running make test.)

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-build-and-tests.sh |  2 --
 t/Makefile                |  2 +-
 t/run-test.sh             | 17 +++++++++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100755 t/run-test.sh

index 7a1466b868..2528f25e31 100755
@@ -50,8 +50,6 @@ if test -n "$run_tests"
 then
 	group "Run tests" make test ||
 	handle_failed_tests
-	group "Run unit tests" \
-		make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
 fi
 check_unignored_build_artifacts
 

index 0ae04f1e42..b2eb9f770b 100644
@@ -68,7 +68,7 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):

new file mode 100755
index 0000000000..a0e2dce5e0
@@ -0,0 +1,17 @@
+#!/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+set}" ; then
+		echo "ERROR: TEST_SHELL_PATH is not set" >&2
+		exit 1
+	fi
+	exec ${TEST_SHELL_PATH} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac

2.44.0.769.g3c40516874-goog

Fixed in V5, thanks.

If we are using ":+" to handle the empty string, I think just:

if test -z "$TEST_SHELL_PATH"

is sufficient, no?

(not that the other is incorrect, but whenever I see something like ":+set" I wonder if something more clever is going on, and of course I get nightmare flashbacks to looking at generated autoconf code).

-Peff

Yes. And the other part of this hunk still needs fixing, namely,

Fixed in V6.

Although the previous commit changed t/Makefile to run unit tests alongside shell tests, the Windows CI still needs a separate unit-tests step due to how the test sharding works.

We want to avoid using prove as a test running on Windows due to performance issues [1], so use the new test-tool runner instead.

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ae8094382f..e167e646f7 100755
@@ -17,7 +17,7 @@ handle_failed_tests
 
 # We only have one unit test at the moment, so run it in the first slice
 if [ "$1" == "0" ] ; then
-	group "Run unit tests" make --quiet -C t unit-tests-prove
+	group "Run unit tests" make --quiet -C t unit-tests-test-tool
 fi
 
 check_unignored_build_artifacts

2.44.0.769.g3c40516874-goog

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

[1] https://lore.kernel.org/git/[email protected]/

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in test-tool ... testsuite
* don't hardcode a test name filter in test-tool ... testsuite
* add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process

Changes in V5:
* Style fix in t/run-test.sh

Changes in V4:
* Set TEST_SHELL_PATH when running the prove test target, and error out of run-test.sh if TEST_SHELL_PATH is not set.

Changes in V3:
* Added new patch (#7) to use the new test-tool runner for unit tests in Windows CI.
* Restored the explicit sort call in t/Makefile, for backwards compatibility with older GNU Make versions.
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Fixed header include order in patch #1.
* Removed a paragraph in patch #1's commit message that is obsolete now that we're building the list of test files from the sources rather than by globbing.
* Added a note in patch #2 that setting a NULL suite.shell_path will be used in a later commit.
* Clarified up some sloppy wording in commit messages and comments in t/helper/test-run-command.c.

Changes in V2:
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Patch 1: move t-basic to a test-tool subcommand rather than a new executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in test-tool run-command testsuite
* New patch 3: remove the hardcoded filename filter in test-tool run-command testsuite
* Patch 4 (previously 2): simplified now that we no longer need to add any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using the unit test list built in the makefile in jk/unit-tests-buildfix.

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (6):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: get shell from env test-tool run-command testsuite: remove hardcoded filter test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool
ci: use test-tool as unit test runner on Windows

 Makefile                                      |  6 ++--
 ci/run-build-and-tests.sh                     |  2 --
 ci/run-test-slice.sh                          |  2 +-
 t/Makefile                                    | 14 ++++++++--
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-run-command.c                   | 28 +++++++++++++++----
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/run-test.sh                                 | 18 ++++++++++++
 t/t0080-unit-test-output.sh                   | 24 ++++++++--------
 10 files changed, 72 insertions(+), 29 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
 create mode 100755 t/run-test.sh

Range-diff against v4:

1:  6777451100 = 1:  6777451100 t0080: turn t-basic unit test into a helper
2:  24f47f8fc7 = 2:  24f47f8fc7 test-tool run-command testsuite: get shell from env
3:  4a16a3ec24 = 3:  4a16a3ec24 test-tool run-command testsuite: remove hardcoded filter
4:  abc9a7afe8 = 4:  abc9a7afe8 test-tool run-command testsuite: support unit tests
5:  a8bbff2c6b = 5:  a8bbff2c6b unit tests: add rule for running with test-tool
6:  0e32de1afe ! 6:  c6606446c4 t/Makefile: run unit tests alongside shell tests
    @@ t/run-test.sh (new)
     +
     +case "$1" in
     +*.sh)
    -+  if test -z "${TEST_SHELL_PATH+set}" ; then
    -+          echo "ERROR: TEST_SHELL_PATH is not set" >&2
    ++  if test -z "${TEST_SHELL_PATH+set}"
    ++  then
    ++          echo >&2 "ERROR: TEST_SHELL_PATH is empty or not set"
     +          exit 1
     +  fi
     +  exec ${TEST_SHELL_PATH} "$@"

7: c562515293 = 7: 4a92131ab9 ci: use test-tool as unit test runner on Windows

base-commit: 4904a4d08cc085716df12ce713ae7ee3d5ecb75a

2.45.0.rc0.197.gbae5840b3b-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/helper/test-example-tap.c and adjust Makefiles as necessary.

---
 Makefile                                      |  4 ++--
 t/Makefile                                    |  2 +-
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/t0080-unit-test-output.sh                   | 24 +++++++++----------
 6 files changed, 20 insertions(+), 17 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)

index 23723367b8..ba55d817ee 100644
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
 TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o

@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype

@@ -3218,7 +3218,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

index 2d95046f26..4861edafe6 100644
@@ -48,7 +48,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
 
 # `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

similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "test-tool.h"
+#include "t/unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

@@ -69,7 +70,7 @@ static void t_empty(void)
 	; /* empty */
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
 {
 	test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
 	TEST(t_res(1), "passing test and assertion return 1");

index 33b9501c21..bb5c04c9c0 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
+	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },

index b72f07ded9..38001bd1c6 100644
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);

index 961b54b06c..83b1e3b7f5 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/helper/test-example-tap.c:77
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/helper/test-example-tap.c:31
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/helper/test-example-tap.c:49
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/helper/test-example-tap.c:57
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/helper/test-example-tap.c:92
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! test-tool example-tap >actual &&
 	test_cmp expect actual
 '
 

2.45.0.rc0.197.gbae5840b3b-goog

When running tests through test-tool run-command testsuite, we currently hardcode sh as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead.

Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly. We will use this feature in a later commit to enable running binary executable unit tests.

However, for now when setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise keep the original behavior by defaulting to sh.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index c0ed8722c8..a41a54d9cb 100644
@@ -65,6 +65,7 @@ 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, \

@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
 		return 0;
 
 	test = suite->tests.items[suite->next++].string;
-	strvec_pushl(&cp->args, "sh", test, NULL);
+	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)

@@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs <= 0)
 		max_jobs = online_cpus();
 
+	suite.shell_path = getenv("TEST_SHELL_PATH");
+	if (!suite.shell_path)
+		suite.shell_path = "sh";
+
 	dir = opendir(".");
 	if (!dir)
 		die("Could not open the current directory");

2.45.0.rc0.197.gbae5840b3b-goog

test-tool run-command testsuite currently assumes that it will only be running the shell test suite, and therefore filters out anything that does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".

Later in this series, we'll adapt test-tool run-command testsuite to also support unit tests, which do not follow the same naming conventions as the shell tests, so this hardcoded pattern is inconvenient.

Since testsuite also allows specifying patterns on the command-line, let's just remove this pattern. As noted in [1], there are no longer any uses of testsuite in our codebase, it should be OK to break backwards compatibility in this case. We also add a new filter to avoid trying to execute "." and "..", so that users who wish to execute every test in a directory can do so without specifying a pattern.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index a41a54d9cb..e6bd792274 100644
@@ -175,9 +175,7 @@ static int testsuite(int argc, const char **argv)
 	while ((d = readdir(dir))) {
 		const char *p = d->d_name;
 
-		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
-		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
-		    !ends_with(p, ".sh"))
+		if (!strcmp(p, ".") || !strcmp(p, ".."))
 			continue;
 
 		/* No pattern: match all */

2.45.0.rc0.197.gbae5840b3b-goog

Teach the testsuite runner in test-tool run-command testsuite how to run unit tests: if TEST_SHELL_PATH is not set, run the programs directly from CWD, rather than defaulting to "sh" as an interpreter.

With this change, you can now use test-tool to run the unit tests:

$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite

This should be helpful on Windows to allow running tests without requiring Perl (for prove), as discussed in [1] and [2].

This again breaks backwards compatibility, as it is now required to set TEST_SHELL_PATH properly for executing shell scripts, but again, as noted in [2], there are no longer any such invocations in our codebase.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index e6bd792274..61eb1175fe 100644
@@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
 		.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);

@@ -165,9 +167,13 @@ static int testsuite(int argc, const char **argv)
 	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)
-		suite.shell_path = "sh";
+		strbuf_addstr(&progpath, "./");
+	path_prefix_len = progpath.len;
 
 	dir = opendir(".");
 	if (!dir)

@@ -180,13 +186,17 @@ static int testsuite(int argc, const char **argv)
 
 		/* No pattern: match all */
 		if (!argc) {
-			string_list_append(&suite.tests, p);
+			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)) {
-				string_list_append(&suite.tests, p);
+				strbuf_setlen(&progpath, path_prefix_len);
+				strbuf_addstr(&progpath, p);
+				string_list_append(&suite.tests, progpath.buf);
 				break;
 			}
 	}

@@ -213,6 +223,7 @@ static int testsuite(int argc, const char **argv)
 
 	string_list_clear(&suite.tests, 0);
 	string_list_clear(&suite.failed, 0);
+	strbuf_release(&progpath);
 
 	return ret;
 }

2.45.0.rc0.197.gbae5840b3b-goog

In the previous commit, we added support in test-tool for running collections of unit tests. Now, add rules in t/Makefile for running in this way.

This new rule can be executed from the top-level Makefile via make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests, or by setting DEFAULT_UNIT_TEST_TARGET in config.mak.

---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

index ba55d817ee..b0d1f04b4d 100644
@@ -3870,5 +3870,5 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests

index 4861edafe6..0ae04f1e42 100644
@@ -49,6 +49,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 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

@@ -76,7 +77,7 @@ $(T):
 $(UNIT_TESTS):
 	@echo "*** $@ ***"; $@
 
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
 unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
 
 unit-tests-raw: $(UNIT_TESTS)

@@ -84,6 +85,13 @@ 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)'
 

2.45.0.rc0.197.gbae5840b3b-goog

From: Jeff King <[email protected]>

Add a wrapper script to allow prove to run both shell tests and unit tests from a single invocation. This avoids issues around running prove twice in CI, as discussed in [1].

Additionally, this moves the unit tests into the main dev workflow, so that errors can be spotted more quickly. Accordingly, we remove the separate unit tests step for Linux CI. (We leave the Windows CI unit-test step as-is, because the sharding scheme there involves selecting specific test files rather than running make test.)

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-build-and-tests.sh |  2 --
 t/Makefile                |  2 +-
 t/run-test.sh             | 18 ++++++++++++++++++
 3 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100755 t/run-test.sh

index 7a1466b868..2528f25e31 100755
@@ -50,8 +50,6 @@ if test -n "$run_tests"
 then
 	group "Run tests" make test ||
 	handle_failed_tests
-	group "Run unit tests" \
-		make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
 fi
 check_unignored_build_artifacts
 

index 0ae04f1e42..b2eb9f770b 100644
@@ -68,7 +68,7 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):

new file mode 100755
index 0000000000..0eabf42d69
@@ -0,0 +1,18 @@
+#!/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+set}"
+	then
+		echo >&2 "ERROR: TEST_SHELL_PATH is empty or not set"
+		exit 1
+	fi
+	exec ${TEST_SHELL_PATH} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac

2.45.0.rc0.197.gbae5840b3b-goog

Fixed in V6.

Although the previous commit changed t/Makefile to run unit tests alongside shell tests, the Windows CI still needs a separate unit-tests step due to how the test sharding works.

We want to avoid using prove as a test running on Windows due to performance issues [1], so use the new test-tool runner instead.

[1] https://lore.kernel.org/git/[email protected]/

---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ae8094382f..e167e646f7 100755
@@ -17,7 +17,7 @@ handle_failed_tests
 
 # We only have one unit test at the moment, so run it in the first slice
 if [ "$1" == "0" ] ; then
-	group "Run unit tests" make --quiet -C t unit-tests-prove
+	group "Run unit tests" make --quiet -C t unit-tests-test-tool
 fi
 
 check_unignored_build_artifacts

2.45.0.rc0.197.gbae5840b3b-goog

I am OK to see it outside the scope of this series, but we would need unit tests supported by the GIT_SKIP_TESTS mechanism (or an alternative mechanism written), given that I hear "migrate tests to unit-test framework" every once in a while, which means we would accumlate more and more tests that ignore GIT_SKIP_TESTS mechansim.

I did spot one potential problem (rather, "we'd want to fix it as we are changing it with this reroll anyway"), but other than that I did not see anything wrong in the other patches.

Thanks. Will queue.

Ack. I think the current GIT_SKIP_TESTS system should be fine. I'll put it on my TODO but can't promise that it will be a priority yet.

For various reasons (see discussion at [1]) we would like an alternative to prove for running test suites (including the unit tests) on Windows.

[1] https://lore.kernel.org/git/[email protected]/

This series extends the existing test-tool run-command testsuite to support running unit tests. In addition, it includes some small cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in test-tool ... testsuite
* don't hardcode a test name filter in test-tool ... testsuite
* add a test wrapper script to allow unit tests and the shell test suite to run in a single prove process

Changes in V4:
* Set TEST_SHELL_PATH when running the prove test target, and error out of run-test.sh if TEST_SHELL_PATH is not set.

Changes in V3:
* Added new patch (#7) to use the new test-tool runner for unit tests in Windows CI.
* Restored the explicit sort call in t/Makefile, for backwards compatibility with older GNU Make versions.
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Fixed header include order in patch #1.
* Removed a paragraph in patch #1's commit message that is obsolete now that we're building the list of test files from the sources rather than by globbing.
* Added a note in patch #2 that setting a NULL suite.shell_path will be used in a later commit.
* Clarified up some sloppy wording in commit messages and comments in t/helper/test-run-command.c.

Changes in V2:
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Patch 1: move t-basic to a test-tool subcommand rather than a new executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in test-tool run-command testsuite
* New patch 3: remove the hardcoded filename filter in test-tool run-command testsuite
* Patch 4 (previously 2): simplified now that we no longer need to add any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using the unit test list built in the makefile in jk/unit-tests-buildfix.

Jeff King (1):
t/Makefile: run unit tests alongside shell tests

Josh Steadmon (6):
t0080: turn t-basic unit test into a helper test-tool run-command testsuite: get shell from env test-tool run-command testsuite: remove hardcoded filter test-tool run-command testsuite: support unit tests unit tests: add rule for running with test-tool
ci: use test-tool as unit test runner on Windows

 Makefile                                      |  6 ++--
 ci/run-build-and-tests.sh                     |  2 --
 ci/run-test-slice.sh                          |  2 +-
 t/Makefile                                    | 14 ++++++++--
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-run-command.c                   | 28 +++++++++++++++----
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/run-test.sh                                 | 18 ++++++++++++
 t/t0080-unit-test-output.sh                   | 24 ++++++++--------
 10 files changed, 72 insertions(+), 29 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
 create mode 100755 t/run-test.sh

Range-diff against v5:

1:  6777451100 = 1:  6777451100 t0080: turn t-basic unit test into a helper
2:  24f47f8fc7 = 2:  24f47f8fc7 test-tool run-command testsuite: get shell from env
3:  4a16a3ec24 = 3:  4a16a3ec24 test-tool run-command testsuite: remove hardcoded filter
4:  abc9a7afe8 = 4:  abc9a7afe8 test-tool run-command testsuite: support unit tests
5:  a8bbff2c6b = 5:  a8bbff2c6b unit tests: add rule for running with test-tool
6:  c6606446c4 ! 6:  dae670fcb1 t/Makefile: run unit tests alongside shell tests
    @@ t/run-test.sh (new)
     +
     +case "$1" in
     +*.sh)
    -+  if test -z "${TEST_SHELL_PATH+set}"
    ++  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} "$@"
    ++  exec "${TEST_SHELL_PATH}" "$@"
     +  ;;
     +*)
     +  exec "$@"

7: 4a92131ab9 = 7: 2f5853a7fa ci: use test-tool as unit test runner on Windows

base-commit: 4904a4d08cc085716df12ce713ae7ee3d5ecb75a

2.45.0.rc1.225.g2a3ae87e7f-goog

While t/unit-tests/t-basic.c uses the unit-test framework added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is not a true unit test in that it intentionally fails in order to exercise various codepaths in the unit-test framework. Thus, we intentionally exclude it when running unit tests through the various t/Makefile targets. Instead, it is executed by t0080-unit-test-output.sh, which verifies its output follows the TAP format expected for the various pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for t0080-unit-test-output.sh, so let's move it to t/helper/test-example-tap.c and adjust Makefiles as necessary.

---
 Makefile                                      |  4 ++--
 t/Makefile                                    |  2 +-
 .../t-basic.c => helper/test-example-tap.c}   |  5 ++--
 t/helper/test-tool.c                          |  1 +
 t/helper/test-tool.h                          |  1 +
 t/t0080-unit-test-output.sh                   | 24 +++++++++----------
 6 files changed, 20 insertions(+), 17 deletions(-)
 rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)

index 23723367b8..ba55d817ee 100644
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
 TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o

@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype

@@ -3218,7 +3218,7 @@ perf: all
 
 .PRECIOUS: $(TEST_OBJS)
 
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

index 2d95046f26..4861edafe6 100644
@@ -48,7 +48,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
 
 # `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

similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "test-tool.h"
+#include "t/unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit

@@ -69,7 +70,7 @@ static void t_empty(void)
 	; /* empty */
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
 {
 	test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
 	TEST(t_res(1), "passing test and assertion return 1");

index 33b9501c21..bb5c04c9c0 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
+	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },

index b72f07ded9..38001bd1c6 100644
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);

index 961b54b06c..83b1e3b7f5 100755
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/helper/test-example-tap.c:77
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/helper/test-example-tap.c:31
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/helper/test-example-tap.c:49
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/helper/test-example-tap.c:57
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/helper/test-example-tap.c:92
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! test-tool example-tap >actual &&
 	test_cmp expect actual
 '
 

2.45.0.rc1.225.g2a3ae87e7f-goog

When running tests through test-tool run-command testsuite, we currently hardcode sh as the command interpreter. As discussed in [1], this is incorrect, and we should be using the shell set in TEST_SHELL_PATH instead.

Add a shell_path field in struct testsuite so that we can pass this to the task runner callback. If this is non-null, we'll use it as the argv[0] of the subprocess. Otherwise, we'll just execute the test program directly. We will use this feature in a later commit to enable running binary executable unit tests.

However, for now when setting up the struct testsuite in testsuite(), use the value of TEST_SHELL_PATH if it's set, otherwise keep the original behavior by defaulting to sh.

[1] https://lore.kernel.org/git/[email protected]/

---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index c0ed8722c8..a41a54d9cb 100644
@@ -65,6 +65,7 @@ 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, \