Re: [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
Hi Junio, On Fri, 27 Jan 2017, Junio C Hamano wrote: > Johannes Schindelin writes: > > > This patch automates the process of determining which tests failed > > previously and re-running them. > > ... > > > > Signed-off-by: Johannes Schindelin > > I stored both versions in files and compared them, and it seems the > single word change in the proposed commit log message is the only > difference. I would have written "Automate the process...", though. Yes, we have different styles. Thanks for letting my commit keep my commit message this time ;-) > If you are resending, touching up to cover all points raised by a > reviewer and doing nothing else, having "Reviewed-by: Jeff King > " would have been nicer. TBH I am not at all sure that I know when to add those footers and when not. After having been asked to remove such a footer, I decided to *not* include them by default. Having gray zones about the footers strikes me as similar to having gray zones in the coding style guidelines: it sure gives the contributors more freedom, but it also creates uncertainty and as a consequence takes up a lot of reviewing space and time (hence taking away space and time from reviewing the code for bugs). In other words: while I appreciate the idea of giving contributors such as myself a lot of leeway, I would love even more to be able to automate away tedious and boring tasks (such as adding Tested-by: or Reviewed-by: footers, or for that matter, addressing code style issues before any reviewer has to shed bikes so that they can focus on the parts of the review that no machine can do for them). Ciao, Johannes
Re: [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
Johannes Schindelin writes: > This patch automates the process of determining which tests failed > previously and re-running them. > ... > > Signed-off-by: Johannes Schindelin I stored both versions in files and compared them, and it seems the single word change in the proposed commit log message is the only difference. I would have written "Automate the process...", though. If you are resending, touching up to cover all points raised by a reviewer and doing nothing else, having "Reviewed-by: Jeff King " would have been nicer. Will queue. Thanks. > --- > Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v4 > Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v4 > > t/Makefile | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/t/Makefile b/t/Makefile > index d613935f14..1bb06c36f2 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -35,6 +35,12 @@ all: $(DEFAULT_TEST_TARGET) > test: pre-clean $(TEST_LINT) > $(MAKE) aggregate-results-and-cleanup > > +failed: > + @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \ > + grep -l '^failed [1-9]' *.counts | \ > + sed -n 's/\.counts$$/.sh/p') && \ > + test -z "$$failed" || $(MAKE) $$failed > + > prove: pre-clean $(TEST_LINT) > @echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' > $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) > $(MAKE) clean-except-prove-cache > > base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
[PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
This patch automates the process of determining which tests failed previously and re-running them. While developing patch series, it is a good practice to run the test suite from time to time, just to make sure that obvious bugs are caught early. With complex patch series, it is common to run `make -j15 -k test`, i.e. run the tests in parallel and *not* stop at the first failing test but continue. This has the advantage of identifying possibly multiple problems in one big test run. It is particularly important to reduce the turn-around time thusly on Windows, where the test suite spends 45 minutes on the computer on which this patch was developed. It is the most convenient way to determine which tests failed after running the entire test suite, in parallel, to look for left-over "trash directory.t*" subdirectories in the t/ subdirectory. However, those directories might live outside t/ when overridden using the --root= option, to which the Makefile has no access. The next best method is to grep explicitly for failed tests in the test-results/ directory, which the Makefile *can* access. Please note that the often-recommended `prove` tool requires Perl, and that opens a whole new can of worms on Windows. As no native Windows Perl comes with Subversion bindings, we have to use a Perl in Git for Windows that uses the POSIX emulation layer named MSYS2 (which is a portable version of Cygwin). When using this emulation layer under stress, e.g. when running massively-parallel tests, unexplicable crashes occur quite frequently, and instead of having a solution to the original problem, the developer now has an additional, quite huge problem. For that reason, this developer rejected `prove` as a solution and went with this patch instead. Signed-off-by: Johannes Schindelin --- Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v4 Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v4 t/Makefile | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/Makefile b/t/Makefile index d613935f14..1bb06c36f2 100644 --- a/t/Makefile +++ b/t/Makefile @@ -35,6 +35,12 @@ all: $(DEFAULT_TEST_TARGET) test: pre-clean $(TEST_LINT) $(MAKE) aggregate-results-and-cleanup +failed: + @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \ + grep -l '^failed [1-9]' *.counts | \ + sed -n 's/\.counts$$/.sh/p') && \ + test -z "$$failed" || $(MAKE) $$failed + prove: pre-clean $(TEST_LINT) @echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS) $(MAKE) clean-except-prove-cache base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe -- 2.11.1.windows.prerelease.2.9.g3014b57
