{"thread":{"id":"40898","subject":"[PATCH v1 0/2] test-must-fail-sigpipe","startedAt":"2015-11-27T09:15:12Z","lastAt":"2015-12-01T09:05:21Z","messageCount":7,"participants":["larsxschneider@gmail.com","Ramsay Jones","Jeff King","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"273790","messageId":"1448615714-43768-1-git-send-email-larsxschneider@gmail.com","threadId":"40898","inReplyTo":null,"subject":"[PATCH v1 0/2] test-must-fail-sigpipe","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-11-27T09:15:12Z","receivedAt":"2015-11-27T09:15:12Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAs suggested by Peff I made \"test-must-fail-sigpipe\" a seperate topic.\n\ndiff to (former) Travis CI v7 topic:\n* added helper function \"list_contains\" provided by Peff (thanks!)\n* fix return value in \"test_must_fail: command succeeded\" (thanks Ramsay)\n* use \"-eq\" instead of \"=\" (thanks Ramsay)\n* accept SIGPIPE exit for t5504 \"push with receive.fsckobjects\"\n\nThanks,\nLars\n\nLars Schneider (2):\n implement test_might_fail using a refactored test_must_fail\n add \"ok=sigpipe\" to test_must_fail and use it to fix flaky tests\n\n t/t5504-fetch-receive-strict.sh | 5 ++---\n t/t5516-fetch-push.sh | 6 ++---\n t/test-lib-functions.sh | 50 +++++++++++++++++++++++++++++------------\n 3 files changed, 41 insertions(+), 20 deletions(-)\n\n--\n2.5.1\n"},{"id":"273792","messageId":"1448615714-43768-2-git-send-email-larsxschneider@gmail.com","threadId":"40898","inReplyTo":"1448615714-43768-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v1 1/2] implement test_might_fail using a refactored test_must_fail","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-11-27T09:15:13Z","receivedAt":"2015-11-27T09:15:13Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAdd an (optional) first parameter \"ok=\" to test_must_fail\nand return success for \"\". Add \"success\" as\n\"\" and use it to implement \"test_might_fail\". This removes\nredundancies in test-lib-function.sh.\n\nYou can pass multiple arguments divided by comma (e.g.\n\"test_must_fail ok=success,something\")\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Jeff King \nSigned-off-by: Ramsay Jones \nSigned-off-by: Lars Schneider \n---\n t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++--------------\n 1 file changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 73e37a1..94c449a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -569,6 +569,21 @@ test_line_count () {\n \tfi\n }\n \n+# Returns success if a comma separated string of keywords ($1) contains a\n+# given keyword ($2).\n+# Examples:\n+# `list_contains \"foo,bar\" bar` returns 0\n+# `list_contains \"foo\" bar` returns 1\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 # This is not among top-level (test_expect_success | test_expect_failure)\n # but is a prefix that can be used in the test script, like:\n #\n@@ -582,18 +597,31 @@ test_line_count () {\n # the failure could be due to a segv. We want a controlled failure.\n \n test_must_fail () {\n+\tcase \"$1\" in\n+\tok=*)\n+\t\t_test_ok=${1#ok=}\n+\t\tshift\n+\t\t;;\n+\t*)\n+\t\t_test_ok=\n+\t\t;;\n+\tesac\n \t\"$@\"\n \texit_code=$?\n-\tif test $exit_code = 0; then\n+\tif ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n+\tthen\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n-\telif test $exit_code -gt 129 && test $exit_code -le 192; then\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; then\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; then\n+\telif test $exit_code -eq 126\n+\tthen\n \t\techo >&2 \"test_must_fail: valgrind error: $*\"\n \t\treturn 1\n \tfi\n@@ -612,16 +640,7 @@ test_must_fail () {\n # because we want to notice if it fails due to segv.\n \n test_might_fail () {\n-\t\"$@\"\n-\texit_code=$?\n-\tif test $exit_code -gt 129 && test $exit_code -le 192; then\n-\t\techo >&2 \"test_might_fail: died by signal: $*\"\n-\t\treturn 1\n-\telif test $exit_code = 127; then\n-\t\techo >&2 \"test_might_fail: command not found: $*\"\n-\t\treturn 1\n-\tfi\n-\treturn 0\n+\ttest_must_fail ok=success \"$@\"\n }\n \n # Similar to test_must_fail and test_might_fail, but check that a\n-- \n2.5.1\n"},{"id":"273791","messageId":"1448615714-43768-3-git-send-email-larsxschneider@gmail.com","threadId":"40898","inReplyTo":"1448615714-43768-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v1 2/2] add \"ok=sigpipe\" to test_must_fail and use it to fix flaky tests","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-11-27T09:15:14Z","receivedAt":"2015-11-27T09:15:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nt5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" is\nflaky in the following case:\n1. remote upload-pack finds out \"not our ref\"\n2. remote sends a response and closes the pipe\n3. fetch-pack still tries to write commands to the remote upload-pack\n4. write call in wrapper.c dies with SIGPIPE\n\nt5504 \"9 - push with transfer.fsckobjects\" is flaky, too, and returns\nSIGPIPE once in a while. I had to remove the final \"To dst...\" output\ncheck because there is no output if the process dies with SIGPUPE.\n\nAccept such a death-with-sigpipe also as OK when we are expecting a\nfailure.\n\nSigned-off-by: Lars Schneider \n---\n t/t5504-fetch-receive-strict.sh | 5 ++---\n t/t5516-fetch-push.sh | 6 +++---\n t/test-lib-functions.sh | 3 +++\n 3 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 44f3d5f..89224ed 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -100,7 +100,7 @@ test_expect_success 'push with receive.fsckobjects' '\n \t\tgit config receive.fsckobjects true &&\n \t\tgit config transfer.fsckobjects false\n \t) &&\n-\ttest_must_fail git push --porcelain dst master:refs/heads/test >act &&\n+\ttest_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act &&\n \ttest_cmp exp act\n '\n \n@@ -111,8 +111,7 @@ test_expect_success 'push with transfer.fsckobjects' '\n \t\tcd dst &&\n \t\tgit config transfer.fsckobjects true\n \t) &&\n-\ttest_must_fail git push --porcelain dst master:refs/heads/test >act &&\n-\ttest_cmp exp act\n+\ttest_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act\n '\n \n cat >bogus-commit <<\\EOF\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex ec22c98..0a87e19 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1162,15 +1162,15 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_1 &&\n+\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n+\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit cat-file commit $SHA1_1 &&\n \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3\n+\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n \t\t)\n \t'\n done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 94c449a..06d3fcb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -612,6 +612,9 @@ test_must_fail () {\n \tthen\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n+\telif list_contains \"$_test_ok\" sigpipe && test \"$exit_code\" -eq 141\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-- \n2.5.1\n"},{"id":"273795","messageId":"56584E92.1030103@ramsayjones.plus.com","threadId":"40898","inReplyTo":"1448615714-43768-2-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1 1/2] implement test_might_fail using a refactored test_must_fail","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2015-11-27T12:37:38Z","receivedAt":"2015-11-27T12:37:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/11/15 09:15, larsxschneider@gmail.com wrote:\n> From: Lars Schneider \n> \n> Add an (optional) first parameter \"ok=\" to test_must_fail\n> and return success for \"\". Add \"success\" as\n> \"\" and use it to implement \"test_might_fail\". This removes\n> redundancies in test-lib-function.sh.\n> \n> You can pass multiple arguments divided by comma (e.g.\n> \"test_must_fail ok=success,something\")\n> \n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Jeff King \n> Signed-off-by: Ramsay Jones \n> Signed-off-by: Lars Schneider \n> ---\n> t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++--------------\n> 1 file changed, 33 insertions(+), 14 deletions(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 73e37a1..94c449a 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -569,6 +569,21 @@ test_line_count () {\n> \tfi\n> }\n> \n> +# Returns success if a comma separated string of keywords ($1) contains a\n> +# given keyword ($2).\n> +# Examples:\n> +# `list_contains \"foo,bar\" bar` returns 0\n> +# `list_contains \"foo\" bar` returns 1\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> # This is not among top-level (test_expect_success | test_expect_failure)\n> # but is a prefix that can be used in the test script, like:\n> #\n> @@ -582,18 +597,31 @@ test_line_count () {\n> # the failure could be due to a segv. We want a controlled failure.\n> \n> test_must_fail () {\n> +\tcase \"$1\" in\n> +\tok=*)\n> +\t\t_test_ok=${1#ok=}\n> +\t\tshift\n> +\t\t;;\n> +\t*)\n> +\t\t_test_ok=\n> +\t\t;;\n> +\tesac\n> \t\"$@\"\n> \texit_code=$?\n> -\tif test $exit_code = 0; then\n> +\tif ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n> +\tthen\n\nminor nit:\n\nI would prefer this was 'if test $exit_code -eq 0 && ! list_contains ...'\n\nie. the test on exit code comes first (and no need for the double quotes).\nThe whole if..elif.. chain is about testing the exit code, with a couple\nof exceptions ...\n\nThe same comment applies to the second patch with exit code 141/SIGPIPE.\n\nATB,\nRamsay Jones\n"},{"id":"273811","messageId":"20151128170355.GB27264@sigill.intra.peff.net","threadId":"40898","inReplyTo":"56584E92.1030103@ramsayjones.plus.com","subject":"Re: [PATCH v1 1/2] implement test_might_fail using a refactored test_must_fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-28T17:03:55Z","receivedAt":"2015-11-28T17:03:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 27, 2015 at 12:37:38PM +0000, Ramsay Jones wrote:\n\n> > -\tif test $exit_code = 0; then\n> > +\tif ! list_contains \"$_test_ok\" success && test \"$exit_code\" -eq 0\n> > +\tthen\n> \n> minor nit:\n> \n> I would prefer this was 'if test $exit_code -eq 0 && ! list_contains ...'\n> \n> ie. the test on exit code comes first (and no need for the double quotes).\n> The whole if..elif.. chain is about testing the exit code, with a couple\n> of exceptions ...\n\nI agree, it makes the logic a bit easier to follow. I've tweaked it\nwhile applying; no need to re-send.\n\n> The same comment applies to the second patch with exit code 141/SIGPIPE.\n\nGot that one, too.\n\n-Peff\n"},{"id":"273812","messageId":"20151128171004.GC27264@sigill.intra.peff.net","threadId":"40898","inReplyTo":"1448615714-43768-3-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1 2/2] add \"ok=sigpipe\" to test_must_fail and use it to fix flaky tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-28T17:10:04Z","receivedAt":"2015-11-28T17:10:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 27, 2015 at 10:15:14AM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" is\n> flaky in the following case:\n> 1. remote upload-pack finds out \"not our ref\"\n> 2. remote sends a response and closes the pipe\n> 3. fetch-pack still tries to write commands to the remote upload-pack\n> 4. write call in wrapper.c dies with SIGPIPE\n> \n> t5504 \"9 - push with transfer.fsckobjects\" is flaky, too, and returns\n> SIGPIPE once in a while. I had to remove the final \"To dst...\" output\n> check because there is no output if the process dies with SIGPUPE.\n\ns/PUPE/PIPE/ :)\n\nI think it would be nice for future readers to understand a bit better\n_why_ this is flaky, and why the fix is to the test suite and not to git\nitself. I added this paragraph in between the two above:\n\n The test is flaky because the sending fetch-pack may or may not have\n finished writing its output by step (3). If it did, then we see a\n closed pipe on the next read() call. If it didn't, then we get the\n SIGPIPE from step (4) above. Both are fine, but the latter fools\n test_must_fail.\n\n-Peff\n"},{"id":"273857","messageId":"990EBE4C-C71F-4F3F-933F-A070D69C94B0@gmail.com","threadId":"40898","inReplyTo":"20151128171004.GC27264@sigill.intra.peff.net","subject":"Re: [PATCH v1 2/2] add \"ok=sigpipe\" to test_must_fail and use it to fix flaky tests","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-01T09:05:21Z","receivedAt":"2015-12-01T09:05:21Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 28 Nov 2015, at 18:10, Jeff King wrote:\n\n> On Fri, Nov 27, 2015 at 10:15:14AM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" is\n>> flaky in the following case:\n>> 1. remote upload-pack finds out \"not our ref\"\n>> 2. remote sends a response and closes the pipe\n>> 3. fetch-pack still tries to write commands to the remote upload-pack\n>> 4. write call in wrapper.c dies with SIGPIPE\n>> \n>> t5504 \"9 - push with transfer.fsckobjects\" is flaky, too, and returns\n>> SIGPIPE once in a while. I had to remove the final \"To dst...\" output\n>> check because there is no output if the process dies with SIGPUPE.\n> \n> s/PUPE/PIPE/ :)\n> \n> I think it would be nice for future readers to understand a bit better\n> _why_ this is flaky, and why the fix is to the test suite and not to git\n> itself. I added this paragraph in between the two above:\n> \n> The test is flaky because the sending fetch-pack may or may not have\n> finished writing its output by step (3). If it did, then we see a\n> closed pipe on the next read() call. If it didn't, then we get the\n> SIGPIPE from step (4) above. Both are fine, but the latter fools\n> test_must_fail.\n> \nSounds good! Thank you :-)\n\n- Lars\n"}]}