diff mbox series

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

Message ID cfcc4bd427318fed1cacc8457381d5a0c408460a.1708728717.git.steadmon@google.com (mailing list archive)
State New, archived
Headers show
Series test-tool: add unit test suite runner | expand

Commit Message

Josh Steadmon Feb. 23, 2024, 11:33 p.m. UTC 
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

Comments

Jeff King March 27, 2024, 8:58 a.m. UTC | #1 
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
Josh Steadmon April 11, 2024, 6:44 p.m. UTC | #2 
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
Jeff King April 12, 2024, 4:29 a.m. UTC | #3 
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
Josh Steadmon April 24, 2024, 6:57 p.m. UTC | #4 
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.
diff mbox series

Patch

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