{"thread":{"id":"58573","subject":"[PATCH 0/3] [RFC] tests: add test_todo() for known failures","startedAt":"2022-10-06T15:01:25Z","lastAt":"2022-12-09T09:04:31Z","messageCount":20,"participants":["Phillip Wood via GitGitGadget","Ævar Arnfjörð Bjarmason","Phillip Wood","Junio C Hamano","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"464293","messageId":"pull.1374.git.1665068476.gitgitgadget@gmail.com","threadId":"58573","inReplyTo":null,"subject":"[PATCH 0/3] [RFC] tests: add test_todo() for known failures","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T15:01:13Z","receivedAt":"2022-10-06T15:01:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"test_todo() is intended as a fine grained alternative to\ntest_expect_failure(). Rather than marking the whole test as failing\ntest_todo() is used to mark individual failing commands within a test. This\napproach to writing failing tests allows us to detect unexpected failures\nthat are hidden by test_expect_failure().\n\nThis series attempts to keep most of the benefits test_expect_todo()\npreviously proposed by Ævar[1] while being simpler to use.\n\n[1]\nhttps://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n\nPhillip Wood (3):\n  [RFC] tests: add test_todo() to mark known breakages\n  [RFC] test_todo: allow [!] grep as the command\n  [RFC] test_todo: allow [verbose] test as the command\n\n t/README                        |  12 ++\n t/t0000-basic.sh                |  92 +++++++++++++++\n t/t0050-filesystem.sh           |   4 +-\n t/t3401-rebase-and-am-rename.sh |  12 +-\n t/t3424-rebase-empty.sh         |   6 +-\n t/t3510-cherry-pick-sequence.sh |  12 +-\n t/t3600-rm.sh                   |   8 +-\n t/t3903-stash.sh                |  12 +-\n t/t4014-format-patch.sh         |  20 ++--\n t/test-lib-functions.sh         | 194 +++++++++++++++++++++++++-------\n 10 files changed, 293 insertions(+), 79 deletions(-)\n\n\nbase-commit: bcd6bc478adc4951d57ec597c44b12ee74bc88fb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1374%2Fphillipwood%2Ftest-todo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1374/phillipwood/test-todo-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1374\n-- \ngitgitgadget\n"},{"id":"464294","messageId":"472d05111a38276192e30f454f42aa39df51d604.1665068476.git.gitgitgadget@gmail.com","threadId":"58573","inReplyTo":"pull.1374.git.1665068476.gitgitgadget@gmail.com","subject":"[PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T15:01:14Z","receivedAt":"2022-10-06T15:01:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ntest_todo() is intended as a fine grained replacement for\ntest_expect_failure(). Rather than marking the whole test as failing\ntest_todo() is used to mark individual failing commands within a\ntest. This approach to writing failing tests allows us to detect\nunexpected failures that are hidden by test_expect_failure().\n\nFailing commands are reported by the test harness in the same way as\ntest_expect_failure() so there is no change in output when migrating\nfrom test_expect_failure() to test_todo(). If a command marked with\ntest_todo() succeeds then the test will fail. This is designed to make\nit easier to see when a command starts succeeding in our CI compared\nto using test_expect_failure() where it is easy to fix a failing test\ncase and not realize it.\n\ntest_todo() is built upon test_expect_failure() but accepts commands\nstarting with test_* in addition to git. As our test_* assertions use\nBUG() to signal usage errors any such error will not be hidden by\ntest_todo().\n\nThis commit coverts a few tests to show the intended use of\ntest_todo().  A limitation of test_todo() as it is currently\nimplemented is that it cannot be used in a subshell.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/README                        |  12 +++\n t/t0000-basic.sh                |  64 ++++++++++++++\n t/t3401-rebase-and-am-rename.sh |  12 +--\n t/t3424-rebase-empty.sh         |   6 +-\n t/t3600-rm.sh                   |   8 +-\n t/test-lib-functions.sh         | 147 +++++++++++++++++++++++---------\n 6 files changed, 194 insertions(+), 55 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 979b2d4833d..642aeab80b4 100644\n--- a/t/README\n+++ b/t/README\n@@ -892,6 +892,10 @@ see test-lib-functions.sh for the full list and their options.\n \n  - test_expect_failure [<prereq>] <message> <script>\n \n+   Where possible new tests should use test_expect_success and mark\n+   the individual failing commands with test_todo (see below) rather\n+   than using test_expect_failure.\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    the usual test_expect_success tests, which say \"ok\" on\n@@ -902,6 +906,14 @@ see test-lib-functions.sh for the full list and their options.\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_todo <command>\n+\n+   This is used to mark commands that should succeed but do not due to\n+   a known issue. The test will be reported as a known failure if the\n+   issue still exists and won’t cause -i (immediate) to stop. If the\n+   command unexpectedly succeeds then the test will be reported as a\n+   failing. test_todo cannot be used in a subshell.\n+\n  - test_debug <script>\n \n    This takes a single argument, <script>, and evaluates it only\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 502b4bcf9ea..52362ad3dd3 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -141,6 +141,70 @@ test_expect_success 'subtest: a passing TODO test' '\n \tEOF\n '\n \n+test_expect_success 'subtest: a failing test_todo' '\n+\twrite_and_run_sub_test_lib_test failing-test-todo <<-\\EOF &&\n+\ttest_false () {\n+\t\tfalse\n+\t}\n+\ttest_expect_success \"passing test\" \"true\"\n+\ttest_expect_success \"known todo\" \"test_todo test_false\"\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test failing-test-todo <<-\\EOF\n+\t> ok 1 - passing test\n+\t> not ok 2 - known todo # TODO known breakage\n+\t> # still have 1 known breakage(s)\n+\t> # passed all remaining 1 test(s)\n+\t> 1..2\n+\tEOF\n+'\n+\n+test_expect_success 'subtest: a passing test_todo' '\n+\twrite_and_run_sub_test_lib_test_err passing-test-todo <<-\\EOF &&\n+\ttest_true () {\n+\t\ttrue\n+\t}\n+\ttest_expect_success \"pretend we have fixed a test_todo breakage\" \\\n+\t\t\"test_todo test_true\"\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test passing-test-todo <<-\\EOF\n+\t> not ok 1 - pretend we have fixed a test_todo breakage\n+\t> #\ttest_todo test_true\n+\t> # failed 1 among 1 test(s)\n+\t> 1..1\n+\tEOF\n+'\n+\n+test_expect_success 'subtest: test_todo allowed arguments' '\n+\twrite_and_run_sub_test_lib_test_err acceptable-test-todo <<-\\EOF &&\n+\t# This an acceptable command for test_todo but not test_must_fail\n+\ttest_true () {\n+\t\t  return 0\n+\t}\n+\ttest_expect_success \"test_todo skips env and accepts good command\" \\\n+\t\t\"test_todo env Var=Value git --invalid-option\"\n+\ttest_expect_success \"test_todo skips env and rejects bad command\" \\\n+\t\t\"test_todo env Var=Value false\"\n+\ttest_expect_success \"test_todo test_must_fail accepts good command\" \\\n+\t\t\"test_todo test_must_fail git --version\"\n+\ttest_expect_success \"test_todo test_must_fail rejects bad command\" \\\n+\t\t\"test_todo test_must_fail test_true\"\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test acceptable-test-todo <<-\\EOF\n+\t> not ok 1 - test_todo skips env and accepts good command # TODO known breakage\n+\t> not ok 2 - test_todo skips env and rejects bad command\n+\t> #\ttest_todo env Var=Value false\n+\t> not ok 3 - test_todo test_must_fail accepts good command # TODO known breakage\n+\t> not ok 4 - test_todo test_must_fail rejects bad command\n+\t> #\ttest_todo test_must_fail test_true\n+\t> # still have 2 known breakage(s)\n+\t> # failed 2 among remaining 2 test(s)\n+\t> 1..4\n+\tEOF\n+'\n+\n test_expect_success 'subtest: 2 TODO tests, one passin' '\n \twrite_and_run_sub_test_lib_test partially-passing-todos <<-\\EOF &&\n \ttest_expect_failure \"pretend we have a known breakage\" \"false\"\ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex f18bae94507..cc5da9f5afe 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 5e1045a0afc..b7cae260759 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/t3600-rm.sh b/t/t3600-rm.sh\nindex e74a318ac33..fa7831c0674 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_success 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,10 @@ 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_todo test_must_fail git rm d/f &&\n+\ttest_todo git rev-parse --verify :d/f &&\n \ttest -h d &&\n-\ttest_path_is_file e/f\n+\ttest_todo test_path_is_file 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 c6479f24eb5..8978709b231 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -802,6 +802,7 @@ test_expect_failure () {\n \texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\n+\t\ttest_todo_=test_expect_failure\n \t\ttest -n \"$test_skip_test_preamble\" ||\n \t\tsay >&3 \"checking known breakage of $TEST_NUMBER.$test_count '$1': $2\"\n \t\tif test_run_ \"$2\" expecting_failure\n@@ -825,9 +826,15 @@ test_expect_success () {\n \tthen\n \t\ttest -n \"$test_skip_test_preamble\" ||\n \t\tsay >&3 \"expecting success of $TEST_NUMBER.$test_count '$1': $2\"\n+\t\ttest_todo_=test_expect_success\n \t\tif test_run_ \"$2\"\n \t\tthen\n-\t\t\ttest_ok_ \"$1\"\n+\t\t\tif test \"$test_todo_\" = \"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@@ -999,10 +1006,18 @@ list_contains () {\n }\n \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-# test the command being run.\n+# accepted by test_must_fail() or test_todo(). If the command is run\n+# with env, the env and its corresponding variable settings will be\n+# stripped before we we test the command being run.\n+#\n+# test_todo() allows any of the assertions beginning test_ such as\n+# test_cmp in addition the commands allowed by test_must_fail().\n+\n test_must_fail_acceptable () {\n+\tlocal name\n+\tname=\"$1\"\n+\tshift\n+\n \tif test \"$1\" = \"env\"\n \tthen\n \t\tshift\n@@ -1023,12 +1038,96 @@ test_must_fail_acceptable () {\n \tgit|__git*|test-tool|test_terminal)\n \t\treturn 0\n \t\t;;\n+\ttest_might_fail|test_todo|test_when_finished)\n+\t\treturn 1\n+\t\t;;\n+\ttest_must_fail)\n+\t\tif test \"$name\" = \"todo\"\n+\t\tthen\n+\t\t\tshift\n+\t\t\ttest_must_fail_acceptable must_fail \"$@\"\n+\t\t\treturn $?\n+\t\tfi\n+\t\treturn 1\n+\t\t;;\n+\ttest_*)\n+\t\ttest \"$name\" = \"todo\"\n+\t\treturn $?\n+\t\t;;\n \t*)\n \t\treturn 1\n \t\t;;\n \tesac\n }\n \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+\t\tshift\n+\t\t;;\n+\t*)\n+\t\t_test_ok=\n+\t\t;;\n+\tesac\n+\tif ! test_must_fail_acceptable $test_must_fail_name_ \"$@\"\n+\tthen\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_$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_$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_$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_$test_must_fail_name_: valgrind error: $*\"\n+\t\treturn 1\n+\tfi\n+\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 will also fail. 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+# test_todo can be used with \"git\" or any of the \"test_*\" assertions\n+# such as test_cmp().\n+\n+test_todo () {\n+\tif test \"$test_todo_\" = \"test_expect_failure\"\n+\tthen\n+\t\tBUG \"test_todo_ cannot be used inside test_expect_failure\"\n+\tfi\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@@ -1061,43 +1160,7 @@ test_must_fail_acceptable () {\n #    ! grep pattern output\n \n test_must_fail () {\n-\tcase \"$1\" in\n-\tok=*)\n-\t\t_test_ok=${1#ok=}\n-\t\tshift\n-\t\t;;\n-\t*)\n-\t\t_test_ok=\n-\t\t;;\n-\tesac\n-\tif ! test_must_fail_acceptable \"$@\"\n-\tthen\n-\t\techo >&7 \"test_must_fail: 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\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\treturn 1\n-\telif test $exit_code -eq 127\n-\tthen\n-\t\techo >&4 \"test_must_fail: 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\treturn 1\n-\tfi\n-\treturn 0\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@@ -1114,7 +1177,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-- \ngitgitgadget\n\n"},{"id":"464295","messageId":"645fa2990f79bdb7ee00ff3fd34122676469a783.1665068476.git.gitgitgadget@gmail.com","threadId":"58573","inReplyTo":"pull.1374.git.1665068476.gitgitgadget@gmail.com","subject":"[PATCH 2/3] [RFC] test_todo: allow [!] grep as the command","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T15:01:15Z","receivedAt":"2022-10-06T15:01:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nMany failing tests use grep, this commit converts a sample to use\ntest_todo(). As POSIX specifies that a return code greater than one\nindicates an error rather than a failing match we take care not the\nhide that.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t0000-basic.sh                | 20 ++++++++++--\n t/t3510-cherry-pick-sequence.sh | 12 +++----\n t/t4014-format-patch.sh         | 20 ++++++------\n t/test-lib-functions.sh         | 56 ++++++++++++++++++++++++++++-----\n 4 files changed, 82 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 52362ad3dd3..db5c2059eb5 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -190,6 +190,14 @@ test_expect_success 'subtest: test_todo allowed arguments' '\n \t\t\"test_todo test_must_fail git --version\"\n \ttest_expect_success \"test_todo test_must_fail rejects bad command\" \\\n \t\t\"test_todo test_must_fail test_true\"\n+\ttest_expect_success \"test_todo accepts grep\" \\\n+\t\t\"echo a >a && test_todo grep b <a\"\n+\ttest_expect_success \"test_todo accepts ! grep\" \\\n+\t\t\"echo a >a && test_todo ! grep -v b <a\"\n+\ttest_expect_success \"test_todo detects grep errors\" \\\n+\t\t\"echo a >a && test_todo grep --invalid-option b <a\"\n+\ttest_expect_success \"test_todo detects ! grep errors\" \\\n+\t\t\"echo a >a && test_todo ! grep --invalid-option -v b <a\"\n \ttest_done\n \tEOF\n \tcheck_sub_test_lib_test acceptable-test-todo <<-\\EOF\n@@ -199,9 +207,15 @@ test_expect_success 'subtest: test_todo allowed arguments' '\n \t> not ok 3 - test_todo test_must_fail accepts good command # TODO known breakage\n \t> not ok 4 - test_todo test_must_fail rejects bad command\n \t> #\ttest_todo test_must_fail test_true\n-\t> # still have 2 known breakage(s)\n-\t> # failed 2 among remaining 2 test(s)\n-\t> 1..4\n+\t> not ok 5 - test_todo accepts grep # TODO known breakage\n+\t> not ok 6 - test_todo accepts ! grep # TODO known breakage\n+\t> not ok 7 - test_todo detects grep errors\n+\t> #\techo a >a && test_todo grep --invalid-option b <a\n+\t> not ok 8 - test_todo detects ! grep errors\n+\t> #\techo a >a && test_todo ! grep --invalid-option -v b <a\n+\t> # still have 4 known breakage(s)\n+\t> # failed 4 among remaining 4 test(s)\n+\t> 1..8\n \tEOF\n '\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3b0fa66c33d..cd869255a2c 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -577,7 +577,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n \tgrep \"cherry picked from.*$picked\" msg\n '\n \n-test_expect_failure '--signoff is automatically propagated to resolved conflict' '\n+test_expect_success '--signoff is automatically propagated to resolved conflict' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick --signoff base..anotherpick &&\n \techo \"c\" >foo &&\n@@ -591,11 +591,11 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'\n \tgit cat-file commit HEAD~3 >initial_msg &&\n \t! grep \"Signed-off-by:\" initial_msg &&\n \tgrep \"Signed-off-by:\" unrelatedpick_msg &&\n-\t! grep \"Signed-off-by:\" picked_msg &&\n+\ttest_todo ! grep \"Signed-off-by:\" picked_msg &&\n \tgrep \"Signed-off-by:\" anotherpick_msg\n '\n \n-test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '\n+test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick -s picked anotherpick &&\n \techo c >foo &&\n@@ -605,10 +605,10 @@ test_expect_failure '--signoff dropped for implicit commit of resolution, multi-\n \tgit diff --exit-code HEAD &&\n \ttest_cmp_rev initial HEAD^^ &&\n \tgit cat-file commit HEAD^ >msg &&\n-\t! grep Signed-off-by: msg\n+\ttest_todo ! grep Signed-off-by: msg\n '\n \n-test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '\n+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick -s picked &&\n \techo c >foo &&\n@@ -618,7 +618,7 @@ test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution,\n \tgit diff --exit-code HEAD &&\n \ttest_cmp_rev initial HEAD^ &&\n \tgit cat-file commit HEAD >msg &&\n-\t! grep Signed-off-by: msg\n+\ttest_todo ! grep Signed-off-by: msg\n '\n \n test_expect_success 'malformed instruction sheet 1' '\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ad5c0292794..ab649bf27e5 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 8978709b231..aee10bd0706 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1010,8 +1010,9 @@ list_contains () {\n # with env, the env and its corresponding variable settings will be\n # stripped before we we test the command being run.\n #\n-# test_todo() allows any of the assertions beginning test_ such as\n-# test_cmp in addition the commands allowed by test_must_fail().\n+# test_todo() allows \"grep\" or any of the assertions beginning test_\n+# such as test_cmp in addition the commands allowed by\n+# test_must_fail().\n \n test_must_fail_acceptable () {\n \tlocal name\n@@ -1050,9 +1051,21 @@ test_must_fail_acceptable () {\n \t\tfi\n \t\treturn 1\n \t\t;;\n-\ttest_*)\n-\t\ttest \"$name\" = \"todo\"\n-\t\treturn $?\n+\tgrep|test_*)\n+\t\tif test \"$name\" = \"todo\"\n+\t\tthen\n+\t\t\ttest_todo_command_=\"$1\"\n+\t\t\treturn 0\n+\t\tfi\n+\t\treturn 1\n+\t\t;;\n+\t'!')\n+\t\tif test \"$name\" = \"todo\" && test \"$2\" = \"grep\"\n+\t\tthen\n+\t\t\ttest_todo_command_=grep\n+\t\t\treturn 0\n+\t\tfi\n+\t\treturn 1\n \t\t;;\n \t*)\n \t\treturn 1\n@@ -1061,6 +1074,7 @@ test_must_fail_acceptable () {\n }\n \n test_must_fail_helper () {\n+\ttest_todo_command_=\n \ttest_must_fail_name_=\"$1\"\n \tshift\n \tcase \"$1\" in\n@@ -1077,8 +1091,27 @@ test_must_fail_helper () {\n \t\techo >&7 \"test_$test_must_fail_name_: only 'git' is allowed: $*\"\n \t\treturn 1\n \tfi\n+\ttest_invert_exit_code_=\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\ttest_invert_exit_code_=1\n+\t\tshift\n+\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n+\tif test -n \"$test_invert_exit_code_\"\n+\t# We only invert the exit code of grep. An exit code greater\n+\t# than 1 indicates an error rather than a failed match so\n+\t# we only want to swap zero and one.\n+\tthen\n+\t\tif test $exit_code -eq 0\n+\t\tthen\n+\t\t\texit_code=1\n+\t\telif test $exit_code -eq 1\n+\t\tthen\n+\t\t\texit_code=0\n+\t\tfi\n+\tfi\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n \t\techo >&4 \"test_$test_must_fail_name_: command succeeded: $*\"\n@@ -1098,6 +1131,15 @@ test_must_fail_helper () {\n \tthen\n \t\techo >&4 \"test_$test_must_fail_name_: valgrind error: $*\"\n \t\treturn 1\n+\telse\n+\t\tcase \"$test_todo_command_\" in\n+\t\tgrep)\n+\t\t\tif test $exit_code -ne 1\n+\t\t\tthen\n+\t\t\t       echo >&4 \"test_todo: $test_todo_command_ error: $*\"\n+\t\t\t       return 1\n+\t\t\tfi;;\n+\t\tesac\n \tfi\n \n \treturn 0\n@@ -1116,8 +1158,8 @@ test_must_fail_helper () {\n #\t'\n #\n # This test will fail if \"git foo\" fails or err is unexpectedly empty.\n-# test_todo can be used with \"git\" or any of the \"test_*\" assertions\n-# such as test_cmp().\n+# test_todo can be used with \"git\", \"grep\" or any of the \"test_*\"\n+# assertions such as test_cmp().\n \n test_todo () {\n \tif test \"$test_todo_\" = \"test_expect_failure\"\n-- \ngitgitgadget\n\n"},{"id":"464296","messageId":"c0d955c2d1fa234ad2e4a8aad467b991030ccf47.1665068476.git.gitgitgadget@gmail.com","threadId":"58573","inReplyTo":"pull.1374.git.1665068476.gitgitgadget@gmail.com","subject":"[PATCH 3/3] [RFC] test_todo: allow [verbose] test as the command","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-06T15:01:16Z","receivedAt":"2022-10-06T15:01:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFor simplicity test_todo() allows verbose to precede any valid\ncommand.  As POSIX specifies that a return code greater than one is an\nerror rather than a failed test we take care not to hide that.\n\nI'm in two minds about this patch. Generally it is better to use one\nof our test helpers such as test_cmp() rather than calling test\ndirectly.  There are so few instances of test being used within\ntest_expect_failure() (the conversions here are not exhaustive but\nthere are not many more) that it would probably be better to convert\nthe tests by using the appropriate helper rather than supporting\ncalling test as the command to test_todo().\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t0000-basic.sh        | 20 +++++++++++++++++---\n t/t0050-filesystem.sh   |  4 ++--\n t/t3903-stash.sh        | 12 ++++++------\n t/test-lib-functions.sh | 17 +++++++++++------\n 4 files changed, 36 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex db5c2059eb5..93d3930d9f6 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -198,6 +198,14 @@ test_expect_success 'subtest: test_todo allowed arguments' '\n \t\t\"echo a >a && test_todo grep --invalid-option b <a\"\n \ttest_expect_success \"test_todo detects ! grep errors\" \\\n \t\t\"echo a >a && test_todo ! grep --invalid-option -v b <a\"\n+\ttest_expect_success \"test_todo accepts test\" \\\n+\t\t\"test_todo test -z a\"\n+\ttest_expect_success \"test_todo detects test errors\" \\\n+\t\t\"test_todo test a xxx b\"\n+\ttest_expect_success \"test_todo skips verbose and accepts good command\" \\\n+\t\t\"test_todo verbose test -z a\"\n+\ttest_expect_success \"test_todo skips verbose and rejects bad command\" \\\n+\t\t\"test_todo verbose false\"\n \ttest_done\n \tEOF\n \tcheck_sub_test_lib_test acceptable-test-todo <<-\\EOF\n@@ -213,9 +221,15 @@ test_expect_success 'subtest: test_todo allowed arguments' '\n \t> #\techo a >a && test_todo grep --invalid-option b <a\n \t> not ok 8 - test_todo detects ! grep errors\n \t> #\techo a >a && test_todo ! grep --invalid-option -v b <a\n-\t> # still have 4 known breakage(s)\n-\t> # failed 4 among remaining 4 test(s)\n-\t> 1..8\n+\t> not ok 9 - test_todo accepts test # TODO known breakage\n+\t> not ok 10 - test_todo detects test errors\n+\t> #\ttest_todo test a xxx b\n+\t> not ok 11 - test_todo skips verbose and accepts good command # TODO known breakage\n+\t> not ok 12 - test_todo skips verbose and rejects bad command\n+\t> #\ttest_todo verbose false\n+\t> # still have 6 known breakage(s)\n+\t> # failed 6 among remaining 6 test(s)\n+\t> 1..12\n \tEOF\n '\n \ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex 325eb1c3cd0..16b933a4c8d 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -100,7 +100,7 @@ test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n \ttest_cmp expected actual\n '\n \n-test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n+test_expect_success CASE_INSENSITIVE_FS 'add (with different case)' '\n \tgit reset --hard initial &&\n \trm camelcase &&\n \techo 1 >CamelCase &&\n@@ -108,7 +108,7 @@ test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n \tgit ls-files >tmp &&\n \tcamel=$(grep -i camelcase tmp) &&\n \ttest $(echo \"$camel\" | wc -l) = 1 &&\n-\ttest \"z$(git cat-file blob :$camel)\" = z1\n+\ttest_todo test \"z$(git cat-file blob :$camel)\" = z1\n '\n \n test_expect_success \"setup unicode normalization tests\" '\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 376cc8f4ab8..afc5b4126f3 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -556,7 +556,7 @@ test_expect_success 'unstash must re-create the file' '\n \ttest bar = \"$(cat file)\"\n '\n \n-test_expect_failure 'stash directory to file' '\n+test_expect_success 'stash directory to file' '\n \tgit reset --hard &&\n \tmkdir dir &&\n \techo foo >dir/file &&\n@@ -567,12 +567,12 @@ test_expect_failure 'stash directory to file' '\n \tgit stash save \"directory to file\" &&\n \ttest_path_is_dir dir &&\n \ttest foo = \"$(cat dir/file)\" &&\n-\ttest_must_fail git stash apply &&\n-\ttest bar = \"$(cat dir)\" &&\n+\ttest_todo test_must_fail git stash apply &&\n+\ttest_todo test bar = \"$(cat dir)\" &&\n \tgit reset --soft HEAD^\n '\n \n-test_expect_failure 'stash file to directory' '\n+test_expect_success 'stash file to directory' '\n \tgit reset --hard &&\n \trm file &&\n \tmkdir file &&\n@@ -581,8 +581,8 @@ test_expect_failure 'stash file to directory' '\n \ttest_path_is_file file &&\n \ttest bar = \"$(cat file)\" &&\n \tgit stash apply &&\n-\ttest_path_is_file file/file &&\n-\ttest foo = \"$(cat file/file)\"\n+\ttest_todo test_path_is_file file/file &&\n+\ttest_todo test foo = \"$(cat file/file)\"\n '\n \n test_expect_success 'giving too many ref arguments does not modify files' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex aee10bd0706..068a0702809 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1010,8 +1010,8 @@ list_contains () {\n # with env, the env and its corresponding variable settings will be\n # stripped before we we test the command being run.\n #\n-# test_todo() allows \"grep\" or any of the assertions beginning test_\n-# such as test_cmp in addition the commands allowed by\n+# test_todo() allows \"grep\", \"test\" or any of the assertions beginning\n+# test_ such as test_cmp in addition the commands allowed by\n # test_must_fail().\n \n test_must_fail_acceptable () {\n@@ -1019,6 +1019,11 @@ test_must_fail_acceptable () {\n \tname=\"$1\"\n \tshift\n \n+\tif test \"$name\" = \"todo\" && test \"$1\" = \"verbose\"\n+\tthen\n+\t\tshift\n+\tfi\n+\n \tif test \"$1\" = \"env\"\n \tthen\n \t\tshift\n@@ -1051,7 +1056,7 @@ test_must_fail_acceptable () {\n \t\tfi\n \t\treturn 1\n \t\t;;\n-\tgrep|test_*)\n+\tgrep|test|test_*)\n \t\tif test \"$name\" = \"todo\"\n \t\tthen\n \t\t\ttest_todo_command_=\"$1\"\n@@ -1133,7 +1138,7 @@ test_must_fail_helper () {\n \t\treturn 1\n \telse\n \t\tcase \"$test_todo_command_\" in\n-\t\tgrep)\n+\t\tgrep|test)\n \t\t\tif test $exit_code -ne 1\n \t\t\tthen\n \t\t\t       echo >&4 \"test_todo: $test_todo_command_ error: $*\"\n@@ -1158,8 +1163,8 @@ test_must_fail_helper () {\n #\t'\n #\n # This test will fail if \"git foo\" fails or err is unexpectedly empty.\n-# test_todo can be used with \"git\", \"grep\" or any of the \"test_*\"\n-# assertions such as test_cmp().\n+# test_todo can be used with \"git\", \"grep\", \"test\" or any of the\n+# \"test_*\" assertions such as test_cmp().\n \n test_todo () {\n \tif test \"$test_todo_\" = \"test_expect_failure\"\n-- \ngitgitgadget\n"},{"id":"464299","messageId":"221006.868rltrltu.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"472d05111a38276192e30f454f42aa39df51d604.1665068476.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T15:36:35Z","receivedAt":"2022-10-06T15:52:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> test_todo() is intended as a fine grained replacement for\n> test_expect_failure(). Rather than marking the whole test as failing\n> test_todo() is used to mark individual failing commands within a\n> test. This approach to writing failing tests allows us to detect\n> unexpected failures that are hidden by test_expect_failure().\n>\n> Failing commands are reported by the test harness in the same way as\n> test_expect_failure() so there is no change in output when migrating\n> from test_expect_failure() to test_todo(). If a command marked with\n> test_todo() succeeds then the test will fail. This is designed to make\n> it easier to see when a command starts succeeding in our CI compared\n> to using test_expect_failure() where it is easy to fix a failing test\n> case and not realize it.\n>\n> test_todo() is built upon test_expect_failure() but accepts commands\n> starting with test_* in addition to git. As our test_* assertions use\n> BUG() to signal usage errors any such error will not be hidden by\n> test_todo().\n\nI think they will, unless I'm missing something. E.g. try out:\n\t\n\tdiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n\tindex 80e76a4695e..1be895abba6 100755\n\t--- a/t/t0210-trace2-normal.sh\n\t+++ b/t/t0210-trace2-normal.sh\n\t@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '\n\t \n\t test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '\n\t \ttest_when_finished \"rm trace.normal actual expect\" &&\n\t-\ttest_expect_code 99 env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n\t+\ttest_todo env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n\t \t\ttest-tool trace2 008bug 2>err &&\n\t \tcat >expect <<-\\EOF &&\n\t \ta bug message\n\nI.e. in our tests you need to look out for exit code 99, not the usual\nabort().\n\nI have local patches to fix this, previously submitted as an RFC here:\nhttps://lore.kernel.org/git/RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com/\n\nI just rebased that & CI is currently running, I'll see how it does:\nhttps://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2\n\nWhen I merged your patches here with that topic yours started doing the\nright thing in this case, i.e. a \"test_todo\" that would get a BUG()\"\nwould be reported as a failure.\n\n> +\ttest_true () {\n> +\t\ttrue\n> +\t}\n> +\ttest_expect_success \"pretend we have fixed a test_todo breakage\" \\\n> +\t\t\"test_todo test_true\"\n\n\"Why the indirection\", until I realized that it's because you're working\naround the whitelist of commands that we have, i.e. we allow 'git' and\n'test-tool', but not 'grep' or whatever.\n\nI'm of the opinion that we should just drop that limitation altogether,\nwhich is shown to be pretty silly in this case. I.e. at some point we\nlisted \"test_*\" as \"this invokes a git binary\", but a lot of our test_*\ncommands don't, including this one.\n\nSo in general I think we should just allow any command in\n\"test_must_fail\" et al, and just catch in code review if someone uses\n\"test_must_fail grep\" as opposed to \"! grep\".\n\nBut for the particular case of \"test_todo\" doing so seems like pointless\nwork, if we think we're going to miss this sort of thing in review in\ngeneral, then surely that's not a concern if we're going to very\nprominently mark tests as TODO tests, given how the test of the output\nshows them?\n\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> +\t\tshift\n> +\t\t;;\n> +\t*)\n> +\t\t_test_ok=\n> +\t\t;;\n> +\tesac\n> +\tif ! test_must_fail_acceptable $test_must_fail_name_ \"$@\"\n> +\tthen\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_$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_$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_$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_$test_must_fail_name_: valgrind error: $*\"\n> +\t\treturn 1\n> +\tfi\n> +\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 will also fail. 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> +# test_todo can be used with \"git\" or any of the \"test_*\" assertions\n> +# such as test_cmp().\n> +\n> +test_todo () {\n> +\tif test \"$test_todo_\" = \"test_expect_failure\"\n> +\tthen\n> +\t\tBUG \"test_todo_ cannot be used inside test_expect_failure\"\n> +\tfi\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> @@ -1061,43 +1160,7 @@ test_must_fail_acceptable () {\n>  #    ! grep pattern output\n>  \n>  test_must_fail () {\n> -\tcase \"$1\" in\n> -\tok=*)\n> -\t\t_test_ok=${1#ok=}\n> -\t\tshift\n> -\t\t;;\n> -\t*)\n> -\t\t_test_ok=\n> -\t\t;;\n> -\tesac\n> -\tif ! test_must_fail_acceptable \"$@\"\n> -\tthen\n> -\t\techo >&7 \"test_must_fail: 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\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\treturn 1\n> -\telif test $exit_code -eq 127\n> -\tthen\n> -\t\techo >&4 \"test_must_fail: 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\treturn 1\n> -\tfi\n> -\treturn 0\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> @@ -1114,7 +1177,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\nI remember finding it annoying that \"test_might_fail\" is misreported\nfrom test_must_fail_acceptable as being called \"test_must_fail\", so this\nrefactoring is very welcome.\n\nBut can you please split this into its own commit? I.e. that improvement\ncan stand on its own, i.e. just a change that has \"test_must_fail\" and\n\"test_might_fail\" reporting their correct name.\n\nThen this commit can follow, and then you'll just need to add (for this part):\n\n\ttest_must_fail_helper todo \"$@\" 2>&7\n\nAnd it'll make the resulting diff much smaller & easier to follow.\n"},{"id":"464301","messageId":"221006.864jwhrldr.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"645fa2990f79bdb7ee00ff3fd34122676469a783.1665068476.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T15:56:00Z","receivedAt":"2022-10-06T16:02:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Many failing tests use grep, this commit converts a sample to use\n> test_todo(). As POSIX specifies that a return code greater than one\n> indicates an error rather than a failing match we take care not the\n> hide that.\n\nAh, so on the one hand this gives me second thoughts about my stance\nin[1], i.e. if we just allowed any command we wouldn't be forced to add\nthese sorts of special-cases.\n\nAlthough, we could also allow any command, and just add smartness for\nones we know about, e.g. \"grep\".\n\nBut I do find doing this to be weirdly inconsistent, i.e. caring about\nthe difference between these two:\n\n\t$ grep blah README.md; echo $?\n\t1\n\t$ grep blah README.mdx; echo $?\n\tgrep: README.mdx: No such file or directory\n\t2\n\nIs basically why I took the approach I did in my [2], i.e. to force us\nto positively assert *what* the bad behavior should be.\n\nWhich is why I ended up doing my verison of this sort of thing as [3],\ni.e. you'd need to assert what specific exit code you expected, or\nequivalent.\n\nBut at this point in the series:\n\t\n\tdiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n\tindex fa7831c0674..086eaf91351 100755\n\t--- a/t/t3600-rm.sh\n\t+++ b/t/t3600-rm.sh\n\t@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n\t \ttest_todo test_must_fail git rm d/f &&\n\t \ttest_todo git rev-parse --verify :d/f &&\n\t \ttest -h d &&\n\t-\ttest_todo test_path_is_file e/f\n\t+\ttest_todo test_path_is_file blah\n\t '\n\t \n\t test_expect_success 'setup for testing rm messages' '\n\nSo, for our own test_path_* helpers we're not going to care at all, and\nany failure will do (including a missing file), but we will care for\ngrep?\n\nI'm obviously more on the \"let's care\" side, I just find it odd that\nyou've picked this halfway point here, but not for other things you're\nwrapping.\n\n1. https://lore.kernel.org/git/221006.868rltrltu.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n3. https://lore.kernel.org/git/patch-5.7-553670da8a9-20220318T002951Z-avarab@gmail.com/\n"},{"id":"464303","messageId":"221006.86zge9q6js.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"c0d955c2d1fa234ad2e4a8aad467b991030ccf47.1665068476.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] [RFC] test_todo: allow [verbose] test as the command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T16:02:23Z","receivedAt":"2022-10-06T16:07:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> For simplicity test_todo() allows verbose to precede any valid\n> command.  As POSIX specifies that a return code greater than one is an\n> error rather than a failed test we take care not to hide that.\n>\n> I'm in two minds about this patch. Generally it is better to use one\n> of our test helpers such as test_cmp() rather than calling test\n> directly.  There are so few instances of test being used within\n> test_expect_failure() (the conversions here are not exhaustive but\n> there are not many more) that it would probably be better to convert\n> the tests by using the appropriate helper rather than supporting\n> calling test as the command to test_todo().\n\nI think that we might want to salvage parts of this, but we really\nshouldn't be spending review time on carrying forward a bad pattern that\nhides segfaults.\n\nI.e. whatever we do about \"test_todo\"'s interaction with \"test\" let's\nfirst change things like...\n\n> -test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n> +test_expect_success CASE_INSENSITIVE_FS 'add (with different case)' '\n>  \tgit reset --hard initial &&\n>  \trm camelcase &&\n>  \techo 1 >CamelCase &&\n> @@ -108,7 +108,7 @@ test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n>  \tgit ls-files >tmp &&\n>  \tcamel=$(grep -i camelcase tmp) &&\n>  \ttest $(echo \"$camel\" | wc -l) = 1 &&\n> -\ttest \"z$(git cat-file blob :$camel)\" = z1\n> +\ttest_todo test \"z$(git cat-file blob :$camel)\" = z1\n\n...this to e.g.:\n\n\techo z1 >expect &&\n\tgit cat-file blob :$camel >actual &&\n\ttest_cmp expect actual\n\nOr whatever, then let's see if migrating \"verbose\" is worthwhile, in the\npost-image you end up with no real users of it, only your tests.\n\nI've wanted to just remove it for a while, all its users seem to be\neither bad uses like that, or we'd get much better bang for the buck out\nof it by having a t/verbose-bin/ or whatever, which would just wrap\narbitrary commands like \"grep\" and the like (i.e. ones where we could\nprovide useful context).\n"},{"id":"464304","messageId":"7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk","threadId":"58573","inReplyTo":"221006.868rltrltu.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-06T16:10:05Z","receivedAt":"2022-10-06T16:10:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 06/10/2022 16:36, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n >>\n>> test_todo() is built upon test_expect_failure() but accepts commands\n>> starting with test_* in addition to git. As our test_* assertions use\n>> BUG() to signal usage errors any such error will not be hidden by\n>> test_todo().\n> \n> I think they will, unless I'm missing something. E.g. try out:\n\nIt's talking about BUG() in test-lib.sh, not the C function. For example\n\ntest_path_is_file () {\n\ttest \"$#\" -ne 1 && BUG \"1 param\"\n\tif ! test -f \"$1\"\n\tthen\n\t\techo \"File $1 doesn't exist\"\n\t\tfalse\n\tfi\n}\n\nSo a test containing \"test_todo test_path_is_file a b\" should fail as \nBUG calls exit rather than returning non-zero (I should probably test \nthat in 0000-basic.sh)\n\n> \tdiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n> \tindex 80e76a4695e..1be895abba6 100755\n> \t--- a/t/t0210-trace2-normal.sh\n> \t+++ b/t/t0210-trace2-normal.sh\n> \t@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '\n> \t\n> \t test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '\n> \t \ttest_when_finished \"rm trace.normal actual expect\" &&\n> \t-\ttest_expect_code 99 env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n> \t+\ttest_todo env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n> \t \t\ttest-tool trace2 008bug 2>err &&\n> \t \tcat >expect <<-\\EOF &&\n> \t \ta bug message\n> \n> I.e. in our tests you need to look out for exit code 99, not the usual\n> abort().\n> \n> I have local patches to fix this, previously submitted as an RFC here:\n> https://lore.kernel.org/git/RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com/\n> \n> I just rebased that & CI is currently running, I'll see how it does:\n> https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2\n> \n> When I merged your patches here with that topic yours started doing the\n> right thing in this case, i.e. a \"test_todo\" that would get a BUG()\"\n> would be reported as a failure.\n> \n>> +\ttest_true () {\n>> +\t\ttrue\n>> +\t}\n>> +\ttest_expect_success \"pretend we have fixed a test_todo breakage\" \\\n>> +\t\t\"test_todo test_true\"\n> \n> \"Why the indirection\", until I realized that it's because you're working\n> around the whitelist of commands that we have, i.e. we allow 'git' and\n> 'test-tool', but not 'grep' or whatever.\n> \n> I'm of the opinion that we should just drop that limitation altogether,\n> which is shown to be pretty silly in this case. I.e. at some point we\n> listed \"test_*\" as \"this invokes a git binary\", but a lot of our test_*\n> commands don't, including this one.\n\ntest_expect_failure does not allow test_* are you thinking of test-tool?\n\n> So in general I think we should just allow any command in\n> \"test_must_fail\" et al, and just catch in code review if someone uses\n> \"test_must_fail grep\" as opposed to \"! grep\".\n\nThat is not going to scale well\n\n> But for the particular case of \"test_todo\" doing so seems like pointless\n> work, if we think we're going to miss this sort of thing in review in\n> general, then surely that's not a concern if we're going to very\n> prominently mark tests as TODO tests, given how the test of the output\n> shows them?\n\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> \n> I remember finding it annoying that \"test_might_fail\" is misreported\n> from test_must_fail_acceptable as being called \"test_must_fail\", so this\n> refactoring is very welcome.\n> \n> But can you please split this into its own commit? I.e. that improvement\n> can stand on its own, i.e. just a change that has \"test_must_fail\" and\n> \"test_might_fail\" reporting their correct name.\n >\n> Then this commit can follow, and then you'll just need to add (for this part) >\n> \ttest_must_fail_helper todo \"$@\" 2>&7\n> \n> And it'll make the resulting diff much smaller & easier to follow.\n\nSure, I should also improve the error message in\n\n >> +\t\techo >&7 \"test_$test_must_fail_name_: only 'git' is allowed: $*\"\n\nfor test_todo.\n\nBest Wishes\n\nPhillip\n"},{"id":"464308","messageId":"c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk","threadId":"58573","inReplyTo":"221006.864jwhrldr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-06T16:42:50Z","receivedAt":"2022-10-06T16:42:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Many failing tests use grep, this commit converts a sample to use\n>> test_todo(). As POSIX specifies that a return code greater than one\n>> indicates an error rather than a failing match we take care not the\n>> hide that.\n> \n> Ah, so on the one hand this gives me second thoughts about my stance\n> in[1], i.e. if we just allowed any command we wouldn't be forced to add\n> these sorts of special-cases.\n> \n> Although, we could also allow any command, and just add smartness for\n> ones we know about, e.g. \"grep\".\n> \n> But I do find doing this to be weirdly inconsistent, i.e. caring about\n> the difference between these two:\n> \n> \t$ grep blah README.md; echo $?\n> \t1\n> \t$ grep blah README.mdx; echo $?\n> \tgrep: README.mdx: No such file or directory\n> \t2\n\nThe intent was to catch bad options, not missing files (i.e. we don't \nwant test_todo to hide a failure from \"grep --invalid-option\"). We could \ncheck the file exists and skip running grep if it does not (hopefully \nthe test wont be grepping multiple files in a single command)\n\n> Is basically why I took the approach I did in my [2], i.e. to force us\n> to positively assert *what* the bad behavior should be.\n\nThat is what made the end result so hard to use though\n\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\nis not exactly readable.\n\nBest Wishes\n\nPhillip\n\n> Which is why I ended up doing my verison of this sort of thing as [3],\n> i.e. you'd need to assert what specific exit code you expected, or\n> equivalent.\n> \n> But at this point in the series:\n> \t\n> \tdiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> \tindex fa7831c0674..086eaf91351 100755\n> \t--- a/t/t3600-rm.sh\n> \t+++ b/t/t3600-rm.sh\n> \t@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n> \t \ttest_todo test_must_fail git rm d/f &&\n> \t \ttest_todo git rev-parse --verify :d/f &&\n> \t \ttest -h d &&\n> \t-\ttest_todo test_path_is_file e/f\n> \t+\ttest_todo test_path_is_file blah\n> \t '\n> \t\n> \t test_expect_success 'setup for testing rm messages' '\n> \n> So, for our own test_path_* helpers we're not going to care at all, and\n> any failure will do (including a missing file), but we will care for\n> grep?\n> \n> I'm obviously more on the \"let's care\" side, I just find it odd that\n> you've picked this halfway point here, but not for other things you're\n> wrapping.\n> \n> 1. https://lore.kernel.org/git/221006.868rltrltu.gmgdl@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n> 3. https://lore.kernel.org/git/patch-5.7-553670da8a9-20220318T002951Z-avarab@gmail.com/\n"},{"id":"464310","messageId":"xmqqsfk0lw6c.fsf@gitster.g","threadId":"58573","inReplyTo":"pull.1374.git.1665068476.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] [RFC] tests: add test_todo() for known failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T17:05:31Z","receivedAt":"2022-10-06T17:05:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> test_todo() is intended as a fine grained alternative to\n> test_expect_failure(). Rather than marking the whole test as failing\n> test_todo() is used to mark individual failing commands within a test. This\n> approach to writing failing tests allows us to detect unexpected failures\n> that are hidden by test_expect_failure().\n>\n> This series attempts to keep most of the benefits test_expect_todo()\n> previously proposed by Ævar[1] while being simpler to use.\n\nGreat.  We discussed this some time ago and I am happy to see the\nwork re-ignited.\n\nThanks.\n"},{"id":"464325","messageId":"221006.86v8owr986.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"pull.1374.git.1665068476.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] [RFC] tests: add test_todo() for known failures","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T19:28:44Z","receivedAt":"2022-10-06T20:24:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n\n> test_todo() is intended as a fine grained alternative to\n> test_expect_failure(). Rather than marking the whole test as failing\n> test_todo() is used to mark individual failing commands within a test. This\n> approach to writing failing tests allows us to detect unexpected failures\n> that are hidden by test_expect_failure().\n>\n> This series attempts to keep most of the benefits test_expect_todo()\n> previously proposed by Ævar[1] while being simpler to use.\n>\n> [1]\n> https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n\nI like the interface you've got here much better than the one I\nsubmitted in [1], so much that it's what I tried to write at first :)\n\nBut as you noted in 1/3:\n\n\ttest_todo cannot be used in a subshell.\n\nSo when we do this:\n\t\n\tdiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n\tindex 93d3930d9f6..75b84a09592 100755\n\t--- a/t/t0000-basic.sh\n\t+++ b/t/t0000-basic.sh\n\t@@ -147,7 +147,7 @@ test_expect_success 'subtest: a failing test_todo' '\n\t \t\tfalse\n\t \t}\n\t \ttest_expect_success \"passing test\" \"true\"\n\t-\ttest_expect_success \"known todo\" \"test_todo test_false\"\n\t+\ttest_expect_success \"known todo\" \"(test_todo test_false)\"\n\t \ttest_done\n\t \tEOF\n\t \tcheck_sub_test_lib_test failing-test-todo <<-\\EOF\n\nWe'll get:\n\t\n\t+ diff -u failing-test-todo/expect.out failing-test-todo/out\n\t--- failing-test-todo/expect.out        2022-10-06 19:30:14.093338392 +0000\n\t+++ failing-test-todo/out       2022-10-06 19:30:14.093338392 +0000\n\t@@ -1,5 +1,4 @@\n\t ok 1 - passing test\n\t-not ok 2 - known todo # TODO known breakage\n\t-# still have 1 known breakage(s)\n\t-# passed all remaining 1 test(s)\n\t+ok 2 - known todo\n\t+# passed all 2 test(s)\n\t 1..2\n\nWhat I was initially trying to do when I tried this approach was to make\nthe \"test_todo\" be the equivalent of a sub-test, i.e. when we encounter\none we'd say \"ok N - DESC\" for the current test so far, and then an \"ok\nN+1 - DESC # TODO: $cmd\" for the \"test_todo\" command.\n\nI think I lost the code for that, but I tried hacking something rough up\non top of your series. I don't think it's a viable approach but it works\nas long as we don't have a subshell (the \"remaining N tests\" count is\noff, but it's fixable, I just couldn't be bothered for a WIP hack):\n\t\n\tdiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n\tindex 93d3930d9f6..7e8e0a54558 100755\n\t--- a/t/t0000-basic.sh\n\t+++ b/t/t0000-basic.sh\n\t@@ -148,14 +148,17 @@ test_expect_success 'subtest: a failing test_todo' '\n\t \t}\n\t \ttest_expect_success \"passing test\" \"true\"\n\t \ttest_expect_success \"known todo\" \"test_todo test_false\"\n\t+\ttest_expect_success \"passing test 2\" \"true\"\n\t \ttest_done\n\t \tEOF\n\t \tcheck_sub_test_lib_test failing-test-todo <<-\\EOF\n\t \t> ok 1 - passing test\n\t-\t> not ok 2 - known todo # TODO known breakage\n\t+\t> not ok 2 - known todo: test_false # TODO known breakage\n\t+\t> ok 3 - known todo (post-test_todo)\n\t+\t> ok 4 - passing test 2\n\t \t> # still have 1 known breakage(s)\n\t-\t> # passed all remaining 1 test(s)\n\t-\t> 1..2\n\t+\t> # passed all remaining 3 test(s)\n\t+\t> 1..4\n\t \tEOF\n\t '\n\t \n\t@@ -171,7 +174,8 @@ test_expect_success 'subtest: a passing test_todo' '\n\t \tcheck_sub_test_lib_test passing-test-todo <<-\\EOF\n\t \t> not ok 1 - pretend we have fixed a test_todo breakage\n\t \t> #\ttest_todo test_true\n\t-\t> # failed 1 among 1 test(s)\n\t+\t> # 1 known breakage(s) vanished; please update test(s)\n\t+\t> # failed 1 among remaining 0 test(s)\n\t \t> 1..1\n\t \tEOF\n\t '\n\tdiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n\tindex 068a0702809..54365fe202f 100644\n\t--- a/t/test-lib-functions.sh\n\t+++ b/t/test-lib-functions.sh\n\t@@ -826,15 +826,12 @@ test_expect_success () {\n\t \tthen\n\t \t\ttest -n \"$test_skip_test_preamble\" ||\n\t \t\tsay >&3 \"expecting success of $TEST_NUMBER.$test_count '$1': $2\"\n\t-\t\ttest_todo_=test_expect_success\n\t+\t\ttest_todo_title_=\"$1\"\n\t+\t\ttest_had_todo_=\n\t \t\tif test_run_ \"$2\"\n\t \t\tthen\n\t-\t\t\tif test \"$test_todo_\" = \"todo\"\n\t-\t\t\tthen\n\t-\t\t\t\ttest_known_broken_failure_ \"$1\"\n\t-\t\t\telse\n\t-\t\t\t\ttest_ok_ \"$1\"\n\t-\t\t\tfi\n\t+\t\t\ttest_ok_ \"$1${test_had_todo_:+ (post-test_todo)}\"\n\t+\t\t\ttest_had_todo_=\n\t \t\telse\n\t \t\t\ttest_failure_ \"$@\"\n\t \t\tfi\n\t@@ -1167,12 +1164,26 @@ test_must_fail_helper () {\n\t # \"test_*\" assertions such as test_cmp().\n\t \n\t test_todo () {\n\t+\tif test -z \"$test_todo_title_\"\n\t+\tthen\n\t+\t\tBUG 'test_todo: expected a $test_todo_title_'\n\t+\tfi &&\n\t \tif test \"$test_todo_\" = \"test_expect_failure\"\n\t \tthen\n\t \t\tBUG \"test_todo_ cannot be used inside test_expect_failure\"\n\t+\tfi &&\n\t+\t# Tell \"test_expect_success\" it had a \"test_todo\"\n\t+\ttest_had_todo_=1 &&\n\t+\t# We say that the test up until this point is OK, and emit an \"ok ..\" for it.\n\t+\ttest_ok_ \"$test_todo_title_\" &&\n\t+\tif test_must_fail_helper todo \"$@\" 2>&7\n\t+\tthen\n\t+\t\ttest_known_broken_failure_ \"$test_todo_title_: $*\" 1>&5 2>&6 &&\n\t+\t\ttest_count=$(($test_count+1))\n\t+\telse\n\t+\t\ttest_known_broken_ok_ \"$test_todo_title_: $*\" &&\n\t+\t\treturn 1\n\t \tfi\n\t-\ttest_todo_=todo\n\t-\ttest_must_fail_helper todo \"$@\" 2>&7\n\t } 7>&2 2>&4\n\t \n\t # This is not among top-level (test_expect_success | test_expect_failure)\n\nAnyway, the core difference between the APIs we proposed for this is\nthat you'd do:\n\n\ttest_expect_success 'desc' 'test_todo false'\n\nWhereas I suggested:\n\n\ttest_expect_todo 'desc' '! false'\n\nNow, let's pick apart the differences:\n\n 1. With \"test_expect_todo\" we're declaring \"this is a TODO test\" for\n    the test as a whole.\n\n 2. With your \"test_todo\" we're not doing that, instead we proceed as\n    normal, and then we might note \"we had a TODO\" midway through, then\n    at the end we'll spot that we had a TODO test (but this approach\n    won't work with subshells).\n\n 3. Your \"test_todo\" is basically a \"let's let this pass\", whereas mine\n    was a helper which exhaustively declared *what* the bad behavior\n    was.\n\n    (Although some of yours seems to be midway between the two,\n    i.e. https://lore.kernel.org/git/c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk/)\n\nI think the main critique you and Junio had of my series was to do with\n#3, i.e. that it was a hassle to exhaustively declare what the behavior\nis & should be, as you note in:\nhttps://lore.kernel.org/git/c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk/\n\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\nThat's fair enough, maybe that's not worth the effort. The reason I\ninitially hacked this up was because I'd noticed a behavior difference\nin a command that was only revealed in a test_expect_failure block, but\nbecause we didn't assert *what* the behavior was we didn't notice.\n\nMy version (if fully used) would spot that, but that's because of how I\nwrote the \"tes_todo\", it's orthagonal to #1 and #2 above.\n\nSo I don't see why we wouldn't instead have a \"test_expect_todo\" and\njust write the helper differently, or have a mode where it's less\nstrict, and (if we find it worthwhile) one where it's more strict.\n\nI rebased my\nhttps://lore.kernel.org/git/patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com/\njust now and applied the below on top, which seems to me to give you\npretty much the end result you want, the only difference is that my\nversion will also work in subshells (see the t2500 one):\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex de1ec89007d..fe47e503bd1 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -468,7 +468,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@@ -502,11 +502,11 @@ 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 git -C tweak add folder1/a &&\n \tgit -C tweak sparse-checkout reapply 2>err &&\n-\ttest_must_be_empty err &&\n+\ttest_todo test_must_be_empty err &&\n \ttest_path_is_missing tweak/deep/deeper2/a &&\n-\ttest_path_is_missing tweak/folder1/a &&\n+\ttest_todo test_path_is_missing tweak/folder1/a &&\n \n \tgit -C tweak sparse-checkout disable\n '\ndiff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh\nindex 5c0bf4d21fc..db7c72d38d8 100755\n--- a/t/t2500-untracked-overwriting.sh\n+++ b/t/t2500-untracked-overwriting.sh\n@@ -167,7 +167,7 @@ test_expect_success 'git rebase fast forwarding and untracked files' '\n \t)\n '\n \n-test_expect_failure 'git rebase --autostash and untracked files' '\n+test_expect_todo 'git rebase --autostash and untracked files' '\n \ttest_setup_sequencing rebase_autostash_and_untracked &&\n \t(\n \t\tcd sequencing_rebase_autostash_and_untracked &&\n@@ -176,7 +176,7 @@ test_expect_failure 'git rebase --autostash and untracked files' '\n \t\tmkdir filler &&\n \t\techo precious >filler/file &&\n \t\tcp filler/file expect &&\n-\t\tgit rebase --autostash init &&\n+\t\ttest_todo git rebase --autostash init &&\n \t\ttest_path_is_file filler/file\n \t)\n '\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3b0fa66c33d..b31b6b0f7a0 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -577,7 +577,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n \tgrep \"cherry picked from.*$picked\" msg\n '\n \n-test_expect_failure '--signoff is automatically propagated to resolved conflict' '\n+test_expect_todo '--signoff is automatically propagated to resolved conflict' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick --signoff base..anotherpick &&\n \techo \"c\" >foo &&\n@@ -591,7 +591,7 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'\n \tgit cat-file commit HEAD~3 >initial_msg &&\n \t! grep \"Signed-off-by:\" initial_msg &&\n \tgrep \"Signed-off-by:\" unrelatedpick_msg &&\n-\t! grep \"Signed-off-by:\" picked_msg &&\n+\ttest_todo ! grep \"Signed-off-by:\" picked_msg &&\n \tgrep \"Signed-off-by:\" anotherpick_msg\n '\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex e74a318ac33..6c7929f5557 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,10 @@ 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_todo test_must_fail git rm d/f &&\n+\ttest_todo git rev-parse --verify :d/f &&\n \ttest -h d &&\n-\ttest_path_is_file e/f\n+\ttest_todo test_path_is_file e/f\n '\n \n test_expect_success 'setup for testing rm messages' '\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ad5c0292794..a6a5a330180 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_todo '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' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex f342954de11..9d5706454a5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1049,6 +1049,21 @@ test_must_fail_acceptable () {\n \tesac\n }\n \n+test_todo () {\n+\tlocal negate=-ne\n+\tlocal cmp_op=-ne\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\tnegate=t &&\n+\t\tcmp_op=-eq\n+\t\tshift\n+\tfi &&\n+\t\"$@\" 2>&7\n+\texit_code=$?\n+\tsay \"test_todo: got $exit_code ${negate:+negated!} from $*\"\n+\ttest \"$exit_code\" \"$cmp_op\" 0\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\n\n"},{"id":"464326","messageId":"221006.86r0zkr8w2.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk","subject":"Re: [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T20:26:03Z","receivedAt":"2022-10-06T20:32:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood wrote:\n\n> On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n>> \n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> Many failing tests use grep, this commit converts a sample to use\n>>> test_todo(). As POSIX specifies that a return code greater than one\n>>> indicates an error rather than a failing match we take care not the\n>>> hide that.\n>> Ah, so on the one hand this gives me second thoughts about my stance\n>> in[1], i.e. if we just allowed any command we wouldn't be forced to add\n>> these sorts of special-cases.\n>> Although, we could also allow any command, and just add smartness\n>> for\n>> ones we know about, e.g. \"grep\".\n>> But I do find doing this to be weirdly inconsistent, i.e. caring\n>> about\n>> the difference between these two:\n>> \t$ grep blah README.md; echo $?\n>> \t1\n>> \t$ grep blah README.mdx; echo $?\n>> \tgrep: README.mdx: No such file or directory\n>> \t2\n>\n> The intent was to catch bad options, not missing files (i.e. we don't\n> want test_todo to hide a failure from \"grep --invalid-option\"). We\n> could check the file exists and skip running grep if it does not\n> (hopefully the test wont be grepping multiple files in a single\n> command)\n\nIt returns the same exit code for missing files and bad options, so I\ndon't think this plan will work.\n\nI.e. I (in my initial series) wanted to have something where we declared\nwhat the behavior was right now, *and* what it should be.\n\nBut some of our tests are wishy-washing \"I wish this worked\", so:\n\n\ttest_todo git some-new-cmd && # should write \"unicorn\" to a new foo.txt\n\ttest_todo grep unicorn foo.txt\n\nWon't do what you expect?\n\n>> Is basically why I took the approach I did in my [2], i.e. to force us\n>> to positively assert *what* the bad behavior should be.\n>\n> That is what made the end result so hard to use though\n>\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>\n> is not exactly readable.\n\nYes, indeed:) Anyway, my just-sent\nhttps://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/\ngoes into that.\n\nI think a \"test_todo\" should either be a \"strictly declare stuff\", or a\n\"YOLO this\" where we just detect segfaults.\n\nBut per the above having it be some mix of the two is just confusing,\ni.e. to extend the example above:\n\n\ttest_todo git some-new-cmd &&\n\ttest_todo test_path_exists foo.txt &&\n\ttest_todo grep unicorn foo.txt\n\nWon't \"work\", because the \"test_path_exists\" isn't strict, but your\n\"grep\" is.\n\nSo I think whatever \"test_todo\" does it should be picking one or the\nother.\n"},{"id":"464328","messageId":"221006.86mta8r860.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T20:33:04Z","receivedAt":"2022-10-06T20:49:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 06 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 06/10/2022 16:36, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n>> \n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> test_todo() is built upon test_expect_failure() but accepts commands\n>>> starting with test_* in addition to git. As our test_* assertions use\n>>> BUG() to signal usage errors any such error will not be hidden by\n>>> test_todo().\n>> I think they will, unless I'm missing something. E.g. try out:\n>\n> It's talking about BUG() in test-lib.sh, not the C function. For example\n>\n> test_path_is_file () {\n> \ttest \"$#\" -ne 1 && BUG \"1 param\"\n> \tif ! test -f \"$1\"\n> \tthen\n> \t\techo \"File $1 doesn't exist\"\n> \t\tfalse\n> \tfi\n> }\n>\n> So a test containing \"test_todo test_path_is_file a b\" should fail as\n> BUG calls exit rather than returning non-zero (I should probably test \n> that in 0000-basic.sh)\n\nAh, anyway, I think getting that to behave correctly is *the* thing any\nless sucky test_expect_failure replacement needs to get right, i.e. to\nhandle BUG() (in C code), abort(), SANITIZE=undefined (and friends, all\nof whom abort()), segfaults etc.\n\n>> \tdiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n>> \tindex 80e76a4695e..1be895abba6 100755\n>> \t--- a/t/t0210-trace2-normal.sh\n>> \t+++ b/t/t0210-trace2-normal.sh\n>> \t@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '\n>> \t\n>> \t test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '\n>> \t \ttest_when_finished \"rm trace.normal actual expect\" &&\n>> \t-\ttest_expect_code 99 env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n>> \t+\ttest_todo env GIT_TRACE2=\"$(pwd)/trace.normal\" \\\n>> \t \t\ttest-tool trace2 008bug 2>err &&\n>> \t \tcat >expect <<-\\EOF &&\n>> \t \ta bug message\n>> I.e. in our tests you need to look out for exit code 99, not the\n>> usual\n>> abort().\n>> I have local patches to fix this, previously submitted as an RFC\n>> here:\n>> https://lore.kernel.org/git/RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com/\n>> I just rebased that & CI is currently running, I'll see how it does:\n>> https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2\n>> When I merged your patches here with that topic yours started doing\n>> the\n>> right thing in this case, i.e. a \"test_todo\" that would get a BUG()\"\n>> would be reported as a failure.\n>> \n>>> +\ttest_true () {\n>>> +\t\ttrue\n>>> +\t}\n>>> +\ttest_expect_success \"pretend we have fixed a test_todo breakage\" \\\n>>> +\t\t\"test_todo test_true\"\n>> \"Why the indirection\", until I realized that it's because you're\n>> working\n>> around the whitelist of commands that we have, i.e. we allow 'git' and\n>> 'test-tool', but not 'grep' or whatever.\n>> I'm of the opinion that we should just drop that limitation\n>> altogether,\n>> which is shown to be pretty silly in this case. I.e. at some point we\n>> listed \"test_*\" as \"this invokes a git binary\", but a lot of our test_*\n>> commands don't, including this one.\n>\n> test_expect_failure does not allow test_* are you thinking of test-tool?\n\nI'm talking about test_todo, and the reason for that \"test_true\" being\nneeded is because test_must_fail_acceptable is picky, but we could also\n(I just adjusted that one test):\n\t\n\tdiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n\tindex 52362ad3dd3..921e0401eb5 100755\n\t--- a/t/t0000-basic.sh\n\t+++ b/t/t0000-basic.sh\n\t@@ -143,11 +143,8 @@ test_expect_success 'subtest: a passing TODO test' '\n\t \n\t test_expect_success 'subtest: a failing test_todo' '\n\t \twrite_and_run_sub_test_lib_test failing-test-todo <<-\\EOF &&\n\t-\ttest_false () {\n\t-\t\tfalse\n\t-\t}\n\t \ttest_expect_success \"passing test\" \"true\"\n\t-\ttest_expect_success \"known todo\" \"test_todo test_false\"\n\t+\ttest_expect_success \"known todo\" \"test_todo false\"\n\t \ttest_done\n\t \tEOF\n\t \tcheck_sub_test_lib_test failing-test-todo <<-\\EOF\n\tdiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n\tindex 8978709b231..9300eaa2c62 100644\n\t--- a/t/test-lib-functions.sh\n\t+++ b/t/test-lib-functions.sh\n\t@@ -1034,6 +1034,11 @@ test_must_fail_acceptable () {\n\t \t\tdone\n\t \tfi\n\t \n\t+\tif test \"$name\" = \"todo\"\n\t+\tthen\n\t+\t\treturn 0\n\t+\tfi\n\t+\n\t \tcase \"$1\" in\n\t \tgit|__git*|test-tool|test_terminal)\n\t \t\treturn 0\n\t@@ -1050,10 +1055,6 @@ test_must_fail_acceptable () {\n\t \t\tfi\n\t \t\treturn 1\n\t \t\t;;\n\t-\ttest_*)\n\t-\t\ttest \"$name\" = \"todo\"\n\t-\t\treturn $?\n\t-\t\t;;\n\t \t*)\n\t \t\treturn 1\n\t \t\t;;\n\t\n\n>> So in general I think we should just allow any command in\n>> \"test_must_fail\" et al, and just catch in code review if someone uses\n>> \"test_must_fail grep\" as opposed to \"! grep\".\n>\n> That is not going to scale well\n\nWell, you're throwing the scaling out the window by whitelisting test_*\nin your 1/3 :)\n\nI don't think we really need it, but *the* reason it's there is to\ndistinguish things that run our own C code from things that don't, and a\nlot of test_* helpers don't.\n\nSo if you want to pursue making this correct per the current intent it\nshould really use the current whitelist to decide whether or not to pass\nthings through the \"should we change the exit code if it's a signal,\nsegfault etc?\" part.\n\nOtherwise you should just negate it or whatever, as the \"test_todo\" I\nshowed in\nhttps://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/\ndoes. I.e. we shouldn't be detecting an abort() in /bin/false and the\nlike.\n"},{"id":"464372","messageId":"8df2260c-7a35-5b50-7273-fbd9894a614c@dunelm.org.uk","threadId":"58573","inReplyTo":"221006.86v8owr986.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/3] [RFC] tests: add test_todo() for known failures","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-07T13:26:59Z","receivedAt":"2022-10-07T13:27:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 06/10/2022 20:28, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> test_todo() is intended as a fine grained alternative to\n>> test_expect_failure(). Rather than marking the whole test as failing\n>> test_todo() is used to mark individual failing commands within a test. This\n>> approach to writing failing tests allows us to detect unexpected failures\n>> that are hidden by test_expect_failure().\n>>\n>> This series attempts to keep most of the benefits test_expect_todo()\n>> previously proposed by Ævar[1] while being simpler to use.\n>>\n>> [1]\n>> https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n> \n> I like the interface you've got here much better than the one I\n> submitted in [1], so much that it's what I tried to write at first :)\n> \n> But as you noted in 1/3:\n> \n> \ttest_todo cannot be used in a subshell.\n> \n> Anyway, the core difference between the APIs we proposed for this is\n> that you'd do:\n> \n> \ttest_expect_success 'desc' 'test_todo false'\n> \n> Whereas I suggested:\n> \n> \ttest_expect_todo 'desc' '! false'\n> \n> Now, let's pick apart the differences:\n> \n>   1. With \"test_expect_todo\" we're declaring \"this is a TODO test\" for\n>      the test as a whole.\n >\n>   2. With your \"test_todo\" we're not doing that, instead we proceed as\n>      normal, and then we might note \"we had a TODO\" midway through, then\n>      at the end we'll spot that we had a TODO test (but this approach\n>      won't work with subshells).\n\nYes, this series avoids adding test_expect_todo and reuses \ntest_expect_success as Junio suggested [1]. By using a new toplevel form \nyour series was able to handle test_todo in a subshell because it did \nnot need any global state related to whether a test_todo had passed/failed.\n\nThe series here uses a variable to check if any test_todo statements \nwere present in a test that ran successfully and that does not work if \nthe test_todo happens in a subshell because the variable in the parent \nis not updated. First we need to consider whether there is any need for \nsupporting test_todo in a subshell. If there is then a possible \nalternative is to store the state in a file. That would add the cost of \n\"test -f\" to test_expect_success but our tests do so much i/o that a \nsingle extra stat should not be noticeable. In particular I believe a \nfile based approach can be implemented without adding any new processes \nto tests that do not use test_todo.\n\n>   3. Your \"test_todo\" is basically a \"let's let this pass\", whereas mine\n>      was a helper which exhaustively declared *what* the bad behavior\n>      was.\n> \n>      (Although some of yours seems to be midway between the two,\n>      i.e. https://lore.kernel.org/git/c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk/)\n\nThe only thing my series tries to assert is that a test_todo command is \nnot failing due to a usage error so that it can catch buggy tests. \nBeyond that it does not care what the reason for failure is.\n\n> I think the main critique you and Junio had of my series was to do with\n> #3, i.e. that it was a hassle to exhaustively declare what the behavior\n> is & should be, as you note in:\n\nThat was certainly my main objection, but Junio was not that keen on \nadding test_expect_todo [1]\n\n> https://lore.kernel.org/git/c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk/\n> \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> \n> That's fair enough, maybe that's not worth the effort. The reason I\n> initially hacked this up was because I'd noticed a behavior difference\n> in a command that was only revealed in a test_expect_failure block, but\n> because we didn't assert *what* the behavior was we didn't notice.\n> \n> My version (if fully used) would spot that, but that's because of how I\n> wrote the \"tes_todo\", it's orthagonal to #1 and #2 above.\n> \n> So I don't see why we wouldn't instead have a \"test_expect_todo\" and\n> just write the helper differently, or have a mode where it's less\n> strict, and (if we find it worthwhile) one where it's more strict.\n\nI think there is a question of whether we need a new toplevel \ntest_expect_todo - why would we add it if we can just reuse \ntest_expect_success? That way when a test failure is fixed all that \nneeds to be done is to remove the test_todo calls.\n\nBest Wishes\n\nPhillip\n\n[1] https://lore.kernel.org/git/xmqq8rt77zp7.fsf@gitster.g/\n\n\n> I rebased my\n> https://lore.kernel.org/git/patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com/\n> just now and applied the below on top, which seems to me to give you\n> pretty much the end result you want, the only difference is that my\n> version will also work in subshells (see the t2500 one):\n> \n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index de1ec89007d..fe47e503bd1 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -468,7 +468,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> @@ -502,11 +502,11 @@ 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 git -C tweak add folder1/a &&\n>   \tgit -C tweak sparse-checkout reapply 2>err &&\n> -\ttest_must_be_empty err &&\n> +\ttest_todo test_must_be_empty err &&\n>   \ttest_path_is_missing tweak/deep/deeper2/a &&\n> -\ttest_path_is_missing tweak/folder1/a &&\n> +\ttest_todo test_path_is_missing tweak/folder1/a &&\n>   \n>   \tgit -C tweak sparse-checkout disable\n>   '\n> diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh\n> index 5c0bf4d21fc..db7c72d38d8 100755\n> --- a/t/t2500-untracked-overwriting.sh\n> +++ b/t/t2500-untracked-overwriting.sh\n> @@ -167,7 +167,7 @@ test_expect_success 'git rebase fast forwarding and untracked files' '\n>   \t)\n>   '\n>   \n> -test_expect_failure 'git rebase --autostash and untracked files' '\n> +test_expect_todo 'git rebase --autostash and untracked files' '\n>   \ttest_setup_sequencing rebase_autostash_and_untracked &&\n>   \t(\n>   \t\tcd sequencing_rebase_autostash_and_untracked &&\n> @@ -176,7 +176,7 @@ test_expect_failure 'git rebase --autostash and untracked files' '\n>   \t\tmkdir filler &&\n>   \t\techo precious >filler/file &&\n>   \t\tcp filler/file expect &&\n> -\t\tgit rebase --autostash init &&\n> +\t\ttest_todo git rebase --autostash init &&\n>   \t\ttest_path_is_file filler/file\n>   \t)\n>   '\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index 3b0fa66c33d..b31b6b0f7a0 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -577,7 +577,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n>   \tgrep \"cherry picked from.*$picked\" msg\n>   '\n>   \n> -test_expect_failure '--signoff is automatically propagated to resolved conflict' '\n> +test_expect_todo '--signoff is automatically propagated to resolved conflict' '\n>   \tpristine_detach initial &&\n>   \ttest_expect_code 1 git cherry-pick --signoff base..anotherpick &&\n>   \techo \"c\" >foo &&\n> @@ -591,7 +591,7 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'\n>   \tgit cat-file commit HEAD~3 >initial_msg &&\n>   \t! grep \"Signed-off-by:\" initial_msg &&\n>   \tgrep \"Signed-off-by:\" unrelatedpick_msg &&\n> -\t! grep \"Signed-off-by:\" picked_msg &&\n> +\ttest_todo ! grep \"Signed-off-by:\" picked_msg &&\n>   \tgrep \"Signed-off-by:\" anotherpick_msg\n>   '\n>   \n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index e74a318ac33..6c7929f5557 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,10 @@ 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_todo test_must_fail git rm d/f &&\n> +\ttest_todo git rev-parse --verify :d/f &&\n>   \ttest -h d &&\n> -\ttest_path_is_file e/f\n> +\ttest_todo test_path_is_file e/f\n>   '\n>   \n>   test_expect_success 'setup for testing rm messages' '\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index ad5c0292794..a6a5a330180 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_todo '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> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index f342954de11..9d5706454a5 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1049,6 +1049,21 @@ test_must_fail_acceptable () {\n>   \tesac\n>   }\n>   \n> +test_todo () {\n> +\tlocal negate=-ne\n> +\tlocal cmp_op=-ne\n> +\tif test \"$1\" = \"!\"\n> +\tthen\n> +\t\tnegate=t &&\n> +\t\tcmp_op=-eq\n> +\t\tshift\n> +\tfi &&\n> +\t\"$@\" 2>&7\n> +\texit_code=$?\n> +\tsay \"test_todo: got $exit_code ${negate:+negated!} from $*\"\n> +\ttest \"$exit_code\" \"$cmp_op\" 0\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> \n> \n"},{"id":"464385","messageId":"xmqq8rlrh880.fsf@gitster.g","threadId":"58573","inReplyTo":"8df2260c-7a35-5b50-7273-fbd9894a614c@dunelm.org.uk","subject":"Re: [PATCH 0/3] [RFC] tests: add test_todo() for known failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-07T17:08:47Z","receivedAt":"2022-10-07T17:08:55Z","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 think there is a question of whether we need a new toplevel\n> test_expect_todo - why would we add it if we can just reuse\n> test_expect_success? That way when a test failure is fixed all that\n> needs to be done is to remove the test_todo calls.\n\nYup, that is one of the reasons why I favor test_todo inside the\nnormal test_expect_success.  A patch that fixes a breakage would\ncome with a change to the tests that flips test_expect_failure to\ntest_expect_success often had the step that were expected to fail\noutside the post context and did not make it immediately obvious\nwhat was fixed, and use of a more focused test_todo would make it\nmore clear.  Unless we gain a clear advantage by using a different\ntop-level (e.g. some of the limitations like \"not in a subshell\" can\nbe lifted?), I do not think we want to add one.\n\n\n"},{"id":"468647","messageId":"07d963f0-45f2-ed8e-ea08-bcea14386a4d@github.com","threadId":"58573","inReplyTo":"472d05111a38276192e30f454f42aa39df51d604.1665068476.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-12-06T22:37:07Z","receivedAt":"2022-12-06T22:37:13Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> test_todo() is intended as a fine grained replacement for\n> test_expect_failure(). Rather than marking the whole test as failing\n> test_todo() is used to mark individual failing commands within a\n> test. This approach to writing failing tests allows us to detect\n> unexpected failures that are hidden by test_expect_failure().\n\nI love this idea! I've nearly been burned a couple of times by the wrong\nline in a 'test_expect_failure' triggering the error (e.g., due to bad\nsyntax earlier in the test). The added specificity of 'test_todo' will help\nboth reviewers and people fixing the underlying issues demonstrated by\nexpected-failing tests.\n\n> \n> Failing commands are reported by the test harness in the same way as\n> test_expect_failure() so there is no change in output when migrating\n> from test_expect_failure() to test_todo(). If a command marked with\n> test_todo() succeeds then the test will fail. This is designed to make\n> it easier to see when a command starts succeeding in our CI compared\n> to using test_expect_failure() where it is easy to fix a failing test\n> case and not realize it.\n> \n> test_todo() is built upon test_expect_failure() but accepts commands\n> starting with test_* in addition to git. As our test_* assertions use\n> BUG() to signal usage errors any such error will not be hidden by\n> test_todo().\n\nShould this be so restrictive? I think 'test_todo' would need to handle any\narbitrary command (mostly because of custom functions like\n'ensure_not_expanded' in 't1092') to be an easy-to-use drop-in replacement\nfor 'test_expect_failure'. \n\nI see there's some related discussion in another subthread [1], but I don't\nnecessarily think removing restrictions (i.e. that the tested command must\nbe 'git', 'test_*', etc.) on 'test_todo' requires doing the same for\n'test_must_fail' et al. to be internally consistent. On one hand,\n'test_todo' could be interpreted as an assertion (like 'test_must_fail'),\nwhere we only want to assert on our code - hence the restrictions. From that\nperspective, it would make sense to ease restrictions uniformly on all of\nour assertion helpers. \n\nOn the other hand, I'm interpreting 'test_todo' as\n'test_expect_failure_on_line_N' - more of a \"post-test result interpreter\"\nthan an assertion helper. So because 'test_expect_failure' doesn't require\nthe failing line to come from a particular command, I don't think\n'test_todo' needs to either. That leaves assertion helpers like\n'test_must_fail' out of the scope of this change, avoiding any hairiness of\nallowing them to assert on arbitrary code.\n\nWhat do you think?\n\n[1] https://lore.kernel.org/git/221006.86mta8r860.gmgdl@evledraar.gmail.com/\n\n> \n> This commit coverts a few tests to show the intended use of\n> test_todo().  A limitation of test_todo() as it is currently\n> implemented is that it cannot be used in a subshell.\n> \n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n"},{"id":"468708","messageId":"221207.86cz8v9zsl.gmgdl@evledraar.gmail.com","threadId":"58573","inReplyTo":"07d963f0-45f2-ed8e-ea08-bcea14386a4d@github.com","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T12:08:25Z","receivedAt":"2022-12-07T12:14:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Victoria Dye wrote:\n\n> Phillip Wood via GitGitGadget wrote:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> \n>> test_todo() is intended as a fine grained replacement for\n>> test_expect_failure(). Rather than marking the whole test as failing\n>> test_todo() is used to mark individual failing commands within a\n>> test. This approach to writing failing tests allows us to detect\n>> unexpected failures that are hidden by test_expect_failure().\n>\n> I love this idea! I've nearly been burned a couple of times by the wrong\n> line in a 'test_expect_failure' triggering the error (e.g., due to bad\n> syntax earlier in the test). The added specificity of 'test_todo' will help\n> both reviewers and people fixing the underlying issues demonstrated by\n> expected-failing tests.\n>\n>> \n>> Failing commands are reported by the test harness in the same way as\n>> test_expect_failure() so there is no change in output when migrating\n>> from test_expect_failure() to test_todo(). If a command marked with\n>> test_todo() succeeds then the test will fail. This is designed to make\n>> it easier to see when a command starts succeeding in our CI compared\n>> to using test_expect_failure() where it is easy to fix a failing test\n>> case and not realize it.\n>> \n>> test_todo() is built upon test_expect_failure() but accepts commands\n>> starting with test_* in addition to git. As our test_* assertions use\n>> BUG() to signal usage errors any such error will not be hidden by\n>> test_todo().\n>\n> Should this be so restrictive? I think 'test_todo' would need to handle any\n> arbitrary command (mostly because of custom functions like\n> 'ensure_not_expanded' in 't1092') to be an easy-to-use drop-in replacement\n> for 'test_expect_failure'. \n>\n> I see there's some related discussion in another subthread [1], but I don't\n> necessarily think removing restrictions (i.e. that the tested command must\n> be 'git', 'test_*', etc.) on 'test_todo' requires doing the same for\n> 'test_must_fail' et al. to be internally consistent. On one hand,\n> 'test_todo' could be interpreted as an assertion (like 'test_must_fail'),\n> where we only want to assert on our code - hence the restrictions. From that\n> perspective, it would make sense to ease restrictions uniformly on all of\n> our assertion helpers. \n>\n> On the other hand, I'm interpreting 'test_todo' as\n> 'test_expect_failure_on_line_N' - more of a \"post-test result interpreter\"\n> than an assertion helper. So because 'test_expect_failure' doesn't require\n> the failing line to come from a particular command, I don't think\n> 'test_todo' needs to either. That leaves assertion helpers like\n> 'test_must_fail' out of the scope of this change, avoiding any hairiness of\n> allowing them to assert on arbitrary code.\n>\n> What do you think?\n\nAre you saying that for the \"test_todo\" we shouldn't care whether it\nexits with a \"normal\" non-zero or a segfault, abort() (e.g. BUG()) etc?\nThat's what the \"test_must_fail\" v.s. \"!\" is about.\n\nEven if we erased tat distinction I think such a thing would be a\nmarginal improvement on \"test_expect_failure\", as we'd at least mark\nwhat line fails, but like \"test_expect_failure\" we'd accept segfaults as\nfailures.\n\nbut as noted in the upthread discussions I think we should do better and\nstill check for segfaults etc. I think we have a couple of\n\"test_expect_failure\" now where we expect a segfault, but for the rest\nwe'd like to know if they start segfaulting.\n"},{"id":"468790","messageId":"1b5fc712-9659-1bd6-493c-197b003d21d6@dunelm.org.uk","threadId":"58573","inReplyTo":"07d963f0-45f2-ed8e-ea08-bcea14386a4d@github.com","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-08T15:06:36Z","receivedAt":"2022-12-08T15:06:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 06/12/2022 22:37, Victoria Dye wrote:\n> Phillip Wood via GitGitGadget wrote:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n >>\n>> Failing commands are reported by the test harness in the same way as\n>> test_expect_failure() so there is no change in output when migrating\n>> from test_expect_failure() to test_todo(). If a command marked with\n>> test_todo() succeeds then the test will fail. This is designed to make\n>> it easier to see when a command starts succeeding in our CI compared\n>> to using test_expect_failure() where it is easy to fix a failing test\n>> case and not realize it.\n>>\n>> test_todo() is built upon test_expect_failure() but accepts commands\n>> starting with test_* in addition to git. As our test_* assertions use\n>> BUG() to signal usage errors any such error will not be hidden by\n>> test_todo().\n> \n> Should this be so restrictive? I think 'test_todo' would need to handle any\n> arbitrary command (mostly because of custom functions like\n> 'ensure_not_expanded' in 't1092') to be an easy-to-use drop-in replacement\n> for 'test_expect_failure'.\n> \n> I see there's some related discussion in another subthread [1], but I don't\n> necessarily think removing restrictions (i.e. that the tested command must\n> be 'git', 'test_*', etc.) on 'test_todo' requires doing the same for\n> 'test_must_fail' et al. to be internally consistent. On one hand,\n> 'test_todo' could be interpreted as an assertion (like 'test_must_fail'),\n> where we only want to assert on our code - hence the restrictions. From that\n> perspective, it would make sense to ease restrictions uniformly on all of\n> our assertion helpers.\n> \n> On the other hand, I'm interpreting 'test_todo' as\n> 'test_expect_failure_on_line_N' - more of a \"post-test result interpreter\"\n> than an assertion helper. So because 'test_expect_failure' doesn't require\n> the failing line to come from a particular command, I don't think\n> 'test_todo' needs to either. That leaves assertion helpers like\n> 'test_must_fail' out of the scope of this change, avoiding any hairiness of\n> allowing them to assert on arbitrary code.\n> \n> What do you think?\n\nI don't think we need to remove the restrictions on 'test_must_fail', \nthey seem to be there for a good reason and I'm not aware of anyone \ncomplaining about being inconvenienced by them. I think of 'test_todo' \nand 'test_must_fail' as being distinct, 'test_todo' only reuses the \nimplementation of 'test_must_fail' for convenience rather than any other \ndeep reason.\n\nI added the restrictions to 'test_todo' to try and stop it being misused \nbut I'm happy to relax them if needed. I'm keen that test_todo is able \nto distinguish between an expected failure and a failure due to the \nwrapped command being misused e.g. 'test_todo grep --invalid-option' \nshould report an error. Restricting the commands makes it easier to \nguarantee that but we can always just add checks for other commands as \nwe use them. In a way the existing restrictions are kind of pointless \nbecause test authors can always name their helper functions test_... to \nget round them.\n\nI think you've convinced be to remove the restrictions on what can be \nwrapped by 'test_todo' when I re-roll.\n\nThanks for your thoughtful comments\n\nPhillip\n\n> [1] https://lore.kernel.org/git/221006.86mta8r860.gmgdl@evledraar.gmail.com/\n> \n>>\n>> This commit coverts a few tests to show the intended use of\n>> test_todo().  A limitation of test_todo() as it is currently\n>> implemented is that it cannot be used in a subshell.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n"},{"id":"468807","messageId":"xmqqedt92xjk.fsf@gitster.g","threadId":"58573","inReplyTo":"1b5fc712-9659-1bd6-493c-197b003d21d6@dunelm.org.uk","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-09T01:09:03Z","receivedAt":"2022-12-09T01:09:18Z","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 added the restrictions to 'test_todo' to try and stop it being\n> misused but I'm happy to relax them if needed. I'm keen that test_todo\n> is able to distinguish between an expected failure and a failure due\n> to the wrapped command being misused e.g. 'test_todo grep\n> --invalid-option' should report an error.\n\nHmm, but it is not useful if the failure is from \"you cannot use the\nsystem command grep with test_todo\", as the (implicitly) encouraged\n\"fix\" the developer who wrote the test would pick would be to use\n\"! grep --invalid-option\" which would still fail for a wrong reason.\n\nIf a \"git\" command is expected to run to a completion but is\ncurrently broken and produces a wrong output, it would be very\nuseful to be able to write\n\n\tgit command --args >actual &&\n\ttest_todo grep -e \"$expected_token\" actual\n\nto say \"when 'git command' is fixed, the output should contains\nthis, but we know it currently is broken\". \n\n> I think you've convinced be to remove the restrictions on what can be\n> wrapped by 'test_todo' when I re-roll.\n>\n> Thanks for your thoughtful comments\n\nThanks.\n"},{"id":"468824","messageId":"f873ad40-f664-bdc4-a6bb-b4e4c113aff4@dunelm.org.uk","threadId":"58573","inReplyTo":"xmqqedt92xjk.fsf@gitster.g","subject":"Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-09T09:04:25Z","receivedAt":"2022-12-09T09:04:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 09/12/2022 01:09, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> I added the restrictions to 'test_todo' to try and stop it being\n>> misused but I'm happy to relax them if needed. I'm keen that test_todo\n>> is able to distinguish between an expected failure and a failure due\n>> to the wrapped command being misused e.g. 'test_todo grep\n>> --invalid-option' should report an error.\n> \n> Hmm, but it is not useful if the failure is from \"you cannot use the\n> system command grep with test_todo\", as the (implicitly) encouraged\n> \"fix\" the developer who wrote the test would pick would be to use\n> \"! grep --invalid-option\" which would still fail for a wrong reason.\n> \n> If a \"git\" command is expected to run to a completion but is\n> currently broken and produces a wrong output, it would be very\n> useful to be able to write\n> \n> \tgit command --args >actual &&\n> \ttest_todo grep -e \"$expected_token\" actual\n> \n> to say \"when 'git command' is fixed, the output should contains\n> this, but we know it currently is broken\".\n\nYou can do that now, the next patch adds support for \"test_todo [!] \ngrep\" but you currently cannot pass arbitrary commands/helper functions.\n\nBest Wishes\n\nPhillip\n\n>> I think you've convinced be to remove the restrictions on what can be\n>> wrapped by 'test_todo' when I re-roll.\n>>\n>> Thanks for your thoughtful comments\n> \n> Thanks.\n"}]}