[PATCH] tests: Use skip_all=* to skip tests

[PATCH] tests: Use skip_all=* to skip tests

[Ævar Arnfjörð Bjarmason]

Change tests to skip with skip_all=* + test_done instead of using say
+ test_done.

This is a follow-up to "tests: Skip tests in a way that makes sense
under TAP" (fadb5156e4). I missed these cases when prepearing that
patch, hopefully this is all of them.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/gitweb-lib.sh           |    4 ++--
 t/lib-cvs.sh              |    6 +++---
 t/lib-git-svn.sh          |   11 +++++------
 t/lib-httpd.sh            |    8 ++++----
 t/lib-patch-mode.sh       |    2 +-
 t/t5800-remote-helpers.sh |    2 +-
 t/t7005-editor.sh         |    2 +-
 7 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index b70b891..81ef2a0 100644
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -76,12 +76,12 @@ gitweb_run () {
 . ./test-lib.sh
 
 if ! test_have_prereq PERL; then
-	say 'skipping gitweb tests, perl not available'
+	skip_all='skipping gitweb tests, perl not available'
 	test_done
 fi
 
 perl -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
-    say 'skipping gitweb tests, perl version is too old'
+    skip_all='skipping gitweb tests, perl version is too old'
     test_done
 }
 
diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 4b3b793..648d161 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -9,7 +9,7 @@ export HOME
 
 if ! type cvs >/dev/null 2>&1
 then
-	say 'skipping cvsimport tests, cvs not found'
+	skip_all='skipping cvsimport tests, cvs not found'
 	test_done
 fi
 
@@ -21,11 +21,11 @@ case "$cvsps_version" in
 2.1 | 2.2*)
 	;;
 '')
-	say 'skipping cvsimport tests, cvsps not found'
+	skip_all='skipping cvsimport tests, cvsps not found'
 	test_done
 	;;
 *)
-	say 'skipping cvsimport tests, unsupported cvsps version'
+	skip_all='skipping cvsimport tests, unsupported cvsps version'
 	test_done
 	;;
 esac
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 344785d..f62f617 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -21,7 +21,7 @@ PERL=${PERL:-perl}
 svn >/dev/null 2>&1
 if test $? -ne 1
 then
-    say 'skipping git svn tests, svn not found'
+    skip_all='skipping git svn tests, svn not found'
     test_done
 fi
 
@@ -40,13 +40,12 @@ x=$?
 if test $x -ne 0
 then
 	if test $x -eq 42; then
-		err='Perl SVN libraries must be >= 1.1.0'
+		skip_all='Perl SVN libraries must be >= 1.1.0'
 	elif test $x -eq 41; then
-		err='svnadmin failed to create fsfs repository'
+		skip_all='svnadmin failed to create fsfs repository'
 	else
-		err='Perl SVN libraries not found or unusable, skipping test'
+		skip_all='Perl SVN libraries not found or unusable'
 	fi
-	say "$err"
 	test_done
 fi
 
@@ -159,7 +158,7 @@ EOF
 require_svnserve () {
     if test -z "$SVNSERVE_PORT"
     then
-        say 'skipping svnserve test. (set $SVNSERVE_PORT to enable)'
+        skip_all='skipping svnserve test. (set $SVNSERVE_PORT to enable)'
         test_done
     fi
 }
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a0944d6..71effc5 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -45,7 +45,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
 
 if ! test -x "$LIB_HTTPD_PATH"
 then
-	say "skipping test, no web server found at '$LIB_HTTPD_PATH'"
+	skip_all="skipping test, no web server found at '$LIB_HTTPD_PATH'"
 	test_done
 fi
 
@@ -58,12 +58,12 @@ then
 	then
 		if ! test $HTTPD_VERSION -ge 2
 		then
-			say "skipping test, at least Apache version 2 is required"
+			skip_all="skipping test, at least Apache version 2 is required"
 			test_done
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
-			say "Apache module directory not found.  Skipping tests."
+			skip_all="Apache module directory not found.  Skipping tests."
 			test_done
 		fi
 
@@ -118,7 +118,7 @@ start_httpd() {
 		>&3 2>&4
 	if test $? -ne 0
 	then
-		say "skipping test, web server setup failed"
+		skip_all="skipping test, web server setup failed"
 		trap 'die' EXIT
 		test_done
 	fi
diff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh
index ce36f34..375e248 100644
--- a/t/lib-patch-mode.sh
+++ b/t/lib-patch-mode.sh
@@ -3,7 +3,7 @@
 . ./test-lib.sh
 
 if ! test_have_prereq PERL; then
-	say 'skipping --patch tests, perl not available'
+	skip_all='skipping --patch tests, perl not available'
 	test_done
 fi
 
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 4ee7b65..637d8e9 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -15,7 +15,7 @@ if sys.hexversion < 0x02040000:
 then
 	:
 else
-	say 'skipping git remote-testgit tests: requires Python 2.4 or newer'
+	skip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'
 	test_done
 fi
 
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index fe60d69..26ddf9d 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -113,7 +113,7 @@ done
 
 if ! echo 'echo space > "$1"' > "e space.sh"
 then
-	say "Skipping; FS does not support spaces in filenames"
+	skip_all="Skipping; FS does not support spaces in filenames"
 	test_done
 fi
 
-- 
1.7.0.4

Re: [PATCH] tests: Use skip_all=* to skip tests

[Michael J Gruber]

After trying out tests with prove (I like it!) I was just about to make
a patch before I saw this...

So, this does what it should, at least with my set of prerequisites.

Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:
> Change tests to skip with skip_all=* + test_done instead of using say
> + test_done.

I didn't understand this at all at first, only after I was about to
write that patch myself. Maybe 'with skip_all="reason"' or
'skip_all=<reason>' etc.?

> 
> This is a follow-up to "tests: Skip tests in a way that makes sense
> under TAP" (fadb5156e4). I missed these cases when prepearing that

"preparing", although that last sentence sounds more like "after ---
material".

> patch, hopefully this is all of them.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

No comments to the patch itself, it works :)

Michael

[PATCH v2] tests: Use skip_all=<reason> to skip tests

[Ævar Arnfjörð Bjarmason]

Change tests to skip with skip_all=<reason> + test_done, instead of
using say <reason> + test_done.

This is a follow-up to "tests: Skip tests in a way that makes sense
under TAP" (fadb5156e4). I missed these cases when preparing that
patch, hopefully this is all of them.

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

On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> After trying out tests with prove (I like it!) I was just about to make
> a patch before I saw this...

Great, good to know that you find the TAP support useful.

> So, this does what it should, at least with my set of prerequisites.
>
> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:
>> Change tests to skip with skip_all=* + test_done instead of using say
>> + test_done.
>
> I didn't understand this at all at first, only after I was about to
> write that patch myself. Maybe 'with skip_all="reason"' or
> 'skip_all=<reason>' etc.?

Changed the subject + body of the patch to use <reason>

>> This is a follow-up to "tests: Skip tests in a way that makes sense
>> under TAP" (fadb5156e4). I missed these cases when prepearing that
>
> "preparing", although that last sentence sounds more like "after ---
> material".

Fixed the spelling error. But I think it'd be useful to have this when
browsing the history of t/ in the future, so I kept the note in the
patch, not just in the E-Mail.

>> patch, hopefully this is all of them.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> No comments to the patch itself, it works :)


 t/gitweb-lib.sh           |    4 ++--
 t/lib-cvs.sh              |    6 +++---
 t/lib-git-svn.sh          |   11 +++++------
 t/lib-httpd.sh            |    8 ++++----
 t/lib-patch-mode.sh       |    2 +-
 t/t5800-remote-helpers.sh |    2 +-
 t/t7005-editor.sh         |    2 +-
 7 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index b70b891..81ef2a0 100644
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -76,12 +76,12 @@ gitweb_run () {
 . ./test-lib.sh
 
 if ! test_have_prereq PERL; then
-	say 'skipping gitweb tests, perl not available'
+	skip_all='skipping gitweb tests, perl not available'
 	test_done
 fi
 
 perl -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
-    say 'skipping gitweb tests, perl version is too old'
+    skip_all='skipping gitweb tests, perl version is too old'
     test_done
 }
 
diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 4b3b793..648d161 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -9,7 +9,7 @@ export HOME
 
 if ! type cvs >/dev/null 2>&1
 then
-	say 'skipping cvsimport tests, cvs not found'
+	skip_all='skipping cvsimport tests, cvs not found'
 	test_done
 fi
 
@@ -21,11 +21,11 @@ case "$cvsps_version" in
 2.1 | 2.2*)
 	;;
 '')
-	say 'skipping cvsimport tests, cvsps not found'
+	skip_all='skipping cvsimport tests, cvsps not found'
 	test_done
 	;;
 *)
-	say 'skipping cvsimport tests, unsupported cvsps version'
+	skip_all='skipping cvsimport tests, unsupported cvsps version'
 	test_done
 	;;
 esac
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 344785d..f62f617 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -21,7 +21,7 @@ PERL=${PERL:-perl}
 svn >/dev/null 2>&1
 if test $? -ne 1
 then
-    say 'skipping git svn tests, svn not found'
+    skip_all='skipping git svn tests, svn not found'
     test_done
 fi
 
@@ -40,13 +40,12 @@ x=$?
 if test $x -ne 0
 then
 	if test $x -eq 42; then
-		err='Perl SVN libraries must be >= 1.1.0'
+		skip_all='Perl SVN libraries must be >= 1.1.0'
 	elif test $x -eq 41; then
-		err='svnadmin failed to create fsfs repository'
+		skip_all='svnadmin failed to create fsfs repository'
 	else
-		err='Perl SVN libraries not found or unusable, skipping test'
+		skip_all='Perl SVN libraries not found or unusable'
 	fi
-	say "$err"
 	test_done
 fi
 
@@ -159,7 +158,7 @@ EOF
 require_svnserve () {
     if test -z "$SVNSERVE_PORT"
     then
-        say 'skipping svnserve test. (set $SVNSERVE_PORT to enable)'
+        skip_all='skipping svnserve test. (set $SVNSERVE_PORT to enable)'
         test_done
     fi
 }
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a0944d6..71effc5 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -45,7 +45,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
 
 if ! test -x "$LIB_HTTPD_PATH"
 then
-	say "skipping test, no web server found at '$LIB_HTTPD_PATH'"
+	skip_all="skipping test, no web server found at '$LIB_HTTPD_PATH'"
 	test_done
 fi
 
@@ -58,12 +58,12 @@ then
 	then
 		if ! test $HTTPD_VERSION -ge 2
 		then
-			say "skipping test, at least Apache version 2 is required"
+			skip_all="skipping test, at least Apache version 2 is required"
 			test_done
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
-			say "Apache module directory not found.  Skipping tests."
+			skip_all="Apache module directory not found.  Skipping tests."
 			test_done
 		fi
 
@@ -118,7 +118,7 @@ start_httpd() {
 		>&3 2>&4
 	if test $? -ne 0
 	then
-		say "skipping test, web server setup failed"
+		skip_all="skipping test, web server setup failed"
 		trap 'die' EXIT
 		test_done
 	fi
diff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh
index ce36f34..375e248 100644
--- a/t/lib-patch-mode.sh
+++ b/t/lib-patch-mode.sh
@@ -3,7 +3,7 @@
 . ./test-lib.sh
 
 if ! test_have_prereq PERL; then
-	say 'skipping --patch tests, perl not available'
+	skip_all='skipping --patch tests, perl not available'
 	test_done
 fi
 
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 4ee7b65..637d8e9 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -15,7 +15,7 @@ if sys.hexversion < 0x02040000:
 then
 	:
 else
-	say 'skipping git remote-testgit tests: requires Python 2.4 or newer'
+	skip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'
 	test_done
 fi
 
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index fe60d69..26ddf9d 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -113,7 +113,7 @@ done
 
 if ! echo 'echo space > "$1"' > "e space.sh"
 then
-	say "Skipping; FS does not support spaces in filenames"
+	skip_all="Skipping; FS does not support spaces in filenames"
 	test_done
 fi
 
-- 
1.7.0.4

Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change tests to skip with skip_all=<reason> + test_done, instead of
> using say <reason> + test_done.
>
> This is a follow-up to "tests: Skip tests in a way that makes sense
> under TAP" (fadb5156e4). I missed these cases when preparing that
> patch, hopefully this is all of them.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>> After trying out tests with prove (I like it!) I was just about to make
>> a patch before I saw this...
>
> Great, good to know that you find the TAP support useful.
>
>> So, this does what it should, at least with my set of prerequisites.
>>
>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:
>>> Change tests to skip with skip_all=* + test_done instead of using say
>>> + test_done.
>>
>> I didn't understand this at all at first, only after I was about to
>> write that patch myself. Maybe 'with skip_all="reason"' or
>> 'skip_all=<reason>' etc.?
>
> Changed the subject + body of the patch to use <reason>

That makes sense, but I am beginning to hate this skip_all business.

There is no "skip"-ness to what the variable does from the semantic point
of view.  In the context of test helper library whose service consists of
counting the number of tests that succeeded, failed, and are known to be
still broken, one would expect that the word "skip" would mean it somehow
would help counting the tests that are prepared by the test author but
were not run for some reason, but this variable is not about that (note
that I am not suggesting us to actually count and say "N tests skipped").
It is only about giving parting words at the end before the test script
exits.

The variable is not about skip_ALL either.  In the majority of cases, it
is more like "finishing here, telling the user that we are doing so
without running the remainder of the script", and in one case, it is more
like "skipped one test, reporting after the fact".

Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the
only ones that are not immediately followed by test_done are in
lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap
before calling test_done), and t3600-rm (makes a mental note to report
that one test was skipped long before all the tests run).

I suspect that it might be much easier in the long run for test writers if
we made test_done take optional "parting words" parameter instead of using
a global variable "$skip_all" and forcing them to carefully set it.  Then
we can lose special meaning to the global variable $skip_all, most of the
scripts that currently assign to the variable do not need the assignments,
and only the very few special cases can use $skip_all as their local
convention to decide what optional "parting words" parameter to give to
the test_done helper.

The change would look something like this (I just did a few as a
demonstration).

What do you think?

 t/lib-git-svn.sh            |   11 ++++-------
 t/t2007-checkout-symlink.sh |    3 +--
 t/test-lib.sh               |    5 +----
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index c3f6676..cfc0d5b 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -5,12 +5,10 @@ git_svn_id=git""-svn-id
 
 if test -n "$NO_SVN_TESTS"
 then
-	skip_all='skipping git svn tests, NO_SVN_TESTS defined'
-	test_done
+	test_done 'skipping git svn tests, NO_SVN_TESTS defined'
 fi
 if ! test_have_prereq PERL; then
-	skip_all='skipping git svn tests, perl not available'
-	test_done
+	test_done 'skipping git svn tests, perl not available'
 fi
 
 GIT_DIR=$PWD/.git
@@ -21,8 +19,7 @@ PERL=${PERL:-perl}
 svn >/dev/null 2>&1
 if test $? -ne 1
 then
-    skip_all='skipping git svn tests, svn not found'
-    test_done
+    test_done 'skipping git svn tests, svn not found'
 fi
 
 svnrepo=$PWD/svnrepo
@@ -46,7 +43,7 @@ then
 	else
 		skip_all='Perl SVN libraries not found or unusable'
 	fi
-	test_done
+	test_done "$skip_all"
 fi
 
 rawsvnrepo="$svnrepo"
diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
index 05cc8fd..fd3d7be 100755
--- a/t/t2007-checkout-symlink.sh
+++ b/t/t2007-checkout-symlink.sh
@@ -8,8 +8,7 @@ test_description='git checkout to switch between branches with symlink<->dir'
 
 if ! test_have_prereq SYMLINKS
 then
-	skip_all="symbolic links not supported - skipping tests"
-	test_done
+	test_done "symbolic links not supported - skipping tests"
 fi
 
 test_expect_success setup '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ac496aa..2c4474d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -657,12 +657,9 @@ test_done () {
 	fi
 	case "$test_failure" in
 	0)
-		# Maybe print SKIP message
-		[ -z "$skip_all" ] || skip_all=" # SKIP $skip_all"
-
 		if test $test_external_has_tap -eq 0; then
 			say_color pass "# passed all $msg"
-			say "1..$test_count$skip_all"
+			say "1..$test_count${1:+" # SKIP $1"}"
 		fi
 
 		test -d "$remove_trash" &&

Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change tests to skip with skip_all=<reason> + test_done, instead of
>> using say <reason> + test_done.
>>
>> This is a follow-up to "tests: Skip tests in a way that makes sense
>> under TAP" (fadb5156e4). I missed these cases when preparing that
>> patch, hopefully this is all of them.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>>> After trying out tests with prove (I like it!) I was just about to make
>>> a patch before I saw this...
>>
>> Great, good to know that you find the TAP support useful.
>>
>>> So, this does what it should, at least with my set of prerequisites.
>>>
>>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:
>>>> Change tests to skip with skip_all=* + test_done instead of using say
>>>> + test_done.
>>>
>>> I didn't understand this at all at first, only after I was about to
>>> write that patch myself. Maybe 'with skip_all="reason"' or
>>> 'skip_all=<reason>' etc.?
>>
>> Changed the subject + body of the patch to use <reason>
>
> That makes sense, but I am beginning to hate this skip_all business.

I'm not sure I like the interface either. It's just something that did
the job. But I do have some grand plans to make the test-lib.sh more
TAP-ish, including adding more advanced skip features.

> There is no "skip"-ness to what the variable does from the semantic point
> of view.  In the context of test helper library whose service consists of
> counting the number of tests that succeeded, failed, and are known to be
> still broken, one would expect that the word "skip" would mean it somehow
> would help counting the tests that are prepared by the test author but
> were not run for some reason, but this variable is not about that (note
> that I am not suggesting us to actually count and say "N tests skipped").
> It is only about giving parting words at the end before the test script
> exits.

The reason this variable exists is that there is a semantic
difference. The test-results/* aggregator that ships with Git doesn't
care about whether a passing test was a TODO test, or record the
reasons for why something was skipped. But TAP harnesses do record and
report this.

The idea is that we'll be able to set up smoke testing machines for
git, and they'll be able to report passing/failed/TODO passing/TODO
failing/skipped (why?) tests.

> The variable is not about skip_ALL either.  In the majority of cases, it
> is more like "finishing here, telling the user that we are doing so
> without running the remainder of the script", and in one case, it is more
> like "skipped one test, reporting after the fact".

Curiously enough, I initially called it "skip_all_remaining=<reason>",
but thought it was too verbose. That's what it does now, anyway. Skips
all remaining tests.

> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the
> only ones that are not immediately followed by test_done are in
> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap
> before calling test_done), and t3600-rm (makes a mental note to report
> that one test was skipped long before all the tests run).
>
> I suspect that it might be much easier in the long run for test writers if
> we made test_done take optional "parting words" parameter instead of using
> a global variable "$skip_all" and forcing them to carefully set it.

Having a magic string like you suggest is overloading the API a
bit. But my opinion differs from yours on this I suspect because I'm
trying to convey a semantic difference. I.e. the skip message isn't
just a custom "bye" message.

Rather than this:

    test_done 'skipping git svn tests, perl not available'

Which doesn't indicate that the "skipping" part is magic. Perhaps this
is better:

    test_done "bye bye now"
    test_done "skip_remaining" "can't test SVN here"

Or something. Anyway, personally I think that's overloading the API a
bit. I experimented with doing that before devising skip_all. And
decided to make test_done just work like Test::More's done_testing()
instead.

> Then we can lose special meaning to the global variable $skip_all,
> most of the scripts that currently assign to the variable do not
> need the assignments, and only the very few special cases can use
> $skip_all as their local convention to decide what optional "parting
> words" parameter to give to the test_done helper.

> The change would look something like this (I just did a few as a
> demonstration).
>
> What do you think?

That should work. Personally I don't really care what the API ends up
looking like (beyond the slight grumpiness associated with having to
amend existing code).

I just care that it's clear & concise. Which admittedly the current
skip_all=<reason> & test_done combo isn't a good example of.

Anyway. I suppose now is as good a time as any to share some of my
evil plans.

When I submitted the TAP series I really just did the bare minimum to
get things working. But there's a lot more that could be done.

For example, in the test case attached to my gettext patch I skip
tests on platforms that don't have the prerequisite locale files. But
there's no indication to the user that something is amiss.

I was planning to add support for skipping tests in the middle of the
test files, so you could do this:

    $ perl -MTest::More -E 'plan tests => 4; pass "gettext stuff ok"
for 1..2; SKIP: { skip "Can not test without locale files", 2 }'
    1..4
    ok 1 - gettext stuff ok
    ok 2 - gettext stuff ok
    ok 3 # skip Can not test without locale files
    ok 4 # skip Can not test without locale files

And a TAP harness could subsequently report on how many tests were
skipped, and we might e.g. see that HP-UX is really unloved because
we're skipping 500/6000 tests or something.

This also allows for skipping tests in the middle of the file for some
reason:

    $ perl -MTest::More -E 'plan tests => 6; pass "gettext stuff ok"
for 1..2; SKIP: { skip "Can not test without locale files", 2 } pass
"moo" for 1..2'
    1..6
    ok 1 - gettext stuff ok
    ok 2 - gettext stuff ok
    ok 3 # skip Can not test without locale files
    ok 4 # skip Can not test without locale files
    ok 5 - moo
    ok 6 - moo

You'll also notice that I planned the number of tests /before/ I
started running them. We don't do this now, but some tests can really
benefit from that (although I haven't spotted anything like that in
Git). At worst you'll get accurate test progress meters when running
prove.

</braindump>

>  t/lib-git-svn.sh            |   11 ++++-------
>  t/t2007-checkout-symlink.sh |    3 +--
>  t/test-lib.sh               |    5 +----
>  3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
> index c3f6676..cfc0d5b 100644
> --- a/t/lib-git-svn.sh
> +++ b/t/lib-git-svn.sh
> @@ -5,12 +5,10 @@ git_svn_id=git""-svn-id
>
>  if test -n "$NO_SVN_TESTS"
>  then
> -       skip_all='skipping git svn tests, NO_SVN_TESTS defined'
> -       test_done
> +       test_done 'skipping git svn tests, NO_SVN_TESTS defined'
>  fi
>  if ! test_have_prereq PERL; then
> -       skip_all='skipping git svn tests, perl not available'
> -       test_done
> +       test_done 'skipping git svn tests, perl not available'
>  fi
>
>  GIT_DIR=$PWD/.git
> @@ -21,8 +19,7 @@ PERL=${PERL:-perl}
>  svn >/dev/null 2>&1
>  if test $? -ne 1
>  then
> -    skip_all='skipping git svn tests, svn not found'
> -    test_done
> +    test_done 'skipping git svn tests, svn not found'
>  fi
>
>  svnrepo=$PWD/svnrepo
> @@ -46,7 +43,7 @@ then
>        else
>                skip_all='Perl SVN libraries not found or unusable'
>        fi
> -       test_done
> +       test_done "$skip_all"
>  fi
>
>  rawsvnrepo="$svnrepo"
> diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
> index 05cc8fd..fd3d7be 100755
> --- a/t/t2007-checkout-symlink.sh
> +++ b/t/t2007-checkout-symlink.sh
> @@ -8,8 +8,7 @@ test_description='git checkout to switch between branches with symlink<->dir'
>
>  if ! test_have_prereq SYMLINKS
>  then
> -       skip_all="symbolic links not supported - skipping tests"
> -       test_done
> +       test_done "symbolic links not supported - skipping tests"
>  fi
>
>  test_expect_success setup '
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ac496aa..2c4474d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -657,12 +657,9 @@ test_done () {
>        fi
>        case "$test_failure" in
>        0)
> -               # Maybe print SKIP message
> -               [ -z "$skip_all" ] || skip_all=" # SKIP $skip_all"
> -
>                if test $test_external_has_tap -eq 0; then
>                        say_color pass "# passed all $msg"
> -                       say "1..$test_count$skip_all"
> +                       say "1..$test_count${1:+" # SKIP $1"}"
>                fi
>
>                test -d "$remove_trash" &&
>

Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the
>> only ones that are not immediately followed by test_done are in
>> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap
>> before calling test_done), and t3600-rm (makes a mental note to report
>> that one test was skipped long before all the tests run).
> ...
>     $ perl -MTest::More -E 'plan tests => 6; pass "gettext stuff ok"
> for 1..2; SKIP: { skip "Can not test without locale files", 2 } pass
> "moo" for 1..2'
>     1..6
>     ok 1 - gettext stuff ok
>     ok 2 - gettext stuff ok
>     ok 3 # skip Can not test without locale files
>     ok 4 # skip Can not test without locale files
>     ok 5 - moo
>     ok 6 - moo

Now you are talking.

What t3600-rm does becomes a lot more natural to express with something
like this.  Any test with "prerequisite" missing will automatically get
"this test was skipped because you lack this prerequisite" for free.  We
can lose skip_all= assignment there but move the logic to test-lib.sh,
which is a good thing.

But that is orthogonal to what you call an API, i.e. your assignment to
the global variable $skip_all that is immediately followed by test_done,
no?  The conversion you did for that does not help counting the remainder
of the tests that are skipped anyway, so you will need to redo that
conversion altogether if you ever want to be able to show "these tests
through the end of the scripts were skipped".

Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 9, 2010 at 22:47, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the
>>> only ones that are not immediately followed by test_done are in
>>> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap
>>> before calling test_done), and t3600-rm (makes a mental note to report
>>> that one test was skipped long before all the tests run).
>> ...
>>     $ perl -MTest::More -E 'plan tests => 6; pass "gettext stuff ok"
>> for 1..2; SKIP: { skip "Can not test without locale files", 2 } pass
>> "moo" for 1..2'
>>     1..6
>>     ok 1 - gettext stuff ok
>>     ok 2 - gettext stuff ok
>>     ok 3 # skip Can not test without locale files
>>     ok 4 # skip Can not test without locale files
>>     ok 5 - moo
>>     ok 6 - moo
>
> Now you are talking.
>
> What t3600-rm does becomes a lot more natural to express with something
> like this.  Any test with "prerequisite" missing will automatically get
> "this test was skipped because you lack this prerequisite" for free.  We
> can lose skip_all= assignment there but move the logic to test-lib.sh,
> which is a good thing.

Right we already do this in the case of t3600-rm.sh. I think in the
general case that using test_set_prereq + 3 arg test_expect_success
(i.e. declare prereq) is a lot better than arranging tests so that you
can skip a lot of dependency-heavy tests at the end.

> But that is orthogonal to what you call an API, i.e. your assignment to
> the global variable $skip_all that is immediately followed by test_done,
> no?  The conversion you did for that does not help counting the remainder
> of the tests that are skipped anyway, so you will need to redo that
> conversion altogether if you ever want to be able to show "these tests
> through the end of the scripts were skipped".

Right, those two are completely orthogonal. That sort of API would
look something like this:

    #!/bin/sh

    plan=6

    . ./test-lib.sh

    for i in {1..2}; do
        test_expect_success 'gettext stuff ok' 'true'
    done

    # something came up, skip the remainder
    skip_all="Oh noes"
    test_done

(or test_done 'Oh noes', or something).

At that point it knows to output 4 skip lines with the message "Oh
noes" because we've only run two tests so far.