{"thread":{"id":"12389","subject":"[PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","startedAt":"2008-02-29T22:23:25Z","lastAt":"2008-03-01T05:40:39Z","messageCount":12,"participants":["Mike Hommey","Jeff King","Junio C Hamano","Johannes Schindelin","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70435","messageId":"1204323805-23185-1-git-send-email-mh@glandium.org","threadId":"12389","inReplyTo":null,"subject":"[PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-02-29T22:23:25Z","receivedAt":"2008-02-29T22:23:25Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n t/t6024-recursive-merge.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\nindex 149ea85..43b5f15 100755\n--- a/t/t6024-recursive-merge.sh\n+++ b/t/t6024-recursive-merge.sh\n@@ -81,7 +81,7 @@ EOF\n \n test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n \n-git reset --hard\n+git reset --hard >&3 2>&4\n test_expect_success 'refuse to merge binary files' '\n \tprintf \"\\0\" > binary-file &&\n \tgit add binary-file &&\n-- \n1.5.4.3.340.g97b97.dirty\n\n"},{"id":"70440","messageId":"20080229225331.GA8440@coredump.intra.peff.net","threadId":"12389","inReplyTo":"1204323805-23185-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-29T22:53:31Z","receivedAt":"2008-02-29T22:53:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:\n\n>  test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n>  \n> -git reset --hard\n> +git reset --hard >&3 2>&4\n>  test_expect_success 'refuse to merge binary files' '\n>  \tprintf \"\\0\" > binary-file &&\n>  \tgit add binary-file &&\n\nShould this perhaps just be:\n\n  test_expect_success 'reset state' 'git reset --hard'\n\n?\n\n-Peff\n"},{"id":"70441","messageId":"20080229225816.GA24892@glandium.org","threadId":"12389","inReplyTo":"20080229225331.GA8440@coredump.intra.peff.net","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-02-29T22:58:16Z","receivedAt":"2008-02-29T22:58:16Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Feb 29, 2008 at 05:53:31PM -0500, Jeff King wrote:\n> On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:\n> \n> >  test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n> >  \n> > -git reset --hard\n> > +git reset --hard >&3 2>&4\n> >  test_expect_success 'refuse to merge binary files' '\n> >  \tprintf \"\\0\" > binary-file &&\n> >  \tgit add binary-file &&\n> \n> Should this perhaps just be:\n> \n>   test_expect_success 'reset state' 'git reset --hard'\n\nIs it really about testing git reset ?\n\nMike\n"},{"id":"70443","messageId":"20080229230156.GA8742@coredump.intra.peff.net","threadId":"12389","inReplyTo":"20080229225816.GA24892@glandium.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-29T23:01:56Z","receivedAt":"2008-02-29T23:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 11:58:16PM +0100, Mike Hommey wrote:\n\n> > > -git reset --hard\n> > > +git reset --hard >&3 2>&4\n> > >  test_expect_success 'refuse to merge binary files' '\n> > >  \tprintf \"\\0\" > binary-file &&\n> > >  \tgit add binary-file &&\n> > \n> > Should this perhaps just be:\n> > \n> >   test_expect_success 'reset state' 'git reset --hard'\n> \n> Is it really about testing git reset ?\n\nNo, of course not. Nor is t0003 about testing 'setup'. But it is a\nconvention[1] in git tests to do everything inside a test_expect_success\nwrapper, which is beneficial because:\n\n  - you get your output handled properly automagically :)\n  - if the setup step fails, we notice. Which means:\n     - it cannot silently cause a later test to succeed (e.g., setup\n       changes a file, then the real test reverts it)\n     - it is easier to debug failing tests, because you are notified\n       of the _first_ failure\n\n-Peff\n\n[1] By convention, I don't necessarily mean that there is a written\nrule, nor that we always do it that way, but only that in my experience\nwe usually do. Somebody like Junio could say more definitely what is\npreferred.\n\n"},{"id":"70444","messageId":"7vfxvbbbhn.fsf@gitster.siamese.dyndns.org","threadId":"12389","inReplyTo":"20080229225816.GA24892@glandium.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-29T23:15:32Z","receivedAt":"2008-02-29T23:15:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> On Fri, Feb 29, 2008 at 05:53:31PM -0500, Jeff King wrote:\n>> On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:\n>> \n>> >  test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n>> >  \n>> > -git reset --hard\n>> > +git reset --hard >&3 2>&4\n>> >  test_expect_success 'refuse to merge binary files' '\n>> >  \tprintf \"\\0\" > binary-file &&\n>> >  \tgit add binary-file &&\n>> \n>> Should this perhaps just be:\n>> \n>>   test_expect_success 'reset state' 'git reset --hard'\n>\n> Is it really about testing git reset ?\n\nNo, it is about setting the stage to perform the test in\nquestion, so it belongs inside.\n"},{"id":"70447","messageId":"alpine.LSU.1.00.0802292334040.22527@racer.site","threadId":"12389","inReplyTo":"1204323805-23185-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-29T23:34:41Z","receivedAt":"2008-02-29T23:34:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 29 Feb 2008, Mike Hommey wrote:\n\n> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\n> index 149ea85..43b5f15 100755\n> --- a/t/t6024-recursive-merge.sh\n> +++ b/t/t6024-recursive-merge.sh\n> @@ -81,7 +81,7 @@ EOF\n>  \n>  test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n>  \n> -git reset --hard\n> +git reset --hard >&3 2>&4\n>  test_expect_success 'refuse to merge binary files' '\n\nWould it not be _much_ more sensible to pull that command _into_ the \ntest_expect_success?\n\nCiao,\nDscho\n"},{"id":"70449","messageId":"7v1w6vb9w4.fsf@gitster.siamese.dyndns.org","threadId":"12389","inReplyTo":"alpine.LSU.1.00.0802292334040.22527@racer.site","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-29T23:50:03Z","receivedAt":"2008-02-29T23:50:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi,\n>\n> On Fri, 29 Feb 2008, Mike Hommey wrote:\n>\n>> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\n>> index 149ea85..43b5f15 100755\n>> --- a/t/t6024-recursive-merge.sh\n>> +++ b/t/t6024-recursive-merge.sh\n>> @@ -81,7 +81,7 @@ EOF\n>>  \n>>  test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n>>  \n>> -git reset --hard\n>> +git reset --hard >&3 2>&4\n>>  test_expect_success 'refuse to merge binary files' '\n>\n> Would it not be _much_ more sensible to pull that command _into_ the \n> test_expect_success?\n\nActually, I think this might be a bit more sensible approach.\n\n-- >8 --\ntests: allow optional clean-up phrase to expect_success/failure\n\nWhen one test modifies the state of the test repository that the later\ntests may depend on, you may want to add a clean-up action that is run\nregardless of the outcome of the main part of the test.\n\nThis can now be specified as the third parameter to test_expect_success\nand test_expect_failure functions.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n t/README                   |   13 +++++++++++--\n t/t6024-recursive-merge.sh |    5 +++--\n t/test-lib.sh              |   32 ++++++++++++++++++++++++--------\n 3 files changed, 38 insertions(+), 12 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 73ed11b..de79c00 100644\n--- a/t/README\n+++ b/t/README\n@@ -146,7 +146,7 @@ Test harness library\n There are a handful helper functions defined in the test harness\n library for your script to use.\n \n- - test_expect_success <message> <script>\n+ - test_expect_success <message> <script> [<cleanup>]\n \n    This takes two strings as parameter, and evaluates the\n    <script>.  If it yields success, test is considered\n@@ -158,7 +158,12 @@ library for your script to use.\n \t    'git-write-tree should be able to write an empty tree.' \\\n \t    'tree=$(git-write-tree)'\n \n- - test_expect_failure <message> <script>\n+   An optional <cleanup> is executed and can be used to clean-up\n+   the state this test modifies (or leaves half-modified when it\n+   fails).  The clean-up script is not run if test fails and the\n+   test script is run under --immediate mode to help postmortem.\n+\n+ - test_expect_failure <message> <script> [<cleanup>]\n \n    This is NOT the opposite of test_expect_success, but is used\n    to mark a test that demonstrates a known breakage.  Unlike\n@@ -167,6 +172,10 @@ library for your script to use.\n    success and \"still broken\" on failure.  Failures from these\n    tests won't cause -i (immediate) to stop.\n \n+   An optional <cleanup> is executed and can be used to clean-up\n+   the state this test modifies (or leaves half-modified when it\n+   fails).\n+\n  - test_debug <script>\n \n    This takes a single argument, <script>, and evaluates it only\ndiff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\nindex 149ea85..ae9706f 100755\n--- a/t/t6024-recursive-merge.sh\n+++ b/t/t6024-recursive-merge.sh\n@@ -79,9 +79,10 @@ cat > expect << EOF\n 100644 fd7923529855d0b274795ae3349c5e0438333979 3\ta1\n EOF\n \n-test_expect_success \"virtual trees were processed\" \"git diff expect out\"\n+test_expect_success \"virtual trees were processed\" \"\n+\tgit diff expect out\n+\" 'git reset --hard'\n \n-git reset --hard\n test_expect_success 'refuse to merge binary files' '\n \tprintf \"\\0\" > binary-file &&\n \tgit add binary-file &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 68efda4..0fb2c98 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -220,9 +220,13 @@ test_skip () {\n }\n \n test_expect_failure () {\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 parameters to test-expect-failure\"\n-\tif ! test_skip \"$@\"\n+\tcase $# in\n+\t2) ;; # traditional\n+\t3) ;; # with clean-up\n+\t*)\n+\t\terror \"bug in the test script\" ;;\n+\tesac\n+\tif ! test_skip \"$1\" \"$2\"\n \tthen\n \t\tsay >&3 \"checking known breakage: $2\"\n \t\ttest_run_ \"$2\"\n@@ -230,16 +234,24 @@ test_expect_failure () {\n \t\tthen\n \t\t\ttest_known_broken_ok_ \"$1\"\n \t\telse\n-\t\t    test_known_broken_failure_ \"$1\"\n+\t\t\ttest_known_broken_failure_ \"$1\"\n+\t\tfi\n+\t\tif test -n \"$3\"\n+\t\tthen\n+\t\t\teval >&3 2>&4 \"$3\"\n \t\tfi\n \tfi\n \techo >&3 \"\"\n }\n \n test_expect_success () {\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 parameters to test-expect-success\"\n-\tif ! test_skip \"$@\"\n+\tcase $# in\n+\t2) ;; # traditional\n+\t3) ;; # with clean-up\n+\t*)\n+\t\terror \"bug in the test script\" ;;\n+\tesac\n+\tif ! test_skip \"$1\" \"$2\"\n \tthen\n \t\tsay >&3 \"expecting success: $2\"\n \t\ttest_run_ \"$2\"\n@@ -247,7 +259,11 @@ test_expect_success () {\n \t\tthen\n \t\t\ttest_ok_ \"$1\"\n \t\telse\n-\t\t\ttest_failure_ \"$@\"\n+\t\t\ttest_failure_ \"$1\" \"$2\"\n+\t\tfi\n+\t\tif test -n \"$3\"\n+\t\tthen\n+\t\t\teval >&3 2>&4 \"$3\"\n \t\tfi\n \tfi\n \techo >&3 \"\"\n"},{"id":"70450","messageId":"20080229235442.GA20028@sigill.intra.peff.net","threadId":"12389","inReplyTo":"7vfxvbbbhn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-29T23:54:43Z","receivedAt":"2008-02-29T23:54:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 03:15:32PM -0800, Junio C Hamano wrote:\n\n> >> >  test_expect_success 'refuse to merge binary files' '\n> >> >  \tprintf \"\\0\" > binary-file &&\n> >> >  \tgit add binary-file &&\n> >> \n> >> Should this perhaps just be:\n> >> \n> >>   test_expect_success 'reset state' 'git reset --hard'\n> >\n> > Is it really about testing git reset ?\n> \n> No, it is about setting the stage to perform the test in\n> question, so it belongs inside.\n\nThat sounds reasonable to me.\n\n-Peff\n"},{"id":"70461","messageId":"20080301041005.GA8969@coredump.intra.peff.net","threadId":"12389","inReplyTo":"7v1w6vb9w4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-01T04:10:05Z","receivedAt":"2008-03-01T04:10:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 03:50:03PM -0800, Junio C Hamano wrote:\n\n> Actually, I think this might be a bit more sensible approach.\n> \n> -- >8 --\n> tests: allow optional clean-up phrase to expect_success/failure\n> \n> When one test modifies the state of the test repository that the later\n> tests may depend on, you may want to add a clean-up action that is run\n> regardless of the outcome of the main part of the test.\n> \n> This can now be specified as the third parameter to test_expect_success\n> and test_expect_failure functions.\n\nI think your heart is in the right place with this patch, but I doubt\nthat it is going to be all that productive in practice. Most tests\nconsist of a long list of commands, and cleaning up properly after every\npossible failure case is going to be a lot of work. And worse, since the\ntests generally _don't_ fail, you have no way to test that your cleanup\nis reasonable.\n\nSo I think we will end up in the case where a few failed tests will\nproperly clean themselves up and let further tests proceed, but most\nfailures will leave a big question. In other words, what problem have we\nsolved?  If tests N and N+k both fail, would you, even with this patch,\nsuspect N+k of actually failing, or would you first go and debug test N?\nIs that any different than what you do now?\n\n-Peff\n"},{"id":"70463","messageId":"20080301042739.GX8410@spearce.org","threadId":"12389","inReplyTo":"20080301041005.GA8969@coredump.intra.peff.net","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-03-01T04:27:39Z","receivedAt":"2008-03-01T04:27:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jeff King <peff@peff.net> wrote:\n> On Fri, Feb 29, 2008 at 03:50:03PM -0800, Junio C Hamano wrote:\n> \n> > Actually, I think this might be a bit more sensible approach.\n> > \n> > -- >8 --\n> > tests: allow optional clean-up phrase to expect_success/failure\n> > \n> > When one test modifies the state of the test repository that the later\n> > tests may depend on, you may want to add a clean-up action that is run\n> > regardless of the outcome of the main part of the test.\n> \n> I think your heart is in the right place with this patch, but I doubt\n> that it is going to be all that productive in practice. Most tests\n> consist of a long list of commands, and cleaning up properly after every\n> possible failure case is going to be a lot of work. And worse, since the\n> tests generally _don't_ fail, you have no way to test that your cleanup\n> is reasonable.\n> \n> So I think we will end up in the case where a few failed tests will\n> properly clean themselves up and let further tests proceed, but most\n> failures will leave a big question. In other words, what problem have we\n> solved?  If tests N and N+k both fail, would you, even with this patch,\n> suspect N+k of actually failing, or would you first go and debug test N?\n> Is that any different than what you do now?\n\nI agree with Jeff.  It is unnecessary complexity that won't be\ntested well enough to be worthwhile.\n\nThis is why when tests start to fail I just rerun the specific case\nwith \"-i -v\" and let the test stop on the first failure, *fix that\none case* and run again to see if anything else is going to barf.\n\n-- \nShawn.\n"},{"id":"70464","messageId":"20080301042822.GA14274@coredump.intra.peff.net","threadId":"12389","inReplyTo":"20080301042739.GX8410@spearce.org","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-01T04:28:22Z","receivedAt":"2008-03-01T04:28:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 11:27:39PM -0500, Shawn O. Pearce wrote:\n\n> This is why when tests start to fail I just rerun the specific case\n> with \"-i -v\" and let the test stop on the first failure, *fix that\n> one case* and run again to see if anything else is going to barf.\n\nYes, exactly. I assumed that everyone did that.\n\n-Peff\n"},{"id":"70476","messageId":"7vpruf80iw.fsf@gitster.siamese.dyndns.org","threadId":"12389","inReplyTo":"20080301042822.GA14274@coredump.intra.peff.net","subject":"Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-01T05:40:39Z","receivedAt":"2008-03-01T05:40:39Z","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 Fri, Feb 29, 2008 at 11:27:39PM -0500, Shawn O. Pearce wrote:\n>\n>> This is why when tests start to fail I just rerun the specific case\n>> with \"-i -v\" and let the test stop on the first failure, *fix that\n>> one case* and run again to see if anything else is going to barf.\n>\n> Yes, exactly. I assumed that everyone did that.\n\nLikewise.\n"}]}