* [PATCH 0/2] prettify t0303-credential-helpers.sh
@ 2012-03-12 12:05 Zbigniew Jędrzejewski-Szmek
2012-03-12 12:05 ` [PATCH 1/2] t0303: set reason for skipping tests Zbigniew Jędrzejewski-Szmek
2012-03-12 12:05 ` [PATCH 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek
0 siblings, 2 replies; 23+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:05 UTC (permalink / raw)
To: git, gitster; +Cc: Jeff King, 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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/2] t0303: set reason for skipping tests 2012-03-12 12:05 [PATCH 0/2] prettify t0303-credential-helpers.sh Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:05 ` Zbigniew Jędrzejewski-Szmek 2012-03-12 12:30 ` Jeff King 2012-03-12 12:05 ` [PATCH 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 1 sibling, 1 reply; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:05 UTC (permalink / raw) To: git, gitster; +Cc: Jeff King, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] t0303: set reason for skipping tests 2012-03-12 12:05 ` [PATCH 1/2] t0303: set reason for skipping tests Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:30 ` Jeff King 2012-03-12 21:07 ` Zbigniew Jędrzejewski-Szmek 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2012-03-12 12:30 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] t0303: set reason for skipping tests 2012-03-12 12:30 ` Jeff King @ 2012-03-12 21:07 ` Zbigniew Jędrzejewski-Szmek 2012-03-13 21:53 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-12 21:07 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] t0303: set reason for skipping tests 2012-03-12 21:07 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-13 21:53 ` Jeff King 2012-03-14 14:14 ` Zbigniew Jędrzejewski-Szmek 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2012-03-13 21:53 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] t0303: set reason for skipping tests 2012-03-13 21:53 ` Jeff King @ 2012-03-14 14:14 ` Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Zbigniew Jędrzejewski-Szmek 0 siblings, 1 reply; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-14 14:14 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-14 14:14 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-14 14:18 ` Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-14 14:18 UTC (permalink / raw) To: git; +Cc: gitster, Jonathan Nieder, Jeff King, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] t0303: resurrect commit message as test documentation 2012-03-14 14:18 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Zbigniew Jędrzejewski-Szmek @ 2012-03-14 14:18 ` Zbigniew Jędrzejewski-Szmek 2012-03-14 22:17 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano 2012-03-15 3:56 ` Jeff King 2 siblings, 0 replies; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-14 14:18 UTC (permalink / raw) To: git; +Cc: gitster, Jonathan Nieder, Jeff King, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-14 14:18 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek @ 2012-03-14 22:17 ` Junio C Hamano 2012-03-15 3:54 ` Jeff King 2012-03-15 3:56 ` Jeff King 2 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2012-03-14 22:17 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Jonathan Nieder, Jeff King 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-14 22:17 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano @ 2012-03-15 3:54 ` Jeff King 2012-03-15 6:55 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2012-03-15 3:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Zbigniew Jędrzejewski-Szmek, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 3:54 ` Jeff King @ 2012-03-15 6:55 ` Junio C Hamano 2012-03-15 10:44 ` Zbigniew Jędrzejewski-Szmek 2012-03-15 13:24 ` [PATCH v2 " Jeff King 0 siblings, 2 replies; 23+ messages in thread From: Junio C Hamano @ 2012-03-15 6:55 UTC (permalink / raw) To: Jeff King; +Cc: Zbigniew Jędrzejewski-Szmek, git, Jonathan Nieder 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...? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 6:55 ` Junio C Hamano @ 2012-03-15 10:44 ` Zbigniew Jędrzejewski-Szmek 2012-03-15 11:08 ` [PATCH v3 " Zbigniew Jędrzejewski-Szmek 2012-03-15 13:24 ` [PATCH v2 " Jeff King 1 sibling, 1 reply; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-15 10:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 10:44 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-15 11:08 ` Zbigniew Jędrzejewski-Szmek 2012-03-15 11:08 ` [PATCH v3 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-15 17:51 ` [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano 0 siblings, 2 replies; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-15 11:08 UTC (permalink / raw) To: git, gitster; +Cc: Jeff King, jrnieder, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/2] t0303: resurrect commit message as test documentation 2012-03-15 11:08 ` [PATCH v3 " Zbigniew Jędrzejewski-Szmek @ 2012-03-15 11:08 ` Zbigniew Jędrzejewski-Szmek 2012-03-15 17:51 ` [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-15 11:08 UTC (permalink / raw) To: git, gitster; +Cc: Jeff King, jrnieder, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 11:08 ` [PATCH v3 " Zbigniew Jędrzejewski-Szmek 2012-03-15 11:08 ` [PATCH v3 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek @ 2012-03-15 17:51 ` Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2012-03-15 17:51 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Jeff King, jrnieder Thanks all; will queue. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 6:55 ` Junio C Hamano 2012-03-15 10:44 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-15 13:24 ` Jeff King 2012-03-15 13:26 ` Jeff King 1 sibling, 1 reply; 23+ messages in thread From: Jeff King @ 2012-03-15 13:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Zbigniew Jędrzejewski-Szmek, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 13:24 ` [PATCH v2 " Jeff King @ 2012-03-15 13:26 ` Jeff King 2012-03-15 17:50 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2012-03-15 13:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Zbigniew Jędrzejewski-Szmek, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-15 13:26 ` Jeff King @ 2012-03-15 17:50 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2012-03-15 17:50 UTC (permalink / raw) To: Jeff King; +Cc: Zbigniew Jędrzejewski-Szmek, git, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER 2012-03-14 14:18 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-14 22:17 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano @ 2012-03-15 3:56 ` Jeff King 2 siblings, 0 replies; 23+ messages in thread From: Jeff King @ 2012-03-15 3:56 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2] t0303: resurrect commit message as test documentation 2012-03-12 12:05 [PATCH 0/2] prettify t0303-credential-helpers.sh Zbigniew Jędrzejewski-Szmek 2012-03-12 12:05 ` [PATCH 1/2] t0303: set reason for skipping tests Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:05 ` Zbigniew Jędrzejewski-Szmek 2012-03-12 12:31 ` Jeff King 2012-03-12 20:43 ` Jonathan Nieder 1 sibling, 2 replies; 23+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:05 UTC (permalink / raw) To: git, gitster; +Cc: Jeff King, 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] t0303: resurrect commit message as test documentation 2012-03-12 12:05 ` [PATCH 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek @ 2012-03-12 12:31 ` Jeff King 2012-03-12 20:43 ` Jonathan Nieder 1 sibling, 0 replies; 23+ messages in thread From: Jeff King @ 2012-03-12 12:31 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] t0303: resurrect commit message as test documentation 2012-03-12 12:05 ` [PATCH 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-12 12:31 ` Jeff King @ 2012-03-12 20:43 ` Jonathan Nieder 2012-03-13 21:38 ` Jeff King 1 sibling, 1 reply; 23+ messages in thread From: Jonathan Nieder @ 2012-03-12 20:43 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster, Jeff King 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] t0303: resurrect commit message as test documentation 2012-03-12 20:43 ` Jonathan Nieder @ 2012-03-13 21:38 ` Jeff King 0 siblings, 0 replies; 23+ messages in thread From: Jeff King @ 2012-03-13 21:38 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Zbigniew Jędrzejewski-Szmek, git, gitster 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2012-03-15 17:51 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-12 12:05 [PATCH 0/2] prettify t0303-credential-helpers.sh Zbigniew Jędrzejewski-Szmek 2012-03-12 12:05 ` [PATCH 1/2] t0303: set reason for skipping tests Zbigniew Jędrzejewski-Szmek 2012-03-12 12:30 ` Jeff King 2012-03-12 21:07 ` Zbigniew Jędrzejewski-Szmek 2012-03-13 21:53 ` Jeff King 2012-03-14 14:14 ` Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Zbigniew Jędrzejewski-Szmek 2012-03-14 14:18 ` [PATCH v2 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-14 22:17 ` [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano 2012-03-15 3:54 ` Jeff King 2012-03-15 6:55 ` Junio C Hamano 2012-03-15 10:44 ` Zbigniew Jędrzejewski-Szmek 2012-03-15 11:08 ` [PATCH v3 " Zbigniew Jędrzejewski-Szmek 2012-03-15 11:08 ` [PATCH v3 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-15 17:51 ` [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER Junio C Hamano 2012-03-15 13:24 ` [PATCH v2 " Jeff King 2012-03-15 13:26 ` Jeff King 2012-03-15 17:50 ` Junio C Hamano 2012-03-15 3:56 ` Jeff King 2012-03-12 12:05 ` [PATCH 2/2] t0303: resurrect commit message as test documentation Zbigniew Jędrzejewski-Szmek 2012-03-12 12:31 ` Jeff King 2012-03-12 20:43 ` Jonathan Nieder 2012-03-13 21:38 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).