[PATCH 0/6] --valgrind improvements

[PATCH 0/6] --valgrind improvements

[Thomas Rast]

Peff and me discussed improving the usability of --valgrind testing.
In particular, two ideas that came up were options for running only a
subset of the tests in a file under --valgrind, and for running a
single test script under valgrind while exploiting parallelism.

So here's a little series.  It goes like this:

  test-lib: enable MALLOC_* for the actual tests

Fix for an unrelated bug that I came across.

  test-lib: refactor $GIT_SKIP_TESTS matching
  test-lib: verbose mode for only tests matching a pattern
  test-lib: valgrind for only tests matching a pattern

An option --valgrind-only=<patterns> that lets you run only the
subtest matching <patterns> under valgrind.

  test-lib: allow prefixing a custom string before "ok N" etc.
  test-lib: support running tests under valgrind in parallel

An option --valgrind-parallel=<n> to run <n> instances in parallel,
each of which runs every <n>-th test under valgrind, staggered so that
they cover everything.  It's a bit of a hack, and thus RFC, but gives
decent results.  On my 2-core laptop I measured a just over 2x
speedup.  On a 6-core it starts falling off because of the extra
(non-valgrind) runs, resulting in a 4.8x speedup.

One open issue with the last patch that currently eludes me: if I
combine --valgrind-parallel with any --valgrind=*, there are lots of
errors as (apparently) the valgrind wrapper setups race against each
other.  However, without any --valgrind=* (thus defaulting to
'memcheck') this doesn't happen.


Thomas Rast (6):
  test-lib: enable MALLOC_* for the actual tests
  test-lib: refactor $GIT_SKIP_TESTS matching
  test-lib: verbose mode for only tests matching a pattern
  test-lib: valgrind for only tests matching a pattern
  test-lib: allow prefixing a custom string before "ok N" etc.
  test-lib: support running tests under valgrind in parallel

 t/README               |  10 +++
 t/test-lib.sh          | 175 ++++++++++++++++++++++++++++++++++++++++---------
 t/valgrind/valgrind.sh |   3 +
 3 files changed, 156 insertions(+), 32 deletions(-)

-- 
1.8.3.rc2.393.g8636c0b

[PATCH 1/6] test-lib: enable MALLOC_* for the actual tests

[Thomas Rast]

1b3185f (MALLOC_CHECK: various clean-ups, 2012-09-14) moved around the
MALLOC_CHECK_ and MALLOC_PERTURB_ assignments, intending to limit
their effect to only the test runs.  However, they were actually
enabled only during test cleanup.  Call setup/teardown_malloc_check
also around the evaluation of the actual test snippet.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ca6bdef..229f5f7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -337,8 +337,10 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+	setup_malloc_check
 	test_eval_ "$1"
 	eval_ret=$?
+	teardown_malloc_check
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-- 
1.8.3.rc2.393.g8636c0b

[PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

It's already used twice, and we will have more of the same kind of
matching in a minute.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 229f5f7..c5a80d1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -328,6 +328,20 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+match_pattern_list () {
+	arg="$1"
+	shift
+	test -z "$*" && return 1
+	for pat in $@
+	do
+		case "$arg" in
+		$pat)
+			return 0
+		esac
+	done
+	return 1
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -358,14 +372,10 @@ test_run_ () {
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
-	for skp in $GIT_SKIP_TESTS
-	do
-		case $this_test.$test_count in
-		$skp)
-			to_skip=t
-			break
-		esac
-	done
+	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	then
+		to_skip=t
+	fi
 	if test -z "$to_skip" && test -n "$test_prereq" &&
 	   ! test_have_prereq "$test_prereq"
 	then
@@ -630,15 +640,12 @@ cd -P "$TRASH_DIRECTORY" || exit 1
 
 this_test=${0##*/}
 this_test=${this_test%%-*}
-for skp in $GIT_SKIP_TESTS
-do
-	case "$this_test" in
-	$skp)
-		say_color info >&3 "skipping test $this_test altogether"
-		skip_all="skip all tests in $this_test"
-		test_done
-	esac
-done
+if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+then
+	say_color info >&3 "skipping test $this_test altogether"
+	skip_all="skip all tests in $this_test"
+	test_done
+fi
 
 # Provide an implementation of the 'yes' utility
 yes () {
-- 
1.8.3.rc2.393.g8636c0b

[PATCH 3/6] test-lib: verbose mode for only tests matching a pattern

[Thomas Rast]

With the new --verbose-only=<pattern> option, one can enable --verbose
at a per-test granularity.  The pattern is matched against the test
number, e.g.

  ./t0000-basic.sh --verbose-only='2[0-2]'

to see only the full output of test 20-22, while showing the rest in the
one-liner format.

This is arguably not *too* useful on its own, but makes the next patch
easier to follow.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README      |  5 +++++
 t/test-lib.sh | 29 +++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index e669bb3..9c8f9b1 100644
--- a/t/README
+++ b/t/README
@@ -76,6 +76,11 @@ appropriately before running "make".
 	command being run and their output if any are also
 	output.
 
+--verbose-only=<pattern>::
+	Like --verbose, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --debug::
 	This may help the person who is developing a new test.
 	It causes the command defined with test_debug to run.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c5a80d1..01e7445 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,6 +184,9 @@ do
 		help=t; shift ;;
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
 		verbose=t; shift ;;
+	--verbose-only=*)
+		verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		# Ignore --quiet under a TAP::Harness. Saying how many tests
 		# passed without the ok/not ok details is always an error.
@@ -342,6 +345,24 @@ match_pattern_list () {
 	return 1
 }
 
+toggle_verbose () {
+	test -z "$verbose_only" && return
+	if match_pattern_list $test_count $verbose_only
+	then
+		exec 4>&2 3>&1
+	else
+		exec 4>/dev/null 3>/dev/null
+	fi
+}
+
+setup_test_eval () {
+	setup_malloc_check
+	toggle_verbose
+}
+teardown_test_eval () {
+	teardown_malloc_check
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -351,16 +372,16 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
-	setup_malloc_check
+	setup_test_eval
 	test_eval_ "$1"
 	eval_ret=$?
-	teardown_malloc_check
+	teardown_test_eval
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-		setup_malloc_check
+		setup_test_eval
 		test_eval_ "$test_cleanup"
-		teardown_malloc_check
+		teardown_test_eval
 	fi
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
 	then
-- 
1.8.3.rc2.393.g8636c0b

[PATCH 4/6] test-lib: valgrind for only tests matching a pattern

[Thomas Rast]

With the new --valgrind-only=<pattern> option, one can enable
--valgrind at a per-test granularity, exactly analogous to
--verbose-only from the previous commit.

The options are wired such that --valgrind implies --verbose (as
before), but --valgrind-only=<pattern> implies
--verbose-only=<pattern> unless --verbose is also in effect.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README               |  5 +++++
 t/test-lib.sh          | 30 +++++++++++++++++++++++++++++-
 t/valgrind/valgrind.sh |  3 +++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9c8f9b1..bffd484 100644
--- a/t/README
+++ b/t/README
@@ -126,6 +126,11 @@ appropriately before running "make".
 	the 't/valgrind/' directory and use the commands under
 	't/valgrind/bin/'.
 
+--valgrind-only=<pattern>::
+	Like --valgrind, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --tee::
 	In addition to printing the test output to the terminal,
 	write it to files named 't/test-results/$TEST_NAME.out'.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 01e7445..9ae7c7b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -201,6 +201,9 @@ do
 	--valgrind=*)
 		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-only=*)
+		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -211,7 +214,14 @@ do
 	esac
 done
 
-test -n "$valgrind" && verbose=t
+if test -n "$valgrind_only"
+then
+	test -z "$valgrind" && valgrind=memcheck
+	test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+	verbose=t
+fi
 
 if test -n "$color"
 then
@@ -355,8 +365,23 @@ toggle_verbose () {
 	fi
 }
 
+toggle_valgrind () {
+	test -z "$GIT_VALGRIND" && return
+	if test -z "$valgrind_only"
+	then
+		GIT_VALGRIND_ENABLED=t
+		return
+	fi
+	GIT_VALGRIND_ENABLED=
+	if match_pattern_list $test_count $valgrind_only
+	then
+		GIT_VALGRIND_ENABLED=t
+	fi
+}
+
 setup_test_eval () {
 	setup_malloc_check
+	toggle_valgrind
 	toggle_verbose
 }
 teardown_test_eval () {
@@ -571,6 +596,9 @@ then
 	export GIT_VALGRIND
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
+	GIT_VALGRIND_ENABLED=t
+	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 6b87c91..4215303 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -4,6 +4,9 @@ base=$(basename "$0")
 
 TOOL_OPTIONS='--leak-check=no'
 
+test -z "$GIT_VALGRIND_ENABLED" &&
+exec "$GIT_VALGRIND"/../../"$base" "$@"
+
 case "$GIT_VALGRIND_MODE" in
 memcheck-fast)
 	;;
-- 
1.8.3.rc2.393.g8636c0b

[PATCH 5/6] test-lib: allow prefixing a custom string before "ok N" etc.

[Thomas Rast]

This is not really meant for external use, but allows the next commit
to neatly distinguish between sub-tests and the main run.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9ae7c7b..55fa749 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -209,6 +209,9 @@ do
 	--root=*)
 		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--statusprefix=*)
+		statusprefix=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	*)
 		echo "error: unknown test option '$1'" >&2; exit 1 ;;
 	esac
@@ -316,12 +319,12 @@ trap 'die' EXIT
 
 test_ok_ () {
 	test_success=$(($test_success + 1))
-	say_color "" "ok $test_count - $@"
+	say_color "" "${statusprefix}ok $test_count - $@"
 }
 
 test_failure_ () {
 	test_failure=$(($test_failure + 1))
-	say_color error "not ok $test_count - $1"
+	say_color error "${statusprefix}not ok $test_count - $1"
 	shift
 	echo "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
@@ -329,12 +332,12 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color error "ok $test_count - $@ # TODO known breakage vanished"
+	say_color error "${statusprefix}ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
-	say_color warn "not ok $test_count - $@ # TODO known breakage"
+	say_color warn "${statusprefix}not ok $test_count - $@ # TODO known breakage"
 }
 
 test_debug () {
@@ -435,8 +438,8 @@ test_skip () {
 			of_prereq=" of $test_prereq"
 		fi
 
-		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
+		say_color skip >&3 "${statusprefix}skipping test: $@"
+		say_color skip "${statusprefix}ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
 		: true
 		;;
 	*)
@@ -472,11 +475,11 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
+		say_color error "${statusprefix}# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
-		say_color warn "# still have $test_broken known breakage(s)"
+		say_color warn "${statusprefix}# still have $test_broken known breakage(s)"
 	fi
 	if test "$test_broken" != 0 || test "$test_fixed" != 0
 	then
@@ -499,9 +502,9 @@ test_done () {
 		then
 			if test $test_remaining -gt 0
 			then
-				say_color pass "# passed all $msg"
+				say_color pass "${statusprefix}# passed all $msg"
 			fi
-			say "1..$test_count$skip_all"
+			say "${statusprefix}1..$test_count$skip_all"
 		fi
 
 		test -d "$remove_trash" &&
@@ -515,8 +518,8 @@ test_done () {
 	*)
 		if test $test_external_has_tap -eq 0
 		then
-			say_color error "# failed $test_failure among $msg"
-			say "1..$test_count"
+			say_color error "${statusprefix}# failed $test_failure among $msg"
+			say "${statusprefix}1..$test_count"
 		fi
 
 		exit 1 ;;
-- 
1.8.3.rc2.393.g8636c0b

[RFC PATCH 6/6] test-lib: support running tests under valgrind in parallel

[Thomas Rast]

With the new --valgrind-parallel=<n> option, we support running the
tests in a single test script under valgrind in parallel using 'n'
processes.

This really follows the dumbest approach possible, as follows:

* We spawn the test script 'n' times, using a throw-away
  TEST_OUTPUT_DIRECTORY.  Each of the instances is given options that
  ensures that it only runs every n-th test under valgrind, but
  together they cover the entire range.

* We add up the numbers from the individual tests, and provide the
  usual output.

This is really a gross hack at this point, and should be improved.  In
particular we should keep the actual outputs somewhere more easily
discoverable, and summarize them to the user.

Nevertheless, this is already workable and gives a speedup of more
than 2 on a dual-core (hyperthreaded) machine, using n=4.  This is
expected since the overhead of valgrind is so big (on the order of 20x
under good conditions, and a large startup overhead at every git
invocation) that redundantly running the non-valgrind tests in between
is not that expensive.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55fa749..b4e81bc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -204,6 +204,15 @@ do
 	--valgrind-only=*)
 		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-parallel=*)
+		valgrind_parallel=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-stride=*)
+		valgrind_only_stride=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-offset=*)
+		valgrind_only_offset=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -217,7 +226,7 @@ do
 	esac
 done
 
-if test -n "$valgrind_only"
+if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
 then
 	test -z "$valgrind" && valgrind=memcheck
 	test -z "$verbose" && verbose_only="$valgrind_only"
@@ -359,8 +368,10 @@ match_pattern_list () {
 }
 
 toggle_verbose () {
-	test -z "$verbose_only" && return
-	if match_pattern_list $test_count $verbose_only
+	test -z "$verbose_only" && test -z "$valgrind_only_stride" && return
+	if match_pattern_list $test_count $verbose_only ||
+		{ test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null; }
 	then
 		exec 4>&2 3>&1
 	else
@@ -370,7 +381,7 @@ toggle_verbose () {
 
 toggle_valgrind () {
 	test -z "$GIT_VALGRIND" && return
-	if test -z "$valgrind_only"
+	if test -z "$valgrind_only" && test -z "$valgrind_only_stride"
 	then
 		GIT_VALGRIND_ENABLED=t
 		return
@@ -379,6 +390,10 @@ toggle_valgrind () {
 	if match_pattern_list $test_count $valgrind_only
 	then
 		GIT_VALGRIND_ENABLED=t
+	elif test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null
+	then
+		GIT_VALGRIND_ENABLED=t
 	fi
 }
 
@@ -600,7 +615,10 @@ then
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
 	GIT_VALGRIND_ENABLED=t
-	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
+	then
+		GIT_VALGRIND_ENABLED=
+	fi
 	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
@@ -686,6 +704,38 @@ then
 else
 	mkdir -p "$TRASH_DIRECTORY"
 fi
+
+if test -n "$valgrind_parallel"
+then
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		mkdir "$root"
+		TEST_OUTPUT_DIRECTORY="$root" \
+			${SHELL_PATH} "$0" \
+			--root="$root" --statusprefix="[$i] " \
+			--valgrind="$valgrind" \
+			--valgrind-only-stride="$valgrind_parallel" \
+			--valgrind-only-offset="$i" &
+		pids="$pids $!"
+	done
+	trap "kill $pids" INT TERM HUP
+	wait $pids
+	trap - INT TERM HUP
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		eval "$(cat "$root/test-results/$(basename "$0" .sh)"-*.counts |
+			sed 's/^\([a-z][a-z]*\) \([0-9][0-9]*\)/inner_\1=\2/')"
+		test_count=$(expr $test_count + $inner_total)
+		test_success=$(expr $test_success + $inner_success)
+		test_fixed=$(expr $test_fixed + $inner_fixed)
+		test_broken=$(expr $test_broken + $inner_broken)
+		test_failed=$(expr $test_failed + $inner_failed)
+	done
+	test_done
+fi
+
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || exit 1
-- 
1.8.3.rc2.393.g8636c0b

Re: [PATCH 1/6] test-lib: enable MALLOC_* for the actual tests

[Elia Pinto]

2013/5/16 Thomas Rast <trast@inf.ethz.ch>:
> 1b3185f (MALLOC_CHECK: various clean-ups, 2012-09-14) moved around the
> MALLOC_CHECK_ and MALLOC_PERTURB_ assignments, intending to limit
> their effect to only the test runs.  However, they were actually
> enabled only during test cleanup.  Call setup/teardown_malloc_check
> also around the evaluation of the actual test snippet.
>
> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
> ---
>  t/test-lib.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ca6bdef..229f5f7 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -337,8 +337,10 @@ test_eval_ () {
>  test_run_ () {
>         test_cleanup=:
>         expecting_failure=$2
> +       setup_malloc_check
>         test_eval_ "$1"
>         eval_ret=$?
> +       teardown_malloc_check
>
>         if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>         then
> --
> 1.8.3.rc2.393.g8636c0b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Good.

Re: [PATCH 1/6] test-lib: enable MALLOC_* for the actual tests

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> 1b3185f (MALLOC_CHECK: various clean-ups, 2012-09-14) moved around the
> MALLOC_CHECK_ and MALLOC_PERTURB_ assignments, intending to limit
> their effect to only the test runs.  However, they were actually
> enabled only during test cleanup.  Call setup/teardown_malloc_check
> also around the evaluation of the actual test snippet.

Sorry about the breakage.

> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
> ---
>  t/test-lib.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ca6bdef..229f5f7 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -337,8 +337,10 @@ test_eval_ () {
>  test_run_ () {
>  	test_cleanup=:
>  	expecting_failure=$2
> +	setup_malloc_check
>  	test_eval_ "$1"
>  	eval_ret=$?
> +	teardown_malloc_check
>  
>  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>  	then

Re: [PATCH 5/6] test-lib: allow prefixing a custom string before "ok N" etc.

[Phil Hord]

On Thu, May 16, 2013 at 4:50 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> This is not really meant for external use, but allows the next commit
> to neatly distinguish between sub-tests and the main run.

Maybe we do not care about standards for this library or for your
use-case, but placing this prefix before the "{ok,not ok}" breaks the
TAProtocol.
http://podwiki.hexten.net/TAP/TAP.html?page=TAP

Maybe you can put the prefix _after_ the "{ok, not ok}" and test number.

Phil

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Johannes Sixt]

Am 5/16/2013 22:50, schrieb Thomas Rast:
> +match_pattern_list () {
> +	arg="$1"
> +	shift
> +	test -z "$*" && return 1
> +	for pat in $@

You should have double-quotes around $@ here, but then you can just as
well abbreviate to

	for pat

and you don't need the 'test -z "$*' check anymore.

-- Hannes

Re: [PATCH 5/6] test-lib: allow prefixing a custom string before "ok N" etc.

[Thomas Rast]

Phil Hord <phil.hord@gmail.com> writes:

> On Thu, May 16, 2013 at 4:50 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> This is not really meant for external use, but allows the next commit
>> to neatly distinguish between sub-tests and the main run.
>
> Maybe we do not care about standards for this library or for your
> use-case, but placing this prefix before the "{ok,not ok}" breaks the
> TAProtocol.
> http://podwiki.hexten.net/TAP/TAP.html?page=TAP
>
> Maybe you can put the prefix _after_ the "{ok, not ok}" and test number.

Actually that was half on purpose.  You will notice I did not document
that option, as it is intended only to be used to distinguish between
the parallel runs implemented in [6/6].

Those parallel runs look something like

[4] ok 1 - plain
[4] ok 2 - plain nested in bare
[...snip until othes catch up...]
[4] ok 33 - re-init to update git link
[4] ok 34 - re-init to move gitdir
[3] ok 1 - plain
[2] ok 1 - plain
[4] ok 35 - re-init to move gitdir symlink
[4] # still have 2 known breakage(s)
[4] # passed all remaining 33 test(s)
[4] 1..35
[3] ok 2 - plain nested in bare

It's invalid TAP no matter what: there are N plans and the ok/not ok
lines from N runs all intermingled.  So I'd rather not even pretend that
it is valid in any way.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 5/16/2013 22:50, schrieb Thomas Rast:
>> +match_pattern_list () {
>> +	arg="$1"
>> +	shift
>> +	test -z "$*" && return 1
>> +	for pat in $@
>
> You should have double-quotes around $@ here, but then you can just as
> well abbreviate to
>
> 	for pat
>
> and you don't need the 'test -z "$*' check anymore.

Hmm, actually the quotes wouldn't help, because it currently reads

-	for skp in $GIT_SKIP_TESTS
-	do
-		case $this_test.$test_count in
-		$skp)
-			to_skip=t
-			break
-		esac
-	done

so the splitting already happens, and in fact needs to, so that one can
pass multiple patterns.  Or am I missing something?

But the 'for pat' with implicit $@ sounds nice regardless, thanks.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 5/6] test-lib: allow prefixing a custom string before "ok N" etc.

[Phil Hord]

On Fri, May 17, 2013 at 4:00 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Thu, May 16, 2013 at 4:50 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> This is not really meant for external use, but allows the next commit
>>> to neatly distinguish between sub-tests and the main run.
>>
>> Maybe we do not care about standards for this library or for your
>> use-case, but placing this prefix before the "{ok,not ok}" breaks the
>> TAProtocol.
>> http://podwiki.hexten.net/TAP/TAP.html?page=TAP
>>
>> Maybe you can put the prefix _after_ the "{ok, not ok}" and test number.
>
> Actually that was half on purpose.  You will notice I did not document
> that option, as it is intended only to be used to distinguish between
> the parallel runs implemented in [6/6].
>
> Those parallel runs look something like
>
> [4] ok 1 - plain
> [4] ok 2 - plain nested in bare
> [...snip until othes catch up...]
> [4] ok 33 - re-init to update git link
> [4] ok 34 - re-init to move gitdir
> [3] ok 1 - plain
> [2] ok 1 - plain
> [4] ok 35 - re-init to move gitdir symlink
> [4] # still have 2 known breakage(s)
> [4] # passed all remaining 33 test(s)
> [4] 1..35
> [3] ok 2 - plain nested in bare
>
> It's invalid TAP no matter what: there are N plans and the ok/not ok
> lines from N runs all intermingled.  So I'd rather not even pretend that
> it is valid in any way.

Yes, I guessed this might have been the goal.  Maybe you can mention
it in the commit message.

I hope some future change might even unwind these back into a valid
continuous TAP stream.  But at least for now, if someone needs such a
stream, she can unwind it herself.

Phil

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Am 5/16/2013 22:50, schrieb Thomas Rast:
>>> +match_pattern_list () {
>>> +	arg="$1"
>>> +	shift
>>> +	test -z "$*" && return 1
>>> +	for pat in $@
>>
>> You should have double-quotes around $@ here, but then you can just as
>> well abbreviate to
>>
>> 	for pat
>>
>> and you don't need the 'test -z "$*' check anymore.
>
> Hmm, actually the quotes wouldn't help, because it currently reads
>
> -	for skp in $GIT_SKIP_TESTS
> -	do
> -		case $this_test.$test_count in
> -		$skp)
> -			to_skip=t
> -			break
> -		esac
> -	done
>
> so the splitting already happens, and in fact needs to, so that one can
> pass multiple patterns.  Or am I missing something?

I think you read it right.  I do pass multiple test number patterns
regularly and not quoting there is exactly for support that.

> But the 'for pat' with implicit $@ sounds nice regardless, thanks.

'for pat' is equivalent to 'for pat in "$@"', not 'for pat in $@';
would it still be useful when you need them split at $IFS?

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>> Am 5/16/2013 22:50, schrieb Thomas Rast:
>>>> +match_pattern_list () {
>>>> +	arg="$1"
>>>> +	shift
>>>> +	test -z "$*" && return 1
>>>> +	for pat in $@
>>>
>>> You should have double-quotes around $@ here, but then you can just as
>>> well abbreviate to
>>>
>>> 	for pat
>>>
>>> and you don't need the 'test -z "$*' check anymore.
>>
>> Hmm, actually the quotes wouldn't help, because it currently reads
>>
>> -	for skp in $GIT_SKIP_TESTS
>> -	do
>> -		case $this_test.$test_count in
>> -		$skp)
>> -			to_skip=t
>> -			break
>> -		esac
>> -	done
[...]
>> But the 'for pat' with implicit $@ sounds nice regardless, thanks.
>
> 'for pat' is equivalent to 'for pat in "$@"', not 'for pat in $@';
> would it still be useful when you need them split at $IFS?

At this point the splitting has already happened in the caller when it
does the (refactored)

+	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS

So $@ and "$@" is actually the same thing.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> At this point the splitting has already happened in the caller when it
> does the (refactored)
>
> +	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
>
> So $@ and "$@" is actually the same thing.

Ah, then it is perfect.  Thanks.

Re: [PATCH 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Johannes Sixt]

Am 17.05.2013 19:02, schrieb Thomas Rast:
> At this point the splitting has already happened in the caller when it
> does the (refactored)
> 
> +	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
> 
> So $@ and "$@" is actually the same thing.

Not in general: If you omit the double-quotes, the individual words
still undergo path name expansion and the result depends on the files
that happen to match patterns given in $GIT_SKIP_TESTS.

This is not a new problem at the call site of the new function, but we
shouldn't duplicate the same problem in the implementation of the function.

-- Hannes

Re: [PATCH 0/6] --valgrind improvements

[Jeff King]

On Thu, May 16, 2013 at 10:50:11PM +0200, Thomas Rast wrote:

> One open issue with the last patch that currently eludes me: if I
> combine --valgrind-parallel with any --valgrind=*, there are lots of
> errors as (apparently) the valgrind wrapper setups race against each
> other.  However, without any --valgrind=* (thus defaulting to
> 'memcheck') this doesn't happen.

I noticed two racy error messages. If you do:

  cd t &&
  make clean &&
  ./some-test --valgrind-parallel=8

you will get complaints from mkdir about existing directories, as we use
mkdir as a poor man's O_EXCL to create lockfiles. These error messages
are harmless (we loop and try again), and we should perhaps just squelch
the stderr from mkdir. Although that might make weird situations hard to
diagnose, like another error that prevents creating the lockfile, so
maybe it is better to just live with the extra output (after the
directory is built once, it does not happen at all).

I also notice:

  $ ./t4052-* --valgrind-parallel=8 --valgrind=memcheck
  ...
  ./t4052-stat-output.sh: 572: ./test-lib.sh: cannot open
  /home/peff/compile/git/t/valgrind/bin/git-send-email.perl: No such
  file

Line 572 is checking the "#!" line of the _source_ file. So it shouldn't
be checking t/valgrind/bin in the first place. It looks like it comes
from this loop:

        for path in $PATH
        do
                ls "$path"/git-* 2> /dev/null |
                while read file
                do
                        make_valgrind_symlink "$file"
                done
        done

as t/valgrind/bin seems to be in the $PATH of the parallel
sub-processes. Hmm. It looks like you set up the valgrind dir in the
parent test process, and _then_ call the sub-processes in parallel. We
shouldn't need to do any valgrind setup at all in the subprocesses,
should we? They would just be replicating what the parent already did.

-Peff

Re: [PATCH 3/6] test-lib: verbose mode for only tests matching a pattern

[Jeff King]

On Thu, May 16, 2013 at 10:50:14PM +0200, Thomas Rast wrote:

> With the new --verbose-only=<pattern> option, one can enable --verbose
> at a per-test granularity.  The pattern is matched against the test
> number, e.g.
> 
>   ./t0000-basic.sh --verbose-only='2[0-2]'
> 
> to see only the full output of test 20-22, while showing the rest in the
> one-liner format.
> 
> This is arguably not *too* useful on its own, but makes the next patch
> easier to follow.

Hmm, I don't think this is quite right. Try:

  ./t4052-stat-output.sh --verbose-only=85

The script and test number aren't important; I just picked these at
random, but they show the issue clearly.  The output I get is:

  [...]
  ok 83 - log respects prefix greater than COLUMNS (big change)
  ok 84 - log --graph respects prefix greater than COLUMNS (big change)
  Switched to a new branch 'branch'
  ok 85 - merge --stat respects COLUMNS (big change)

  expecting success: 
          COLUMNS=100 git merge --stat --no-ff master >output &&
          grep " | " output >actual
          test_cmp expect actual

  ok 86 - merge --stat respects COLUMNS (long filename)

So we see 83 and 84 non-verbose, which is good. And we see the actual
output from 85 (the output from a "git checkout"). But we do not see the
"expecting success" for it. We see it for the _next_ test, which we
should not see at all. So I think your toggling is happening in the
wrong spot, but I haven't looked further than that.

-Peff

Re: [PATCH 3/6] test-lib: verbose mode for only tests matching a pattern

[Jeff King]

On Wed, May 29, 2013 at 01:00:00AM -0400, Jeff King wrote:

> So we see 83 and 84 non-verbose, which is good. And we see the actual
> output from 85 (the output from a "git checkout"). But we do not see the
> "expecting success" for it. We see it for the _next_ test, which we
> should not see at all. So I think your toggling is happening in the
> wrong spot, but I haven't looked further than that.

I think you want something more like:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5251009..75351f5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -349,6 +349,7 @@ test_expect_failure () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
+		toggle_verbose
 		say >&3 "checking known breakage: $2"
 		if test_run_ "$2" expecting_failure
 		then
@@ -367,6 +368,7 @@ test_expect_success () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
+		toggle_verbose
 		say >&3 "expecting success: $2"
 		if test_run_ "$2"
 		then
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b4e81bc..165e84e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -400,7 +400,6 @@ setup_test_eval () {
 setup_test_eval () {
 	setup_malloc_check
 	toggle_valgrind
-	toggle_verbose
 }
 teardown_test_eval () {
 	teardown_malloc_check

However, I'm not sure the toggle is the right thing. However, the whole
toggle thing seems weird to me, as there is a big "gap" between
finishing test X and starting test X+1 where we inherit the verbosity
(and valgrind) settings from X. In general we frown upon doing much at
all outside of test_expect_*, but I would think that:

  test_expect_success 'one' '...'
  git foo
  test_expect_success 'two' '...'

when run with "--valgrind-only=1" would not run the intermediate "git
foo" with valgrind. I would have expected the implementation to be more
like:

  maybe_turn_on_valgrind
  maybe_turn_on_verbose
  run_the_actual_test
  maybe_turn_off_verbose
  maybe_turn_off_valgrind

rather than relying on the next test to return to normal. I doubt that
it matters too much in practice, though.

-Peff

Re: [PATCH 3/6] test-lib: verbose mode for only tests matching a pattern

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 29, 2013 at 01:00:00AM -0400, Jeff King wrote:
>
>> So we see 83 and 84 non-verbose, which is good. And we see the actual
>> output from 85 (the output from a "git checkout"). But we do not see the
>> "expecting success" for it. We see it for the _next_ test, which we
>> should not see at all. So I think your toggling is happening in the
>> wrong spot, but I haven't looked further than that.
>
> I think you want something more like:
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 5251009..75351f5 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -349,6 +349,7 @@ test_expect_failure () {
>  	export test_prereq
>  	if ! test_skip "$@"
>  	then
> +		toggle_verbose
>  		say >&3 "checking known breakage: $2"
>  		if test_run_ "$2" expecting_failure
>  		then
> @@ -367,6 +368,7 @@ test_expect_success () {
>  	export test_prereq
>  	if ! test_skip "$@"
>  	then
> +		toggle_verbose
>  		say >&3 "expecting success: $2"
>  		if test_run_ "$2"
>  		then
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index b4e81bc..165e84e 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -400,7 +400,6 @@ setup_test_eval () {
>  setup_test_eval () {
>  	setup_malloc_check
>  	toggle_valgrind
> -	toggle_verbose
>  }
>  teardown_test_eval () {
>  	teardown_malloc_check
>
> However, I'm not sure the toggle is the right thing. However, the whole
> toggle thing seems weird to me, as there is a big "gap" between
> finishing test X and starting test X+1 where we inherit the verbosity
> (and valgrind) settings from X. In general we frown upon doing much at
> all outside of test_expect_*, but I would think that:
>
>   test_expect_success 'one' '...'
>   git foo
>   test_expect_success 'two' '...'
>
> when run with "--valgrind-only=1" would not run the intermediate "git
> foo" with valgrind. I would have expected the implementation to be more
> like:
>
>   maybe_turn_on_valgrind
>   maybe_turn_on_verbose
>   run_the_actual_test
>   maybe_turn_off_verbose
>   maybe_turn_off_valgrind
>
> rather than relying on the next test to return to normal.

That matches my expectation as well (I had the same thought while
reading the series).

Thanks.

[PATCH v2 0/6] --valgrind improvements

[Thomas Rast]

Here's the promised reroll.  It integrates everyone's suggestions (I
hope I didn't miss any), most notably the two by Peff:

* --valgrind-parallel only does the valgrind wrapper setup in the
  children, avoiding lock contention and confusion because it tries to
  wrap the wrappers.

* more careful --verbose-only toggling (with some extra care about the
  blank lines in between)



Thomas Rast (6):
  test-lib: enable MALLOC_* for the actual tests
  test-lib: refactor $GIT_SKIP_TESTS matching
  test-lib: verbose mode for only tests matching a pattern
  test-lib: valgrind for only tests matching a pattern
  test-lib: allow prefixing a custom string before "ok N" etc.
  test-lib: support running tests under valgrind in parallel

 t/README                |  10 ++
 t/test-lib-functions.sh |   2 +
 t/test-lib.sh           | 248 ++++++++++++++++++++++++++++++++++++++----------
 t/valgrind/valgrind.sh  |   3 +
 4 files changed, 213 insertions(+), 50 deletions(-)

-- 
1.8.3.1.530.g6f90e57

[PATCH v2 1/6] test-lib: enable MALLOC_* for the actual tests

[Thomas Rast]

1b3185f (MALLOC_CHECK: various clean-ups, 2012-09-14) moved around the
MALLOC_CHECK_ and MALLOC_PERTURB_ assignments, intending to limit
their effect to only the test runs.  However, they were actually
enabled only during test cleanup.  Call setup/teardown_malloc_check
also around the evaluation of the actual test snippet.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index eff3a65..35da859 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -337,8 +337,10 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+	setup_malloc_check
 	test_eval_ "$1"
 	eval_ret=$?
+	teardown_malloc_check
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-- 
1.8.3.1.530.g6f90e57

[PATCH v2 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

It's already used twice, and we will have more of the same kind of
matching in a minute.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 35da859..d9a74ff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -328,6 +328,20 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+match_pattern_list () {
+	arg="$1"
+	shift
+	test -z "$*" && return 1
+	for pat
+	do
+		case "$arg" in
+		$pat)
+			return 0
+		esac
+	done
+	return 1
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -358,14 +372,10 @@ test_run_ () {
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
-	for skp in $GIT_SKIP_TESTS
-	do
-		case $this_test.$test_count in
-		$skp)
-			to_skip=t
-			break
-		esac
-	done
+	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	then
+		to_skip=t
+	fi
 	if test -z "$to_skip" && test -n "$test_prereq" &&
 	   ! test_have_prereq "$test_prereq"
 	then
@@ -630,15 +640,12 @@ cd -P "$TRASH_DIRECTORY" || exit 1
 
 this_test=${0##*/}
 this_test=${this_test%%-*}
-for skp in $GIT_SKIP_TESTS
-do
-	case "$this_test" in
-	$skp)
-		say_color info >&3 "skipping test $this_test altogether"
-		skip_all="skip all tests in $this_test"
-		test_done
-	esac
-done
+if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+then
+	say_color info >&3 "skipping test $this_test altogether"
+	skip_all="skip all tests in $this_test"
+	test_done
+fi
 
 # Provide an implementation of the 'yes' utility
 yes () {
-- 
1.8.3.1.530.g6f90e57

[PATCH v2 3/6] test-lib: verbose mode for only tests matching a pattern

[Thomas Rast]

With the new --verbose-only=<pattern> option, one can enable --verbose
at a per-test granularity.  The pattern is matched against the test
number, e.g.

  ./t0000-basic.sh --verbose-only='2[0-2]'

to see only the full output of test 20-22, while showing the rest in the
one-liner format.

As suggested by Jeff King, this takes care to wrap the entire test_expect_*
block, but nothing else, in the verbose toggling.  To that end we use
a new pair of hook functions.  The placement is a bit weird because we
need to wait until the beginning of test_skip for $test_count to be
incremented.

This is arguably not *too* useful on its own, but makes the next patch
easier to follow.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README                |  5 +++++
 t/test-lib-functions.sh |  2 ++
 t/test-lib.sh           | 44 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index 35b3c5c..f4e6299 100644
--- a/t/README
+++ b/t/README
@@ -76,6 +76,11 @@ appropriately before running "make".
 	command being run and their output if any are also
 	output.
 
+--verbose-only=<pattern>::
+	Like --verbose, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --debug::
 	This may help the person who is developing a new test.
 	It causes the command defined with test_debug to run.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5251009..0eac1dd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -358,6 +358,7 @@ test_expect_failure () {
 		fi
 	fi
 	echo >&3 ""
+	test_teardown_hook_
 }
 
 test_expect_success () {
@@ -376,6 +377,7 @@ test_expect_success () {
 		fi
 	fi
 	echo >&3 ""
+	test_teardown_hook_
 }
 
 # test_external runs external test scripts that provide continuous
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d9a74ff..84e5f03 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,6 +184,9 @@ do
 		help=t; shift ;;
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
 		verbose=t; shift ;;
+	--verbose-only=*)
+		verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		# Ignore --quiet under a TAP::Harness. Saying how many tests
 		# passed without the ok/not ok details is always an error.
@@ -342,6 +345,44 @@ match_pattern_list () {
 	return 1
 }
 
+maybe_teardown_verbose () {
+	test -z "$verbose_only" && return
+	exec 4>/dev/null 3>/dev/null
+	verbose=
+}
+
+last_verbose=t
+maybe_setup_verbose () {
+	test -z "$verbose_only" && return
+	if match_pattern_list $test_count $verbose_only
+	then
+		exec 4>&2 3>&1
+		# Emit a delimiting blank line when going from
+		# non-verbose to verbose.  Within verbose mode the
+		# delimiter is printed by test_expect_*.  The choice
+		# of the initial $last_verbose is such that before
+		# test 1, we do not print it.
+		test -z "$last_verbose" && echo >&3 ""
+		verbose=t
+	else
+		exec 4>/dev/null 3>/dev/null
+		verbose=
+	fi
+	last_verbose=$verbose
+}
+
+# Called from test_skip after it has incremented $test_count.  This
+# means it runs before any test-specific code and output.
+test_setup_hook_ () {
+	maybe_setup_verbose
+}
+
+# Called at the end of test_expect_*.  This means it runs after all
+# test-specific code and output.
+test_teardown_hook_ () {
+	maybe_teardown_verbose
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -358,9 +399,7 @@ test_run_ () {
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-		setup_malloc_check
 		test_eval_ "$test_cleanup"
-		teardown_malloc_check
 	fi
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
 	then
@@ -372,6 +411,7 @@ test_run_ () {
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
+	test_setup_hook_
 	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
 	then
 		to_skip=t
-- 
1.8.3.1.530.g6f90e57

[PATCH v2 4/6] test-lib: valgrind for only tests matching a pattern

[Thomas Rast]

With the new --valgrind-only=<pattern> option, one can enable
--valgrind at a per-test granularity, exactly analogous to
--verbose-only from the previous commit.

The options are wired such that --valgrind implies --verbose (as
before), but --valgrind-only=<pattern> implies
--verbose-only=<pattern> unless --verbose is also in effect.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README               |  5 +++++
 t/test-lib.sh          | 36 +++++++++++++++++++++++++++++++++++-
 t/valgrind/valgrind.sh |  3 +++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index f4e6299..abe991f 100644
--- a/t/README
+++ b/t/README
@@ -126,6 +126,11 @@ appropriately before running "make".
 	the 't/valgrind/' directory and use the commands under
 	't/valgrind/bin/'.
 
+--valgrind-only=<pattern>::
+	Like --valgrind, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --tee::
 	In addition to printing the test output to the terminal,
 	write it to files named 't/test-results/$TEST_NAME.out'.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 84e5f03..40bd7da 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -201,6 +201,9 @@ do
 	--valgrind=*)
 		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-only=*)
+		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -211,7 +214,14 @@ do
 	esac
 done
 
-test -n "$valgrind" && verbose=t
+if test -n "$valgrind_only"
+then
+	test -z "$valgrind" && valgrind=memcheck
+	test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+	verbose=t
+fi
 
 if test -n "$color"
 then
@@ -371,15 +381,36 @@ maybe_setup_verbose () {
 	last_verbose=$verbose
 }
 
+maybe_teardown_valgrind () {
+	test -z "$GIT_VALGRIND" && return
+	GIT_VALGRIND_ENABLED=
+}
+
+maybe_setup_valgrind () {
+	test -z "$GIT_VALGRIND" && return
+	if test -z "$valgrind_only"
+	then
+		GIT_VALGRIND_ENABLED=t
+		return
+	fi
+	GIT_VALGRIND_ENABLED=
+	if match_pattern_list $test_count $valgrind_only
+	then
+		GIT_VALGRIND_ENABLED=t
+	fi
+}
+
 # Called from test_skip after it has incremented $test_count.  This
 # means it runs before any test-specific code and output.
 test_setup_hook_ () {
 	maybe_setup_verbose
+	maybe_setup_valgrind
 }
 
 # Called at the end of test_expect_*.  This means it runs after all
 # test-specific code and output.
 test_teardown_hook_ () {
+	maybe_teardown_valgrind
 	maybe_teardown_verbose
 }
 
@@ -590,6 +621,9 @@ then
 	export GIT_VALGRIND
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
+	GIT_VALGRIND_ENABLED=t
+	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 6b87c91..4215303 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -4,6 +4,9 @@ base=$(basename "$0")
 
 TOOL_OPTIONS='--leak-check=no'
 
+test -z "$GIT_VALGRIND_ENABLED" &&
+exec "$GIT_VALGRIND"/../../"$base" "$@"
+
 case "$GIT_VALGRIND_MODE" in
 memcheck-fast)
 	;;
-- 
1.8.3.1.530.g6f90e57

[PATCH v2 5/6] test-lib: allow prefixing a custom string before "ok N" etc.

[Thomas Rast]

This is not really meant for external use, and thus not documented. It
allows the next commit to neatly distinguish between sub-tests and the
main run.

The format is intentionally not valid TAP.  The use in the next commit
would not result in anything valid either way, and it seems better to
make it obvious.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 40bd7da..aaf6084 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -209,6 +209,9 @@ do
 	--root=*)
 		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--statusprefix=*)
+		statusprefix=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	*)
 		echo "error: unknown test option '$1'" >&2; exit 1 ;;
 	esac
@@ -316,12 +319,12 @@ trap 'die' EXIT
 
 test_ok_ () {
 	test_success=$(($test_success + 1))
-	say_color "" "ok $test_count - $@"
+	say_color "" "${statusprefix}ok $test_count - $@"
 }
 
 test_failure_ () {
 	test_failure=$(($test_failure + 1))
-	say_color error "not ok $test_count - $1"
+	say_color error "${statusprefix}not ok $test_count - $1"
 	shift
 	echo "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
@@ -329,12 +332,12 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color error "ok $test_count - $@ # TODO known breakage vanished"
+	say_color error "${statusprefix}ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
-	say_color warn "not ok $test_count - $@ # TODO known breakage"
+	say_color warn "${statusprefix}not ok $test_count - $@ # TODO known breakage"
 }
 
 test_debug () {
@@ -460,8 +463,8 @@ test_skip () {
 			of_prereq=" of $test_prereq"
 		fi
 
-		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
+		say_color skip >&3 "${statusprefix}skipping test: $@"
+		say_color skip "${statusprefix}ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
 		: true
 		;;
 	*)
@@ -497,11 +500,11 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
+		say_color error "${statusprefix}# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
-		say_color warn "# still have $test_broken known breakage(s)"
+		say_color warn "${statusprefix}# still have $test_broken known breakage(s)"
 	fi
 	if test "$test_broken" != 0 || test "$test_fixed" != 0
 	then
@@ -524,9 +527,9 @@ test_done () {
 		then
 			if test $test_remaining -gt 0
 			then
-				say_color pass "# passed all $msg"
+				say_color pass "${statusprefix}# passed all $msg"
 			fi
-			say "1..$test_count$skip_all"
+			say "${statusprefix}1..$test_count$skip_all"
 		fi
 
 		test -d "$remove_trash" &&
@@ -540,8 +543,8 @@ test_done () {
 	*)
 		if test $test_external_has_tap -eq 0
 		then
-			say_color error "# failed $test_failure among $msg"
-			say "1..$test_count"
+			say_color error "${statusprefix}# failed $test_failure among $msg"
+			say "${statusprefix}1..$test_count"
 		fi
 
 		exit 1 ;;
-- 
1.8.3.1.530.g6f90e57

[PATCH v2 6/6] test-lib: support running tests under valgrind in parallel

[Thomas Rast]

With the new --valgrind-parallel=<n> option, we support running the
tests in a single test script under valgrind in parallel using 'n'
processes.

This really follows the dumbest approach possible, as follows:

* We spawn the test script 'n' times, using a throw-away
  TEST_OUTPUT_DIRECTORY.  Each of the instances is given options that
  ensures that it only runs every n-th test under valgrind, but
  together they cover the entire range.

* We add up the numbers from the individual tests, and provide the
  usual output.

This is really a gross hack at this point, and should be improved.  In
particular we should keep the actual outputs somewhere more easily
discoverable, and summarize them to the user.

Nevertheless, this is already workable and gives a speedup of more
than 2 on a dual-core (hyperthreaded) machine, using n=4.  This is
expected since the overhead of valgrind is so big (on the order of 20x
under good conditions, and a large startup overhead at every git
invocation) that redundantly running the non-valgrind tests in between
is not that expensive.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 22 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index aaf6084..ca293c6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -204,6 +204,15 @@ do
 	--valgrind-only=*)
 		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-parallel=*)
+		valgrind_parallel=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-stride=*)
+		valgrind_only_stride=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-offset=*)
+		valgrind_only_offset=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -217,7 +226,7 @@ do
 	esac
 done
 
-if test -n "$valgrind_only"
+if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
 then
 	test -z "$valgrind" && valgrind=memcheck
 	test -z "$verbose" && verbose_only="$valgrind_only"
@@ -367,7 +376,9 @@ maybe_teardown_verbose () {
 last_verbose=t
 maybe_setup_verbose () {
 	test -z "$verbose_only" && return
-	if match_pattern_list $test_count $verbose_only
+	if match_pattern_list $test_count $verbose_only ||
+		{ test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null; }
 	then
 		exec 4>&2 3>&1
 		# Emit a delimiting blank line when going from
@@ -391,7 +402,7 @@ maybe_teardown_valgrind () {
 
 maybe_setup_valgrind () {
 	test -z "$GIT_VALGRIND" && return
-	if test -z "$valgrind_only"
+	if test -z "$valgrind_only" && test -z "$valgrind_only_stride"
 	then
 		GIT_VALGRIND_ENABLED=t
 		return
@@ -400,6 +411,10 @@ maybe_setup_valgrind () {
 	if match_pattern_list $test_count $valgrind_only
 	then
 		GIT_VALGRIND_ENABLED=t
+	elif test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null
+	then
+		GIT_VALGRIND_ENABLED=t
 	fi
 }
 
@@ -552,6 +567,9 @@ test_done () {
 	esac
 }
 
+
+# Set up a directory that we can put in PATH which redirects all git
+# calls to 'valgrind git ...'.
 if test -n "$valgrind"
 then
 	make_symlink () {
@@ -599,33 +617,42 @@ then
 		make_symlink "$symlink_target" "$GIT_VALGRIND/bin/$base" || exit
 	}
 
-	# override all git executables in TEST_DIRECTORY/..
-	GIT_VALGRIND=$TEST_DIRECTORY/valgrind
-	mkdir -p "$GIT_VALGRIND"/bin
-	for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*
-	do
-		make_valgrind_symlink $file
-	done
-	# special-case the mergetools loadables
-	make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
-	OLDIFS=$IFS
-	IFS=:
-	for path in $PATH
-	do
-		ls "$path"/git-* 2> /dev/null |
-		while read file
+	# In the case of --valgrind-parallel, we only need to do the
+	# wrapping once, in the main script.  The worker children all
+	# have $valgrind_only_stride set, so we can skip based on that.
+	if test -z "$valgrind_stride"
+	then
+		# override all git executables in TEST_DIRECTORY/..
+		GIT_VALGRIND=$TEST_DIRECTORY/valgrind
+		mkdir -p "$GIT_VALGRIND"/bin
+		for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*
 		do
-			make_valgrind_symlink "$file"
+			make_valgrind_symlink $file
 		done
-	done
-	IFS=$OLDIFS
+		# special-case the mergetools loadables
+		make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
+		OLDIFS=$IFS
+		IFS=:
+		for path in $PATH
+		do
+			ls "$path"/git-* 2> /dev/null |
+			while read file
+			do
+				make_valgrind_symlink "$file"
+			done
+		done
+		IFS=$OLDIFS
+	fi
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
 	GIT_VALGRIND_ENABLED=t
-	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
+	then
+		GIT_VALGRIND_ENABLED=
+	fi
 	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
@@ -711,6 +738,41 @@ then
 else
 	mkdir -p "$TRASH_DIRECTORY"
 fi
+
+# Gross hack to spawn N sub-instances of the tests in parallel, and
+# summarize the results.  Note that if this is enabled, the script
+# terminates at the end of this 'if' block.
+if test -n "$valgrind_parallel"
+then
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		mkdir "$root"
+		TEST_OUTPUT_DIRECTORY="$root" \
+			${SHELL_PATH} "$0" \
+			--root="$root" --statusprefix="[$i] " \
+			--valgrind="$valgrind" \
+			--valgrind-only-stride="$valgrind_parallel" \
+			--valgrind-only-offset="$i" &
+		pids="$pids $!"
+	done
+	trap "kill $pids" INT TERM HUP
+	wait $pids
+	trap - INT TERM HUP
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		eval "$(cat "$root/test-results/$(basename "$0" .sh)"-*.counts |
+			sed 's/^\([a-z][a-z]*\) \([0-9][0-9]*\)/inner_\1=\2/')"
+		test_count=$(expr $test_count + $inner_total)
+		test_success=$(expr $test_success + $inner_success)
+		test_fixed=$(expr $test_fixed + $inner_fixed)
+		test_broken=$(expr $test_broken + $inner_broken)
+		test_failure=$(expr $test_failure + $inner_failed)
+	done
+	test_done
+fi
+
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || exit 1
-- 
1.8.3.1.530.g6f90e57

Re: [PATCH v2 3/6] test-lib: verbose mode for only tests matching a pattern

[Jeff King]

On Mon, Jun 17, 2013 at 11:18:48AM +0200, Thomas Rast wrote:

> As suggested by Jeff King, this takes care to wrap the entire test_expect_*
> block, but nothing else, in the verbose toggling.  To that end we use
> a new pair of hook functions.  The placement is a bit weird because we
> need to wait until the beginning of test_skip for $test_count to be
> incremented.

I guess it is not surprising because I suggested it, but I find the new
setup/teardown logic easier to follow than the old toggle. The weird
placement did throw me for a minute while reading the patch. I wonder if
it is worth pulling the increment out of test_skip, to have something
like this in test_expect_*:

  test_start ;# increment number, run setup hooks
  if ! test_skip
  then
    ...
  fi
  test_finish ;# teardown hooks

Then it is a bit easier to see that each start has a finish (whereas in
the current version, the setups in test_skip are matched by individual
teardowns in each caller). I did not look too hard at it, though, so I
wouldn't be surprised if there is some other hidden order dependency
that makes that not work. :)

> +# Called from test_skip after it has incremented $test_count.  This
> +# means it runs before any test-specific code and output.
> +test_setup_hook_ () {
> +	maybe_setup_verbose
> +}
> +
> +# Called at the end of test_expect_*.  This means it runs after all
> +# test-specific code and output.
> +test_teardown_hook_ () {
> +	maybe_teardown_verbose
> +}

So these do your verbose setup/teardown. Makes sense.

But then what is this hunk doing:

>  test_eval_ () {
>  	# This is a separate function because some tests use
>  	# "return" to end a test_expect_success block early.
> @@ -358,9 +399,7 @@ test_run_ () {
>  
>  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>  	then
> -		setup_malloc_check
>  		test_eval_ "$test_cleanup"
> -		teardown_malloc_check
>  	fi
>  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
>  	then

At first I thought these should go into your setup/teardown hooks, but I
don't think so. They are made redundant by patch 1/6, which wraps all of
test_eval with malloc setup/teardown, aren't they? Should this hunk be
in patch 1?

-Peff

Re: [PATCH v2 0/6] --valgrind improvements

[Jeff King]

On Mon, Jun 17, 2013 at 11:18:45AM +0200, Thomas Rast wrote:

> Here's the promised reroll.  It integrates everyone's suggestions (I
> hope I didn't miss any), most notably the two by Peff:

Thanks. With the exception of a few comments on patch 3, this looks good
to me.

I agree with your "gross hack" assessment of patch 6/6 (besides the
general implementation, it would fail pretty badly for any test script
which had global state outside of the filesystem, like the lib-httpd
tests). But I also think it cannot not cause too much damage, being both
confined to the test scripts and only having an effect if the option is
enabled. So I'd be fine with it going in just to see if people end up
finding it useful.

-Peff

Re: [PATCH v2 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Johannes Sixt]

Am 6/17/2013 11:18, schrieb Thomas Rast:
> +match_pattern_list () {
> +	arg="$1"
> +	shift
> +	test -z "$*" && return 1
> +	for pat
> +	do
> +		case "$arg" in
> +		$pat)
> +			return 0
> +		esac
> +	done
> +	return 1
> +}

Watch this failing test case:

   GIT_SKIP_TESTS="t950[012]" ./t3006-ls-files-long.sh

'pat' is too short and too sweet to be used as a shell variable name in a
library function. ;)

-- Hannes

Re: [PATCH v2 2/6] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 6/17/2013 11:18, schrieb Thomas Rast:
>> +match_pattern_list () {
>> +	arg="$1"
>> +	shift
>> +	test -z "$*" && return 1
>> +	for pat
>> +	do
>> +		case "$arg" in
>> +		$pat)
>> +			return 0
>> +		esac
>> +	done
>> +	return 1
>> +}
>
> Watch this failing test case:
>
>    GIT_SKIP_TESTS="t950[012]" ./t3006-ls-files-long.sh
>
> 'pat' is too short and too sweet to be used as a shell variable name in a
> library function. ;)

Gah!  Serves me right for not testing test-lib.  Thanks for noticing.

/me goes stand in the corner for a while

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2 3/6] test-lib: verbose mode for only tests matching a pattern

[Thomas Rast]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 17, 2013 at 11:18:48AM +0200, Thomas Rast wrote:
>
>> As suggested by Jeff King, this takes care to wrap the entire test_expect_*
>> block, but nothing else, in the verbose toggling.  To that end we use
>> a new pair of hook functions.  The placement is a bit weird because we
>> need to wait until the beginning of test_skip for $test_count to be
>> incremented.
[...]
>   test_start ;# increment number, run setup hooks
>   if ! test_skip
>   then
>     ...
>   fi
>   test_finish ;# teardown hooks
>
> Then it is a bit easier to see that each start has a finish (whereas in
> the current version, the setups in test_skip are matched by individual
> teardowns in each caller). I did not look too hard at it, though, so I
> wouldn't be surprised if there is some other hidden order dependency
> that makes that not work. :)

No, I think that's actually very reasonable.  I'll do it that way in v3.

> But then what is this hunk doing:
>
>>  test_eval_ () {
>>  	# This is a separate function because some tests use
>>  	# "return" to end a test_expect_success block early.
>> @@ -358,9 +399,7 @@ test_run_ () {
>>  
>>  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>>  	then
>> -		setup_malloc_check
>>  		test_eval_ "$test_cleanup"
>> -		teardown_malloc_check
>>  	fi
>>  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
>>  	then

Thanks for catching this -- it's just a mis-edit that would effectively
revert 1/6.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH v3 0/8] --valgrind improvements

[Thomas Rast]

Changes from v2:

- $pat renamed to $pattern_ to avoid collisions (thanks j6t)

- New patch 3 that tests --verbose operation, and additions to patch
  (now) 4 that test --verbose-only.  (Adding similar tests for
  --valgrind[-only] and associated options would be nice, but is much
  harder because the user may not have valgrind.)

- Rearranged the hooking in (now) 4, 6 and 8 to make it more obvious
  what is going on, as per Peff's comments

- Fixed a misspelled variable that prevented the "valgrind setup only
  once" logic from working

Thomas Rast (8):
  test-lib: enable MALLOC_* for the actual tests
  test-lib: refactor $GIT_SKIP_TESTS matching
  test-lib: rearrange start/end of test_expect_* and test_skip
  test-lib: self-test that --verbose works
  test-lib: verbose mode for only tests matching a pattern
  test-lib: valgrind for only tests matching a pattern
  test-lib: allow prefixing a custom string before "ok N" etc.
  test-lib: support running tests under valgrind in parallel

 t/README                |  10 ++
 t/t0000-basic.sh        |  54 ++++++++++-
 t/test-lib-functions.sh |   6 +-
 t/test-lib.sh           | 244 ++++++++++++++++++++++++++++++++++++++----------
 t/valgrind/valgrind.sh  |   3 +
 5 files changed, 265 insertions(+), 52 deletions(-)

-- 
1.8.3.1.530.g6f90e57

[PATCH v3 1/8] test-lib: enable MALLOC_* for the actual tests

[Thomas Rast]

1b3185f (MALLOC_CHECK: various clean-ups, 2012-09-14) moved around the
MALLOC_CHECK_ and MALLOC_PERTURB_ assignments, intending to limit
their effect to only the test runs.  However, they were actually
enabled only during test cleanup.  Call setup/teardown_malloc_check
also around the evaluation of the actual test snippet.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index eff3a65..35da859 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -337,8 +337,10 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+	setup_malloc_check
 	test_eval_ "$1"
 	eval_ret=$?
+	teardown_malloc_check
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 2/8] test-lib: refactor $GIT_SKIP_TESTS matching

[Thomas Rast]

It's already used twice, and we will have more of the same kind of
matching in a minute.

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 35da859..4fa141a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -328,6 +328,20 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+match_pattern_list () {
+	arg="$1"
+	shift
+	test -z "$*" && return 1
+	for pattern_
+	do
+		case "$arg" in
+		$pattern_)
+			return 0
+		esac
+	done
+	return 1
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -358,14 +372,10 @@ test_run_ () {
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
-	for skp in $GIT_SKIP_TESTS
-	do
-		case $this_test.$test_count in
-		$skp)
-			to_skip=t
-			break
-		esac
-	done
+	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	then
+		to_skip=t
+	fi
 	if test -z "$to_skip" && test -n "$test_prereq" &&
 	   ! test_have_prereq "$test_prereq"
 	then
@@ -630,15 +640,12 @@ cd -P "$TRASH_DIRECTORY" || exit 1
 
 this_test=${0##*/}
 this_test=${this_test%%-*}
-for skp in $GIT_SKIP_TESTS
-do
-	case "$this_test" in
-	$skp)
-		say_color info >&3 "skipping test $this_test altogether"
-		skip_all="skip all tests in $this_test"
-		test_done
-	esac
-done
+if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+then
+	say_color info >&3 "skipping test $this_test altogether"
+	skip_all="skip all tests in $this_test"
+	test_done
+fi
 
 # Provide an implementation of the 'yes' utility
 yes () {
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 3/8] test-lib: rearrange start/end of test_expect_* and test_skip

[Thomas Rast]

This moves

* the early setup part from test_skip to a new function test_start_

* the final common parts of test_expect_* to a new function
  test_finish_

to make the next commit more obvious.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib-functions.sh | 6 ++++--
 t/test-lib.sh           | 9 ++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5251009..ad6d60a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -343,6 +343,7 @@ test_declared_prereq () {
 }
 
 test_expect_failure () {
+	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
@@ -357,10 +358,11 @@ test_expect_failure () {
 			test_known_broken_failure_ "$1"
 		fi
 	fi
-	echo >&3 ""
+	test_finish_
 }
 
 test_expect_success () {
+	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
@@ -375,7 +377,7 @@ test_expect_success () {
 			test_failure_ "$@"
 		fi
 	fi
-	echo >&3 ""
+	test_finish_
 }
 
 # test_external runs external test scripts that provide continuous
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4fa141a..e99b0ea 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -369,8 +369,15 @@ test_run_ () {
 	return "$eval_ret"
 }
 
-test_skip () {
+test_start_ () {
 	test_count=$(($test_count+1))
+}
+
+test_finish_ () {
+	echo >&3 ""
+}
+
+test_skip () {
 	to_skip=
 	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
 	then
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 4/8] test-lib: self-test that --verbose works

[Thomas Rast]

t0000 contains some light self-tests of test-lib.sh, but --verbose was
not covered.  Add a test.

The only catch is that the presence of a test harness influences the
output (specifically, the presence of some empty lines).  The easiest
fix is to unset TEST_HARNESS for the sub-test scripts.  This means
that we no longer check whether test-lib.sh works under the harness;
however, since running everything under 'prove' seems far more common
than the esoteric --verbose-only feature introduced in the next
commit, this seems the smaller risk.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/t0000-basic.sh | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index cefe33d..b568c06 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -47,8 +47,10 @@ test_expect_failure 'pretend we have a known breakage' '
 
 run_sub_test_lib_test () {
 	name="$1" descr="$2" # stdin is the body of the test code
+	shift 2
 	mkdir "$name" &&
 	(
+		unset HARNESS_ACTIVE
 		cd "$name" &&
 		cat >"$name.sh" <<-EOF &&
 		#!$SHELL_PATH
@@ -65,7 +67,7 @@ run_sub_test_lib_test () {
 		cat >>"$name.sh" &&
 		chmod +x "$name.sh" &&
 		export TEST_DIRECTORY &&
-		./"$name.sh" >out 2>err
+		./"$name.sh" "$@" >out 2>err
 	)
 }
 
@@ -215,6 +217,30 @@ test_expect_success 'pretend we have a mix of all possible results' "
 	EOF
 "
 
+test_expect_success 'test --verbose' '
+	test_must_fail run_sub_test_lib_test \
+		test-verbose "test verbose" --verbose <<-\EOF &&
+	test_expect_success "passing test" true
+	test_expect_success "test with output" "echo foo"
+	test_expect_success "failing test" false
+	test_done
+	EOF
+	check_sub_test_lib_test test-verbose <<-\EOF
+	> expecting success: true
+	> ok 1 - passing test
+	>
+	> expecting success: echo foo
+	> foo
+	>
+	> ok 2 - test with output
+	> expecting success: false
+	> not ok 3 - failing test
+	> #	false
+	> # failed 1 among 3 test(s)
+	> 1..3
+	EOF
+'
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 5/8] test-lib: verbose mode for only tests matching a pattern

[Thomas Rast]

With the new --verbose-only=<pattern> option, one can enable --verbose
at a per-test granularity.  The pattern is matched against the test
number, e.g.

  ./t0000-basic.sh --verbose-only='2[0-2]'

to see only the full output of test 20-22, while showing the rest in the
one-liner format.

As suggested by Jeff King, this takes care to wrap the entire
test_expect_* block, but nothing else, in the verbose toggling.  We
can use the test_start/end functions from the previous commit for the
purpose.

This is arguably not *too* useful on its own, but makes the next patch
easier to follow.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README         |  5 +++++
 t/t0000-basic.sh | 30 ++++++++++++++++++++++++++++--
 t/test-lib.sh    | 31 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index 35b3c5c..f4e6299 100644
--- a/t/README
+++ b/t/README
@@ -76,6 +76,11 @@ appropriately before running "make".
 	command being run and their output if any are also
 	output.
 
+--verbose-only=<pattern>::
+	Like --verbose, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --debug::
 	This may help the person who is developing a new test.
 	It causes the command defined with test_debug to run.
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b568c06..0d86039 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -225,17 +225,43 @@ test_expect_success 'test --verbose' '
 	test_expect_success "failing test" false
 	test_done
 	EOF
+	mv test-verbose/out test-verbose/out+
+	grep -v "^Initialized empty" test-verbose/out+ >test-verbose/out &&
 	check_sub_test_lib_test test-verbose <<-\EOF
 	> expecting success: true
 	> ok 1 - passing test
-	>
+	> Z
 	> expecting success: echo foo
 	> foo
-	>
 	> ok 2 - test with output
+	> Z
 	> expecting success: false
 	> not ok 3 - failing test
 	> #	false
+	> Z
+	> # failed 1 among 3 test(s)
+	> 1..3
+	EOF
+'
+
+test_expect_success 'test --verbose-only' '
+	test_must_fail run_sub_test_lib_test \
+		test-verbose-only-2 "test verbose-only=2" \
+		--verbose-only=2 <<-\EOF &&
+	test_expect_success "passing test" true
+	test_expect_success "test with output" "echo foo"
+	test_expect_success "failing test" false
+	test_done
+	EOF
+	check_sub_test_lib_test test-verbose-only-2 <<-\EOF
+	> ok 1 - passing test
+	> Z
+	> expecting success: echo foo
+	> foo
+	> ok 2 - test with output
+	> Z
+	> not ok 3 - failing test
+	> #	false
 	> # failed 1 among 3 test(s)
 	> 1..3
 	EOF
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e99b0ea..2bceb92 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,6 +184,9 @@ do
 		help=t; shift ;;
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
 		verbose=t; shift ;;
+	--verbose-only=*)
+		verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		# Ignore --quiet under a TAP::Harness. Saying how many tests
 		# passed without the ok/not ok details is always an error.
@@ -342,6 +345,32 @@ match_pattern_list () {
 	return 1
 }
 
+maybe_teardown_verbose () {
+	test -z "$verbose_only" && return
+	exec 4>/dev/null 3>/dev/null
+	verbose=
+}
+
+last_verbose=t
+maybe_setup_verbose () {
+	test -z "$verbose_only" && return
+	if match_pattern_list $test_count $verbose_only
+	then
+		exec 4>&2 3>&1
+		# Emit a delimiting blank line when going from
+		# non-verbose to verbose.  Within verbose mode the
+		# delimiter is printed by test_expect_*.  The choice
+		# of the initial $last_verbose is such that before
+		# test 1, we do not print it.
+		test -z "$last_verbose" && echo >&3 ""
+		verbose=t
+	else
+		exec 4>/dev/null 3>/dev/null
+		verbose=
+	fi
+	last_verbose=$verbose
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -371,10 +400,12 @@ test_run_ () {
 
 test_start_ () {
 	test_count=$(($test_count+1))
+	maybe_setup_verbose
 }
 
 test_finish_ () {
 	echo >&3 ""
+	maybe_teardown_verbose
 }
 
 test_skip () {
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 6/8] test-lib: valgrind for only tests matching a pattern

[Thomas Rast]

With the new --valgrind-only=<pattern> option, one can enable
--valgrind at a per-test granularity, exactly analogous to
--verbose-only from the previous commit.

The options are wired such that --valgrind implies --verbose (as
before), but --valgrind-only=<pattern> implies
--verbose-only=<pattern> unless --verbose is also in effect.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README               |  5 +++++
 t/test-lib.sh          | 36 +++++++++++++++++++++++++++++++++++-
 t/valgrind/valgrind.sh |  3 +++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index f4e6299..abe991f 100644
--- a/t/README
+++ b/t/README
@@ -126,6 +126,11 @@ appropriately before running "make".
 	the 't/valgrind/' directory and use the commands under
 	't/valgrind/bin/'.
 
+--valgrind-only=<pattern>::
+	Like --valgrind, but the effect is limited to tests with
+	numbers matching <pattern>.  The number matched against is
+	simply the running count of the test within the file.
+
 --tee::
 	In addition to printing the test output to the terminal,
 	write it to files named 't/test-results/$TEST_NAME.out'.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2bceb92..817ab43 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -201,6 +201,9 @@ do
 	--valgrind=*)
 		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-only=*)
+		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -211,7 +214,14 @@ do
 	esac
 done
 
-test -n "$valgrind" && verbose=t
+if test -n "$valgrind_only"
+then
+	test -z "$valgrind" && valgrind=memcheck
+	test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+	verbose=t
+fi
 
 if test -n "$color"
 then
@@ -371,6 +381,25 @@ maybe_setup_verbose () {
 	last_verbose=$verbose
 }
 
+maybe_teardown_valgrind () {
+	test -z "$GIT_VALGRIND" && return
+	GIT_VALGRIND_ENABLED=
+}
+
+maybe_setup_valgrind () {
+	test -z "$GIT_VALGRIND" && return
+	if test -z "$valgrind_only"
+	then
+		GIT_VALGRIND_ENABLED=t
+		return
+	fi
+	GIT_VALGRIND_ENABLED=
+	if match_pattern_list $test_count $valgrind_only
+	then
+		GIT_VALGRIND_ENABLED=t
+	fi
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -401,10 +430,12 @@ test_run_ () {
 test_start_ () {
 	test_count=$(($test_count+1))
 	maybe_setup_verbose
+	maybe_setup_valgrind
 }
 
 test_finish_ () {
 	echo >&3 ""
+	maybe_teardown_valgrind
 	maybe_teardown_verbose
 }
 
@@ -588,6 +619,9 @@ then
 	export GIT_VALGRIND
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
+	GIT_VALGRIND_ENABLED=t
+	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 6b87c91..4215303 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -4,6 +4,9 @@ base=$(basename "$0")
 
 TOOL_OPTIONS='--leak-check=no'
 
+test -z "$GIT_VALGRIND_ENABLED" &&
+exec "$GIT_VALGRIND"/../../"$base" "$@"
+
 case "$GIT_VALGRIND_MODE" in
 memcheck-fast)
 	;;
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 7/8] test-lib: allow prefixing a custom string before "ok N" etc.

[Thomas Rast]

This is not really meant for external use, and thus not documented. It
allows the next commit to neatly distinguish between sub-tests and the
main run.

The format is intentionally not valid TAP.  The use in the next commit
would not result in anything valid either way, and it seems better to
make it obvious.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 817ab43..f2abff9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -209,6 +209,9 @@ do
 	--root=*)
 		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--statusprefix=*)
+		statusprefix=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	*)
 		echo "error: unknown test option '$1'" >&2; exit 1 ;;
 	esac
@@ -316,12 +319,12 @@ trap 'die' EXIT
 
 test_ok_ () {
 	test_success=$(($test_success + 1))
-	say_color "" "ok $test_count - $@"
+	say_color "" "${statusprefix}ok $test_count - $@"
 }
 
 test_failure_ () {
 	test_failure=$(($test_failure + 1))
-	say_color error "not ok $test_count - $1"
+	say_color error "${statusprefix}not ok $test_count - $1"
 	shift
 	echo "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
@@ -329,12 +332,12 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color error "ok $test_count - $@ # TODO known breakage vanished"
+	say_color error "${statusprefix}ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
-	say_color warn "not ok $test_count - $@ # TODO known breakage"
+	say_color warn "${statusprefix}not ok $test_count - $@ # TODO known breakage"
 }
 
 test_debug () {
@@ -458,8 +461,8 @@ test_skip () {
 			of_prereq=" of $test_prereq"
 		fi
 
-		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
+		say_color skip >&3 "${statusprefix}skipping test: $@"
+		say_color skip "${statusprefix}ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
 		: true
 		;;
 	*)
@@ -495,11 +498,11 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
+		say_color error "${statusprefix}# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
-		say_color warn "# still have $test_broken known breakage(s)"
+		say_color warn "${statusprefix}# still have $test_broken known breakage(s)"
 	fi
 	if test "$test_broken" != 0 || test "$test_fixed" != 0
 	then
@@ -522,9 +525,9 @@ test_done () {
 		then
 			if test $test_remaining -gt 0
 			then
-				say_color pass "# passed all $msg"
+				say_color pass "${statusprefix}# passed all $msg"
 			fi
-			say "1..$test_count$skip_all"
+			say "${statusprefix}1..$test_count$skip_all"
 		fi
 
 		test -d "$remove_trash" &&
@@ -538,8 +541,8 @@ test_done () {
 	*)
 		if test $test_external_has_tap -eq 0
 		then
-			say_color error "# failed $test_failure among $msg"
-			say "1..$test_count"
+			say_color error "${statusprefix}# failed $test_failure among $msg"
+			say "${statusprefix}1..$test_count"
 		fi
 
 		exit 1 ;;
-- 
1.8.3.1.530.g6f90e57

[PATCH v3 8/8] test-lib: support running tests under valgrind in parallel

[Thomas Rast]

With the new --valgrind-parallel=<n> option, we support running the
tests in a single test script under valgrind in parallel using 'n'
processes.

This really follows the dumbest approach possible, as follows:

* We spawn the test script 'n' times, using a throw-away
  TEST_OUTPUT_DIRECTORY.  Each of the instances is given options that
  ensures that it only runs every n-th test under valgrind, but
  together they cover the entire range.

* We add up the numbers from the individual tests, and provide the
  usual output.

This is really a gross hack at this point, and should be improved.  In
particular we should keep the actual outputs somewhere more easily
discoverable, and summarize them to the user.

Nevertheless, this is already workable and gives a speedup of more
than 2 on a dual-core (hyperthreaded) machine, using n=4.  This is
expected since the overhead of valgrind is so big (on the order of 20x
under good conditions, and a large startup overhead at every git
invocation) that redundantly running the non-valgrind tests in between
is not that expensive.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/test-lib.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 22 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2abff9..acc7f2a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -204,6 +204,15 @@ do
 	--valgrind-only=*)
 		valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--valgrind-parallel=*)
+		valgrind_parallel=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-stride=*)
+		valgrind_only_stride=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
+	--valgrind-only-offset=*)
+		valgrind_only_offset=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -217,7 +226,7 @@ do
 	esac
 done
 
-if test -n "$valgrind_only"
+if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
 then
 	test -z "$valgrind" && valgrind=memcheck
 	test -z "$verbose" && verbose_only="$valgrind_only"
@@ -367,7 +376,9 @@ maybe_teardown_verbose () {
 last_verbose=t
 maybe_setup_verbose () {
 	test -z "$verbose_only" && return
-	if match_pattern_list $test_count $verbose_only
+	if match_pattern_list $test_count $verbose_only ||
+		{ test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null; }
 	then
 		exec 4>&2 3>&1
 		# Emit a delimiting blank line when going from
@@ -391,7 +402,7 @@ maybe_teardown_valgrind () {
 
 maybe_setup_valgrind () {
 	test -z "$GIT_VALGRIND" && return
-	if test -z "$valgrind_only"
+	if test -z "$valgrind_only" && test -z "$valgrind_only_stride"
 	then
 		GIT_VALGRIND_ENABLED=t
 		return
@@ -400,6 +411,10 @@ maybe_setup_valgrind () {
 	if match_pattern_list $test_count $valgrind_only
 	then
 		GIT_VALGRIND_ENABLED=t
+	elif test -n "$valgrind_only_stride" &&
+		expr $test_count "%" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null
+	then
+		GIT_VALGRIND_ENABLED=t
 	fi
 }
 
@@ -550,6 +565,9 @@ test_done () {
 	esac
 }
 
+
+# Set up a directory that we can put in PATH which redirects all git
+# calls to 'valgrind git ...'.
 if test -n "$valgrind"
 then
 	make_symlink () {
@@ -597,33 +615,42 @@ then
 		make_symlink "$symlink_target" "$GIT_VALGRIND/bin/$base" || exit
 	}
 
-	# override all git executables in TEST_DIRECTORY/..
-	GIT_VALGRIND=$TEST_DIRECTORY/valgrind
-	mkdir -p "$GIT_VALGRIND"/bin
-	for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*
-	do
-		make_valgrind_symlink $file
-	done
-	# special-case the mergetools loadables
-	make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
-	OLDIFS=$IFS
-	IFS=:
-	for path in $PATH
-	do
-		ls "$path"/git-* 2> /dev/null |
-		while read file
+	# In the case of --valgrind-parallel, we only need to do the
+	# wrapping once, in the main script.  The worker children all
+	# have $valgrind_only_stride set, so we can skip based on that.
+	if test -z "$valgrind_only_stride"
+	then
+		# override all git executables in TEST_DIRECTORY/..
+		GIT_VALGRIND=$TEST_DIRECTORY/valgrind
+		mkdir -p "$GIT_VALGRIND"/bin
+		for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*
 		do
-			make_valgrind_symlink "$file"
+			make_valgrind_symlink $file
 		done
-	done
-	IFS=$OLDIFS
+		# special-case the mergetools loadables
+		make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
+		OLDIFS=$IFS
+		IFS=:
+		for path in $PATH
+		do
+			ls "$path"/git-* 2> /dev/null |
+			while read file
+			do
+				make_valgrind_symlink "$file"
+			done
+		done
+		IFS=$OLDIFS
+	fi
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
 	GIT_VALGRIND_MODE="$valgrind"
 	export GIT_VALGRIND_MODE
 	GIT_VALGRIND_ENABLED=t
-	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
+	if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
+	then
+		GIT_VALGRIND_ENABLED=
+	fi
 	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
@@ -709,6 +736,41 @@ then
 else
 	mkdir -p "$TRASH_DIRECTORY"
 fi
+
+# Gross hack to spawn N sub-instances of the tests in parallel, and
+# summarize the results.  Note that if this is enabled, the script
+# terminates at the end of this 'if' block.
+if test -n "$valgrind_parallel"
+then
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		mkdir "$root"
+		TEST_OUTPUT_DIRECTORY="$root" \
+			${SHELL_PATH} "$0" \
+			--root="$root" --statusprefix="[$i] " \
+			--valgrind="$valgrind" \
+			--valgrind-only-stride="$valgrind_parallel" \
+			--valgrind-only-offset="$i" &
+		pids="$pids $!"
+	done
+	trap "kill $pids" INT TERM HUP
+	wait $pids
+	trap - INT TERM HUP
+	for i in $(test_seq 1 $valgrind_parallel)
+	do
+		root="$TRASH_DIRECTORY/vgparallel-$i"
+		eval "$(cat "$root/test-results/$(basename "$0" .sh)"-*.counts |
+			sed 's/^\([a-z][a-z]*\) \([0-9][0-9]*\)/inner_\1=\2/')"
+		test_count=$(expr $test_count + $inner_total)
+		test_success=$(expr $test_success + $inner_success)
+		test_fixed=$(expr $test_fixed + $inner_fixed)
+		test_broken=$(expr $test_broken + $inner_broken)
+		test_failure=$(expr $test_failure + $inner_failed)
+	done
+	test_done
+fi
+
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || exit 1
-- 
1.8.3.1.530.g6f90e57

Re: [PATCH v3 3/8] test-lib: rearrange start/end of test_expect_* and test_skip

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4fa141a..e99b0ea 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -369,8 +369,15 @@ test_run_ () {
>  	return "$eval_ret"
>  }
>  
> -test_skip () {
> +test_start_ () {
>  	test_count=$(($test_count+1))
> +}
> +
> +test_finish_ () {
> +	echo >&3 ""
> +}
> +
> +test_skip () {
>  	to_skip=
>  	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
>  	then

This puzzled me for a few minutes, until I realized that the code
before this patch was using the call to test_skip, whose primary
purpose is to answer "do we want to run this test, or do we want to
skip it?", as a way to increment the test_count variable.  Arguably
each test would call test_skip once, so it may not be too bad, but
it does look like it is depending on a subtle side-effect.

That increment does logically belong to "now we are starting a new
test" much better.  This change makes perfect sense.

Thanks.