Subject: Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target

From: Junio C Hamano <[email-masked]>
Date: Thu, 19 Sep 2013 12:52:50 -0700

Junio C Hamano <[email-masked]> writes:

> Thomas Gummerer <[email-masked]> writes:
>
>> When one performance test fails, the testing is aborted and the cleanup
>> commands are not executed anymore, leaving the trash directory in the
>> failed state.
>
> Ah, that I overlooked. In that case, the comments in my previous
> message do not apply.

Having said that, it was unclear to me why we would want a new
test_perf_cleanup added.

The name is misleading (does it define only clean-up actions?) to
say the least, and one way of fixing it would be to call it
test_perf_with_cleanup.

I wondered why this clean-up section cannot be an optional parameter
to test_perf, but that would not fly well because we won't know if
3-arg form is with one prerequisite and no clean-up, or no prereq
with a clean-up, so perhaps adding a new function may be the best
you could do.

But in the longer term, I think we would be better off to allow two
styles (one traditional, another modern) and add new features only
to the "modern" (aka "more easily extensible") form:

	test_perf [PREREQ] BODY
	test_perf [--prereq PREREQ] [--cleanup CLEANUP] ... BODY

perhaps like this (this is without --cleanup but it should be
obvious how to add a support for it to the command line parser).

The patch to t0008 is primarily to adjust for test labels that begin
with double-dashes that will be mistaken as a new-style option, but
it is unnecessarily big because it uses random custom shell functions
to hide the real calls to underlying test_expect_success X-<.

 t/test-lib-functions.sh | 72 ++++++++++++++++++++++++++++++++++++++-----------
 t/perf/perf-lib.sh      | 17 +++++-------
 t/t0008-ignores.sh      | 43 +++++++++++++++--------------
 3 files changed, 87 insertions(+), 45 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index a7e9aac..10202dc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -342,20 +342,65 @@ test_declared_prereq () {
 	return 1
 }
 
+test_expect_parse () {
+	whoami=$1
+	shift
+	test_expect_new_style=
+	while case $# in 0) false ;; esac
+	do
+		case "$1" in
+		--prereq)
+			test $# -gt 1 ||
+			error "bug in the test script: --prereq needs a parameter"
+			test_prereq=$2
+			shift
+			;;
+		--)
+			shift
+			break
+			;;
+		--*)
+			error "bug in the test script: unknown option '$1'"
+			;;
+		*)
+			break
+			;;
+		esac
+		test_expect_new_style=yes
+		shift
+	done
+
+	# Traditional "test_expect_what [PREREQ] BODY"
+	if test -z "$test_expect_new_style"
+	then
+		test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
+	fi
+
+	if test $# != 2
+	then
+		if test -z "$test_expect_new_style"
+		then
+			error "bug in the test script: not 2 or 3 parameters to $whoami"
+		else
+			error "bug in the test script: not 2 parameters to $whoami"
+		fi
+	fi
+	test_label=$1 test_body=$2
+
+	export test_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"
-	export test_prereq
+	test_expect_parse test_expect_failure "$@"
 	if ! test_skip "$@"
 	then
-		say >&3 "checking known breakage: $2"
-		if test_run_ "$2" expecting_failure
+		say >&3 "checking known breakage: $test_body"
+		if test_run_ "$test_body" expecting_failure
 		then
-			test_known_broken_ok_ "$1"
+			test_known_broken_ok_ "$test_label"
 		else
-			test_known_broken_failure_ "$1"
+			test_known_broken_failure_ "$test_label"
 		fi
 	fi
 	test_finish_
@@ -363,16 +408,13 @@ test_expect_failure () {
 
 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"
-	export test_prereq
+	test_expect_parse test_expect_success "$@"
 	if ! test_skip "$@"
 	then
-		say >&3 "expecting success: $2"
-		if test_run_ "$2"
+		say >&3 "expecting success: $test_body"
+		if test_run_ "$test_body"
 		then
-			test_ok_ "$1"
+			test_ok_ "$test_label"
 		else
 			test_failure_ "$@"
 		fi
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f4eecaa..6477d38 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -151,23 +151,20 @@ exit $ret' >&3 2>&4
 
 test_perf () {
 	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"
-	export test_prereq
+	test_expect_parse test_perf "$@"
 	if ! test_skip "$@"
 	then
 		base=$(basename "$0" .sh)
 		echo "$test_count" >>"$perf_results_dir"/$base.subtests
-		echo "$1" >"$perf_results_dir"/$base.$test_count.descr
+		echo "$test_label" >"$perf_results_dir"/$base.$test_count.descr
 		if test -z "$verbose"; then
-			printf "%s" "perf $test_count - $1:"
+			printf "%s" "perf $test_count - $test_label:"
 		else
-			echo "perf $test_count - $1:"
+			echo "perf $test_count - $test_label:"
 		fi
 		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
-			say >&3 "running: $2"
-			if test_run_perf_ "$2"
+			say >&3 "running: $test_body"
+			if test_run_perf_ "$test_body"
 			then
 				if test -z "$verbose"; then
 					printf " %s" "$i"
@@ -183,7 +180,7 @@ test_perf () {
 		if test -z "$verbose"; then
 			echo " ok"
 		else
-			test_ok_ "$1"
+			test_ok_ "$test_label"
 		fi
 		base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count"
 		"$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 96f40fe..795de61 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -106,7 +106,7 @@ test_expect_success_multi () {
 	expect_verbose=$( echo "$expect_all" | grep -v '^::	' )
 	expect=$( echo "$expect_verbose" | sed -e 's/.*	//' )
 
-	test_expect_success $prereq "$testname" '
+	test_expect_success ${prereq:+--prereq $prereq} -- "$testname" '
 		expect "$expect" &&
 		eval "$code"
 	'
@@ -116,10 +116,11 @@ test_expect_success_multi () {
 	then
 		for quiet_opt in '-q' '--quiet'
 		do
-			test_expect_success $prereq "$testname${quiet_opt:+ with $quiet_opt}" "
-			expect '' &&
-			$code
-		"
+			test_expect_success ${prereq:+--prereq $prereq} -- \
+				"$testname${quiet_opt:+ with $quiet_opt}" '
+				expect "" &&
+				eval "$code"
+			'
 		done
 		quiet_opt=
 	fi
@@ -140,7 +141,9 @@ test_expect_success_multi () {
 				$code
 			"
 			opts="$verbose_opt$non_matching_opt"
-			test_expect_success $prereq "$testname${opts:+ with $opts}" "$test_code"
+
+			test_expect_success ${prereq:+--prereq $prereq} -- \
+				"$testname${opts:+ with $opts}" "$test_code"
 		done
 	done
 	verbose_opt=
@@ -225,7 +228,7 @@ test_expect_success '-q with multiple args' '
 	stderr_contains "fatal: --quiet is only valid with a single pathname"
 '
 
-test_expect_success '--quiet with multiple args' '
+test_expect_success -- '--quiet with multiple args' '
 	expect "" &&
 	test_check_ignore "--quiet one two" 128 &&
 	stderr_contains "fatal: --quiet is only valid with a single pathname"
@@ -235,7 +238,7 @@ for verbose_opt in '-v' '--verbose'
 do
 	for quiet_opt in '-q' '--quiet'
 	do
-		test_expect_success "$quiet_opt $verbose_opt" "
+		test_expect_success -- "$quiet_opt $verbose_opt" "
 			expect '' &&
 			test_check_ignore '$quiet_opt $verbose_opt foo' 128 &&
 			stderr_contains 'fatal: cannot have both --quiet and --verbose'
@@ -243,7 +246,7 @@ do
 	done
 done
 
-test_expect_success '--quiet with multiple args' '
+test_expect_success -- '--quiet with multiple args' '
 	expect "" &&
 	test_check_ignore "--quiet one two" 128 &&
 	stderr_contains "fatal: --quiet is only valid with a single pathname"
@@ -559,34 +562,34 @@ sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
 sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
 	tr ":\t\n" "\0" >expected-verbose0
 
-test_expect_success '--stdin' '
+test_expect_success -- '--stdin' '
 	expect_from_stdin <expected-default &&
 	test_check_ignore "--stdin" <stdin
 '
 
-test_expect_success '--stdin -q' '
+test_expect_success -- '--stdin -q' '
 	expect "" &&
 	test_check_ignore "-q --stdin" <stdin
 '
 
-test_expect_success '--stdin -v' '
+test_expect_success -- '--stdin -v' '
 	expect_from_stdin <expected-verbose &&
 	test_check_ignore "-v --stdin" <stdin
 '
 
 for opts in '--stdin -z' '-z --stdin'
 do
-	test_expect_success "$opts" "
+	test_expect_success -- "$opts" "
 		expect_from_stdin <expected-default0 &&
 		test_check_ignore '$opts' <stdin0
 	"
 
-	test_expect_success "$opts -q" "
+	test_expect_success -- "$opts -q" "
 		expect "" &&
 		test_check_ignore '-q $opts' <stdin0
 	"
 
-	test_expect_success "$opts -v" "
+	test_expect_success -- "$opts -v" "
 		expect_from_stdin <expected-verbose0 &&
 		test_check_ignore '-v $opts' <stdin0
 	"
@@ -645,7 +648,7 @@ sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
 sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
 	tr ":\t\n" "\0" >expected-verbose0
 
-test_expect_success '--stdin from subdirectory' '
+test_expect_success -- '--stdin from subdirectory' '
 	expect_from_stdin <expected-default &&
 	(
 		cd a &&
@@ -653,7 +656,7 @@ test_expect_success '--stdin from subdirectory' '
 	)
 '
 
-test_expect_success '--stdin from subdirectory with -v' '
+test_expect_success -- '--stdin from subdirectory with -v' '
 	expect_from_stdin <expected-verbose &&
 	(
 		cd a &&
@@ -661,7 +664,7 @@ test_expect_success '--stdin from subdirectory with -v' '
 	)
 '
 
-test_expect_success '--stdin from subdirectory with -v -n' '
+test_expect_success -- '--stdin from subdirectory with -v -n' '
 	expect_from_stdin <expected-all &&
 	(
 		cd a &&
@@ -671,7 +674,7 @@ test_expect_success '--stdin from subdirectory with -v -n' '
 
 for opts in '--stdin -z' '-z --stdin'
 do
-	test_expect_success "$opts from subdirectory" '
+	test_expect_success -- "$opts from subdirectory" '
 		expect_from_stdin <expected-default0 &&
 		(
 			cd a &&
@@ -679,7 +682,7 @@ do
 		)
 	'
 
-	test_expect_success "$opts from subdirectory with -v" '
+	test_expect_success -- "$opts from subdirectory with -v" '
 		expect_from_stdin <expected-verbose0 &&
 		(
 			cd a &&