[PATCH 0/3] tests: Better prerequisite handling & documentation

[PATCH 0/3] tests: Better prerequisite handling & documentation

[Ævar Arnfjörð Bjarmason]

There were some useful nuggets in the "Tests in Cygwin" thread from
May last year that I've dug out and improved. This series adds support
for multiple test prerequisite, and improves the t/README
documentation by adding a "Prerequisites" section.

There's also a small fix to the raw test output.

Ævar Arnfjörð Bjarmason (3):
  test-lib: Add support for multiple test prerequisites
  test-lib: Print missing prerequisites in test output
  t/README: Document the predefined test prerequisites

 t/README         |   51 ++++++++++++++++++++++++++++++++++++++++++++++-----
 t/t0000-basic.sh |   17 +++++++++++++++++
 t/test-lib.sh    |   22 +++++++++++++++-------
 3 files changed, 78 insertions(+), 12 deletions(-)

[PATCH 1/3] test-lib: Add support for multiple test prerequisites

[Ævar Arnfjörð Bjarmason]

Change the test_have_prereq function in test-lib.sh to support a
comma-separated list of prerequisites. This is useful for tests that
need e.g. both POSIXPERM and SANITY.

The implementation was stolen from Junio C Hamano and Johannes Sixt,
the tests and documentation were not. See the "Tests in Cygwin" thread
in May 2009 for the originals:

    http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385
    http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118434

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README         |    6 ++++++
 t/t0000-basic.sh |   17 +++++++++++++++++
 t/test-lib.sh    |   20 ++++++++++++++------
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..d07b67a 100644
--- a/t/README
+++ b/t/README
@@ -350,6 +350,12 @@ library for your script to use.
 	test_expect_success TTY 'git --paginate rev-list uses a pager' \
 	    ' ... '
 
+   You can also supply a comma-separated list of prerequisites, in the
+   rare case where your test depends on more than one:
+
+	test_expect_success PERL,PYTHON 'yo dawg' \
+	    ' test $(perl -E 'print eval "1 +" . qx[python -c "print 2"]') == "4" '
+
  - test_expect_failure [<prereq>] <message> <script>
 
    This is NOT the opposite of test_expect_success, but is used
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f2c7336..2887677 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -73,6 +73,23 @@ then
 	exit 1
 fi
 
+test_set_prereq HAVETHIS
+haveit=no
+test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' '
+    test_have_prereq HAVEIT &&
+    test_have_prereq HAVETHIS &&
+    haveit=yes
+'
+donthaveit=yes
+test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
+    donthaveit=no
+'
+if test $haveit$donthaveit != yesyes
+then
+	say "bug in test framework: multiple prerequisite tags do not work reliably"
+	exit 1
+fi
+
 clean=no
 test_expect_success 'tests clean up after themselves' '
     test_when_finished clean=yes
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..8701923 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -327,12 +327,20 @@ test_set_prereq () {
 satisfied=" "
 
 test_have_prereq () {
-	case $satisfied in
-	*" $1 "*)
-		: yes, have it ;;
-	*)
-		! : nope ;;
-	esac
+	# prerequisites can be concatenated with ','
+	save_IFS=$IFS
+	IFS=,
+	set -- $*
+	IFS=$save_IFS
+	for prerequisite
+	do
+		case $satisfied in
+		*" $prerequisite "*)
+			: yes, have it ;;
+		*)
+			! : nope ;;
+		esac
+	done
 }
 
 # You are not expected to call test_ok_ and test_failure_ directly, use
-- 
1.7.1

[PATCH 2/3] test-lib: Print missing prerequisites in test output

[Ævar Arnfjörð Bjarmason]

Change the test output to print needed prerequisites as part of the
TAP. This makes it easy to see at a glance why a test was
skipped. Before:

    ok 7 # skip <message>
    ok 9 # skip <message>

After:

    ok 7 # skip <message> (prereqs: DONTHAVEIT)
    ok 9 # skip <message> (prereqs: HAVEIT,DONTHAVEIT)

This'll also be useful for smoke testing output, where the developer
reading the output may not be familiar with the system where tests are
being skipped.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8701923..4e0a1c3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -403,7 +403,7 @@ test_skip () {
 	case "$to_skip" in
 	t)
 		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1"
+		say_color skip "ok $test_count # skip $1 (prereqs: $prereq)"
 		: true
 		;;
 	*)
-- 
1.7.1

[PATCH 3/3] t/README: Document the predefined test prerequisites

[Ævar Arnfjörð Bjarmason]

The README for the test library suggested that you grep the
test-lib.sh for test_set_prereq to see what the preset prerequisites
were.

Remove that bit, and write a section explaining all the preset
prerequisites. Most of the text was lifted from from Junio C Hamano
and Johannes Sixt, See the "Tests in Cygwin" thread in May 2009 for
the originals:

    http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385
    http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118434

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   45 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index d07b67a..dc07939 100644
--- a/t/README
+++ b/t/README
@@ -410,11 +410,12 @@ library for your script to use.
  - test_set_prereq SOME_PREREQ
 
    Set a test prerequisite to be used later with test_have_prereq. The
-   test-lib will set some prerequisites for you, e.g. PERL and PYTHON
-   which are derived from ./GIT-BUILD-OPTIONS (grep test_set_prereq
-   test-lib.sh for more). Others you can set yourself and use later
-   with either test_have_prereq directly, or the three argument
-   invocation of test_expect_success and test_expect_failure.
+   test-lib will set some prerequisites for you, see the
+   "Prerequisites" section below for a full list of these.
+
+   Others you can set yourself and use later with either
+   test_have_prereq directly, or the three argument invocation of
+   test_expect_success and test_expect_failure.
 
  - test_have_prereq SOME PREREQ
 
@@ -487,6 +488,40 @@ library for your script to use.
 		...
 	'
 
+Prerequisites
+-------------
+
+These are the prerequisites that the test library predefines with
+test_have_prereq.
+
+See the prereq argument to the test_* functions in the "Test harness
+library" section above and the "test_have_prereq" function for how to
+use these, and "test_set_prereq" for how to define your own.
+
+ - PERL & PYTHON
+
+   Git wasn't compiled with NO_PERL=YesPlease or
+   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
+   these.
+
+ - POSIXPERM
+
+   The filesystem supports POSIX style permission bits.
+
+ - BSLASHPSPEC
+
+   Backslashes in pathspec are not directory separators. This is not
+   set on Windows. See 6fd1106a for details.
+
+ - EXECKEEPSPID
+
+   The process retains the same pid across exec(2). See fb9a2bea for
+   details.
+
+ - SYMLINKS
+
+   The filesystem we're on supports symbolic links. E.g. a FAT
+   filesystem doesn't support these. See 704a3143 for details.
 
 Tips for Writing Tests
 ----------------------
-- 
1.7.1

Re: [PATCH 1/3] test-lib: Add support for multiple test prerequisites

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Change the test_have_prereq function in test-lib.sh to support a
> comma-separated list of prerequisites. This is useful for tests that
> need e.g. both POSIXPERM and SANITY.
> 
> The implementation was stolen from Junio C Hamano and Johannes Sixt,
> the tests and documentation were not.

I think you can sell it better. :)

	From: Johannes Sixt <j6t@kdbg.org>
	Subject: test-lib: Allow tests with multiple prerequisites

	Occasionally an especially taxing test comes around that
	makes multiple assumptions on the system running it.
	Currently the test suite works around that with such hacks as

	 if
		test_have_prereq SYMLINKS &&
		test_have_prereq POSIXPERM
	 then
		test_set_prereq SYMLINKPERM
	 fi
	 test_expect_success SYMLINKPERM 'funny symlink in work tree, un-unlink-able' '
		...
	 '

	or even worse, as in the test that example is based on, leaves
	out mention of the “easier” of the two assumptions.

	Maybe you would be happier to be able to write

	 test_expect_success SYMLINKS,POSIXPERM 'funny symlink' '
		...
	 '

	Based on a patch by Junio.

	[ab: added tests and documentation]

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

> +++ b/t/t0000-basic.sh
> @@ -73,6 +73,23 @@ then
>  	exit 1
>  fi
>  
> +test_set_prereq HAVETHIS
> +haveit=no
> +test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' '
> +    test_have_prereq HAVEIT &&
> +    test_have_prereq HAVETHIS &&

I think the similar code above was just a way to sneak in a sanity
check for test_have_prereq().  I’d leave it out.

> +    haveit=yes
> +'
> +donthaveit=yes
> +test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
> +    donthaveit=no
> +'
> +if test $haveit$donthaveit != yesyes
> +then
> +	say "bug in test framework: multiple prerequisite tags do not work reliably"
> +	exit 1
> +fi

Maybe it would be simpler to squash this in with the other similar checks.

> +++ b/t/test-lib.sh
> @@ -327,12 +327,20 @@ test_set_prereq () {
>  satisfied=" "
>  
>  test_have_prereq () {
> -	case $satisfied in
> -	*" $1 "*)
> -		: yes, have it ;;
> -	*)
> -		! : nope ;;
> -	esac
> +	# prerequisites can be concatenated with ','
> +	save_IFS=$IFS
> +	IFS=,
> +	set -- $*
> +	IFS=$save_IFS
> +	for prerequisite
> +	do
> +		case $satisfied in
> +		*" $prerequisite "*)
> +			: yes, have it ;;
> +		*)
> +			! : nope ;;
> +		esac
> +	done

Does that work?

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.
---
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2887677..cdb25af 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -58,35 +58,28 @@ test_expect_failure 'pretend we have fixed a known breakage' '
     :
 '
 test_set_prereq HAVEIT
+test_set_prereq ONE
+test_set_prereq TWO
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
     test_have_prereq HAVEIT &&
     haveit=yes
 '
+haveboth=no
+test_expect_success ONE,TWO 'test runs if prerequisites are satisfied' '
+    haveboth=yes
+'
 donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' '
     donthaveit=no
 '
-if test $haveit$donthaveit != yesyes
-then
-	say "bug in test framework: prerequisite tags do not work reliably"
-	exit 1
-fi
-
-test_set_prereq HAVETHIS
-haveit=no
-test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' '
-    test_have_prereq HAVEIT &&
-    test_have_prereq HAVETHIS &&
-    haveit=yes
-'
-donthaveit=yes
-test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
-    donthaveit=no
+donthaveboth=yes
+test_expect_success THREE,ONE 'one unmet prerequisite is enough' '
+    donthaveboth=no
 '
-if test $haveit$donthaveit != yesyes
+if test $haveit$donthaveit$haveboth$donthaveboth != yesyesyesyes
 then
-	say "bug in test framework: multiple prerequisite tags do not work reliably"
+	say "bug in test framework: prerequisite tags do not work reliably"
 	exit 1
 fi
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8701923..a3b0b22 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -338,7 +338,7 @@ test_have_prereq () {
 		*" $prerequisite "*)
 			: yes, have it ;;
 		*)
-			! : nope ;;
+			return 1
 		esac
 	done
 }
-- 

Re: [PATCH 1/3] test-lib: Add support for multiple test prerequisites

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -338,7 +338,7 @@ test_have_prereq () {
>  		*" $prerequisite "*)
>  			: yes, have it ;;
>  		*)
> -			! : nope ;;
> +			return 1

The ";;" is optional for the last arm in a case statement, but
it is probably simpler to include it.  Sorry about that.

(The only intended effect was only to use “return” that terminates and
propagates out of the loop instead of “false” that is overridden by a
later “true”.)

Re: [PATCH 1/3] test-lib: Add support for multiple test prerequisites

[Johannes Sixt]

On Sonntag, 8. August 2010, Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
> > Change the test_have_prereq function in test-lib.sh to support a
> > comma-separated list of prerequisites. This is useful for tests that
> > need e.g. both POSIXPERM and SANITY.
> >
> > The implementation was stolen from Junio C Hamano and Johannes Sixt,
> > the tests and documentation were not.
>
> I think you can sell it better. :)
>
> 	From: Johannes Sixt <j6t@kdbg.org>
> 	Subject: test-lib: Allow tests with multiple prerequisites

I think the patch has changed sufficiently, particularly in the subtle points, 
that authorship should not be attributed to me anymore.

Thanks for your review!

-- Hannes

Re: [PATCH 1/3] test-lib: Add support for multiple test prerequisites

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 8, 2010 at 01:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the test_have_prereq function in test-lib.sh to support a
>> comma-separated list of prerequisites. This is useful for tests that
>> need e.g. both POSIXPERM and SANITY.
>>
>> The implementation was stolen from Junio C Hamano and Johannes Sixt,
>> the tests and documentation were not.
>
> I think you can sell it better. :)

As Johannes points out the patch is different enough that I changed
the authorship. Actually I wouldn't modify any patch under someone
else's name without getting them to sign off on it.

>> +++ b/t/t0000-basic.sh
>> @@ -73,6 +73,23 @@ then
>>       exit 1
>>  fi
>>
>> +test_set_prereq HAVETHIS
>> +haveit=no
>> +test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' '
>> +    test_have_prereq HAVEIT &&
>> +    test_have_prereq HAVETHIS &&
>
> I think the similar code above was just a way to sneak in a sanity
> check for test_have_prereq().  I’d leave it out.

It's a sanity test. If that wasn't there the test might succeed
e.g. if the implementation was broken and only did a OR on the comma
delimited list, not a AND.

>> +    haveit=yes
>> +'
>> +donthaveit=yes
>> +test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
>> +    donthaveit=no
>> +'
>> +if test $haveit$donthaveit != yesyes
>> +then
>> +     say "bug in test framework: multiple prerequisite tags do not work reliably"
>> +     exit 1
>> +fi
>
> Maybe it would be simpler to squash this in with the other similar checks.

I think that's too much complexity for one test, especially in the
sanity file. I didn't squash it with the existing "yesyes" test
because we're testing basic functionality first (one prereq) then the
fancy stuff (multiple).

>> +++ b/t/test-lib.sh
>> @@ -327,12 +327,20 @@ test_set_prereq () {
>>  satisfied=" "
>>
>>  test_have_prereq () {
>> -     case $satisfied in
>> -     *" $1 "*)
>> -             : yes, have it ;;
>> -     *)
>> -             ! : nope ;;
>> -     esac
>> +     # prerequisites can be concatenated with ','
>> +     save_IFS=$IFS
>> +     IFS=,
>> +     set -- $*
>> +     IFS=$save_IFS
>> +     for prerequisite
>> +     do
>> +             case $satisfied in
>> +             *" $prerequisite "*)
>> +                     : yes, have it ;;
>> +             *)
>> +                     ! : nope ;;
>> +             esac
>> +     done
>
> Does that work?

It passes all the tests :)

> Except as noted above,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks.

Likewise.