All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] test-lib: Multi-prereq support only checked the last prereq
@ 2010-08-10 23:21 Ævar Arnfjörð Bjarmason
  2010-08-11  1:43 ` Jonathan Nieder
  0 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-10 23:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Æ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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] test-lib: Multi-prereq support only checked the last prereq
  2010-08-10 23:21 [PATCH] test-lib: Multi-prereq support only checked the last prereq Ævar Arnfjörð Bjarmason
@ 2010-08-11  1:43 ` Jonathan Nieder
  2010-08-11 12:04   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Nieder @ 2010-08-11  1:43 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano

Æ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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] test-lib: Multi-prereq support only checked the last prereq
  2010-08-11  1:43 ` Jonathan Nieder
@ 2010-08-11 12:04   ` Ævar Arnfjörð Bjarmason
  2010-08-13 20:28     ` Johannes Sixt
  2010-08-24  7:34     ` [PATCH] tests: simplify "missing PREREQ" message Jonathan Nieder
  0 siblings, 2 replies; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-11 12:04 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jonathan Nieder, Æ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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] test-lib: Multi-prereq support only checked the last prereq
  2010-08-11 12:04   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
@ 2010-08-13 20:28     ` Johannes Sixt
  2010-08-24  7:34     ` [PATCH] tests: simplify "missing PREREQ" message Jonathan Nieder
  1 sibling, 0 replies; 7+ messages in thread
From: Johannes Sixt @ 2010-08-13 20:28 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] tests: simplify "missing PREREQ" message
  2010-08-11 12:04   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
  2010-08-13 20:28     ` Johannes Sixt
@ 2010-08-24  7:34     ` Jonathan Nieder
  2010-08-24 10:16       ` Johannes Sixt
  2010-08-24 19:01       ` Junio C Hamano
  1 sibling, 2 replies; 7+ messages in thread
From: Jonathan Nieder @ 2010-08-24  7:34 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests: simplify "missing PREREQ" message
  2010-08-24  7:34     ` [PATCH] tests: simplify "missing PREREQ" message Jonathan Nieder
@ 2010-08-24 10:16       ` Johannes Sixt
  2010-08-24 19:01       ` Junio C Hamano
  1 sibling, 0 replies; 7+ messages in thread
From: Johannes Sixt @ 2010-08-24 10:16 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tests: simplify "missing PREREQ" message
  2010-08-24  7:34     ` [PATCH] tests: simplify "missing PREREQ" message Jonathan Nieder
  2010-08-24 10:16       ` Johannes Sixt
@ 2010-08-24 19:01       ` Junio C Hamano
  1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2010-08-24 19:01 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Ævar Arnfjörð Bjarmason, git

Jonathan Nieder <jrnieder@gmail.com> writes:

> Noticed this when building "next".  Thoughts?

Definitely nicer ;-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2010-08-24 19:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-10 23:21 [PATCH] test-lib: Multi-prereq support only checked the last prereq Ævar Arnfjörð Bjarmason
2010-08-11  1:43 ` Jonathan Nieder
2010-08-11 12:04   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2010-08-13 20:28     ` Johannes Sixt
2010-08-24  7:34     ` [PATCH] tests: simplify "missing PREREQ" message Jonathan Nieder
2010-08-24 10:16       ` Johannes Sixt
2010-08-24 19:01       ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.