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

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

[Johannes Schindelin]

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 without having to wait for the complete test suite to
finish.

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.

This patch automates the process of determinig which tests failed
previously and re-running them; It turned out to be quite convenient
when trying to squash bugs that crept in during rebases.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v1
 t/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..1459a7f 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,6 +35,8 @@ 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]*)))
+
 prove: pre-clean $(TEST_LINT)
 	@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
-- 
2.9.0.118.g0e1a633

base-commit: cf4c2cfe52be5bd973a4838f73a35d3959ce2f43

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> 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.

Hmmm, my tests run in parallel and do not stop at the first one
without '-k'.  What are we doing differently?

> 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.

Good idea, but I'd drop "in the t/ subdirectory" from the
description.

> +failed: $(patsubst trash,,$(patsubst directory.%,%.sh,$(wildcard trash\ directory.t[0-9]*)))
> +

This would not work if you use --root=<there> in GIT_TEST_OPTS, I am
afraid.

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

[Jeff King]

On Wed, Jun 29, 2016 at 09:02:37AM +0200, Johannes Schindelin wrote:

> 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.

As Junio noted, this doesn't work with --root. I have sometimes used:

  grep 'failed [^0]' test-results/*

for this purpose.

> This patch automates the process of determinig which tests failed
> previously and re-running them; It turned out to be quite convenient
> when trying to squash bugs that crept in during rebases.

I suspect your response will be "perl tools on Windows are too painful
to use", but the "prove" tool which comes with perl can do this and more
(e.g., running the failed tests first, and then following up with the
others to double-check), and our test suite supports it quite well.

  $ grep -B1 PROVE config.mak
  # run tests in parallel, with slow ones first to keep pipelines full
  GIT_PROVE_OPTS = -j16 --state=slow,save

  $ cd t
  $ make prove
  ... reports some test failed ...
  $ prove --state=failed
  ... re-runs just the failed test ...

-Peff

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

[Johannes Schindelin]

Hi Junio,

On Wed, 29 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > 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.
> 
> Hmmm, my tests run in parallel and do not stop at the first one
> without '-k'.  What are we doing differently?

Probably none of your tests are failing... When I run tests with -j15 and
without -k, as soon as *any* test fails, the other 14 jobs stop after
running their respective current tests.

This is particularly annoying when some early test fails and a subsequent
test run reveals that *also* one of those pesky SVN tests failed.

> > 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.
> 
> Good idea, but I'd drop "in the t/ subdirectory" from the
> description.

Okay.

> > +failed: $(patsubst trash,,$(patsubst directory.%,%.sh,$(wildcard trash\ directory.t[0-9]*)))
> > +
> 
> This would not work if you use --root=<there> in GIT_TEST_OPTS, I am
> afraid.

Bah. You're correct. Would it be okay with you if I simply punted, like
this:

ifeq (,$(findstring --root,$(GIT_TEST_OPTS)))
failed: ...
else
failed:
	echo "Sorry, the 'failed' rule is incompatible with --root=..." >&2
endif

? I really do not have time to spend more time on this right now...

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Peff,

On Thu, 30 Jun 2016, Jeff King wrote:

> On Wed, Jun 29, 2016 at 09:02:37AM +0200, Johannes Schindelin wrote:
> 
> > 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.
> 
> As Junio noted, this doesn't work with --root. I have sometimes used:
> 
>   grep 'failed [^0]' test-results/*
> 
> for this purpose.

True, I could also do that. Looking for directories rather than spawning a
full-fledged grep is more light-weight, though.

> > This patch automates the process of determinig which tests failed
> > previously and re-running them; It turned out to be quite convenient
> > when trying to squash bugs that crept in during rebases.
> 
> I suspect your response will be "perl tools on Windows are too painful
> to use", but the "prove" tool which comes with perl can do this and more
> (e.g., running the failed tests first, and then following up with the
> others to double-check), and our test suite supports it quite well.

It will surprise you to learn that I did use `prove` extensively. There
have been enough problems with it, though, that I stopped it.

Modern Windows does not have too many problems with it, but it appears as
if Windows Server 2008 R2 (which I used for quite some time for my
principal development) requires too many work-arounds for Perl to work
reliably so that every once in a while, `prove` hangs without any real
reason.

That is when I stopped using it.

Ciao,
Dscho

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

[Johannes Schindelin]

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)


 t/Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/Makefile b/t/Makefile
index d613935..8aa6a72 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,6 +35,13 @@ 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]' $$(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)
 	$(MAKE) clean-except-prove-cache
-- 
2.10.0.rc1.114.g2bd6b38

base-commit: d5cb9cbd64165153a318e1049f8bf14b09a16b11

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

[Jeff King]

On Mon, Aug 29, 2016 at 03:46:05PM +0200, Johannes Schindelin wrote:

> 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.

Hmm, interesting. Your approach seems reasonable, but I have to wonder
if writing the pid in the first place is sane.

I started to write up my reasoning in this email, but realized it was
rapidly becoming the content of a commit message. So here is that
commit.

-- >8 --
Subject: [PATCH] test-lib: drop PID from test-results/*.count

Each test run generates a "count" file in t/test-results
that stores the number of successful, failed, etc tests.
If you run "t1234-foo.sh", that file is named as
"t/test-results/t1234-foo-$$.count"

The addition of the PID there is serving no purpose, and
makes analysis of the count files harder.

The presence of the PID dates back to 2d84e9f (Modify
test-lib.sh to output stats to t/test-results/*,
2008-06-08), but no reasoning is given there. Looking at the
current code, we can see that other files we write to
test-results (like *.exit and *.out) do _not_ have the PID
included. So the presence of the PID does not meaningfully
allow one to store the results from multiple runs anyway.

Moreover, anybody wishing to read the *.count files to
aggregate results has to deal with the presence of multiple
files for a given test (and figure out which one is the most
recent based on their timestamps!). The only consumer of
these files is the aggregate.sh script, which arguably gets
this wrong. If a test is run multiple times, its counts will
appear multiple times in the total (I say arguably only
because the desired semantics aren't documented anywhere,
but I have trouble seeing how this behavior could be
useful).

So let's just drop the PID, which fixes aggregate.sh, and
will make new features based around the count files easier
to write.

Note that since the count-file may already exist (when
re-running a test), we also switch the "cat" from appending
to truncating. The use of append here was pointless in the
first place, as we expected to always write to a unique file.

Signed-off-by: Jeff King <peff@peff.net>
---
The presence of the append, combined with the way aggregate.sh is
written makes me wonder if the intent was to store multiple run results
for each test in a single file (and aggregate would just report the last
one). Which _still_ makes the use of the PID wrong. But again, I don't
see much use for it.

 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..eada492 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -687,9 +687,9 @@ test_done () {
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
 		mkdir -p "$test_results_dir"
 		base=${0##*/}
-		test_results_path="$test_results_dir/${base%.sh}-$$.counts"
+		test_results_path="$test_results_dir/${base%.sh}.counts"
 
-		cat >>"$test_results_path" <<-EOF
+		cat >"$test_results_path" <<-EOF
 		total $test_count
 		success $test_success
 		fixed $test_fixed
-- 
2.10.0.rc2.123.ga991f9e

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm, interesting. Your approach seems reasonable, but I have to wonder
> if writing the pid in the first place is sane.
>
> I started to write up my reasoning in this email, but realized it was
> rapidly becoming the content of a commit message. So here is that
> commit.

Sounds sensible; if this makes Dscho's "which ones failed in the
previous run" simpler, that is even better ;-)

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Jeff King]

On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:

> >  -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.

Heh. I think each iteration of this patch will be destined to have
somebody[1] point Johannes at prove. ;)

(But I really do recommend prove if you can use it).

-Peff

[1] http://public-inbox.org/git/20160630063725.GC15380@sigill.intra.peff.net/

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

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 30, 2016 at 10:51 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >  -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.
>
> Heh. I think each iteration of this patch will be destined to have
> somebody[1] point Johannes at prove. ;)
>
> (But I really do recommend prove if you can use it).
>
> -Peff
>
> [1] http://public-inbox.org/git/20160630063725.GC15380@sigill.intra.peff.net/

Sorry about that, I see it's been mentioned already. My only excuse is
that I don't know how to operate my E-Mail client :)

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Hi Ævar,

On Tue, 30 Aug 2016, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Aug 30, 2016 at 10:51 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >
> >> >  -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.
> >
> > Heh. I think each iteration of this patch will be destined to have
> > somebody[1] point Johannes at prove. ;)
> >
> > (But I really do recommend prove if you can use it).
> >
> > -Peff
> >
> > [1] http://public-inbox.org/git/20160630063725.GC15380@sigill.intra.peff.net/
> 
> Sorry about that, I see it's been mentioned already.

Yeah, it is true that prove(1) would be able to help. If it worked
reliably on Windows. (Probably Perl's fault, not prove's.)

> My only excuse is that I don't know how to operate my E-Mail client :)

But we use email to discuss all things Git because it makes everything so
easy and convenient... ;-)

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

[Sverre: we are considering to remove the -<pid> suffix in test-results/,
see more below.]

On Tue, 30 Aug 2016, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > if writing the pid in the first place is sane.
> >
> > I started to write up my reasoning in this email, but realized it was
> > rapidly becoming the content of a commit message. So here is that
> > commit.
> 
> Sounds sensible; if this makes Dscho's "which ones failed in the
> previous run" simpler, that is even better ;-)

I did not have the time to dig further before now. There must have been a
good reason why we append the PID.

Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
to t/test-results/*, 2008-06-08): any idea why the -<pid> suffix was
needed?

Ciao,
Dscho

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

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31, 2016 at 12:29 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Ævar,
>
> On Tue, 30 Aug 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Aug 30, 2016 at 10:51 PM, Jeff King <peff@peff.net> wrote:
>> > On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> >  -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.
>> >
>> > Heh. I think each iteration of this patch will be destined to have
>> > somebody[1] point Johannes at prove. ;)
>> >
>> > (But I really do recommend prove if you can use it).
>> >
>> > -Peff
>> >
>> > [1] http://public-inbox.org/git/20160630063725.GC15380@sigill.intra.peff.net/
>>
>> Sorry about that, I see it's been mentioned already.
>
> Yeah, it is true that prove(1) would be able to help. If it worked
> reliably on Windows. (Probably Perl's fault, not prove's.)

I haven't used it myself (or any Windows thing) but people say good
things about http://strawberryperl.com

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

Hi Ævar,

On Wed, 31 Aug 2016, Ævar Arnfjörð Bjarmason wrote:

> I haven't used it myself (or any Windows thing) but people say good
> things about http://strawberryperl.com

Ah yes. This comes up frequently. Many a Git for Windows user pointed me
into that direction.

The biggest problem with Strawberry Perl is that it is virtually
impossible to build the Subversion-Perl bindings using the Git for Windows
SDK when using Strawberry Perl.

Which pretty much precludes it from being used in Git for Windows.

And then there are the path issues... Git's Perl scripts are pretty
certain that they live in a POSIX-y environment. Which MSYS2 Perl
provides. Strawberry Perl not.

Ciao,
Johannes

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

[Sverre Rabbelier]

On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 30 Aug 2016, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > > if writing the pid in the first place is sane.
> > >
> > > I started to write up my reasoning in this email, but realized it was
> > > rapidly becoming the content of a commit message. So here is that
> > > commit.
> >
> > Sounds sensible; if this makes Dscho's "which ones failed in the
> > previous run" simpler, that is even better ;-)
>
> I did not have the time to dig further before now. There must have been a
> good reason why we append the PID.
>
> Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
> to t/test-results/*, 2008-06-08): any idea why the -<pid> suffix was
> needed?

I can't really recall, but I think it may have been related to me
doing something like this:
1. Make a change, and start running tests (this takes a long time)
2. Notice a failure, start fixing it, leave tests running to find
further failures
3. Finish fix, first tests are still running, start another run in a
new terminal (possibly of just the one failed test I was fixing) to
see if the fix worked.

Without the pid, the second run would clobber the results from the first run.


If only past-me was more rigorous about writing good commit messages :P.

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

[Johannes Schindelin]

Hi Sverre,

On Wed, 31 Aug 2016, Sverre Rabbelier wrote:

> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
> > > Jeff King <peff@peff.net> writes:
> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > > > if writing the pid in the first place is sane.
> > > >
> > > > I started to write up my reasoning in this email, but realized it was
> > > > rapidly becoming the content of a commit message. So here is that
> > > > commit.
> > >
> > > Sounds sensible; if this makes Dscho's "which ones failed in the
> > > previous run" simpler, that is even better ;-)
> >
> > I did not have the time to dig further before now. There must have been a
> > good reason why we append the PID.
> >
> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
> > to t/test-results/*, 2008-06-08): any idea why the -<pid> suffix was
> > needed?
> 
> I can't really recall, but I think it may have been related to me
> doing something like this:
> 1. Make a change, and start running tests (this takes a long time)
> 2. Notice a failure, start fixing it, leave tests running to find
> further failures
> 3. Finish fix, first tests are still running, start another run in a
> new terminal (possibly of just the one failed test I was fixing) to
> see if the fix worked.
> 
> Without the pid, the second run would clobber the results from the first run.
> 
> 
> If only past-me was more rigorous about writing good commit messages :P.

:-)

Would present-you disagree with stripping off the -<pid> suffix, based on
your recollections?

Ciao,
Dscho

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

[Sverre Rabbelier]

On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 31 Aug 2016, Sverre Rabbelier wrote:
>> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
>> > > Jeff King <peff@peff.net> writes:
>> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
>> > > > if writing the pid in the first place is sane.
>> > > >
>> > > > I started to write up my reasoning in this email, but realized it was
>> > > > rapidly becoming the content of a commit message. So here is that
>> > > > commit.
>> > >
>> > > Sounds sensible; if this makes Dscho's "which ones failed in the
>> > > previous run" simpler, that is even better ;-)
>> >
>> > I did not have the time to dig further before now. There must have been a
>> > good reason why we append the PID.
>> >
>> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
>> > to t/test-results/*, 2008-06-08): any idea why the -<pid> suffix was
>> > needed?
>>
>> I can't really recall, but I think it may have been related to me
>> doing something like this:
>> 1. Make a change, and start running tests (this takes a long time)
>> 2. Notice a failure, start fixing it, leave tests running to find
>> further failures
>> 3. Finish fix, first tests are still running, start another run in a
>> new terminal (possibly of just the one failed test I was fixing) to
>> see if the fix worked.
>>
>> Without the pid, the second run would clobber the results from the first run.
>>
>>
>> If only past-me was more rigorous about writing good commit messages :P.
>
> :-)
>
> Would present-you disagree with stripping off the -<pid> suffix, based on
> your recollections?

No objections, I think it should be fine. If anyone uncovers a
particularly compelling reason later on, it's only a commit away :).

-- 
Cheers,

Sverre Rabbelier

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

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>>> I can't really recall, but I think it may have been related to me
>>> doing something like this:
>>> 1. Make a change, and start running tests (this takes a long time)
>>> 2. Notice a failure, start fixing it, leave tests running to find
>>> further failures
>>> 3. Finish fix, first tests are still running, start another run in a
>>> new terminal (possibly of just the one failed test I was fixing) to
>>> see if the fix worked.
>>>
>>> Without the pid, the second run would clobber the results from the first run.
>>>
>> Would present-you disagree with stripping off the -<pid> suffix, based on
>> your recollections?
>
> No objections, I think it should be fine. If anyone uncovers a
> particularly compelling reason later on, it's only a commit away :).

OK, especially with the earlier observation made by Peff in the log
message:

    ... we can see that other files we write to test-results (like
    *.exit and *.out) do _not_ have the PID included. So the
    presence of the PID does not meaningfully allow one to store the
    results from multiple runs anyway.

even if we wanted to, keeping the current code with suffix is not
sufficient, so I suspect it won't be just "a commit" away, but we
should be able to lose it for now.  Hopefully that would help making
Dscho's "what are the failed tests?" logic simpler.

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Thu, 1 Sep 2016, Junio C Hamano wrote:

> Hopefully that [patch removing the -<pid> suffix] would help making
> Dscho's "what are the failed tests?" logic simpler.

Of course.

It also makes sure that those 2 hours I spent on writing and perfecting
the sed magic were spent in vain... ;-)

Ciao,
Dscho

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

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Ævar,
>
> On Wed, 31 Aug 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> I haven't used it myself (or any Windows thing) but people say good
>> things about http://strawberryperl.com
>
> Ah yes. This comes up frequently. Many a Git for Windows user pointed me
> into that direction.
>
> The biggest problem with Strawberry Perl is that it is virtually
> impossible to build the Subversion-Perl bindings using the Git for Windows
> SDK when using Strawberry Perl.
>
> Which pretty much precludes it from being used in Git for Windows.
>
> And then there are the path issues... Git's Perl scripts are pretty
> certain that they live in a POSIX-y environment. Which MSYS2 Perl
> provides. Strawberry Perl not.

This might be me missing the point, and I'm really just trying to be
helpful here and make "prove" work for you because it's awesome, but
as far as just you running this for development purposes does any of
this SVN stuff matter? I.e. you can build Git itself not with
Strawberry, but just use Strawberry to get a working copy of "prove".

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]

Hi Ævar,

On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > The biggest problem with Strawberry Perl is that it is virtually
> > impossible to build the Subversion-Perl bindings using the Git for
> > Windows SDK when using Strawberry Perl.
> >
> > Which pretty much precludes it from being used in Git for Windows.
> >
> > And then there are the path issues... Git's Perl scripts are pretty
> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
> > provides. Strawberry Perl not.
> 
> This might be me missing the point, and I'm really just trying to be
> helpful here and make "prove" work for you because it's awesome, but
> as far as just you running this for development purposes does any of
> this SVN stuff matter? I.e. you can build Git itself not with
> Strawberry, but just use Strawberry to get a working copy of "prove".

Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
take a substantial time to run). So if I run the test suite, I better do
it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
might just as well not bother with running the entire test suite...

Ciao,
Dscho

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

[Matthieu Moy]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Ævar,
>
> On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > The biggest problem with Strawberry Perl is that it is virtually
>> > impossible to build the Subversion-Perl bindings using the Git for
>> > Windows SDK when using Strawberry Perl.
>> >
>> > Which pretty much precludes it from being used in Git for Windows.
>> >
>> > And then there are the path issues... Git's Perl scripts are pretty
>> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
>> > provides. Strawberry Perl not.
>> 
>> This might be me missing the point, and I'm really just trying to be
>> helpful here and make "prove" work for you because it's awesome, but
>> as far as just you running this for development purposes does any of
>> this SVN stuff matter? I.e. you can build Git itself not with
>> Strawberry, but just use Strawberry to get a working copy of "prove".
>
> Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
> take a substantial time to run). So if I run the test suite, I better do
> it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
> might just as well not bother with running the entire test suite...

Maybe something like

\path\to\strawberry-perl\perl.exe \path\to\prove ...

without changing the PATH would work. I wouldn't call that convenient
though.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

Hi,

On Fri, 2 Sep 2016, Matthieu Moy wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hi Ævar,
> >
> > On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> > The biggest problem with Strawberry Perl is that it is virtually
> >> > impossible to build the Subversion-Perl bindings using the Git for
> >> > Windows SDK when using Strawberry Perl.
> >> >
> >> > Which pretty much precludes it from being used in Git for Windows.
> >> >
> >> > And then there are the path issues... Git's Perl scripts are pretty
> >> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
> >> > provides. Strawberry Perl not.
> >> 
> >> This might be me missing the point, and I'm really just trying to be
> >> helpful here and make "prove" work for you because it's awesome, but
> >> as far as just you running this for development purposes does any of
> >> this SVN stuff matter? I.e. you can build Git itself not with
> >> Strawberry, but just use Strawberry to get a working copy of "prove".
> >
> > Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
> > take a substantial time to run). So if I run the test suite, I better do
> > it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
> > might just as well not bother with running the entire test suite...
> 
> Maybe something like
> 
> \path\to\strawberry-perl\perl.exe \path\to\prove ...
> 
> without changing the PATH would work. I wouldn't call that convenient
> though.

Wouldn't Perl-specific environment variables set by Strawberry Perl (such
as PERL_PATH bleed through to the spawned child processes?

We're dancing around the issue, really. Rather than piling workaround on
workaround with no end in sight, I think it is time to admit that using
prove(1) on Windows is just not a good solution for the problem to re-run
failed tests.

Ciao,
Johannes

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

[Matthieu Moy]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Fri, 2 Sep 2016, Matthieu Moy wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > Hi Ævar,
>> >
>> > On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> This might be me missing the point, and I'm really just trying to be
>> >> helpful here and make "prove" work for you because it's awesome, but
>> >> as far as just you running this for development purposes does any of
>> >> this SVN stuff matter? I.e. you can build Git itself not with
>> >> Strawberry, but just use Strawberry to get a working copy of "prove".
>> >
>> > Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
>> > take a substantial time to run). So if I run the test suite, I better do
>> > it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
>> > might just as well not bother with running the entire test suite...
>> 
>> Maybe something like
>> 
>> \path\to\strawberry-perl\perl.exe \path\to\prove ...
>> 
>> without changing the PATH would work. I wouldn't call that convenient
>> though.
>
> Wouldn't Perl-specific environment variables set by Strawberry Perl (such
> as PERL_PATH bleed through to the spawned child processes?
>
> We're dancing around the issue, really. Rather than piling workaround on
> workaround with no end in sight, I think it is time to admit that using
> prove(1) on Windows is just not a good solution for the problem to re-run
> failed tests.

I didn't re-add Ævar's disclaimer, but my message was really not
intended to be an objection to your patch, just a (not necessarily good)
idea in case you or someone else on windows wanted to give one more
chance to prove.

I'm all for adding "make failed". Actually, we could even make the
feature more discoverable by echoing "You may run 'make failed' to
re-run failed tests" at the end of the tests when one of them failed.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 1 Sep 2016, Junio C Hamano wrote:
>
>> Hopefully that [patch removing the -<pid> suffix] would help making
>> Dscho's "what are the failed tests?" logic simpler.
>
> Of course.
>
> It also makes sure that those 2 hours I spent on writing and perfecting
> the sed magic were spent in vain... ;-)

Well it is either

 * the sed magic is so arcane that you'd need to spend a long time,
   comparable to 2 hours you already spent, if you ever need to look
   at it and figure out what it does next time you need to change
   something in it.

or

 * you are not familiar with the sed magic and you would be able to
   write the same thing in 2 minutes next time if you need to adjust
   it when we add -pid back later.

Either way, those 2 hours are not wasted.

I personally fall into the former category.  Any sed script that
needs G, h, and x together I need to spend at least 15 minutes just
to warm myself up, as I do not work with the language that often.

Thanks ;-)

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

[Johannes Schindelin]

This patch automates the process of determinig 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=<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.

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 <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v3
Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v3
Interdiff vs v2:

 diff --git a/t/Makefile b/t/Makefile
 index 8aa6a72a70..1bb06c36f2 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -37,9 +37,8 @@ test: pre-clean $(TEST_LINT)
  
  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') && \
 +		grep -l '^failed [1-9]' *.counts | \
 +		sed -n 's/\.counts$$/.sh/p') && \
  	test -z "$$failed" || $(MAKE) $$failed
  
  prove: pre-clean $(TEST_LINT)


 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

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

[Jeff King]

On Fri, Jan 27, 2017 at 03:17:36PM +0100, Johannes Schindelin wrote:

> This patch automates the process of determinig which tests failed
> previously and re-running them.

s/determinig/determining/

Patch otherwise looks good, and I'm happy to be rid of the sed
complexity from v2.

-Peff

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

[Johannes Schindelin]

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=<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.

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 <johannes.schindelin@gmx.de>
---
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

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

[Johannes Schindelin]

Hi Peff,

On Fri, 27 Jan 2017, Jeff King wrote:

> On Fri, Jan 27, 2017 at 03:17:36PM +0100, Johannes Schindelin wrote:
> 
> > This patch automates the process of determinig which tests failed
> > previously and re-running them.
> 
> s/determinig/determining/

Fixed in v4,
Johannes

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This patch automates the process of determining which tests failed
> previously and re-running them.
> ...
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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
<peff@peff.net>" 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

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

[Johannes Schindelin]

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This patch automates the process of determining which tests failed
> > previously and re-running them.
> > ...
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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
> <peff@peff.net>" 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