| Message ID | cd7467a7bd51fbc01c999ee1bd7688770b1d11e5.1706921262.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-tool: add unit test suite runner | expand |
Josh Steadmon <steadmon@google.com> writes: > From: Jeff King <peff@peff.net> > > Add a wrapper script to allow `prove` to run both shell tests and unit > tests from a single invocation. This avoids issues around running prove > twice in CI, as discussed in [1]. > > Additionally, this moves the unit tests into the main dev workflow, so > that errors can be spotted more quickly. > > NEEDS WORK: as discussed in previous commits in this series, there's a > desire to avoid `prove` specifically and (IIUC) unnecessary > fork()/exec()ing in general on Windows. This change adds an extra exec() > for each shell and unit test execution, will that be a problem for > Windows? > > [1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/ > > Signed-off-by: Jeff King <peff@peff.net> > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > t/Makefile | 2 +- > t/run-test.sh | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > create mode 100755 t/run-test.sh > > diff --git a/t/Makefile b/t/Makefile > index 6e6316c29b..6a67fc22d7 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -64,7 +64,7 @@ failed: > test -z "$$failed" || $(MAKE) $$failed > > prove: pre-clean check-chainlint $(TEST_LINT) > - @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) > + @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS) > $(MAKE) clean-except-prove-cache > > $(T): > diff --git a/t/run-test.sh b/t/run-test.sh > new file mode 100755 > index 0000000000..c29fef48dc > --- /dev/null > +++ b/t/run-test.sh > @@ -0,0 +1,13 @@ > +#!/bin/sh > + > +# A simple wrapper to run shell tests via TEST_SHELL_PATH, > +# or exec unit tests directly. > + > +case "$1" in > +*.sh) > + exec ${TEST_SHELL_PATH:-/bin/sh} "$@" > + ;; > +*) > + exec "$@" > + ;; > +esac Hmph. This penalizes the non-unit tests by doing an extra "exec", once per program? Of course we cannot run two $(PROVE) invocations serially, one for doing $(T) and the other for doing $(UNIT_TESTS)?
On Wed, Feb 07, 2024 at 12:55:09PM -0800, Junio C Hamano wrote: > > +# A simple wrapper to run shell tests via TEST_SHELL_PATH, > > +# or exec unit tests directly. > > + > > +case "$1" in > > +*.sh) > > + exec ${TEST_SHELL_PATH:-/bin/sh} "$@" > > + ;; > > +*) > > + exec "$@" > > + ;; > > +esac > > Hmph. This penalizes the non-unit tests by doing an extra "exec", > once per program? It does, but IMHO that is not likely to be a problem. It's once per top-level script (so ~1000), and each of those scripts spawns hundreds or thousands of sub-commands. I didn't do any measurements, though. You can extend "prove" with extra perl modules so that it makes the distinction internally without the extra shell invocation. But when I tried to do it, I found it rather baroque and complicated (I can't remember if I succeeded but found it too gross, or just gave up halfway through trying). > Of course we cannot run two $(PROVE) invocations serially, one for > doing $(T) and the other for doing $(UNIT_TESTS)? Not if they share the same command-line options. If you use something like "--state=slow,save", then the first run will write the list of all tests to ".prove", and then the second will run every test mentioned in .prove (in addition to the unit-tests provided on the command-line). You should be able to work around it by passing "--statefile". I _think_ it might be OK to just do that unconditionally. Something like: prove --exec $(TEST_SHELL_PATH $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) prove --statefile=.prove-unit-tests $(GIT_PROVE_OPTS) $(UNIT_TESTS) :: $(GIT_TEST_OPTS) and then it's just a noop if GIT_PROVE_OPTS doesn't use --state. But I haven't played with it myself. -Peff
Jeff King <peff@peff.net> writes: > Not if they share the same command-line options. If you use something > like "--state=slow,save", then the first run will write the list of all > tests to ".prove", and then the second will run every test mentioned in > .prove (in addition to the unit-tests provided on the command-line). > > You should be able to work around it by passing "--statefile". I _think_ > it might be OK to just do that unconditionally. Something like: > > prove --exec $(TEST_SHELL_PATH $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) > prove --statefile=.prove-unit-tests $(GIT_PROVE_OPTS) $(UNIT_TESTS) :: $(GIT_TEST_OPTS) > > and then it's just a noop if GIT_PROVE_OPTS doesn't use --state. But I > haven't played with it myself. I do not think it warrants such complexity. The wrapper script is fine.
diff --git a/t/Makefile b/t/Makefile index 6e6316c29b..6a67fc22d7 100644 --- a/t/Makefile +++ b/t/Makefile @@ -64,7 +64,7 @@ failed: test -z "$$failed" || $(MAKE) $$failed prove: pre-clean check-chainlint $(TEST_LINT) - @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) + @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS) $(MAKE) clean-except-prove-cache $(T): diff --git a/t/run-test.sh b/t/run-test.sh new file mode 100755 index 0000000000..c29fef48dc --- /dev/null +++ b/t/run-test.sh @@ -0,0 +1,13 @@ +#!/bin/sh + +# A simple wrapper to run shell tests via TEST_SHELL_PATH, +# or exec unit tests directly. + +case "$1" in +*.sh) + exec ${TEST_SHELL_PATH:-/bin/sh} "$@" + ;; +*) + exec "$@" + ;; +esac