* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-29 13:46 ` [PATCH v2] " Johannes Schindelin
@ 2016-08-30 8:43 ` Jeff King
2016-08-30 19:15 ` Junio C Hamano
2016-08-30 20:48 ` Ævar Arnfjörð Bjarmason
2017-01-27 14:17 ` [PATCH v3] " Johannes Schindelin
2 siblings, 1 reply; 32+ messages in thread
From: Jeff King @ 2016-08-30 8:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-30 8:43 ` Jeff King
@ 2016-08-30 19:15 ` Junio C Hamano
2016-08-31 10:36 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2016-08-30 19:15 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
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 ;-)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-30 19:15 ` Junio C Hamano
@ 2016-08-31 10:36 ` Johannes Schindelin
2016-09-01 3:59 ` Sverre Rabbelier
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-08-31 10:36 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, Jeff King, git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-31 10:36 ` Johannes Schindelin
@ 2016-09-01 3:59 ` Sverre Rabbelier
2016-09-01 8:27 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Sverre Rabbelier @ 2016-09-01 3:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, Git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-01 3:59 ` Sverre Rabbelier
@ 2016-09-01 8:27 ` Johannes Schindelin
2016-09-01 16:57 ` Sverre Rabbelier
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-09-01 8:27 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, Jeff King, Git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-01 8:27 ` Johannes Schindelin
@ 2016-09-01 16:57 ` Sverre Rabbelier
2016-09-01 22:52 ` Junio C Hamano
0 siblings, 1 reply; 32+ messages in thread
From: Sverre Rabbelier @ 2016-09-01 16:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, Git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-01 16:57 ` Sverre Rabbelier
@ 2016-09-01 22:52 ` Junio C Hamano
2016-09-02 7:35 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2016-09-01 22:52 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johannes Schindelin, Jeff King, Git
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-01 22:52 ` Junio C Hamano
@ 2016-09-02 7:35 ` Johannes Schindelin
2016-09-08 20:34 ` Junio C Hamano
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-09-02 7:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, Jeff King, Git
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-02 7:35 ` Johannes Schindelin
@ 2016-09-08 20:34 ` Junio C Hamano
0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2016-09-08 20:34 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Sverre Rabbelier, Jeff King, Git
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 ;-)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-29 13:46 ` [PATCH v2] " Johannes Schindelin
2016-08-30 8:43 ` Jeff King
@ 2016-08-30 20:48 ` Ævar Arnfjörð Bjarmason
2016-08-30 20:51 ` Jeff King
2017-01-27 14:17 ` [PATCH v3] " Johannes Schindelin
2 siblings, 1 reply; 32+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2016-08-30 20:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git, Junio C Hamano, Jeff King
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-30 20:48 ` Ævar Arnfjörð Bjarmason
@ 2016-08-30 20:51 ` Jeff King
2016-08-30 20:58 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 32+ messages in thread
From: Jeff King @ 2016-08-30 20:51 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin, Git, Junio C Hamano
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/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-30 20:51 ` Jeff King
@ 2016-08-30 20:58 ` Ævar Arnfjörð Bjarmason
2016-08-31 10:29 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2016-08-30 20:58 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Git, Junio C Hamano
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 :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-30 20:58 ` Ævar Arnfjörð Bjarmason
@ 2016-08-31 10:29 ` Johannes Schindelin
2016-08-31 13:42 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-08-31 10:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, Git, Junio C Hamano
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-31 10:29 ` Johannes Schindelin
@ 2016-08-31 13:42 ` Ævar Arnfjörð Bjarmason
2016-08-31 15:05 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2016-08-31 13:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-31 13:42 ` Ævar Arnfjörð Bjarmason
@ 2016-08-31 15:05 ` Johannes Schindelin
2016-09-02 10:25 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-08-31 15:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, Git, Junio C Hamano
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-08-31 15:05 ` Johannes Schindelin
@ 2016-09-02 10:25 ` Ævar Arnfjörð Bjarmason
2016-09-02 12:08 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2016-09-02 10:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Git, Junio C Hamano
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".
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-02 10:25 ` Ævar Arnfjörð Bjarmason
@ 2016-09-02 12:08 ` Johannes Schindelin
2016-09-02 16:36 ` Matthieu Moy
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-09-02 12:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, Git, Junio C Hamano
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-02 12:08 ` Johannes Schindelin
@ 2016-09-02 16:36 ` Matthieu Moy
2016-09-04 7:55 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Matthieu Moy @ 2016-09-02 16:36 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git, Junio C Hamano
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/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-02 16:36 ` Matthieu Moy
@ 2016-09-04 7:55 ` Johannes Schindelin
2016-09-04 9:19 ` Matthieu Moy
0 siblings, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2016-09-04 7:55 UTC (permalink / raw)
To: Matthieu Moy
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git, Junio C Hamano
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
2016-09-04 7:55 ` Johannes Schindelin
@ 2016-09-04 9:19 ` Matthieu Moy
0 siblings, 0 replies; 32+ messages in thread
From: Matthieu Moy @ 2016-09-04 9:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git, Junio C Hamano
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/
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
2016-08-29 13:46 ` [PATCH v2] " Johannes Schindelin
2016-08-30 8:43 ` Jeff King
2016-08-30 20:48 ` Ævar Arnfjörð Bjarmason
@ 2017-01-27 14:17 ` Johannes Schindelin
2017-01-27 17:07 ` Jeff King
2017-01-27 17:21 ` [PATCH v4] " Johannes Schindelin
2 siblings, 2 replies; 32+ messages in thread
From: Johannes Schindelin @ 2017-01-27 14:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
2017-01-27 14:17 ` [PATCH v3] " Johannes Schindelin
@ 2017-01-27 17:07 ` Jeff King
2017-01-27 17:21 ` Johannes Schindelin
2017-01-27 17:21 ` [PATCH v4] " Johannes Schindelin
1 sibling, 1 reply; 32+ messages in thread
From: Jeff King @ 2017-01-27 17:07 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
2017-01-27 17:07 ` Jeff King
@ 2017-01-27 17:21 ` Johannes Schindelin
0 siblings, 0 replies; 32+ messages in thread
From: Johannes Schindelin @ 2017-01-27 17:21 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
2017-01-27 14:17 ` [PATCH v3] " Johannes Schindelin
2017-01-27 17:07 ` Jeff King
@ 2017-01-27 17:21 ` Johannes Schindelin
2017-01-27 18:53 ` Junio C Hamano
1 sibling, 1 reply; 32+ messages in thread
From: Johannes Schindelin @ 2017-01-27 17:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
2017-01-27 17:21 ` [PATCH v4] " Johannes Schindelin
@ 2017-01-27 18:53 ` Junio C Hamano
2017-01-30 15:35 ` Johannes Schindelin
0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2017-01-27 18:53 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
2017-01-27 18:53 ` Junio C Hamano
@ 2017-01-30 15:35 ` Johannes Schindelin
0 siblings, 0 replies; 32+ messages in thread
From: Johannes Schindelin @ 2017-01-30 15:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
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
^ permalink raw reply [flat|nested] 32+ messages in thread