[PATCH 0/3] Enable parallelized tests

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

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

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

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

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

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

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

René

Hi,

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

Johannes Schindelin schrieb:

quoted

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*

quoted

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

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

	> 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

Hi,

Johannes Schindelin wrote:

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 [off-list ref], PGP 0x6EDDD207FCC5040F

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

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

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 [off-list ref], PGP 0x6EDDD207FCC5040F

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

Johannes Schindelin wrote:

Hi,

On Fri, 8 Aug 2008, Stephan Beyer wrote:

quoted

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 [off-list ref], PGP 0x6EDDD207FCC5040F

Hi,

On Fri, 8 Aug 2008, Stephan Beyer wrote:

Johannes Schindelin wrote:

quoted

On Fri, 8 Aug 2008, Stephan Beyer wrote:

quoted

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.

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 [off-list ref], PGP 0x6EDDD207FCC5040F