[PATCH] test-lib: write test results to test-results/<basename>-<pid>

[PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Johannes Schindelin]

The earlier code meant to attempt to strip everything except the test
number, but only stripped the part starting with the last dash.

However, there is no reason why we should not use the whole basename.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Even if this is not strictly necessary after Hannes' test cleanup, 
	it would still be nice.

	The alternative fix would be to use two percent signs instead of 
	just one.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 75b5a89..ccb5d0a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -477,7 +477,7 @@ test_done () {
 	trap - EXIT
 	test_results_dir="$TEST_DIRECTORY/test-results"
 	mkdir -p "$test_results_dir"
-	test_results_path="$test_results_dir/${0%-*}-$$"
+	test_results_path="$test_results_dir/${0%.sh}-$$"
 
 	echo "total $test_count" >> $test_results_path
 	echo "success $test_success" >> $test_results_path
-- 
1.6.2.240.g23c7

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Johannes Schindelin]

Hi,

On Fri, 13 Mar 2009, Johannes Schindelin wrote:

> The earlier code meant to attempt to strip everything except the test
> number, but only stripped the part starting with the last dash.
> 
> However, there is no reason why we should not use the whole basename.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Even if this is not strictly necessary after Hannes' test cleanup, 
> 	it would still be nice.

Just to clarify: it fixes the issue that these two tests share the same 
file in test-results/: t5521-pull-options.sh  t5521-pull-symlink.sh

As a consequence, one's results overwrite the other one's.

Ciao,
Dscho

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

Hi,


On Fri, Mar 13, 2009 at 05:36:13PM +0100, Johannes Schindelin wrote:
> On Fri, 13 Mar 2009, Johannes Schindelin wrote:
> 
> > The earlier code meant to attempt to strip everything except the test
> > number, but only stripped the part starting with the last dash.
> > 
> > However, there is no reason why we should not use the whole basename.

I agree.

> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > 
> > 	Even if this is not strictly necessary after Hannes' test cleanup, 
> > 	it would still be nice.
>
> Just to clarify: it fixes the issue that these two tests share the same 
> file in test-results/: t5521-pull-options.sh  t5521-pull-symlink.sh
> 
> As a consequence, one's results overwrite the other one's.

The pid of the test process makes the name of the test result file
unique for each test, even in the mentioned case, e.g. it would be
something like t5521-pull-12345 and t5521-pull-23456.  However, after
Hannes' patch there is no need for keeping that pid around because the
test result file names would be unique for each test anyway.

Moreover, if we would remove the pif from the test result file name,
we could also remove the 'pre-clean' target from 't/Makefile'.  With
the pid appended, we need that 'pre-clean' target to clean up all
leftovers from the previous run.  Without the pid each test will
always write to the same test result file, so we could actually just
overwrite the cruft from the last run.

Something like the patch below.  Thoughts?


Best,
Gábor

---
 t/Makefile    |    7 ++-----
 t/test-lib.sh |    4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 0d65ced..2e6e205 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -14,14 +14,11 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
 TSVN = $(wildcard t91[0-9][0-9]-*.sh)
 
-all: pre-clean $(T) aggregate-results clean
+all: $(T) aggregate-results clean
 
 $(T):
 	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
-pre-clean:
-	$(RM) -r test-results
-
 clean:
 	$(RM) -r 'trash directory' test-results
 
@@ -33,5 +30,5 @@ full-svn-test:
 	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=1 LC_ALL=C
 	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=0 LC_ALL=en_US.UTF-8
 
-.PHONY: pre-clean $(T) aggregate-results clean
+.PHONY: $(T) aggregate-results clean
 .NOTPARALLEL:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0bd24d5..d82c784 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -418,9 +418,9 @@ test_done () {
 	trap - exit
 	test_results_dir="$TEST_DIRECTORY/test-results"
 	mkdir -p "$test_results_dir"
-	test_results_path="$test_results_dir/${0%.sh}-$$"
+	test_results_path="$test_results_dir/${0%.sh}"
 
-	echo "total $test_count" >> $test_results_path
+	echo "total $test_count" > $test_results_path
 	echo "success $test_success" >> $test_results_path
 	echo "fixed $test_fixed" >> $test_results_path
 	echo "broken $test_broken" >> $test_results_path

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

On Fri, Mar 13, 2009 at 06:20:02PM +0100, SZEDER Gábor wrote:
> Hi,
> 
> 
> On Fri, Mar 13, 2009 at 05:36:13PM +0100, Johannes Schindelin wrote:
> > On Fri, 13 Mar 2009, Johannes Schindelin wrote:
> > 
> > > The earlier code meant to attempt to strip everything except the test
> > > number, but only stripped the part starting with the last dash.
> > > 
> > > However, there is no reason why we should not use the whole basename.
> 
> I agree.
> 
> > > 
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > > 
> > > 	Even if this is not strictly necessary after Hannes' test cleanup, 
> > > 	it would still be nice.
> >
> > Just to clarify: it fixes the issue that these two tests share the same 
> > file in test-results/: t5521-pull-options.sh  t5521-pull-symlink.sh
> > 
> > As a consequence, one's results overwrite the other one's.
> 
> The pid of the test process makes the name of the test result file
> unique for each test, even in the mentioned case, e.g. it would be
> something like t5521-pull-12345 and t5521-pull-23456.

Correction:  those files are not always unique, because although
unlikely, it's possible that these two tests get the same pid.

But with Hannes' patch this issue goes away, and the rest of my
previous mail still holds.


Best,
Gábor

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1119 bytes --]

Hi,

On Fri, 13 Mar 2009, SZEDER Gábor wrote:

> diff --git a/t/Makefile b/t/Makefile
> index 0d65ced..2e6e205 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -14,14 +14,11 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
>  T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
>  TSVN = $(wildcard t91[0-9][0-9]-*.sh)
>  
> -all: pre-clean $(T) aggregate-results clean
> +all: $(T) aggregate-results clean
>  
>  $(T):
>  	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
>  
> -pre-clean:
> -	$(RM) -r test-results
> -
>  clean:
>  	$(RM) -r 'trash directory' test-results
>  
> @@ -33,5 +30,5 @@ full-svn-test:
>  	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=1 LC_ALL=C
>  	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=0 LC_ALL=en_US.UTF-8
>  
> -.PHONY: pre-clean $(T) aggregate-results clean
> +.PHONY: $(T) aggregate-results clean
>  .NOTPARALLEL:

This is wrong.  If you have failing tests, or if you interrupt the tests, 
it will never clean the test results, and after Hannes' patch you _will_ 
have stale files lying around all the time.

I'd rather not have this change.

Ciao,
Dscho

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

Hi,

On Sat, Mar 14, 2009 at 12:53:06PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 13 Mar 2009, SZEDER Gábor wrote:
> 
> > diff --git a/t/Makefile b/t/Makefile
> > index 0d65ced..2e6e205 100644
> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -14,14 +14,11 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> >  T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
> >  TSVN = $(wildcard t91[0-9][0-9]-*.sh)
> >  
> > -all: pre-clean $(T) aggregate-results clean
> > +all: $(T) aggregate-results clean

Well, this part is wrong, or at least not up-to-date.  I just digged
up an ancient branch in my tree and sent out the diff, without
realizing that there were some conflicting changes since then.

> >  
> >  $(T):
> >  	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
> >  
> > -pre-clean:
> > -	$(RM) -r test-results
> > -
> >  clean:
> >  	$(RM) -r 'trash directory' test-results
> >  
> > @@ -33,5 +30,5 @@ full-svn-test:
> >  	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=1 LC_ALL=C
> >  	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=0 LC_ALL=en_US.UTF-8
> >  
> > -.PHONY: pre-clean $(T) aggregate-results clean
> > +.PHONY: $(T) aggregate-results clean
> >  .NOTPARALLEL:
> 
> This is wrong.  If you have failing tests, or if you interrupt the tests, 
> it will never clean the test results, and after Hannes' patch you _will_ 
> have stale files lying around all the time.

If you have failing tests, or if you interrupt the tests, then you
will have stale files lying around _anyway_:  not only test results
are left there, but also trash directories.  To remove the trash
directories, you'll need to run 'make clean' (in t/), but that will
remove the test results, too, so there is no difference.  But even if
you don't run 'make clean' before running the test suite again, test
results cruft from the previous run doesn't matter, because they will
be overwritten.


Best,
Gábor

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 378 bytes --]

Hi,

On Sat, 14 Mar 2009, SZEDER Gábor wrote:

> If you have failing tests, or if you interrupt the tests, then you
> will have stale files lying around _anyway_:  not only test results
> are left there, but also trash directories.

The 'pre-clean' target actually cleans test-results/, and test-lib.sh make 
sure that the trash directory is removed and recreated.

Ciao,
Dscho

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

On Sat, Mar 14, 2009 at 01:22:59PM +0100, Johannes Schindelin wrote:
> On Sat, 14 Mar 2009, SZEDER Gábor wrote:
> 
> > If you have failing tests, or if you interrupt the tests, then you
> > will have stale files lying around _anyway_:  not only test results
> > are left there, but also trash directories.
> 
> The 'pre-clean' target actually cleans test-results/, and test-lib.sh make 
> sure that the trash directory is removed and recreated.

With my proposed change there would be no need to clean 'test-results'
before running the tests, because test-lib.sh would take care of that
(not by removing and recreating 'test-results/', but by overwriting
(IOW: removing and recreating, but in one step) individual test result
files).


Best,
Gábor

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Sverre Rabbelier]

Heya,

On Sat, Mar 14, 2009 at 13:28, SZEDER Gábor <szeder@ira.uka.de> wrote:
> With my proposed change there would be no need to clean 'test-results'
> before running the tests, because test-lib.sh would take care of that
> (not by removing and recreating 'test-results/', but by overwriting
> (IOW: removing and recreating, but in one step) individual test result
> files).

Wouldn't that result in possible stale files being counted in the
result (e.g., if those tests were not run this time, but they were run
previously)?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

Hi,

On Sat, Mar 14, 2009 at 02:16:57PM +0100, Sverre Rabbelier wrote:
> On Sat, Mar 14, 2009 at 13:28, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > With my proposed change there would be no need to clean 'test-results'
> > before running the tests, because test-lib.sh would take care of that
> > (not by removing and recreating 'test-results/', but by overwriting
> > (IOW: removing and recreating, but in one step) individual test result
> > files).
> 
> Wouldn't that result in possible stale files being counted in the
> result (e.g., if those tests were not run this time, but they were run
> previously)?

It depends.

If you run only a few tests, then you do it with a command like 'make
t1234-foo.sh t5678-bar.sh'.

Currently this doesn't run the 'pre-clean' target, therefore if you
run different tests (e.g. 'make t1234-foo.sh ; make t5678-bar.sh'),
then a 'make aggregate-results' will include the result of both of
those tests.  The same happens with my proposal, too, because the test
of the last run will not overwrite the results of the test in the
first run.

Now suppose that you want to run the same set of tests twice (or more;
e.g. 'make t1234-foo.sh ; make t1234-foo.sh ; make
aggregate-results').  Since currently the pid of the test is included
in the test result file name, there will be two files (t1234-<pid1>
and t1234-<pid2>) created under 'test-results/', and _both_ will be
counted by 'aggregate-results'.  In this case my proposal is better,
because the last round will overwrite the result of the previous runs,
therefore no stale files will be counted.


Best,
Gábor

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1055 bytes --]

Hi,

On Sat, 14 Mar 2009, Sverre Rabbelier wrote:

> On Sat, Mar 14, 2009 at 13:28, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > With my proposed change there would be no need to clean 'test-results'
> > before running the tests, because test-lib.sh would take care of that
> > (not by removing and recreating 'test-results/', but by overwriting
> > (IOW: removing and recreating, but in one step) individual test result
> > files).
> 
> Wouldn't that result in possible stale files being counted in the
> result (e.g., if those tests were not run this time, but they were run
> previously)?

Yes.  Stale files would be counted in.  The fact that aggregate-results.sh 
is called when running "make" in t/ is a sure sign for me that you should 
not muddy waters by making unnecessary changes that break the default 
usage from time to time.

So I really, really, really, really would like that patch _not_ to be 
applied.

And I really would like to be able to spend my time on other things than 
discussing this at more length than necessary.

Ciao,
Dscho

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[SZEDER Gábor]

On Mon, Mar 16, 2009 at 11:18:19AM +0100, Johannes Schindelin wrote:
> On Sat, 14 Mar 2009, Sverre Rabbelier wrote:
> 
> > On Sat, Mar 14, 2009 at 13:28, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > > With my proposed change there would be no need to clean 'test-results'
> > > before running the tests, because test-lib.sh would take care of that
> > > (not by removing and recreating 'test-results/', but by overwriting
> > > (IOW: removing and recreating, but in one step) individual test result
> > > files).
> > 
> > Wouldn't that result in possible stale files being counted in the
> > result (e.g., if those tests were not run this time, but they were run
> > previously)?
> 
> Yes.  Stale files would be counted in.  The fact that aggregate-results.sh 
> is called when running "make" in t/ is a sure sign for me that you should 
> not muddy waters by making unnecessary changes that break the default 
> usage from time to time.

As I explained earlier, it won't change the default usage at all, but,
as I explained in my response to Sverre, it would actually fix a
current breakage in certain cases (i.e. make t1234-foo.sh ; make
t1234-foo.sh ; make aggregate-results would report the correct
numbers).

> And I really would like to be able to spend my time on other things than 
> discussing this at more length than necessary.

Ok, then I will also spare the effort of updating the patch.
Unless, of course, there are others who are interested.


Thanks,
Gábor

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Junio C Hamano]

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

> On Fri, 13 Mar 2009, Johannes Schindelin wrote:
>
>> The earlier code meant to attempt to strip everything except the test
>> number, but only stripped the part starting with the last dash.
>> 
>> However, there is no reason why we should not use the whole basename.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> 
>> 	Even if this is not strictly necessary after Hannes' test cleanup, 
>> 	it would still be nice.
>
> Just to clarify: it fixes the issue that these two tests share the same 
> file in test-results/: t5521-pull-options.sh  t5521-pull-symlink.sh
>
> As a consequence, one's results overwrite the other one's.

Are you saying that your "fix" will break things if we did not have
renames from j6t?

What bug/problem/issue does this patch fix?

I never look into test-results/ subdirectory myself (I'll let the
summarizing code do that for me) and the full names never bothered me.

Actually I've always thought it was a very nice touch by you to make sure
results will not be mixed up even when we have more than 10 tests in the
same category and some tests need to share their numbers, and and your
"meant to ... strip everything" above gave me "Huh?"

Re: [PATCH] test-lib: write test results to test-results/<basename>-<pid>

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ...
>> As a consequence, one's results overwrite the other one's.
>
> Are you saying that your "fix" will break things if we did not have
> renames from j6t?

Please disregard; I shouldn't be writing an e-mail when I do not have time
to re-read and think.  Sorry for the noise.