Re: [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2015, 18:27 UTC
Message-ID: <xmqqr3k32dc9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1446800323-2914-2-git-send-email-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 12 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
>
> t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" is
> flaky in the following case:
> 1. remote upload-pack finds out "not our ref"
> 2. remote sends a response and closes the pipe
> 3. fetch-pack still tries to write commands to the remote upload-pack
> 4. write call in wrapper.c dies with SIGPIPE
>
> t5504 "9 - push with transfer.fsckobjects" is flaky, too, and returns
> SIGPIPE once in a while. I had to remove the final "To dst..." output
> check because there is no output if the process dies with SIGPIPE.

Thanks for a clear write-up.

> This patch accepts the SIGPIPE exit as legitimate test exit.

It is not this patch that accepts such a failure as legit ;-).

"Accept such a death-with-sigpipe also as OK when we are expecting a failure", perhaps?

Show 13 quoted lines

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index ec22c98..22a941b 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1162,15 +1162,15 @@ do
>  		mk_empty shallow &&
>  		(
>  			cd shallow &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3 &&
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_1 &&
>  			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&

These I understand.

>  			git fetch ../testrepo/.git $SHA1_1 &&
>  			git cat-file commit $SHA1_1 &&
> -			test_must_fail git cat-file commit $SHA1_2 &&
> +			test_must_fail_or_sigpipe git cat-file commit $SHA1_2 &&

This I don't. Under what condition is it sane for this "cat-file commit" to fail with sigpipe?

>  			git fetch ../testrepo/.git $SHA1_2 &&
>  			git cat-file commit $SHA1_2 &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_3
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3

And this I do.

Show 34 quoted lines

>  		)
>  	'
>  done
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 73e37a1..19a598e 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -600,6 +600,29 @@ test_must_fail () {
>  	return 0
>  }
>
> +# Similar to test_must_fail, but tolerates sigpipe signal, too.
> +
> +test_must_fail_or_sigpipe () {
> +	"$@"
> +	exit_code=$?
> +	if test $exit_code = 0; then
> +		echo >&2 "test_must_fail: command succeeded: $*"
> +		return 1
> +	elif test $exit_code -ne 141 && \
> +		 test $exit_code -gt 129 && \
> +		 test $exit_code -le 192; then
> +		echo >&2 "test_must_fail: died by signal: $*"
> +		return 1
> +	elif test $exit_code = 127; then
> +		echo >&2 "test_must_fail: command not found: $*"
> +		return 1
> +	elif test $exit_code = 126; then
> +		echo >&2 "test_must_fail: valgrind error: $*"
> +		return 1
> +	fi
> +	return 0
> +}
> +

When you are coming up with this patch, you must have checked the existing code around this area. Did you notice that 126 is handled differently between must_fail and might_fail? Can you explain and justify the difference?

One explanation I can think of is that the contributor who did eeb69131 (tests: notice valgrind error in test_must_fail, 2013-03-31) did not notice that we do essentially the same thing in might_fail and forgot to add it. And it is not the fault of that contributor, but the fault of the duplicated and poorly organized code back then.

Adding the third variant in the way this patch does is making things worse by inviting more mistakes.

How about doing something like the attached to consolidate the existing two into one, and then build this third one on top?

 t/test-lib-functions.sh | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..54497d6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -560,22 +560,7 @@ test_line_count () {
 # the failure could be due to a segv.  We want a controlled failure.
 
 test_must_fail () {
-	"$@"
-	exit_code=$?
-	if test $exit_code = 0; then
-		echo >&2 "test_must_fail: command succeeded: $*"
-		return 1
-	elif test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_must_fail: died by signal: $*"
-		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_must_fail: command not found: $*"
-		return 1
-	elif test $exit_code = 126; then
-		echo >&2 "test_must_fail: valgrind error: $*"
-		return 1
-	fi
-	return 0
+	test_failure_helper test_must_fail "$@"
 }
 
 # Similar to test_must_fail, but tolerates success, too.  This is
@@ -590,13 +575,29 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
+	test_failure_helper test_might_fail "$@"
+}
+
+test_failure_helper () {
+	test_expect=$1
+	shift
 	"$@"
 	exit_code=$?
-	if test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_might_fail: died by signal: $*"
+	if test $test_expect != test_might_fail && test $exit_code = 0
+	then
+		echo >&2 "$test_expect: command succeeded: $*"
+		return 1
+	elif test $exit_code -gt 129 && test $exit_code -le 192
+	then
+		echo >&2 "$test_expect: died by signal: $*"
+		return 1
+	elif test $exit_code = 127
+	then
+		echo >&2 "$test_expect: command not found: $*"
 		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_might_fail: command not found: $*"
+	elif test $exit_code = 126
+	then
+		echo >&2 "$test_expect: valgrind error: $*"
 		return 1
 	fi
 	return 0