From: Ramsay Jones <ramsay@ramsayjones•plus.com>
To: Jeff King <peff@peff•net>
Cc: Lars Schneider <larsxschneider@gmail•com>,
Junio C Hamano <gitster@pobox•com>,
GIT Mailing-list <git@vger•kernel.org>
Subject: Re: [PATCH] test-lib-functions: fix logic error in test_must_fail
Date: Wed, 25 Nov 2015 00:05:36 +0000 [thread overview]
Message-ID: <5654FB50.4000102@ramsayjones.plus.com> (raw)
In-Reply-To: <20151124210822.GG7174@sigill.intra.peff.net>
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
next prev parent reply other threads:[~2015-11-25 0:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 15:59 [PATCH] test-lib-functions: fix logic error in test_must_fail Ramsay Jones
2015-11-24 21:08 ` Jeff King
2015-11-25 0:05 ` Ramsay Jones [this message]
2015-11-25 0:10 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5654FB50.4000102@ramsayjones.plus.com \
--to=ramsay@ramsayjones$(echo .)plus.com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=larsxschneider@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox