RFC patch, 4 partstest-tool: add unit test suite runner

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

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

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

[1] https://lore.kernel.org/git/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

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

Josh Steadmon <steadmon@google.com> writes:

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

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

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

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

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

Heh, I was about to ask the same thing.

In particular...

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

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

-Peff

On 2024.01.22 19:43, Jeff King wrote:

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

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

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

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

With these changes, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite --no-run-in-shell \
    --no-require-shell-test-pattern

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

[1] https://lore.kernel.org/git/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(-)

Josh Steadmon <steadmon@google.com> writes:

Makes perfect sense.

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

	... goes and looks ...

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

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

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

But that is orthogonal to the enhancement we have here.

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

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.

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

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

On 2024.01.16 15:18, Junio C Hamano wrote:

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

Josh Steadmon <steadmon@google.com> writes:

OK.

On 2024.01.16 15:18, Junio C Hamano wrote:

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.

On 2024.02.01 15:08, Josh Steadmon wrote:

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

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

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

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

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

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

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

Josh Steadmon <steadmon@google.com> writes:

;-)

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

OK.

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

Thanks for an entertaining read.

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

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

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

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

[1] https://lore.kernel.org/git/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

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

Josh Steadmon <steadmon@google.com> writes:

Nice that we no longer need the special casing.

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.

On 2024.02.07 12:55, Junio C Hamano wrote:

Fixed in V3.

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

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

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

Jeff King <peff@peff.net> writes:

Sensible.

On 2024.02.07 17:58, Jeff King wrote:

Right, thanks for the catch. Removed in V3.

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

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

Josh Steadmon <steadmon@google.com> writes:

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

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

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

On Tuesday, February 13, 2024 2:41 AM, Peff wrote:

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

On 2024.02.13 09:36, rsbecker@nexbridge.com wrote:

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?

On Thursday, February 22, 2024 6:58 PM, Josh Steadmon wrote:

Makefile in a

becomes

problems in

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

Jeff King <peff@peff.net> writes:

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

Thanks.

On 2024.02.13 02:41, Jeff King wrote:

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

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

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

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

[1] https://lore.kernel.org/git/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(-)

Josh Steadmon <steadmon@google.com> writes:

That sounds nice in theory, but ...

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

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

On 2024.02.07 12:55, Junio C Hamano wrote:

Reworded in V3.

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

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

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

[1] https://lore.kernel.org/git/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(-)

Josh Steadmon <steadmon@google.com> writes:

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

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

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

On 2024.02.07 12:55, Junio C Hamano wrote:

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

Josh Steadmon <steadmon@google.com> writes:

No preference either way at all.

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

With this change, you can now use test-tool to run the unit tests: $ make $ cd t/unit-tests/bin $ ../../helper/test-tool run-command testsuite

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

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

[1] https://lore.kernel.org/git/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(-)

Hi Josh

On 03/02/2024 00:50, Josh Steadmon wrote:

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

On 2024.02.05 16:16, phillip.wood123@gmail.com wrote:

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.

Josh Steadmon <steadmon@google.com> writes:

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.

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

It is perfectly fine, because ...

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

On 2024.02.07 12:48, Junio C Hamano wrote:

Reworded both of these in V3.

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

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

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

Josh Steadmon <steadmon@google.com> writes:

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.

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

Josh Steadmon <steadmon@google.com> writes:

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

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

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

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

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

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

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

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

-Peff

Jeff King <peff@peff.net> writes:

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

Josh Steadmon <steadmon@google.com> writes:

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

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

On 2024.02.03 10:52, Junio C Hamano wrote:

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.

Josh Steadmon <steadmon@google.com> writes:

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

Thanks.

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

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

[1] https://lore.kernel.org/git/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

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

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

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

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

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

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

On Fri, Feb 23, 2024 at 03:33:55PM -0800, 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.

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

On 2024.03.27 04:58, Jeff King wrote:

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.

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

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

On 2024.04.12 00:29, Jeff King wrote:

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

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

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

[1] https://lore.kernel.org/git/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(-)

Josh Steadmon <steadmon@google.com> writes:

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.

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

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

-Peff

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

[1] https://lore.kernel.org/git/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

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

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

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

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

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

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

Josh Steadmon <steadmon@google.com> writes:

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.

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

Thanks.

On 2024.04.24 14:25, Junio C Hamano wrote:

Fixed in V5, thanks.

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

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

Jeff King <peff@peff.net> writes:

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

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

On 2024.05.03 14:02, Jeff King wrote:

Fixed in V6.

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

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

[1] https://lore.kernel.org/git/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(-)

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

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

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

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

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

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

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

Josh Steadmon <steadmon@google.com> writes:

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

	exec "$TEST_SHELL_PATH" "$@"

On 2024.04.30 14:05, Junio C Hamano wrote:

Fixed in V6.

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

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

[1] https://lore.kernel.org/git/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(-)

Josh Steadmon <steadmon@google.com> writes:

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.

On 2024.04.30 14:15, Junio C Hamano wrote:

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

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

[1] https://lore.kernel.org/git/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

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

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

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

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

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

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

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

Josh Steadmon <steadmon@google.com> writes:

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.