{"thread":{"id":"56691","subject":"test-lib.sh musings: test_expect_failure considered harmful","startedAt":"2021-10-12T10:04:25Z","lastAt":"2021-10-14T17:21:29Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"438528","messageId":"87tuhmk19c.fsf@evledraar.gmail.com","threadId":"56691","inReplyTo":null,"subject":"test-lib.sh musings: test_expect_failure considered harmful","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T09:23:40Z","receivedAt":"2021-10-12T10:04:25Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 11 2021, Junio C Hamano wrote:\n\n[Removed \"In-reply-to: <xmqq5yu3b80j.fsf@gitster.g>\" with the Subject\nchange]\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>>  test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '\n>>  \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n>>  \tchmod a-w .git/objects .git/objects/?? &&\n>> -\ttest_must_fail git commit -m second\n>> +\n>> +\tcat >expect <<-\\EOF &&\n>> +\terror: insufficient permission for adding an object to repository database .git/objects\n>> +\terror: insufficient permission for adding an object to repository database .git/objects\n>> +\terror: Error building trees\n>> +\tEOF\n>\n> This is odd.  Shouldn't the test expect one message from write-tree\n> and be marked as expecting a failure until the bug gets fixed?\n\nPresumably with test_expect_failure.\n\nI'll change it, in this case we'd end up with a test_expect_success at\nthe end, so it doesn't matter much & I don't care.\n\nBut FWIW $subject, or at least s/harmful/running with scissors/g :)\n\n[CC'd some recent-ish users of test_expect_failure, and I'm no innocent\nin that department :)]\n\nIn the Perl world (Test::More et al) the \"#TODO\" keyword we map\ntest_expect_failure to (and yeah, I know the latter pre-dates the\nformer...) doesn't generally lead to subtle breakages and mismatched\nexpectations, i.e. you do:\n\n    TODO: {\n        local $TODO = \"not implemented yet\";\n        is($a, $b, \"this is why this in particular fails\");\n    }\n\nSo you generally mark the *specific* thing that fails, as separate from\nyour test setup itself.\n\nBut our test-lib.sh API for it is the equivalent of marking an entire\nlogical test block and its setup as a TODO.\n\nSo the diff below \"passes\". But did we intend for the test_cmp to fail,\nfor the thing to segfault or hit a BUG?\n\nAny of those conditions being hit will have the TODO test pass. So will\nall of it succeeding.\n\n=== snip ===\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex df544bb321f..15724e6a358 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -601,4 +601,13 @@ test_expect_success 'branch -m with the initial branch' '\n \ttest again = $(git -C rename-initial symbolic-ref --short HEAD)\n '\n \n+test_expect_failure 'do stuff' '\n+\tgit config alias.fake-SEGV \"!f() { echo Fake SEGV; exit 139; }; f\" &&\n+\tgit config alias.fake-BUG \"!f() { echo Fake BUG; exit 99; }; f\" &&\n+\n+\tgit fake-BUG >expect &&\n+\tgit fake-SEGV >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n=== snip ===\n\n(Although for the \"suceeding\" case we'll print out a summary from\n\"prove\", but unless you're carefully eyeballing that...).\n\nSo I think \"test_expect_failure\" should be avoided, the only useful way\nof holding it which works in combination with other test-lib.sh features\nthat I've come up with is:\n\n\ttest_expect_success 'setup flaky failure' '\n\t\t[multi-line test code that passes here] &&\n\t\t>setup-todo\n\t'\n\n\tif test -e setup-todo\n\tthen\n\t\ttest_expect_failure 'flaky failure due to XYZ' '\n\t\t\ttest_cmp todo.expect todo.actual\n\t\t'\n\tfi\n\nI.e.:\n\n * Don't say that the failure of your passing test setup is OK too.\n * In doing that don't break --run=N, so that \"test -e setup-todo\" test\n   (or equivalent) is needed, in case the \"setup\" is skipped.\n * Have *just* the \"test_cmp\" (or other specific failure test) in the\n   \"test_expect_failure\"\n\nBut it's only useful if you can't make that a \"! test_cmp\" (or rather, a\nmore specific positive & passing \"test_cmp\".\n\nI.e. it's flaky, or the output/end state is otherwise unknown (but we\nexpect it to be once bugs are fixed).\n\nWe have ~150 uses of test_expect_failure in the test suite, I'm pretty\nsure that <20 of them at most are \"correct\" under the above\ncriteria. E.g. this is ok-ish:\n    \n    t7815-grep-binary.sh-# This test actually passes on platforms where regexec() supports the\n    t7815-grep-binary.sh-# flag REG_STARTEND.\n    t7815-grep-binary.sh-test_expect_success 'git grep ile a' '\n    t7815-grep-binary.sh-   git grep ile a\n    t7815-grep-binary.sh-'\n\nAlthough in that case we should make it a test_expect_success if we can\nget a \"REG_STARTEND\" build flag exported to the test suite. Skimming the\ngrep hits *maybe* some of the ones in \"t9602-cvsimport-branches-tags.sh\"\n(I haven't looked carefully).\n\nBut most of them I Consider Harmful, i.e. they're a bunch of setup code\nthat could be hiding an unexpected bug/segfault. Running into that with\nsome past WIP work (a thing I considered to \"just fail a test_cmp\"\nstarted segfaulting) is why I try to avoid it.\n"},{"id":"438568","messageId":"xmqq4k9m6vkw.fsf@gitster.g","threadId":"56691","inReplyTo":"87tuhmk19c.fsf@evledraar.gmail.com","subject":"Re: test-lib.sh musings: test_expect_failure considered harmful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T16:45:19Z","receivedAt":"2021-10-12T16:45:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Mon, Oct 11 2021, Junio C Hamano wrote:\n>\n> [Removed \"In-reply-to: <xmqq5yu3b80j.fsf@gitster.g>\" with the Subject\n> change]\n\nPlease do not do the former, although it is welcome to change Subject.\n\n> Presumably with test_expect_failure.\n>\n> I'll change it, in this case we'd end up with a test_expect_success at\n> the end, so it doesn't matter much & I don't care.\n\nI do agree with you that compared to expect_success, which requires\n_all_ steps to succeed, so an failure in any of its steps is\nimmediately noticeable, it is harder to write and keep\nexpect_failure useful, because it is not like we are happy to see\nany failure in any step.  We do not expect a failure in many\npreparation and conclusion steps in the &&-chain in expect_failure\nblock, and we consider it is an error if these steps fail.  We only\nwant to mark only a single step to exhibit an expected but undesirable\nbehaviour.\n\nBut even with the shortcomings of expect_failure, it still is much\nbetter than claiming that we expect a bogus outcome.\n\nImproving the shortcomings of expect_failure would be a much better\nuse of our time than advocating an abuse of expect_sucess, I would\nthink.\n"},{"id":"438644","messageId":"87o87ti5n6.fsf@evledraar.gmail.com","threadId":"56691","inReplyTo":"xmqq4k9m6vkw.fsf@gitster.g","subject":"Re: test-lib.sh musings: test_expect_failure considered harmful","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T10:10:43Z","receivedAt":"2021-10-13T10:24:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 12 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> On Mon, Oct 11 2021, Junio C Hamano wrote:\n> [...]\n>> Presumably with test_expect_failure.\n>>\n>> I'll change it, in this case we'd end up with a test_expect_success at\n>> the end, so it doesn't matter much & I don't care.\n>\n> I do agree with you that compared to expect_success, which requires\n> _all_ steps to succeed, so an failure in any of its steps is\n> immediately noticeable, it is harder to write and keep\n> expect_failure useful, because it is not like we are happy to see\n> any failure in any step.  We do not expect a failure in many\n> preparation and conclusion steps in the &&-chain in expect_failure\n> block, and we consider it is an error if these steps fail.  We only\n> want to mark only a single step to exhibit an expected but undesirable\n> behaviour.\n>\n> But even with the shortcomings of expect_failure, it still is much\n> better than claiming that we expect a bogus outcome.\n>\n> Improving the shortcomings of expect_failure would be a much better\n> use of our time than advocating an abuse of expect_sucess, I would\n> think.\n\nI'd like to improve it, but I'll have to get any patch in this are past\nyou :)\n\nMy reading of your opinion from past exchanges is that you find it\nobjectionable to say \"this is a success\" when it's not the /desired/\nbehavior, whereas I think it's valuable to just test for and document\nthe exact existing behavior, even if it's not desirable. So you don't\nreally need a function different from test_expect_success, just a\ncomment saying \"this should change\", or add a (\"non-hash so it's not TAP\nsyntax\") \"TODO\" to the description of the test.\n\nBut if you agree that we shouldn't conflate failures in the different\nsteps I think we're getting somewhere, so to begin with what do you\nthink about the hack in the v2 of my series?\nhttps://lore.kernel.org/git/cover-v2-0.2-00000000000-20211012T142950Z-avarab@gmail.com/\n\nIf we were to prompote those semantics to something that\ntest_expect_failure would use it would be the below, which I think is\nthe only sensible way to use it.\n\nBut that would mean changing all existing test_expect_failure uses in\nthe test suite, so it would need either a pretty large patch, or some\nincremental steps to get there:\n\nBut it will mean we can't use it for any test that's actually flaky, so\nwe'll need a test_expect_flaky, or have some test-specific workarounds\nin those areas.\n\ndiff --git a/t/t7815-grep-binary.sh b/t/t7815-grep-binary.sh\nindex 90ebb64f46e..9a95c9e7d69 100755\n--- a/t/t7815-grep-binary.sh\n+++ b/t/t7815-grep-binary.sh\n@@ -64,7 +64,7 @@ test_expect_success 'git grep ile a' '\n '\n \n test_expect_failure 'git grep .fi a' '\n-\tgit grep .fi a\n+\ttest_must_fail git grep .fi a\n '\n \n test_expect_success 'grep respects binary diff attribute' '\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8361b5c1c57..6d9291b7ead 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -728,8 +728,8 @@ test_known_broken_ok_ () {\n \tthen\n \t\twrite_junit_xml_testcase \"$* (breakage fixed)\"\n \tfi\n-\ttest_fixed=$(($test_fixed+1))\n-\tsay_color error \"ok $test_count - $@ # TODO known breakage vanished\"\n+\ttest_broken=$(($test_broken+1))\n+\tsay_color warn \"not ok $test_count - $@ # TODO known breakage\"\n }\n \n test_known_broken_failure_ () {\n@@ -737,8 +737,8 @@ test_known_broken_failure_ () {\n \tthen\n \t\twrite_junit_xml_testcase \"$* (known breakage)\"\n \tfi\n-\ttest_broken=$(($test_broken+1))\n-\tsay_color warn \"not ok $test_count - $@ # TODO known breakage\"\n+\ttest_fixed=$(($test_fixed+1))\n+\tsay_color error \"not ok $test_count - $@ # TODO a 'known breakage' changed behavior!\"\n }\n \n test_debug () {\n"},{"id":"438649","messageId":"d643863c-7683-fe4b-e4f2-3fefc475006b@gmail.com","threadId":"56691","inReplyTo":"xmqq4k9m6vkw.fsf@gitster.g","subject":"Re: test-lib.sh musings: test_expect_failure considered harmful","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-13T13:05:08Z","receivedAt":"2021-10-13T13:05:14Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/12/2021 12:45 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n>> On Mon, Oct 11 2021, Junio C Hamano wrote:\n>>\n>> [Removed \"In-reply-to: <xmqq5yu3b80j.fsf@gitster.g>\" with the Subject\n>> change]\n> \n> Please do not do the former, although it is welcome to change Subject.\n> \n>> Presumably with test_expect_failure.\n>>\n>> I'll change it, in this case we'd end up with a test_expect_success at\n>> the end, so it doesn't matter much & I don't care.\n> \n> I do agree with you that compared to expect_success, which requires\n> _all_ steps to succeed, so an failure in any of its steps is\n> immediately noticeable, it is harder to write and keep\n> expect_failure useful, because it is not like we are happy to see\n> any failure in any step.  We do not expect a failure in many\n> preparation and conclusion steps in the &&-chain in expect_failure\n> block, and we consider it is an error if these steps fail.  We only\n> want to mark only a single step to exhibit an expected but undesirable\n> behaviour.\n> \n> But even with the shortcomings of expect_failure, it still is much\n> better than claiming that we expect a bogus outcome.\n> \n> Improving the shortcomings of expect_failure would be a much better\n> use of our time than advocating an abuse of expect_sucess, I would\n> think.\n\nI agree that test_expect_failure has these drawbacks. I've recently\nbeen using _expect_success to document \"bad\" behavior so we can verify\nthat behavior changes when that behavior is fixed. But it does have\nthe drawback of looking like we claim the result is by design.\n\nOne possible way to correct this is to create a \"test_expected_failure\"\nhelper that could be placed on the step(s) of the &&-chain that are\nexpected to fail. The helper could set some variable to true if the\nfailure is hit, and false otherwise. It can also convert a failure\ninto a positive result. Then, test_expect_failure could look for that\nvariable's value (after verifying that the &&-chain returns success)\nto show that all expected failures completed correctly.\n\nThis could have the side-effect of having a \"fixed\" test_expect_failure\nshow as a failed test, not a \"TODO\" message.\n\nThanks,\n-Stolee\n"},{"id":"438668","messageId":"xmqq4k9kj15p.fsf@gitster.g","threadId":"56691","inReplyTo":"d643863c-7683-fe4b-e4f2-3fefc475006b@gmail.com","subject":"Re: test-lib.sh musings: test_expect_failure considered harmful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-13T17:16:18Z","receivedAt":"2021-10-13T17:16:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> But even with the shortcomings of expect_failure, it still is much\n>> better than claiming that we expect a bogus outcome.\n>> \n>> Improving the shortcomings of expect_failure would be a much better\n>> use of our time than advocating an abuse of expect_sucess, I would\n>> think.\n>\n> I agree that test_expect_failure has these drawbacks. I've recently\n> been using _expect_success to document \"bad\" behavior so we can verify\n> that behavior changes when that behavior is fixed. But it does have\n> the drawback of looking like we claim the result is by design.\n\nYeah, I think I saw (and I think I used the same technique myself)\npeople expect a bad output with test_expect_success with an in-code\n(not in-log) comment that explicitly says \"This documents the\ncurrent behaviour, which is wrong\", and that is a very acceptable\nsolution, I would think.\n\n> One possible way to correct this is to create a \"test_expected_failure\"\n> helper that could be placed on the step(s) of the &&-chain that are\n> expected to fail. The helper could set some variable to true if the\n> failure is hit, and false otherwise. It can also convert a failure\n> into a positive result. Then, test_expect_failure could look for that\n> variable's value (after verifying that the &&-chain returns success)\n> to show that all expected failures completed correctly.\n\nYup, I would very much like the direction, and further imagine that\nthe above approach can be extended to ...\n\n> This could have the side-effect of having a \"fixed\" test_expect_failure\n> show as a failed test, not a \"TODO\" message.\n\n... avoid such downside.  Perhaps call that magic \"we know this step\nfails currently\" test_known_breakage and declare that we deprecate\nthe use of test_expect_failure in new tests.  Such a test might look\nlike this:\n\n    test_expect_success 'commit error message should not duplicate' '\n\ttest_when_finished \"chmod -R u+rwx .\" &&\n\tchmod u-rwx .git/objects/ &&\n\torig_head=$(git rev-parse HEAD) &&\n\ttest_must_fail git commit --allow-empty -m \"read-only\" 2>rawerr &&\n\tgrep \"insufficient permission\" rawerr >err &&\n\ttest_known_breakage test_line_count = 1 err &&\n\tnew_head=$(git rev-parse HEAD) &&\n\ttest \"$orig_head\" = \"$new_head\"\n    '\n\nwhich may use your trick to turn both failure and success to OK (to\nlet the remainder of the test to continue) but signal the\nsurrounding test_expect_success to say either \"TODO know breakage\"\nor \"Fixed\".\n\nThanks.\n\n"},{"id":"438768","messageId":"87zgrbfrot.fsf@evledraar.gmail.com","threadId":"56691","inReplyTo":"xmqq4k9kj15p.fsf@gitster.g","subject":"Re: test-lib.sh musings: test_expect_failure considered harmful","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T17:11:17Z","receivedAt":"2021-10-14T17:21:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 13 2021, Junio C Hamano wrote:\n\n> Derrick Stolee <stolee@gmail.com> writes:\n>\n>>> But even with the shortcomings of expect_failure, it still is much\n>>> better than claiming that we expect a bogus outcome.\n>>> \n>>> Improving the shortcomings of expect_failure would be a much better\n>>> use of our time than advocating an abuse of expect_sucess, I would\n>>> think.\n>>\n>> I agree that test_expect_failure has these drawbacks. I've recently\n>> been using _expect_success to document \"bad\" behavior so we can verify\n>> that behavior changes when that behavior is fixed. But it does have\n>> the drawback of looking like we claim the result is by design.\n>\n> Yeah, I think I saw (and I think I used the same technique myself)\n> people expect a bad output with test_expect_success with an in-code\n> (not in-log) comment that explicitly says \"This documents the\n> current behaviour, which is wrong\", and that is a very acceptable\n> solution, I would think.\n>\n>> One possible way to correct this is to create a \"test_expected_failure\"\n>> helper that could be placed on the step(s) of the &&-chain that are\n>> expected to fail. The helper could set some variable to true if the\n>> failure is hit, and false otherwise. It can also convert a failure\n>> into a positive result. Then, test_expect_failure could look for that\n>> variable's value (after verifying that the &&-chain returns success)\n>> to show that all expected failures completed correctly.\n>\n> Yup, I would very much like the direction, and further imagine that\n> the above approach can be extended to ...\n>\n>> This could have the side-effect of having a \"fixed\" test_expect_failure\n>> show as a failed test, not a \"TODO\" message.\n>\n> ... avoid such downside.  Perhaps call that magic \"we know this step\n> fails currently\" test_known_breakage and declare that we deprecate\n> the use of test_expect_failure in new tests.  Such a test might look\n> like this:\n>\n>     test_expect_success 'commit error message should not duplicate' '\n> \ttest_when_finished \"chmod -R u+rwx .\" &&\n> \tchmod u-rwx .git/objects/ &&\n> \torig_head=$(git rev-parse HEAD) &&\n> \ttest_must_fail git commit --allow-empty -m \"read-only\" 2>rawerr &&\n> \tgrep \"insufficient permission\" rawerr >err &&\n> \ttest_known_breakage test_line_count = 1 err &&\n> \tnew_head=$(git rev-parse HEAD) &&\n> \ttest \"$orig_head\" = \"$new_head\"\n>     '\n>\n> which may use your trick to turn both failure and success to OK (to\n> let the remainder of the test to continue) but signal the\n> surrounding test_expect_success to say either \"TODO know breakage\"\n> or \"Fixed\".\n\nI don't see how it's a downside. Considering the behavior bad now\nshouldn't entail that we should be fuzzy about testing what exactly *is*\nhappening right now. If one bad but expected state turns into another\nunexpected bad state the test should fail.\n\nIn this case this thread spawned off a fix where we print an error\ntwice, instead of once:\nhttps://lore.kernel.org/git/cover-v2-0.2-00000000000-20211012T142950Z-avarab@gmail.com/#t\n\nThat sucks a bit, but printing pages full of such errors in a loop would\nbe way worse, which we'll hide if we insist on not testing the exact\nemitted output, or on such \"test_known_breakage\" helpers.\n\nIt's just a downside because when we fix bugs we'll need to go through\nthe \"expected failure\" tests and adjust them, but that seems like a\nfeature to me.\n\nNow I can submit a patch that fixes a known bug with no test suite\nchanges, and I might not even notice that I fixed one.\n\nWe may want to have tests for something that really is nondeterministic,\ne.g. for the code I added in 2d3c02f5db6 (die(): stop hiding errors due\nto overzealous recursion guard, 2017-06-21).\n\nBut that'll only be the case for some tiny minority (or none) of the\nexisting callers of \"test_expect_failure\".\n\n1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/\n"}]}