Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>
> On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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" &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html