Re: [PATCH] test-lib-functions: fix logic error in test_must_fail

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 24/11/15 21:08, Jeff King wrote:
> On Tue, Nov 24, 2015 at 03:59:24PM +0000, Ramsay Jones wrote:
> 
>> 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).
> 
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index 1fdc58c..9061742 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -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.

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

> 
> 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
>   }

OK.

>   ...
>   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

> 
> which is perhaps a bit more clear, as it encapsulates the funny
> negative.
> 
>> 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?

> 
>> [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.

> 
>> 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.

OK

> 
> 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.

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.]

>                   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