Hi Ævar
On 19/03/2022 11:11, Ævar Arnfjörð Bjarmason wrote:
On Sat, Mar 19 2022, Junio C Hamano wrote:
[...]
I don't think we can categorically replace all of the
"test_expect_failure" cases, because in some of those it's too much of a
hassle to assert the exact current behavior, or we don't really care.
But I think for most cases instead of a:
test_ki ! grep unwanted output
It makes more sense to do (as that helper does):
test_todo grep --want yay --expect unwanted -- output
My take is the complete opposite. We can and should start small,
and how exactly the current implementation happens to be broken does
not matter most of the time.
Well, the tip of this series leaves ~20 uses of test_expect_todo v.s. a
remaining ~100 uses of test_expect_failure, so it is a small start. I
converted those things I thought made the most sense.
But I do think you want to test at least a fuzzy "how exactly" most of
the time. The reason I worked on this was because while authoring the
series you merged in ea05fd5fbf7 (Merge branch
'ab/keep-git-exit-codes-in-tests', 2022-03-16) I found that we had
various test_expect_failure that failed in ways very different than what
we'd expect.
Or, saying that something exits non-zero and we'd like to fix it isn't
the same as saying that we'd like to e.g. exclude it from SANITIZE=leak
or SANITIZE=address testing. I.e. it still shouldn't leak, double-free()
or run into a BUG(), and if it does we'd like to know most of the time.
I think having test_todo based on test_must_fail as Junio
suggested avoids this as it means the test will still fail on SIGSEV
or SIGABRT (if we don't already do so we can make LSAN exit with the
same code as vargrind which test_must_fail checks for). I had a look
at some of the conversions with your test_todo --want/--expect/--reset
and found the result really hard to follow. Junio's suggestions chimed
with some things I've been thinking about so I had a go at
implementing it and doing some sample conversions (see below). Marking
the individual commands that should fail is a big step forward and the
failing commands are checked to make sure they don't segfault etc.
Best Wishes
Phillip
---- >8 -----
t/t0000-basic.sh | 9 +++++--
t/t3401-rebase-and-am-rename.sh | 12 +++++-----
t/t3424-rebase-empty.sh | 6 ++---
t/t4014-format-patch.sh | 20 ++++++++--------
t/test-lib-functions.sh | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------
5 files changed, 116 insertions(+), 61 deletions(-)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b007f0efef..53217d4cd5 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -103,16 +103,21 @@ test_expect_success 'subtest: 2/3 tests passing' '
test_expect_success 'subtest: a failing TODO test' '
write_and_run_sub_test_lib_test failing-todo <<-\EOF &&
+ test_false () {
+ false
+ }
test_expect_success "passing test" "true"
test_expect_failure "pretend we have a known breakage" "false"
+ test_expect_success "known todo" "test_todo test_false"
test_done
EOF
check_sub_test_lib_test failing-todo <<-\EOF
> ok 1 - passing test
> not ok 2 - pretend we have a known breakage # TODO known breakage
- > # still have 1 known breakage(s)
+ > not ok 3 - known todo # TODO known breakage
+ > # still have 2 known breakage(s)
> # passed all remaining 1 test(s)
- > 1..2
+ > 1..3
EOF
'
diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
index f18bae9450..cc5da9f5af 100755
--- a/t/t3401-rebase-and-am-rename.sh
+++ b/t/t3401-rebase-and-am-rename.sh
@@ -52,7 +52,7 @@ test_expect_success 'rebase --interactive: directory rename detected' '
)
'
-test_expect_failure 'rebase --apply: directory rename detected' '
+test_expect_success 'rebase --apply: directory rename detected' '
(
cd dir-rename &&
@@ -63,8 +63,8 @@ test_expect_failure 'rebase --apply: directory rename detected' '
git ls-files -s >out &&
test_line_count = 5 out &&
- test_path_is_file y/d &&
- test_path_is_missing x/d
+ test_todo test_path_is_file y/d &&
+ test_todo test_path_is_missing x/d
)
'
@@ -84,7 +84,7 @@ test_expect_success 'rebase --merge: directory rename detected' '
)
'
-test_expect_failure 'am: directory rename detected' '
+test_expect_success 'am: directory rename detected' '
(
cd dir-rename &&
@@ -97,8 +97,8 @@ test_expect_failure 'am: directory rename detected' '
git ls-files -s >out &&
test_line_count = 5 out &&
- test_path_is_file y/d &&
- test_path_is_missing x/d
+ test_todo test_path_is_file y/d &&
+ test_todo test_path_is_missing x/d
)
'
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index 5e1045a0af..b7cae26075 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -34,15 +34,15 @@ test_expect_success 'setup test repository' '
git commit -m "Five letters ought to be enough for anybody"
'
-test_expect_failure 'rebase (apply-backend)' '
- test_when_finished "git rebase --abort" &&
+test_expect_success 'rebase (apply-backend)' '
+ test_when_finished "test_might_fail git rebase --abort" &&
git checkout -B testing localmods &&
# rebase (--apply) should not drop commits that start empty
git rebase --apply upstream &&
test_write_lines D C B A >expect &&
git log --format=%s >actual &&
- test_cmp expect actual
+ test_todo test_cmp expect actual
'
test_expect_success 'rebase --merge --empty=drop' '
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7dc5a5c736..bb0fcef40e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -165,12 +165,12 @@ test_expect_success 'additional command line cc (ascii)' '
grep "^ *S E Cipient <scipient@xxxxxxxxxxx>\$" hdrs5
'
-test_expect_failure 'additional command line cc (rfc822)' '
+test_expect_success 'additional command line cc (rfc822)' '
git config --replace-all format.headers "Cc: R E Cipient <rcipient@xxxxxxxxxxx>" &&
git format-patch --cc="S. E. Cipient <scipient@xxxxxxxxxxx>" --stdout main..side >patch5 &&
sed -e "/^\$/q" patch5 >hdrs5 &&
grep "^Cc: R E Cipient <rcipient@xxxxxxxxxxx>,\$" hdrs5 &&
- grep "^ *\"S. E. Cipient\" <scipient@xxxxxxxxxxx>\$" hdrs5
+ test_todo grep "^ *\"S. E. Cipient\" <scipient@xxxxxxxxxxx>\$" hdrs5
'
test_expect_success 'command line headers' '
@@ -195,16 +195,16 @@ test_expect_success 'command line To: header (ascii)' '
grep "^To: R E Cipient <rcipient@xxxxxxxxxxx>\$" hdrs8
'
-test_expect_failure 'command line To: header (rfc822)' '
+test_expect_success 'command line To: header (rfc822)' '
git format-patch --to="R. E. Cipient <rcipient@xxxxxxxxxxx>" --stdout main..side >patch8 &&
sed -e "/^\$/q" patch8 >hdrs8 &&
- grep "^To: \"R. E. Cipient\" <rcipient@xxxxxxxxxxx>\$" hdrs8
+ test_todo grep "^To: \"R. E. Cipient\" <rcipient@xxxxxxxxxxx>\$" hdrs8
'
-test_expect_failure 'command line To: header (rfc2047)' '
+test_expect_success 'command line To: header (rfc2047)' '
git format-patch --to="R Ä Cipient <rcipient@xxxxxxxxxxx>" --stdout main..side >patch8 &&
sed -e "/^\$/q" patch8 >hdrs8 &&
- grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@xxxxxxxxxxx>\$" hdrs8
+ test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@xxxxxxxxxxx>\$" hdrs8
'
test_expect_success 'configuration To: header (ascii)' '
@@ -214,18 +214,18 @@ test_expect_success 'configuration To: header (ascii)' '
grep "^To: R E Cipient <rcipient@xxxxxxxxxxx>\$" hdrs9
'
-test_expect_failure 'configuration To: header (rfc822)' '
+test_expect_success 'configuration To: header (rfc822)' '
git config format.to "R. E. Cipient <rcipient@xxxxxxxxxxx>" &&
git format-patch --stdout main..side >patch9 &&
sed -e "/^\$/q" patch9 >hdrs9 &&
- grep "^To: \"R. E. Cipient\" <rcipient@xxxxxxxxxxx>\$" hdrs9
+ test_todo grep "^To: \"R. E. Cipient\" <rcipient@xxxxxxxxxxx>\$" hdrs9
'
-test_expect_failure 'configuration To: header (rfc2047)' '
+test_expect_success 'configuration To: header (rfc2047)' '
git config format.to "R Ä Cipient <rcipient@xxxxxxxxxxx>" &&
git format-patch --stdout main..side >patch9 &&
sed -e "/^\$/q" patch9 >hdrs9 &&
- grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@xxxxxxxxxxx>\$" hdrs9
+ test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@xxxxxxxxxxx>\$" hdrs9
'
# check_patch <patch>: Verify that <patch> looks like a half-sane
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d6..deb74a22f3 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -739,6 +739,7 @@ test_expect_failure () {
}
test_expect_success () {
+ test_todo_=
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
@@ -750,7 +751,12 @@ test_expect_success () {
say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $2"
if test_run_ "$2"
then
- test_ok_ "$1"
+ if test -n "$test_todo_"
+ then
+ test_known_broken_failure_ "$1"
+ else
+ test_ok_ "$1"
+ fi
else
test_failure_ "$@"
fi
@@ -1011,8 +1017,20 @@ list_contains () {
# Returns success if the arguments indicate that a command should be
# accepted by test_must_fail(). If the command is run with env, the env
# and its corresponding variable settings will be stripped before we
+# we test the command being run.
+#
+# For test_todo we allow a wider range of commands to and if the
+# command is run with verbose then verbose will be stripped before we
# test the command being run.
+
test_must_fail_acceptable () {
+ local name
+ name="$1"
+ shift
+ if test "$name" = "todo" && test "$1" = "verbose"
+ then
+ shift
+ fi
if test "$1" = "env"
then
shift
@@ -1033,44 +1051,22 @@ test_must_fail_acceptable () {
git|__git*|test-tool|test_terminal)
return 0
;;
+ grep|test|test_*)
+ if test "$name" = "todo"
+ then
+ return 0
+ fi
+ return 1
+ ;;
*)
return 1
;;
esac
}
-# This is not among top-level (test_expect_success | test_expect_failure)
-# but is a prefix that can be used in the test script, like:
-#
-# test_expect_success 'complain and die' '
-# do something &&
-# do something else &&
-# test_must_fail git checkout ../outerspace
-# '
-#
-# Writing this as "! git checkout ../outerspace" is wrong, because
-# the failure could be due to a segv. We want a controlled failure.
-#
-# Accepts the following options:
-#
-# ok=<signal-name>[,<...>]:
-# Don't treat an exit caused by the given signal as error.
-# Multiple signals can be specified as a comma separated list.
-# Currently recognized signal names are: sigpipe, success.
-# (Don't use 'success', use 'test_might_fail' instead.)
-#
-# Do not use this to run anything but "git" and other specific testable
-# commands (see test_must_fail_acceptable()). We are not in the
-# business of vetting system supplied commands -- in other words, this
-# is wrong:
-#
-# test_must_fail grep pattern output
-#
-# Instead use '!':
-#
-# ! grep pattern output
-
-test_must_fail () {
+test_must_fail_helper () {
+ test_must_fail_name_="$1"
+ shift
case "$1" in
ok=*)
_test_ok=${1#ok=}
@@ -1080,36 +1076,90 @@ test_must_fail () {
_test_ok=
;;
esac
- if ! test_must_fail_acceptable "$@"
+ if ! test_must_fail_acceptable $test_must_fail_name_ "$@"
then
- echo >&7 "test_must_fail: only 'git' is allowed: $*"
+ echo >&7 "test_$test_must_fail_name_: only 'git' is allowed: $*"
return 1
fi
"$@" 2>&7
exit_code=$?
if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
then
- echo >&4 "test_must_fail: command succeeded: $*"
+ echo >&4 "test_$test_must_fail_name_: command succeeded: $*"
return 1
elif test_match_signal 13 $exit_code && list_contains "$_test_ok" sigpipe
then
return 0
elif test $exit_code -gt 129 && test $exit_code -le 192
then
- echo >&4 "test_must_fail: died by signal $(($exit_code - 128)): $*"
+ echo >&4 "test_$test_must_fail_name_: died by signal $(($exit_code - 128)): $*"
return 1
elif test $exit_code -eq 127
then
- echo >&4 "test_must_fail: command not found: $*"
+ echo >&4 "test_$test_must_fail_name_: command not found: $*"
return 1
elif test $exit_code -eq 126
then
- echo >&4 "test_must_fail: valgrind error: $*"
+ echo >&4 "test_$test_must_fail_name_: valgrind error: $*"
return 1
fi
return 0
} 7>&2 2>&4
+# This is used to mark commands that should succeed but do not due to
+# a known issue. Marking the individual commands that fail rather than
+# using test_expect_failure allows us to detect any unexpected
+# failures. As with test_must_fail if the command is killed by a
+# signal the test will fail. If the command unexpectedly succeeds then
+# the test fails. For example:
+#
+# test_expect_success 'test a known failure' '
+# git foo 2>err &&
+# test_todo test_must_be_empty err
+# '
+#
+# This test will fail if "git foo" fails or err is unexpectedly empty
+
+test_todo () {
+ test_todo_=todo
+ test_must_fail_helper todo "$@" 2>&7
+} 7>&2 2>&4
+
+# This is not among top-level (test_expect_success | test_expect_failure)
+# but is a prefix that can be used in the test script, like:
+#
+# test_expect_success 'complain and die' '
+# do something &&
+# do something else &&
+# test_must_fail git checkout ../outerspace
+# '
+#
+# Writing this as "! git checkout ../outerspace" is wrong, because
+# the failure could be due to a segv. We want a controlled failure.
+#
+# Accepts the following options:
+#
+# ok=<signal-name>[,<...>]:
+# Don't treat an exit caused by the given signal as error.
+# Multiple signals can be specified as a comma separated list.
+# Currently recognized signal names are: sigpipe, success.
+# (Don't use 'success', use 'test_might_fail' instead.)
+#
+# Do not use this to run anything but "git" and other specific testable
+# commands (see test_must_fail_acceptable()). We are not in the
+# business of vetting system supplied commands -- in other words, this
+# is wrong:
+#
+# test_must_fail grep pattern output
+#
+# Instead use '!':
+#
+# ! grep pattern output
+
+test_must_fail () {
+ test_must_fail_helper must_fail "$@" 2>&7
+} 7>&2 2>&4
+
# Similar to test_must_fail, but tolerates success, too. This is
# meant to be used in contexts like:
#
@@ -1124,7 +1174,7 @@ test_must_fail () {
# Accepts the same options as test_must_fail.
test_might_fail () {
- test_must_fail ok=success "$@" 2>&7
+ test_must_fail_helper might_fail ok=success "$@" 2>&7
} 7>&2 2>&4
# Similar to test_must_fail and test_might_fail, but check that a
