diff mbox series

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

Message ID 0e32de1afe9cbab02c5d3476a0fc2a1ba0151dcf.1713985716.git.steadmon@google.com (mailing list archive)
State Superseded
Headers show
Series test-tool: add unit test suite runner | expand

Commit Message

Josh Steadmon April 24, 2024, 7:14 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             | 17 +++++++++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100755 t/run-test.sh

Comments

Junio C Hamano April 24, 2024, 9:25 p.m. UTC | #1 
Josh Steadmon <steadmon@google.com> writes:

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

Style.

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

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

may be what we want here.

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

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

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

Fixed in V5, thanks.
Jeff King May 3, 2024, 6:02 p.m. UTC | #3 
On Wed, Apr 24, 2024 at 02:25:44PM -0700, Junio C Hamano wrote:

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

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

  if test -z "$TEST_SHELL_PATH"

is sufficient, no?

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

-Peff
Junio C Hamano May 3, 2024, 7:17 p.m. UTC | #4 
Jeff King <peff@peff.net> writes:

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

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

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

the above reference needs to be quoted protect $IFS in the path.
Josh Steadmon May 6, 2024, 7:58 p.m. UTC | #5 
On 2024.05.03 14:02, Jeff King wrote:
> On Wed, Apr 24, 2024 at 02:25:44PM -0700, Junio C Hamano wrote:
> 
> > Josh Steadmon <steadmon@google.com> writes:
> > 
> > > +case "$1" in
> > > +*.sh)
> > > +	if test -z "${TEST_SHELL_PATH+set}" ; then
> > > +		echo "ERROR: TEST_SHELL_PATH is not set" >&2
> > 
> > Style.
> > 
> > As an empty string is not a reasonable value for this variable (and
> > you do not quote ${TEST_SHELL_PATH} when you use it in "exec" below),
> > 
> > 	if test -z "${TEST_SHELL_PATH:+set}"
> > 	then
> > 		echo >&2 "ERROR: TEST_SHELL_PATH is not set or empty"
> > 
> > may be what we want here.
> 
> If we are using ":+" to handle the empty string, I think just:
> 
>   if test -z "$TEST_SHELL_PATH"
> 
> is sufficient, no?
> 
> (not that the other is incorrect, but whenever I see something like
> ":+set" I wonder if something more clever is going on, and of course I
> get nightmare flashbacks to looking at generated autoconf code).
> 
> -Peff

Fixed in V6.
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..b2eb9f770b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -68,7 +68,7 @@  failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean check-chainlint $(TEST_LINT)
-	@echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..a0e2dce5e0
--- /dev/null
+++ b/t/run-test.sh
@@ -0,0 +1,17 @@ 
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+	if test -z "${TEST_SHELL_PATH+set}" ; then
+		echo "ERROR: TEST_SHELL_PATH is not set" >&2
+		exit 1
+	fi
+	exec ${TEST_SHELL_PATH} "$@"
+	;;
+*)
+	exec "$@"
+	;;
+esac