[PATCH 0/3] Enable parallelized tests

[PATCH 0/3] Enable parallelized tests

[Johannes Schindelin]

This patch pair enables parallel tests.  On a pretty beefy machine,

	$ /usr/bin/time make -j50

shows this:

	69.33user 92.33system 0:59.26elapsed 272%CPU (0avgtext+0avgdata
	0maxresident)k 0inputs+0outputs (0major+33007360minor)pagefaults 0swaps

vs.

	$ /usr/bin/time make

showing this:

	61.25user 75.10system 3:57.68elapsed 57%CPU (0avgtext+0avgdata
	0maxresident)k 0inputs+0outputs (0major+32897071minor)pagefaults 0swaps

Note: the machine was used for other tasks during the test, too.

These results are with SVN/CVS tests enabled.  I am pretty sure that the
results would be even more impressive without them (the SVN/CVS tests come
all at the end, and seem to idle the CPU mostly, and the last few seconds
are only spent on 2 tests).

Johannes Schindelin (3):
  t9700: remove useless check
  tests: Clarify dependencies between tests, 'aggregate-results' and
    'clean'
  Enable parallel tests

 t/Makefile      |   15 ++++++++++++---
 t/t9700/test.pl |    3 ---
 t/test-lib.sh   |   11 ++++++++++-
 3 files changed, 22 insertions(+), 7 deletions(-)

[PATCH 1/3] t9700: remove useless check

[Johannes Schindelin]

t9700 used to check if the basename of the current directory is
'trash directory', the expensive way.

However, there is absolutely no good reason why this test should not
run in, say 'life is good' or 'i love tests'.  So remove the check
altogether.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t9700/test.pl |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 4d23125..851cea4 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -14,10 +14,7 @@ use File::Temp;
 BEGIN { use_ok('Git') }
 
 # set up
-our $repo_dir = "trash directory";
 our $abs_repo_dir = Cwd->cwd;
-die "this must be run by calling the t/t97* shell script(s)\n"
-    if basename(Cwd->cwd) ne $repo_dir;
 ok(our $r = Git->repository(Directory => "."), "open repository");
 
 # config
-- 
1.6.0.rc2.23.gd08e9

[PATCH 2/3] tests: Clarify dependencies between tests, 'aggregate-results' and 'clean'

[Johannes Schindelin]

The Makefile targets 'aggregate-results' and 'clean' pretended to be
independent.  This is not true, of course, since aggregate-results
needs the results _before_ they are removed.

Likewise, the tests should have been run already when the results are
to be aggregated.

However, as it is legitimate to run only a few tests, and then aggregate
just those results, so another target is introduced, that depends on all
tests, then aggregates the results, and only then removes the results.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/Makefile |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 0d65ced..aa952e1 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -14,7 +14,8 @@ 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: pre-clean
+	$(MAKE) aggregate-results-and-cleanup
 
 $(T):
 	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
@@ -25,6 +26,10 @@ pre-clean:
 clean:
 	$(RM) -r 'trash directory' test-results
 
+aggregate-results-and-cleanup: $(T)
+	$(MAKE) aggregate-results
+	$(MAKE) clean
+
 aggregate-results:
 	'$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*
 
-- 
1.6.0.rc2.23.gd08e9

[PATCH 3/3] Enable parallel tests

[Johannes Schindelin]

On multiprocessor machines, or with I/O heavy tests (that leave the
CPU waiting a lot), it makes sense to parallelize the tests.

However, care has to be taken that the different jobs use different
trash directories.

This commit does so, by inspecting the MAKEFLAGS variable to detect
if the option "-j" or "--jobs" was passed to make.  In that case, the
test is run with the new "--parallel" option.

If parallel mode was detected, the trash directories are created with
a suffix that is unique with regard to the test, as it is the test's
base name.

Parallel mode also triggers removal of the trash directory in the test
itself if everything went fine, so that the trash directories do not
pile up only to be removed at the very end.

If a test failed, the trash directory is not removed.  Chances are
that the exact error message is lost in the clutter, but you can still
see what test failed from the name of the trash directory, and repeat
the test (without -j).

If all was good, you will see the aggregated results.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/Makefile    |    8 ++++++--
 t/test-lib.sh |   11 ++++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index aa952e1..fb2fba9 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -14,6 +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)
 
+# MAKEFLAGS only sees the -j flag when expanded in the task, so we cannot
+# use ifeq() games here.  Instead we play shell games.
+GIT_TEST_OPTS += $(shell echo " $(MAKEFLAGS)" | \
+	sed -n "s/^.* \(--jobs\|\(-\|[^-]*\)j\).*/--parallel/p")
+
 all: pre-clean
 	$(MAKE) aggregate-results-and-cleanup
 
@@ -24,7 +29,7 @@ pre-clean:
 	$(RM) -r test-results
 
 clean:
-	$(RM) -r 'trash directory' test-results
+	$(RM) -rf 'trash directory' test-results
 
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
@@ -39,4 +44,3 @@ full-svn-test:
 	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=0 LC_ALL=en_US.UTF-8
 
 .PHONY: pre-clean $(T) aggregate-results clean
-.NOTPARALLEL:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11c0275..c5868c4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -74,6 +74,7 @@ esac
 	) &&
 	color=t
 
+test="trash directory"
 while test "$#" -ne 0
 do
 	case "$1" in
@@ -94,6 +95,10 @@ do
 	--no-python)
 		# noop now...
 		shift ;;
+	--parallel)
+		test="$test.$(basename "$0" .sh)"
+		remove_trash="$(pwd)/$test"
+		shift ;;
 	*)
 		break ;;
 	esac
@@ -449,6 +454,11 @@ test_done () {
 		# we will leave things as they are.
 
 		say_color pass "passed all $msg"
+
+		test ! -z = "$remove_trash" &&
+		cd "$(dirname "$remove_trash")" &&
+		rm -rf "$(basename "$remove_trash")"
+
 		exit 0 ;;
 
 	*)
@@ -485,7 +495,6 @@ fi
 . ../GIT-BUILD-OPTIONS
 
 # Test repository
-test="trash directory"
 rm -fr "$test" || {
 	trap - exit
 	echo >&5 "FATAL: Cannot prepare test area"
-- 
1.6.0.rc2.23.gd08e9

Re: [PATCH 3/3] Enable parallel tests

[Junio C Hamano]

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

> On multiprocessor machines, or with I/O heavy tests (that leave the
> CPU waiting a lot), it makes sense to parallelize the tests.

I was actually thinking about doing this eventually.  Thanks for beating
me to it.

> Parallel mode also triggers removal of the trash directory in the test
> itself if everything went fine, so that the trash directories do not
> pile up only to be removed at the very end.

I think making the tests remove their own mess makes sense regardless.

I have to wonder why you would want to make this change conditional on
MAKEFLAGS.  I was envisioning that parallel tests would run in "trash
directory/$(basename $0)" or something.

Are there downsides of doing this change unconditionally?

>  clean:
> -	$(RM) -r 'trash directory' test-results
> +	$(RM) -rf 'trash directory' test-results

This is not needed, I think, as RM is defined with -f already.

Re: [PATCH 3/3] Enable parallel tests

[Johannes Schindelin]

Hi,

On Thu, 7 Aug 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Parallel mode also triggers removal of the trash directory in the test 
> > itself if everything went fine, so that the trash directories do not 
> > pile up only to be removed at the very end.
> 
> I think making the tests remove their own mess makes sense regardless.

When I add tests, I first run the appropriate t/t*.sh, then expect what is 
in trash directory, then extend the test.  So at least I need an option to 
keep the directory.

> >  clean:
> > -	$(RM) -r 'trash directory' test-results
> > +	$(RM) -rf 'trash directory' test-results
> 
> This is not needed, I think, as RM is defined with -f already.

Okay, thanks.

Ciao,
Dscho

Re: [PATCH 3/3] Enable parallel tests

[Junio C Hamano]

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

> Hi,
>
> On Thu, 7 Aug 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > Parallel mode also triggers removal of the trash directory in the test 
>> > itself if everything went fine, so that the trash directories do not 
>> > pile up only to be removed at the very end.
>> 
>> I think making the tests remove their own mess makes sense regardless.
>
> When I add tests, I first run the appropriate t/t*.sh, then expect what is 
> in trash directory, then extend the test.  So at least I need an option to 
> keep the directory.

That's easy.  I do the same as you but do so by disabling "test_done" ;-)

Re: [PATCH 3/3] Enable parallel tests

[René Scharfe]

Johannes Schindelin schrieb:
> On multiprocessor machines, or with I/O heavy tests (that leave the
> CPU waiting a lot), it makes sense to parallelize the tests.
> 
> However, care has to be taken that the different jobs use different
> trash directories.

Good idea!

> This commit does so, by inspecting the MAKEFLAGS variable to detect
> if the option "-j" or "--jobs" was passed to make.  In that case, the
> test is run with the new "--parallel" option.

How about making the test harness be able to run multiple tests in
parallel by default, i.e. always use a different trash directory name
for each test, without adding the new option?  The implementation would
be a bit simpler (no -j detection needed) and the documentation would be
simpler, too.  We could say "look in 'trash directory/tNNNN'" instead of
"look in this place unless you used -j".

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 11c0275..c5868c4 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -74,6 +74,7 @@ esac
>  	) &&
>  	color=t
>  
> +test="trash directory"
>  while test "$#" -ne 0
>  do
>  	case "$1" in
> @@ -94,6 +95,10 @@ do
>  	--no-python)
>  		# noop now...
>  		shift ;;
> +	--parallel)
> +		test="$test.$(basename "$0" .sh)"
> +		remove_trash="$(pwd)/$test"
> +		shift ;;

test="trash directory/$this_test"?

The advantage would be that all trash was still inside "trash
directory".  Not sure if the extra directory level would break
something.  (Note: $this_test is defined a bit later in the script.)

test="trash for $this_test"?

This one still has a space in it..

>  	*)
>  		break ;;
>  	esac
> @@ -449,6 +454,11 @@ test_done () {
>  		# we will leave things as they are.
>  
>  		say_color pass "passed all $msg"
> +
> +		test ! -z = "$remove_trash" &&

This test succeeds always, because = is not an empty string.

René

Re: [PATCH 3/3] Enable parallel tests

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> test="trash directory/$this_test"?
>
> The advantage would be that all trash was still inside "trash
> directory".  Not sure if the extra directory level would break
> something.  (Note: $this_test is defined a bit later in the script.)

The extra directory level may break some tests that refer to their
precomputed test vectors in ../tXXXX, but I think they should be fixed
regardless.  That's what $TEST_DIRECTORY is for.

I'd very much prefer having 't/trash directory/t1234-test-name/' so that
we can say "make clean" to clean "t/trash directory" in one go.

[PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Junio C Hamano]

I'll push this out as 'test-deeper' branch to repo.or.cz (alt-git.git)
because the test suite has unprintable bytes that are inappropriate for
e-mail transmission.

-- >8 --
Many test scripts assumed that they will start in a 'trash' subdirectory
that is a single level down from the t/ directory, and referred to their
test vector files by asking for files like "../t9999/expect".  This will
break if we move the 'trash' subdirectory elsewhere.

To solve this, we earlier introduced "$TEST_DIRECTORY" so that they can
refer to t/ directory reliably.  This finally makes all the tests use
it to refer to the outside environment.

With this patch, and a one-liner not included here (because it would
contradict with what Dscho really wants to do):

| diff --git a/t/test-lib.sh b/t/test-lib.sh
| index 70ea7e0..60e69e4 100644
| --- a/t/test-lib.sh
| +++ b/t/test-lib.sh
| @@ -485,7 +485,7 @@ fi
|  . ../GIT-BUILD-OPTIONS
|
|  # Test repository
| -test="trash directory"
| +test="trash directory/another level/yet another"
|  rm -fr "$test" || {
|         trap - exit
|         echo >&5 "FATAL: Cannot prepare test area"

all the tests still pass, but we would want extra sets of eyeballs on this
type of change to really make sure.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0022-crlf-rename.sh                   |    4 ++--
 t/t1000-read-tree-m-3way.sh              |    2 +-
 t/t3900-i18n-commit.sh                   |   18 +++++++++---------
 t/t3901-i18n-patch.sh                    |   28 ++++++++++++++--------------
 t/t4000-diff-format.sh                   |    2 +-
 t/t4001-diff-rename.sh                   |    2 +-
 t/t4002-diff-basic.sh                    |    2 +-
 t/t4003-diff-rename-1.sh                 |    6 +++---
 t/t4004-diff-rename-symlink.sh           |    2 +-
 t/t4005-diff-rename-2.sh                 |    6 +++---
 t/t4007-rename-3.sh                      |    4 ++--
 t/t4008-diff-break-rewrite.sh            |    6 +++---
 t/t4009-diff-rename-4.sh                 |    6 +++---
 t/t4010-diff-pathspec.sh                 |    2 +-
 t/t4011-diff-symlink.sh                  |    2 +-
 t/t4012-diff-binary.sh                   |    2 +-
 t/t4013-diff-various.sh                  |    2 +-
 t/t4015-diff-whitespace.sh               |    2 +-
 t/t4020-diff-external.sh                 |    2 +-
 t/t4022-diff-rewrite.sh                  |    4 ++--
 t/t4023-diff-rename-typechange.sh        |   14 +++++++-------
 t/t4027-diff-submodule.sh                |    2 +-
 t/t4100-apply-stat.sh                    |    4 ++--
 t/t4101-apply-nonl.sh                    |    2 +-
 t/t5100-mailinfo.sh                      |   18 +++++++++---------
 t/t5515-fetch-merge-logic.sh             |    4 ++--
 t/t5540-http-push.sh                     |    2 +-
 t/t6002-rev-list-bisect.sh               |    2 +-
 t/t6003-rev-list-topo-order.sh           |    2 +-
 t/t6023-merge-file.sh                    |    2 +-
 t/t6027-merge-binary.sh                  |    2 +-
 t/t6101-rev-parse-parents.sh             |    2 +-
 t/t6200-fmt-merge-msg.sh                 |    4 ++--
 t/t7001-mv.sh                            |    4 ++--
 t/t7004-tag.sh                           |    2 +-
 t/t7101-reset.sh                         |   10 +++++-----
 t/t7500-commit.sh                        |   16 ++++++++--------
 t/t8001-annotate.sh                      |    2 +-
 t/t8002-blame.sh                         |    2 +-
 t/t9110-git-svn-use-svm-props.sh         |    2 +-
 t/t9111-git-svn-use-svnsync-props.sh     |    2 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |    2 +-
 t/t9121-git-svn-fetch-renamed-dir.sh     |    2 +-
 t/t9200-git-cvsexportcommit.sh           |   14 +++++++-------
 t/t9300-fast-import.sh                   |    2 +-
 t/t9301-fast-export.sh                   |    2 +-
 t/t9500-gitweb-standalone-no-errors.sh   |   16 ++++++++--------
 t/t9700-perl-git.sh                      |    2 +-
 t/t9700/test.pl                          |    3 ---
 t/test-lib.sh                            |    2 +-
 50 files changed, 123 insertions(+), 126 deletions(-)

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Johannes Schindelin]

Hi,

On Fri, 8 Aug 2008, Junio C Hamano wrote:

> |  # Test repository
> | -test="trash directory"
> | +test="trash directory/another level/yet another"

Oh my.  If you continue like that, we are soon going to hit PATH_MAX on 
some systems (*cough* Windows *cough*), badly.

>  t/t0022-crlf-rename.sh                   |    4 ++--
>  t/t1000-read-tree-m-3way.sh              |    2 +-
>  t/t3900-i18n-commit.sh                   |   18 +++++++++---------
>  t/t3901-i18n-patch.sh                    |   28 ++++++++++++++--------------
>  t/t4000-diff-format.sh                   |    2 +-
>  t/t4001-diff-rename.sh                   |    2 +-
>  t/t4002-diff-basic.sh                    |    2 +-
>  t/t4003-diff-rename-1.sh                 |    6 +++---
>  t/t4004-diff-rename-symlink.sh           |    2 +-
>  t/t4005-diff-rename-2.sh                 |    6 +++---
>  t/t4007-rename-3.sh                      |    4 ++--
>  t/t4008-diff-break-rewrite.sh            |    6 +++---
>  t/t4009-diff-rename-4.sh                 |    6 +++---
>  t/t4010-diff-pathspec.sh                 |    2 +-
>  t/t4011-diff-symlink.sh                  |    2 +-
>  t/t4012-diff-binary.sh                   |    2 +-
>  t/t4013-diff-various.sh                  |    2 +-
>  t/t4015-diff-whitespace.sh               |    2 +-
>  t/t4020-diff-external.sh                 |    2 +-
>  t/t4022-diff-rewrite.sh                  |    4 ++--
>  t/t4023-diff-rename-typechange.sh        |   14 +++++++-------
>  t/t4027-diff-submodule.sh                |    2 +-
>  t/t4100-apply-stat.sh                    |    4 ++--
>  t/t4101-apply-nonl.sh                    |    2 +-
>  t/t5100-mailinfo.sh                      |   18 +++++++++---------
>  t/t5515-fetch-merge-logic.sh             |    4 ++--
>  t/t5540-http-push.sh                     |    2 +-
>  t/t6002-rev-list-bisect.sh               |    2 +-
>  t/t6003-rev-list-topo-order.sh           |    2 +-
>  t/t6023-merge-file.sh                    |    2 +-
>  t/t6027-merge-binary.sh                  |    2 +-
>  t/t6101-rev-parse-parents.sh             |    2 +-
>  t/t6200-fmt-merge-msg.sh                 |    4 ++--
>  t/t7001-mv.sh                            |    4 ++--
>  t/t7004-tag.sh                           |    2 +-
>  t/t7101-reset.sh                         |   10 +++++-----
>  t/t7500-commit.sh                        |   16 ++++++++--------
>  t/t8001-annotate.sh                      |    2 +-
>  t/t8002-blame.sh                         |    2 +-
>  t/t9110-git-svn-use-svm-props.sh         |    2 +-
>  t/t9111-git-svn-use-svnsync-props.sh     |    2 +-
>  t/t9115-git-svn-dcommit-funky-renames.sh |    2 +-
>  t/t9121-git-svn-fetch-renamed-dir.sh     |    2 +-
>  t/t9200-git-cvsexportcommit.sh           |   14 +++++++-------
>  t/t9300-fast-import.sh                   |    2 +-
>  t/t9301-fast-export.sh                   |    2 +-
>  t/t9500-gitweb-standalone-no-errors.sh   |   16 ++++++++--------
>  t/t9700-perl-git.sh                      |    2 +-
>  t/t9700/test.pl                          |    3 ---
>  t/test-lib.sh                            |    2 +-
>  50 files changed, 123 insertions(+), 126 deletions(-)

Frankly, I do not have the time.  It is not only about looking what you 
changed, but also what you did not change.

Besides, I do not see the point.  "clean" can just as well

	$(RM) -r 'trash directory.t'[0-9]*

Ciao,
Dscho

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Junio C Hamano]

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

> Frankly, I do not have the time.  It is not only about looking what you 
> changed, but also what you did not change.

That's Ok.  You are not (and you shouldn't be) the only person who is
capable of reviewing and helping the development process ;-)

Hint, hint...

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Stephan Beyer]

Hi,

Junio C Hamano wrote:
> Hint, hint...

I've taken only a short look, and:

	$ GIT_TEST_HTTPD=1 ./t5540-http-push.sh
	* skipping test, web server setup failed
	* passed all 0 test(s)

after the following change, it became:

	$ GIT_TEST_HTTPD=1 ./t5540-http-push.sh
	*   ok 1: setup remote repository
	*   ok 2: clone remote repository
	*   still broken 3: push to remote repository
	*   still broken 4: create and delete remote branch
	* still have 2 known breakage(s)
	* passed all remaining 2 test(s)

Regards.

--snip--

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index dc473df..6ac312b 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -14,7 +14,7 @@ fi
 LIB_HTTPD_PATH=${LIB_HTTPD_PATH-'/usr/sbin/apache2'}
 LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
 
-TEST_PATH="$PWD"/../lib-httpd
+TEST_PATH="$TEST_DIRECTORY"/lib-httpd
 HTTPD_ROOT_PATH="$PWD"/httpd
 HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
 
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Olivier Marin]

Junio C Hamano a écrit :
> 
> all the tests still pass, but we would want extra sets of eyeballs on this
> type of change to really make sure.

OK, I read the diff and found some trivial quoting issues that will break the
following tests if $TEST_DIRECTORY contain a space:

>  t/t4101-apply-nonl.sh                    |    2 +-
>  t/t5100-mailinfo.sh                      |   18 +++++++++---------
>  t/t7500-commit.sh                        |   16 ++++++++--------

Olivier.

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Junio C Hamano]

Olivier Marin <dkr+ml.git@free.fr> writes:

> Junio C Hamano a écrit :
>> 
>> all the tests still pass, but we would want extra sets of eyeballs on this
>> type of change to really make sure.
>
> OK, I read the diff and found some trivial quoting issues that will break the
> following tests if $TEST_DIRECTORY contain a space:
>
>>  t/t4101-apply-nonl.sh                    |    2 +-
>>  t/t5100-mailinfo.sh                      |   18 +++++++++---------
>>  t/t7500-commit.sh                        |   16 ++++++++--------

Ah, GIT_EDITOR is dereferenced twice.  Sheesh.

We could probably use test_set_editor() function from test-lib.sh, hmm?

Re: [PATCH] tests: use $TEST_DIRECTORY to refer to the t/ directory

[Junio C Hamano]

> OK, I read the diff and found some trivial quoting issues that will break the
> following tests if $TEST_DIRECTORY contain a space:

Thanks.  I think this should catch all of them.  I've run the tests in a
clone that has SP in it.

 t/t4101-apply-nonl.sh |    7 ++++---
 t/t5100-mailinfo.sh   |   17 +++++++++--------
 t/t7500-commit.sh     |   39 +++++++++++++++++++++++++++++----------
 3 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/t/t4101-apply-nonl.sh b/t/t4101-apply-nonl.sh
index 1391d20..e3443d0 100755
--- a/t/t4101-apply-nonl.sh
+++ b/t/t4101-apply-nonl.sh
@@ -21,9 +21,10 @@ do
   do
     test $i -eq $j && continue
     cat frotz.$i >frotz
-    test_expect_success \
-        "apply diff between $i and $j" \
-	"git apply <"$TEST_DIRECTORY"/t4101/diff.$i-$j && diff frotz.$j frotz"
+    test_expect_success "apply diff between $i and $j" '
+	git apply <"$TEST_DIRECTORY"/t4101/diff.$i-$j &&
+	test_cmp frotz.$j frotz
+    '
   done
 done
 
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index a40d48b..c3ab881 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -15,20 +15,21 @@ test_expect_success 'split sample box' \
 
 for mail in `echo 00*`
 do
-	test_expect_success "mailinfo $mail" \
-		"git mailinfo -u msg$mail patch$mail <$mail >info$mail &&
+	test_expect_success "mailinfo $mail" '
+		git mailinfo -u msg$mail patch$mail <$mail >info$mail &&
 		echo msg &&
-		diff "$TEST_DIRECTORY"/t5100/msg$mail msg$mail &&
+		test_cmp "$TEST_DIRECTORY"/t5100/msg$mail msg$mail &&
 		echo patch &&
-		diff "$TEST_DIRECTORY"/t5100/patch$mail patch$mail &&
+		test_cmp "$TEST_DIRECTORY"/t5100/patch$mail patch$mail &&
 		echo info &&
-		diff "$TEST_DIRECTORY"/t5100/info$mail info$mail"
+		test_cmp "$TEST_DIRECTORY"/t5100/info$mail info$mail
+	'
 done
 
 test_expect_success 'respect NULs' '
 
 	git mailsplit -d3 -o. "$TEST_DIRECTORY"/t5100/nul-plain &&
-	cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
+	test_cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
 	(cat 001 | git mailinfo msg patch) &&
 	test 4 = $(wc -l < patch)
 
@@ -37,9 +38,9 @@ test_expect_success 'respect NULs' '
 test_expect_success 'Preserve NULs out of MIME encoded message' '
 
 	git mailsplit -d5 -o. "$TEST_DIRECTORY"/t5100/nul-b64.in &&
-	cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 00001 &&
+	test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 00001 &&
 	git mailinfo msg patch <00001 &&
-	cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
+	test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
 
 '
 
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 86c1647..7ae0bd0 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -46,15 +46,24 @@ test_expect_success 'unedited template with comments should not commit' '
 '
 
 test_expect_success 'a Signed-off-by line by itself should not commit' '
-	! GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-signed-off git commit --template "$TEMPLATE"
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-signed-off &&
+		test_must_fail git commit --template "$TEMPLATE"
+	)
 '
 
 test_expect_success 'adding comments to a template should not commit' '
-	! GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-comments git commit --template "$TEMPLATE"
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-comments &&
+		test_must_fail git commit --template "$TEMPLATE"
+	)
 '
 
 test_expect_success 'adding real content to a template should commit' '
-	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit --template "$TEMPLATE" &&
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+		git commit --template "$TEMPLATE"
+	) &&
 	commit_msg_is "template linecommit message"
 '
 
@@ -62,7 +71,10 @@ test_expect_success '-t option should be short for --template' '
 	echo "short template" > "$TEMPLATE" &&
 	echo "new content" >> foo &&
 	git add foo &&
-	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit -t "$TEMPLATE" &&
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+		git commit -t "$TEMPLATE"
+	) &&
 	commit_msg_is "short templatecommit message"
 '
 
@@ -71,7 +83,10 @@ test_expect_success 'config-specified template should commit' '
 	git config commit.template "$TEMPLATE" &&
 	echo "more content" >> foo &&
 	git add foo &&
-	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit &&
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+		git commit
+	) &&
 	git config --unset commit.template &&
 	commit_msg_is "new templatecommit message"
 '
@@ -88,8 +103,10 @@ test_expect_success 'commit message from file should override template' '
 	echo "content galore" >> foo &&
 	git add foo &&
 	echo "standard input msg" |
-		GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit \
-			--template "$TEMPLATE" --file - &&
+	(
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+		git commit --template "$TEMPLATE" --file -
+	) &&
 	commit_msg_is "standard input msg"
 '
 
@@ -132,10 +149,12 @@ EOF
 
 test_expect_success '--signoff' '
 	echo "yet another content *narf*" >> foo &&
-	echo "zort" |
-		GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit -s -F - foo &&
+	echo "zort" | (
+		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+		git commit -s -F - foo
+	) &&
 	git cat-file commit HEAD | sed "1,/^$/d" > output &&
-	diff expect output
+	test_cmp expect output
 '
 
 test_expect_success 'commit message from file (1)' '
-- 
1.6.0.rc2.22.g71b99

Re: [PATCH 3/3] Enable parallel tests

[Johannes Schindelin]

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

Hi,

On Fri, 8 Aug 2008, René Scharfe wrote:

> Johannes Schindelin schrieb:
> 
> > This commit does so, by inspecting the MAKEFLAGS variable to detect if 
> > the option "-j" or "--jobs" was passed to make.  In that case, the 
> > test is run with the new "--parallel" option.
> 
> How about making the test harness be able to run multiple tests in
> parallel by default, i.e. always use a different trash directory name
> for each test, without adding the new option?  The implementation would
> be a bit simpler (no -j detection needed) and the documentation would be
> simpler, too.

I am totally opposed to dropping the -j detection.  This is what cost me 3 
hours to research/implement.  *sighs*

> >  	*)
> >  		break ;;
> >  	esac
> > @@ -449,6 +454,11 @@ test_done () {
> >  		# we will leave things as they are.
> >  
> >  		say_color pass "passed all $msg"
> > +
> > +		test ! -z = "$remove_trash" &&
> 
> This test succeeds always, because = is not an empty string.

Thanks.

Ciao,
Dscho

[PATCH 3/3 v2] Enable parallel tests

[Johannes Schindelin]

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


On multiprocessor machines, or with I/O heavy tests (that leave the
CPU waiting a lot), it makes sense to parallelize the tests.

However, care has to be taken that the different jobs use different
trash directories.

This commit does so, by creating the trash directories with a suffix
that is unique with regard to the test, as it is the test's base name.

Further, the trash directory is removed in the test itself if
everything went fine, so that the trash directories do not
pile up only to be removed at the very end.

If a test failed, the trash directory is not removed.  Chances are
that the exact error message is lost in the clutter, but you can still
see what test failed from the name of the trash directory, and repeat
the test (without -j).

If all was good, you will see the aggregated results.

Suggestions to simplify this commit came from Junio and René.

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

	> On Fri, 8 Aug 2008, René Scharfe wrote:
	> 
	> > The implementation would be a bit simpler (no -j detection 
	> > needed) and the documentation would be simpler, too.

	Oh well, here it goes.

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

diff --git a/t/Makefile b/t/Makefile
index aa952e1..ed49c20 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -39,4 +39,3 @@ full-svn-test:
 	$(MAKE) $(TSVN) GIT_SVN_NO_OPTIMIZE_COMMITS=0 LC_ALL=en_US.UTF-8
 
 .PHONY: pre-clean $(T) aggregate-results clean
-.NOTPARALLEL:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11c0275..75c8a36 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -449,6 +449,11 @@ test_done () {
 		# we will leave things as they are.
 
 		say_color pass "passed all $msg"
+
+		test ! -z "$remove_trash" &&
+		cd "$(dirname "$remove_trash")" &&
+		rm -rf "$(basename "$remove_trash")"
+
 		exit 0 ;;
 
 	*)
@@ -485,7 +490,8 @@ fi
 . ../GIT-BUILD-OPTIONS
 
 # Test repository
-test="trash directory"
+test="trash directory.$(basename "$0" .sh)"
+remove_trash="$(pwd)/$test"
 rm -fr "$test" || {
 	trap - exit
 	echo >&5 "FATAL: Cannot prepare test area"
-- 
1.6.0.rc2.23.gd08e9

Re: [PATCH 3/3 v2] Enable parallel tests

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> @@ -485,7 +490,8 @@ fi
>  . ../GIT-BUILD-OPTIONS
>  
>  # Test repository
> -test="trash directory"
> +test="trash directory.$(basename "$0" .sh)"
> +remove_trash="$(pwd)/$test"
>  rm -fr "$test" || {
>  	trap - exit
>  	echo >&5 "FATAL: Cannot prepare test area"

Please also change t/README, there is a text like:

  [...]
   database and chdir(2) into it.  This directory is 't/trash directory'
   if you must know, but I do not think you care.

If the subdirectory variant is chosen ("trash directory/foo/" instead
of "trash directory.foo/"), then

   This directory is below 't/trash directory'."

could be sufficient.

Btw, Junio, about the passage: "I do not think you care" -- I cared :)
Sometimes it's nice to change to 'trash directory' and do git log, git diff,
git show or whatever.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 3/3 v2] Enable parallel tests

[Johannes Schindelin]

Hi,

On Fri, 8 Aug 2008, Stephan Beyer wrote:

> Please also change t/README

No time.  But I am sure should you provide a patch that Junio would be 
able to squash it in.  Provided he takes the series at all.

> Btw, Junio, about the passage: "I do not think you care" -- I cared :) 
> Sometimes it's nice to change to 'trash directory' and do git log, git 
> diff, git show or whatever.

Junio already adressed that.

Ciao,
Dscho

Re: [PATCH 0/3] Enable parallelized tests

[SZEDER Gábor]

Hi,

On Fri, Aug 08, 2008 at 07:59:08AM +0200, Johannes Schindelin wrote:
> This patch pair enables parallel tests.
Glad to see that others have also picked up this topic.  I have also
written parallel testing patches back in March, but did not send them
out, as there were issues I could not resolve in a satisfactory way -
and your patches doesn't seem to address theim either.

There are a few tests involving http transfers, namely:
  t5540-http-push.sh
  t9115-git-svn-dcommit-funky-renames.sh 
  t9118-git-svn-funky-branch-names.sh
  t9120-git-svn-clone-with-percent-escapes.sh

These start an apache web server at the beginning of the test and shut
it down after the test finished.  Obviously, if you run tests in
parallel then these tests can also run concurrently.  The problem is
with the svn tests, as all those tests use the same directory and port
for the web server, resulting in failed tests with -jN.

t5540 is not an issue at the moment, as it uses lib-httpd.sh, hence a
different directory and a (possibly) different port than the svn
tests.  However, who knows, in the future we might have other tests
using lib-httpd.sh.

The simplest solution would be to disable parallel testing altogether
if http tests are enabled (GIT_TEST_HTTPD and SVN_HTTPD_PORT).  But
IMHO it would be much better to have only one apache process for the
_whole_ testsuite, and to have different paths for the test repos
under its documentroot.  But yes, it's more difficult to implement; at
least I could not do it.


Regards,
Gábor

Re: [PATCH 0/3] Enable parallelized tests

[Stephan Beyer]

Hi,

> There are a few tests involving http transfers, namely:
>   t5540-http-push.sh
>   t9115-git-svn-dcommit-funky-renames.sh 
>   t9118-git-svn-funky-branch-names.sh
>   t9120-git-svn-clone-with-percent-escapes.sh
> 
> These start an apache web server at the beginning of the test and shut
> it down after the test finished.  Obviously, if you run tests in
> parallel then these tests can also run concurrently.  The problem is
> with the svn tests, as all those tests use the same directory and port
> for the web server, resulting in failed tests with -jN.
> 
> t5540 is not an issue at the moment, as it uses lib-httpd.sh, hence a
> different directory and a (possibly) different port than the svn
> tests.  However, who knows, in the future we might have other tests
> using lib-httpd.sh.
> 
> The simplest solution would be to disable parallel testing altogether
> if http tests are enabled (GIT_TEST_HTTPD and SVN_HTTPD_PORT).

Hm, another simple(?) solution could be to make the tests that try to
access the same port/directory/whatever depend on each other.
Well, this could bloat the Makefile, but seems to be clean (at least to
me).

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 0/3] Enable parallelized tests

[Johannes Schindelin]

Hi,

On Fri, 8 Aug 2008, Stephan Beyer wrote:

> Hm, another simple(?) solution could be to make the tests that try to 
> access the same port/directory/whatever depend on each other.

No.  Because then you cannot run them independently anymore.

Ciao,
Dscho

Re: [PATCH 0/3] Enable parallelized tests

[Stephan Beyer]

Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 8 Aug 2008, Stephan Beyer wrote:
> 
> > Hm, another simple(?) solution could be to make the tests that try to 
> > access the same port/directory/whatever depend on each other.
> 
> No.  Because then you cannot run them independently anymore.

Sorry, I meant, "depend on each other _in the Makefile_".

So "./t91xy-git-svn-foo.sh" will work independently, won't it?
What does not work independently is "make t91xy-git-svn-foo.sh"
but is it that bad?

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 0/3] Enable parallelized tests

[Johannes Schindelin]

Hi,

On Fri, 8 Aug 2008, Stephan Beyer wrote:

> Johannes Schindelin wrote:
> > 
> > On Fri, 8 Aug 2008, Stephan Beyer wrote:
> > 
> > > Hm, another simple(?) solution could be to make the tests that try 
> > > to access the same port/directory/whatever depend on each other.
> > 
> > No.  Because then you cannot run them independently anymore.
> 
> Sorry, I meant, "depend on each other _in the Makefile_".
> 
> So "./t91xy-git-svn-foo.sh" will work independently, won't it? What does 
> not work independently is "make t91xy-git-svn-foo.sh" but is it that 
> bad?

I went out of my way to keep that functionality intact.  But of course, we 
could throw it away.  We could also throw Git away and go back to tarballs 
and patches.

Ciao,
Dscho

P.S.: And I do not think it is clean to say that one test depends on the 
other.  Because they do not.  They depend on not being run concurrently.  
But that could be fixed.

Re: [PATCH 0/3] Enable parallelized tests

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> P.S.: And I do not think it is clean to say that one test depends on the 
> other.  Because they do not.  They depend on not being run concurrently.

Ah ok, you're right here.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F