[RFC PATCH 0/4] test-tool: add unit test suite runner

[RFC PATCH 0/4] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/


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

[RFC PATCH 1/4] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 88ba7a3c51..ab32ec1101 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index b7a6fefe28..0bee7bc6ea 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..7431023d97 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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
 '
 
diff --git a/t/t0080/.gitignore b/t/t0080/.gitignore
new file mode 100644
index 0000000000..1903542827
--- /dev/null
+++ b/t/t0080/.gitignore
@@ -0,0 +1 @@
+/t-basic
diff --git a/t/unit-tests/t-basic.c b/t/t0080/t-basic.c
similarity index 98%
rename from t/unit-tests/t-basic.c
rename to t/t0080/t-basic.c
index fda1ae59a6..83727221b1 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/t0080/t-basic.c
@@ -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

Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> As such, it makes more sense for t-basic to be a helper item for
> t0080-unit-test-output.sh, ...

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

> ... so let's move it to t/t0080/t-basic.c and
> adjust Makefiles and .gitignores as necessary.

... 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?

> 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.

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.

Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper

[Jeff King]

On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:

> > ... so let's move it to t/t0080/t-basic.c and
> > adjust Makefiles and .gitignores as necessary.
> 
> ... 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.

Heh, I was about to ask the same thing.

In particular...

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

...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

Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

On 2024.01.22 19:43, Jeff King wrote:
> On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:
> 
> > > ... so let's move it to t/t0080/t-basic.c and
> > > adjust Makefiles and .gitignores as necessary.
> > 
> > ... 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.
> 
> Heh, I was about to ask the same thing.
> 
> In particular...
> 
> > Do we have any precedent to place programs placed under t/tXXXX/ and
> > get them compiled?
> 
> ...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.

[RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 40 +++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..2db7e1ef03 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

Makes perfect sense.

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

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.

> 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 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.

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Josh Steadmon <steadmon@google.com> writes:
>
>> 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.
>
> Makes perfect sense.

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.

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Jeff King]

On Tue, Jan 16, 2024 at 03:40:01PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Josh Steadmon <steadmon@google.com> writes:
> >
> >> 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.
> >
> > Makes perfect sense.
> 
> 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

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

On 2024.01.16 15:18, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > 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 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?

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

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> On 2024.01.16 15:18, Junio C Hamano wrote:
>> Josh Steadmon <steadmon@google.com> writes:
>> > 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 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?
>
> 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.

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

On 2024.01.16 15:18, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > Second "--(no-)require-shell-test-pattern" bypasses the check that the
> > test filenames match the expected t####-*.sh pattern.
> 
> 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.

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.

Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

On 2024.02.01 15:08, Josh Steadmon wrote:
> On 2024.01.16 15:18, Junio C Hamano wrote:
> > Josh Steadmon <steadmon@google.com> writes:
> > > Second "--(no-)require-shell-test-pattern" bypasses the check that the
> > > test filenames match the expected t####-*.sh pattern.
> > 
> > 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.
> 
> 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.

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 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.

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.

[RFC PATCH 3/4] unit tests: add rule for running with test-tool

[Josh Steadmon]

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)]

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   | 2 +-
 t/Makefile | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ab32ec1101..ce43ad2ae8 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 0bee7bc6ea..ad57ec0a41 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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

[RFC PATCH 4/4] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/Makefile    |  2 +-
 t/run-test.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100755 t/run-test.sh

diff --git a/t/Makefile b/t/Makefile
index ad57ec0a41..0038a25e33 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..c29fef48dc
--- /dev/null
+++ b/t/run-test.sh
@@ -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

Re: [RFC PATCH 0/4] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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`.

;-)

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

> * We should investigate switching the Windows CI to use `test-tool`
>   instead of prove.

OK.

> * 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.

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.

[RFC PATCH v2 0/6] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
         [2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
     
     
      ## 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

[RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index 281f4c3534..1283c90c10 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..21e4848e78 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -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");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> -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))

Nice that we no longer need the special casing.

> diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
> similarity index 95%
> rename from t/unit-tests/t-basic.c
> rename to t/helper/test-example-tap.c
> index fda1ae59a6..21e4848e78 100644
> --- a/t/unit-tests/t-basic.c
> +++ b/t/helper/test-example-tap.c
> @@ -1,4 +1,5 @@
> -#include "test-lib.h"
> +#include "t/unit-tests/test-lib.h"
> +#include "test-tool.h"

As the first thing both of these headers include is
"git-compat-util.h", so the ordering should be safe either way,
because everybody else in the directory seems to include
"test-tool.h" before including headers that are specific to the
subsystem it is testing, and "t/unit-tests/test-lib.h" in this case
is the header that is specific to the unit-test subsystem being
tested, it may raise fewer eyebrows if we swapped the order of the
inclusion here.

Other than that, looks good to me.

Thanks.

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > -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))
> 
> Nice that we no longer need the special casing.
> 
> > diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
> > similarity index 95%
> > rename from t/unit-tests/t-basic.c
> > rename to t/helper/test-example-tap.c
> > index fda1ae59a6..21e4848e78 100644
> > --- a/t/unit-tests/t-basic.c
> > +++ b/t/helper/test-example-tap.c
> > @@ -1,4 +1,5 @@
> > -#include "test-lib.h"
> > +#include "t/unit-tests/test-lib.h"
> > +#include "test-tool.h"
> 
> As the first thing both of these headers include is
> "git-compat-util.h", so the ordering should be safe either way,
> because everybody else in the directory seems to include
> "test-tool.h" before including headers that are specific to the
> subsystem it is testing, and "t/unit-tests/test-lib.h" in this case
> is the header that is specific to the unit-test subsystem being
> tested, it may raise fewer eyebrows if we swapped the order of the
> inclusion here.
> 
> Other than that, looks good to me.
> 
> Thanks.

Fixed in V3.

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Jeff King]

On Fri, Feb 02, 2024 at 04:50:26PM -0800, Josh Steadmon wrote:

> 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.

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/.

> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -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))

This drops the intermediate UNIT_TEST_PROGRAMS, which makes sense. It
was only used to keep the long lines a bit more readable. But it also
drops the $(sort) call. Do we need to keep it?

Certainly I'd think we want the contents of $(UNIT_TESTS) to be in a
deterministic order. Does the $(wildcard) function already return things
in sorted order? I can't find any mention in the documention. It seems
to do so for me in a simple test, but aae5239be2 (t/Makefile: Use $(sort
...) explicitly where needed, 2011-09-04) argues otherwise.

So I think we probably want to keep it (or possibly move it onto the
UNIT_TEST_SOURCES line, which keeps it close to the wildcard call).

-Peff

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I think we probably want to keep it (or possibly move it onto the
> UNIT_TEST_SOURCES line, which keeps it close to the wildcard call).

Sensible.

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

On 2024.02.07 17:58, Jeff King wrote:
> On Fri, Feb 02, 2024 at 04:50:26PM -0800, Josh Steadmon wrote:
> 
> > 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.
> 
> 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/.

Right, thanks for the catch. Removed in V3.


> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -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))
> 
> This drops the intermediate UNIT_TEST_PROGRAMS, which makes sense. It
> was only used to keep the long lines a bit more readable. But it also
> drops the $(sort) call. Do we need to keep it?
> 
> Certainly I'd think we want the contents of $(UNIT_TESTS) to be in a
> deterministic order. Does the $(wildcard) function already return things
> in sorted order? I can't find any mention in the documention. It seems
> to do so for me in a simple test, but aae5239be2 (t/Makefile: Use $(sort
> ...) explicitly where needed, 2011-09-04) argues otherwise.

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

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.""").

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Jeff King]

On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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

RE: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[rsbecker]

On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
>On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>
>> Josh Steadmon <steadmon@google.com> writes:
>>
>> > 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).

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

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

On 2024.02.13 09:36, rsbecker@nexbridge.com wrote:
> On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
> >On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
> >
> >> Josh Steadmon <steadmon@google.com> writes:
> >>
> >> > 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).
> 
> 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)

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?

RE: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[rsbecker]

On Thursday, February 22, 2024 6:58 PM, Josh Steadmon wrote:
>On 2024.02.13 09:36, rsbecker@nexbridge.com wrote:
>> On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
>> >On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>> >
>> >> Josh Steadmon <steadmon@google.com> writes:
>> >>
>> >> > 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-Funct
>> >> > ion .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).
>>
>> 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)
>
>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?

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

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).

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

Thanks.

Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

On 2024.02.13 02:41, Jeff King wrote:
> On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
> 
> > Josh Steadmon <steadmon@google.com> writes:
> > 
> > > 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).

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

[RFC PATCH v2 2/6] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

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/20240123005913.GB835964@coredump.intra.peff.net/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

Re: [RFC PATCH v2 2/6] test-tool run-command testsuite: get shell from env

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

That sounds nice in theory, but ...

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

... 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.

Re: [RFC PATCH v2 2/6] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> That sounds nice in theory, but ...
> 
> > When setting up the struct testsuite in testsuite(), use the value
> > of TEST_SHELL_PATH if it's set, otherwise default to `sh`.
> 
> ... 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.

[RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

`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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

Re: [RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> `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.

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

> 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.

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.

> [1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  t/helper/test-run-command.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index a41a54d9cb..e6bd792274 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -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 */

Re: [RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > `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.
> 
> Makes sense to explain what future steps this prepares the codebase
> for like this.
> 
> > 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.
> 
> 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?

Re: [RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

>> 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.

[RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..a0b8dc6fd7 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests

[phillip.wood123]

Hi Josh

On 03/02/2024 00:50, Josh Steadmon wrote:
> 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.

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

> 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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
> [2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
> 
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>   t/helper/test-run-command.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index e6bd792274..a0b8dc6fd7 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -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;
>   }

Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

On 2024.02.05 16:16, phillip.wood123@gmail.com wrote:
> Hi Josh
> 
> On 03/02/2024 00:50, Josh Steadmon wrote:
> > 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.
> 
> 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...

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.

Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

> +	/*
> +	 * If we run without a shell, we have to provide the relative path to
> +	 * the executables.
> +	 */

It sounds more like "If TEST_SHELL_PATH is not given, then we run
them in the current directory.".

It is perfectly fine, because ...

>  	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)
> ...
>  		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;
>  			}
>  	}

... this "prefixing" is done to a path discovered by readdir() from
a directory handle obtained by opendir(".").  If there were a way to
pass paths to executables directly, possibly as absolute paths, the
unconditional prefixing of "./" would have been a problem, but we do
not have such a facility, so this should be OK.

Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

On 2024.02.07 12:48, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> 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.
> 
> > +	/*
> > +	 * If we run without a shell, we have to provide the relative path to
> > +	 * the executables.
> > +	 */
> 
> It sounds more like "If TEST_SHELL_PATH is not given, then we run
> them in the current directory.".

Reworded both of these in V3.

[RFC PATCH v2 5/6] unit tests: add rule for running with test-tool

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 1283c90c10..6e6316c29b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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

Re: [RFC PATCH v2 5/6] unit tests: add rule for running with test-tool

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +unit-tests-test-tool:
> +	@echo "*** test-tool - unit tests **"
> +	( \
> +		cd unit-tests/bin && \
> +		../../helper/test-tool$X run-command testsuite $(UNIT_TESTS_NO_DIR)\
> +	)

This limits us to the tests that the Makefile knows about, which is
good.  Of course, we might bust the command line length limit some
day when we have too many unit test executables, but for now this is
simple, clean, and readable.

Thanks.

[RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/Makefile    |  2 +-
 t/run-test.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100755 t/run-test.sh

diff --git a/t/Makefile b/t/Makefile
index 6e6316c29b..6a67fc22d7 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..c29fef48dc
--- /dev/null
+++ b/t/run-test.sh
@@ -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

Re: [RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> From: Jeff King <peff@peff.net>
>
> 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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  t/Makefile    |  2 +-
>  t/run-test.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>  create mode 100755 t/run-test.sh
>
> diff --git a/t/Makefile b/t/Makefile
> index 6e6316c29b..6a67fc22d7 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -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):
> diff --git a/t/run-test.sh b/t/run-test.sh
> new file mode 100755
> index 0000000000..c29fef48dc
> --- /dev/null
> +++ b/t/run-test.sh
> @@ -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

Hmph.  This penalizes the non-unit tests by doing an extra "exec",
once per program?

Of course we cannot run two $(PROVE) invocations serially, one for
doing $(T) and the other for doing $(UNIT_TESTS)?

Re: [RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests

[Jeff King]

On Wed, Feb 07, 2024 at 12:55:09PM -0800, Junio C Hamano wrote:

> > +# 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
> 
> Hmph.  This penalizes the non-unit tests by doing an extra "exec",
> once per program?

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).

> Of course we cannot run two $(PROVE) invocations serially, one for
> doing $(T) and the other for doing $(UNIT_TESTS)?

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

Re: [RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

Re: [RFC PATCH v2 0/6] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

> * 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.

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.

Re: [RFC PATCH v2 0/6] test-tool: add unit test suite runner

[Josh Steadmon]

On 2024.02.03 10:52, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > * 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.
> 
> 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.

Re: [RFC PATCH v2 0/6] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

[PATCH v3 0/7] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

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/20240123005913.GB835964@coredump.intra.peff.net/
     
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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    + ## 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

[PATCH v3 1/7] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index 2d95046f26..4861edafe6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -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");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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

[PATCH v3 2/7] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

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/20240123005913.GB835964@coredump.intra.peff.net/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v3 3/7] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

`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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v3 4/7] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..61eb1175fe 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v3 5/7] unit tests: add rule for running with test-tool

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 4861edafe6..0ae04f1e42 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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

[PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..2528f25e31 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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
 
diff --git a/t/Makefile b/t/Makefile
index 0ae04f1e42..0c739754d8 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..c29fef48dc
--- /dev/null
+++ b/t/run-test.sh
@@ -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

Re: [PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests

[Jeff King]

On Fri, Feb 23, 2024 at 03:33:55PM -0800, Josh Steadmon wrote:

> diff --git a/t/run-test.sh b/t/run-test.sh
> new file mode 100755
> index 0000000000..c29fef48dc
> --- /dev/null
> +++ b/t/run-test.sh
> @@ -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

An earlier step required that runs via "test-tool run-command" have
TEST_SHELL_PATH set correctly. So defaulting to /bin/sh here is
pointless, I'd think? This is used only for the in-Makefile "prove"
invocation, so running individual tests or even a manual "prove" outside
of the Makefile (where the user might not have set TEST_SHELL_PATH)
would not apply.

It obviously is not hurting anything, but I wonder if you'd want to have
it complain loudly to catch any instance where your assumption is not
true.

-Peff

Re: [PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

On 2024.03.27 04:58, Jeff King wrote:
> On Fri, Feb 23, 2024 at 03:33:55PM -0800, Josh Steadmon wrote:
> 
> > diff --git a/t/run-test.sh b/t/run-test.sh
> > new file mode 100755
> > index 0000000000..c29fef48dc
> > --- /dev/null
> > +++ b/t/run-test.sh
> > @@ -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
> 
> An earlier step required that runs via "test-tool run-command" have
> TEST_SHELL_PATH set correctly. So defaulting to /bin/sh here is
> pointless, I'd think? This is used only for the in-Makefile "prove"
> invocation, so running individual tests or even a manual "prove" outside
> of the Makefile (where the user might not have set TEST_SHELL_PATH)
> would not apply.

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.

> It obviously is not hurting anything, but I wonder if you'd want to have
> it complain loudly to catch any instance where your assumption is not
> true.
> 
> -Peff

Re: [PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests

[Jeff King]

On Thu, Apr 11, 2024 at 11:44:09AM -0700, Josh Steadmon wrote:

> > An earlier step required that runs via "test-tool run-command" have
> > TEST_SHELL_PATH set correctly. So defaulting to /bin/sh here is
> > pointless, I'd think? This is used only for the in-Makefile "prove"
> > invocation, so running individual tests or even a manual "prove" outside
> > of the Makefile (where the user might not have set TEST_SHELL_PATH)
> > would not apply.
> 
> 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

Re: [PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

On 2024.04.12 00:29, Jeff King wrote:
> On Thu, Apr 11, 2024 at 11:44:09AM -0700, Josh Steadmon wrote:
> 
> > > An earlier step required that runs via "test-tool run-command" have
> > > TEST_SHELL_PATH set correctly. So defaulting to /bin/sh here is
> > > pointless, I'd think? This is used only for the in-Makefile "prove"
> > > invocation, so running individual tests or even a manual "prove" outside
> > > of the Makefile (where the user might not have set TEST_SHELL_PATH)
> > > would not apply.
> > 
> > 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.

[PATCH v3 7/7] ci: use test-tool as unit test runner on Windows

[Josh Steadmon]

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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index ae8094382f..e167e646f7 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -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

Re: [PATCH v3 0/7] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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.

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.

Re: [PATCH v3 0/7] test-tool: add unit test suite runner

[Jeff King]

On Tue, Mar 26, 2024 at 02:33:16PM -0700, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.
> 
> 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".

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

[PATCH v4 0/7] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

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

[PATCH v4 1/7] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index 2d95046f26..4861edafe6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -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");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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

[PATCH v4 2/7] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

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/20240123005913.GB835964@coredump.intra.peff.net/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v4 3/7] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

`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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v4 4/7] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..61eb1175fe 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v4 5/7] unit tests: add rule for running with test-tool

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 4861edafe6..0ae04f1e42 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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

[PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..2528f25e31 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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
 
diff --git a/t/Makefile b/t/Makefile
index 0ae04f1e42..b2eb9f770b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..a0e2dce5e0
--- /dev/null
+++ b/t/run-test.sh
@@ -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

Re: [PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +case "$1" in
> +*.sh)
> +	if test -z "${TEST_SHELL_PATH+set}" ; then
> +		echo "ERROR: TEST_SHELL_PATH is not set" >&2

Style.

As an empty string is not a reasonable value for this variable (and
you do not quote ${TEST_SHELL_PATH} when you use it in "exec" below),

	if test -z "${TEST_SHELL_PATH:+set}"
	then
		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"

may be what we want here.

> +		exit 1
> +	fi
> +	exec ${TEST_SHELL_PATH} "$@"
> +	;;
> +*)
> +	exec "$@"
> +	;;
> +esac

Other than that, the update in this iteration looks reasonable to
me.

Thanks.

Re: [PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

On 2024.04.24 14:25, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > +case "$1" in
> > +*.sh)
> > +	if test -z "${TEST_SHELL_PATH+set}" ; then
> > +		echo "ERROR: TEST_SHELL_PATH is not set" >&2
> 
> Style.
> 
> As an empty string is not a reasonable value for this variable (and
> you do not quote ${TEST_SHELL_PATH} when you use it in "exec" below),
> 
> 	if test -z "${TEST_SHELL_PATH:+set}"
> 	then
> 		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"
> 
> may be what we want here.
> 
> > +		exit 1
> > +	fi
> > +	exec ${TEST_SHELL_PATH} "$@"
> > +	;;
> > +*)
> > +	exec "$@"
> > +	;;
> > +esac
> 
> Other than that, the update in this iteration looks reasonable to
> me.
> 
> Thanks.

Fixed in V5, thanks.

Re: [PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Jeff King]

On Wed, Apr 24, 2024 at 02:25:44PM -0700, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
> 
> > +case "$1" in
> > +*.sh)
> > +	if test -z "${TEST_SHELL_PATH+set}" ; then
> > +		echo "ERROR: TEST_SHELL_PATH is not set" >&2
> 
> Style.
> 
> As an empty string is not a reasonable value for this variable (and
> you do not quote ${TEST_SHELL_PATH} when you use it in "exec" below),
> 
> 	if test -z "${TEST_SHELL_PATH:+set}"
> 	then
> 		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"
> 
> may be what we want here.

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

Re: [PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 	if test -z "${TEST_SHELL_PATH:+set}"
>> 	then
>> 		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"
>> 
>> may be what we want here.
>
> If we are using ":+" to handle the empty string, I think just:
>
>   if test -z "$TEST_SHELL_PATH"
>
> is sufficient, no?

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

> +		exit 1
> +	fi
> +	exec ${TEST_SHELL_PATH} "$@"
> +	;;

the above reference needs to be quoted protect $IFS in the path.

Re: [PATCH v4 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

On 2024.05.03 14:02, Jeff King wrote:
> On Wed, Apr 24, 2024 at 02:25:44PM -0700, Junio C Hamano wrote:
> 
> > Josh Steadmon <steadmon@google.com> writes:
> > 
> > > +case "$1" in
> > > +*.sh)
> > > +	if test -z "${TEST_SHELL_PATH+set}" ; then
> > > +		echo "ERROR: TEST_SHELL_PATH is not set" >&2
> > 
> > Style.
> > 
> > As an empty string is not a reasonable value for this variable (and
> > you do not quote ${TEST_SHELL_PATH} when you use it in "exec" below),
> > 
> > 	if test -z "${TEST_SHELL_PATH:+set}"
> > 	then
> > 		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"
> > 
> > may be what we want here.
> 
> 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

Fixed in V6.

[PATCH v4 7/7] ci: use test-tool as unit test runner on Windows

[Josh Steadmon]

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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index ae8094382f..e167e646f7 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -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

[PATCH v5 0/7] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

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

[PATCH v5 1/7] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index 2d95046f26..4861edafe6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -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");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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

[PATCH v5 2/7] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

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/20240123005913.GB835964@coredump.intra.peff.net/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v5 3/7] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

`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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v5 4/7] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..61eb1175fe 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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

[PATCH v5 5/7] unit tests: add rule for running with test-tool

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 4861edafe6..0ae04f1e42 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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

[PATCH v5 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..2528f25e31 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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
 
diff --git a/t/Makefile b/t/Makefile
index 0ae04f1e42..b2eb9f770b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..0eabf42d69
--- /dev/null
+++ b/t/run-test.sh
@@ -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

Re: [PATCH v5 6/7] t/Makefile: run unit tests alongside shell tests

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +# 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} "$@"

You want to be prepared for a path like "//C/My Programs/bash".

	exec "$TEST_SHELL_PATH" "$@"

> +	;;
> +*)
> +	exec "$@"
> +	;;
> +esac

Re: [PATCH v5 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

On 2024.04.30 14:05, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > +# 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} "$@"
> 
> You want to be prepared for a path like "//C/My Programs/bash".
> 
> 	exec "$TEST_SHELL_PATH" "$@"
> 
> > +	;;
> > +*)
> > +	exec "$@"
> > +	;;
> > +esac

Fixed in V6.

[PATCH v5 7/7] ci: use test-tool as unit test runner on Windows

[Josh Steadmon]

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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index ae8094382f..e167e646f7 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -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

Re: [PATCH v5 0/7] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> 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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
>
> 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

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.

Re: [PATCH v5 0/7] test-tool: add unit test suite runner

[Josh Steadmon]

On 2024.04.30 14:15, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
> >
> > 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
> 
> 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.

[PATCH v6 0/7] test-tool: add unit test suite runner

[Josh Steadmon]

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

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

[PATCH v6 1/7] t0080: turn t-basic unit test into a helper

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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%)

diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -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)
diff --git a/t/Makefile b/t/Makefile
index 2d95046f26..4861edafe6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -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");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -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

[PATCH v6 2/7] test-tool run-command testsuite: get shell from env

[Josh Steadmon]

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/20240123005913.GB835964@coredump.intra.peff.net/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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.rc1.225.g2a3ae87e7f-goog

[PATCH v6 3/7] test-tool run-command testsuite: remove hardcoded filter

[Josh Steadmon]

`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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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.rc1.225.g2a3ae87e7f-goog

[PATCH v6 4/7] test-tool run-command testsuite: support unit tests

[Josh Steadmon]

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/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/helper/test-run-command.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..61eb1175fe 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -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.rc1.225.g2a3ae87e7f-goog

[PATCH v6 5/7] unit tests: add rule for running with test-tool

[Josh Steadmon]

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.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile   |  2 +-
 t/Makefile | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/Makefile b/t/Makefile
index 4861edafe6..0ae04f1e42 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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.rc1.225.g2a3ae87e7f-goog

[PATCH v6 6/7] t/Makefile: run unit tests alongside shell tests

[Josh Steadmon]

From: Jeff King <peff@peff.net>

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/pull.1613.git.1699894837844.gitgitgadget@gmail.com/

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 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

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..2528f25e31 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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
 
diff --git a/t/Makefile b/t/Makefile
index 0ae04f1e42..b2eb9f770b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -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):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..13c353b91b
--- /dev/null
+++ b/t/run-test.sh
@@ -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}"
+	then
+		echo >&2 "ERROR: TEST_SHELL_PATH is empty or not set"
+		exit 1
+	fi
+	exec "${TEST_SHELL_PATH}" "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v6 7/7] ci: use test-tool as unit test runner on Windows

[Josh Steadmon]

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/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 ci/run-test-slice.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index ae8094382f..e167e646f7 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -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.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v6 0/7] test-tool: add unit test suite runner

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

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

As I got tired of keeping that "Windows" bit outside this series as
a separate and dependent topic, let me see if I can get rid of
js/cmake-with-test-tool topic by queuing it at the tip of these 7
patches, making it an 8-patch series.  It looks like these 7 patches
are pretty well done, so let's plan to merge them together down to
'next' soonish.

Thanks.