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

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:

quoted

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.

quoted

  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.

quoted

+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