| Message ID | cfcc4bd427318fed1cacc8457381d5a0c408460a.1708728717.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | test-tool: add unit test suite runner | expand |
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
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
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
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 --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