Re: Tests in Cygwin

Don Slutz [off-list ref] writes:

A lot of these failures are do to running as root (member of
Administrators)....

Perhaps we can do this to reduce the noise level.

---
 t/README                    |   26 +++++++++++++++++++++++---
 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/t7300-clean.sh            |    2 +-
 t/test-lib.sh               |   22 +++++++++++++++-------
 4 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index d8f6c7d..a532c32 100644
--- a/t/README
+++ b/t/README

@@ -194,19 +194,28 @@ Test harness library
 There are a handful helper functions defined in the test harness
 library for your script to use.
 
- - test_expect_success <message> <script>
+ - test_expect_success [<prereq>] <message> <script>
 
-   This takes two strings as parameter, and evaluates the
+   This takes two or three strings as parameters, and evaluates the
    <script>.  If it yields success, test is considered
    successful.  <message> should state what it is testing.
 
+   An optional parameter <prereq> is a space separated list of prerequisite
+   tokens.  The test will be skipped if the test environment lacks any of
+   the prerequiste feature listed (see below for commonly used
+   prerequisites).
+
    Example:
 
 	test_expect_success \
 	    'git-write-tree should be able to write an empty tree.' \
 	    'tree=$(git-write-tree)'
 
- - test_expect_failure <message> <script>
+	test_expect_success SANITY '/ should not be writable' '
+		! test -w /
+	'
+
+ - test_expect_failure [<prereq>] <message> <script>
 
    This is NOT the opposite of test_expect_success, but is used
    to mark a test that demonstrates a known breakage.  Unlike

@@ -247,6 +256,17 @@ library for your script to use.
    Merges the given rev using the given message.  Like test_commit,
    creates a tag and calls test_tick before committing.
 
+Prerequisites
+-------------
+
+ POSIXPERM	The filesystem supports POSIX style permission bits
+ BSLASHPSPEC	???
+ EXECKEEPSPID	The process retains the same pid across exec(2)
+
+ SANITY		Test is not run by root user, and an attempt to an
+                unwritable file is expected to fail correctly.
+
+
 Tips for Writing Tests
 ----------------------
 

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index f19b4a2..eb60f80 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh

@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '
 
 '
 
-test_expect_success SYMLINKS 'funny symlink in work tree, un-unlink-able' '
+test_expect_success 'SYMLINKS SANITY' 'funny symlink in work tree, un-unlink-able' '
 
 	rm -fr a b &&
 	git reset --hard &&

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 929d5d4..7a5f680 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh

@@ -369,7 +369,7 @@ test_expect_success 'core.excludesfile' '
 
 '
 
-test_expect_success 'removal failure' '
+test_expect_success SANITY 'removal failure' '
 
 	mkdir foo &&
 	touch foo/bar &&

diff --git a/t/test-lib.sh b/t/test-lib.sh
index dad1437..1922e0b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -264,12 +264,16 @@ test_set_prereq () {
 satisfied=" "
 
 test_have_prereq () {
-	case $satisfied in
-	*" $1 "*)
-		: yes, have it ;;
-	*)
-		! : nope ;;
-	esac
+	for prerequisite
+	do
+		case $satisfied in
+		*" $prerequisite "*)
+			: yes, have it ;;
+		*)
+			return 1 ;;
+		esac
+	done
+	:
 }
 
 # You are not expected to call test_ok_ and test_failure_ directly, use

@@ -670,6 +674,10 @@ do
 	esac
 done
 
+# When tests are run as root (which is silly), permission
+# tests will report things are writable when they should not be.
+test -w / || test_set_prereq SANITY
+
 # Fix some commands on Windows
 case $(uname -s) in
 *MINGW*)

@@ -692,7 +700,7 @@ case $(uname -s) in
 	# exec does not inherit the PID
 	;;
 *)
-	test_set_prereq POSIXPERM
+	test_have_prereq SANITY && test_set_prereq POSIXPERM
 	test_set_prereq BSLASHPSPEC
 	test_set_prereq EXECKEEPSPID
 	;;

Junio C Hamano schrieb:

Don Slutz [off-list ref] writes:

quoted

A lot of these failures are do to running as root (member of
Administrators)....

Perhaps we can do this to reduce the noise level.

Thank you for this patch. Unfortunately, it does not work as intended
because the call to test_have_prereq in test_skip does not split
the prereq tokens so that tests with space-separated prerequisite tokens
are always skipped.

My first attempt to solve that is to remove the quotes from the
test_have_prereq call in test_skip; but I think that this simple
solution is rather unintuitive, and will look like a bug if you inspect
the code again in two months.

You could squash in the patch below, where I chose to concatenate
tokens with '+' because I dislike the extra quoting that is otherwise
necessary at the call site. The downside is that it plays games with
IFS, which you might find too ugly and not worth it. It's your call.

BTW, I didn't dare test this as root on my production system; I'll do
that later today elsewhere.

diff --git a/t/README b/t/README
index a532c32..49c3a51 100644
--- a/t/README
+++ b/t/README

@@ -200,7 +200,7 @@ library for your script to use.
    <script>.  If it yields success, test is considered
    successful.  <message> should state what it is testing.

-   An optional parameter <prereq> is a space separated list of prerequisite
+   An optional parameter <prereq> is a '+' separated list of prerequisite
    tokens.  The test will be skipped if the test environment lacks any of
    the prerequiste feature listed (see below for commonly used
    prerequisites).

@@ -260,7 +260,8 @@ Prerequisites
 -------------

  POSIXPERM	The filesystem supports POSIX style permission bits
- BSLASHPSPEC	???
+ BSLASHPSPEC	Backslashes in pathspec are not directory separators
+                (i.e. they are not converted to forward-slash)
  EXECKEEPSPID	The process retains the same pid across exec(2)

  SANITY		Test is not run by root user, and an attempt to an

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index eb60f80..8683189 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh

@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '

 '

-test_expect_success 'SYMLINKS SANITY' 'funny symlink in work tree, un-unlink-able' '
+test_expect_success SYMLINKS+SANITY 'funny symlink in work tree, un-unlink-able' '

 	rm -fr a b &&
 	git reset --hard &&

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1922e0b..2d12799 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -263,7 +263,12 @@ test_set_prereq () {
 }
 satisfied=" "

+# prerequisites can be concatenated with '+'
 test_have_prereq () {
+	save_IFS=$IFS
+	IFS=+
+	set -- $*
+	IFS=$save_IFS
 	for prerequisite
 	do
 		case $satisfied in

Here is a change back to using spaces.  It also includes a change
to test for the code working.  Squash on top.

Signed-off-by: Don Slutz <redacted>
---
 t/README                    |    4 ++--
 t/t0000-basic.sh            |   13 ++++++++++++-
 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/test-lib.sh               |    7 +------
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/t/README b/t/README
index 49c3a51..c61e1d4 100644
--- a/t/README
+++ b/t/README

@@ -200,9 +200,9 @@ library for your script to use.
    <script>.  If it yields success, test is considered
    successful.  <message> should state what it is testing.
 
-   An optional parameter <prereq> is a '+' separated list of prerequisite
+   An optional parameter <prereq> is a space separated list of prerequisite
    tokens.  The test will be skipped if the test environment lacks any of
-   the prerequiste feature listed (see below for commonly used
+   the prerequisite feature listed (see below for commonly used
    prerequisites).
 
    Example:

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f4ca4fc..58bac69 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -67,7 +67,18 @@ donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be 

skipped' '
     donthaveit=no
 '
-if test $haveit$donthaveit != yesyes
+test_set_prereq HAVEBOTH
+haveboth1=no
+test_expect_success 'HAVEBOTH HAVEIT' 'test runs if both prerequisites 
are satisfied' '
+    test_have_prereq "HAVEIT HAVEBOTH" &&
+    haveboth1=yes
+'
+haveboth2=no
+test_expect_success 'HAVEIT HAVEBOTH' 'test passes if both 
prerequisites are satisfied' '
+    test_have_prereq "HAVEBOTH HAVEIT" &&
+    haveboth2=yes
+'
+if test $haveit$donthaveit$haveboth1$haveboth2 != yesyesyesyes
 then
        say "bug in test framework: prerequisite tags do not work reliably"
        exit 1

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index 8683189..eb60f80 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh

@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work 

tree' '
 
 '
 
-test_expect_success SYMLINKS+SANITY 'funny symlink in work tree, 
un-unlink-able' '
+test_expect_success 'SYMLINKS SANITY' 'funny symlink in work tree, 
un-unlink-able' '
 
        rm -fr a b &&
        git reset --hard &&

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2d12799..dd3b8d7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -263,13 +263,8 @@ test_set_prereq () {
 }
 satisfied=" "
 
-# prerequisites can be concatenated with '+'
 test_have_prereq () {
-       save_IFS=$IFS
-       IFS=+
-       set -- $*
-       IFS=$save_IFS
-       for prerequisite
+       for prerequisite in $(echo $*)
        do
                case $satisfied in
                *" $prerequisite "*)

-- 
1.5.4.2


-------- Original Message --------
Subject: Re: Tests in Cygwin
From: Johannes Sixt <j.sixt@viscovery.net>
To: Junio C Hamano <gitster@pobox.com>
CC: Don Slutz <Don.Slutz@SierraAtlantic.com>, Hannu Koivisto 
<azure@iki.fi>, git@vger.kernel.org
Date: 5/7/2009 3:13 AM
> Junio C Hamano schrieb:
>   
>> Don Slutz <Don.Slutz@SierraAtlantic.com> writes:
>>
>>     
>>> A lot of these failures are do to running as root (member of
>>> Administrators)....
>>>       
>> Perhaps we can do this to reduce the noise level.
>>     
>
> Thank you for this patch. Unfortunately, it does not work as intended
> because the call to test_have_prereq in test_skip does not split
> the prereq tokens so that tests with space-separated prerequisite tokens
> are always skipped.
>
> My first attempt to solve that is to remove the quotes from the
> test_have_prereq call in test_skip; but I think that this simple
> solution is rather unintuitive, and will look like a bug if you inspect
> the code again in two months.
>
> You could squash in the patch below, where I chose to concatenate
> tokens with '+' because I dislike the extra quoting that is otherwise
> necessary at the call site. The downside is that it plays games with
> IFS, which you might find too ugly and not worth it. It's your call.
>
> BTW, I didn't dare test this as root on my production system; I'll do
> that later today elsewhere.
>
>
> diff --git a/t/README b/t/README
> index a532c32..49c3a51 100644
> --- a/t/README
> +++ b/t/README
> @@ -200,7 +200,7 @@ library for your script to use.
>     <script>.  If it yields success, test is considered
>     successful.  <message> should state what it is testing.
>
> -   An optional parameter <prereq> is a space separated list of prerequisite
> +   An optional parameter <prereq> is a '+' separated list of prerequisite
>     tokens.  The test will be skipped if the test environment lacks any of
>     the prerequiste feature listed (see below for commonly used
>     prerequisites).
> @@ -260,7 +260,8 @@ Prerequisites
>  -------------
>
>   POSIXPERM	The filesystem supports POSIX style permission bits
> - BSLASHPSPEC	???
> + BSLASHPSPEC	Backslashes in pathspec are not directory separators
> +                (i.e. they are not converted to forward-slash)
>   EXECKEEPSPID	The process retains the same pid across exec(2)
>
>   SANITY		Test is not run by root user, and an attempt to an
> diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
> index eb60f80..8683189 100755
> --- a/t/t1004-read-tree-m-u-wf.sh
> +++ b/t/t1004-read-tree-m-u-wf.sh
> @@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '
>
>  '
>
> -test_expect_success 'SYMLINKS SANITY' 'funny symlink in work tree, un-unlink-able' '
> +test_expect_success SYMLINKS+SANITY 'funny symlink in work tree, un-unlink-able' '
>
>  	rm -fr a b &&
>  	git reset --hard &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1922e0b..2d12799 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -263,7 +263,12 @@ test_set_prereq () {
>  }
>  satisfied=" "
>
> +# prerequisites can be concatenated with '+'
>  test_have_prereq () {
> +	save_IFS=$IFS
> +	IFS=+
> +	set -- $*
> +	IFS=$save_IFS
>  	for prerequisite
>  	do
>  		case $satisfied in
>
>   



__________________________________________________________________________________________________________________
DISCLAIMER:"The information contained in this message and the attachments (if any) may be privileged and confidential and protected from disclosure. You are hereby notified that any unauthorized use, dissemination, distribution or copying of this communication, review, retransmission, or taking of any action based upon this information, by persons or entities other than the intended recipient, is strictly prohibited. If you are not the intended recipient or an employee or agent responsible for delivering this message, and have received this communication in error, please notify us immediately by replying to the message and kindly delete the original message, attachments, if any, and all its copies from your computer system. Thank you for your cooperation." 
________________________________________________________________________________________________________________

Before these patches, I get:

make aggregate-results
make[3]: Entering directory `/home/adslutz/BinMode/git/t'
'/bin/sh' ./aggregate-results.sh test-results/t*-*
fixed   3
success 4201
failed  15
broken  5
total   4272
make[3]: Leaving directory `/home/adslutz/BinMode/git/t'

After:

make aggregate-results
make[2]: Entering directory `/home/adslutz/BinMode/git/t'
'/bin/sh' ./aggregate-results.sh test-results/t*-*
fixed   3
success 4181
failed  6
broken  5
total   4274
make[2]: Leaving directory `/home/adslutz/BinMode/git/t'

   -Don

-------- Original Message --------
Subject: Re: Tests in Cygwin
From: Don Slutz <redacted>
To: Johannes Sixt <redacted>
CC: Junio C Hamano <redacted>, Hannu Koivisto <redacted>, 
git@vger.kernel.org
Date: 5/7/2009 11:47 AM

__________________________________________________________________________________________________________________
DISCLAIMER:"The information contained in this message and the attachments (if any) may be privileged and confidential and protected from disclosure. You are hereby notified that any unauthorized use, dissemination, distribution or copying of this communication, review, retransmission, or taking of any action based upon this information, by persons or entities other than the intended recipient, is strictly prohibited. If you are not the intended recipient or an employee or agent responsible for delivering this message, and have received this communication in error, please notify us immediately by replying to the message and kindly delete the original message, attachments, if any, and all its copies from your computer system. Thank you for your cooperation." 
________________________________________________________________________________________________________________

On Thu, May 07, 2009 at 11:47:03AM -0400, Don Slutz wrote:

Here is a change back to using spaces.  It also includes a change
to test for the code working.  Squash on top.

Maybe it is just me, but I think

  test_expect_success SYMLINKS,SANITY 'some description' '
    the actual test
  '

is more readable than the space (since it keeps the number of single
quoted chunks on the line down). Which should be as simple as:

-# prerequisites can be concatenated with '+'
test_have_prereq () {
-       save_IFS=$IFS
-       IFS=+
-       set -- $*
-       IFS=$save_IFS
-       for prerequisite
+       for prerequisite in $(echo $*)
       do
               case $satisfied in
               *" $prerequisite "*)

  +for prerequisite in $(echo "$1" | tr , ' ')

Just my bikeshedding two cents,

-Peff

Jeff King schrieb:

On Thu, May 07, 2009 at 11:47:03AM -0400, Don Slutz wrote:

quoted

Here is a change back to using spaces.  It also includes a change
to test for the code working.  Squash on top.

Maybe it is just me, but I think

  test_expect_success SYMLINKS,SANITY 'some description' '
    the actual test
  '

is more readable than the space (since it keeps the number of single
quoted chunks on the line down). Which should be as simple as:

quoted

-# prerequisites can be concatenated with '+'
test_have_prereq () {
-       save_IFS=$IFS
-       IFS=+
-       set -- $*
-       IFS=$save_IFS
-       for prerequisite
+       for prerequisite in $(echo $*)
       do
               case $satisfied in
               *" $prerequisite "*)

  +for prerequisite in $(echo "$1" | tr , ' ')

So, you dislike the space separator, but you also dislike the IFS games
that save a few new processes? ;) (Think of Windows, where fork is expensive).

-- Hannes