threads / rfc / 27713

RFC patchtest: skip clean-up when running under --immediate mode

Subject: [RFC/PATCH] test: skip clean-up when running under --immediate mode

## tl;dr

4 messages between Jun 27, 2011 and Jul 1, 2011. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Junio C Hamano· Jun 27, 2011, 18:02 UTC · lore

Some tests try to be too careful about cleaning themselves up and do

    test_expect_success description '
        set-up some test refs and/or configuration &&
        test_when_finished "revert the above changes" &&
	the real test
    '

Which is nice to make sure that a potential failure would not have unexpected interaction with the next test. This however interferes when "the real test" fails and we want to see what is going on, by running the test with --immediate mode and descending into its trash directory after the test stops. The precondition to run the real test and cause it to fail is all gone after the clean-up procedure defined by test_when_finished is done.

Update test_run_ which is the workhorse of running a test script called from test_expect_success and test_expect_failure, so that we do not run clean-up script defined with test_when_finished when a test that is expected to succeed fails under the --immediate mode.

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

 * Likes, dislikes?

 t/test-lib.sh |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

Show changes to t/test-lib.sh +10 −2

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8c57a00..e36e67a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,9 +446,14 @@ test_debug () {
 
 test_run_ () {
 	test_cleanup=:
+	expecting_failure=$1
 	eval >&3 2>&4 "$1"
 	eval_ret=$?
-	eval >&3 2>&4 "$test_cleanup"
+
+	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
+	then
+		eval >&3 2>&4 "$test_cleanup"
+	fi
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
 		echo ""
 	fi
@@ -497,7 +502,7 @@ test_expect_failure () {
 	if ! test_skip "$@"
 	then
 		say >&3 "checking known breakage: $2"
-		test_run_ "$2"
+		test_run_ "$2" expecting_failure
 		if [ "$?" = 0 -a "$eval_ret" = 0 ]
 		then
 			test_known_broken_ok_ "$1"
@@ -774,6 +779,9 @@ test_cmp() {
 #
 # except that the greeting and config --unset must both succeed for
 # the test to pass.
+#
+# Note that under --immediate mode, no clean-up is done to help diagnose
+# what went wrong.
 
 test_when_finished () {
 	test_cleanup="{ $*

-- 
1.7.6

Jeff King· Jun 28, 2011, 22:51 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] test: skip clean-up when running under --immediate mode

On Mon, Jun 27, 2011 at 11:02:22AM -0700, Junio C Hamano wrote:

Show 26 quoted lines

> Some tests try to be too careful about cleaning themselves up and
> do
> 
>     test_expect_success description '
>         set-up some test refs and/or configuration &&
>         test_when_finished "revert the above changes" &&
> 	the real test
>     '
> 
> Which is nice to make sure that a potential failure would not have
> unexpected interaction with the next test. This however interferes when
> "the real test" fails and we want to see what is going on, by running the
> test with --immediate mode and descending into its trash directory after
> the test stops. The precondition to run the real test and cause it to fail
> is all gone after the clean-up procedure defined by test_when_finished is
> done.
> 
> Update test_run_ which is the workhorse of running a test script
> called from test_expect_success and test_expect_failure, so that we do not
> run clean-up script defined with test_when_finished when a test that is
> expected to succeed fails under the --immediate mode.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Likes, dislikes?

This sounds to me like the right thing to do. The current behavior has never been a problem for me, but that is probably because test_when_finished is relatively new. If I were to run into this situation, your patch does exactly what I would expect.

-Peff

Michael J Gruber· Jun 30, 2011, 06:45 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] test: skip clean-up when running under --immediate mode

Junio C Hamano venit, vidit, dixit 27.06.2011 20:02:

Show 26 quoted lines

> Some tests try to be too careful about cleaning themselves up and
> do
> 
>     test_expect_success description '
>         set-up some test refs and/or configuration &&
>         test_when_finished "revert the above changes" &&
> 	the real test
>     '
> 
> Which is nice to make sure that a potential failure would not have
> unexpected interaction with the next test. This however interferes when
> "the real test" fails and we want to see what is going on, by running the
> test with --immediate mode and descending into its trash directory after
> the test stops. The precondition to run the real test and cause it to fail
> is all gone after the clean-up procedure defined by test_when_finished is
> done.
> 
> Update test_run_ which is the workhorse of running a test script
> called from test_expect_success and test_expect_failure, so that we do not
> run clean-up script defined with test_when_finished when a test that is
> expected to succeed fails under the --immediate mode.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Likes, dislikes?

Likes!

Show 18 quoted lines

> 
>  t/test-lib.sh |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 8c57a00..e36e67a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -446,9 +446,14 @@ test_debug () {
>  
>  test_run_ () {
>  	test_cleanup=:
> +	expecting_failure=$1
>  	eval >&3 2>&4 "$1"
>  	eval_ret=$?
> -	eval >&3 2>&4 "$test_cleanup"
> +
> +	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"

One may argue that a "test_expect_failure" which succeeds is a fail and should stop with --immediate (so that it's easier to spot fixed bugs), but...

Show 15 quoted lines

> +	then
> +		eval >&3 2>&4 "$test_cleanup"
> +	fi
>  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
>  		echo ""
>  	fi
> @@ -497,7 +502,7 @@ test_expect_failure () {
>  	if ! test_skip "$@"
>  	then
>  		say >&3 "checking known breakage: $2"
> -		test_run_ "$2"
> +		test_run_ "$2" expecting_failure
>  		if [ "$?" = 0 -a "$eval_ret" = 0 ]
>  		then
>  			test_known_broken_ok_ "$1"

...we would have to change that here also, and it would be a change in behaviour of the test suite. Anyway, that is orthogonal to this patch which I do like.

Show 10 quoted lines

> @@ -774,6 +779,9 @@ test_cmp() {
>  #
>  # except that the greeting and config --unset must both succeed for
>  # the test to pass.
> +#
> +# Note that under --immediate mode, no clean-up is done to help diagnose
> +# what went wrong.
>  
>  test_when_finished () {
>  	test_cleanup="{ $*

Sverre Rabbelier· Jul 1, 2011, 10:49 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] test: skip clean-up when running under --immediate mode

Heya,

On Mon, Jun 27, 2011 at 20:02, Junio C Hamano <gitster@pobox.com> wrote:

>  * Likes, dislikes?

Makes a lot of sense, the 'test_when_finished'-ed code will get executed when the test doesn't fail anyway, so I don't think there need to be a way to turn this behavior off. (I.e., the patch is good as-is).

-- 
Cheers,

Sverre Rabbelier