{"thread":{"id":"29916","subject":"[PATCH 0/2] prettify t0303-credential-helpers.sh","startedAt":"2012-03-12T12:05:05Z","lastAt":"2012-03-15T17:51:47Z","messageCount":23,"participants":["Zbigniew Jędrzejewski-Szmek","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"186698","messageId":"1331553907-19576-1-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":null,"subject":"[PATCH 0/2] prettify t0303-credential-helpers.sh","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-12T12:05:05Z","receivedAt":"2012-03-12T12:05:05Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"Two patches which are not very important, but trivial, so probably\nsafe.\n\n  1/2: set reason for skipping tests\n    (Fix an imperfection in test output under prove)\n  2/2: resurrect commit message as test documentation\n    (Add documentation)\n\n t/t0303-credential-external.sh |   27 +++++++++++++++++++++++++--\n 1 file changed, 25 insertions(+), 2 deletions(-)\n\n-- \n1.7.9.3.467.g8f1c7\n"},{"id":"186699","messageId":"1331553907-19576-2-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"1331553907-19576-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 1/2] t0303: set reason for skipping tests","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-12T12:05:06Z","receivedAt":"2012-03-12T12:05:06Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"t0300-credential-helpers.sh runs two sets of tests. Each set is\ncontrolled by an environment variable and is skipped if the variable\nis not defined. If both sets are skipped, prove will say:\n  ./t0303-credential-external.sh .. skipped: (no reason given)\nwhich isn't very nice.\n\nUse skip_all=\"...\" to set the reason when both sets are skipped.\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t0303-credential-external.sh |    9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 267f4c8..f1e0e75 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -21,7 +21,7 @@ post_test() {\n }\n \n if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n-\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n+\tsay \"# skipping external helper tests (GIT_TEST_CREDENTIAL_HELPER not set)\"\n else\n \tpre_test\n \thelper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n@@ -29,11 +29,16 @@ else\n fi\n \n if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n-\tsay \"# skipping external helper timeout tests\"\n+\tsay \"# skipping external helper timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n else\n \tpre_test\n \thelper_test_timeout \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"\n \tpost_test\n fi\n \n+if test -z \"$GIT_TEST_CREDENTIAL_HELPER\" \\\n+    -o -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n+    skip_all=\"used to test external credential helpers\"\n+fi\n+\n test_done\n-- \n1.7.9.3.467.g8f1c7\n"},{"id":"186700","messageId":"1331553907-19576-3-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"1331553907-19576-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH 2/2] t0303: resurrect commit message as test documentation","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-12T12:05:07Z","receivedAt":"2012-03-12T12:05:07Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"The commit message which added those tests (861444f 't: add test\nharness for external credential helpers' 2011-12-10) provided nice\ndocumentation in the commit message. Let's make it more visible.\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t0303-credential-external.sh |   18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex f1e0e75..4ab9a54 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -1,5 +1,23 @@\n #!/bin/sh\n \n+# Test harness for external credential helpers\n+#\n+# This is a tool for authors of external helper tools to sanity-check\n+# their helpers. If you have written the \"git-credential-foo\" helper,\n+# you check it with:\n+#\n+# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh\n+#\n+# This assumes that your helper is capable of both storing and\n+# retrieving credentials (some helpers may be read-only, and\n+# they will fail these tests).\n+#\n+# If your helper supports time-based expiration with a\n+# configurable timeout, you can test that feature with:\n+#\n+#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n+#      make t0303-credential-external.sh\n+\n test_description='external credential helper tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n-- \n1.7.9.3.467.g8f1c7\n"},{"id":"186702","messageId":"20120312123031.GA14456@sigill.intra.peff.net","threadId":"29916","inReplyTo":"1331553907-19576-2-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 1/2] t0303: set reason for skipping tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-12T12:30:31Z","receivedAt":"2012-03-12T12:30:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 12, 2012 at 01:05:06PM +0100, Zbigniew Jędrzejewski-Szmek wrote:\n\n> t0300-credential-helpers.sh runs two sets of tests. Each set is\n> controlled by an environment variable and is skipped if the variable\n> is not defined. If both sets are skipped, prove will say:\n>   ./t0303-credential-external.sh .. skipped: (no reason given)\n> which isn't very nice.\n> \n> Use skip_all=\"...\" to set the reason when both sets are skipped.\n\nSounds reasonable. A few nits:\n\n>  if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n> -\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n> +\tsay \"# skipping external helper tests (GIT_TEST_CREDENTIAL_HELPER not set)\"\n>  else\n> [...]\n>  if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n> -\tsay \"# skipping external helper timeout tests\"\n> +\tsay \"# skipping external helper timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n\nThese don't affect prove at all, do they? I'm OK with the changes, but I\nwas confused to see them after reading the commit message.\n\nShould they actually say \"# SKIP ...\" to tell prove what's going on? I\ndon't know very much about TAP.\n\n> +if test -z \"$GIT_TEST_CREDENTIAL_HELPER\" \\\n> +    -o -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n> +    skip_all=\"used to test external credential helpers\"\n> +fi\n\nActually, I think it is not OK to run t0303 with HELPER_TIMEOUT set, but\nHELPER not set. The \"helper_test_clean\" bits will fail badly. The\ndocumentation given in the commit message is actually wrong (I added the\nclean bits to the patch later, and failed to realize the dependency or\nupdate the commit message).\n\nAlso, our usual idiom is to check the prerequisites at the top of the\nscript and bail immediately.\n\nSo maybe the whole script should be restructured as:\n\n  if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n          skip_all=\"GIT_TEST_CREDENTIAL_HELPER not set\"\n          test_done\n  fi\n\n  pre_test\n  helper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n  if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n          say \"# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n  else\n          helper_test_timeout \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"\n  fi\n  post_test\n\n-Peff\n"},{"id":"186703","messageId":"20120312123153.GB14456@sigill.intra.peff.net","threadId":"29916","inReplyTo":"1331553907-19576-3-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 2/2] t0303: resurrect commit message as test documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-12T12:31:53Z","receivedAt":"2012-03-12T12:31:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 12, 2012 at 01:05:07PM +0100, Zbigniew Jędrzejewski-Szmek wrote:\n\n> The commit message which added those tests (861444f 't: add test\n> harness for external credential helpers' 2011-12-10) provided nice\n> documentation in the commit message. Let's make it more visible.\n\nThanks, good idea.\n\n> +# If your helper supports time-based expiration with a\n> +# configurable timeout, you can test that feature with:\n> +#\n> +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n> +#      make t0303-credential-external.sh\n\nThis example is slightly bogus, as described in my previous message. It\nneeds to set GIT_TEST_CREDENTIAL_HELPER, as well.\n\n-Peff\n"},{"id":"186779","messageId":"20120312204340.GA10661@burratino","threadId":"29916","inReplyTo":"1331553907-19576-3-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH 2/2] t0303: resurrect commit message as test documentation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-12T20:43:40Z","receivedAt":"2012-03-12T20:43:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nZbigniew Jędrzejewski-Szmek wrote:\n\n> --- a/t/t0303-credential-external.sh\n> +++ b/t/t0303-credential-external.sh\n> @@ -1,5 +1,23 @@\n>  #!/bin/sh\n>  \n> +# Test harness for external credential helpers\n> +#\n> +# This is a tool for authors of external helper tools to sanity-check\n> +# their helpers. If you have written the \"git-credential-foo\" helper,\n> +# you check it with:\n> +#\n> +# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh\n> +#\n> +# This assumes that your helper is capable of both storing and\n> +# retrieving credentials (some helpers may be read-only, and\n> +# they will fail these tests).\n> +#\n> +# If your helper supports time-based expiration with a\n> +# configurable timeout, you can test that feature with:\n> +#\n> +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n> +#      make t0303-credential-external.sh\n> +\n>  test_description='external credential helper tests'\n\nNice idea, but shouldn't this description be in test_description so I\ncan view it by running \"sh t0303-credential-external.sh --help\"?\n\nThanks,\nJonathan\n"},{"id":"186783","messageId":"4F5E65AE.8050401@in.waw.pl","threadId":"29916","inReplyTo":"20120312123031.GA14456@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0303: set reason for skipping tests","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-12T21:07:58Z","receivedAt":"2012-03-12T21:07:58Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 03/12/2012 01:30 PM, Jeff King wrote:\n> On Mon, Mar 12, 2012 at 01:05:06PM +0100, Zbigniew Jędrzejewski-Szmek wrote:\n>\n>> t0300-credential-helpers.sh runs two sets of tests. Each set is\n>> controlled by an environment variable and is skipped if the variable\n>> is not defined. If both sets are skipped, prove will say:\n>>    ./t0303-credential-external.sh .. skipped: (no reason given)\n>> which isn't very nice.\n>>\n>> Use skip_all=\"...\" to set the reason when both sets are skipped.\n>\n> Sounds reasonable. A few nits:\nOK, it seems this might be more complicated than I expected. I admit \nthat I didn't test this (apart from failing without the variables \ndefined) and assumed that it more or less works already.\n\nI think that the tests are not very robust:\n     ln -s /bin/true ~/bin/git-credential-fooooooo\n     GIT_TEST_CREDENTIAL_HELPER=fooooooo\\\n       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=fooooooo\\\n       ./t0303-credential-external.sh\n\ngives me:\nok 1 - helper (fooooooo) has no existing data\nok 2 - helper (fooooooo) stores password\nnot ok - 3 helper (fooooooo) can retrieve password\nok 4 - helper (fooooooo) requires matching protocol\nok 5 - helper (fooooooo) requires matching host\nok 6 - helper (fooooooo) requires matching username\nok 7 - helper (fooooooo) requires matching path\nok 8 - helper (fooooooo) can forget host\nnot ok - 9 helper (fooooooo) can store multiple users\nok 10 - helper (fooooooo) can forget user\nnot ok - 11 helper (fooooooo) remembers other user\nok 12 - helper (fooooooo) times out\n# failed 3 among 12 test(s)\n1..12\n\nI guess that the fact that #1 succeeds reflects reality, but e.g.\n4-7 and 12 probably should fail.\n\n>>   if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n>> -\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n>> +\tsay \"# skipping external helper tests (GIT_TEST_CREDENTIAL_HELPER not set)\"\n>>   else\n>> [...]\n>>   if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n>> -\tsay \"# skipping external helper timeout tests\"\n>> +\tsay \"# skipping external helper timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n>\n> These don't affect prove at all, do they? I'm OK with the changes, but I\n> was confused to see them after reading the commit message.\nRight, this was just for symmetry when running test directly. Forgot\nto mention this in the commit message.\n\n> Should they actually say \"# SKIP ...\" to tell prove what's going on? I\n> don't know very much about TAP.\n# SKIP is used when skipping individual tests (IIUC), but when we skip a \ngroup of tests, we simply jump over them and this message is purely \ninformative output that is not interpreted by the harness.\n\n>> +if test -z \"$GIT_TEST_CREDENTIAL_HELPER\" \\\n>> +    -o -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n>> +    skip_all=\"used to test external credential helpers\"\n>> +fi\n>\n> Actually, I think it is not OK to run t0303 with HELPER_TIMEOUT set, but\n> HELPER not set. The \"helper_test_clean\" bits will fail badly. The\n> documentation given in the commit message is actually wrong (I added the\n> clean bits to the patch later, and failed to realize the dependency or\n> update the commit message).\n>\n> Also, our usual idiom is to check the prerequisites at the top of the\n> script and bail immediately.\n>\n> So maybe the whole script should be restructured as:\n>\n>    if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n>            skip_all=\"GIT_TEST_CREDENTIAL_HELPER not set\"\n>            test_done\n>    fi\n>\n>    pre_test\n>    helper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n>    if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n>            say \"# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n>    else\n>            helper_test_timeout \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"\n>    fi\n>    post_test\nYeah, this seems to be the right approach. I'll repost with a proper \ncommit message once my doubts about the tests are cleared up.\n\n-\nZbyszek\n"},{"id":"186907","messageId":"20120313213850.GB27752@sigill.intra.peff.net","threadId":"29916","inReplyTo":"20120312204340.GA10661@burratino","subject":"Re: [PATCH 2/2] t0303: resurrect commit message as test documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-13T21:38:50Z","receivedAt":"2012-03-13T21:38:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 12, 2012 at 03:43:40PM -0500, Jonathan Nieder wrote:\n\n> > +# Test harness for external credential helpers\n> > +#\n> > +# This is a tool for authors of external helper tools to sanity-check\n> > +# their helpers. If you have written the \"git-credential-foo\" helper,\n> > +# you check it with:\n> > +#\n> > +# GIT_TEST_CREDENTIAL_HELPER=foo make t0303-credential-external.sh\n> > +#\n> > +# This assumes that your helper is capable of both storing and\n> > +# retrieving credentials (some helpers may be read-only, and\n> > +# they will fail these tests).\n> > +#\n> > +# If your helper supports time-based expiration with a\n> > +# configurable timeout, you can test that feature with:\n> > +#\n> > +#  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n> > +#      make t0303-credential-external.sh\n> > +\n> >  test_description='external credential helper tests'\n> \n> Nice idea, but shouldn't this description be in test_description so I\n> can view it by running \"sh t0303-credential-external.sh --help\"?\n\nYes, that makes sense. I didn't even know that \"--help\" printed out the\ntest description; most of our descriptions are not very useful, so I\nnever bothered. But this is the perfect thing to put in there.\n\n-Peff\n"},{"id":"186908","messageId":"20120313215331.GC27752@sigill.intra.peff.net","threadId":"29916","inReplyTo":"4F5E65AE.8050401@in.waw.pl","subject":"Re: [PATCH 1/2] t0303: set reason for skipping tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-13T21:53:32Z","receivedAt":"2012-03-13T21:53:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 12, 2012 at 10:07:58PM +0100, Zbigniew Jędrzejewski-Szmek wrote:\n\n> OK, it seems this might be more complicated than I expected. I admit\n> that I didn't test this (apart from failing without the variables\n> defined) and assumed that it more or less works already.\n\nThis script is not very well tested, as it is meant to be run manually\nwhen testing an out-of-tree helper. I used it to test the osx-keychain\nhelper, but that's it.\n\n> I think that the tests are not very robust:\n>     ln -s /bin/true ~/bin/git-credential-fooooooo\n>     GIT_TEST_CREDENTIAL_HELPER=fooooooo\\\n>       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=fooooooo\\\n>       ./t0303-credential-external.sh\n> \n> gives me:\n> ok 1 - helper (fooooooo) has no existing data\n> ok 2 - helper (fooooooo) stores password\n> not ok - 3 helper (fooooooo) can retrieve password\n> ok 4 - helper (fooooooo) requires matching protocol\n> ok 5 - helper (fooooooo) requires matching host\n> ok 6 - helper (fooooooo) requires matching username\n> ok 7 - helper (fooooooo) requires matching path\n> ok 8 - helper (fooooooo) can forget host\n> not ok - 9 helper (fooooooo) can store multiple users\n> ok 10 - helper (fooooooo) can forget user\n> not ok - 11 helper (fooooooo) remembers other user\n> ok 12 - helper (fooooooo) times out\n> # failed 3 among 12 test(s)\n> 1..12\n> \n> I guess that the fact that #1 succeeds reflects reality, but e.g.\n> 4-7 and 12 probably should fail.\n\nThe reason is that the individual tests do not verify all of the\npreconditions themselves, but rather build on each other. So in test 2,\nwe ask to store some data. The helper tells us it did so successfully\n(which is a lie, of course). And then in test 3 we ask it to tell us\nwhat it stored, but of course it can't, and we notice. And then in test\n4, we ask again with a restricted query, expecting to see no answer. And\nwe get it, because of course, the helper will never give us an answer.\nIf you really wanted to know whether that feature worked, you would\ncheck that we can get anything at all, but not with the restricted query.\n\nIn an ideal world, each test snippet would be totally independent and\ncheck its preconditions. That would give us an accurate count of how\nmany tests actually passed or failed. But fundamentally we only care\nabout \"did they all succeed or not?\", which the current script does tell\nus (either test 2 fails, or if it succeeds, then we have checked the\nprecondition for test 4). And the tests end up way shorter, because we\ndon't repeat the preconditions over and over.\n\nIf you want to try to make the tests more robust, you can (for example,\nyou can tighten the precondition on 4 to check \"does it give the right\nanswer with the right protocol\" instead of just \"does it ever give us\nthe right answer\"). But personally, I'm not sure it's worth that much\neffort.\n\n> >Should they actually say \"# SKIP ...\" to tell prove what's going on? I\n> >don't know very much about TAP.\n> # SKIP is used when skipping individual tests (IIUC), but when we\n> skip a group of tests, we simply jump over them and this message is\n> purely informative output that is not interpreted by the harness.\n\nJust looking at test-lib.sh, it seems like we output \"# SKIP\" when we do\nskip_all. But I think you would have to give a count of which tests you\nskipped (e.g., try \"./t5541-http-push.sh\" to see its TAP output). Which\nmeans when skipping a subset, you'd have to deal with test numbering,\nwhich is a pain. So it's probably not worth worrying about.\n\n-Peff\n"},{"id":"186943","messageId":"20120314141401.GC28595@in.waw.pl","threadId":"29916","inReplyTo":"20120313215331.GC27752@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0303: set reason for skipping tests","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-14T14:14:01Z","receivedAt":"2012-03-14T14:14:01Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On Tue, Mar 13, 2012 at 05:53:32PM -0400, Jeff King wrote:\n> The reason is that the individual tests do not verify all of the\n> preconditions themselves, but rather build on each other.\nRight. I added a note based on this sentence in the test description.\n\n> In an ideal world, each test snippet would be totally independent and\n> check its preconditions. That would give us an accurate count of how\n> many tests actually passed or failed. But fundamentally we only care\n> about \"did they all succeed or not?\", which the current script does tell\n> us (either test 2 fails, or if it succeeds, then we have checked the\n> precondition for test 4). And the tests end up way shorter, because we\n> don't repeat the preconditions over and over.\n> \n> If you want to try to make the tests more robust, you can (for example,\n> you can tighten the precondition on 4 to check \"does it give the right\n> answer with the right protocol\" instead of just \"does it ever give us\n> the right answer\"). But personally, I'm not sure it's worth that much\n> effort.\nYeah.\n\n> > >Should they actually say \"# SKIP ...\" to tell prove what's going on? I\n> > >don't know very much about TAP.\n> > # SKIP is used when skipping individual tests (IIUC), but when we\n> > skip a group of tests, we simply jump over them and this message is\n> > purely informative output that is not interpreted by the harness.\n> \n> Just looking at test-lib.sh, it seems like we output \"# SKIP\" when we do\n> skip_all. But I think you would have to give a count of which tests you\n> skipped (e.g., try \"./t5541-http-push.sh\" to see its TAP output). Which\n> means when skipping a subset, you'd have to deal with test numbering,\n> which is a pain. So it's probably not worth worrying about.\nSkipped test numbering could done automatically by using test prereqs,\nbut (after actually doing that and discarding) I agree that it isn't\nworth the trouble.\n\n\nJonathan Nieder wrote:\n> Nice idea, but shouldn't this description be in test_description so I\n> can view it by running \"sh t0303-credential-external.sh --help\"?\nDone.\n\nUpdated patches follow.\n\n(This time I tested with GIT_TEST_CREDENTIAL_HELPER=cache\nGIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"cache --timeout=1,3\" and things\nseem to work as expected.)\n\nZbyszek\n"},{"id":"186944","messageId":"1331734704-14281-1-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"20120314141401.GC28595@in.waw.pl","subject":"[PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-14T14:18:23Z","receivedAt":"2012-03-14T14:18:23Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be\nconfigured to do something sensible. If it is not set, prove will say:\n  ./t0303-credential-external.sh .. skipped: (no reason given)\nwhich isn't very nice.\n\nUse skip_all=\"...\" && test_done to bail out immediately and provide a\nnicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the\ntimeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t0303-credential-external.sh |   40 ++++++++++++++++------------------------\n 1 files changed, 16 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 267f4c8..4479bf8 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -4,36 +4,28 @@ test_description='external credential helper tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-pre_test() {\n-\ttest -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n-\teval \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\"\n-\n-\t# clean before the test in case there is cruft left\n-\t# over from a previous run that would impact results\n-\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n-}\n-\n-post_test() {\n-\t# clean afterwards so that we are good citizens\n-\t# and don't leave cruft in the helper's storage, which\n-\t# might be long-term system storage\n-\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n-}\n-\n if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n-\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n-else\n-\tpre_test\n-\thelper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n-\tpost_test\n+\tskip_all=\"used to test external credential helpers\"\n+\ttest_done\n fi\n \n+$GIT_TEST_CREDENTIAL_HELPER_SETUP\n+\n+# clean before the test in case there is cruft left\n+# over from a previous run that would impact results\n+helper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n+\n+helper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n+\n if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n-\tsay \"# skipping external helper timeout tests\"\n+\tsay \"# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n else\n-\tpre_test\n \thelper_test_timeout \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"\n-\tpost_test\n fi\n \n+# clean afterwards so that we are good citizens\n+# and don't leave cruft in the helper's storage, which\n+# might be long-term system storage\n+helper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n+\n test_done\n-- \n1.7.9.1\n"},{"id":"186945","messageId":"1331734704-14281-2-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"1331734704-14281-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH v2 2/2] t0303: resurrect commit message as test documentation","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-14T14:18:24Z","receivedAt":"2012-03-14T14:18:24Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"The commit message which added those tests (861444f 't: add test\nharness for external credential helpers' 2011-12-10) provided nice\ndocumentation in the commit message. Let's make it more visible\nby putting it in the test description.\n\nThe documentation is updated to reflect the fact that\nGIT_TEST_CREDENTIAL_HELPER must be set for\nGIT_TEST_CREDENTIAL_HELPER_TIMEOUT to be used\nand GIT_TEST_CREDENTIAL_HELPER_SETUP can be used.\n\nBased-on-commit-message-by: Jeff King <peff@peff.net>\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n---\n t/t0303-credential-external.sh |   30 +++++++++++++++++++++++++++++-\n 1 files changed, 29 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 4479bf8..136da1e 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -1,6 +1,34 @@\n #!/bin/sh\n \n-test_description='external credential helper tests'\n+test_description='external credential helper tests\n+\n+This is a tool for authors of external helper tools to sanity-check\n+their helpers. If you have written the \"git-credential-foo\" helper,\n+you check it with:\n+\n+  make GIT_TEST_CREDENTIAL_HELPER=foo t0303-credential-external.sh\n+\n+This assumes that your helper is capable of both storing and\n+retrieving credentials (some helpers may be read-only, and they will\n+fail these tests).\n+\n+Please note that the individual tests do not verify all of the\n+preconditions themselves, but rather build on each other. A failing\n+test means that tests later in the sequence can return false \"OK\"\n+results.\n+\n+If your helper supports time-based expiration with a configurable\n+timeout, you can test that feature with:\n+\n+  make GIT_TEST_CREDENTIAL_HELPER=foo \\\n+       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n+       t0303-credential-external.sh\n+\n+If your helper requires additional setup before the tests are started,\n+you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell\n+commands.\n+'\n+\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-- \n1.7.9.1\n"},{"id":"186998","messageId":"7v8vj2omiv.fsf@alter.siamese.dyndns.org","threadId":"29916","inReplyTo":"1331734704-14281-1-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-14T22:17:28Z","receivedAt":"2012-03-14T22:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:\n\n> t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be\n> configured to do something sensible. If it is not set, prove will say:\n>   ./t0303-credential-external.sh .. skipped: (no reason given)\n> which isn't very nice.\n>\n> Use skip_all=\"...\" && test_done to bail out immediately and provide a\n> nicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the\n> timeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.\n>\n> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\n> ---\n>  t/t0303-credential-external.sh |   40 ++++++++++++++++------------------------\n>  1 files changed, 16 insertions(+), 24 deletions(-)\n>\n> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\n> index 267f4c8..4479bf8 100755\n> --- a/t/t0303-credential-external.sh\n> +++ b/t/t0303-credential-external.sh\n> @@ -4,36 +4,28 @@ test_description='external credential helper tests'\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY\"/lib-credential.sh\n>  \n> -pre_test() {\n> -\ttest -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n> -\teval \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\"\n> -\n> -\t# clean before the test in case there is cruft left\n> -\t# over from a previous run that would impact results\n> -\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n> -}\n> -\n> -post_test() {\n> -\t# clean afterwards so that we are good citizens\n> -\t# and don't leave cruft in the helper's storage, which\n> -\t# might be long-term system storage\n> -\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n> -}\n> -\n>  if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n> -\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n> -else\n> -\tpre_test\n> -\thelper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n> -\tpost_test\n> +\tskip_all=\"used to test external credential helpers\"\n> +\ttest_done\n>  fi\n>  \n> +$GIT_TEST_CREDENTIAL_HELPER_SETUP\n\nThis used to be 'test -z \"$it\" || eval \"$it\"'; doesn't it make a\ndifference?\n\nWhat is the value expected to be in this variable?  Nobody seems to set it\nin our codebase, so I cannot say \"with the current code, this rewrite is\nsafe\" or anything like that.\n\nThis is probably not related to your patch, but\n\n\tGIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh\n\npasses OK for me while\n\n\tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n\nseems to get stuck forever.\n"},{"id":"187006","messageId":"20120315035405.GA4149@sigill.intra.peff.net","threadId":"29916","inReplyTo":"7v8vj2omiv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-15T03:54:05Z","receivedAt":"2012-03-15T03:54:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:\n\n> > +$GIT_TEST_CREDENTIAL_HELPER_SETUP\n> \n> This used to be 'test -z \"$it\" || eval \"$it\"'; doesn't it make a\n> difference?\n>\n> What is the value expected to be in this variable?  Nobody seems to set it\n> in our codebase, so I cannot say \"with the current code, this rewrite is\n> safe\" or anything like that.\n\nI think eval is a better route, as it gives the caller more flexibility\nabout what shell code to run. The only use is here:\n\n  http://article.gmane.org/gmane.comp.version-control.git/186757\n\nwhich does work either way.\n\n> This is probably not related to your patch, but\n> \n> \tGIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh\n> \n> passes OK for me while\n> \n> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n> \n> seems to get stuck forever.\n\nIt's because t0303 is the generic \"test any helper\" script, and does not\nknow how to clean up the credential-cache daemon. So the daemon sticks\naround, holding onto a file descriptor that causes prove to hang.\nIf you look at t0301 (which runs the same tests on credential-cache), we\nkill the resulting daemon explicitly. t0303 could learn hooks to do\nthis, but I didn't bother, as I didn't need them for testing the\nosxkeychain helper (which is the only thing I've used t0303 for, as\nt0301 and t0302 cover the in-tree helpers). I figured that somebody\ncould add the hooks easily if and when they needed.\n\n-Peff\n"},{"id":"187007","messageId":"20120315035642.GB4149@sigill.intra.peff.net","threadId":"29916","inReplyTo":"1331734704-14281-1-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-15T03:56:42Z","receivedAt":"2012-03-15T03:56:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 14, 2012 at 03:18:23PM +0100, Zbigniew Jędrzejewski-Szmek wrote:\n\n> -pre_test() {\n> -\ttest -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n> -\teval \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\"\n> [...]\n> +$GIT_TEST_CREDENTIAL_HELPER_SETUP\n\nExcept for losing the eval here that Junio mentioned, I think both\npatches look good.\n\n-Peff\n"},{"id":"187019","messageId":"7vk42ml5er.fsf@alter.siamese.dyndns.org","threadId":"29916","inReplyTo":"20120315035405.GA4149@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T06:55:24Z","receivedAt":"2012-03-15T06:55:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:\n>> This is probably not related to your patch, but\n>> \n>> \tGIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh\n>> \n>> passes OK for me while\n>> \n>> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n>> \n>> seems to get stuck forever.\n>\n> It's because t0303 is the generic \"test any helper\" script, and does not\n> know how to clean up the credential-cache daemon. So the daemon sticks\n> around, holding onto a file descriptor that causes prove to hang.\n\nAnd the reason why \"sh t0303-*.sh\" version does not have this problem is...?\n"},{"id":"187041","messageId":"4F61C828.8060506@in.waw.pl","threadId":"29916","inReplyTo":"7vk42ml5er.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-15T10:44:56Z","receivedAt":"2012-03-15T10:44:56Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 03/15/2012 07:55 AM, Junio C Hamano wrote:\n> Jeff King<peff@peff.net>  writes:\n>\n>> On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:\n>>> This is probably not related to your patch, but\n>>>\n>>> \tGIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh\n>>>\n>>> passes OK for me while\n>>>\n>>> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n>>>\n>>> seems to get stuck forever.\n>>\n>> It's because t0303 is the generic \"test any helper\" script, and does not\n>> know how to clean up the credential-cache daemon. So the daemon sticks\n>> around, holding onto a file descriptor that causes prove to hang.\n>\n> And the reason why \"sh t0303-*.sh\" version does not have this problem is...?\n\nIt does :)\n\nIn both cases git-credential-cache--daemon is running. It is stuck in \npoll() with a timeout of 30*1000 ms (credentail-cache--daemon.c:175). \nWhen running without prove, it is left in the background and terminates \nafter 30 s. When running under prove, prove waits for 30 seconds for the \nprocess to end and then terminates.\n\nI think that this delay is OK, as it happens only when running an \nexplicitly requested test, and only under prove.\n\nZbyszek\n"},{"id":"187042","messageId":"1331809681-26113-1-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"4F61C828.8060506@in.waw.pl","subject":"[PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-15T11:08:00Z","receivedAt":"2012-03-15T11:08:00Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"t0300-credential-helpers.sh requires GIT_TEST_CREDENTIAL_HELPER to be\nconfigured to do something sensible. If it is not set, prove will say:\n  ./t0303-credential-external.sh .. skipped: (no reason given)\nwhich isn't very nice.\n\nUse skip_all=\"...\" && test_done to bail out immediately and provide a\nnicer message. In case GIT_TEST_CREDENTIAL_HELPER is set, but the\ntimeout tests are skipped, mention GIT_TEST_CREDENTIAL_HELPER_TIMEOUT.\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\nAcked-by: Jeff King <peff@peff.net>\n---\nThis is v3: the only change is the removal of the removal of the eval around\n$GIT_TEST_CREDENTIAL_HELPER_SETUP.\n\n t/t0303-credential-external.sh |   39 ++++++++++++++++-----------------------\n 1 file changed, 16 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 267f4c8..e771075 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -4,36 +4,29 @@ test_description='external credential helper tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-pre_test() {\n-\ttest -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n-\teval \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\"\n+if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n+\tskip_all=\"used to test external credential helpers\"\n+\ttest_done\n+fi\n \n-\t# clean before the test in case there is cruft left\n-\t# over from a previous run that would impact results\n-\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n-}\n+test -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n+\teval \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\"\n \n-post_test() {\n-\t# clean afterwards so that we are good citizens\n-\t# and don't leave cruft in the helper's storage, which\n-\t# might be long-term system storage\n-\thelper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n-}\n+# clean before the test in case there is cruft left\n+# over from a previous run that would impact results\n+helper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n \n-if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n-\tsay \"# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)\"\n-else\n-\tpre_test\n-\thelper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n-\tpost_test\n-fi\n+helper_test \"$GIT_TEST_CREDENTIAL_HELPER\"\n \n if test -z \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"; then\n-\tsay \"# skipping external helper timeout tests\"\n+\tsay \"# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)\"\n else\n-\tpre_test\n \thelper_test_timeout \"$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\"\n-\tpost_test\n fi\n \n+# clean afterwards so that we are good citizens\n+# and don't leave cruft in the helper's storage, which\n+# might be long-term system storage\n+helper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n+\n test_done\n-- \n1.7.10.rc0.160.g12d89\n"},{"id":"187043","messageId":"1331809681-26113-2-git-send-email-zbyszek@in.waw.pl","threadId":"29916","inReplyTo":"1331809681-26113-1-git-send-email-zbyszek@in.waw.pl","subject":"[PATCH v3 2/2] t0303: resurrect commit message as test documentation","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-15T11:08:01Z","receivedAt":"2012-03-15T11:08:01Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"The commit message which added those tests (861444f 't: add test\nharness for external credential helpers' 2011-12-10) provided nice\ndocumentation in the commit message. Let's make it more visible\nby putting it in the test description.\n\nThe documentation is updated to reflect the fact that\nGIT_TEST_CREDENTIAL_HELPER must be set for\nGIT_TEST_CREDENTIAL_HELPER_TIMEOUT to be used\nand GIT_TEST_CREDENTIAL_HELPER_SETUP can be used.\n\nBased-on-commit-message-by: Jeff King <peff@peff.net>\nSigned-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>\nAcked-by: Jeff King <peff@peff.net>\n---\n t/t0303-credential-external.sh |   30 +++++++++++++++++++++++++++++-\n 1 file changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex e771075..f028fd1 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -1,6 +1,34 @@\n #!/bin/sh\n \n-test_description='external credential helper tests'\n+test_description='external credential helper tests\n+\n+This is a tool for authors of external helper tools to sanity-check\n+their helpers. If you have written the \"git-credential-foo\" helper,\n+you check it with:\n+\n+  make GIT_TEST_CREDENTIAL_HELPER=foo t0303-credential-external.sh\n+\n+This assumes that your helper is capable of both storing and\n+retrieving credentials (some helpers may be read-only, and they will\n+fail these tests).\n+\n+Please note that the individual tests do not verify all of the\n+preconditions themselves, but rather build on each other. A failing\n+test means that tests later in the sequence can return false \"OK\"\n+results.\n+\n+If your helper supports time-based expiration with a configurable\n+timeout, you can test that feature with:\n+\n+  make GIT_TEST_CREDENTIAL_HELPER=foo \\\n+       GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=\"foo --timeout=1\" \\\n+       t0303-credential-external.sh\n+\n+If your helper requires additional setup before the tests are started,\n+you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell\n+commands.\n+'\n+\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-- \n1.7.10.rc0.160.g12d89\n"},{"id":"187045","messageId":"20120315132428.GB8467@sigill.intra.peff.net","threadId":"29916","inReplyTo":"7vk42ml5er.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-15T13:24:28Z","receivedAt":"2012-03-15T13:24:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 14, 2012 at 11:55:24PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Mar 14, 2012 at 03:17:28PM -0700, Junio C Hamano wrote:\n> >> This is probably not related to your patch, but\n> >> \n> >> \tGIT_TEST_CREDENTIAL_HELPER=cache sh t0303-*.sh\n> >> \n> >> passes OK for me while\n> >> \n> >> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n> >> \n> >> seems to get stuck forever.\n> >\n> > It's because t0303 is the generic \"test any helper\" script, and does not\n> > know how to clean up the credential-cache daemon. So the daemon sticks\n> > around, holding onto a file descriptor that causes prove to hang.\n> \n> And the reason why \"sh t0303-*.sh\" version does not have this problem is...?\n\nMost helpers don't spawn a daemon that hangs around (and if they do, the\ninstructions for killing said daemon are outside the scope of the helper\nprotocol -- though I would recommend having an \"exit\" command, as\ncredential-cache has). You could add something like:\n\n  GIT_TEST_CREDENTIAL_HELPER='cache' \\\n  GIT_TEST_CREDENTIAL_HELPER_EXIT='git credential-cache exit' \\\n  ./t0303-*\n\nBut like I said, I didn't bother. If you are testing credential-cache,\nthen use t0301, which handles this. If you are testing something\nexternal, use t0303. My external testing didn't require such an exit\nhook, so I didn't bother with it. If somebody writes a helper that\nrequires such a hook, they can add it then. I didn't want to get into\nthe business of guessing which hooks people might need (and it is not as\nif these tests are an end-user visible piece of code; they are purely a\nconvenience for developers to test their implementations against the\nsame battery of tests that credential-cache and credential-store use).\n\n-Peff\n"},{"id":"187046","messageId":"20120315132642.GA8945@sigill.intra.peff.net","threadId":"29916","inReplyTo":"20120315132428.GB8467@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-15T13:26:43Z","receivedAt":"2012-03-15T13:26:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2012 at 09:24:28AM -0400, Jeff King wrote:\n\n> > >> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n> > >> \n> > >> seems to get stuck forever.\n> > >\n> > > It's because t0303 is the generic \"test any helper\" script, and does not\n> > > know how to clean up the credential-cache daemon. So the daemon sticks\n> > > around, holding onto a file descriptor that causes prove to hang.\n> > \n> > And the reason why \"sh t0303-*.sh\" version does not have this problem is...?\n> [long-winded explanation from me]\n\nOops. I read this as \"why does t0301 not have the problem?\". So ignore\neverything I said.\n\nThe reason why running it via sh works is that we leave the daemon\nrunning in both cases, but only prove actually cares about the leaked\nfile descriptor and blocks.\n\n-Peff\n"},{"id":"187049","messageId":"7vobrxkb3m.fsf@alter.siamese.dyndns.org","threadId":"29916","inReplyTo":"20120315132642.GA8945@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T17:50:05Z","receivedAt":"2012-03-15T17:50:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 15, 2012 at 09:24:28AM -0400, Jeff King wrote:\n>\n>> > >> \tmake GIT_TEST_CREDENTIAL_HELPER=cache T=t0303-*.sh prove\n>> > >> \n>> > >> seems to get stuck forever.\n>> > >\n>> > > It's because t0303 is the generic \"test any helper\" script, and does not\n>> > > know how to clean up the credential-cache daemon. So the daemon sticks\n>> > > around, holding onto a file descriptor that causes prove to hang.\n>> > \n>> > And the reason why \"sh t0303-*.sh\" version does not have this problem is...?\n>> [long-winded explanation from me]\n>\n> Oops. I read this as \"why does t0301 not have the problem?\". So ignore\n> everything I said.\n>\n> The reason why running it via sh works is that we leave the daemon\n> running in both cases, but only prove actually cares about the leaked\n> file descriptor and blocks.\n\nAh, OK, the last part was what I was missing.  Thanks for a clarification.\n"},{"id":"187050","messageId":"7vk42lkb0s.fsf@alter.siamese.dyndns.org","threadId":"29916","inReplyTo":"1331809681-26113-1-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH v3 1/2] t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T17:51:47Z","receivedAt":"2012-03-15T17:51:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks all; will queue.\n"}]}