tests: add test_todo() for known failures

From: Phillip Wood <[email protected]>

test_todo() is intended as a fine grained replacement for test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

Failing commands are reported by the test harness in the same way as test_expect_failure() so there is no change in output when migrating from test_expect_failure() to test_todo(). If a command marked with test_todo() succeeds then the test will fail. This is designed to make it easier to see when a command starts succeeding in our CI compared to using test_expect_failure() where it is easy to fix a failing test case and not realize it.

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

t/test-lib-functions.sh +69 -37

+test_must_fail_helper () {
+	test_must_fail_name_="$1"
+	shift
+	case "$1" in
+	ok=*)
+		_test_ok=${1#ok=}
+		shift
+		;;
+	*)
+		_test_ok=
+		;;
+	esac
+	if ! test_must_fail_acceptable $test_must_fail_name_ "$@"
+	then
+		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_$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_$test_must_fail_name_: died by signal $(($exit_code - 128)): $*"
+		return 1
+	elif test $exit_code -eq 127
+	then
+		echo >&4 "test_$test_must_fail_name_: command not found: $*"
+		return 1
+	elif test $exit_code -eq 126
+	then
+		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 will also fail. 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 can be used with "git" or any of the "test_*" assertions
+# such as test_cmp().
+
+test_todo () {
+	if test "$test_todo_" = "test_expect_failure"
+	then
+		BUG "test_todo_ cannot be used inside test_expect_failure"
+	fi
+	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:
 #

@@ -1061,43 +1160,7 @@ test_must_fail_acceptable () {
 #    ! grep pattern output
 
 test_must_fail () {
-	case "$1" in
-	ok=*)
-		_test_ok=${1#ok=}
-		shift
-		;;
-	*)
-		_test_ok=
-		;;
-	esac
-	if ! test_must_fail_acceptable "$@"
-	then
-		echo >&7 "test_must_fail: 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: $*"
-		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)): $*"
-		return 1
-	elif test $exit_code -eq 127
-	then
-		echo >&4 "test_must_fail: command not found: $*"
-		return 1
-	elif test $exit_code -eq 126
-	then
-		echo >&4 "test_must_fail: valgrind error: $*"
-		return 1
-	fi
-	return 0
+	test_must_fail_helper must_fail "$@" 2>&7
 } 7>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is

From: Phillip Wood <[email protected]>

Many failing tests use grep, this commit converts a sample to use test_todo(). As POSIX specifies that a return code greater than one indicates an error rather than a failing match we take care not the hide that.

From: Phillip Wood <[email protected]>

For simplicity test_todo() allows verbose to precede any valid command. As POSIX specifies that a return code greater than one is an error rather than a failed test we take care not to hide that.

I'm in two minds about this patch. Generally it is better to use one of our test helpers such as test_cmp() rather than calling test directly. There are so few instances of test being used within test_expect_failure() (the conversions here are not exhaustive but there are not many more) that it would probably be better to convert the tests by using the appropriate helper rather than supporting calling test as the command to test_todo().

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

I think they will, unless I'm missing something. E.g. try out:

	diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
	index 80e76a4695e..1be895abba6 100755
	--- a/t/t0210-trace2-normal.sh
	+++ b/t/t0210-trace2-normal.sh
	@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '
	
	 test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
	 	test_when_finished "rm trace.normal actual expect" &&
	-	test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
	+	test_todo env GIT_TRACE2="$(pwd)/trace.normal" \
	 		test-tool trace2 008bug 2>err &&
	 	cat >expect <<-\EOF &&
	 	a bug message

I.e. in our tests you need to look out for exit code 99, not the usual abort().

I have local patches to fix this, previously submitted as an RFC here:
https://lore.kernel.org/git/[email protected]/

I just rebased that & CI is currently running, I'll see how it does:
https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2

When I merged your patches here with that topic yours started doing the right thing in this case, i.e. a "test_todo" that would get a BUG()" would be reported as a failure.

t/t0000-basic.sh test_expect_success 'subtest: a passing TODO test' ' · line 168

Quoted 5 lines

t/t0000-basic.sh +5 -0

+	test_true () {
+		true
+	}
+	test_expect_success "pretend we have fixed a test_todo breakage" \
+		"test_todo test_true"

"Why the indirection", until I realized that it's because you're working around the whitelist of commands that we have, i.e. we allow 'git' and 'test-tool', but not 'grep' or whatever.

I'm of the opinion that we should just drop that limitation altogether, which is shown to be pretty silly in this case. I.e. at some point we listed "test_*" as "this invokes a git binary", but a lot of our test_* commands don't, including this one.

So in general I think we should just allow any command in "test_must_fail" et al, and just catch in code review if someone uses "test_must_fail grep" as opposed to "! grep".

But for the particular case of "test_todo" doing so seems like pointless work, if we think we're going to miss this sort of thing in review in general, then surely that's not a concern if we're going to very prominently mark tests as TODO tests, given how the test of the output shows them?

[...]

t/test-lib-functions.sh test_must_fail () { · line 1183

Quoted 6 lines

  test_might_fail () {

t/test-lib-functions.sh +1 -1

-	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

I remember finding it annoying that "test_might_fail" is misreported from test_must_fail_acceptable as being called "test_must_fail", so this refactoring is very welcome.

But can you please split this into its own commit? I.e. that improvement can stand on its own, i.e. just a change that has "test_must_fail" and "test_might_fail" reporting their correct name.

Then this commit can follow, and then you'll just need to add (for this part) >

	test_must_fail_helper todo "$@" 2>&7

And it'll make the resulting diff much smaller & easier to follow.

t/test-lib-functions.sh test_must_fail_acceptable () { · line 1077

Quoted 1 line

t/test-lib-functions.sh +1 -0

+		echo >&7 "test_$test_must_fail_name_: only 'git' is allowed: $*"

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Many failing tests use grep, this commit converts a sample to use test_todo(). As POSIX specifies that a return code greater than one indicates an error rather than a failing match we take care not the hide that.

Ah, so on the one hand this gives me second thoughts about my stance in[1], i.e. if we just allowed any command we wouldn't be forced to add these sorts of special-cases.

Although, we could also allow any command, and just add smartness for ones we know about, e.g. "grep".

But I do find doing this to be weirdly inconsistent, i.e. caring about the difference between these two:

	$ grep blah README.md; echo $?
	1
	$ grep blah README.mdx; echo $?
	grep: README.mdx: No such file or directory
	2

Is basically why I took the approach I did in my [2], i.e. to force us to positively assert *what* the bad behavior should be.

Which is why I ended up doing my verison of this sort of thing as [3], i.e. you'd need to assert what specific exit code you expected, or equivalent.

But at this point in the series:

	diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
	index fa7831c0674..086eaf91351 100755
	--- a/t/t3600-rm.sh
	+++ b/t/t3600-rm.sh
	@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '
	 	test_todo test_must_fail git rm d/f &&
	 	test_todo git rev-parse --verify :d/f &&
	 	test -h d &&
	-	test_todo test_path_is_file e/f
	+	test_todo test_path_is_file blah
	 '
	
	 test_expect_success 'setup for testing rm messages' '

So, for our own test_path_* helpers we're not going to care at all, and any failure will do (including a missing file), but we will care for grep?

I'm obviously more on the "let's care" side, I just find it odd that you've picked this halfway point here, but not for other things you're wrapping.

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
3. https://lore.kernel.org/git/[email protected]/

test_todo() is intended as a fine grained alternative to test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

This series attempts to keep most of the benefits test_expect_todo() previously proposed by Ævar[1] while being simpler to use.

test_todo() is intended as a fine grained alternative to test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

This series attempts to keep most of the benefits test_expect_todo() previously proposed by Ævar[1] while being simpler to use.

[1]
https://lore.kernel.org/git/[email protected]/

Quoted 17 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Many failing tests use grep, this commit converts a sample to use test_todo(). As POSIX specifies that a return code greater than one indicates an error rather than a failing match we take care not the hide that.

Ah, so on the one hand this gives me second thoughts about my stance in[1], i.e. if we just allowed any command we wouldn't be forced to add these sorts of special-cases.
Although, we could also allow any command, and just add smartness for
ones we know about, e.g. "grep".
But I do find doing this to be weirdly inconsistent, i.e. caring about
the difference between these two:

Quoted 5 lines

	$ grep blah README.md; echo $?
	1
	$ grep blah README.mdx; echo $?
	grep: README.mdx: No such file or directory
	2

The intent was to catch bad options, not missing files (i.e. we don't want test_todo to hide a failure from "grep --invalid-option"). We could check the file exists and skip running grep if it does not (hopefully the test wont be grepping multiple files in a single command)

Quoted 2 lines

Is basically why I took the approach I did in my [2], i.e. to force us to positively assert *what* the bad behavior should be.

That is what made the end result so hard to use though

	test_todo \
		--want "test_must_fail git" \
		--reset "git reset --hard" \
		--expect git \
		-- \
		rm d/f &&

is not exactly readable.

Hi Ævar

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

I think they will, unless I'm missing something. E.g. try out:

It's talking about BUG() in test-lib.sh, not the C function. For example

test_path_is_file () {
	test "$#" -ne 1 && BUG "1 param"
	if ! test -f "$1"
	then
		echo "File $1 doesn't exist"
		false
	fi
}

So a test containing "test_todo test_path_is_file a b" should fail as BUG calls exit rather than returning non-zero (I should probably test that in 0000-basic.sh)

Quoted 26 lines

	diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
	index 80e76a4695e..1be895abba6 100755
	--- a/t/t0210-trace2-normal.sh
	+++ b/t/t0210-trace2-normal.sh
	@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '
	
	 test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
	 	test_when_finished "rm trace.normal actual expect" &&
	-	test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
	+	test_todo env GIT_TRACE2="$(pwd)/trace.normal" \
	 		test-tool trace2 008bug 2>err &&
	 	cat >expect <<-\EOF &&
	 	a bug message

I.e. in our tests you need to look out for exit code 99, not the usual
abort().
I have local patches to fix this, previously submitted as an RFC here:
https://lore.kernel.org/git/[email protected]/
I just rebased that & CI is currently running, I'll see how it does:
https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2
When I merged your patches here with that topic yours started doing the
right thing in this case, i.e. a "test_todo" that would get a BUG()" would be reported as a failure.

Quoted 14 lines

t/t0000-basic.sh test_expect_success 'subtest: a passing TODO test' ' · line 168

Quoted 5 lines

t/t0000-basic.sh +5 -0

+	test_true () {
+		true
+	}
+	test_expect_success "pretend we have fixed a test_todo breakage" \
+		"test_todo test_true"

"Why the indirection", until I realized that it's because you're working
around the whitelist of commands that we have, i.e. we allow 'git' and 'test-tool', but not 'grep' or whatever.
I'm of the opinion that we should just drop that limitation altogether,
which is shown to be pretty silly in this case. I.e. at some point we listed "test_*" as "this invokes a git binary", but a lot of our test_* commands don't, including this one.

test_expect_failure does not allow test_* are you thinking of test-tool?

Quoted 3 lines

So in general I think we should just allow any command in "test_must_fail" et al, and just catch in code review if someone uses "test_must_fail grep" as opposed to "! grep".

That is not going to scale well

Quoted 11 lines · Phillip Wood via GitGitGadget

test_todo() is intended as a fine grained alternative to test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

This series attempts to keep most of the benefits test_expect_todo() previously proposed by Ævar[1] while being simpler to use.

[1]
https://lore.kernel.org/git/[email protected]/

I like the interface you've got here much better than the one I submitted in [1], so much that it's what I tried to write at first :)

But as you noted in 1/3:

test_todo cannot be used in a subshell.

Anyway, the core difference between the APIs we proposed for this is that you'd do:

test_expect_success 'desc' 'test_todo false'

Whereas I suggested:

test_expect_todo 'desc' '! false'

Now, let's pick apart the differences:

1. With "test_expect_todo" we're declaring "this is a TODO test" for the test as a whole.

2. With your "test_todo" we're not doing that, instead we proceed as normal, and then we might note "we had a TODO" midway through, then at the end we'll spot that we had a TODO test (but this approach won't work with subshells).

3. Your "test_todo" is basically a "let's let this pass", whereas mine was a helper which exhaustively declared *what* the bad behavior was.

(Although some of yours seems to be midway between the two,
i.e. https://lore.kernel.org/git/[email protected]/)

I think the main critique you and Junio had of my series was to do with #3, i.e. that it was a hassle to exhaustively declare what the behavior is & should be, as you note in:

https://lore.kernel.org/git/[email protected]/

	test_todo \
		--want "test_must_fail git" \
		--reset "git reset --hard" \
		--expect git \
		-- \
		rm d/f &&

That's fair enough, maybe that's not worth the effort. The reason I initially hacked this up was because I'd noticed a behavior difference in a command that was only revealed in a test_expect_failure block, but because we didn't assert *what* the behavior was we didn't notice.

My version (if fully used) would spot that, but that's because of how I wrote the "tes_todo", it's orthagonal to #1 and #2 above.

So I don't see why we wouldn't instead have a "test_expect_todo" and just write the helper differently, or have a mode where it's less strict, and (if we find it worthwhile) one where it's more strict.

I rebased my
https://lore.kernel.org/git/[email protected]/
just now and applied the below on top, which seems to me to give you pretty much the end result you want, the only difference is that my version will also work in subshells (see the t2500 one):

t/t1091-sparse-checkout-builtin.sh +4 -4

index de1ec89007d..fe47e503bd1 100755
@@ -468,7 +468,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat
  	git -C unmerged sparse-checkout disable
  '
 
-test_expect_failure 'sparse-checkout reapply' '
+test_expect_todo 'sparse-checkout reapply' '
  	git clone repo tweak &&
 
  	echo dirty >tweak/deep/deeper2/a &&

@@ -502,11 +502,11 @@ test_expect_failure 'sparse-checkout reapply' '
 
  	# NEEDSWORK: We are asking to update a file outside of the
  	# sparse-checkout cone, but this is no longer allowed.
-	git -C tweak add folder1/a &&
+	test_todo git -C tweak add folder1/a &&
  	git -C tweak sparse-checkout reapply 2>err &&
-	test_must_be_empty err &&
+	test_todo test_must_be_empty err &&
  	test_path_is_missing tweak/deep/deeper2/a &&
-	test_path_is_missing tweak/folder1/a &&
+	test_todo test_path_is_missing tweak/folder1/a &&
 
  	git -C tweak sparse-checkout disable
  '

t/t2500-untracked-overwriting.sh +2 -2

index 5c0bf4d21fc..db7c72d38d8 100755
@@ -167,7 +167,7 @@ test_expect_success 'git rebase fast forwarding and untracked files' '
  	)
  '
 
-test_expect_failure 'git rebase --autostash and untracked files' '
+test_expect_todo 'git rebase --autostash and untracked files' '
  	test_setup_sequencing rebase_autostash_and_untracked &&
  	(
  		cd sequencing_rebase_autostash_and_untracked &&

@@ -176,7 +176,7 @@ test_expect_failure 'git rebase --autostash and untracked files' '
  		mkdir filler &&
  		echo precious >filler/file &&
  		cp filler/file expect &&
-		git rebase --autostash init &&
+		test_todo git rebase --autostash init &&
  		test_path_is_file filler/file
  	)
  '

t/t3510-cherry-pick-sequence.sh +2 -2

index 3b0fa66c33d..b31b6b0f7a0 100755
@@ -577,7 +577,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
  	grep "cherry picked from.*$picked" msg
  '
 
-test_expect_failure '--signoff is automatically propagated to resolved conflict' '
+test_expect_todo '--signoff is automatically propagated to resolved conflict' '
  	pristine_detach initial &&
  	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
  	echo "c" >foo &&

@@ -591,7 +591,7 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'
  	git cat-file commit HEAD~3 >initial_msg &&
  	! grep "Signed-off-by:" initial_msg &&
  	grep "Signed-off-by:" unrelatedpick_msg &&
-	! grep "Signed-off-by:" picked_msg &&
+	test_todo ! grep "Signed-off-by:" picked_msg &&
  	grep "Signed-off-by:" anotherpick_msg
  '
 

t/t3600-rm.sh +4 -4

index e74a318ac33..6c7929f5557 100755
@@ -790,7 +790,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (no index)' '
  	test_path_is_file e/f
  '
 
-test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '
+test_expect_todo SYMLINKS 'rm across a symlinked leading path (w/ index)' '
  	rm -rf d e &&
  	mkdir d &&
  	echo content >d/f &&

@@ -798,10 +798,10 @@ test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '
  	git commit -m "d/f exists" &&
  	mv d e &&
  	ln -s e d &&
-	test_must_fail git rm d/f &&
-	git rev-parse --verify :d/f &&
+	test_todo test_must_fail git rm d/f &&
+	test_todo git rev-parse --verify :d/f &&
  	test -h d &&
-	test_path_is_file e/f
+	test_todo test_path_is_file e/f
  '
 
  test_expect_success 'setup for testing rm messages' '

t/t4014-format-patch.sh +2 -2

index ad5c0292794..a6a5a330180 100755
@@ -165,12 +165,12 @@ test_expect_success 'additional command line cc (ascii)' '
  	grep "^ *S E Cipient <[email protected]>\$" hdrs5
  '
 
-test_expect_failure 'additional command line cc (rfc822)' '
+test_expect_todo 'additional command line cc (rfc822)' '
  	git config --replace-all format.headers "Cc: R E Cipient <[email protected]>" &&
  	git format-patch --cc="S. E. Cipient <[email protected]>" --stdout main..side >patch5 &&
  	sed -e "/^\$/q" patch5 >hdrs5 &&
  	grep "^Cc: R E Cipient <[email protected]>,\$" hdrs5 &&
-	grep "^ *\"S. E. Cipient\" <[email protected]>\$" hdrs5
+	test_todo grep "^ *\"S. E. Cipient\" <[email protected]>\$" hdrs5
  '
 
  test_expect_success 'command line headers' '

t/test-lib-functions.sh +15 -0

index f342954de11..9d5706454a5 100644
@@ -1049,6 +1049,21 @@ test_must_fail_acceptable () {
  	esac
  }
 
+test_todo () {
+	local negate=-ne
+	local cmp_op=-ne
+	if test "$1" = "!"
+	then
+		negate=t &&
+		cmp_op=-eq
+		shift
+	fi &&
+	"$@" 2>&7
+	exit_code=$?
+	say "test_todo: got $exit_code ${negate:+negated!} from $*"
+	test "$exit_code" "$cmp_op" 0
+}
+
  # 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:
  #

I think there is a question of whether we need a new toplevel test_expect_todo - why would we add it if we can just reuse test_expect_success? That way when a test failure is fixed all that needs to be done is to remove the test_todo calls.

From: Phillip Wood <[email protected]>

test_todo() is intended as a fine grained replacement for test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

Failing commands are reported by the test harness in the same way as test_expect_failure() so there is no change in output when migrating from test_expect_failure() to test_todo(). If a command marked with test_todo() succeeds then the test will fail. This is designed to make it easier to see when a command starts succeeding in our CI compared to using test_expect_failure() where it is easy to fix a failing test case and not realize it.

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

This commit coverts a few tests to show the intended use of test_todo(). A limitation of test_todo() as it is currently implemented is that it cannot be used in a subshell.

Signed-off-by Phillip Wood <[email protected]>

Quoted 7 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

test_todo() is intended as a fine grained replacement for test_expect_failure(). Rather than marking the whole test as failing test_todo() is used to mark individual failing commands within a test. This approach to writing failing tests allows us to detect unexpected failures that are hidden by test_expect_failure().

I love this idea! I've nearly been burned a couple of times by the wrong line in a 'test_expect_failure' triggering the error (e.g., due to bad syntax earlier in the test). The added specificity of 'test_todo' will help both reviewers and people fixing the underlying issues demonstrated by expected-failing tests.

Quoted 9 lines

Failing commands are reported by the test harness in the same way as test_expect_failure() so there is no change in output when migrating from test_expect_failure() to test_todo(). If a command marked with test_todo() succeeds then the test will fail. This is designed to make it easier to see when a command starts succeeding in our CI compared to using test_expect_failure() where it is easy to fix a failing test case and not realize it.

Quoted 4 lines

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

Should this be so restrictive? I think 'test_todo' would need to handle any arbitrary command (mostly because of custom functions like 'ensure_not_expanded' in 't1092') to be an easy-to-use drop-in replacement for 'test_expect_failure'.

I see there's some related discussion in another subthread [1], but I don't necessarily think removing restrictions (i.e. that the tested command must be 'git', 'test_*', etc.) on 'test_todo' requires doing the same for 'test_must_fail' et al. to be internally consistent. On one hand, 'test_todo' could be interpreted as an assertion (like 'test_must_fail'), where we only want to assert on our code - hence the restrictions. From that perspective, it would make sense to ease restrictions uniformly on all of our assertion helpers.

On the other hand, I'm interpreting 'test_todo' as 'test_expect_failure_on_line_N' - more of a "post-test result interpreter" than an assertion helper. So because 'test_expect_failure' doesn't require the failing line to come from a particular command, I don't think 'test_todo' needs to either. That leaves assertion helpers like 'test_must_fail' out of the scope of this change, avoiding any hairiness of allowing them to assert on arbitrary code.

What do you think?

Quoted 10 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Failing commands are reported by the test harness in the same way as test_expect_failure() so there is no change in output when migrating from test_expect_failure() to test_todo(). If a command marked with test_todo() succeeds then the test will fail. This is designed to make it easier to see when a command starts succeeding in our CI compared to using test_expect_failure() where it is easy to fix a failing test case and not realize it.

Quoted 4 lines

test_todo() is built upon test_expect_failure() but accepts commands starting with test_* in addition to git. As our test_* assertions use BUG() to signal usage errors any such error will not be hidden by test_todo().

Should this be so restrictive? I think 'test_todo' would need to handle any arbitrary command (mostly because of custom functions like 'ensure_not_expanded' in 't1092') to be an easy-to-use drop-in replacement for 'test_expect_failure'.

I see there's some related discussion in another subthread [1], but I don't necessarily think removing restrictions (i.e. that the tested command must be 'git', 'test_*', etc.) on 'test_todo' requires doing the same for 'test_must_fail' et al. to be internally consistent. On one hand, 'test_todo' could be interpreted as an assertion (like 'test_must_fail'), where we only want to assert on our code - hence the restrictions. From that perspective, it would make sense to ease restrictions uniformly on all of our assertion helpers.

On the other hand, I'm interpreting 'test_todo' as 'test_expect_failure_on_line_N' - more of a "post-test result interpreter" than an assertion helper. So because 'test_expect_failure' doesn't require the failing line to come from a particular command, I don't think 'test_todo' needs to either. That leaves assertion helpers like 'test_must_fail' out of the scope of this change, avoiding any hairiness of allowing them to assert on arbitrary code.

What do you think?

[1] https://lore.kernel.org/git/[email protected]/

Quoted 6 lines

This commit coverts a few tests to show the intended use of test_todo(). A limitation of test_todo() as it is currently implemented is that it cannot be used in a subshell.

Signed-off-by Phillip Wood <[email protected]>

I added the restrictions to 'test_todo' to try and stop it being misused but I'm happy to relax them if needed. I'm keen that test_todo is able to distinguish between an expected failure and a failure due to the wrapped command being misused e.g. 'test_todo grep --invalid-option' should report an error.

I think you've convinced be to remove the restrictions on what can be wrapped by 'test_todo' when I re-roll.

Thanks for your thoughtful comments

Quoted 5 lines · Phillip Wood

I added the restrictions to 'test_todo' to try and stop it being misused but I'm happy to relax them if needed. I'm keen that test_todo is able to distinguish between an expected failure and a failure due to the wrapped command being misused e.g. 'test_todo grep --invalid-option' should report an error.

Hmm, but it is not useful if the failure is from "you cannot use the system command grep with test_todo", as the (implicitly) encouraged "fix" the developer who wrote the test would pick would be to use "! grep --invalid-option" which would still fail for a wrong reason.

If a "git" command is expected to run to a completion but is currently broken and produces a wrong output, it would be very useful to be able to write

	git command --args >actual &&
	test_todo grep -e "$expected_token" actual

to say "when 'git command' is fixed, the output should contains this, but we know it currently is broken".

Quoted 4 lines

I think you've convinced be to remove the restrictions on what can be wrapped by 'test_todo' when I re-roll.

Thanks for your thoughtful comments

Thanks.