git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Cc: git@vger.kernel.org, John Cai <johncai86@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"
Date: Sun, 20 Mar 2022 15:13:36 +0000	[thread overview]
Message-ID: <db558292-2783-3270-4824-43757822a389@gmail.com> (raw)
In-Reply-To: <220319.86ilsadw69.gmgdl@evledraar.gmail.com>

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@example.com>\$" 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@example.com>" &&
  	git format-patch --cc="S. E. Cipient <scipient@example.com>" --stdout main..side >patch5 &&
  	sed -e "/^\$/q" patch5 >hdrs5 &&
  	grep "^Cc: R E Cipient <rcipient@example.com>,\$" hdrs5 &&
-	grep "^ *\"S. E. Cipient\" <scipient@example.com>\$" hdrs5
+	test_todo grep "^ *\"S. E. Cipient\" <scipient@example.com>\$" 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@example.com>\$" 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@example.com>" --stdout main..side >patch8 &&
  	sed -e "/^\$/q" patch8 >hdrs8 &&
-	grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs8
+	test_todo grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs8
  '
  
-test_expect_failure 'command line To: header (rfc2047)' '
+test_expect_success 'command line To: header (rfc2047)' '
  	git format-patch --to="R Ä Cipient <rcipient@example.com>" --stdout main..side >patch8 &&
  	sed -e "/^\$/q" patch8 >hdrs8 &&
-	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs8
+	test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" 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@example.com>\$" hdrs9
  '
  
-test_expect_failure 'configuration To: header (rfc822)' '
+test_expect_success 'configuration To: header (rfc822)' '
  	git config format.to "R. E. Cipient <rcipient@example.com>" &&
  	git format-patch --stdout main..side >patch9 &&
  	sed -e "/^\$/q" patch9 >hdrs9 &&
-	grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs9
+	test_todo grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs9
  '
  
-test_expect_failure 'configuration To: header (rfc2047)' '
+test_expect_success 'configuration To: header (rfc2047)' '
  	git config format.to "R Ä Cipient <rcipient@example.com>" &&
  	git format-patch --stdout main..side >patch9 &&
  	sed -e "/^\$/q" patch9 >hdrs9 &&
-	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs9
+	test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" 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

  reply	other threads:[~2022-03-20 15:13 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-18  0:33 [PATCH 0/7] test-lib-functions: a better "test_expect_failure" Ævar Arnfjörð Bjarmason
2022-03-18  0:33 ` [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure" Ævar Arnfjörð Bjarmason
2022-03-18 18:59   ` Junio C Hamano
2022-03-18 20:50     ` Junio C Hamano
2022-03-18 23:07       ` Ævar Arnfjörð Bjarmason
2022-03-19  7:12         ` Junio C Hamano
2022-03-19 11:11           ` Ævar Arnfjörð Bjarmason
2022-03-20 15:13             ` Phillip Wood [this message]
2022-03-20 18:07               ` Junio C Hamano
2022-03-22 14:43                 ` Derrick Stolee
2022-03-23 22:13                   ` Junio C Hamano
2022-03-24 11:24                     ` Phillip Wood
2022-03-18  0:33 ` [PATCH 2/7] test-lib-functions: add and use a "test_todo" helper Ævar Arnfjörð Bjarmason
2022-03-18  0:33 ` [PATCH 3/7] tests: allow test_* in "test_must_fail_acceptable" for "test_todo" Ævar Arnfjörð Bjarmason
2022-03-18  0:33 ` [PATCH 4/7] test-lib-functions: add and use a "todo_test_cmp" helper Ævar Arnfjörð Bjarmason
2022-03-18  0:34 ` [PATCH 5/7] test-lib-functions: add and use a "todo_test_path" helper Ævar Arnfjörð Bjarmason
2022-03-18  0:34 ` [PATCH 6/7] test-lib-functions: make test_todo support a --reset Ævar Arnfjörð Bjarmason
2022-03-18  0:34 ` [PATCH 7/7] sparse tests: convert a TODO test to use "test_expect_todo" Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=db558292-2783-3270-4824-43757822a389@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johncai86@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).