threads / patch / 42741

patcht/Makefile: add a rule to re-run previously-failed tests

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

## tl;dr

32 messages between Jun 29, 2016 and Jan 30, 2017. Diffs are folded; open one to read it.

replies: 31people: 6as markdown or json

Johannes Schindelin· Jun 29, 2016, 07:02 UTC · lore

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(+)

Show changes to t/Makefile +2 −0

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

Junio C Hamano· Jun 29, 2016, 17:18 UTC · re: Johannes Schindelin · lore

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

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

Show 5 quoted lines

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

Johannes Schindelin· Jul 1, 2016, 13:57 UTC · re: Junio C Hamano · lore

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

Hi Junio,

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

Show 10 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Jeff King· Jun 30, 2016, 06:37 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Jul 1, 2016, 14:00 UTC · re: Jeff King · lore

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

Hi Peff,

On Thu, 30 Jun 2016, Jeff King wrote:

Show 11 quoted lines

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

Show 8 quoted lines

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

Johannes Schindelin· Aug 29, 2016, 13:46 UTC · re: Johannes Schindelin · lore

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

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(+)

Show changes to t/Makefile +7 −0

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

Jeff King· Aug 30, 2016, 08:43 UTC · re: Johannes Schindelin · lore

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

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

Show 6 quoted lines

> 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(-)

Show changes to t/test-lib.sh +2 −2

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

Junio C Hamano· Aug 30, 2016, 19:15 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> 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 ;-)

Johannes Schindelin· Aug 31, 2016, 10:36 UTC · re: Junio C Hamano · lore

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

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:

Show 11 quoted lines

> 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

Sverre Rabbelier· Sep 1, 2016, 03:59 UTC · re: Johannes Schindelin · lore

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

On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines

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

Johannes Schindelin· Sep 1, 2016, 08:27 UTC · re: Sverre Rabbelier · lore

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

Hi Sverre,

On Wed, 31 Aug 2016, Sverre Rabbelier wrote:

Show 34 quoted lines

> 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

Sverre Rabbelier· Sep 1, 2016, 16:57 UTC · re: Johannes Schindelin · lore

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

On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 40 quoted lines

> 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

Junio C Hamano· Sep 1, 2016, 22:52 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier <srabbelier@gmail.com> writes:

Show 16 quoted lines

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

Johannes Schindelin· Sep 2, 2016, 07:35 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 8, 2016, 20:34 UTC · re: Johannes Schindelin · lore

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

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

Show 9 quoted lines

> 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 ;-)

Ævar Arnfjörð Bjarmason· Aug 30, 2016, 20:48 UTC · re: Johannes Schindelin · lore

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

On Mon, Aug 29, 2016 at 3:46 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 58 quoted lines

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

Jeff King· Aug 30, 2016, 20:51 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Show 16 quoted lines

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

Ævar Arnfjörð Bjarmason· Aug 30, 2016, 20:58 UTC · re: Jeff King · lore

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

On Tue, Aug 30, 2016 at 10:51 PM, Jeff King <peff@peff.net> wrote:

Show 27 quoted lines

> 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 :)

Johannes Schindelin· Aug 31, 2016, 10:29 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi Ævar,

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

Show 30 quoted lines

> 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

Ævar Arnfjörð Bjarmason· Aug 31, 2016, 13:42 UTC · re: Johannes Schindelin · lore

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

On Wed, Aug 31, 2016 at 12:29 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 37 quoted lines

> 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

Johannes Schindelin· Aug 31, 2016, 15:05 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Ævar Arnfjörð Bjarmason· Sep 2, 2016, 10:25 UTC · re: Johannes Schindelin · lore

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

On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 19 quoted lines

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

Johannes Schindelin· Sep 2, 2016, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi Ævar,

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

Show 18 quoted lines

> 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

Matthieu Moy· Sep 2, 2016, 16:36 UTC · re: Johannes Schindelin · lore

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

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

Show 27 quoted lines

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

Johannes Schindelin· Sep 4, 2016, 07:55 UTC · re: Matthieu Moy · lore

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

Hi,

On Fri, 2 Sep 2016, Matthieu Moy wrote:

Show 36 quoted lines

> 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

Matthieu Moy· Sep 4, 2016, 09:19 UTC · re: Johannes Schindelin · lore

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

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

Show 35 quoted lines

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

Johannes Schindelin· Jan 27, 2017, 14:17 UTC · re: Johannes Schindelin · lore

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

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(+)

Show changes to t/Makefile +6 −0

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

Jeff King· Jan 27, 2017, 17:07 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Jan 27, 2017, 17:21 UTC · re: Jeff King · lore

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

Hi Peff,

On Fri, 27 Jan 2017, Jeff King wrote:

Show 6 quoted lines

> 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

Johannes Schindelin· Jan 27, 2017, 17:21 UTC · re: Johannes Schindelin · lore

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

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(+)

Show changes to t/Makefile +6 −0

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

Junio C Hamano· Jan 27, 2017, 18:53 UTC · re: Johannes Schindelin · lore

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

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

Show 5 quoted lines

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

Show 26 quoted lines

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

Johannes Schindelin· Jan 30, 2017, 15:35 UTC · re: Junio C Hamano · lore

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:

Show 11 quoted lines

> 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