[PATCH 0/2] prettify t0303-credential-helpers.sh

[PATCH 0/2] prettify t0303-credential-helpers.sh

[Zbigniew Jędrzejewski-Szmek]

Two patches which are not very important, but trivial, so probably
safe.

  1/2: set reason for skipping tests
    (Fix an imperfection in test output under prove)
  2/2: resurrect commit message as test documentation
    (Add documentation)

 t/t0303-credential-external.sh |   27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

-- 
1.7.9.3.467.g8f1c7

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

[Zbigniew Jędrzejewski-Szmek]

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.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t0303-credential-external.sh |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 267f4c8..f1e0e75 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -21,7 +21,7 @@ post_test() {
 }
 
 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
 	pre_test
 	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
@@ -29,11 +29,16 @@ else
 fi
 
 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)"
 else
 	pre_test
 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
 	post_test
 fi
 
+if test -z "$GIT_TEST_CREDENTIAL_HELPER" \
+    -o -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+    skip_all="used to test external credential helpers"
+fi
+
 test_done
-- 
1.7.9.3.467.g8f1c7

[PATCH 2/2] t0303: resurrect commit message as test documentation

[Zbigniew Jędrzejewski-Szmek]

The commit message which added those tests (861444f 't: add test
harness for external credential helpers' 2011-12-10) provided nice
documentation in the commit message. Let's make it more visible.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t0303-credential-external.sh |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f1e0e75..4ab9a54 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -1,5 +1,23 @@
 #!/bin/sh
 
+# Test harness for external credential helpers
+#
+# This is a tool for authors of external helper tools to sanity-check
+# their helpers. If you have written the "git-credential-foo" helper,
+# you check it with:
+#
+# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh
+#
+# This assumes that your helper is capable of both storing and
+# retrieving credentials (some helpers may be read-only, and
+# they will fail these tests).
+#
+# If your helper supports time-based expiration with a
+# configurable timeout, you can test that feature with:
+#
+#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
+#      make t0303-credential-external.sh
+
 test_description='external credential helper tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
-- 
1.7.9.3.467.g8f1c7

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

[Jeff King]

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:

>  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

Re: [PATCH 2/2] t0303: resurrect commit message as test documentation

[Jeff King]

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

> The commit message which added those tests (861444f 't: add test
> harness for external credential helpers' 2011-12-10) provided nice
> documentation in the commit message. Let's make it more visible.

Thanks, good idea.

> +# If your helper supports time-based expiration with a
> +# configurable timeout, you can test that feature with:
> +#
> +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
> +#      make t0303-credential-external.sh

This example is slightly bogus, as described in my previous message. It
needs to set GIT_TEST_CREDENTIAL_HELPER, as well.

-Peff

Re: [PATCH 2/2] t0303: resurrect commit message as test documentation

[Jonathan Nieder]

Hi,

Zbigniew Jędrzejewski-Szmek wrote:

> --- a/t/t0303-credential-external.sh
> +++ b/t/t0303-credential-external.sh
> @@ -1,5 +1,23 @@
>  #!/bin/sh
>  
> +# Test harness for external credential helpers
> +#
> +# This is a tool for authors of external helper tools to sanity-check
> +# their helpers. If you have written the "git-credential-foo" helper,
> +# you check it with:
> +#
> +# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh
> +#
> +# This assumes that your helper is capable of both storing and
> +# retrieving credentials (some helpers may be read-only, and
> +# they will fail these tests).
> +#
> +# If your helper supports time-based expiration with a
> +# configurable timeout, you can test that feature with:
> +#
> +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
> +#      make t0303-credential-external.sh
> +
>  test_description='external credential helper tests'

Nice idea, but shouldn't this description be in test_description so I
can view it by running "sh t0303-credential-external.sh --help"?

Thanks,
Jonathan

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

[Zbigniew Jędrzejewski-Szmek]

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

Re: [PATCH 2/2] t0303: resurrect commit message as test documentation

[Jeff King]

On Mon, Mar 12, 2012 at 03:43:40PM -0500, Jonathan Nieder wrote:

> > +# Test harness for external credential helpers
> > +#
> > +# This is a tool for authors of external helper tools to sanity-check
> > +# their helpers. If you have written the "git-credential-foo" helper,
> > +# you check it with:
> > +#
> > +# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh
> > +#
> > +# This assumes that your helper is capable of both storing and
> > +# retrieving credentials (some helpers may be read-only, and
> > +# they will fail these tests).
> > +#
> > +# If your helper supports time-based expiration with a
> > +# configurable timeout, you can test that feature with:
> > +#
> > +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
> > +#      make t0303-credential-external.sh
> > +
> >  test_description='external credential helper tests'
> 
> Nice idea, but shouldn't this description be in test_description so I
> can view it by running "sh t0303-credential-external.sh --help"?

Yes, that makes sense. I didn't even know that "--help" printed out the
test description; most of our descriptions are not very useful, so I
never bothered. But this is the perfect thing to put in there.

-Peff

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

[Jeff King]

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

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

[Zbigniew Jędrzejewski-Szmek]

On Tue, Mar 13, 2012 at 05:53:32PM -0400, Jeff King wrote:
> The reason is that the individual tests do not verify all of the
> preconditions themselves, but rather build on each other.
Right. I added a note based on this sentence in the test description.

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

> > >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.
Skipped test numbering could done automatically by using test prereqs,
but (after actually doing that and discarding) I agree that it isn't
worth the trouble.


Jonathan Nieder wrote:
> Nice idea, but shouldn't this description be in test_description so I
> can view it by running "sh t0303-credential-external.sh --help"?
Done.

Updated patches follow.

(This time I tested with GIT_TEST_CREDENTIAL_HELPER=cache
GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="cache --timeout=1,3" and things
seem to work as expected.)

Zbyszek

[PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Zbigniew Jędrzejewski-Szmek]

t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be
configured to do something sensible. If it is not set, prove will say:
  ./t0303-credential-external.sh .. skipped: (no reason given)
which isn't very nice.

Use skip_all="..." && test_done to bail out immediately and provide a
nicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the
timeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t0303-credential-external.sh |   40 ++++++++++++++++------------------------
 1 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 267f4c8..4479bf8 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -4,36 +4,28 @@ test_description='external credential helper tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-pre_test() {
-	test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
-	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
-
-	# clean before the test in case there is cruft left
-	# over from a previous run that would impact results
-	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
-}
-
-post_test() {
-	# clean afterwards so that we are good citizens
-	# and don't leave cruft in the helper's storage, which
-	# might be long-term system storage
-	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
-}
-
 if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
-	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
-else
-	pre_test
-	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
-	post_test
+	skip_all="used to test external credential helpers"
+	test_done
 fi
 
+$GIT_TEST_CREDENTIAL_HELPER_SETUP
+
+# clean before the test in case there is cruft left
+# over from a previous run that would impact results
+helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+
+helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+
 if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
-	say "# skipping external helper timeout tests"
+	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
 else
-	pre_test
 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
-	post_test
 fi
 
+# clean afterwards so that we are good citizens
+# and don't leave cruft in the helper's storage, which
+# might be long-term system storage
+helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+
 test_done
-- 
1.7.9.1

[PATCH v2 2/2] t0303: resurrect commit message as test documentation

[Zbigniew Jędrzejewski-Szmek]

The commit message which added those tests (861444f 't: add test
harness for external credential helpers' 2011-12-10) provided nice
documentation in the commit message. Let's make it more visible
by putting it in the test description.

The documentation is updated to reflect the fact that
GIT_TEST_CREDENTIAL_HELPER must be set for
GIT_TEST_CREDENTIAL_HELPER_TIMEOUT to be used
and GIT_TEST_CREDENTIAL_HELPER_SETUP can be used.

Based-on-commit-message-by: Jeff King <peff@peff.net>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t0303-credential-external.sh |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 4479bf8..136da1e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -1,6 +1,34 @@
 #!/bin/sh
 
-test_description='external credential helper tests'
+test_description='external credential helper tests
+
+This is a tool for authors of external helper tools to sanity-check
+their helpers. If you have written the "git-credential-foo" helper,
+you check it with:
+
+  make GIT_TEST_CREDENTIAL_HELPER=foo t0303-credential-external.sh
+
+This assumes that your helper is capable of both storing and
+retrieving credentials (some helpers may be read-only, and they will
+fail these tests).
+
+Please note that the individual tests do not verify all of the
+preconditions themselves, but rather build on each other. A failing
+test means that tests later in the sequence can return false "OK"
+results.
+
+If your helper supports time-based expiration with a configurable
+timeout, you can test that feature with:
+
+  make GIT_TEST_CREDENTIAL_HELPER=foo \
+       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
+       t0303-credential-external.sh
+
+If your helper requires additional setup before the tests are started,
+you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell
+commands.
+'
+
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
1.7.9.1

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Junio C Hamano]

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be
> configured to do something sensible. If it is not set, prove will say:
>   ./t0303-credential-external.sh .. skipped: (no reason given)
> which isn't very nice.
>
> Use skip_all="..." && test_done to bail out immediately and provide a
> nicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the
> timeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.
>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
>  t/t0303-credential-external.sh |   40 ++++++++++++++++------------------------
>  1 files changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> index 267f4c8..4479bf8 100755
> --- a/t/t0303-credential-external.sh
> +++ b/t/t0303-credential-external.sh
> @@ -4,36 +4,28 @@ test_description='external credential helper tests'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-credential.sh
>  
> -pre_test() {
> -	test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> -	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
> -
> -	# clean before the test in case there is cruft left
> -	# over from a previous run that would impact results
> -	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> -}
> -
> -post_test() {
> -	# clean afterwards so that we are good citizens
> -	# and don't leave cruft in the helper's storage, which
> -	# might be long-term system storage
> -	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> -}
> -
>  if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
> -	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
> -else
> -	pre_test
> -	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> -	post_test
> +	skip_all="used to test external credential helpers"
> +	test_done
>  fi
>  
> +$GIT_TEST_CREDENTIAL_HELPER_SETUP

This used to be 'test -z "$it" || eval "$it"'; doesn't it make a
difference?

What is the value expected to be in this variable?  Nobody seems to set it
in our codebase, so I cannot say "with the current code, this rewrite is
safe" or anything like that.

This is probably not related to your patch, but

	GIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh

passes OK for me while

	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove

seems to get stuck forever.

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Jeff King]

On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:

> > +$GIT_TEST_CREDENTIAL_HELPER_SETUP
> 
> This used to be 'test -z "$it" || eval "$it"'; doesn't it make a
> difference?
>
> What is the value expected to be in this variable?  Nobody seems to set it
> in our codebase, so I cannot say "with the current code, this rewrite is
> safe" or anything like that.

I think eval is a better route, as it gives the caller more flexibility
about what shell code to run. The only use is here:

  http://article.gmane.org/gmane.comp.version-control.git/186757

which does work either way.

> This is probably not related to your patch, but
> 
> 	GIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh
> 
> passes OK for me while
> 
> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
> 
> seems to get stuck forever.

It's because t0303 is the generic "test any helper" script, and does not
know how to clean up the credential-cache daemon. So the daemon sticks
around, holding onto a file descriptor that causes prove to hang.
If you look at t0301 (which runs the same tests on credential-cache), we
kill the resulting daemon explicitly. t0303 could learn hooks to do
this, but I didn't bother, as I didn't need them for testing the
osxkeychain helper (which is the only thing I've used t0303 for, as
t0301 and t0302 cover the in-tree helpers). I figured that somebody
could add the hooks easily if and when they needed.

-Peff

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Jeff King]

On Wed, Mar 14, 2012 at 03:18:23PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

> -pre_test() {
> -	test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> -	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
> [...]
> +$GIT_TEST_CREDENTIAL_HELPER_SETUP

Except for losing the eval here that Junio mentioned, I think both
patches look good.

-Peff

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:
>> This is probably not related to your patch, but
>> 
>> 	GIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh
>> 
>> passes OK for me while
>> 
>> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
>> 
>> seems to get stuck forever.
>
> It's because t0303 is the generic "test any helper" script, and does not
> know how to clean up the credential-cache daemon. So the daemon sticks
> around, holding onto a file descriptor that causes prove to hang.

And the reason why "sh t0303-*.sh" version does not have this problem is...?

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Zbigniew Jędrzejewski-Szmek]

On 03/15/2012 07:55 AM, Junio C Hamano wrote:
> Jeff King<peff@peff.net>  writes:
>
>> On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:
>>> This is probably not related to your patch, but
>>>
>>> 	GIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh
>>>
>>> passes OK for me while
>>>
>>> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
>>>
>>> seems to get stuck forever.
>>
>> It's because t0303 is the generic "test any helper" script, and does not
>> know how to clean up the credential-cache daemon. So the daemon sticks
>> around, holding onto a file descriptor that causes prove to hang.
>
> And the reason why "sh t0303-*.sh" version does not have this problem is...?

It does :)

In both cases git-credential-cache--daemon is running. It is stuck in 
poll() with a timeout of 30*1000 ms (credentail-cache--daemon.c:175). 
When running without prove, it is left in the background and terminates 
after 30 s. When running under prove, prove waits for 30 seconds for the 
process to end and then terminates.

I think that this delay is OK, as it happens only when running an 
explicitly requested test, and only under prove.

Zbyszek

[PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Zbigniew Jędrzejewski-Szmek]

t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be
configured to do something sensible. If it is not set, prove will say:
  ./t0303-credential-external.sh .. skipped: (no reason given)
which isn't very nice.

Use skip_all="..." && test_done to bail out immediately and provide a
nicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the
timeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Acked-by: Jeff King <peff@peff.net>
---
This is v3: the only change is the removal of the removal of the eval around
$GIT_TEST_CREDENTIAL_HELPER_SETUP.

 t/t0303-credential-external.sh |   39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 267f4c8..e771075 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -4,36 +4,29 @@ test_description='external credential helper tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-pre_test() {
-	test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
-	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
+if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
+	skip_all="used to test external credential helpers"
+	test_done
+fi
 
-	# clean before the test in case there is cruft left
-	# over from a previous run that would impact results
-	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
-}
+test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
+	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
 
-post_test() {
-	# clean afterwards so that we are good citizens
-	# and don't leave cruft in the helper's storage, which
-	# might be long-term system storage
-	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
-}
+# clean before the test in case there is cruft left
+# over from a previous run that would impact results
+helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
 
-if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
-	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
-else
-	pre_test
-	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
-	post_test
-fi
+helper_test "$GIT_TEST_CREDENTIAL_HELPER"
 
 if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
-	say "# skipping external helper timeout tests"
+	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
 else
-	pre_test
 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
-	post_test
 fi
 
+# clean afterwards so that we are good citizens
+# and don't leave cruft in the helper's storage, which
+# might be long-term system storage
+helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+
 test_done
-- 
1.7.10.rc0.160.g12d89

[PATCH v3 2/2] t0303: resurrect commit message as test documentation

[Zbigniew Jędrzejewski-Szmek]

The commit message which added those tests (861444f 't: add test
harness for external credential helpers' 2011-12-10) provided nice
documentation in the commit message. Let's make it more visible
by putting it in the test description.

The documentation is updated to reflect the fact that
GIT_TEST_CREDENTIAL_HELPER must be set for
GIT_TEST_CREDENTIAL_HELPER_TIMEOUT to be used
and GIT_TEST_CREDENTIAL_HELPER_SETUP can be used.

Based-on-commit-message-by: Jeff King <peff@peff.net>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Acked-by: Jeff King <peff@peff.net>
---
 t/t0303-credential-external.sh |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index e771075..f028fd1 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -1,6 +1,34 @@
 #!/bin/sh
 
-test_description='external credential helper tests'
+test_description='external credential helper tests
+
+This is a tool for authors of external helper tools to sanity-check
+their helpers. If you have written the "git-credential-foo" helper,
+you check it with:
+
+  make GIT_TEST_CREDENTIAL_HELPER=foo t0303-credential-external.sh
+
+This assumes that your helper is capable of both storing and
+retrieving credentials (some helpers may be read-only, and they will
+fail these tests).
+
+Please note that the individual tests do not verify all of the
+preconditions themselves, but rather build on each other. A failing
+test means that tests later in the sequence can return false "OK"
+results.
+
+If your helper supports time-based expiration with a configurable
+timeout, you can test that feature with:
+
+  make GIT_TEST_CREDENTIAL_HELPER=foo \
+       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
+       t0303-credential-external.sh
+
+If your helper requires additional setup before the tests are started,
+you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell
+commands.
+'
+
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-- 
1.7.10.rc0.160.g12d89

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Jeff King]

On Wed, Mar 14, 2012 at 11:55:24PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:
> >> This is probably not related to your patch, but
> >> 
> >> 	GIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh
> >> 
> >> passes OK for me while
> >> 
> >> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
> >> 
> >> seems to get stuck forever.
> >
> > It's because t0303 is the generic "test any helper" script, and does not
> > know how to clean up the credential-cache daemon. So the daemon sticks
> > around, holding onto a file descriptor that causes prove to hang.
> 
> And the reason why "sh t0303-*.sh" version does not have this problem is...?

Most helpers don't spawn a daemon that hangs around (and if they do, the
instructions for killing said daemon are outside the scope of the helper
protocol -- though I would recommend having an "exit" command, as
credential-cache has). You could add something like:

  GIT_TEST_CREDENTIAL_HELPER='cache' \
  GIT_TEST_CREDENTIAL_HELPER_EXIT='git credential-cache exit' \
  ./t0303-*

But like I said, I didn't bother. If you are testing credential-cache,
then use t0301, which handles this. If you are testing something
external, use t0303. My external testing didn't require such an exit
hook, so I didn't bother with it. If somebody writes a helper that
requires such a hook, they can add it then. I didn't want to get into
the business of guessing which hooks people might need (and it is not as
if these tests are an end-user visible piece of code; they are purely a
convenience for developers to test their implementations against the
same battery of tests that credential-cache and credential-store use).

-Peff

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Jeff King]

On Thu, Mar 15, 2012 at 09:24:28AM -0400, Jeff King wrote:

> > >> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
> > >> 
> > >> seems to get stuck forever.
> > >
> > > It's because t0303 is the generic "test any helper" script, and does not
> > > know how to clean up the credential-cache daemon. So the daemon sticks
> > > around, holding onto a file descriptor that causes prove to hang.
> > 
> > And the reason why "sh t0303-*.sh" version does not have this problem is...?
> [long-winded explanation from me]

Oops. I read this as "why does t0301 not have the problem?". So ignore
everything I said.

The reason why running it via sh works is that we leave the daemon
running in both cases, but only prove actually cares about the leaked
file descriptor and blocks.

-Peff

Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 15, 2012 at 09:24:28AM -0400, Jeff King wrote:
>
>> > >> 	make GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove
>> > >> 
>> > >> seems to get stuck forever.
>> > >
>> > > It's because t0303 is the generic "test any helper" script, and does not
>> > > know how to clean up the credential-cache daemon. So the daemon sticks
>> > > around, holding onto a file descriptor that causes prove to hang.
>> > 
>> > And the reason why "sh t0303-*.sh" version does not have this problem is...?
>> [long-winded explanation from me]
>
> Oops. I read this as "why does t0301 not have the problem?". So ignore
> everything I said.
>
> The reason why running it via sh works is that we leave the daemon
> running in both cases, but only prove actually cares about the leaked
> file descriptor and blocks.

Ah, OK, the last part was what I was missing.  Thanks for a clarification.

Re: [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

[Junio C Hamano]

Thanks all; will queue.