{"thread":{"id":"40869","subject":"[PATCH] test-lib-functions: fix logic error in test_must_fail","startedAt":"2015-11-24T15:59:24Z","lastAt":"2015-11-25T00:10:41Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273641","messageId":"5654895C.2000909@ramsayjones.plus.com","threadId":"40869","inReplyTo":null,"subject":"[PATCH] test-lib-functions: fix logic error in test_must_fail","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2015-11-24T15:59:24Z","receivedAt":"2015-11-24T15:59:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAfter commit 710eb805 (\"implement test_might_fail using a refactored\ntest_must_fail\", 19-11-2015) several tests now unexpectedly pass:\n\n  $ tail -21 ptest-out\n  [17:25:53]\n  All tests successful.\n\n  Test Summary Report\n  -------------------\n  t1060-object-corruption.sh                       (Wstat: 0 Tests: 13 Failed: 0)\n    TODO passed:   13\n  t3510-cherry-pick-sequence.sh                    (Wstat: 0 Tests: 39 Failed: 0)\n    TODO passed:   31\n  t5707-clone-detached.sh                          (Wstat: 0 Tests: 13 Failed: 0)\n    TODO passed:   4\n  t6042-merge-rename-corner-cases.sh               (Wstat: 0 Tests: 26 Failed: 0)\n    TODO passed:   8\n  Files=727, Tests=13139, 407 wallclock secs ( 3.66 usr  0.54 sys + 85.10 cusr 258.06 csys = 347.36 CPU)\n  Result: PASS\n  make clean-except-prove-cache\n  make[2]: Entering directory `/home/ramsay/git/t'\n  rm -f -r 'trash directory'.* 'test-results'\n  rm -f -r valgrind/bin\n  make[2]: Leaving directory `/home/ramsay/git/t'\n  make[1]: Leaving directory `/home/ramsay/git/t'\n  $\n\nFix the logic bug in the refactored test_must_fail. Also, use the\ninteger comparison operator -eq when comparing the exit code, rather\nthan the string = operator.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n\nHi Lars,\n\nThis patch is on top of the pu branch, but it relates to the two\npatches on your 'ls/test-must-fail-sigpipe' branch. Could you\nplease squash the relevant parts of this patch into your patches.\n\nSo, in addition to the logic error in commit 710eb805, I modified\nthe test introduced by commit 653a48e56 (\"add \"ok=sigpipe\" to\ntest_must_fail and use it to fix flaky tests\", 19-11-2015).\n\nSince I cannot test this second change (t5516 and t5504 don't\nfail for me), I don't know if this change is correct - please\ntest and confirm. (No, it's not clear to me exactly what this\ncommit is supposed to do! :-D ).\n\n[I didn't have time to go look what value would be returned by\na case statement where there is no 'default' limb - I suspect\nthat it is undefined behaviour. Even if it is defined, do all\nshells behave properly? In any event, it is much simpler to\ncompare the strings directly!]\n\nI have to say, I'm not keen on either of these commits, but Jeff\nand Junio seem OK with it, so ... (the tests being flaky implies\nthat the git client is flaky - we should fix that).\n\nThanks.\n\nATB,\nRamsay Jones\n\n t/test-lib-functions.sh | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1fdc58c..9061742 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -593,24 +593,22 @@ test_must_fail () {\n \tesac\n \t\"$@\"\n \texit_code=$?\n-\tif ! case \",$_test_ok,\" in *,success,*) false;; esac &&\n-\t\ttest $exit_code = 0\n+\tif test $exit_code -eq 0 && test x$_test_ok != xsuccess\n \tthen\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n-\t\treturn 0\n-\telif ! case \",$_test_ok,\" in *,sigpipe,*) false;; esac &&\n-\t\ttest $exit_code = 141\n+\t\treturn 1\n+\telif test $exit_code -eq 141 && test x$_test_ok = xsigpipe\n \tthen\n \t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192\n \tthen\n \t\techo >&2 \"test_must_fail: died by signal: $*\"\n \t\treturn 1\n-\telif test $exit_code = 127\n+\telif test $exit_code -eq 127\n \tthen\n \t\techo >&2 \"test_must_fail: command not found: $*\"\n \t\treturn 1\n-\telif test $exit_code = 126\n+\telif test $exit_code -eq 126\n \tthen\n \t\techo >&2 \"test_must_fail: valgrind error: $*\"\n \t\treturn 1\n-- \n2.6.0\n"},{"id":"273654","messageId":"20151124210822.GG7174@sigill.intra.peff.net","threadId":"40869","inReplyTo":"5654895C.2000909@ramsayjones.plus.com","subject":"Re: [PATCH] test-lib-functions: fix logic error in test_must_fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T21:08:22Z","receivedAt":"2015-11-24T21:08:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 24, 2015 at 03:59:24PM +0000, Ramsay Jones wrote:\n\n> After commit 710eb805 (\"implement test_might_fail using a refactored\n> test_must_fail\", 19-11-2015) several tests now unexpectedly pass:\n\nThanks. I noticed there were some new passes, but I hadn't investigated\nyet (I assumed it was \"yay, we fixed a bug\" not \"oops, we broke the test\nsuite).\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 1fdc58c..9061742 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -593,24 +593,22 @@ test_must_fail () {\n>  \tesac\n>  \t\"$@\"\n>  \texit_code=$?\n> -\tif ! case \",$_test_ok,\" in *,success,*) false;; esac &&\n> -\t\ttest $exit_code = 0\n> +\tif test $exit_code -eq 0 && test x$_test_ok != xsuccess\n\nI don't think this is quite right. I had the impression the original was\ntrying to allow something like:\n\n  test_must_fail ok=success,sigpipe\n\nAnd that's what the commas were for in the case statement.\n\nIf I understand the logic bug correctly, we simply need to flip the \"!\"\nat the start of the case statement. But we could do something like:\n\n  list_contains () {\n\tcase \",$1,\" in\n\t*,$2,*)\n\t\treturn 0\n\t\t;;\n\tesac\n\treturn 1\n  }\n  ...\n  if ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n  then\n\treturn 0\n  fi\n\nwhich is perhaps a bit more clear, as it encapsulates the funny\nnegative.\n\n> Since I cannot test this second change (t5516 and t5504 don't\n> fail for me), I don't know if this change is correct - please\n> test and confirm. (No, it's not clear to me exactly what this\n> commit is supposed to do! :-D ).\n\nThey don't fail consistently. It's a SIGPIPE race.\n\n> [I didn't have time to go look what value would be returned by\n> a case statement where there is no 'default' limb - I suspect\n> that it is undefined behaviour. Even if it is defined, do all\n> shells behave properly? In any event, it is much simpler to\n> compare the strings directly!]\n\nYeah, I wondered that. We wouldn't depend on it in the example I gave\nabove.\n\n> I have to say, I'm not keen on either of these commits, but Jeff\n> and Junio seem OK with it, so ... (the tests being flaky implies\n> that the git client is flaky - we should fix that).\n\nI was actually reasonably happy with just having test_must_fail ignore\nSIGPIPE, for the second one. But the infrastructure added by the first\npatch does fix real issues in test_might_fail (e.g., that it should\ncomplain of a valgrind failure but doesn't). And once you have that\ninfrastructure, the second patch becomes trivial.\n\nI don't think the git client is actually flaky here in a way that we can\nfix. If I understand the issue correctly, it is that the server side\nhangs up in an error case while the client is still writing. So we might\nget one of two outcomes:\n\n  1. If the client has finished writing, it will do a read, see the\n     hang up, and die().\n\n  2. If the client is still writing, it will get SIGPIPE and die.\n\nThere's no solution outside of ignoring SIGPIPE and handling the write()\nerror ourselves. We've been loathe to do that in the past because\nSIGPIPE applies globally, and missing a write() anywhere in the program\nmeans we may spin on bogus writes longer than we need to.\n\n-Peff\n"},{"id":"273679","messageId":"5654FB50.4000102@ramsayjones.plus.com","threadId":"40869","inReplyTo":"20151124210822.GG7174@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib-functions: fix logic error in test_must_fail","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2015-11-25T00:05:36Z","receivedAt":"2015-11-25T00:05:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 24/11/15 21:08, Jeff King wrote:\n> On Tue, Nov 24, 2015 at 03:59:24PM +0000, Ramsay Jones wrote:\n> \n>> After commit 710eb805 (\"implement test_might_fail using a refactored\n>> test_must_fail\", 19-11-2015) several tests now unexpectedly pass:\n> \n> Thanks. I noticed there were some new passes, but I hadn't investigated\n> yet (I assumed it was \"yay, we fixed a bug\" not \"oops, we broke the test\n> suite).\n> \n>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>> index 1fdc58c..9061742 100644\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -593,24 +593,22 @@ test_must_fail () {\n>>  \tesac\n>>  \t\"$@\"\n>>  \texit_code=$?\n>> -\tif ! case \",$_test_ok,\" in *,success,*) false;; esac &&\n>> -\t\ttest $exit_code = 0\n>> +\tif test $exit_code -eq 0 && test x$_test_ok != xsuccess\n> \n> I don't think this is quite right. I had the impression the original was\n> trying to allow something like:\n> \n>   test_must_fail ok=success,sigpipe\n> \n> And that's what the commas were for in the case statement.\n\nAh, OK, that makes a bit more sense. (perhaps the commit message\ncould mention this).\n\n> \n> If I understand the logic bug correctly, we simply need to flip the \"!\"\n> at the start of the case statement. But we could do something like:\n> \n>   list_contains () {\n> \tcase \",$1,\" in\n> \t*,$2,*)\n> \t\treturn 0\n> \t\t;;\n> \tesac\n> \treturn 1\n>   }\n\nOK.\n\n>   ...\n>   if ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n>   then\n> \treturn 0\n>   fi\n    ^^\nIs this intended to be part of the if..elif.. chain, or a separate\ninitial conditional? Hmm, actually it doesn't matter since it has\nexactly the same logic error as the original patch ... :-D\n\n> \n> which is perhaps a bit more clear, as it encapsulates the funny\n> negative.\n> \n>> Since I cannot test this second change (t5516 and t5504 don't\n>> fail for me), I don't know if this change is correct - please\n>> test and confirm. (No, it's not clear to me exactly what this\n>> commit is supposed to do! :-D ).\n> \n> They don't fail consistently. It's a SIGPIPE race.\n\nYes, and unfortunately I can't get it to happen for me.\nWhich platforms has it been observed on?\n\n> \n>> [I didn't have time to go look what value would be returned by\n>> a case statement where there is no 'default' limb - I suspect\n>> that it is undefined behaviour. Even if it is defined, do all\n>> shells behave properly? In any event, it is much simpler to\n>> compare the strings directly!]\n> \n> Yeah, I wondered that. We wouldn't depend on it in the example I gave\n> above.\n\nIndeed.\n\n> \n>> I have to say, I'm not keen on either of these commits, but Jeff\n>> and Junio seem OK with it, so ... (the tests being flaky implies\n>> that the git client is flaky - we should fix that).\n> \n> I was actually reasonably happy with just having test_must_fail ignore\n> SIGPIPE, for the second one. But the infrastructure added by the first\n> patch does fix real issues in test_might_fail (e.g., that it should\n> complain of a valgrind failure but doesn't). And once you have that\n> infrastructure, the second patch becomes trivial.\n\nOK\n\n> \n> I don't think the git client is actually flaky here in a way that we can\n> fix. If I understand the issue correctly, it is that the server side\n> hangs up in an error case while the client is still writing. So we might\n> get one of two outcomes:\n> \n>   1. If the client has finished writing, it will do a read, see the\n>      hang up, and die().\n> \n>   2. If the client is still writing, it will get SIGPIPE and die.\n> \n> There's no solution outside of ignoring SIGPIPE and handling the write()\n> error ourselves.\n\nThis is what I had in mind. Of course, I haven't spent any time looking\ninto how much work is involved (or what effect it would have on code\nmaintenance) ... [So, it may not be worth doing, but \"we can't fix\" is\nperhaps a bit strong.]\n\n>                   We've been loathe to do that in the past because\n> SIGPIPE applies globally, and missing a write() anywhere in the program\n> means we may spin on bogus writes longer than we need to.\n\nHmm, OK.\n\nATB,\nRamsay Jones\n"},{"id":"273680","messageId":"20151125001040.GA19620@sigill.intra.peff.net","threadId":"40869","inReplyTo":"5654FB50.4000102@ramsayjones.plus.com","subject":"Re: [PATCH] test-lib-functions: fix logic error in test_must_fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-25T00:10:41Z","receivedAt":"2015-11-25T00:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2015 at 12:05:36AM +0000, Ramsay Jones wrote:\n\n> >   ...\n> >   if ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n> >   then\n> > \treturn 0\n> >   fi\n>     ^^\n> Is this intended to be part of the if..elif.. chain, or a separate\n> initial conditional? Hmm, actually it doesn't matter since it has\n> exactly the same logic error as the original patch ... :-D\n\nI meant it as part of the same one, but yeah, it has the wrong \"return\"\n(I noticed it when writing up the real patch as a SQUASH on top of\nLars's topics).\n\n> > They don't fail consistently. It's a SIGPIPE race.\n> \n> Yes, and unfortunately I can't get it to happen for me.\n> Which platforms has it been observed on?\n\nI haven't tried to reproduce it, but I think it was seen originally in\nthe Travis build.\n\n-Peff\n"}]}