[PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 t/t6024-recursive-merge.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 149ea85..43b5f15 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -81,7 +81,7 @@ EOF
 
 test_expect_success "virtual trees were processed" "git diff expect out"
 
-git reset --hard
+git reset --hard >&3 2>&4
 test_expect_success 'refuse to merge binary files' '
 	printf "\0" > binary-file &&
 	git add binary-file &&
-- 
1.5.4.3.340.g97b97.dirty

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Jeff King]

On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:

>  test_expect_success "virtual trees were processed" "git diff expect out"
>  
> -git reset --hard
> +git reset --hard >&3 2>&4
>  test_expect_success 'refuse to merge binary files' '
>  	printf "\0" > binary-file &&
>  	git add binary-file &&

Should this perhaps just be:

  test_expect_success 'reset state' 'git reset --hard'

?

-Peff

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Mike Hommey]

On Fri, Feb 29, 2008 at 05:53:31PM -0500, Jeff King wrote:
> On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:
> 
> >  test_expect_success "virtual trees were processed" "git diff expect out"
> >  
> > -git reset --hard
> > +git reset --hard >&3 2>&4
> >  test_expect_success 'refuse to merge binary files' '
> >  	printf "\0" > binary-file &&
> >  	git add binary-file &&
> 
> Should this perhaps just be:
> 
>   test_expect_success 'reset state' 'git reset --hard'

Is it really about testing git reset ?

Mike

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Jeff King]

On Fri, Feb 29, 2008 at 11:58:16PM +0100, Mike Hommey wrote:

> > > -git reset --hard
> > > +git reset --hard >&3 2>&4
> > >  test_expect_success 'refuse to merge binary files' '
> > >  	printf "\0" > binary-file &&
> > >  	git add binary-file &&
> > 
> > Should this perhaps just be:
> > 
> >   test_expect_success 'reset state' 'git reset --hard'
> 
> Is it really about testing git reset ?

No, of course not. Nor is t0003 about testing 'setup'. But it is a
convention[1] in git tests to do everything inside a test_expect_success
wrapper, which is beneficial because:

  - you get your output handled properly automagically :)
  - if the setup step fails, we notice. Which means:
     - it cannot silently cause a later test to succeed (e.g., setup
       changes a file, then the real test reverts it)
     - it is easier to debug failing tests, because you are notified
       of the _first_ failure

-Peff

[1] By convention, I don't necessarily mean that there is a written
rule, nor that we always do it that way, but only that in my experience
we usually do. Somebody like Junio could say more definitely what is
preferred.

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Fri, Feb 29, 2008 at 05:53:31PM -0500, Jeff King wrote:
>> On Fri, Feb 29, 2008 at 11:23:25PM +0100, Mike Hommey wrote:
>> 
>> >  test_expect_success "virtual trees were processed" "git diff expect out"
>> >  
>> > -git reset --hard
>> > +git reset --hard >&3 2>&4
>> >  test_expect_success 'refuse to merge binary files' '
>> >  	printf "\0" > binary-file &&
>> >  	git add binary-file &&
>> 
>> Should this perhaps just be:
>> 
>>   test_expect_success 'reset state' 'git reset --hard'
>
> Is it really about testing git reset ?

No, it is about setting the stage to perform the test in
question, so it belongs inside.

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Johannes Schindelin]

Hi,

On Fri, 29 Feb 2008, Mike Hommey wrote:

> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
> index 149ea85..43b5f15 100755
> --- a/t/t6024-recursive-merge.sh
> +++ b/t/t6024-recursive-merge.sh
> @@ -81,7 +81,7 @@ EOF
>  
>  test_expect_success "virtual trees were processed" "git diff expect out"
>  
> -git reset --hard
> +git reset --hard >&3 2>&4
>  test_expect_success 'refuse to merge binary files' '

Would it not be _much_ more sensible to pull that command _into_ the 
test_expect_success?

Ciao,
Dscho

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Fri, 29 Feb 2008, Mike Hommey wrote:
>
>> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
>> index 149ea85..43b5f15 100755
>> --- a/t/t6024-recursive-merge.sh
>> +++ b/t/t6024-recursive-merge.sh
>> @@ -81,7 +81,7 @@ EOF
>>  
>>  test_expect_success "virtual trees were processed" "git diff expect out"
>>  
>> -git reset --hard
>> +git reset --hard >&3 2>&4
>>  test_expect_success 'refuse to merge binary files' '
>
> Would it not be _much_ more sensible to pull that command _into_ the 
> test_expect_success?

Actually, I think this might be a bit more sensible approach.

-- >8 --
tests: allow optional clean-up phrase to expect_success/failure

When one test modifies the state of the test repository that the later
tests may depend on, you may want to add a clean-up action that is run
regardless of the outcome of the main part of the test.

This can now be specified as the third parameter to test_expect_success
and test_expect_failure functions.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 t/README                   |   13 +++++++++++--
 t/t6024-recursive-merge.sh |    5 +++--
 t/test-lib.sh              |   32 ++++++++++++++++++++++++--------
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 73ed11b..de79c00 100644
--- a/t/README
+++ b/t/README
@@ -146,7 +146,7 @@ Test harness library
 There are a handful helper functions defined in the test harness
 library for your script to use.
 
- - test_expect_success <message> <script>
+ - test_expect_success <message> <script> [<cleanup>]
 
    This takes two strings as parameter, and evaluates the
    <script>.  If it yields success, test is considered
@@ -158,7 +158,12 @@ library for your script to use.
 	    'git-write-tree should be able to write an empty tree.' \
 	    'tree=$(git-write-tree)'
 
- - test_expect_failure <message> <script>
+   An optional <cleanup> is executed and can be used to clean-up
+   the state this test modifies (or leaves half-modified when it
+   fails).  The clean-up script is not run if test fails and the
+   test script is run under --immediate mode to help postmortem.
+
+ - test_expect_failure <message> <script> [<cleanup>]
 
    This is NOT the opposite of test_expect_success, but is used
    to mark a test that demonstrates a known breakage.  Unlike
@@ -167,6 +172,10 @@ library for your script to use.
    success and "still broken" on failure.  Failures from these
    tests won't cause -i (immediate) to stop.
 
+   An optional <cleanup> is executed and can be used to clean-up
+   the state this test modifies (or leaves half-modified when it
+   fails).
+
  - test_debug <script>
 
    This takes a single argument, <script>, and evaluates it only
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 149ea85..ae9706f 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -79,9 +79,10 @@ cat > expect << EOF
 100644 fd7923529855d0b274795ae3349c5e0438333979 3	a1
 EOF
 
-test_expect_success "virtual trees were processed" "git diff expect out"
+test_expect_success "virtual trees were processed" "
+	git diff expect out
+" 'git reset --hard'
 
-git reset --hard
 test_expect_success 'refuse to merge binary files' '
 	printf "\0" > binary-file &&
 	git add binary-file &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 68efda4..0fb2c98 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -220,9 +220,13 @@ test_skip () {
 }
 
 test_expect_failure () {
-	test "$#" = 2 ||
-	error "bug in the test script: not 2 parameters to test-expect-failure"
-	if ! test_skip "$@"
+	case $# in
+	2) ;; # traditional
+	3) ;; # with clean-up
+	*)
+		error "bug in the test script" ;;
+	esac
+	if ! test_skip "$1" "$2"
 	then
 		say >&3 "checking known breakage: $2"
 		test_run_ "$2"
@@ -230,16 +234,24 @@ test_expect_failure () {
 		then
 			test_known_broken_ok_ "$1"
 		else
-		    test_known_broken_failure_ "$1"
+			test_known_broken_failure_ "$1"
+		fi
+		if test -n "$3"
+		then
+			eval >&3 2>&4 "$3"
 		fi
 	fi
 	echo >&3 ""
 }
 
 test_expect_success () {
-	test "$#" = 2 ||
-	error "bug in the test script: not 2 parameters to test-expect-success"
-	if ! test_skip "$@"
+	case $# in
+	2) ;; # traditional
+	3) ;; # with clean-up
+	*)
+		error "bug in the test script" ;;
+	esac
+	if ! test_skip "$1" "$2"
 	then
 		say >&3 "expecting success: $2"
 		test_run_ "$2"
@@ -247,7 +259,11 @@ test_expect_success () {
 		then
 			test_ok_ "$1"
 		else
-			test_failure_ "$@"
+			test_failure_ "$1" "$2"
+		fi
+		if test -n "$3"
+		then
+			eval >&3 2>&4 "$3"
 		fi
 	fi
 	echo >&3 ""

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Jeff King]

On Fri, Feb 29, 2008 at 03:15:32PM -0800, Junio C Hamano wrote:

> >> >  test_expect_success 'refuse to merge binary files' '
> >> >  	printf "\0" > binary-file &&
> >> >  	git add binary-file &&
> >> 
> >> Should this perhaps just be:
> >> 
> >>   test_expect_success 'reset state' 'git reset --hard'
> >
> > Is it really about testing git reset ?
> 
> No, it is about setting the stage to perform the test in
> question, so it belongs inside.

That sounds reasonable to me.

-Peff

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Jeff King]

On Fri, Feb 29, 2008 at 03:50:03PM -0800, Junio C Hamano wrote:

> Actually, I think this might be a bit more sensible approach.
> 
> -- >8 --
> tests: allow optional clean-up phrase to expect_success/failure
> 
> When one test modifies the state of the test repository that the later
> tests may depend on, you may want to add a clean-up action that is run
> regardless of the outcome of the main part of the test.
> 
> This can now be specified as the third parameter to test_expect_success
> and test_expect_failure functions.

I think your heart is in the right place with this patch, but I doubt
that it is going to be all that productive in practice. Most tests
consist of a long list of commands, and cleaning up properly after every
possible failure case is going to be a lot of work. And worse, since the
tests generally _don't_ fail, you have no way to test that your cleanup
is reasonable.

So I think we will end up in the case where a few failed tests will
properly clean themselves up and let further tests proceed, but most
failures will leave a big question. In other words, what problem have we
solved?  If tests N and N+k both fail, would you, even with this patch,
suspect N+k of actually failing, or would you first go and debug test N?
Is that any different than what you do now?

-Peff

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Fri, Feb 29, 2008 at 03:50:03PM -0800, Junio C Hamano wrote:
> 
> > Actually, I think this might be a bit more sensible approach.
> > 
> > -- >8 --
> > tests: allow optional clean-up phrase to expect_success/failure
> > 
> > When one test modifies the state of the test repository that the later
> > tests may depend on, you may want to add a clean-up action that is run
> > regardless of the outcome of the main part of the test.
> 
> I think your heart is in the right place with this patch, but I doubt
> that it is going to be all that productive in practice. Most tests
> consist of a long list of commands, and cleaning up properly after every
> possible failure case is going to be a lot of work. And worse, since the
> tests generally _don't_ fail, you have no way to test that your cleanup
> is reasonable.
> 
> So I think we will end up in the case where a few failed tests will
> properly clean themselves up and let further tests proceed, but most
> failures will leave a big question. In other words, what problem have we
> solved?  If tests N and N+k both fail, would you, even with this patch,
> suspect N+k of actually failing, or would you first go and debug test N?
> Is that any different than what you do now?

I agree with Jeff.  It is unnecessary complexity that won't be
tested well enough to be worthwhile.

This is why when tests start to fail I just rerun the specific case
with "-i -v" and let the test stop on the first failure, *fix that
one case* and run again to see if anything else is going to barf.

-- 
Shawn.

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Jeff King]

On Fri, Feb 29, 2008 at 11:27:39PM -0500, Shawn O. Pearce wrote:

> This is why when tests start to fail I just rerun the specific case
> with "-i -v" and let the test stop on the first failure, *fix that
> one case* and run again to see if anything else is going to barf.

Yes, exactly. I assumed that everyone did that.

-Peff

Re: [PATCH] t6024-recursive-merge.sh: hide spurious output when not running verbosely

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 29, 2008 at 11:27:39PM -0500, Shawn O. Pearce wrote:
>
>> This is why when tests start to fail I just rerun the specific case
>> with "-i -v" and let the test stop on the first failure, *fix that
>> one case* and run again to see if anything else is going to barf.
>
> Yes, exactly. I assumed that everyone did that.

Likewise.