Re: [PATCH 1/2] t0303: set reason for skipping tests

[Jeff King <peff@peff.net>]

On Mon, Mar 12, 2012 at 10:07:58PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

> OK, it seems this might be more complicated than I expected. I admit
> that I didn't test this (apart from failing without the variables
> defined) and assumed that it more or less works already.

This script is not very well tested, as it is meant to be run manually
when testing an out-of-tree helper. I used it to test the osx-keychain
helper, but that's it.

> I think that the tests are not very robust:
>     ln -s /bin/true ~/bin/git-credential-fooooooo
>     GIT_TEST_CREDENTIAL_HELPER=fooooooo\
>       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=fooooooo\
>       ./t0303-credential-external.sh
> 
> gives me:
> ok 1 - helper (fooooooo) has no existing data
> ok 2 - helper (fooooooo) stores password
> not ok - 3 helper (fooooooo) can retrieve password
> ok 4 - helper (fooooooo) requires matching protocol
> ok 5 - helper (fooooooo) requires matching host
> ok 6 - helper (fooooooo) requires matching username
> ok 7 - helper (fooooooo) requires matching path
> ok 8 - helper (fooooooo) can forget host
> not ok - 9 helper (fooooooo) can store multiple users
> ok 10 - helper (fooooooo) can forget user
> not ok - 11 helper (fooooooo) remembers other user
> ok 12 - helper (fooooooo) times out
> # failed 3 among 12 test(s)
> 1..12
> 
> I guess that the fact that #1 succeeds reflects reality, but e.g.
> 4-7 and 12 probably should fail.

The reason is that the individual tests do not verify all of the
preconditions themselves, but rather build on each other. So in test 2,
we ask to store some data. The helper tells us it did so successfully
(which is a lie, of course). And then in test 3 we ask it to tell us
what it stored, but of course it can't, and we notice. And then in test
4, we ask again with a restricted query, expecting to see no answer. And
we get it, because of course, the helper will never give us an answer.
If you really wanted to know whether that feature worked, you would
check that we can get anything at all, but not with the restricted query.

In an ideal world, each test snippet would be totally independent and
check its preconditions. That would give us an accurate count of how
many tests actually passed or failed. But fundamentally we only care
about "did they all succeed or not?", which the current script does tell
us (either test 2 fails, or if it succeeds, then we have checked the
precondition for test 4). And the tests end up way shorter, because we
don't repeat the preconditions over and over.

If you want to try to make the tests more robust, you can (for example,
you can tighten the precondition on 4 to check "does it give the right
answer with the right protocol" instead of just "does it ever give us
the right answer"). But personally, I'm not sure it's worth that much
effort.

> >Should they actually say "# SKIP ..." to tell prove what's going on? I
> >don't know very much about TAP.
> # SKIP is used when skipping individual tests (IIUC), but when we
> skip a group of tests, we simply jump over them and this message is
> purely informative output that is not interpreted by the harness.

Just looking at test-lib.sh, it seems like we output "# SKIP" when we do
skip_all. But I think you would have to give a count of which tests you
skipped (e.g., try "./t5541-http-push.sh" to see its TAP output). Which
means when skipping a subset, you'd have to deal with test numbering,
which is a pain. So it's probably not worth worrying about.

-Peff