{"thread":{"id":"57580","subject":"[PATCH 0/7] test-lib-functions: a better \"test_expect_failure\"","startedAt":"2022-03-18T00:34:14Z","lastAt":"2022-03-24T11:24:13Z","messageCount":18,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Phillip Wood","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"451603","messageId":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":null,"subject":"[PATCH 0/7] test-lib-functions: a better \"test_expect_failure\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:33:55Z","receivedAt":"2022-03-18T00:34:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As explained in detail in 1/7 there's several major shortcomings to\n\"test_expect_failure\", this series adds a \"test_expect_todo\".\n\nIt allows us to assert exactly how something that fails .. fails (no\nmore conflating segfaults for \"exit 1\"), but also emits all the right\noutput & metadata about the test being \"TODO\" that\n\"test_expect_failure\" does.\n\nThe rest of the series then expands on that to convert several more\ntests, along with adding helpers like:\n\n    todo_test_cmp want expect actual\n\nWhere the \"want\" is what we want once we'd mark the test as\n\"test_expect_success\", but the \"expect\" is what we get right now with\nthe failing TODO test.\n\nThis series has a (rather trivial) textual conflict with\njs/ci-github-workflow-markup in \"seen\".\n\nÆvar Arnfjörð Bjarmason (7):\n  test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"\n  test-lib-functions: add and use a \"test_todo\" helper\n  tests: allow test_* in \"test_must_fail_acceptable\" for \"test_todo\"\n  test-lib-functions: add and use a \"todo_test_cmp\" helper\n  test-lib-functions: add and use a \"todo_test_path\" helper\n  test-lib-functions: make test_todo support a --reset\n  sparse tests: convert a TODO test to use \"test_expect_todo\"\n\n t/README                           |  20 +++-\n t/t0000-basic.sh                   |  48 +++++++-\n t/t1021-rerere-in-workdir.sh       |   9 +-\n t/t1060-object-corruption.sh       |   4 +-\n t/t1091-sparse-checkout-builtin.sh |  17 ++-\n t/t1309-early-config.sh            |  14 ++-\n t/t1410-reflog.sh                  |   4 +-\n t/t3600-rm.sh                      |  23 +++-\n t/t6403-merge-file.sh              |  32 +++++-\n t/t7814-grep-recurse-submodules.sh |  60 ++++++----\n t/t7815-grep-binary.sh             |   4 +-\n t/test-lib-functions.sh            | 172 ++++++++++++++++++++++++++++-\n t/test-lib.sh                      |  32 +++++-\n 13 files changed, 379 insertions(+), 60 deletions(-)\n\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451604","messageId":"patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:33:56Z","receivedAt":"2022-03-18T00:34:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add an alternative to the \"test_expect_failure\" function. Like\n\"test_expect_failure\" it will marks a test as \"not ok ... TODO\" in the\nTAP output, and thus document that it's a \"TODO\" test that fails\ncurrently.\n\nUnlike \"test_expect_failure\" it asserts that the tested code in\nexactly one manner, and not any other. We'll thus stop conflating\ne.g. segfaults with other sorts of errors, and generally be able to\ntell if our \"expected to fail\" tests start failing in new and\nunexpected ways.\n\nIn more detail, the these problems of \"test_expect_failure\" are solved\nby this new function.\n\n * When \"test_expect_failure\" is used in combination with\n   \"test_{must,might}_fail\" it will hide segfaults or abort()\n   failures, such as failures due to LSAN.\n\n   This is because we do do the right thing in those helpers, but they\n   themselves are expected to be used within \"test_expect_success\" and\n   return 1. The \"test_expect_failure\" can't differentiate this from a\n   \"real\" failure.\n\n   We could change \"test_{must,might}_fail\" to appropriately carry\n   forward the status code to work around this specific issue, but\n   doing so would be a large semantic change in the current test\n   suite.\n\n * More generally, \"test_expect_failure\" by design doesn't test what\n   does, but just asserts that the test fails in some way.\n\n   For some tests that truly fail in unpredictable ways this behavior\n   makes sense, but it's almost never what our tests want. We know we\n   e.g. have a revision at HEAD~ instead of HEAD, and would like to\n   know conflate that with a potential segfault or other behavior\n   change.\n\nIn summary, this new function behaves exactly like\n\"test_expect_success\", except that its \"success\" is then reported as a\n\"not ok .. TODO\".\n\nLet's convert a few \"test_expect_failure\" uses to the new\n\"test_expect_todo\".\n\nFor previous discussion on this topic see [1] and [2], in particular\nthe points by Junio that it's desired that the \"test_expect_failure\"\noutput differentiate the TODO tests from \"test_expect_success\".\n\n1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/xmqqo824e145.fsf@gitster.g/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/README                     | 20 +++++++++++++++++++-\n t/t0000-basic.sh             | 11 ++++++++---\n t/t1060-object-corruption.sh |  4 ++--\n t/t7815-grep-binary.sh       |  4 ++--\n t/test-lib-functions.sh      | 28 ++++++++++++++++++++++++----\n t/test-lib.sh                | 32 ++++++++++++++++++++++++++++----\n 6 files changed, 83 insertions(+), 16 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex f48e0542cdc..e0aa8ebb0eb 100644\n--- a/t/README\n+++ b/t/README\n@@ -805,10 +805,28 @@ see test-lib-functions.sh for the full list and their options.\n \ttest_expect_success PERL,PYTHON 'yo dawg' \\\n \t    ' test $(perl -E 'print eval \"1 +\" . qx[python -c \"print 2\"]') == \"4\" '\n \n+ - test_expect_todo [<prereq>] <message> <script>\n+\n+   A \"test_expect_success\" which on \"success\" will mark the test as a\n+   TODO test, and on failure will emit a real failure.\n+\n+   This should be used to mark a test whose exact bad behavior is\n+   known, but whose outcome isn't preferred, to distinguish it from\n+   those tests that use \"test_expect_success\" to indicate known and\n+   preferred behavior.\n+\n+   Like test_expect_success this function can optionally use a three\n+   argument invocation with a prerequisite as the first argument.\n+\n  - test_expect_failure [<prereq>] <message> <script>\n \n    This is NOT the opposite of test_expect_success, but is used\n-   to mark a test that demonstrates a known breakage.  Unlike\n+   to mark a test that demonstrates a known breakage whose exact failure\n+   behavior isn't predictable.\n+\n+   If the exact breakage is known the \"test_expect_todo\" function\n+   should be used instead. Usethis function if it's hard to pin down\n+   the exact nature of the failure. Unlike\n    the usual test_expect_success tests, which say \"ok\" on\n    success and \"FAIL\" on failure, this will say \"FIXED\" on\n    success and \"still broken\" on failure.  Failures from these\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b007f0efef2..e46638f1f7b 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -176,6 +176,8 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n \ttest_expect_failure \"pretend we have fixed a known breakage\" \"true\"\n+\ttest_expect_todo \"pretend we have a known TODO\" \"true\"\n+\ttest_expect_todo \"pretend we have a bad TODO\" \"false\"\n \ttest_done\n \tEOF\n \tcheck_sub_test_lib_test mixed-results2 <<-\\EOF\n@@ -192,10 +194,13 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'\n \t> not ok 8 - pretend we have a known breakage # TODO known breakage\n \t> not ok 9 - pretend we have a known breakage # TODO known breakage\n \t> ok 10 - pretend we have fixed a known breakage # TODO known breakage vanished\n+\t> not ok 11 - pretend we have a known TODO # TODO known breakage\n+\t> not ok 12 - pretend we have a bad TODO (broken '\\''test_expect_todo'\\''!)\n+\t> #\tfalse\n \t> # 1 known breakage(s) vanished; please update test(s)\n-\t> # still have 2 known breakage(s)\n-\t> # failed 3 among remaining 7 test(s)\n-\t> 1..10\n+\t> # still have 3 known breakage(s)\n+\t> # failed 4 among remaining 8 test(s)\n+\t> 1..12\n \tEOF\n '\n \ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex bc89371f534..b7023f5644c 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -112,8 +112,8 @@ test_expect_success 'clone --local detects missing objects' '\n \ttest_must_fail git clone --local missing missing-checkout\n '\n \n-test_expect_failure 'clone --local detects misnamed objects' '\n-\ttest_must_fail git clone --local misnamed misnamed-checkout\n+test_expect_todo 'clone --local detects misnamed objects' '\n+\tgit clone --local misnamed misnamed-checkout\n '\n \n test_expect_success 'fetch into corrupted repo with index-pack' '\ndiff --git a/t/t7815-grep-binary.sh b/t/t7815-grep-binary.sh\nindex ac871287c03..2d17e05036e 100755\n--- a/t/t7815-grep-binary.sh\n+++ b/t/t7815-grep-binary.sh\n@@ -64,8 +64,8 @@ test_expect_success 'git grep ile a' '\n \tgit grep ile a\n '\n \n-test_expect_failure 'git grep .fi a' '\n-\tgit grep .fi a\n+test_expect_todo 'git 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-functions.sh b/t/test-lib-functions.sh\nindex 0f439c99d61..a219b126d93 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -718,11 +718,13 @@ test_verify_prereq () {\n \tBUG \"'$test_prereq' does not look like a prereq\"\n }\n \n-test_expect_failure () {\n+_test_expect_todo () {\n+\tlocal func=$1\n+\tshift\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n-\tBUG \"not 2 or 3 parameters to test-expect-failure\"\n+\tBUG \"not 2 or 3 parameters to $func\"\n \ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$@\"\n@@ -730,14 +732,32 @@ test_expect_failure () {\n \t\tsay >&3 \"checking known breakage of $TEST_NUMBER.$test_count '$1': $2\"\n \t\tif test_run_ \"$2\" expecting_failure\n \t\tthen\n-\t\t\ttest_known_broken_ok_ \"$1\"\n+\t\t\tcase \"$func\" in\n+\t\t\ttest_expect_todo) test_known_broken_ok_ \"$func\" \"$1\" ;;\n+\t\t\ttest_expect_failure) test_known_broken_ok_ \"$func\" \"$1\" ;;\n+\t\t\tesac\n \t\telse\n-\t\t\ttest_known_broken_failure_ \"$1\"\n+\t\t\tcase \"$func\" in\n+\t\t\ttest_expect_todo)\n+\t\t\t\ttest_title_=$1\n+\t\t\t\tshift\n+\t\t\t\ttest_failure_ \"$test_title_ (broken 'test_expect_todo'!)\" \"$@\"\n+\t\t\t\t;;\n+\t\t\ttest_expect_failure) test_known_broken_failure_ \"$func\" \"$1\" ;;\n+\t\t\tesac\n \t\tfi\n \tfi\n \ttest_finish_\n }\n \n+test_expect_failure () {\n+\t_test_expect_todo test_expect_failure \"$@\"\n+}\n+\n+test_expect_todo () {\n+\t_test_expect_todo test_expect_todo \"$@\"\n+}\n+\n test_expect_success () {\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9af5fb7674d..fb23a6f6682 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -792,21 +792,45 @@ test_failure_ () {\n }\n \n test_known_broken_ok_ () {\n+\tlocal func=$1\n+\tshift\n+\n \tif test -n \"$write_junit_xml\"\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+\n+\tif test \"$func\" = \"test_expect_todo\"\n+\tthen\n+\t\t# test_expect_todo\n+\t\ttest_broken=$(($test_broken+1))\n+\t\tsay_color warn \"not ok $test_count - $@ # TODO known breakage\"\n+\telse\n+\t\t# test_expect_failure\n+\t\ttest_fixed=$(($test_fixed+1))\n+\t\tsay_color error \"ok $test_count - $@ # TODO known breakage vanished\"\n+\tfi\n }\n \n test_known_broken_failure_ () {\n+\tlocal func=$1\n+\tshift\n+\n \tif test -n \"$write_junit_xml\"\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+\n+\tif test \"$func\" = \"test_expect_todo\"\n+\tthen\n+\t\t# test_expect_todo\n+\t\ttest_fixed=$(($test_fixed+1))\n+\t\tsay_color error \"not ok $test_count - $@ # TODO a 'known breakage' changed behavior!\"\n+\telse\n+\t\t# test_expect_failure\n+\t\ttest_broken=$(($test_broken+1))\n+\t\tsay_color warn \"not ok $test_count - $@ # TODO known breakage\"\n+\tfi\n }\n \n test_debug () {\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451605","messageId":"patch-2.7-0834a3ed049-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 2/7] test-lib-functions: add and use a \"test_todo\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:33:57Z","receivedAt":"2022-03-18T00:34:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Introduce a new \"test_todo\" function for use with the new\n\"test_expect_todo\" function, and convert a couple of tests to use it,\nincluding one added in aecad374ae7 (reflog-walk: don't segfault on\nnon-commit sha1's in the reflog, 2016-01-05).\n\nA good thing about \"test_expect_failure\" is that it allowed for\nwriting the sort of tests added in aecad374ae7, where we declare what\nwe'd like to eventually get, but just mark that we're still failing to\nget that.\n\nFor cases like the one-line \"t7815-grep-binary.sh\" test adjusted in\nthe preceding commit to use \"test_expect_todo\" it's obvious what we'd\neventually like to do, to remove the \"test_must_fail\" and convert\n\"test_expect_todo\" to \"test_expect_success\".\n\nBut in the cases being adjusted here it would be a regression to\nsimply remove the \"test_line_count\", as the \"expect\" part of it is\nwhat we'd eventually like to have pass.\n\nBy introducing a \"test_todo\" we can both assert how something\ncurrently fails, and how it should behave once the relevant feature is\nimplemented (or a bug is fixed).\n\nThe \"test_todo\" function will always run the check for what we\neventually want, and BUG() out if it succeeds. We'll also BUG() out if\nneither succeed.\n\nThis way we can ensure that unlike \"test_expect_failure\" blocks the\nassertions aren't dead or broken code. A few uses of\n\"test_expect_failure\" in the test suite (not altered here) are\nhopelessly broken, and could never pass even if a bug was fixed or a\ngiven feature was implemented.\n\nWe just haven't noticed because we've been treating all failures in\n\"test_expect_failure\" equally, using \"test_expect_todo\" with this\n\"test_todo\" helper is a better way forward.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0000-basic.sh        | 37 ++++++++++++++++++++\n t/t1410-reflog.sh       |  4 +--\n t/t6403-merge-file.sh   |  9 +++--\n t/test-lib-functions.sh | 77 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 123 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex e46638f1f7b..bbe48d63b16 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -204,6 +204,43 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'\n \tEOF\n '\n \n+test_expect_success 'subtest: test_expect_todo with test_todo' '\n+\twrite_and_run_sub_test_lib_test test-expect-todo <<-\\EOF &&\n+\ttest_expect_todo \"command not implemented yet\" \"\n+\t\ttest_todo \\\n+\t\t\t--want \\\"git unimplemented\\\" \\\n+\t\t\t--expect \\\"test_must_fail git unimplemented\\\"\n+\t\"\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test test-expect-todo <<-\\EOF\n+\t> not ok 1 - command not implemented yet # TODO known breakage\n+\t> # still have 1 known breakage(s)\n+\t> 1..1\n+\tEOF\n+'\n+\n+test_expect_success 'subtest: test_expect_todo with test_todo: prefix + suffix arguments' '\n+\twrite_and_run_sub_test_lib_test test-expect-todo-pfx <<-\\EOF &&\n+\ttest_expect_todo \"prefix argument for test_todo\" \"\n+\t\techo x >want &&\n+\t\techo y >expect &&\n+\t\tcp expect actual &&\n+\t\ttest_todo test_cmp \\\n+\t\t\t--want want \\\n+\t\t\t--expect expect \\\n+\t\t\t-- \\\n+\t\t\tactual\n+\t\"\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test test-expect-todo-pfx <<-\\EOF\n+\t> not ok 1 - prefix argument for test_todo # TODO known breakage\n+\t> # still have 1 known breakage(s)\n+\t> 1..1\n+\tEOF\n+'\n+\n test_expect_success 'subtest: --verbose option' '\n \twrite_and_run_sub_test_lib_test_err t1234-verbose --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 68f69bb5431..fcd5ff4e313 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -369,9 +369,9 @@ test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n \tgit reflog refs/tests/tree-in-reflog\n '\n \n-test_expect_failure 'reflog with non-commit entries displays all entries' '\n+test_expect_todo 'reflog with non-commit entries displays all entries' '\n \tgit reflog refs/tests/tree-in-reflog >actual &&\n-\ttest_line_count = 3 actual\n+\ttest_todo test_line_count --want \"= 3\" --expect \"= 2\" -- actual\n '\n \n # This test takes a lock on an individual ref; this is not supported in\ndiff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh\nindex 2f421d967ab..12b334af85c 100755\n--- a/t/t6403-merge-file.sh\n+++ b/t/t6403-merge-file.sh\n@@ -82,9 +82,14 @@ test_expect_success \"merge without conflict (--quiet)\" '\n \tgit merge-file --quiet test.txt orig.txt new2.txt\n '\n \n-test_expect_failure \"merge without conflict (missing LF at EOF)\" '\n+test_expect_todo \"merge without conflict (missing LF at EOF)\" '\n \tcp new1.txt test2.txt &&\n-\tgit merge-file test2.txt orig.txt new4.txt\n+\ttest_todo \\\n+\t\ttest_expect_code \\\n+\t\t--expect 1 \\\n+\t\t--want 0 \\\n+\t\t-- \\\n+\t\tgit merge-file test2.txt orig.txt new4.txt\n '\n \n test_expect_failure \"merge result added missing LF\" '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex a219b126d93..53335393b9b 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -959,6 +959,83 @@ test_path_is_missing () {\n \tfi\n }\n \n+# Usage: test_todo [<common-prefix>] <options> [-- <common-suffix>...]\n+#\t--want <want>\n+#\t\tThe condition we'd like. Injected between\n+#\t\t<common-prefix> and <common-suffix> arguments.\n+#\t--expect <expect>\n+#\t\tThe condition we have now. Injected in the same way as\n+#\t\tthe arguments to --want.\n+#\n+# test_todo is a wrapper for use with \"test_expect_todo\". It declares\n+# an outcome we want, and one we currently expect:\n+#\n+#\ttest_todo --want true --expect false\n+#\n+# It can also take a <common-prefix> prefix along with\n+# <common-suffix>... parameters after a \"--\", e.g.:\n+#\n+#\t# We want 1 line, not 2\n+# \ttest_todo test_line_count --want \"= 1\" --expect \"= 2\" -- actual\n+#\n+# Here both variants of the \"test_line_count\" will be run to assert\n+# that the \"want\" variant doesn't pass yet, and that the \"expect\"\n+# variant describes the current behavior.\n+#\n+# Because we run both neither of them can mutate the test\n+# state. I.e. they must be read-only commands such as \"wc -l\", and not\n+# a state-altering command such as \"rm\".\n+test_todo () {\n+\tlocal common_fn= &&\n+\tlocal have_want= &&\n+\tlocal want= &&\n+\tlocal expect= &&\n+\tlocal have_expect= &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t--want)\n+\t\t\twant=\"$2\" &&\n+\t\t\thave_want=t &&\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--expect)\n+\t\t\texpect=\"$2\" &&\n+\t\t\thave_expect=t &&\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--)\n+\t\t\tshift &&\n+\t\t\tbreak\n+\t\t\t;;\n+\t\t*)\n+\t\t\tif test -n \"$common_fn\"\n+\t\t\tthen\n+\t\t\t\tBUG \"the <common-fn> can only be given once\" &&\n+\t\t\t\treturn 1\n+\t\t\tfi &&\n+\t\t\tcommon_fn=\"$1\"\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tif test \"$have_want$have_expect\" != \"tt\"\n+\tthen\n+\t\tBUG \"test_todo must get a --want <want> and --expect <expect>\"\n+\tfi &&\n+\n+\tif $common_fn $want \"$@\"\n+\tthen\n+\t\tBUG \"a test_todo succeeded with --want ('$want').  Turn it into a test_expect_success + $@ $want?\" &&\n+\t\treturn 1\n+\telif $common_fn $expect \"$@\"\n+\tthen\n+\t\tsay \"a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead\" >&3 &&\n+\t\treturn 0\n+\tfi &&\n+\tBUG \"a test_todo didn't pass with either --want ('$want') or --expect ('$expect')\"\n+}\n+\n # test_line_count checks that a file has the number of lines it\n # ought to. For example:\n #\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451606","messageId":"patch-3.7-ce64e5a5268-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 3/7] tests: allow test_* in \"test_must_fail_acceptable\" for \"test_todo\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:33:58Z","receivedAt":"2022-03-18T00:34:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Convert tests added in [1] and [2] to use \"test_expect_todo\" to more\nspecifically mark the failure condition, rather then the blank check\nfor failure given by \"test_expect_failure\".\n\nFor [1] there isn't an easy way to fit the \"echo world\" and \"test_cmp\"\ninto a \"test_todo\", and in any case we might not have that output once\nwe fix the bug noted in [1].\n\nFor [2] we'd need add \"test_with_config\" (which invokes both \"git\" and\n\"test-tool\" to the list in \"test_must_fail_acceptable\", but adding a\nfile-specific function to \"test-lib-functions.sh\" would be a bit odd.\n\nLet's instead expand the restrictive list added in\n6a67c759489 (test-lib-functions: restrict test_must_fail usage,\n2020-07-07). As shown in the commits that preceded it the list came\nabout because we wanted to exclude the likes of \"cvs\" and \"p4\" from\n\"test_might_fail\".\n\nIt's a fair bet that if we use it with a test_* function that that\nfunction is being appropriately used with it (i.e. invokes one of or\nown programs). It's possible that without an exhaustive list we'll get\nit wrong, but I also don't think it's worth it to maintain that\nexhaustive list. Let's just allow all test_* names instead.\n\n1. 90a6464b4ad (rerere: make sure it works even in a workdir attached\n   to a young repository, 2011-03-10\n2. 751d3b9415f (t1309: document cases where we would want early config\n   not to die(), 2017-03-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t1021-rerere-in-workdir.sh |  9 +++++----\n t/t1309-early-config.sh      | 14 ++++++++++----\n t/test-lib-functions.sh      |  2 +-\n 3 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1021-rerere-in-workdir.sh b/t/t1021-rerere-in-workdir.sh\nindex 0b892894eb9..e4f4b9124c3 100755\n--- a/t/t1021-rerere-in-workdir.sh\n+++ b/t/t1021-rerere-in-workdir.sh\n@@ -41,7 +41,7 @@ test_expect_success SYMLINKS 'rerere in workdir' '\n # For the purpose of helping contrib/workdir/git-new-workdir users, we do not\n # have to support relative symlinks, but it might be nicer to make this work\n # with a relative symbolic link someday.\n-test_expect_failure SYMLINKS 'rerere in workdir (relative)' '\n+test_expect_todo SYMLINKS 'rerere in workdir (relative)' '\n \trm -rf .git/rr-cache &&\n \t\"$SHELL_PATH\" \"$TEST_DIRECTORY/../contrib/workdir/git-new-workdir\" . krow &&\n \t(\n@@ -49,9 +49,10 @@ test_expect_failure SYMLINKS 'rerere in workdir (relative)' '\n \t\trm -f .git/rr-cache &&\n \t\tln -s ../.git/rr-cache .git/rr-cache &&\n \t\ttest_must_fail git merge side &&\n-\t\tgit rerere status >actual &&\n-\t\techo world >expect &&\n-\t\ttest_cmp expect actual\n+\t\ttest_todo \\\n+\t\t\t--want \"git\" \\\n+\t\t\t--expect \"test_must_fail git\" \\\n+\t\t\t -- rerere status\n \t)\n '\n \ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex 537435b90ae..fedbdac621a 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -81,13 +81,19 @@ test_expect_success 'ignore .git/ with incompatible repository version' '\n \ttest_i18ngrep \"warning:.* Expected git repo version <= [1-9]\" err\n '\n \n-test_expect_failure 'ignore .git/ with invalid repository version' '\n-\ttest_with_config \"[core]repositoryformatversion = invalid\"\n+test_expect_todo 'ignore .git/ with invalid repository version' '\n+\ttest_todo \\\n+\t\t--want test_with_config \\\n+\t\t--expect \"test_must_fail test_with_config\" \\\n+\t\t-- \"[core]repositoryformatversion = invalid\"\n '\n \n \n-test_expect_failure 'ignore .git/ with invalid config' '\n-\ttest_with_config \"[\"\n+test_expect_todo 'ignore .git/ with invalid config' '\n+\ttest_todo \\\n+\t\t--want test_with_config \\\n+\t\t--expect \"test_must_fail test_with_config\" \\\n+\t\t-- \"[\"\n '\n \n test_expect_success 'early config and onbranch' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 53335393b9b..64b9580f2bc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1127,7 +1127,7 @@ test_must_fail_acceptable () {\n \tfi\n \n \tcase \"$1\" in\n-\tgit|__git*|test-tool|test_terminal)\n+\tgit|__git*|test-tool|test_*)\n \t\treturn 0\n \t\t;;\n \t*)\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451607","messageId":"patch-4.7-25a4dadd5f3-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 4/7] test-lib-functions: add and use a \"todo_test_cmp\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:33:59Z","receivedAt":"2022-03-18T00:34:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Introduce a new \"todo_test_cmp\" for use with the new\n\"test_expect_todo\" function. This is a thin wrapper around the\npreviously introduced \"test_todo\". Instead of the more verbose:\n\n    test_todo test_cmp --want want --expect expect -- actual\n\nWe can now do:\n\n    todo_test_cmp want expect actual\n\nSince it uses \"test_todo\", this \"test_cmp_todo\" function will BUG()\nout if \"want\" and \"expect\" are the same, and likewise if the \"want\" is\nequivalent to \"actual\".\n\nLet's convert most of the tests added in 45bde58ef8f (grep:\ndemonstrate bug with textconv attributes and submodules, 2021-09-29)\nto use it, as well as a merge test added in\n6d49de414f9 (t6023-merge-file.sh: fix and mark as broken invalid\ntests, 2014-06-29).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t6403-merge-file.sh              | 23 +++++++++++--\n t/t7814-grep-recurse-submodules.sh | 54 +++++++++++++++++++++---------\n t/test-lib-functions.sh            | 22 ++++++++++++\n 3 files changed, 81 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh\nindex 12b334af85c..d466360c41a 100755\n--- a/t/t6403-merge-file.sh\n+++ b/t/t6403-merge-file.sh\n@@ -92,8 +92,27 @@ test_expect_todo \"merge without conflict (missing LF at EOF)\" '\n \t\tgit merge-file test2.txt orig.txt new4.txt\n '\n \n-test_expect_failure \"merge result added missing LF\" '\n-\ttest_cmp test.txt test2.txt\n+test_expect_todo \"merge result added missing LF\" '\n+\tcat >expect <<-\\EOF &&\n+\tDominus regit me, et nihil mihi deerit.\n+\tIn loco pascuae ibi me collocavit,\n+\tsuper aquam refectionis educavit me;\n+\tanimam meam convertit,\n+\tdeduxit me super semitas jusitiae,\n+\t<<<<<<< test2.txt\n+\t<<<<<<< test2.txt\n+\tpropter nomen suum.\n+\tNam et si ambulavero in medio umbrae mortis,\n+\tnon timebo mala, quoniam tu mecum es:\n+\tvirga tua et baculus tuus ipsa me consolata sunt.\n+\t=======\n+\tpropter nomen suum.\n+\t>>>>>>> new4.txt\n+\t=======\n+\tpropter nomen suum.\n+\t>>>>>>> new4.txt\n+\tEOF\n+\ttodo_test_cmp test.txt expect test2.txt\n '\n \n test_expect_success \"merge without conflict (missing LF at EOF, away from change in the other file)\" '\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex a4476dc4922..8d9b53ccfed 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -442,77 +442,98 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n \ttest_must_be_empty actual\n '\n \n-test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '\n+test_expect_todo 'grep --textconv: superproject .gitattributes does not affect submodules' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \techo \"a diff=d2x\" >.gitattributes &&\n \n+\tcat >want <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n \tcat >expect <<-\\EOF &&\n \ta:(1|2)x(3|4)\n+\tsubmodule/a:(1|2)x(3|4)\n+\tsubmodule/sub/a:(1|2)x(3|4)\n \tEOF\n \tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\ttodo_test_cmp want expect actual\n '\n \n-test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n+test_expect_todo 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \techo \"a diff=d2x\" >.gitattributes &&\n \tgit add .gitattributes &&\n \trm .gitattributes &&\n \n+\tcat >want <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n \tcat >expect <<-\\EOF &&\n \ta:(1|2)x(3|4)\n+\tsubmodule/a:(1|2)x(3|4)\n+\tsubmodule/sub/a:(1|2)x(3|4)\n \tEOF\n \tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\ttodo_test_cmp want expect actual\n '\n \n-test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n+test_expect_todo 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \tsuper_attr=\"$(git rev-parse --git-path info/attributes)\" &&\n \ttest_when_finished \"rm -f \\\"$super_attr\\\"\" &&\n \techo \"a diff=d2x\" >\"$super_attr\" &&\n \n+\tcat >want <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n \tcat >expect <<-\\EOF &&\n \ta:(1|2)x(3|4)\n+\tsubmodule/a:(1|2)x(3|4)\n+\tsubmodule/sub/a:(1|2)x(3|4)\n \tEOF\n \tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\ttodo_test_cmp want expect actual\n '\n \n # Note: what currently prevents this test from passing is not that the\n # .gitattributes file from \"./submodule\" is being ignored, but that it is being\n # propagated to the nested \"./submodule/sub\" files.\n #\n-test_expect_failure 'grep --textconv correctly reads submodule .gitattributes' '\n+test_expect_todo 'grep --textconv correctly reads submodule .gitattributes' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \techo \"a diff=d2x\" >submodule/.gitattributes &&\n \n+\tcat >want <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n \tcat >expect <<-\\EOF &&\n \tsubmodule/a:(1|2)x(3|4)\n+\tsubmodule/sub/a:(1|2)x(3|4)\n \tEOF\n \tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\ttodo_test_cmp want expect actual\n '\n \n-test_expect_failure 'grep --textconv correctly reads submodule .gitattributes (from index)' '\n+test_expect_todo 'grep --textconv correctly reads submodule .gitattributes (from index)' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \techo \"a diff=d2x\" >submodule/.gitattributes &&\n \tgit -C submodule add .gitattributes &&\n \trm submodule/.gitattributes &&\n \n-\tcat >expect <<-\\EOF &&\n+\tcat >want <<-\\EOF &&\n \tsubmodule/a:(1|2)x(3|4)\n \tEOF\n-\tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\t>expect &&\n+\n+\ttest_might_fail git grep --textconv --recurse-submodules x >actual &&\n+\ttodo_test_cmp want expect actual\n '\n \n-test_expect_failure 'grep --textconv correctly reads submodule .git/info/attributes' '\n+test_expect_todo 'grep --textconv correctly reads submodule .git/info/attributes' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \n@@ -520,11 +541,12 @@ test_expect_failure 'grep --textconv correctly reads submodule .git/info/attribu\n \ttest_when_finished \"rm -f \\\"$submodule_attr\\\"\" &&\n \techo \"a diff=d2x\" >\"$submodule_attr\" &&\n \n-\tcat >expect <<-\\EOF &&\n+\tcat >want <<-\\EOF &&\n \tsubmodule/a:(1|2)x(3|4)\n \tEOF\n-\tgit grep --textconv --recurse-submodules x >actual &&\n-\ttest_cmp expect actual\n+\t>expect &&\n+\ttest_might_fail git grep --textconv --recurse-submodules x >actual &&\n+\ttodo_test_cmp want expect actual\n '\n \n test_expect_failure 'grep saves textconv cache in the appropriate repository' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 64b9580f2bc..4d1eca380e8 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1263,6 +1263,28 @@ test_cmp () {\n \teval \"$GIT_TEST_CMP\" '\"$@\"'\n }\n \n+# todo_test_cmp is a \"test_cmp\" for use in conjunction with\n+# \"test_expect_todo\".\n+#\n+# It takes a mandatory extra first argument of \"want\", indicating the\n+# output we'd like to have once we turn that \"test_expect_todo\" into a\n+# \"test_expect_success\":\n+#\n+#\ttest_expect_todo 'foo still doesn't work' '\n+#\t\techo yay >want &&\n+#\t\techo error >expect &&\n+#\t\tfoo >actual &&\n+#\t\ttest_cmp want expect actual\n+#\t'\n+todo_test_cmp () {\n+\ttest \"$#\" -ne 3 && BUG \"3 param, not $#\"\n+\tlocal want=$1 &&\n+\tlocal expect=$2 &&\n+\tlocal actual=$3 &&\n+\n+\ttest_todo test_cmp --want \"$want\" --expect \"$expect\" -- \"$actual\"\n+}\n+\n # Check that the given config key has the expected value.\n #\n #    test_cmp_config [-C <dir>] <expected-value>\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451608","messageId":"patch-6.7-2ee27a7773e-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 6/7] test-lib-functions: make test_todo support a --reset","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:34:01Z","receivedAt":"2022-03-18T00:34:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the preceding commit that introduced \"test_todo\" we\ncouldn't run something like \"git rm\" since we run both the --want and\n--expect variants, and if --want has removed a file the --expect won't\nsucceed.\n\nLet's add a --reset option to the command, this allows us to convert a\ntest added in 03415ca8db2 (t3600: document failure of rm across\nsymbolic links, 2013-04-04) to a more exact \"test_expect_todo\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t3600-rm.sh           | 23 ++++++++++++++++++-----\n t/test-lib-functions.sh | 26 ++++++++++++++++++++++----\n 2 files changed, 40 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex e74a318ac33..42879e9060b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -790,7 +790,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (no index)' '\n \ttest_path_is_file e/f\n '\n \n-test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n+test_expect_todo SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n \trm -rf d e &&\n \tmkdir d &&\n \techo content >d/f &&\n@@ -798,10 +798,23 @@ test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n \tgit commit -m \"d/f exists\" &&\n \tmv d e &&\n \tln -s e d &&\n-\ttest_must_fail git rm d/f &&\n-\tgit rev-parse --verify :d/f &&\n-\ttest -h d &&\n-\ttest_path_is_file e/f\n+\ttest_todo \\\n+\t\t--want \"test_must_fail git\" \\\n+\t\t--reset \"git reset --hard\" \\\n+\t\t--expect git \\\n+\t\t-- \\\n+\t\trm d/f &&\n+\ttest_todo \\\n+\t\t--want git \\\n+\t\t--expect \"test_must_fail git\" \\\n+\t\t-- \\\n+\t\trev-parse --verify :d/f &&\n+\ttest_todo \\\n+\t\t--want \"test -h\" \\\n+\t\t--expect \"test_path_is_missing\" \\\n+\t\t-- \\\n+\t\td &&\n+\ttodo_test_path is_file is_missing e/f\n '\n \n test_expect_success 'setup for testing rm messages' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3febf4b0811..5313ab28e72 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -966,6 +966,10 @@ test_path_is_missing () {\n #\t--expect <expect>\n #\t\tThe condition we have now. Injected in the same way as\n #\t\tthe arguments to --want.\n+#\t--reset <reset>\n+#\t\tA command to run between the <want> and <expect>\n+#\t\tconditions to reset the repository state. Used e.g. if\n+#\t\tboth run a \"git rm\" command that might succeed.\n #\n # test_todo is a wrapper for use with \"test_expect_todo\". It declares\n # an outcome we want, and one we currently expect:\n@@ -985,8 +989,12 @@ test_path_is_missing () {\n # Because we run both neither of them can mutate the test\n # state. I.e. they must be read-only commands such as \"wc -l\", and not\n # a state-altering command such as \"rm\".\n+#\n+# To run a command that mutates the repository state supply a --reset\n+# option, e.g. \"git reset --hard\" if you need to run \"git rm\".\n test_todo () {\n \tlocal common_fn= &&\n+\tlocal reset= &&\n \tlocal have_want= &&\n \tlocal want= &&\n \tlocal expect= &&\n@@ -1004,6 +1012,10 @@ test_todo () {\n \t\t\thave_expect=t &&\n \t\t\tshift\n \t\t\t;;\n+\t\t--reset)\n+\t\t\treset=\"$2\" &&\n+\t\t\tshift\n+\t\t\t;;\n \t\t--)\n \t\t\tshift &&\n \t\t\tbreak\n@@ -1028,10 +1040,16 @@ test_todo () {\n \tthen\n \t\tBUG \"a test_todo succeeded with --want ('$want').  Turn it into a test_expect_success + $@ $want?\" &&\n \t\treturn 1\n-\telif $common_fn $expect \"$@\"\n-\tthen\n-\t\tsay \"a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead\" >&3 &&\n-\t\treturn 0\n+\telse\n+\t\tif test -n \"$reset\"\n+\t\tthen\n+\t\t\t$reset\n+\t\tfi &&\n+\t\tif $common_fn $expect \"$@\"\n+\t\tthen\n+\t\t\tsay \"a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead\" >&3 &&\n+\t\t\treturn 0\n+\t\tfi\n \tfi &&\n \tBUG \"a test_todo didn't pass with either --want ('$want') or --expect ('$expect')\"\n }\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451609","messageId":"patch-5.7-553670da8a9-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 5/7] test-lib-functions: add and use a \"todo_test_path\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:34:00Z","receivedAt":"2022-03-18T00:34:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a new \"todo_test_path\" helper and convert an additional test added\nin 45bde58ef8f (grep: demonstrate bug with textconv attributes and\nsubmodules, 2021-09-29) to use it in conjunction with\n\"test_expect_todo\".\n\nLike the \"todo_test_cmp\" function introduced in a preceding commit,\nthis function is a trivial wrapper around \"test_todo\". Rather than a\nmore verbose:\n\n\ttest_todo \\\n\t\t--want \"test_path_is_missing\" \\\n\t\t--expect \"test_path_is_file\" \\\n\t\t-- \"$super_textconv_cache\"\n\nWe can do:\n\n\ttodo_test_path is_missing is_file \"$super_textconv_cache\"\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t7814-grep-recurse-submodules.sh |  6 +++---\n t/test-lib-functions.sh            | 25 +++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 8d9b53ccfed..8df692ee9a0 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -549,7 +549,7 @@ test_expect_todo 'grep --textconv correctly reads submodule .git/info/attributes\n \ttodo_test_cmp want expect actual\n '\n \n-test_expect_failure 'grep saves textconv cache in the appropriate repository' '\n+test_expect_todo 'grep saves textconv cache in the appropriate repository' '\n \treset_and_clean &&\n \ttest_config_global diff.d2x_cached.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n \ttest_config_global diff.d2x_cached.cachetextconv true &&\n@@ -562,8 +562,8 @@ test_expect_failure 'grep saves textconv cache in the appropriate repository' '\n \tsuper_textconv_cache=\"$(git rev-parse --git-path refs/notes/textconv/d2x_cached)\" &&\n \tsub_textconv_cache=\"$(git -C submodule rev-parse \\\n \t\t\t--path-format=absolute --git-path refs/notes/textconv/d2x_cached)\" &&\n-\ttest_path_is_missing \"$super_textconv_cache\" &&\n-\ttest_path_is_file \"$sub_textconv_cache\"\n+\ttodo_test_path is_missing is_file \"$super_textconv_cache\" &&\n+\ttodo_test_path is_file is_missing \"$sub_textconv_cache\"\n '\n \n test_expect_success 'grep partially-cloned submodule' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 4d1eca380e8..3febf4b0811 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1036,6 +1036,31 @@ test_todo () {\n \tBUG \"a test_todo didn't pass with either --want ('$want') or --expect ('$expect')\"\n }\n \n+# todo_test_path is a test_path_* for use in conjunction with\n+# \"test_expect_todo\".\n+#\n+# It takes \"want_fn\" and \"expect_fn\" arguments of e.g. \"is_file\" or\n+# \"is_dir\", which will be turned into corresponding \"test_file_*\"\n+# calls. Use it like:\n+#\n+#\ttest_expect_todo 'foo should be a directory' '\n+#\t\t>foo &&\n+#\t\ttodo_test_path is_dir is_file foo\n+#\t'\n+todo_test_path () {\n+\ttest \"$#\" -ne 3 && BUG \"3 param, not $#\"\n+\tlocal want_fn=$1\n+\tlocal expect_fn=$2\n+\tlocal path=$3 &&\n+\tshift 3 &&\n+\n+\ttest_todo \\\n+\t\t--want \"test_path_$want_fn\" \\\n+\t\t--expect \"test_path_$expect_fn\" \\\n+\t\t-- \\\n+\t\t\"$path\"\n+}\n+\n # test_line_count checks that a file has the number of lines it\n # ought to. For example:\n #\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451610","messageId":"patch-7.7-10677b32b78-20220318T002951Z-avarab@gmail.com","threadId":"57580","inReplyTo":"cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com","subject":"[PATCH 7/7] sparse tests: convert a TODO test to use \"test_expect_todo\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T00:34:02Z","receivedAt":"2022-03-18T00:34:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a test that used \"test_expect_success\" before 49fdd51a235 (add:\nskip tracked paths outside sparse-checkout cone, 2021-09-24) to use\n\"test_expect_todo\" instead.\n\nNow we'll test for the exact current behavior, while documenting what\nbehavior we'd like to get instead.\n\nThis test is a good example of the sort of cases where we benefit most\nfrom \"test_expect_todo\". In 49fdd51a235 the only change here (aside\nfrom the \"NEEDSWORK\" comment) was changing \"test_expect_success\" to\n\"test_expect_failure\".\n\nWe thus lost test coverage, and would not have noticed if we failed in\nsome unexpected place in this rather large test (it's 30 lines of\nsetup before getting to the \"NEEDSWORK\" comment). Now we can get the\ntest coverage back, while documenting what is and isn't desired\nbehavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t1091-sparse-checkout-builtin.sh | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 9a900310186..23973bc186b 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -467,7 +467,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat\n \tgit -C unmerged sparse-checkout disable\n '\n \n-test_expect_failure 'sparse-checkout reapply' '\n+test_expect_todo 'sparse-checkout reapply' '\n \tgit clone repo tweak &&\n \n \techo dirty >tweak/deep/deeper2/a &&\n@@ -501,11 +501,18 @@ test_expect_failure 'sparse-checkout reapply' '\n \n \t# NEEDSWORK: We are asking to update a file outside of the\n \t# sparse-checkout cone, but this is no longer allowed.\n-\tgit -C tweak add folder1/a &&\n+\ttest_todo test_expect_code \\\n+\t\t--want 0 \\\n+\t\t--expect 1 \\\n+\t\t-- \\\n+\t\tgit -C tweak add folder1/a &&\n \tgit -C tweak sparse-checkout reapply 2>err &&\n-\ttest_must_be_empty err &&\n-\ttest_path_is_missing tweak/deep/deeper2/a &&\n-\ttest_path_is_missing tweak/folder1/a &&\n+\ttest_todo \\\n+\t\t--want test_must_be_empty \\\n+\t\t--expect \"grep warning:.*paths.*unmerged\" \\\n+\t\t-- err &&\n+\ttodo_test_path is_file is_missing tweak/deep/deeper2/a &&\n+\ttodo_test_path is_missing is_file tweak/folder1/a &&\n \n \tgit -C tweak sparse-checkout disable\n '\n-- \n2.35.1.1436.g756b814e59f\n\n"},{"id":"451640","messageId":"xmqqczij9jeg.fsf@gitster.g","threadId":"57580","inReplyTo":"patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T18:59:19Z","receivedAt":"2022-03-18T18:59:27Z","isPatch":true,"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> Add an alternative to the \"test_expect_failure\" function. Like\n> \"test_expect_failure\" it will marks a test as \"not ok ... TODO\" in the\n\n\"will marks\" -> \"marks\".\n\n> TAP output, and thus document that it's a \"TODO\" test that fails\n> currently.\n>\n> Unlike \"test_expect_failure\" it asserts that the tested code in\n> exactly one manner, and not any other.\n\nECANNOTPARSE due to a verb missing.  For now I'll assume \"It asserts\nthat the tested code fails in exactly one matter\" and keep going.\n\n> We'll thus stop conflating\n> e.g. segfaults with other sorts of errors, and generally be able to\n> tell if our \"expected to fail\" tests start failing in new and\n> unexpected ways.\n\nThe above makes it sound wonderful but I got somewhat confused when\nI tried to see how well it conveys what it wants to tell by picking\nan example from this patch.  Say, for example:\n\n> -test_expect_failure 'git grep .fi a' '\n> -\tgit grep .fi a\n> +test_expect_todo 'git grep .fi a' '\n> +\ttest_must_fail git grep .fi a\n>  '\n\nSo, we used to say\n\n    \"We eventually, when things are fixed, want 'git grep' to find a\n    string '.fi' in file 'a', but currently this does not work\".\n\nNow the updated one says\n\n    \"We know 'git grep' fails to find string '.fi' in file 'a'\"\n\nIf it is a trivial single statement test like the above, the\ndistinction does not matter, but if it were a test with multiple\nsteps, the readability would become quite different.  It would turn\n\n\ttest_expect_failure 'sample test' '\n\t\tpreparation 1 &&\n\t\tpreparation 2 &&\n\t\tgit command invocation that we want to succeed\n\t'\n\ninto\n\n\ttest_expect_todo 'sample test' '\n\t\tpreparation 1 &&\n\t\tpreparation 2 &&\n\t\ttest_must_fail git command invocation that we want to succeed\n\t'\n\n\"expect_failure\" expects the whole thing to fail, so we will miss if\nany preparation steps fail, but \"expect_todo\" is like \"expect_success\"\nso it will help us debuging the test by catching a silly mistake in\npreparation steps.\n\nMarking the step that demonstrates the current shortcomings with\n\"MUST FAIL\" is a bad taste, but let's pretend that we didn't notice\nit ;-).  Other than that, it looks like an improvement.\n\n>  * More generally, \"test_expect_failure\" by design doesn't test what\n>    does, but just asserts that the test fails in some way.\n\nExactly.  \n\nIt matters in complex tests that !(A&B&C) is different from\n(A&B&!C), the latter of which is what we want to do with tests that\ndocument what currently does not work.\n\n>   - test_expect_failure [<prereq>] <message> <script>\n>  \n>     This is NOT the opposite of test_expect_success, but is used\n> -   to mark a test that demonstrates a known breakage.  Unlike\n> +   to mark a test that demonstrates a known breakage whose exact failure\n> +   behavior isn't predictable.\n> +\n> +   If the exact breakage is known the \"test_expect_todo\" function\n> +   should be used instead. Usethis function if it's hard to pin down\n> +   the exact nature of the failure. Unlike\n\n\"Usethis\" -> \"Use this\"\n\n> -test_expect_failure 'clone --local detects misnamed objects' '\n> -\ttest_must_fail git clone --local misnamed misnamed-checkout\n> +test_expect_todo 'clone --local detects misnamed objects' '\n> +\tgit clone --local misnamed misnamed-checkout\n>  '\n\nJust like too many negatives confuse readers, I have to say this is\nquite confusing.  Do we or do we not want \"git clone\" invocation to\nsucceed?\n\n> +test_expect_failure () {\n> +\t_test_expect_todo test_expect_failure \"$@\"\n> +}\n> +\n> +test_expect_todo () {\n> +\t_test_expect_todo test_expect_todo \"$@\"\n> +}\n\nIt is a bit surprising that _test_expect_todo does not share more of\nits implementation with test_expect_success, but let's pretend we\ndidn't see it.\n\nLooks like a good first step.  I'd stop reading the series for now\nat this one.\n"},{"id":"451644","messageId":"xmqq8rt77zp7.fsf@gitster.g","threadId":"57580","inReplyTo":"xmqqczij9jeg.fsf@gitster.g","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T20:50:12Z","receivedAt":"2022-03-18T20:50:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Marking the step that demonstrates the current shortcomings with\n> \"MUST FAIL\" is a bad taste, but let's pretend that we didn't notice\n> it ;-).  Other than that, it looks like an improvement.\n> ...\n>> +test_expect_failure () {\n>> +\t_test_expect_todo test_expect_failure \"$@\"\n>> +}\n>> +\n>> +test_expect_todo () {\n>> +\t_test_expect_todo test_expect_todo \"$@\"\n>> +}\n>\n> It is a bit surprising that _test_expect_todo does not share more of\n> its implementation with test_expect_success, but let's pretend we\n> didn't see it.\n\nWith a bit more tweak, I think this can be made palatable:\n\n * introduce something that is *NOT* test_must_fail but behaves like\n   so, but with a bit of magic (see below).\n\n * do not introduce test_expect_todo, but use an improved version of\n   test_expect_success.\n\nLet's illustrate what I mean by starting from an example that we\nwant to have _after_ fixing an known breakage.  Let's say we expect\nthat our test preparation succeeds, 'git foo' fails when given a bad\noption, 'git foo' runs successfully, and the command is expected to\nemit certain output.  We may assert the ideal future world like so:\n\n\ttest_expect_success 'make sure foo works the way we want' '\n\t\tpreparatory step &&\n\t\ttest_must_fail git foo --bad-option >error &&\n\t\tgrep \"expected error message\" error &&\n\t\t! grep \"unwanted error message\" error &&\n\t\tgit foo >output &&\n\t\tgrep expected output &&\n\t\t! grep unwanted output\n\t'\n\nLet's also imagine that right now, option parsing in \"git foo\",\nworks but the main execution of the command does not work.\n\nWith test_expect_todo, you have to write something like this\nto document the current breakage:\n\n\ttest_expect_todo 'document breakage' '\n\t\tpreparatory step &&\n\t\ttest_must_fail git foo --bad-option >error &&\n\t\tgrep \"expected error message\" error &&\n\t\t! grep \"unwanted error message\" error &&\n\t\ttest_must_fail git foo >output &&\n\t\t! grep expected output &&\n\t\tgrep unwanted output\n\t'\n\nYou can see that this makes one thing unclear.\n\nAmong the two test_must_fail and two !, which one(s) document the\nbreakage?  In other words, which one of these four negations do we\nwish to lift eventually?  The answer is the latter two (we said that\nhandling of \"--bad-option\" is already working), but it is not obvious\nin the above test_expect_todo test sequence.\n\nI'd suggest we allow our test to be written this way:\n\n\ttest_expect_success 'make sure foo works the way we want' '\n\t\tpreparatory step &&\n\t\ttest_must_fail git foo --bad-option >error &&\n\t\tgrep \"expected error message\" error &&\n\t\t! grep \"unwanted error message\" error &&\n\ttest_ki git foo >output &&\n\ttest_ki grep expected output &&\n\ttest_ki ! grep unwanted output\n\t'\n\nand teach test_expect_success that an invocation of test_ki (\"known\nissue\"---a better name that is NOT test_must_fail very much welcome)\nmeans we hope this test someday passes without test_ki but not\ntoday, i.e. what your test_expect_todo means, and we unfortunately\nhave to expect that the lines annotated with test_ki would \"fail\".\n\nTo readers, test_ki should appear as if an annotation to a single\ncommand that highlights what we want to eventually be able to fix,\nand when the issue around the line marked as test_ki is fixed, we\ncan signal the fact by removing it from the beginning of these lines\n(that is why the last one is \"test_ki !\" and not \"!  test_ki\").\n\nTo the test framework and the shell that is running the test,\ntest_ki would be almost identical to test_must_fail, i.e. runs the\nrest of the command, catch segv and report, but otherwise the\nfailure from that \"rest of the command\" execution is turned into\n\"success\" that lets &&-chain to continue.  In addition, it tells\nthe test_expect_success running it that a success of the test piece\nshould be shown as TODO (expected failure).\n\nHmm?\n\n\n"},{"id":"451647","messageId":"220319.86v8waetae.gmgdl@evledraar.gmail.com","threadId":"57580","inReplyTo":"xmqq8rt77zp7.fsf@gitster.g","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-18T23:07:12Z","receivedAt":"2022-03-18T23:26:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 18 2022, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Marking the step that demonstrates the current shortcomings with\n>> \"MUST FAIL\" is a bad taste, but let's pretend that we didn't notice\n>> it ;-).  Other than that, it looks like an improvement.\n>> ...\n>>> +test_expect_failure () {\n>>> +\t_test_expect_todo test_expect_failure \"$@\"\n>>> +}\n>>> +\n>>> +test_expect_todo () {\n>>> +\t_test_expect_todo test_expect_todo \"$@\"\n>>> +}\n>>\n>> It is a bit surprising that _test_expect_todo does not share more of\n>> its implementation with test_expect_success, but let's pretend we\n>> didn't see it.\n>\n> With a bit more tweak, I think this can be made palatable:\n>\n>  * introduce something that is *NOT* test_must_fail but behaves like\n>    so, but with a bit of magic (see below).\n>\n>  * do not introduce test_expect_todo, but use an improved version of\n>    test_expect_success.\n>\n> Let's illustrate what I mean by starting from an example that we\n> want to have _after_ fixing an known breakage.  Let's say we expect\n> that our test preparation succeeds, 'git foo' fails when given a bad\n> option, 'git foo' runs successfully, and the command is expected to\n> emit certain output.  We may assert the ideal future world like so:\n>\n> \ttest_expect_success 'make sure foo works the way we want' '\n> \t\tpreparatory step &&\n> \t\ttest_must_fail git foo --bad-option >error &&\n> \t\tgrep \"expected error message\" error &&\n> \t\t! grep \"unwanted error message\" error &&\n> \t\tgit foo >output &&\n> \t\tgrep expected output &&\n> \t\t! grep unwanted output\n> \t'\n>\n> Let's also imagine that right now, option parsing in \"git foo\",\n> works but the main execution of the command does not work.\n>\n> With test_expect_todo, you have to write something like this\n> to document the current breakage:\n>\n> \ttest_expect_todo 'document breakage' '\n> \t\tpreparatory step &&\n> \t\ttest_must_fail git foo --bad-option >error &&\n> \t\tgrep \"expected error message\" error &&\n> \t\t! grep \"unwanted error message\" error &&\n> \t\ttest_must_fail git foo >output &&\n> \t\t! grep expected output &&\n> \t\tgrep unwanted output\n> \t'\n>\n> You can see that this makes one thing unclear.\n>\n> Among the two test_must_fail and two !, which one(s) document the\n> breakage?  In other words, which one of these four negations do we\n> wish to lift eventually?  The answer is the latter two (we said that\n> handling of \"--bad-option\" is already working), but it is not obvious\n> in the above test_expect_todo test sequence.\n>\n> I'd suggest we allow our test to be written this way:\n>\n> \ttest_expect_success 'make sure foo works the way we want' '\n> \t\tpreparatory step &&\n> \t\ttest_must_fail git foo --bad-option >error &&\n> \t\tgrep \"expected error message\" error &&\n> \t\t! grep \"unwanted error message\" error &&\n> \ttest_ki git foo >output &&\n> \ttest_ki grep expected output &&\n> \ttest_ki ! grep unwanted output\n> \t'\n>\n> and teach test_expect_success that an invocation of test_ki (\"known\n> issue\"---a better name that is NOT test_must_fail very much welcome)\n> means we hope this test someday passes without test_ki but not\n> today, i.e. what your test_expect_todo means, and we unfortunately\n> have to expect that the lines annotated with test_ki would \"fail\".\n>\n> To readers, test_ki should appear as if an annotation to a single\n> command that highlights what we want to eventually be able to fix,\n> and when the issue around the line marked as test_ki is fixed, we\n> can signal the fact by removing it from the beginning of these lines\n> (that is why the last one is \"test_ki !\" and not \"!  test_ki\").\n>\n> To the test framework and the shell that is running the test,\n> test_ki would be almost identical to test_must_fail, i.e. runs the\n> rest of the command, catch segv and report, but otherwise the\n> failure from that \"rest of the command\" execution is turned into\n> \"success\" that lets &&-chain to continue.  In addition, it tells\n> the test_expect_success running it that a success of the test piece\n> should be shown as TODO (expected failure).\n>\n> Hmm?\n\nHave you had the time to look past patch 1/7 of this series? 2/7\nintroduces a \"test_todo\" helper, the \"test_expect_todo\" is just the\nbasic top-level primitive.\n\nI don't think we can categorically replace all of the\n\"test_expect_failure\" cases, because in some of those it's too much of a\nhassle to assert the exact current behavior, or we don't really care.\n\nBut I think for most cases instead of a:\n\n\ttest_ki ! grep unwanted output\n\nIt makes more sense to do (as that helper does):\n\n\ttest_todo grep --want yay --expect unwanted -- output\n\nWhich is quite a handful, which is why the series goes on to\ne.g. introduce a todo_test_cmp, used e.g. like this (and we can easily\nadd more wrappers for common cases):\n\n\tcat >want <<-EOF &&\n\t$(git rev-parse HEAD)\n\tEOF\n\tcat >expect <<-\\EOF &&\n\terror: can't rev-parse stuff\n\tEOF\n\ttest_might_fail git some-cmd HEAD >actual &&\n\ttodo_test_cmp want expect actual\n\nI.e. if you just want to throw your hands in the air and say \"I don't\ncare how this fails and just emit a 'not ok .. TODO' line\" we already\nhave test_expect_failure for that use-case.\n\nI think in most other cases documenting that something is broken or\nshould behave differently shouldn't be synonymous with not caring *how*\nthat unwanted behavior works right now.\n\nUnderstanding of your past commentary on this topic is that you strongly\nobjected to not having the test suite output reflect that a given test\nwas \"not ok ... TODO\" in some way. I.e. even though I think\n\"test_expect_success\" has the approximate *semantics* we want we\nshouldn't use those.\n\nBut I think the combination of \"test_expect_todo\" and the \"test_todo\"\nprimitive should satisfy that, and will give us accurate assertions\nabout what we're actually doing now.\n\n"},{"id":"451665","messageId":"xmqqilsa76ve.fsf@gitster.g","threadId":"57580","inReplyTo":"220319.86v8waetae.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-19T07:12:53Z","receivedAt":"2022-03-19T07:13:01Z","isPatch":true,"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>> emit certain output.  We may assert the ideal future world like so:\n>>\n>> \ttest_expect_success 'make sure foo works the way we want' '\n>> \t\tpreparatory step &&\n>> \t\ttest_must_fail git foo --bad-option >error &&\n>> \t\tgrep \"expected error message\" error &&\n>> \t\t! grep \"unwanted error message\" error &&\n>> \t\tgit foo >output &&\n>> \t\tgrep expected output &&\n>> \t\t! grep unwanted output\n>> \t'\n>>\n>> Let's also imagine that right now, option parsing in \"git foo\",\n>> works but the main execution of the command does not work.\n>>\n>> With test_expect_todo, you have to write something like this\n>> to document the current breakage:\n>>\n>> \ttest_expect_todo 'document breakage' '\n>> \t\tpreparatory step &&\n>> \t\ttest_must_fail git foo --bad-option >error &&\n>> \t\tgrep \"expected error message\" error &&\n>> \t\t! grep \"unwanted error message\" error &&\n>> \t\ttest_must_fail git foo >output &&\n>> \t\t! grep expected output &&\n>> \t\tgrep unwanted output\n>> \t'\n>>\n>> You can see that this makes one thing unclear.\n>>\n>> Among the two test_must_fail and two !, which one(s) document the\n>> breakage?  In other words, which one of these four negations do we\n>> wish to lift eventually?  The answer is the latter two (we said that\n>> handling of \"--bad-option\" is already working), but it is not obvious\n>> in the above test_expect_todo test sequence.\n>>\n>> I'd suggest we allow our test to be written this way:\n>>\n>> \ttest_expect_success 'make sure foo works the way we want' '\n>> \t\tpreparatory step &&\n>> \t\ttest_must_fail git foo --bad-option >error &&\n>> \t\tgrep \"expected error message\" error &&\n>> \t\t! grep \"unwanted error message\" error &&\n>> \ttest_ki git foo >output &&\n>> \ttest_ki grep expected output &&\n>> \ttest_ki ! grep unwanted output\n>> \t'\n>>\n>> and teach test_expect_success that an invocation of test_ki (\"known\n>> issue\"---a better name that is NOT test_must_fail very much welcome)\n>> means we hope this test someday passes without test_ki but not\n>> today, i.e. what your test_expect_todo means, and we unfortunately\n>> have to expect that the lines annotated with test_ki would \"fail\".\n\n> Have you had the time to look past patch 1/7 of this series? 2/7\n> introduces a \"test_todo\" helper, the \"test_expect_todo\" is just the\n> basic top-level primitive.\n\nNo, and I do not have to.  I care about the most basic form first,\nand if you cannot get it right, it is not interesting.  You can\nconsider the test_ki above as the primitive form of your \"test_todo\"\nthat says \"I want the command to give true, but I know it currently\ngives false\".\n\nAnd quite honestly, I am not interested in _how_ it currently\nhappens to break.  We may want the command being tested to\neventually count three commits, but due to a bug, it may only count\none.  You may say \"test_todo count --want 3 --expect 1 blah\", but\nthe \"expect\" part is much less interesting than the fact that the\ncommand being tested on _that_ line (not the whole sequence run with\ntest_expect_failure) is clearly documented to want 3 but currently\nis broken, and it can be clearly distinguished from the normal\ntest_must_fail or ! that documents that we do want a failure out of\nthe command being tested there.\n\nSo with or without the \"higher level\" wrappers, how else, other than\nthe way I showed in the message you are responding to as a rewrite\nof the example to use test_expect_todo, that uses two test_must_fail\nand two ! and makes which ones are expected failure and which ones\nare documentation of the current breakage, do you propose to write\nthe equivalent?  It may be that your test_todo may be a different\nway to spell the test_ki marker I showed above, and if that is the\ncase it is perfectly fine, but I want it to be THE primitive, not\ntest_must_fail or !, to mark a single command in the test that\ncurrently does not work as expected.\n\n> I don't think we can categorically replace all of the\n> \"test_expect_failure\" cases, because in some of those it's too much of a\n> hassle to assert the exact current behavior, or we don't really care.\n>\n> But I think for most cases instead of a:\n>\n> \ttest_ki ! grep unwanted output\n>\n> It makes more sense to do (as that helper does):\n>\n> \ttest_todo grep --want yay --expect unwanted -- output\n\nMy take is the complete opposite.  We can and should start small,\nand how exactly the current implementation happens to be broken does\nnot matter most of the time.\n\n"},{"id":"451671","messageId":"220319.86ilsadw69.gmgdl@evledraar.gmail.com","threadId":"57580","inReplyTo":"xmqqilsa76ve.fsf@gitster.g","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-19T11:11:10Z","receivedAt":"2022-03-19T11:22:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Mar 19 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> emit certain output.  We may assert the ideal future world like so:\n>>>\n>>> \ttest_expect_success 'make sure foo works the way we want' '\n>>> \t\tpreparatory step &&\n>>> \t\ttest_must_fail git foo --bad-option >error &&\n>>> \t\tgrep \"expected error message\" error &&\n>>> \t\t! grep \"unwanted error message\" error &&\n>>> \t\tgit foo >output &&\n>>> \t\tgrep expected output &&\n>>> \t\t! grep unwanted output\n>>> \t'\n>>>\n>>> Let's also imagine that right now, option parsing in \"git foo\",\n>>> works but the main execution of the command does not work.\n>>>\n>>> With test_expect_todo, you have to write something like this\n>>> to document the current breakage:\n>>>\n>>> \ttest_expect_todo 'document breakage' '\n>>> \t\tpreparatory step &&\n>>> \t\ttest_must_fail git foo --bad-option >error &&\n>>> \t\tgrep \"expected error message\" error &&\n>>> \t\t! grep \"unwanted error message\" error &&\n>>> \t\ttest_must_fail git foo >output &&\n>>> \t\t! grep expected output &&\n>>> \t\tgrep unwanted output\n>>> \t'\n>>>\n>>> You can see that this makes one thing unclear.\n>>>\n>>> Among the two test_must_fail and two !, which one(s) document the\n>>> breakage?  In other words, which one of these four negations do we\n>>> wish to lift eventually?  The answer is the latter two (we said that\n>>> handling of \"--bad-option\" is already working), but it is not obvious\n>>> in the above test_expect_todo test sequence.\n>>>\n>>> I'd suggest we allow our test to be written this way:\n>>>\n>>> \ttest_expect_success 'make sure foo works the way we want' '\n>>> \t\tpreparatory step &&\n>>> \t\ttest_must_fail git foo --bad-option >error &&\n>>> \t\tgrep \"expected error message\" error &&\n>>> \t\t! grep \"unwanted error message\" error &&\n>>> \ttest_ki git foo >output &&\n>>> \ttest_ki grep expected output &&\n>>> \ttest_ki ! grep unwanted output\n>>> \t'\n>>>\n>>> and teach test_expect_success that an invocation of test_ki (\"known\n>>> issue\"---a better name that is NOT test_must_fail very much welcome)\n>>> means we hope this test someday passes without test_ki but not\n>>> today, i.e. what your test_expect_todo means, and we unfortunately\n>>> have to expect that the lines annotated with test_ki would \"fail\".\n>\n>> Have you had the time to look past patch 1/7 of this series? 2/7\n>> introduces a \"test_todo\" helper, the \"test_expect_todo\" is just the\n>> basic top-level primitive.\n>\n> No, and I do not have to.  I care about the most basic form first,\n> and if you cannot get it right, it is not interesting.  You can\n> consider the test_ki above as the primitive form of your \"test_todo\"\n> that says \"I want the command to give true, but I know it currently\n> gives false\".\n\nSure, and I do have that implemented. If you're just asking that my\n\"test_todo\" or another helper do that by default, then that's easy.\n\nI.e. I've got that, but not as one short \"test_*\" verb.\n\n> And quite honestly, I am not interested in _how_ it currently\n> happens to break.  We may want the command being tested to\n> eventually count three commits, but due to a bug, it may only count\n> one.  You may say \"test_todo count --want 3 --expect 1 blah\", but\n> the \"expect\" part is much less interesting than the fact that the\n> command being tested on _that_ line (not the whole sequence run with\n> test_expect_failure) is clearly documented to want 3 but currently\n> is broken, and it can be clearly distinguished from the normal\n> test_must_fail or ! that documents that we do want a failure out of\n> the command being tested there.\n\nYes, if you don't want to test the exact behavior you have/want that's\nalso easy.\n\n> So with or without the \"higher level\" wrappers, how else, other than\n> the way I showed in the message you are responding to as a rewrite\n> of the example to use test_expect_todo, that uses two test_must_fail\n> and two ! and makes which ones are expected failure and which ones\n> are documentation of the current breakage, do you propose to write\n> the equivalent?  It may be that your test_todo may be a different\n> way to spell the test_ki marker I showed above, and if that is the\n> case it is perfectly fine, but I want it to be THE primitive, not\n> test_must_fail or !, to mark a single command in the test that\n> currently does not work as expected.\n\nSure, yes it's basically a different way to spell the same thing....\n\n>> I don't think we can categorically replace all of the\n>> \"test_expect_failure\" cases, because in some of those it's too much of a\n>> hassle to assert the exact current behavior, or we don't really care.\n>>\n>> But I think for most cases instead of a:\n>>\n>> \ttest_ki ! grep unwanted output\n>>\n>> It makes more sense to do (as that helper does):\n>>\n>> \ttest_todo grep --want yay --expect unwanted -- output\n>\n> My take is the complete opposite.  We can and should start small,\n> and how exactly the current implementation happens to be broken does\n> not matter most of the time.\n\nWell, the tip of this series leaves ~20 uses of test_expect_todo v.s. a\nremaining ~100 uses of test_expect_failure, so it is a small start. I\nconverted those things I thought made the most sense.\n\nBut I do think you want to test at least a fuzzy \"how exactly\" most of\nthe time. The reason I worked on this was because while authoring the\nseries you merged in ea05fd5fbf7 (Merge branch\n'ab/keep-git-exit-codes-in-tests', 2022-03-16) I found that we had\nvarious test_expect_failure that failed in ways very different than what\nwe'd expect.\n\nOr, saying that something exits non-zero and we'd like to fix it isn't\nthe same as saying that we'd like to e.g. exclude it from SANITIZE=leak\nor SANITIZE=address testing. I.e. it still shouldn't leak, double-free()\nor run into a BUG(), and if it does we'd like to know most of the time.\n\nI think the only sensible thing to do to fix that is to have the\nsemantics of test_expect_todo, within that you can always decide to\nignore individual exit codes, but you can't really do it the other way\naround (which is what test_expect_failure does).\n\n\n"},{"id":"451684","messageId":"db558292-2783-3270-4824-43757822a389@gmail.com","threadId":"57580","inReplyTo":"220319.86ilsadw69.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-20T15:13:36Z","receivedAt":"2022-03-20T15:13:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 19/03/2022 11:11, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sat, Mar 19 2022, Junio C Hamano wrote:\n>[...] \n>>> I don't think we can categorically replace all of the\n>>> \"test_expect_failure\" cases, because in some of those it's too much of a\n>>> hassle to assert the exact current behavior, or we don't really care.\n>>>\n>>> But I think for most cases instead of a:\n>>>\n>>> \ttest_ki ! grep unwanted output\n>>>\n>>> It makes more sense to do (as that helper does):\n>>>\n>>> \ttest_todo grep --want yay --expect unwanted -- output\n>>\n>> My take is the complete opposite.  We can and should start small,\n>> and how exactly the current implementation happens to be broken does\n>> not matter most of the time.\n> \n> Well, the tip of this series leaves ~20 uses of test_expect_todo v.s. a\n> remaining ~100 uses of test_expect_failure, so it is a small start. I\n> converted those things I thought made the most sense.\n> \n> But I do think you want to test at least a fuzzy \"how exactly\" most of\n> the time. The reason I worked on this was because while authoring the\n> series you merged in ea05fd5fbf7 (Merge branch\n> 'ab/keep-git-exit-codes-in-tests', 2022-03-16) I found that we had\n> various test_expect_failure that failed in ways very different than what\n> we'd expect.\n> \n> Or, saying that something exits non-zero and we'd like to fix it isn't\n> the same as saying that we'd like to e.g. exclude it from SANITIZE=leak\n> or SANITIZE=address testing. I.e. it still shouldn't leak, double-free()\n> or run into a BUG(), and if it does we'd like to know most of the time.\n\nI think having test_todo based on test_must_fail as Junio\nsuggested avoids this as it means the test will still fail on SIGSEV\nor SIGABRT (if we don't already do so we can make LSAN exit with the\nsame code as vargrind which test_must_fail checks for). I had a look\nat some of the conversions with your test_todo --want/--expect/--reset\nand found the result really hard to follow. Junio's suggestions chimed\nwith some things I've been thinking about so I had a go at\nimplementing it and doing some sample conversions (see below). Marking\nthe individual commands that should fail is a big step forward and the\nfailing commands are checked to make sure they don't segfault etc.\n\nBest Wishes\n\nPhillip\n\n---- >8 -----\n  t/t0000-basic.sh                |   9 +++++--\n  t/t3401-rebase-and-am-rename.sh |  12 +++++-----\n  t/t3424-rebase-empty.sh         |   6 ++---\n  t/t4014-format-patch.sh         |  20 ++++++++--------\n  t/test-lib-functions.sh         | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------\n  5 files changed, 116 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b007f0efef..53217d4cd5 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -103,16 +103,21 @@ test_expect_success 'subtest: 2/3 tests passing' '\n  \n  test_expect_success 'subtest: a failing TODO test' '\n  \twrite_and_run_sub_test_lib_test failing-todo <<-\\EOF &&\n+\ttest_false () {\n+\t\tfalse\n+\t}\n  \ttest_expect_success \"passing test\" \"true\"\n  \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\n+\ttest_expect_success \"known todo\" \"test_todo test_false\"\n  \ttest_done\n  \tEOF\n  \tcheck_sub_test_lib_test failing-todo <<-\\EOF\n  \t> ok 1 - passing test\n  \t> not ok 2 - pretend we have a known breakage # TODO known breakage\n-\t> # still have 1 known breakage(s)\n+\t> not ok 3 - known todo # TODO known breakage\n+\t> # still have 2 known breakage(s)\n  \t> # passed all remaining 1 test(s)\n-\t> 1..2\n+\t> 1..3\n  \tEOF\n  '\n  \ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex f18bae9450..cc5da9f5af 100755\n--- a/t/t3401-rebase-and-am-rename.sh\n+++ b/t/t3401-rebase-and-am-rename.sh\n@@ -52,7 +52,7 @@ test_expect_success 'rebase --interactive: directory rename detected' '\n  \t)\n  '\n  \n-test_expect_failure 'rebase --apply: directory rename detected' '\n+test_expect_success 'rebase --apply: directory rename detected' '\n  \t(\n  \t\tcd dir-rename &&\n  \n@@ -63,8 +63,8 @@ test_expect_failure 'rebase --apply: directory rename detected' '\n  \t\tgit ls-files -s >out &&\n  \t\ttest_line_count = 5 out &&\n  \n-\t\ttest_path_is_file y/d &&\n-\t\ttest_path_is_missing x/d\n+\t\ttest_todo test_path_is_file y/d &&\n+\t\ttest_todo test_path_is_missing x/d\n  \t)\n  '\n  \n@@ -84,7 +84,7 @@ test_expect_success 'rebase --merge: directory rename detected' '\n  \t)\n  '\n  \n-test_expect_failure 'am: directory rename detected' '\n+test_expect_success 'am: directory rename detected' '\n  \t(\n  \t\tcd dir-rename &&\n  \n@@ -97,8 +97,8 @@ test_expect_failure 'am: directory rename detected' '\n  \t\tgit ls-files -s >out &&\n  \t\ttest_line_count = 5 out &&\n  \n-\t\ttest_path_is_file y/d &&\n-\t\ttest_path_is_missing x/d\n+\t\ttest_todo test_path_is_file y/d &&\n+\t\ttest_todo test_path_is_missing x/d\n  \t)\n  '\n  \ndiff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh\nindex 5e1045a0af..b7cae26075 100755\n--- a/t/t3424-rebase-empty.sh\n+++ b/t/t3424-rebase-empty.sh\n@@ -34,15 +34,15 @@ test_expect_success 'setup test repository' '\n  \tgit commit -m \"Five letters ought to be enough for anybody\"\n  '\n  \n-test_expect_failure 'rebase (apply-backend)' '\n-\ttest_when_finished \"git rebase --abort\" &&\n+test_expect_success 'rebase (apply-backend)' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n  \tgit checkout -B testing localmods &&\n  \t# rebase (--apply) should not drop commits that start empty\n  \tgit rebase --apply upstream &&\n  \n  \ttest_write_lines D C B A >expect &&\n  \tgit log --format=%s >actual &&\n-\ttest_cmp expect actual\n+\ttest_todo test_cmp expect actual\n  '\n  \n  test_expect_success 'rebase --merge --empty=drop' '\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7dc5a5c736..bb0fcef40e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -165,12 +165,12 @@ test_expect_success 'additional command line cc (ascii)' '\n  \tgrep \"^ *S E Cipient <scipient@example.com>\\$\" hdrs5\n  '\n  \n-test_expect_failure 'additional command line cc (rfc822)' '\n+test_expect_success 'additional command line cc (rfc822)' '\n  \tgit config --replace-all format.headers \"Cc: R E Cipient <rcipient@example.com>\" &&\n  \tgit format-patch --cc=\"S. E. Cipient <scipient@example.com>\" --stdout main..side >patch5 &&\n  \tsed -e \"/^\\$/q\" patch5 >hdrs5 &&\n  \tgrep \"^Cc: R E Cipient <rcipient@example.com>,\\$\" hdrs5 &&\n-\tgrep \"^ *\\\"S. E. Cipient\\\" <scipient@example.com>\\$\" hdrs5\n+\ttest_todo grep \"^ *\\\"S. E. Cipient\\\" <scipient@example.com>\\$\" hdrs5\n  '\n  \n  test_expect_success 'command line headers' '\n@@ -195,16 +195,16 @@ test_expect_success 'command line To: header (ascii)' '\n  \tgrep \"^To: R E Cipient <rcipient@example.com>\\$\" hdrs8\n  '\n  \n-test_expect_failure 'command line To: header (rfc822)' '\n+test_expect_success 'command line To: header (rfc822)' '\n  \tgit format-patch --to=\"R. E. Cipient <rcipient@example.com>\" --stdout main..side >patch8 &&\n  \tsed -e \"/^\\$/q\" patch8 >hdrs8 &&\n-\tgrep \"^To: \\\"R. E. Cipient\\\" <rcipient@example.com>\\$\" hdrs8\n+\ttest_todo grep \"^To: \\\"R. E. Cipient\\\" <rcipient@example.com>\\$\" hdrs8\n  '\n  \n-test_expect_failure 'command line To: header (rfc2047)' '\n+test_expect_success 'command line To: header (rfc2047)' '\n  \tgit format-patch --to=\"R Ä Cipient <rcipient@example.com>\" --stdout main..side >patch8 &&\n  \tsed -e \"/^\\$/q\" patch8 >hdrs8 &&\n-\tgrep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\\$\" hdrs8\n+\ttest_todo grep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\\$\" hdrs8\n  '\n  \n  test_expect_success 'configuration To: header (ascii)' '\n@@ -214,18 +214,18 @@ test_expect_success 'configuration To: header (ascii)' '\n  \tgrep \"^To: R E Cipient <rcipient@example.com>\\$\" hdrs9\n  '\n  \n-test_expect_failure 'configuration To: header (rfc822)' '\n+test_expect_success 'configuration To: header (rfc822)' '\n  \tgit config format.to \"R. E. Cipient <rcipient@example.com>\" &&\n  \tgit format-patch --stdout main..side >patch9 &&\n  \tsed -e \"/^\\$/q\" patch9 >hdrs9 &&\n-\tgrep \"^To: \\\"R. E. Cipient\\\" <rcipient@example.com>\\$\" hdrs9\n+\ttest_todo grep \"^To: \\\"R. E. Cipient\\\" <rcipient@example.com>\\$\" hdrs9\n  '\n  \n-test_expect_failure 'configuration To: header (rfc2047)' '\n+test_expect_success 'configuration To: header (rfc2047)' '\n  \tgit config format.to \"R Ä Cipient <rcipient@example.com>\" &&\n  \tgit format-patch --stdout main..side >patch9 &&\n  \tsed -e \"/^\\$/q\" patch9 >hdrs9 &&\n-\tgrep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\\$\" hdrs9\n+\ttest_todo grep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\\$\" hdrs9\n  '\n  \n  # check_patch <patch>: Verify that <patch> looks like a half-sane\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0f439c99d6..deb74a22f3 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -739,6 +739,7 @@ test_expect_failure () {\n  }\n  \n  test_expect_success () {\n+\ttest_todo_=\n  \ttest_start_\n  \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n  \ttest \"$#\" = 2 ||\n@@ -750,7 +751,12 @@ test_expect_success () {\n  \t\tsay >&3 \"expecting success of $TEST_NUMBER.$test_count '$1': $2\"\n  \t\tif test_run_ \"$2\"\n  \t\tthen\n-\t\t\ttest_ok_ \"$1\"\n+\t\t\tif test -n \"$test_todo_\"\n+\t\t\tthen\n+\t\t\t\ttest_known_broken_failure_ \"$1\"\n+\t\t\telse\n+\t\t\t\ttest_ok_ \"$1\"\n+\t\t\tfi\n  \t\telse\n  \t\t\ttest_failure_ \"$@\"\n  \t\tfi\n@@ -1011,8 +1017,20 @@ list_contains () {\n  # Returns success if the arguments indicate that a command should be\n  # accepted by test_must_fail(). If the command is run with env, the env\n  # and its corresponding variable settings will be stripped before we\n+# we test the command being run.\n+#\n+# For test_todo we allow a wider range of commands to and if the\n+# command is run with verbose then verbose will be stripped before we\n  # test the command being run.\n+\n  test_must_fail_acceptable () {\n+\tlocal name\n+\tname=\"$1\"\n+\tshift\n+\tif test \"$name\" = \"todo\" && test \"$1\" = \"verbose\"\n+\tthen\n+\t\tshift\n+\tfi\n  \tif test \"$1\" = \"env\"\n  \tthen\n  \t\tshift\n@@ -1033,44 +1051,22 @@ test_must_fail_acceptable () {\n  \tgit|__git*|test-tool|test_terminal)\n  \t\treturn 0\n  \t\t;;\n+\tgrep|test|test_*)\n+\t\tif test \"$name\" = \"todo\"\n+\t\tthen\n+\t\t    return 0\n+\t\tfi\n+\t\treturn 1\n+\t\t;;\n  \t*)\n  \t\treturn 1\n  \t\t;;\n  \tesac\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-#\ttest_expect_success 'complain and die' '\n-#           do something &&\n-#           do something else &&\n-#\t    test_must_fail git checkout ../outerspace\n-#\t'\n-#\n-# Writing this as \"! git checkout ../outerspace\" is wrong, because\n-# the failure could be due to a segv.  We want a controlled failure.\n-#\n-# Accepts the following options:\n-#\n-#   ok=<signal-name>[,<...>]:\n-#     Don't treat an exit caused by the given signal as error.\n-#     Multiple signals can be specified as a comma separated list.\n-#     Currently recognized signal names are: sigpipe, success.\n-#     (Don't use 'success', use 'test_might_fail' instead.)\n-#\n-# Do not use this to run anything but \"git\" and other specific testable\n-# commands (see test_must_fail_acceptable()).  We are not in the\n-# business of vetting system supplied commands -- in other words, this\n-# is wrong:\n-#\n-#    test_must_fail grep pattern output\n-#\n-# Instead use '!':\n-#\n-#    ! grep pattern output\n-\n-test_must_fail () {\n+test_must_fail_helper () {\n+\ttest_must_fail_name_=\"$1\"\n+\tshift\n  \tcase \"$1\" in\n  \tok=*)\n  \t\t_test_ok=${1#ok=}\n@@ -1080,36 +1076,90 @@ test_must_fail () {\n  \t\t_test_ok=\n  \t\t;;\n  \tesac\n-\tif ! test_must_fail_acceptable \"$@\"\n+\tif ! test_must_fail_acceptable $test_must_fail_name_ \"$@\"\n  \tthen\n-\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n+\t\techo >&7 \"test_$test_must_fail_name_: only 'git' is allowed: $*\"\n  \t\treturn 1\n  \tfi\n  \t\"$@\" 2>&7\n  \texit_code=$?\n  \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n  \tthen\n-\t\techo >&4 \"test_must_fail: command succeeded: $*\"\n+\t\techo >&4 \"test_$test_must_fail_name_: command succeeded: $*\"\n  \t\treturn 1\n  \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n  \tthen\n  \t\treturn 0\n  \telif test $exit_code -gt 129 && test $exit_code -le 192\n  \tthen\n-\t\techo >&4 \"test_must_fail: died by signal $(($exit_code - 128)): $*\"\n+\t\techo >&4 \"test_$test_must_fail_name_: died by signal $(($exit_code - 128)): $*\"\n  \t\treturn 1\n  \telif test $exit_code -eq 127\n  \tthen\n-\t\techo >&4 \"test_must_fail: command not found: $*\"\n+\t\techo >&4 \"test_$test_must_fail_name_: command not found: $*\"\n  \t\treturn 1\n  \telif test $exit_code -eq 126\n  \tthen\n-\t\techo >&4 \"test_must_fail: valgrind error: $*\"\n+\t\techo >&4 \"test_$test_must_fail_name_: valgrind error: $*\"\n  \t\treturn 1\n  \tfi\n  \treturn 0\n  } 7>&2 2>&4\n  \n+# This is used to mark commands that should succeed but do not due to\n+# a known issue. Marking the individual commands that fail rather than\n+# using test_expect_failure allows us to detect any unexpected\n+# failures. As with test_must_fail if the command is killed by a\n+# signal the test will fail. If the command unexpectedly succeeds then\n+# the test fails. For example:\n+#\n+#\ttest_expect_success 'test a known failure' '\n+#\t\tgit foo 2>err &&\n+#\t\ttest_todo test_must_be_empty err\n+#\t'\n+#\n+# This test will fail if \"git foo\" fails or err is unexpectedly empty\n+\n+test_todo () {\n+\ttest_todo_=todo\n+\ttest_must_fail_helper todo \"$@\" 2>&7\n+} 7>&2 2>&4\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+#\ttest_expect_success 'complain and die' '\n+#           do something &&\n+#           do something else &&\n+#\t    test_must_fail git checkout ../outerspace\n+#\t'\n+#\n+# Writing this as \"! git checkout ../outerspace\" is wrong, because\n+# the failure could be due to a segv.  We want a controlled failure.\n+#\n+# Accepts the following options:\n+#\n+#   ok=<signal-name>[,<...>]:\n+#     Don't treat an exit caused by the given signal as error.\n+#     Multiple signals can be specified as a comma separated list.\n+#     Currently recognized signal names are: sigpipe, success.\n+#     (Don't use 'success', use 'test_might_fail' instead.)\n+#\n+# Do not use this to run anything but \"git\" and other specific testable\n+# commands (see test_must_fail_acceptable()).  We are not in the\n+# business of vetting system supplied commands -- in other words, this\n+# is wrong:\n+#\n+#    test_must_fail grep pattern output\n+#\n+# Instead use '!':\n+#\n+#    ! grep pattern output\n+\n+test_must_fail () {\n+\ttest_must_fail_helper must_fail \"$@\" 2>&7\n+} 7>&2 2>&4\n+\n  # Similar to test_must_fail, but tolerates success, too.  This is\n  # meant to be used in contexts like:\n  #\n@@ -1124,7 +1174,7 @@ test_must_fail () {\n  # Accepts the same options as test_must_fail.\n  \n  test_might_fail () {\n-\ttest_must_fail ok=success \"$@\" 2>&7\n+\ttest_must_fail_helper might_fail ok=success \"$@\" 2>&7\n  } 7>&2 2>&4\n  \n  # Similar to test_must_fail and test_might_fail, but check that a\n"},{"id":"451686","messageId":"xmqqy2144hwa.fsf@gitster.g","threadId":"57580","inReplyTo":"db558292-2783-3270-4824-43757822a389@gmail.com","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-20T18:07:33Z","receivedAt":"2022-03-20T18:07:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> .... I had a look\n> at some of the conversions with your test_todo --want/--expect/--reset\n> and found the result really hard to follow. Junio's suggestions chimed\n> with some things I've been thinking about so I had a go at\n> implementing it and doing some sample conversions (see below). Marking\n> the individual commands that should fail is a big step forward and the\n> failing commands are checked to make sure they don't segfault etc.\n\n;-)\n\nAnother small detail in my suggestion that will make a huge\ndifference in the end is not to introduce test_expect_todo as a\nseparate top-level construct, and instead teach test_expect_success\nto pay attention to the use of test_todo \"unfortunately this does\nnot work yet\" mark in it.  It allows us to use test_todo in a shared\ntest helper function and call them in test_expect_success, and when\nthe step the test helper has trouble with gets fixed, the \"unmark\"\nstep will be an isolated change.\n\nYour sample change seems to already have it, which is good.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 0f439c99d6..deb74a22f3 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -739,6 +739,7 @@ test_expect_failure () {\n>  }\n>    test_expect_success () {\n> +\ttest_todo_=\n>  \ttest_start_\n>  \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n>  \ttest \"$#\" = 2 ||\n> @@ -750,7 +751,12 @@ test_expect_success () {\n>  \t\tsay >&3 \"expecting success of $TEST_NUMBER.$test_count '$1': $2\"\n>  \t\tif test_run_ \"$2\"\n>  \t\tthen\n> -\t\t\ttest_ok_ \"$1\"\n> +\t\t\tif test -n \"$test_todo_\"\n> +\t\t\tthen\n> +\t\t\t\ttest_known_broken_failure_ \"$1\"\n> +\t\t\telse\n> +\t\t\t\ttest_ok_ \"$1\"\n> +\t\t\tfi\n>  \t\telse\n>  \t\t\ttest_failure_ \"$@\"\n>  \t\tfi\n"},{"id":"451846","messageId":"a9478c1b-4441-0c79-332d-a7c5592dbc70@github.com","threadId":"57580","inReplyTo":"xmqqy2144hwa.fsf@gitster.g","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-22T14:43:23Z","receivedAt":"2022-03-22T14:44:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/20/22 2:07 PM, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> .... I had a look\n>> at some of the conversions with your test_todo --want/--expect/--reset\n>> and found the result really hard to follow. Junio's suggestions chimed\n>> with some things I've been thinking about so I had a go at\n>> implementing it and doing some sample conversions (see below). Marking\n>> the individual commands that should fail is a big step forward and the\n>> failing commands are checked to make sure they don't segfault etc.\n> \n> ;-)\n> \n> Another small detail in my suggestion that will make a huge\n> difference in the end is not to introduce test_expect_todo as a\n> separate top-level construct, and instead teach test_expect_success\n> to pay attention to the use of test_todo \"unfortunately this does\n> not work yet\" mark in it.  It allows us to use test_todo in a shared\n> test helper function and call them in test_expect_success, and when\n> the step the test helper has trouble with gets fixed, the \"unmark\"\n> step will be an isolated change.\n> \n> Your sample change seems to already have it, which is good.\n\nAfter reading this thread, I agree with many of the ideas that were\ngenerated in response to this topic.\n\nThe thing I'm hoping to see from a final version is that a top-level\nhelper like test_expect_todo will expect at least one test_todo\nhelper to be executed inside of the test (perhaps communicated by\nsetting a special GIT_TEST_* environment variable?) and if any of\nthe test_todo lines change from fail to pass, then that is\ncommunicated as a _failure_ from CI's perspective. Let us discover\nif we have accidentally \"fixed\" any of these test cases and update\nthe tests accordingly.\n\nI can predict writing a test case with multiple test_todo lines\nthat need to be updated to drop the test_todo helpers one-by-one\nas a change is being introduced.\n\nThanks,\n-Stolee\n"},{"id":"452076","messageId":"xmqqy210pbbh.fsf@gitster.g","threadId":"57580","inReplyTo":"a9478c1b-4441-0c79-332d-a7c5592dbc70@github.com","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T22:13:06Z","receivedAt":"2022-03-23T22:13:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> The thing I'm hoping to see from a final version is that a top-level\n> helper like test_expect_todo will expect at least one test_todo\n> helper to be executed inside of the test (perhaps communicated by\n> setting a special GIT_TEST_* environment variable?) and if any of\n\nI was hoping that we can do without test_expect_todo.\ntest_expect_success can turn itself into test_expect_todo when it\nsees test_todo is invoked even once in it.  And Phillip's outline\nactually implements the idea, if I am not mistaken.\n\n> the test_todo lines change from fail to pass, then that is\n> communicated as a _failure_ from CI's perspective. Let us discover\n> if we have accidentally \"fixed\" any of these test cases and update\n> the tests accordingly.\n\nIn other words, we do not want to lose the \"TODO fixed\" we have been\ngetting out of test_expect_failure, which I agree with.  I am not\nsure if Phillip's outline had that feature.\n\n> I can predict writing a test case with multiple test_todo lines\n> that need to be updated to drop the test_todo helpers one-by-one\n> as a change is being introduced.\n\nYes.\n"},{"id":"452103","messageId":"3ee3833a-af2b-90d9-d209-19cab8a8ae43@gmail.com","threadId":"57580","inReplyTo":"xmqqy210pbbh.fsf@gitster.g","subject":"Re: [PATCH 1/7] test-lib: add a \"test_expect_todo\", similar to \"test_expect_failure\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-24T11:24:07Z","receivedAt":"2022-03-24T11:24:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/03/2022 22:13, Junio C Hamano wrote:\n> Derrick Stolee <derrickstolee@github.com> writes:\n> \n>> The thing I'm hoping to see from a final version is that a top-level\n>> helper like test_expect_todo will expect at least one test_todo\n>> helper to be executed inside of the test (perhaps communicated by\n>> setting a special GIT_TEST_* environment variable?) and if any of\n> \n> I was hoping that we can do without test_expect_todo.\n> test_expect_success can turn itself into test_expect_todo when it\n> sees test_todo is invoked even once in it.  And Phillip's outline\n> actually implements the idea, if I am not mistaken.\n\nYou are correct, there is no test_expect_todo in the outline I posted, \ntest_todo lives within test_expect_success and works like test_must_fail.\n>> the test_todo lines change from fail to pass, then that is\n>> communicated as a _failure_ from CI's perspective. Let us discover\n>> if we have accidentally \"fixed\" any of these test cases and update\n>> the tests accordingly.\n> \n> In other words, we do not want to lose the \"TODO fixed\" we have been\n> getting out of test_expect_failure, which I agree with. \n\nI read Stolee's comments the other way, that we want the test harness to \nsee the test fail rather passing as it does with the \"TODO fixed\" \nfeature. In one of my early contributions I inadvertently fixed a \nsubmodule test and did not realize until someone pointed it out because \nthe CI passes rather than fails when a test_expect_failure is fixed.\n\n> I am not\n> sure if Phillip's outline had that feature.\n\nIn my outline the test fails if any command marked by test_todo is \nsuccessful and test_todo prints a messaging saying the command was \nunexpectedly successful. Implementing the \"TODO fixed\" feature gets \ntricky when there is more than one test_todo within a single \ntest_expect_success - what if one is test_todo command is successful and \nthe others fail?\n\n\nBest Wishes\n\nPhillip\n\n>> I can predict writing a test case with multiple test_todo lines\n>> that need to be updated to drop the test_todo helpers one-by-one\n>> as a change is being introduced.\n> \n> Yes.\n\n"}]}