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

From: Jeff King <peff@peff.net>
Date: Mar 12, 2012, 12:30 UTC
Message-ID: <20120312123031.GA14456@sigill.intra.peff.net>
In-Reply-To: <1331553907-19576-2-git-send-email-zbyszek@in.waw.pl>

On Mon, Mar 12, 2012 at 01:05:06PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

Show 7 quoted lines

> 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:

Show 8 quoted lines

>  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.

Should they actually say "# SKIP ..." to tell prove what's going on? I don't know very much about TAP.

> +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

-Peff