Jeff King wrote:
quoted text
> On Thu, May 06, 2010 at 01:44:28AM -0500, Jonathan Nieder wrote:

quoted text
>>  test_when_finished () {
>> 	test_cleanup="$* && $test_cleanup"
>>  }
>
> Doesn't that violate your rule that the cleanup will _always_ be run?

Thanks for the sanity check.

quoted text
> Perhaps the simplest would be to just keep a cleanup_ret in the second
> eval (where you have eval_ret in the original patch), and then act
> appropriately after the eval finishes. That would be easier on the eyes,
> too, I think.

I tried the cleanup_ret idea; test_run_ ended up looking like this:

	test_cleanup=:
	eval >&3 2>&4 "$1"
	eval_ret=$?
	eval >&3 2>&4 ":; $test_cleanup"
	cleanup_ret=?
	(exit "$test_ret") && (exit "$cleanup_ret")
	eval_ret=$?
	return 0

That breaks the principle of keeping the ugliness in test_when_finished.
So here’s the minimal fix.

-- 8< --
Subject: test-lib: some shells do not let $? propagate into an eval

In 3bf7886 (test-lib: Let tests specify commands to be run at end of
test, 2010-05-02), the git test harness learned to run cleanup
commands unconditionally at the end of a test.  During each test,
the intended cleanup actions are collected in the test_cleanup variable
and evaluated.  That variable looks something like this:

	eval_ret=$?; clean_something && (exit "$eval_ret")
	eval_ret=$?; clean_something_else && (exit "$eval_ret")
	eval_ret=$?; final_cleanup && (exit "$eval_ret")
	eval_ret=$?

All cleanup actions are run unconditionally but if one of them fails
it is properly reported through $eval_ret.

On FreeBSD, unfortunately, $? is set at the beginning of an ‘eval’
to 0 instead of the exit status of the previous command.  This results
in tests using test_expect_code appearing to fail and all others
appearing to pass, unless their cleanup fails.  Avoid the problem by
setting eval_ret before the ‘eval’ begins.

Thanks to Jeff King for the explanation.

Cc: Jeff King <peff@peff.net>
Cc: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I was also surprised to see this migrate to maint so quickly, but I
was happy to see it broke early and loudly.

Because there is some unhappiness with the feature[1], it might make
more sense to revert it for now.  If that discussion can be taken as
license to write tests that take down other tests with them on
failure, then such a revert would not interfere with other work.

[1] http://thread.gmane.org/gmane.comp.version-control.git/146375/focus=146451

 t/t0000-basic.sh |   21 +++++++++++++++++++++
 t/test-lib.sh    |    7 ++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f4ca4fc..3ec9cbe 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -73,6 +73,27 @@ then
 	exit 1
 fi
 
+clean=no
+test_expect_success 'tests clean up after themselves' '
+    test_when_finished clean=yes
+'
+
+cleaner=no
+test_expect_code 1 'tests clean up even after a failure' '
+    test_when_finished cleaner=yes &&
+    (exit 1)
+'
+
+if test $clean$cleaner != yesyes
+then
+	say "bug in test framework: cleanup commands do not work reliably"
+	exit 1
+fi
+
+test_expect_code 2 'failure to clean up causes the test to fail' '
+    test_when_finished "(exit 2)"
+'
+
 ################################################################
 # Basics of the basics
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index acce3d0..7422bba 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -366,8 +366,9 @@ test_debug () {
 }
 
 test_run_ () {
-	test_cleanup='eval_ret=$?'
+	test_cleanup=:
 	eval >&3 2>&4 "$1"
+	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
 	return 0
 }
@@ -567,8 +568,8 @@ test_cmp() {
 # the test to pass.
 
 test_when_finished () {
-	test_cleanup="eval_ret=\$?; { $*
-		} && (exit \"\$eval_ret\"); $test_cleanup"
+	test_cleanup="{ $*
+		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
 
 # Most tests can use the created repository, but some may need to create more.
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html