{"thread":{"id":"44303","subject":"[RFC] test-lib: detect common misuse of test_expect_failure","startedAt":"2016-10-16T00:33:54Z","lastAt":"2016-10-17T17:36:27Z","messageCount":3,"participants":["Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"304198","messageId":"xmqqk2day2ry.fsf@gitster.mtv.corp.google.com","threadId":"44303","inReplyTo":null,"subject":"[RFC] test-lib: detect common misuse of test_expect_failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-14T22:38:41Z","receivedAt":"2016-10-16T00:33:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is a very easy mistake to make to say test_expect_failure when\nmaking sure a step in the test fails, which must be spelled\n\"test_must_fail\".  By introducing a toggle $test_in_progress that is\nturned on at the beginning of test_start_() and off at the end of\ntest_finish_() helper, we can detect this fairly easily.\n\nStrictly speaking, writing \"test_expect_success\" inside another\ntest_expect_success (or inside test_expect_failure for that matter)\ncan be detected with the same mechanism if we really wanted to, but\nthat is a lot less likely confusion, so let's not bother.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * It is somewhat embarrassing to admit that I had to stare at the\n   offending code for more than 5 minutes to notice what went wrong\n   to come up with <xmqqr37iy5bw.fsf@gitster.mtv.corp.google.com>;\n   if we had something like this, it would have helped.\n\n t/test-lib-functions.sh | 4 ++++\n t/test-lib.sh           | 3 +++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fdaeb3a96b..fc8c10a061 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -381,6 +381,10 @@ test_verify_prereq () {\n }\n \n test_expect_failure () {\n+\tif test \"$test_in_progress\" = 1\n+\tthen\n+\t\terror \"bug in the test script: did you mean test_must_fail instead of test_expect_failure?\"\n+\tfi\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 11562bde10..4c360216e5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -344,6 +344,7 @@ test_count=0\n test_fixed=0\n test_broken=0\n test_success=0\n+test_in_progress=0\n \n test_external_has_tap=0\n \n@@ -625,6 +626,7 @@ test_run_ () {\n }\n \n test_start_ () {\n+\ttest_in_progress=1\n \ttest_count=$(($test_count+1))\n \tmaybe_setup_verbose\n \tmaybe_setup_valgrind\n@@ -634,6 +636,7 @@ test_finish_ () {\n \techo >&3 \"\"\n \tmaybe_teardown_valgrind\n \tmaybe_teardown_verbose\n+\ttest_in_progress=0\n }\n \n test_skip () {\n"},{"id":"304217","messageId":"20161014235718.2rbejrzs5axhopt7@sigill.intra.peff.net","threadId":"44303","inReplyTo":"xmqqk2day2ry.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] test-lib: detect common misuse of test_expect_failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-14T23:57:18Z","receivedAt":"2016-10-16T00:34:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2016 at 03:38:41PM -0700, Junio C Hamano wrote:\n\n> It is a very easy mistake to make to say test_expect_failure when\n> making sure a step in the test fails, which must be spelled\n> \"test_must_fail\".  By introducing a toggle $test_in_progress that is\n> turned on at the beginning of test_start_() and off at the end of\n> test_finish_() helper, we can detect this fairly easily.\n> \n> Strictly speaking, writing \"test_expect_success\" inside another\n> test_expect_success (or inside test_expect_failure for that matter)\n> can be detected with the same mechanism if we really wanted to, but\n> that is a lot less likely confusion, so let's not bother.\n\nI like the general idea, but I'm not sure how this would interact with\nthe tests in t0000 that test the test suite. It looks like that always\nhappens in a full sub-shell invocation (via run_sub_test_lib_test), so\nwe're OK as long as test_in_progress is not exported (and obviously the\nsubshell cannot accidentally overwrite our variable with a 0 when it is\nfinished).\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index fdaeb3a96b..fc8c10a061 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -381,6 +381,10 @@ test_verify_prereq () {\n>  }\n>  \n>  test_expect_failure () {\n> +\tif test \"$test_in_progress\" = 1\n> +\tthen\n> +\t\terror \"bug in the test script: did you mean test_must_fail instead of test_expect_failure?\"\n> +\tfi\n\nThis follows existing practice for things like the &&-lint-checker, and\nbails out on the whole test script. That sometimes makes it hard to find\nthe problematic test, especially if you're running via something like\n\"prove\", because it doesn't make valid TAP output.\n\nIt might be nicer if we just said \"this test is malformed, and therefore\nfails\", and then you get all the usual niceties for recording and\nfinding the failed test.\n\nI don't think it would be robust enough to try to propagate the error up\nto the outer test_expect_success block (and anyway, you'd also want to\nknow about it in a test_expect_failure block; it's a bug in the test,\nnot a known breakage). But perhaps error() could dump some TAP-like\noutput with a \"virtual\" failed test.\n\nSomething like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 11562bd..dc6b1f5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -299,9 +299,8 @@ TERM=dumb\n export TERM\n \n error () {\n-\tsay_color error \"error: $*\"\n-\tGIT_EXIT_OK=t\n-\texit 1\n+\ttest_failure_ \"$@\"\n+\ttest_done\n }\n \n say () {\n@@ -600,7 +599,7 @@ test_run_ () {\n \t\t# code of other programs\n \t\ttest_eval_ \"(exit 117) && $1\"\n \t\tif test \"$?\" != 117; then\n-\t\t\terror \"bug in the test script: broken &&-chain: $1\"\n+\t\t\terror \"bug in the test script: broken &&-chain\" \"$1\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n\nwhich lets \"make prove\" collect the broken test number.\n\nIt would perhaps need to cover the case when $test_count is \"0\"\nseparately. I dunno. It would be nicer still if we could continue\nrunning other tests in the script, but I think it's impossible to\nrobustly jump back to the outer script.\n\nThese kinds of \"bug in the test suite\" are presumably rare enough that\nthe niceties don't matter that much, but I trigger the &&-checker\nreasonably frequently (that and test_line_count, because I can never\nremember the correct invocation).\n\nAnyway. That's all orthogonal to your patch. I just wondered if we could\ndo better, but AFAICT the right way to do better is to hook into\nerror(), which means your patch would not have to care exactly how it\nfails.\n\n-Peff\n"},{"id":"304289","messageId":"xmqqa8e2x4h8.fsf@gitster.mtv.corp.google.com","threadId":"44303","inReplyTo":"20161014235718.2rbejrzs5axhopt7@sigill.intra.peff.net","subject":"Re: [RFC] test-lib: detect common misuse of test_expect_failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-17T17:36:19Z","receivedAt":"2016-10-17T17:36:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I like the general idea, but I'm not sure how this would interact with\n> the tests in t0000 that test the test suite.\n\nI tried but gave up adding a new test for this to t0000 ;-)\n\n>>  test_expect_failure () {\n>> +\tif test \"$test_in_progress\" = 1\n>> +\tthen\n>> +\t\terror \"bug in the test script: did you mean test_must_fail instead of test_expect_failure?\"\n>> +\tfi\n>\n> This follows existing practice for things like the &&-lint-checker, and\n> bails out on the whole test script.\n\nYes, you guessed correctly where the above came from.\n\n> That sometimes makes it hard to find\n> the problematic test, especially if you're running via something like\n> \"prove\", because it doesn't make valid TAP output.\n\nYeah, true.\n\n> It might be nicer if we just said \"this test is malformed, and therefore\n> fails\", and then you get all the usual niceties for recording and\n> finding the failed test.\n>\n> I don't think it would be robust enough to try to propagate the error up\n> to the outer test_expect_success block (and anyway, you'd also want to\n> know about it in a test_expect_failure block; it's a bug in the test,\n> not a known breakage). But perhaps error() could dump some TAP-like\n> output with a \"virtual\" failed test.\n>\n> Something like:\n> ...\n> which lets \"make prove\" collect the broken test number.\n>\n> It would perhaps need to cover the case when $test_count is \"0\"\n> separately. I dunno. It would be nicer still if we could continue\n> running other tests in the script, but I think it's impossible to\n> robustly jump back to the outer script.\n>\n> These kinds of \"bug in the test suite\" are presumably rare enough that\n> the niceties don't matter that much, but I trigger the &&-checker\n> reasonably frequently (that and test_line_count, because I can never\n> remember the correct invocation).\n>\n> Anyway. That's all orthogonal to your patch. I just wondered if we could\n> do better, but AFAICT the right way to do better is to hook into\n> error(), which means your patch would not have to care exactly how it\n> fails.\n\nYeah, the change to error() may be a good thing to do, but it has\nquite a many callers in t/*lib*.sh and definitely deserves to be a\nseparate patch, not tied to this single test.\n"}]}