threads / patch / 24702

patchtest-lib: Multi-prereq support only checked the last prereq

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

## tl;dr

7 messages between Aug 10, 2010 and Aug 24, 2010. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Aug 10, 2010, 23:21 UTC · lore

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

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

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
Jonathan Nieder· Aug 11, 2010, 01:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
> The clever for-loop (which I blindly copied from Junio)
You did not copy his “return” statement, though. :)
Show 32 quoted lines
> +++ 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

Ævar Arnfjörð Bjarmason· Aug 11, 2010, 12:04 UTC · re: Jonathan Nieder · lore

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

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:
Show 5 quoted lines
> Æ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.

Show 6 quoted lines
> 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(-)
Show changes to 2 files +23 −4

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

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
Johannes Sixt· Aug 13, 2010, 20:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Jonathan Nieder· Aug 24, 2010, 07:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH] tests: simplify "missing PREREQ" message

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(-)
Show changes to t/test-lib.sh +7 −1
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
Johannes Sixt· Aug 24, 2010, 10:16 UTC · re: Jonathan Nieder · lore

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

Am 8/24/2010 9:34, schrieb Jonathan Nieder:
Show 16 quoted lines
> @@ -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

← back to recent threads