[PATCH] test-lib: Multi-prereq support only checked the last prereq

[PATCH] test-lib: Multi-prereq support only checked the last prereq

[Ævar Arnfjörð Bjarmason]

The support for multiple test prerequisites added by me in "test-lib:
Add support for multiple test prerequisites" was broken.

The clever for-loop (which I blindly copied from Junio) iterated over
each prerequisite and returned true/false within a case statement.
Thus only the last prerequisite in the list of prerequisites was ever
considered, the rest were ignored.

Fix that by changing the test_have_prereq code to something less
clever that keeps a count of the total prereqs and the ones we have
and compares the count at the end.

This comes with the added advantage that it's easy to list the missing
prerequisites in the test output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I should have spotted this earlier.

 t/t0000-basic.sh |    6 +++++-
 t/test-lib.sh    |   21 ++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2887677..9602085 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -84,7 +84,11 @@ donthaveit=yes
 test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
     donthaveit=no
 '
-if test $haveit$donthaveit != yesyes
+donthaveiteither=yes
+test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
+    donthaveiteither=no
+'
+if test $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4e73fff..8c8e129 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -337,15 +337,30 @@ test_have_prereq () {
 	IFS=,
 	set -- $*
 	IFS=$save_IFS
+
+	total_prereq=0
+	ok_prereq=0
+	missing_prereq=
+
 	for prerequisite
 	do
+		total_prereq=$(($total_prereq + 1))
 		case $satisfied in
 		*" $prerequisite "*)
-			: yes, have it ;;
+			ok_prereq=$(($ok_prereq + 1))
+			;;
 		*)
-			! : nope ;;
+			# Keep a list of missing prerequisites
+			if test -z "$missing_prereq"
+			then
+				missing_prereq=$prerequisite
+			else
+				missing_prereq="$prerequisite,$missing_prereq"
+			fi
 		esac
 	done
+
+	test $total_prereq = $ok_prereq
 }
 
 # You are not expected to call test_ok_ and test_failure_ directly, use
@@ -408,7 +423,7 @@ test_skip () {
 	case "$to_skip" in
 	t)
 		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (prereqs: $prereq)"
+		say_color skip "ok $test_count # skip $1 (missing $missing_prereq of $prereq)"
 		: true
 		;;
 	*)
-- 
1.7.2.1.295.gd03d

Re: [PATCH] test-lib: Multi-prereq support only checked the last prereq

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> The clever for-loop (which I blindly copied from Junio)

You did not copy his “return” statement, though. :)

> +++ b/t/test-lib.sh
> @@ -337,15 +337,30 @@ test_have_prereq () {
>  	IFS=,
>  	set -- $*
>  	IFS=$save_IFS
> +
> +	total_prereq=0
> +	ok_prereq=0
> +	missing_prereq=
> +
>  	for prerequisite
>  	do
> +		total_prereq=$(($total_prereq + 1))
>  		case $satisfied in
>  		*" $prerequisite "*)
> -			: yes, have it ;;
> +			ok_prereq=$(($ok_prereq + 1))
> +			;;
>  		*)
> -			! : nope ;;
> +			# Keep a list of missing prerequisites
> +			if test -z "$missing_prereq"
> +			then
> +				missing_prereq=$prerequisite
> +			else
> +				missing_prereq="$prerequisite,$missing_prereq"
> +			fi
>  		esac
>  	done
> +
> +	test $total_prereq = $ok_prereq
>  }

Wouldn’t

-		! : nope ;;
+		return 1 ;;

be simpler?

This and a small cleanup, including a test that catches this, are at
http://thread.gmane.org/gmane.comp.version-control.git/152814/focus=152875

Hope that helps,
Jonathan

[PATCH v2] test-lib: Multi-prereq support only checked the last prereq

[Ævar Arnfjörð Bjarmason]

The support for multiple test prerequisites added by me in "test-lib:
Add support for multiple test prerequisites" was broken.

The for iterated over each prerequisite and returned true/false within
a case statement, but since it missed a return statement only the last
prerequisite in the list of prerequisites was ever considered, the
rest were ignored.

Fix that by changing the test_have_prereq code to something less
clever that keeps a count of the total prereqs and the ones we have
and compares the count at the end.

This comes with the added advantage that it's easy to list the missing
prerequisites in the test output, implement that while I'm at it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This v2 patch has a new commit message.

On Wed, Aug 11, 2010 at 01:43, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> The clever for-loop (which I blindly copied from Junio)
>
> You did not copy his “return” statement, though. :)

Oops, yeah. I copied it from some mix-match of different
patches. Corrected the patch message for that.

> Wouldn’t
>
> -               ! : nope ;;
> +               return 1 ;;
>
> be simpler?

Yeah, that'd make it work, but I wanted to add something that spewed a
complete listing of missing prereqs for ecah test, bailing out with a
return wouldn't do that.

 t/t0000-basic.sh |    6 +++++-
 t/test-lib.sh    |   21 ++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2887677..9602085 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -84,7 +84,11 @@ donthaveit=yes
 test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
     donthaveit=no
 '
-if test $haveit$donthaveit != yesyes
+donthaveiteither=yes
+test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
+    donthaveiteither=no
+'
+if test $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4e73fff..8c8e129 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -337,15 +337,30 @@ test_have_prereq () {
 	IFS=,
 	set -- $*
 	IFS=$save_IFS
+
+	total_prereq=0
+	ok_prereq=0
+	missing_prereq=
+
 	for prerequisite
 	do
+		total_prereq=$(($total_prereq + 1))
 		case $satisfied in
 		*" $prerequisite "*)
-			: yes, have it ;;
+			ok_prereq=$(($ok_prereq + 1))
+			;;
 		*)
-			! : nope ;;
+			# Keep a list of missing prerequisites
+			if test -z "$missing_prereq"
+			then
+				missing_prereq=$prerequisite
+			else
+				missing_prereq="$prerequisite,$missing_prereq"
+			fi
 		esac
 	done
+
+	test $total_prereq = $ok_prereq
 }
 
 # You are not expected to call test_ok_ and test_failure_ directly, use
@@ -408,7 +423,7 @@ test_skip () {
 	case "$to_skip" in
 	t)
 		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (prereqs: $prereq)"
+		say_color skip "ok $test_count # skip $1 (missing $missing_prereq of $prereq)"
 		: true
 		;;
 	*)
-- 
1.7.2.1.295.gdf931

Re: [PATCH v2] test-lib: Multi-prereq support only checked the last prereq

[Johannes Sixt]

Am 11.08.2010 14:04, schrieb Ævar Arnfjörð Bjarmason:
> -		say_color skip "ok $test_count # skip $1 (prereqs: $prereq)"
> +		say_color skip "ok $test_count # skip $1 (missing $missing_prereq of $prereq)"

This needs fixing: When a test is skipped due to being listed in 
GIT_SKIP_TESTS, then the output looks like this:

ok 47 # skip --ignore-submodules=untracked suppresses submodules with 
untracked content (missing SANITY,POSIXPERM of )

(In this case, there was a test before number 47 that was skipped due to 
missing prerequisite.)

Please also note that - when a prerequisite is not satisfied - the text in 
parentheses does not say "prerequisite" or similar anymore. IMO, this 
should be added back.

-- Hannes

[PATCH] tests: simplify "missing PREREQ" message

[Jonathan Nieder]

When a test has no prerequisites satisfied (the usual case), instead
of "missing THING of THING", just say "missing THING".  This does not
affect the output when a test is skipped due to a missing
prerequisites if another prerequisite is satisfied.

For example: instead of

 ok 8 # skip notes work (missing EXPENSIVE of EXPENSIVE)
 ok 9 # skip notes timing with /usr/bin/time (missing EXPENSIVE of USR_BIN_TIME,EXPENSIVE)

write

 ok 8 # skip notes work (missing EXPENSIVE)
 ok 9 # skip notes timing with /usr/bin/time (missing EXPENSIVE of USR_BIN_TIME,EXPENSIVE)

Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Noticed this when building "next".  Thoughts?

 t/test-lib.sh |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4617998..c9193cb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -417,8 +417,14 @@ test_skip () {
 	fi
 	case "$to_skip" in
 	t)
+		of_prereq=
+		if test "$missing_prereq" != "$prereq"
+		then
+			of_prereq=" of $prereq"
+		fi
+
 		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (missing $missing_prereq of $prereq)"
+		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
 		: true
 		;;
 	*)
-- 
1.7.2.2

Re: [PATCH] tests: simplify "missing PREREQ" message

[Johannes Sixt]

Am 8/24/2010 9:34, schrieb Jonathan Nieder:
> @@ -417,8 +417,14 @@ test_skip () {
>  	fi
>  	case "$to_skip" in
>  	t)
> +		of_prereq=
> +		if test "$missing_prereq" != "$prereq"
> +		then
> +			of_prereq=" of $prereq"
> +		fi
> +
>  		say_color skip >&3 "skipping test: $@"
> -		say_color skip "ok $test_count # skip $1 (missing $missing_prereq of $prereq)"
> +		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
>  		: true
>  		;;
>  	*)

While you are touching this area, would also fix messages like these:

ok 70 # skip set --path (missing  of )
ok 71 # skip get --path (missing  of HOMEVAR)
ok 72 # skip get --path copes with unset $HOME (missing  of )

which are due to GIT_SKIP_TESTS=t1300.7[012]

-- Hannes

Re: [PATCH] tests: simplify "missing PREREQ" message

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Noticed this when building "next".  Thoughts?

Definitely nicer ;-)