From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:16
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
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:16
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 <redacted>
---
t/t0303-credential-external.sh | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:16
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 <redacted>
---
t/t0303-credential-external.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
@@ -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
From: Jeff King <hidden> Date: 2016-06-15 22:53:16
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
From: Jeff King <hidden> Date: 2016-06-15 22:53:16
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
@@ -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
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:17
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
From: Jeff King <hidden> Date: 2016-06-15 22:53:18
On Mon, Mar 12, 2012 at 03:43:40PM -0500, Jonathan Nieder wrote:
quoted
+# 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
From: Jeff King <hidden> Date: 2016-06-15 22:53:18
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.
quoted
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
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:18
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.
quoted
quoted
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
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:18
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 <redacted>
---
t/t0303-credential-external.sh | 40 ++++++++++++++++------------------------
1 files changed, 16 insertions(+), 24 deletions(-)
@@ -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"-}-iftest-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_donefi+$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"+iftest-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_testhelper_test_timeout"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"-post_testfi+# 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
From: Zbigniew Jędrzejewski-Szmek <hidden> Date: 2016-06-15 22:53:18
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 [off-list ref]
Signed-off-by: Zbigniew Jędrzejewski-Szmek <redacted>
---
t/t0303-credential-external.sh | 30 +++++++++++++++++++++++++++++-
1 files changed, 29 insertions(+), 1 deletions(-)