[PATCH 1/2] test: rename $satisfied to $satisfied_prereq

[PATCH 1/2] test: rename $satisfied to $satisfied_prereq

[Junio C Hamano]

All other shell variables that globally keep track of prerequisite
related states have "prereq" somewhere in their names.  Be consistent
and avoid name crashes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Just a preparatory clean-up.

 t/test-lib-functions.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80daaca..4dc027d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -221,9 +221,9 @@ write_script () {
 # capital letters by convention).
 
 test_set_prereq () {
-	satisfied="$satisfied$1 "
+	satisfied_prereq="$satisfied_prereq$1 "
 }
-satisfied=" "
+satisfied_prereq=" "
 
 test_have_prereq () {
 	# prerequisites can be concatenated with ','
@@ -239,7 +239,7 @@ test_have_prereq () {
 	for prerequisite
 	do
 		total_prereq=$(($total_prereq + 1))
-		case $satisfied in
+		case "$satisfied_prereq" in
 		*" $prerequisite "*)
 			ok_prereq=$(($ok_prereq + 1))
 			;;
-- 
1.7.12.rc0.44.gc69a8ad

[RFC/PATCH 2/2] test: allow prerequisite to be evaluated lazily

[Junio C Hamano]

The test prerequisite mechanism is a useful way to allow some tests
in a test script to be skipped in environments that do not offer
certain features (e.g. checking how symbolic links are handled on
filesystems that do not support them).  It is OK for commonly used
prerequisites to be always tested during start-up of test script by
having a codeblock that tests a feature and calls test_set_prereq,
but for an uncommon feature, forcing 90% of scripts to pay the same
probing overhead for prerequisite they do not care about is wasteful.

Introduce a mechanism to probe the prerequiste lazily.  Changes are:

 - test_lazy_prereq () function, which takes the name of the
   prerequisite it probes and the script to probe for it, is
   added.  This only registers the name of the prerequiste that can
   be lazily probed and the script to eval (without running).

 - test_have_prereq() function (which is used by test_expect_success
   and also can be called directly by test scripts) learns to look
   at the list of prerequisites that can be lazily probed, and the
   prerequisites that have already been probed that way.

Update the codeblock to probe and set SYMLINKS prerequisite using
the new mechanism as an example.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I thought about various error conditions but didn't come up with
   a solid conclusion.  For example, what should happen when the
   prober directory cannot be created, or removed?  Perhaps aborting
   the whole thing may be a safe and better option.

   Also, I am not distinguishing a syntax error in the script and
   "the prerequisite is not satisfied" signal (they would both be a
   false in the "if ()" part). I do not think we care too much, but
   others may have better ideas.

 t/test-lib-functions.sh | 28 ++++++++++++++++++++++++++++
 t/test-lib.sh           |  7 ++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4dc027d..2214388 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -224,6 +224,13 @@ test_set_prereq () {
 	satisfied_prereq="$satisfied_prereq$1 "
 }
 satisfied_prereq=" "
+lazy_testable_prereq= lazy_tested_prereq=
+
+# Usage: test_lazy_prereq PREREQ 'script'
+test_lazy_prereq () {
+	lazy_testable_prereq="$lazy_testable_prereq$1 "
+	eval test_prereq_lazily_$1=\$2
+}
 
 test_have_prereq () {
 	# prerequisites can be concatenated with ','
@@ -238,6 +245,27 @@ test_have_prereq () {
 
 	for prerequisite
 	do
+		case " $lazy_tested_prereq " in
+		*" $prerequisite "*)
+			;;
+		*)
+			case " $lazy_testable_prereq " in
+			*" $prerequisite "*)
+				mkdir -p "$TRASH_DIRECTORY/prereq-test-dir"
+				if (
+					eval "script=\$test_prereq_lazily_$prerequisite" &&
+					cd "$TRASH_DIRECTORY/prereq-test-dir" &&
+					eval "$script"
+				)
+				then
+					test_set_prereq $prerequisite
+				fi
+				rm -fr "$TRASH_DIRECTORY/prereq-test-dir"
+				lazy_tested_prereq="$lazy_tested_prereq$prerequisite "
+			esac
+			;;
+		esac
+
 		total_prereq=$(($total_prereq + 1))
 		case "$satisfied_prereq" in
 		*" $prerequisite "*)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bb4f886..878d000 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -659,9 +659,10 @@ test_i18ngrep () {
 	fi
 }
 
-# test whether the filesystem supports symbolic links
-ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
-rm -f y
+test_lazy_prereq SYMLINKS '
+	# test whether the filesystem supports symbolic links
+	ln -s x y 2>/dev/null && test -h y 2>/dev/null
+'
 
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
-- 
1.7.12.rc0.44.gc69a8ad

[PATCH 3/2] test-lib: provide case insensitivity as a prerequisite

[Junio C Hamano]

And on top, Michael's [1/5] would become like this, and [2/5] would
apply unchanged.

-- >8 --
From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Thu, 26 Jul 2012 15:39:53 +0200

Case insensitivity plays a role in several tests and is tested in several
tests. Therefore, move the test from t003 into the test lib and use the
prerequisite in t0003.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/README              |  4 ++++
 t/t0003-attributes.sh | 10 ----------
 t/test-lib.sh         |  6 ++++++
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/README b/t/README
index 4c3ea25..5725607 100644
--- a/t/README
+++ b/t/README
@@ -625,6 +625,10 @@ use these, and "test_set_prereq" for how to define your own.
    Git was compiled with USE_LIBPCRE=YesPlease. Wrap any tests
    that use git-grep --perl-regexp or git-grep -P in these.
 
+ - CASE_INSENSITIVE_FS
+
+   Test is run on a case insensitive file system.
+
 Tips for Writing Tests
 ----------------------
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 51f3045..febc45c 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -123,16 +123,6 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase
 
 '
 
-test_expect_success 'check whether FS is case-insensitive' '
-	mkdir junk &&
-	echo good >junk/CamelCase &&
-	echo bad >junk/camelcase &&
-	if test "$(cat junk/CamelCase)" != good
-	then
-		test_set_prereq CASE_INSENSITIVE_FS
-	fi
-'
-
 test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
 	test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
 	test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 878d000..52cb32a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -664,6 +664,12 @@ test_lazy_prereq SYMLINKS '
 	ln -s x y 2>/dev/null && test -h y 2>/dev/null
 '
 
+test_lazy_prereq CASE_INSENSITIVE_FS '
+	echo good >CamelCase &&
+	echo bad >camelcase &&
+	test "$(cat CamelCase)" != good
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
-- 
1.7.12.rc0.44.gc69a8ad

[PATCH 4/2] test-lib: provide UTF8 behaviour as a prerequisite

[Junio C Hamano]

And Michael's [4/5] would become like this (again, [3/5] does not
need any change).

-- >8 --

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Thu, 26 Jul 2012 15:39:56 +0200

UTF8 behaviour of the filesystem (conversion from nfd to nfc)  plays a
role in several tests and is tested in several tests. Therefore, move
the test from t0050 into the test lib and use the prerequisite in t0050.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/README              |  5 +++++
 t/t0050-filesystem.sh | 24 +++++++-----------------
 t/test-lib.sh         | 13 +++++++++++++
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/t/README b/t/README
index 5725607..e4128e5 100644
--- a/t/README
+++ b/t/README
@@ -629,6 +629,11 @@ use these, and "test_set_prereq" for how to define your own.
 
    Test is run on a case insensitive file system.
 
+ - UTF8_NFD_TO_NFC
+
+   Test is run on a filesystem which converts decomposed utf-8 (nfd)
+   to precomposed utf-8 (nfc).
+
 Tips for Writing Tests
 ----------------------
 
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b46ae72..78816d9 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -7,22 +7,6 @@ test_description='Various filesystem issues'
 auml=$(printf '\303\244')
 aumlcdiar=$(printf '\141\314\210')
 
-unibad=
-test_expect_success 'see what we expect' '
-
-	test_unicode=test_expect_success &&
-	mkdir junk &&
-	>junk/"$auml" &&
-	case "$(cd junk && echo *)" in
-	"$aumlcdiar")
-		test_unicode=test_expect_failure &&
-		unibad=t
-		;;
-	*)	;;
-	esac &&
-	rm -fr junk
-'
-
 if test_have_prereq CASE_INSENSITIVE_FS
 then
 	say "will test on a case insensitive filesystem"
@@ -31,8 +15,14 @@ else
 	test_case=test_expect_success
 fi
 
-test "$unibad" &&
+if test_have_prereq UTF8_NFD_TO_NFC
+then
 	say "will test on a unicode corrupting filesystem"
+	test_unicode=test_expect_failure
+else
+	test_unicode=test_expect_success
+fi
+
 test_have_prereq SYMLINKS ||
 	say "will test on a filesystem lacking symbolic links"
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 52cb32a..95c966e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -670,6 +670,19 @@ test_lazy_prereq CASE_INSENSITIVE_FS '
 	test "$(cat CamelCase)" != good
 '
 
+test_lazy_prereq UTF8_NFD_TO_NFC '
+	# check whether FS converts nfd unicode to nfc
+	auml=$(printf "\303\244")
+	aumlcdiar=$(printf "\141\314\210")
+	>"$auml" &&
+	case "$(echo *)" in
+	"$aumlcdiar")
+		true ;;
+	*)
+		false ;;
+	esac
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
-- 
1.7.12.rc0.44.gc69a8ad

Re: [RFC/PATCH 2/2] test: allow prerequisite to be evaluated lazily

[Jeff King]

On Thu, Jul 26, 2012 at 04:11:00PM -0700, Junio C Hamano wrote:

> Introduce a mechanism to probe the prerequiste lazily.  Changes are:
> 
>  - test_lazy_prereq () function, which takes the name of the
>    prerequisite it probes and the script to probe for it, is
>    added.  This only registers the name of the prerequiste that can
>    be lazily probed and the script to eval (without running).
> 
>  - test_have_prereq() function (which is used by test_expect_success
>    and also can be called directly by test scripts) learns to look
>    at the list of prerequisites that can be lazily probed, and the
>    prerequisites that have already been probed that way.

The overall strategy looks good to me. A few minor comments follow.

>  * I thought about various error conditions but didn't come up with
>    a solid conclusion.  For example, what should happen when the
>    prober directory cannot be created, or removed?  Perhaps aborting
>    the whole thing may be a safe and better option.

Yeah, it would be safer. But we have the same problems in the probes
themselves, which sometimes have setup steps (e.g., before this patch,
they typically set up the equivalent of the probe directory themselves).
Fixing it completely would involve the probes returning a tri-state
(yes, no, error), but it would make them more annoying to write.

So you are at least preserving the status quo, and as far as I know,
that has never been a problem in practice.

>    Also, I am not distinguishing a syntax error in the script and
>    "the prerequisite is not satisfied" signal (they would both be a
>    false in the "if ()" part). I do not think we care too much, but
>    others may have better ideas.

For a syntax error in a regular test, the script aborts (and you get
a "FATAL: unexpected exit..."). It probably makes sense to do the same
thing here. I find it is helpful when writing tests to have an immediate
abort instead of having to investigate why your test did not pass.

> +			*" $prerequisite "*)
> +				mkdir -p "$TRASH_DIRECTORY/prereq-test-dir"
> +				if (
> +					eval "script=\$test_prereq_lazily_$prerequisite" &&
> +					cd "$TRASH_DIRECTORY/prereq-test-dir" &&
> +					eval "$script"
> +				)

I expected to see test_run_ here so we could get the usual redirections
and "-v" support in case the probe has useful output (and it might be
nice to report to the user during "-v" that we are checking the prereq,
and what we are about to do).

Pushing the subshell and chdir inside the test would let us just die
naturally on syntax errors, like regular tests do. There's no reason not
to put the mkdir there, too, so that somebody debugging via "-v" can see
more of what's happening.

Something like this on top of your patch:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2214388..f5831cc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -232,6 +232,21 @@ test_lazy_prereq () {
 	eval test_prereq_lazily_$1=\$2
 }
 
+test_run_prereq_ () {
+	script='
+mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
+(cd "$TRASH_DIRECTORY/prereq-test-dir" &&
+'"$2"'
+)
+'
+	say >&3 "checking prerequisite: $1"
+	say >&3 "$script"
+	test_eval_ "$script"
+	eval_ret=$?
+	rm -rf "$TRASH_DIRECTORY/prereq-test-dir"
+	return $eval_ret
+}
+
 test_have_prereq () {
 	# prerequisites can be concatenated with ','
 	save_IFS=$IFS
@@ -251,16 +266,11 @@ test_have_prereq () {
 		*)
 			case " $lazy_testable_prereq " in
 			*" $prerequisite "*)
-				mkdir -p "$TRASH_DIRECTORY/prereq-test-dir"
-				if (
-					eval "script=\$test_prereq_lazily_$prerequisite" &&
-					cd "$TRASH_DIRECTORY/prereq-test-dir" &&
-					eval "$script"
-				)
+				eval "script=\$test_prereq_lazily_$prerequisite" &&
+				if test_run_prereq_ "$prerequisite" "$script"
 				then
 					test_set_prereq $prerequisite
 				fi
-				rm -fr "$TRASH_DIRECTORY/prereq-test-dir"
 				lazy_tested_prereq="$lazy_tested_prereq$prerequisite "
 			esac
 			;;

Try it with a broken probe like:

  $ cat >foo.sh <<EOF
  #!/bin/sh

  test_description='broken lazy eval'
  . ./test-lib.sh
  test_lazy_prereq LAZY 'broken &&'
  test_expect_success LAZY 'need lazy' 'echo lazy ok'
  test_done
  EOF

  : properly alert us to syntactic error
  $ sh foo.sh
  FATAL: Unexpected exit with code 2

  : give sane output for debugging a broken probe
  $ sh foo.sh -v
  Initialized empty Git repository in /home/peff/compile/git/t/trash directory.foo/.git/
  checking prerequisite: LAZY

  mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
  (cd "$TRASH_DIRECTORY/prereq-test-dir" &&
  broken &&
  )

  foo.sh: 5: eval: Syntax error: ")" unexpected
  FATAL: Unexpected exit with code 2

And of course with non-broken probes, "-v" would also help to see exactly
when the probe is run, using which commands, and what output they
produced. E.g., for SYMLINKS, we should probably drop the stderr
redirection from the probe so we can see how "ln" complains.

It might also be worth doing a "say >&3" at the end of the check, too,
to tell "-v" users the outcome.

-Peff

Re: [RFC/PATCH 2/2] test: allow prerequisite to be evaluated lazily

[Jeff King]

On Fri, Jul 27, 2012 at 11:45:00AM -0400, Jeff King wrote:

> And of course with non-broken probes, "-v" would also help to see exactly
> when the probe is run, using which commands, and what output they
> produced. E.g., for SYMLINKS, we should probably drop the stderr
> redirection from the probe so we can see how "ln" complains.
> 
> It might also be worth doing a "say >&3" at the end of the check, too,
> to tell "-v" users the outcome.

Here is an updated version of my suggestion that does both of those
things.  I think it should just be squashed into your patch.

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2214388..d2ebe24 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -232,6 +232,26 @@ test_lazy_prereq () {
 	eval test_prereq_lazily_$1=\$2
 }
 
+test_run_prereq_ () {
+	script='
+mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
+(cd "$TRASH_DIRECTORY/prereq-test-dir" &&
+'"$2"'
+)
+'
+	say >&3 "checking prerequisite: $1"
+	say >&3 "$script"
+	test_eval_ "$script"
+	eval_ret=$?
+	rm -rf "$TRASH_DIRECTORY/prereq-test-dir"
+	if test "$eval_ret" = 0; then
+		say >&3 "prerequisite $1 ok"
+	else
+		say >&3 "prerequisite $1 not fulfilled"
+	fi
+	return $eval_ret
+}
+
 test_have_prereq () {
 	# prerequisites can be concatenated with ','
 	save_IFS=$IFS
@@ -251,16 +271,11 @@ test_have_prereq () {
 		*)
 			case " $lazy_testable_prereq " in
 			*" $prerequisite "*)
-				mkdir -p "$TRASH_DIRECTORY/prereq-test-dir"
-				if (
-					eval "script=\$test_prereq_lazily_$prerequisite" &&
-					cd "$TRASH_DIRECTORY/prereq-test-dir" &&
-					eval "$script"
-				)
+				eval "script=\$test_prereq_lazily_$prerequisite" &&
+				if test_run_prereq_ "$prerequisite" "$script"
 				then
 					test_set_prereq $prerequisite
 				fi
-				rm -fr "$TRASH_DIRECTORY/prereq-test-dir"
 				lazy_tested_prereq="$lazy_tested_prereq$prerequisite "
 			esac
 			;;
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 878d000..35739b9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -661,7 +661,7 @@ test_i18ngrep () {
 
 test_lazy_prereq SYMLINKS '
 	# test whether the filesystem supports symbolic links
-	ln -s x y 2>/dev/null && test -h y 2>/dev/null
+	ln -s x y && test -h y
 '
 
 # When the tests are run as root, permission tests will report that

Re: [RFC/PATCH 2/2] test: allow prerequisite to be evaluated lazily

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 27, 2012 at 11:45:00AM -0400, Jeff King wrote:
>
>> And of course with non-broken probes, "-v" would also help to see exactly
>> when the probe is run, using which commands, and what output they
>> produced. E.g., for SYMLINKS, we should probably drop the stderr
>> redirection from the probe so we can see how "ln" complains.
>> 
>> It might also be worth doing a "say >&3" at the end of the check, too,
>> to tell "-v" users the outcome.
>
> Here is an updated version of my suggestion that does both of those
> things.  I think it should just be squashed into your patch.

Thanks. The changes look good.