threads / patch / 33843

patch, 6 parts--valgrind improvements

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

## tl;dr

44 messages between May 16, 2013 and Jun 18, 2013. Diffs are folded; open one to read it.

replies: 43people: 6as markdown or json

Thomas Rast· May 16, 2013, 20:50 UTC · lore

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
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(+)
Show changes to t/test-lib.sh +2 −0
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
Elia Pinto· May 16, 2013, 21:28 UTC · re: Thomas Rast · lore

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

2013/5/16 Thomas Rast <trast@inf.ethz.ch>:
Show 33 quoted lines
> 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.
Junio C Hamano· May 16, 2013, 22:43 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 5 quoted lines
> 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.
Show 20 quoted lines
> 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
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +24 −17
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
Johannes Sixt· May 17, 2013, 05:48 UTC · re: Thomas Rast · lore

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

Am 5/16/2013 22:50, schrieb Thomas Rast:
Show 5 quoted lines
> +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
Thomas Rast· May 17, 2013, 08:04 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 13 quoted lines
> 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
Junio C Hamano· May 17, 2013, 16:48 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 29 quoted lines
> 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?

Thomas Rast· May 17, 2013, 17:02 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 28 quoted lines
> 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
Junio C Hamano· May 17, 2013, 17:22 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 6 quoted lines
> 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.
Johannes Sixt· May 17, 2013, 21:29 UTC · re: Thomas Rast · lore

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

Am 17.05.2013 19:02, schrieb Thomas Rast:
Show 6 quoted lines
> 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
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 2 files +30 −4

t/README, t/test-lib.sh

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
Jeff King· May 29, 2013, 05:00 UTC · re: Thomas Rast · lore

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

On Thu, May 16, 2013 at 10:50:14PM +0200, Thomas Rast wrote:
Show 11 quoted lines
> 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
Jeff King· May 29, 2013, 05:07 UTC · re: Jeff King · lore

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

On Wed, May 29, 2013 at 01:00:00AM -0400, Jeff King wrote:
Show 5 quoted lines
> 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:
Show changes to 2 files +2 −2

t/test-lib-functions.sh, t/test-lib.sh

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
Junio C Hamano· May 29, 2013, 17:53 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 64 quoted lines
> 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.
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 3 files +37 −1

t/README, t/test-lib.sh, t/valgrind/valgrind.sh

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
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +15 −12
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
Phil Hord· May 16, 2013, 22:53 UTC · re: Thomas Rast · lore

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

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
Thomas Rast· May 17, 2013, 08:00 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 10 quoted lines
> 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
Phil Hord· May 17, 2013, 13:00 UTC · re: Thomas Rast · lore

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

On Fri, May 17, 2013 at 4:00 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
Show 35 quoted lines
> 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
Thomas Rast· May 16, 2013, 20:50 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +55 −5
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
Jeff King· May 29, 2013, 04:53 UTC · re: Thomas Rast · lore

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

On Thu, May 16, 2013 at 10:50:11PM +0200, Thomas Rast wrote:
Show 5 quoted lines
> 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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

[PATCH v2 0/6] --valgrind improvements

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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(+)
Show changes to t/test-lib.sh +2 −0
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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +24 −17
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
Johannes Sixt· Jun 18, 2013, 07:03 UTC · re: Thomas Rast · lore

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

Am 6/17/2013 11:18, schrieb Thomas Rast:
Show 13 quoted lines
> +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
Thomas Rast· Jun 18, 2013, 08:23 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 21 quoted lines
> 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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 3 files +49 −2

t/README, t/test-lib-functions.sh, t/test-lib.sh

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
Jeff King· Jun 18, 2013, 05:37 UTC · re: Thomas Rast · lore

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

On Mon, Jun 17, 2013 at 11:18:48AM +0200, Thomas Rast wrote:
Show 5 quoted lines
> 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. :)

Show 11 quoted lines
> +# 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:
Show 13 quoted lines
>  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
Thomas Rast· Jun 18, 2013, 08:45 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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.
[...]
Show 12 quoted lines
>   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.
Show 15 quoted lines
> 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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 3 files +43 −1

t/README, t/test-lib.sh, t/valgrind/valgrind.sh

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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +15 −12
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
Thomas Rast· Jun 17, 2013, 09:18 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +84 −22
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
Jeff King· Jun 18, 2013, 05:46 UTC · re: Thomas Rast · lore

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

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
Thomas Rast· Jun 18, 2013, 12:25 UTC · re: Thomas Rast · lore

[PATCH v3 0/8] --valgrind improvements

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
Thomas Rast· Jun 18, 2013, 12:25 UTC · re: Thomas Rast · lore

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

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(+)
Show changes to t/test-lib.sh +2 −0
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
Thomas Rast· Jun 18, 2013, 12:25 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +24 −17
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
Thomas Rast· Jun 18, 2013, 12:25 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 2 files +12 −3

t/test-lib-functions.sh, t/test-lib.sh

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
Junio C Hamano· Jun 18, 2013, 18:21 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 21 quoted lines
> 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.
Thomas Rast· Jun 18, 2013, 12:26 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/t0000-basic.sh +27 −1
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
Thomas Rast· Jun 18, 2013, 12:26 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 3 files +64 −2

t/README, t/t0000-basic.sh, t/test-lib.sh

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
Thomas Rast· Jun 18, 2013, 12:26 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 3 files +43 −1

t/README, t/test-lib.sh, t/valgrind/valgrind.sh

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
Thomas Rast· Jun 18, 2013, 12:26 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +15 −12
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
Thomas Rast· Jun 18, 2013, 12:26 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/test-lib.sh +84 −22
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

← back to recent threads