On 03/12/2012 01:30 PM, Jeff King wrote:
On Mon, Mar 12, 2012 at 01:05:06PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
t0300-credential-helpers.sh runs two sets of tests. Each set is
controlled by an environment variable and is skipped if the variable
is not defined. If both sets are skipped, prove will say:
./t0303-credential-external.sh .. skipped: (no reason given)
which isn't very nice.
Use skip_all="..." to set the reason when both sets are skipped.
Sounds reasonable. A few nits:
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.
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.
if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
- say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
+ say "# skipping external helper tests (GIT_TEST_CREDENTIAL_HELPER not set)"
else
[...]
if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
- say "# skipping external helper timeout tests"
+ say "# skipping external helper timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
These don't affect prove at all, do they? I'm OK with the changes, but I
was confused to see them after reading the commit message.
Right, this was just for symmetry when running test directly. Forgot
to mention this in the commit message.
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.
+if test -z "$GIT_TEST_CREDENTIAL_HELPER" \
+ -o -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+ skip_all="used to test external credential helpers"
+fi
Actually, I think it is not OK to run t0303 with HELPER_TIMEOUT set, but
HELPER not set. The "helper_test_clean" bits will fail badly. The
documentation given in the commit message is actually wrong (I added the
clean bits to the patch later, and failed to realize the dependency or
update the commit message).
Also, our usual idiom is to check the prerequisites at the top of the
script and bail immediately.
So maybe the whole script should be restructured as:
if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
skip_all="GIT_TEST_CREDENTIAL_HELPER not set"
test_done
fi
pre_test
helper_test "$GIT_TEST_CREDENTIAL_HELPER"
if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
else
helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
fi
post_test
Yeah, this seems to be the right approach. I'll repost with a proper
commit message once my doubts about the tests are cleared up.
-
Zbyszek
--
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