test-lib-functions: fix logic error in test_must_fail

PATCH In review Ramsay Jones 2 participants 4 messages Jun 15, 2016

Share & tools

Ramsay Jones

author

2016-06-15 23:07 UTC #0 lore raw

PATCH 1/1 test-lib-functions: fix logic error in test_must_fail

After commit 710eb805 ("implement test_might_fail using a refactored test_must_fail", 19-11-2015) several tests now unexpectedly pass:

  $ tail -21 ptest-out
  [17:25:53]
  All tests successful.

Test Summary Report
-------------------
t1060-object-corruption.sh (Wstat: 0 Tests: 13 Failed: 0)
TODO passed: 13
t3510-cherry-pick-sequence.sh (Wstat: 0 Tests: 39 Failed: 0)
TODO passed: 31
t5707-clone-detached.sh (Wstat: 0 Tests: 13 Failed: 0)
TODO passed: 4
t6042-merge-rename-corner-cases.sh (Wstat: 0 Tests: 26 Failed: 0)
TODO passed: 8
Files=727, Tests=13139, 407 wallclock secs ( 3.66 usr 0.54 sys + 85.10 cusr 258.06 csys = 347.36 CPU)
Result: PASS
make clean-except-prove-cache
make[2]: Entering directory `/home/ramsay/git/t'
rm -f -r 'trash directory'.* 'test-results'
rm -f -r valgrind/bin
make[2]: Leaving directory `/home/ramsay/git/t'
make[1]: Leaving directory `/home/ramsay/git/t'
$

Fix the logic bug in the refactored test_must_fail. Also, use the integer comparison operator -eq when comparing the exit code, rather than the string = operator.

Signed-off-by Ramsay Jones <[email protected]>

---

Hi Lars,

This patch is on top of the pu branch, but it relates to the two patches on your 'ls/test-must-fail-sigpipe' branch. Could you please squash the relevant parts of this patch into your patches.

So, in addition to the logic error in commit 710eb805, I modified the test introduced by commit 653a48e56 ("add "ok=sigpipe" to test_must_fail and use it to fix flaky tests", 19-11-2015).

Since I cannot test this second change (t5516 and t5504 don't fail for me), I don't know if this change is correct - please test and confirm. (No, it's not clear to me exactly what this commit is supposed to do! :-D ).

[I didn't have time to go look what value would be returned by a case statement where there is no 'default' limb - I suspect that it is undefined behaviour. Even if it is defined, do all shells behave properly? In any event, it is much simpler to compare the strings directly!]

I have to say, I'm not keen on either of these commits, but Jeff and Junio seem OK with it, so ... (the tests being flaky implies that the git client is flaky - we should fix that).

Thanks.

ATB,
Ramsay Jones

 t/test-lib-functions.sh | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

t/test-lib-functions.sh +5 -7

index 1fdc58c..9061742 100644
@@ -593,24 +593,22 @@ test_must_fail () {
 	esac
 	"$@"
 	exit_code=$?
-	if ! case ",$_test_ok," in *,success,*) false;; esac &&
-		test $exit_code = 0
+	if test $exit_code -eq 0 && test x$_test_ok != xsuccess
 	then
 		echo >&2 "test_must_fail: command succeeded: $*"
-		return 0
-	elif ! case ",$_test_ok," in *,sigpipe,*) false;; esac &&
-		test $exit_code = 141
+		return 1
+	elif test $exit_code -eq 141 && test x$_test_ok = xsigpipe
 	then
 		return 0
 	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
+	elif test $exit_code -eq 127
 	then
 		echo >&2 "test_must_fail: command not found: $*"
 		return 1
-	elif test $exit_code = 126
+	elif test $exit_code -eq 126
 	then
 		echo >&2 "test_must_fail: valgrind error: $*"
 		return 1

Signature · 1 line

2.6.0

Jeff King

2016-06-15 23:07 UTC #1 lore raw

Quoted 2 lines · Ramsay Jones

After commit 710eb805 ("implement test_might_fail using a refactored test_must_fail", 19-11-2015) several tests now unexpectedly pass:

Thanks. I noticed there were some new passes, but I hadn't investigated yet (I assumed it was "yay, we fixed a bug" not "oops, we broke the test suite).

t/test-lib-functions.sh test_must_fail () { · line 596

Quoted 11 lines

t/test-lib-functions.sh +1 -2

index 1fdc58c..9061742 100644
@@ -593,24 +593,22 @@ test_must_fail () {
 	esac
 	"$@"
 	exit_code=$?
-	if ! case ",$_test_ok," in *,success,*) false;; esac &&
-		test $exit_code = 0
+	if test $exit_code -eq 0 && test x$_test_ok != xsuccess

I don't think this is quite right. I had the impression the original was trying to allow something like:

test_must_fail ok=success,sigpipe

And that's what the commas were for in the case statement.

If I understand the logic bug correctly, we simply need to flip the "!" at the start of the case statement. But we could do something like:

  list_contains () {
	case ",$1," in
	*,$2,*)
		return 0
		;;
	esac
	return 1

}
...
if ! list_contains "$_test_ok" success && test "$exit_code" -eq 0 then
return 0
fi

which is perhaps a bit more clear, as it encapsulates the funny negative.

Quoted 4 lines

Since I cannot test this second change (t5516 and t5504 don't fail for me), I don't know if this change is correct - please test and confirm. (No, it's not clear to me exactly what this commit is supposed to do! :-D ).

They don't fail consistently. It's a SIGPIPE race.

Quoted 5 lines

[I didn't have time to go look what value would be returned by a case statement where there is no 'default' limb - I suspect that it is undefined behaviour. Even if it is defined, do all shells behave properly? In any event, it is much simpler to compare the strings directly!]

Yeah, I wondered that. We wouldn't depend on it in the example I gave above.

Quoted 3 lines

I have to say, I'm not keen on either of these commits, but Jeff and Junio seem OK with it, so ... (the tests being flaky implies that the git client is flaky - we should fix that).

I was actually reasonably happy with just having test_must_fail ignore SIGPIPE, for the second one. But the infrastructure added by the first patch does fix real issues in test_might_fail (e.g., that it should complain of a valgrind failure but doesn't). And once you have that infrastructure, the second patch becomes trivial.

I don't think the git client is actually flaky here in a way that we can fix. If I understand the issue correctly, it is that the server side hangs up in an error case while the client is still writing. So we might get one of two outcomes:

1. If the client has finished writing, it will do a read, see the hang up, and die().

2. If the client is still writing, it will get SIGPIPE and die.

There's no solution outside of ignoring SIGPIPE and handling the write() error ourselves. We've been loathe to do that in the past because SIGPIPE applies globally, and missing a write() anywhere in the program means we may spin on bogus writes longer than we need to.

-Peff

Ramsay Jones

author

2016-06-15 23:07 UTC #2 lore raw

Quoted 24 lines · Jeff King

Quoted 2 lines · Ramsay Jones
After commit 710eb805 ("implement test_might_fail using a refactored test_must_fail", 19-11-2015) several tests now unexpectedly pass:
Thanks. I noticed there were some new passes, but I hadn't investigated yet (I assumed it was "yay, we fixed a bug" not "oops, we broke the test suite).
t/test-lib-functions.sh test_must_fail () { · line 596
Quoted 11 lines
t/test-lib-functions.sh +1 -2
index 1fdc58c..9061742 100644
@@ -593,24 +593,22 @@ test_must_fail () {
 	esac
 	"$@"
 	exit_code=$?
-	if ! case ",$_test_ok," in *,success,*) false;; esac &&
-		test $exit_code = 0
+	if test $exit_code -eq 0 && test x$_test_ok != xsuccess
I don't think this is quite right. I had the impression the original was trying to allow something like:

Quoted 3 lines

test_must_fail ok=success,sigpipe
And that's what the commas were for in the case statement.

Ah, OK, that makes a bit more sense. (perhaps the commit message could mention this).

Quoted 4 lines

If I understand the logic bug correctly, we simply need to flip the "!" at the start of the case statement. But we could do something like:

Quoted 8 lines

  list_contains () {
	case ",$1," in
	*,$2,*)
		return 0
		;;
	esac
	return 1

}

OK.

Quoted 5 lines

...
if ! list_contains "$_test_ok" success && test "$exit_code" -eq 0 then
return 0
fi

^^

Is this intended to be part of the if..elif.. chain, or a separate initial conditional? Hmm, actually it doesn't matter since it has exactly the same logic error as the original patch ... :-D

Quoted 10 lines

which is perhaps a bit more clear, as it encapsulates the funny negative.
Quoted 4 lines
Since I cannot test this second change (t5516 and t5504 don't fail for me), I don't know if this change is correct - please test and confirm. (No, it's not clear to me exactly what this commit is supposed to do! :-D ).
They don't fail consistently. It's a SIGPIPE race.

Yes, and unfortunately I can't get it to happen for me. Which platforms has it been observed on?

Quoted 9 lines

Quoted 6 lines
[I didn't have time to go look what value would be returned by a case statement where there is no 'default' limb - I suspect that it is undefined behaviour. Even if it is defined, do all shells behave properly? In any event, it is much simpler to compare the strings directly!]
Yeah, I wondered that. We wouldn't depend on it in the example I gave above.

Indeed.

Quoted 10 lines

Quoted 4 lines
I have to say, I'm not keen on either of these commits, but Jeff and Junio seem OK with it, so ... (the tests being flaky implies that the git client is flaky - we should fix that).
I was actually reasonably happy with just having test_must_fail ignore SIGPIPE, for the second one. But the infrastructure added by the first patch does fix real issues in test_might_fail (e.g., that it should complain of a valgrind failure but doesn't). And once you have that infrastructure, the second patch becomes trivial.

Quoted 6 lines

I don't think the git client is actually flaky here in a way that we can fix. If I understand the issue correctly, it is that the server side hangs up in an error case while the client is still writing. So we might get one of two outcomes:

Quoted 7 lines

1. If the client has finished writing, it will do a read, see the hang up, and die().
2. If the client is still writing, it will get SIGPIPE and die.
There's no solution outside of ignoring SIGPIPE and handling the write() error ourselves.

This is what I had in mind. Of course, I haven't spent any time looking into how much work is involved (or what effect it would have on code maintenance) ... [So, it may not be worth doing, but "we can't fix" is perhaps a bit strong.]

Quoted 3 lines

We've been loathe to do that in the past because
SIGPIPE applies globally, and missing a write() anywhere in the program means we may spin on bogus writes longer than we need to.

Hmm, OK.

ATB,
Ramsay Jones

Jeff King

2016-06-15 23:07 UTC #3 lore raw

Quoted 9 lines · Ramsay Jones

Quoted 5 lines
...
if ! list_contains "$_test_ok" success && test "$exit_code" -eq 0 then
return 0
fi
    ^^
Is this intended to be part of the if..elif.. chain, or a separate initial conditional? Hmm, actually it doesn't matter since it has exactly the same logic error as the original patch ... :-D

I meant it as part of the same one, but yeah, it has the wrong "return" (I noticed it when writing up the real patch as a SQUASH on top of Lars's topics).

Quoted 4 lines

Quoted 1 line
They don't fail consistently. It's a SIGPIPE race.
Yes, and unfortunately I can't get it to happen for me. Which platforms has it been observed on?

I haven't tried to reproduce it, but I think it was seen originally in the Travis build.

-Peff