[PATCH 0/2] Revert --valgrind-parallel test option

[PATCH 0/2] Revert --valgrind-parallel test option

[Thomas Rast]

These patches remove the --valgrind-parallel=N option that was broken
from the outset (shame on me).  Peff's judgement at the time that its
usefulness would approximately be "meh" turns out to be correct.

What's not in the commit message, but drives part of my reasoning in
doing a revert instead of a fix: I did fix it up locally only to
notice that it was too slow in this case for what I actually wanted to
use it for.  The only valgrind-test workflow that I find bearable is
to run all the tests in the background under prove (takes hours), and
then use the prove output (which says exactly which subtests fail) in
--valgrind-only=<subtest>.  So the latter is -- again Peff was right
-- the really useful thing.

The only consolation is that I apparently didn't break any other use
of the test suite -- otherwise it would presumably have been fixed
very quickly.

Thomas Rast (2):
  Revert "test-lib: support running tests under valgrind in parallel"
  Revert "test-lib: allow prefixing a custom string before "ok N" etc."

 t/test-lib.sh | 133 +++++++++++++++-------------------------------------------
 1 file changed, 34 insertions(+), 99 deletions(-)

-- 
1.8.4.1.810.g312044e

[PATCH 1/2] Revert "test-lib: support running tests under valgrind in parallel"

[Thomas Rast]

This reverts commit ad0e6233320b004f0d686f6887c803e508607bd2.

--valgrind-parallel was broken from the start: during review I made
the whole valgrind setup code conditional on not being a
--valgrind-parallel worker child.  But even the children crucially
need $GIT_VALGRIND to be set; it should therefore have been set
outside the conditional.

The fix would be a two-liner, but since the introduction of the
feature, almost four months have passed without anyone noticing that
it is broken.  So this feature is not worth the about hundred lines of
test-lib.sh complexity.  Revert it.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 t/test-lib.sh | 106 ++++++++++++----------------------------------------------
 1 file changed, 22 insertions(+), 84 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0fa7dfd..eaf6759 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -205,15 +205,6 @@ 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=*)
@@ -227,7 +218,7 @@ do
 	esac
 done
 
-if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
+if test -n "$valgrind_only"
 then
 	test -z "$valgrind" && valgrind=memcheck
 	test -z "$verbose" && verbose_only="$valgrind_only"
@@ -377,9 +368,7 @@ maybe_teardown_verbose () {
 last_verbose=t
 maybe_setup_verbose () {
 	test -z "$verbose_only" && 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; }
+	if match_pattern_list $test_count $verbose_only
 	then
 		exec 4>&2 3>&1
 		# Emit a delimiting blank line when going from
@@ -403,7 +392,7 @@ maybe_teardown_valgrind () {
 
 maybe_setup_valgrind () {
 	test -z "$GIT_VALGRIND" && return
-	if test -z "$valgrind_only" && test -z "$valgrind_only_stride"
+	if test -z "$valgrind_only"
 	then
 		GIT_VALGRIND_ENABLED=t
 		return
@@ -412,10 +401,6 @@ 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
 }
 
@@ -568,9 +553,6 @@ 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 () {
@@ -618,42 +600,33 @@ then
 		make_symlink "$symlink_target" "$GIT_VALGRIND/bin/$base" || exit
 	}
 
-	# 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
-		done
-		# special-case the mergetools loadables
-		make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
-		OLDIFS=$IFS
-		IFS=:
-		for path in $PATH
+	# 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
 		do
-			ls "$path"/git-* 2> /dev/null |
-			while read file
-			do
-				make_valgrind_symlink "$file"
-			done
+			make_valgrind_symlink "$file"
 		done
-		IFS=$OLDIFS
-	fi
+	done
+	IFS=$OLDIFS
 	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
-	if test -n "$valgrind_only" || test -n "$valgrind_only_stride"
-	then
-		GIT_VALGRIND_ENABLED=
-	fi
+	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
 	export GIT_VALGRIND_ENABLED
 elif test -n "$GIT_TEST_INSTALLED"
 then
@@ -730,41 +703,6 @@ 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.4.1.810.g312044e

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

[Thomas Rast]

Now that ad0e623 (test-lib: support running tests under valgrind in
parallel, 2013-06-23) has been reverted, this support code has no
users any more.  Revert it, too.

This reverts commit e939e15d241e942662b9f88f6127ab470ab0a0b9.
---
 t/test-lib.sh | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index eaf6759..29c1410 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -210,9 +210,6 @@ 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
@@ -320,12 +317,12 @@ trap 'die' EXIT
 
 test_ok_ () {
 	test_success=$(($test_success + 1))
-	say_color "" "${statusprefix}ok $test_count - $@"
+	say_color "" "ok $test_count - $@"
 }
 
 test_failure_ () {
 	test_failure=$(($test_failure + 1))
-	say_color error "${statusprefix}not ok $test_count - $1"
+	say_color error "not ok $test_count - $1"
 	shift
 	echo "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
@@ -333,12 +330,12 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color error "${statusprefix}ok $test_count - $@ # TODO known breakage vanished"
+	say_color error "ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
-	say_color warn "${statusprefix}not ok $test_count - $@ # TODO known breakage"
+	say_color warn "not ok $test_count - $@ # TODO known breakage"
 }
 
 test_debug () {
@@ -462,8 +459,8 @@ test_skip () {
 			of_prereq=" of $test_prereq"
 		fi
 
-		say_color skip >&3 "${statusprefix}skipping test: $@"
-		say_color skip "${statusprefix}ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
+		say_color skip >&3 "skipping test: $@"
+		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
 		: true
 		;;
 	*)
@@ -501,11 +498,11 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color error "${statusprefix}# $test_fixed known breakage(s) vanished; please update test(s)"
+		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
-		say_color warn "${statusprefix}# still have $test_broken known breakage(s)"
+		say_color warn "# still have $test_broken known breakage(s)"
 	fi
 	if test "$test_broken" != 0 || test "$test_fixed" != 0
 	then
@@ -528,9 +525,9 @@ test_done () {
 		then
 			if test $test_remaining -gt 0
 			then
-				say_color pass "${statusprefix}# passed all $msg"
+				say_color pass "# passed all $msg"
 			fi
-			say "${statusprefix}1..$test_count$skip_all"
+			say "1..$test_count$skip_all"
 		fi
 
 		test -d "$remove_trash" &&
@@ -544,8 +541,8 @@ test_done () {
 	*)
 		if test $test_external_has_tap -eq 0
 		then
-			say_color error "${statusprefix}# failed $test_failure among $msg"
-			say "${statusprefix}1..$test_count"
+			say_color error "# failed $test_failure among $msg"
+			say "1..$test_count"
 		fi
 
 		exit 1 ;;
-- 
1.8.4.1.810.g312044e