Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Aug 29, 2016 at 3:46 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> 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, as was
> pointed out by Jeff King, those directories might live outside t/ when
> overridden using the --root=<directory> 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.
>
> This patch automates the process of determinig which tests failed
> previously and re-running them.
>
> Note that we need to be careful to inspect only the *newest* entries in
> test-results/: this directory contains files of the form
> tNNNN-<name>-<pid>.counts and is only removed wholesale when running the
> *entire* test suite, not when running individual tests. We ensure that
> with a little sed magic on `ls -t`'s output that simply skips lines
> when the file name was seen earlier.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
>         The patch is unfortunately no longer as trivial as before, but it
>         now also works with --root=..., i.e. when the user overrode the
>         location of the trash directories.
>
> Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v2
> Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v2
> Interdiff vs v1:
>
>  diff --git a/t/Makefile b/t/Makefile
>  index c402a9ec..8aa6a72 100644
>  --- a/t/Makefile
>  +++ b/t/Makefile
>  @@ -35,7 +35,12 @@ all: $(DEFAULT_TEST_TARGET)
>   test: pre-clean $(TEST_LINT)
>         $(MAKE) aggregate-results-and-cleanup
>
>  -failed: $(patsubst trash,,$(patsubst directory.%,%.sh,$(wildcard trash\ directory.t[0-9]*)))
>  +failed:
>  +      @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
>  +              grep -l '^failed [1-9]' $$(ls -t *.counts | \
>  +                      sed 'G;h;/^\(t[^.]*\)-[0-9]*\..*\n\1-[0-9]*\./d;P;d') | \
>  +              sed -n 's/-[0-9]*\.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)

I don't at all mind this solution to the problem, if it works for that's cool.

But FWIW something you may have missed is that you can just use
prove(1) for this, which is why I initially patched git.git to support
TAP, so I didn't have to implement stuff like this.

I.e.:

    $ prove --state=save t[0-9]*.sh
    $ prove --state=failed,save t[0-9]*.sh

Does exactly what you're trying to do here with existing tools we support.

I.e. your new target could just be implemented in terms of calling prove.

Check out its man page, it may have other stuff you want to use, e.g.
you can run the slowest tests first etc.