On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Æ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.
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" &&
>
--
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