[PATCH 0/7] test-lib-functions: a better "test_expect_failure"

[PATCH 0/7] test-lib-functions: a better "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

As explained in detail in 1/7 there's several major shortcomings to
"test_expect_failure", this series adds a "test_expect_todo".

It allows us to assert exactly how something that fails .. fails (no
more conflating segfaults for "exit 1"), but also emits all the right
output & metadata about the test being "TODO" that
"test_expect_failure" does.

The rest of the series then expands on that to convert several more
tests, along with adding helpers like:

    todo_test_cmp want expect actual

Where the "want" is what we want once we'd mark the test as
"test_expect_success", but the "expect" is what we get right now with
the failing TODO test.

This series has a (rather trivial) textual conflict with
js/ci-github-workflow-markup in "seen".

Ævar Arnfjörð Bjarmason (7):
  test-lib: add a "test_expect_todo", similar to "test_expect_failure"
  test-lib-functions: add and use a "test_todo" helper
  tests: allow test_* in "test_must_fail_acceptable" for "test_todo"
  test-lib-functions: add and use a "todo_test_cmp" helper
  test-lib-functions: add and use a "todo_test_path" helper
  test-lib-functions: make test_todo support a --reset
  sparse tests: convert a TODO test to use "test_expect_todo"

 t/README                           |  20 +++-
 t/t0000-basic.sh                   |  48 +++++++-
 t/t1021-rerere-in-workdir.sh       |   9 +-
 t/t1060-object-corruption.sh       |   4 +-
 t/t1091-sparse-checkout-builtin.sh |  17 ++-
 t/t1309-early-config.sh            |  14 ++-
 t/t1410-reflog.sh                  |   4 +-
 t/t3600-rm.sh                      |  23 +++-
 t/t6403-merge-file.sh              |  32 +++++-
 t/t7814-grep-recurse-submodules.sh |  60 ++++++----
 t/t7815-grep-binary.sh             |   4 +-
 t/test-lib-functions.sh            | 172 ++++++++++++++++++++++++++++-
 t/test-lib.sh                      |  32 +++++-
 13 files changed, 379 insertions(+), 60 deletions(-)

-- 
2.35.1.1436.g756b814e59f

[PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

Add an alternative to the "test_expect_failure" function. Like
"test_expect_failure" it will marks a test as "not ok ... TODO" in the
TAP output, and thus document that it's a "TODO" test that fails
currently.

Unlike "test_expect_failure" it asserts that the tested code in
exactly one manner, and not any other. We'll thus stop conflating
e.g. segfaults with other sorts of errors, and generally be able to
tell if our "expected to fail" tests start failing in new and
unexpected ways.

In more detail, the these problems of "test_expect_failure" are solved
by this new function.

 * When "test_expect_failure" is used in combination with
   "test_{must,might}_fail" it will hide segfaults or abort()
   failures, such as failures due to LSAN.

   This is because we do do the right thing in those helpers, but they
   themselves are expected to be used within "test_expect_success" and
   return 1. The "test_expect_failure" can't differentiate this from a
   "real" failure.

   We could change "test_{must,might}_fail" to appropriately carry
   forward the status code to work around this specific issue, but
   doing so would be a large semantic change in the current test
   suite.

 * More generally, "test_expect_failure" by design doesn't test what
   does, but just asserts that the test fails in some way.

   For some tests that truly fail in unpredictable ways this behavior
   makes sense, but it's almost never what our tests want. We know we
   e.g. have a revision at HEAD~ instead of HEAD, and would like to
   know conflate that with a potential segfault or other behavior
   change.

In summary, this new function behaves exactly like
"test_expect_success", except that its "success" is then reported as a
"not ok .. TODO".

Let's convert a few "test_expect_failure" uses to the new
"test_expect_todo".

For previous discussion on this topic see [1] and [2], in particular
the points by Junio that it's desired that the "test_expect_failure"
output differentiate the TODO tests from "test_expect_success".

1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/xmqqo824e145.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README                     | 20 +++++++++++++++++++-
 t/t0000-basic.sh             | 11 ++++++++---
 t/t1060-object-corruption.sh |  4 ++--
 t/t7815-grep-binary.sh       |  4 ++--
 t/test-lib-functions.sh      | 28 ++++++++++++++++++++++++----
 t/test-lib.sh                | 32 ++++++++++++++++++++++++++++----
 6 files changed, 83 insertions(+), 16 deletions(-)

diff --git a/t/README b/t/README
index f48e0542cdc..e0aa8ebb0eb 100644
--- a/t/README
+++ b/t/README
@@ -805,10 +805,28 @@ see test-lib-functions.sh for the full list and their options.
 	test_expect_success PERL,PYTHON 'yo dawg' \
 	    ' test $(perl -E 'print eval "1 +" . qx[python -c "print 2"]') == "4" '
 
+ - test_expect_todo [<prereq>] <message> <script>
+
+   A "test_expect_success" which on "success" will mark the test as a
+   TODO test, and on failure will emit a real failure.
+
+   This should be used to mark a test whose exact bad behavior is
+   known, but whose outcome isn't preferred, to distinguish it from
+   those tests that use "test_expect_success" to indicate known and
+   preferred behavior.
+
+   Like test_expect_success this function can optionally use a three
+   argument invocation with a prerequisite as the first argument.
+
  - test_expect_failure [<prereq>] <message> <script>
 
    This is NOT the opposite of test_expect_success, but is used
-   to mark a test that demonstrates a known breakage.  Unlike
+   to mark a test that demonstrates a known breakage whose exact failure
+   behavior isn't predictable.
+
+   If the exact breakage is known the "test_expect_todo" function
+   should be used instead. Usethis function if it's hard to pin down
+   the exact nature of the failure. Unlike
    the usual test_expect_success tests, which say "ok" on
    success and "FAIL" on failure, this will say "FIXED" on
    success and "still broken" on failure.  Failures from these
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b007f0efef2..e46638f1f7b 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -176,6 +176,8 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'
 	test_expect_failure "pretend we have a known breakage" "false"
 	test_expect_failure "pretend we have a known breakage" "false"
 	test_expect_failure "pretend we have fixed a known breakage" "true"
+	test_expect_todo "pretend we have a known TODO" "true"
+	test_expect_todo "pretend we have a bad TODO" "false"
 	test_done
 	EOF
 	check_sub_test_lib_test mixed-results2 <<-\EOF
@@ -192,10 +194,13 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'
 	> not ok 8 - pretend we have a known breakage # TODO known breakage
 	> not ok 9 - pretend we have a known breakage # TODO known breakage
 	> ok 10 - pretend we have fixed a known breakage # TODO known breakage vanished
+	> not ok 11 - pretend we have a known TODO # TODO known breakage
+	> not ok 12 - pretend we have a bad TODO (broken '\''test_expect_todo'\''!)
+	> #	false
 	> # 1 known breakage(s) vanished; please update test(s)
-	> # still have 2 known breakage(s)
-	> # failed 3 among remaining 7 test(s)
-	> 1..10
+	> # still have 3 known breakage(s)
+	> # failed 4 among remaining 8 test(s)
+	> 1..12
 	EOF
 '
 
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index bc89371f534..b7023f5644c 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -112,8 +112,8 @@ test_expect_success 'clone --local detects missing objects' '
 	test_must_fail git clone --local missing missing-checkout
 '
 
-test_expect_failure 'clone --local detects misnamed objects' '
-	test_must_fail git clone --local misnamed misnamed-checkout
+test_expect_todo 'clone --local detects misnamed objects' '
+	git clone --local misnamed misnamed-checkout
 '
 
 test_expect_success 'fetch into corrupted repo with index-pack' '
diff --git a/t/t7815-grep-binary.sh b/t/t7815-grep-binary.sh
index ac871287c03..2d17e05036e 100755
--- a/t/t7815-grep-binary.sh
+++ b/t/t7815-grep-binary.sh
@@ -64,8 +64,8 @@ test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 
-test_expect_failure 'git grep .fi a' '
-	git grep .fi a
+test_expect_todo 'git grep .fi a' '
+	test_must_fail git grep .fi a
 '
 
 test_expect_success 'grep respects binary diff attribute' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d61..a219b126d93 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -718,11 +718,13 @@ test_verify_prereq () {
 	BUG "'$test_prereq' does not look like a prereq"
 }
 
-test_expect_failure () {
+_test_expect_todo () {
+	local func=$1
+	shift
 	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
-	BUG "not 2 or 3 parameters to test-expect-failure"
+	BUG "not 2 or 3 parameters to $func"
 	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$@"
@@ -730,14 +732,32 @@ test_expect_failure () {
 		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $2"
 		if test_run_ "$2" expecting_failure
 		then
-			test_known_broken_ok_ "$1"
+			case "$func" in
+			test_expect_todo) test_known_broken_ok_ "$func" "$1" ;;
+			test_expect_failure) test_known_broken_ok_ "$func" "$1" ;;
+			esac
 		else
-			test_known_broken_failure_ "$1"
+			case "$func" in
+			test_expect_todo)
+				test_title_=$1
+				shift
+				test_failure_ "$test_title_ (broken 'test_expect_todo'!)" "$@"
+				;;
+			test_expect_failure) test_known_broken_failure_ "$func" "$1" ;;
+			esac
 		fi
 	fi
 	test_finish_
 }
 
+test_expect_failure () {
+	_test_expect_todo test_expect_failure "$@"
+}
+
+test_expect_todo () {
+	_test_expect_todo test_expect_todo "$@"
+}
+
 test_expect_success () {
 	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9af5fb7674d..fb23a6f6682 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -792,21 +792,45 @@ test_failure_ () {
 }
 
 test_known_broken_ok_ () {
+	local func=$1
+	shift
+
 	if test -n "$write_junit_xml"
 	then
 		write_junit_xml_testcase "$* (breakage fixed)"
 	fi
-	test_fixed=$(($test_fixed+1))
-	say_color error "ok $test_count - $@ # TODO known breakage vanished"
+
+	if test "$func" = "test_expect_todo"
+	then
+		# test_expect_todo
+		test_broken=$(($test_broken+1))
+		say_color warn "not ok $test_count - $@ # TODO known breakage"
+	else
+		# test_expect_failure
+		test_fixed=$(($test_fixed+1))
+		say_color error "ok $test_count - $@ # TODO known breakage vanished"
+	fi
 }
 
 test_known_broken_failure_ () {
+	local func=$1
+	shift
+
 	if test -n "$write_junit_xml"
 	then
 		write_junit_xml_testcase "$* (known breakage)"
 	fi
-	test_broken=$(($test_broken+1))
-	say_color warn "not ok $test_count - $@ # TODO known breakage"
+
+	if test "$func" = "test_expect_todo"
+	then
+		# test_expect_todo
+		test_fixed=$(($test_fixed+1))
+		say_color error "not ok $test_count - $@ # TODO a 'known breakage' changed behavior!"
+	else
+		# test_expect_failure
+		test_broken=$(($test_broken+1))
+		say_color warn "not ok $test_count - $@ # TODO known breakage"
+	fi
 }
 
 test_debug () {
-- 
2.35.1.1436.g756b814e59f

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add an alternative to the "test_expect_failure" function. Like
> "test_expect_failure" it will marks a test as "not ok ... TODO" in the

"will marks" -> "marks".

> TAP output, and thus document that it's a "TODO" test that fails
> currently.
>
> Unlike "test_expect_failure" it asserts that the tested code in
> exactly one manner, and not any other.

ECANNOTPARSE due to a verb missing.  For now I'll assume "It asserts
that the tested code fails in exactly one matter" and keep going.

> We'll thus stop conflating
> e.g. segfaults with other sorts of errors, and generally be able to
> tell if our "expected to fail" tests start failing in new and
> unexpected ways.

The above makes it sound wonderful but I got somewhat confused when
I tried to see how well it conveys what it wants to tell by picking
an example from this patch.  Say, for example:

> -test_expect_failure 'git grep .fi a' '
> -	git grep .fi a
> +test_expect_todo 'git grep .fi a' '
> +	test_must_fail git grep .fi a
>  '

So, we used to say

    "We eventually, when things are fixed, want 'git grep' to find a
    string '.fi' in file 'a', but currently this does not work".

Now the updated one says

    "We know 'git grep' fails to find string '.fi' in file 'a'"

If it is a trivial single statement test like the above, the
distinction does not matter, but if it were a test with multiple
steps, the readability would become quite different.  It would turn

	test_expect_failure 'sample test' '
		preparation 1 &&
		preparation 2 &&
		git command invocation that we want to succeed
	'

into

	test_expect_todo 'sample test' '
		preparation 1 &&
		preparation 2 &&
		test_must_fail git command invocation that we want to succeed
	'

"expect_failure" expects the whole thing to fail, so we will miss if
any preparation steps fail, but "expect_todo" is like "expect_success"
so it will help us debuging the test by catching a silly mistake in
preparation steps.

Marking the step that demonstrates the current shortcomings with
"MUST FAIL" is a bad taste, but let's pretend that we didn't notice
it ;-).  Other than that, it looks like an improvement.

>  * More generally, "test_expect_failure" by design doesn't test what
>    does, but just asserts that the test fails in some way.

Exactly.  

It matters in complex tests that !(A&B&C) is different from
(A&B&!C), the latter of which is what we want to do with tests that
document what currently does not work.

>   - test_expect_failure [<prereq>] <message> <script>
>  
>     This is NOT the opposite of test_expect_success, but is used
> -   to mark a test that demonstrates a known breakage.  Unlike
> +   to mark a test that demonstrates a known breakage whose exact failure
> +   behavior isn't predictable.
> +
> +   If the exact breakage is known the "test_expect_todo" function
> +   should be used instead. Usethis function if it's hard to pin down
> +   the exact nature of the failure. Unlike

"Usethis" -> "Use this"

> -test_expect_failure 'clone --local detects misnamed objects' '
> -	test_must_fail git clone --local misnamed misnamed-checkout
> +test_expect_todo 'clone --local detects misnamed objects' '
> +	git clone --local misnamed misnamed-checkout
>  '

Just like too many negatives confuse readers, I have to say this is
quite confusing.  Do we or do we not want "git clone" invocation to
succeed?

> +test_expect_failure () {
> +	_test_expect_todo test_expect_failure "$@"
> +}
> +
> +test_expect_todo () {
> +	_test_expect_todo test_expect_todo "$@"
> +}

It is a bit surprising that _test_expect_todo does not share more of
its implementation with test_expect_success, but let's pretend we
didn't see it.

Looks like a good first step.  I'd stop reading the series for now
at this one.

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Marking the step that demonstrates the current shortcomings with
> "MUST FAIL" is a bad taste, but let's pretend that we didn't notice
> it ;-).  Other than that, it looks like an improvement.
> ...
>> +test_expect_failure () {
>> +	_test_expect_todo test_expect_failure "$@"
>> +}
>> +
>> +test_expect_todo () {
>> +	_test_expect_todo test_expect_todo "$@"
>> +}
>
> It is a bit surprising that _test_expect_todo does not share more of
> its implementation with test_expect_success, but let's pretend we
> didn't see it.

With a bit more tweak, I think this can be made palatable:

 * introduce something that is *NOT* test_must_fail but behaves like
   so, but with a bit of magic (see below).

 * do not introduce test_expect_todo, but use an improved version of
   test_expect_success.

Let's illustrate what I mean by starting from an example that we
want to have _after_ fixing an known breakage.  Let's say we expect
that our test preparation succeeds, 'git foo' fails when given a bad
option, 'git foo' runs successfully, and the command is expected to
emit certain output.  We may assert the ideal future world like so:

	test_expect_success 'make sure foo works the way we want' '
		preparatory step &&
		test_must_fail git foo --bad-option >error &&
		grep "expected error message" error &&
		! grep "unwanted error message" error &&
		git foo >output &&
		grep expected output &&
		! grep unwanted output
	'

Let's also imagine that right now, option parsing in "git foo",
works but the main execution of the command does not work.

With test_expect_todo, you have to write something like this
to document the current breakage:

	test_expect_todo 'document breakage' '
		preparatory step &&
		test_must_fail git foo --bad-option >error &&
		grep "expected error message" error &&
		! grep "unwanted error message" error &&
		test_must_fail git foo >output &&
		! grep expected output &&
		grep unwanted output
	'

You can see that this makes one thing unclear.

Among the two test_must_fail and two !, which one(s) document the
breakage?  In other words, which one of these four negations do we
wish to lift eventually?  The answer is the latter two (we said that
handling of "--bad-option" is already working), but it is not obvious
in the above test_expect_todo test sequence.

I'd suggest we allow our test to be written this way:

	test_expect_success 'make sure foo works the way we want' '
		preparatory step &&
		test_must_fail git foo --bad-option >error &&
		grep "expected error message" error &&
		! grep "unwanted error message" error &&
	test_ki git foo >output &&
	test_ki grep expected output &&
	test_ki ! grep unwanted output
	'

and teach test_expect_success that an invocation of test_ki ("known
issue"---a better name that is NOT test_must_fail very much welcome)
means we hope this test someday passes without test_ki but not
today, i.e. what your test_expect_todo means, and we unfortunately
have to expect that the lines annotated with test_ki would "fail".

To readers, test_ki should appear as if an annotation to a single
command that highlights what we want to eventually be able to fix,
and when the issue around the line marked as test_ki is fixed, we
can signal the fact by removing it from the beginning of these lines
(that is why the last one is "test_ki !" and not "!  test_ki").

To the test framework and the shell that is running the test,
test_ki would be almost identical to test_must_fail, i.e. runs the
rest of the command, catch segv and report, but otherwise the
failure from that "rest of the command" execution is turned into
"success" that lets &&-chain to continue.  In addition, it tells
the test_expect_success running it that a success of the test piece
should be shown as TODO (expected failure).

Hmm?

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 18 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Marking the step that demonstrates the current shortcomings with
>> "MUST FAIL" is a bad taste, but let's pretend that we didn't notice
>> it ;-).  Other than that, it looks like an improvement.
>> ...
>>> +test_expect_failure () {
>>> +	_test_expect_todo test_expect_failure "$@"
>>> +}
>>> +
>>> +test_expect_todo () {
>>> +	_test_expect_todo test_expect_todo "$@"
>>> +}
>>
>> It is a bit surprising that _test_expect_todo does not share more of
>> its implementation with test_expect_success, but let's pretend we
>> didn't see it.
>
> With a bit more tweak, I think this can be made palatable:
>
>  * introduce something that is *NOT* test_must_fail but behaves like
>    so, but with a bit of magic (see below).
>
>  * do not introduce test_expect_todo, but use an improved version of
>    test_expect_success.
>
> Let's illustrate what I mean by starting from an example that we
> want to have _after_ fixing an known breakage.  Let's say we expect
> that our test preparation succeeds, 'git foo' fails when given a bad
> option, 'git foo' runs successfully, and the command is expected to
> emit certain output.  We may assert the ideal future world like so:
>
> 	test_expect_success 'make sure foo works the way we want' '
> 		preparatory step &&
> 		test_must_fail git foo --bad-option >error &&
> 		grep "expected error message" error &&
> 		! grep "unwanted error message" error &&
> 		git foo >output &&
> 		grep expected output &&
> 		! grep unwanted output
> 	'
>
> Let's also imagine that right now, option parsing in "git foo",
> works but the main execution of the command does not work.
>
> With test_expect_todo, you have to write something like this
> to document the current breakage:
>
> 	test_expect_todo 'document breakage' '
> 		preparatory step &&
> 		test_must_fail git foo --bad-option >error &&
> 		grep "expected error message" error &&
> 		! grep "unwanted error message" error &&
> 		test_must_fail git foo >output &&
> 		! grep expected output &&
> 		grep unwanted output
> 	'
>
> You can see that this makes one thing unclear.
>
> Among the two test_must_fail and two !, which one(s) document the
> breakage?  In other words, which one of these four negations do we
> wish to lift eventually?  The answer is the latter two (we said that
> handling of "--bad-option" is already working), but it is not obvious
> in the above test_expect_todo test sequence.
>
> I'd suggest we allow our test to be written this way:
>
> 	test_expect_success 'make sure foo works the way we want' '
> 		preparatory step &&
> 		test_must_fail git foo --bad-option >error &&
> 		grep "expected error message" error &&
> 		! grep "unwanted error message" error &&
> 	test_ki git foo >output &&
> 	test_ki grep expected output &&
> 	test_ki ! grep unwanted output
> 	'
>
> and teach test_expect_success that an invocation of test_ki ("known
> issue"---a better name that is NOT test_must_fail very much welcome)
> means we hope this test someday passes without test_ki but not
> today, i.e. what your test_expect_todo means, and we unfortunately
> have to expect that the lines annotated with test_ki would "fail".
>
> To readers, test_ki should appear as if an annotation to a single
> command that highlights what we want to eventually be able to fix,
> and when the issue around the line marked as test_ki is fixed, we
> can signal the fact by removing it from the beginning of these lines
> (that is why the last one is "test_ki !" and not "!  test_ki").
>
> To the test framework and the shell that is running the test,
> test_ki would be almost identical to test_must_fail, i.e. runs the
> rest of the command, catch segv and report, but otherwise the
> failure from that "rest of the command" execution is turned into
> "success" that lets &&-chain to continue.  In addition, it tells
> the test_expect_success running it that a success of the test piece
> should be shown as TODO (expected failure).
>
> Hmm?

Have you had the time to look past patch 1/7 of this series? 2/7
introduces a "test_todo" helper, the "test_expect_todo" is just the
basic top-level primitive.

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

Which is quite a handful, which is why the series goes on to
e.g. introduce a todo_test_cmp, used e.g. like this (and we can easily
add more wrappers for common cases):

	cat >want <<-EOF &&
	$(git rev-parse HEAD)
	EOF
	cat >expect <<-\EOF &&
	error: can't rev-parse stuff
	EOF
	test_might_fail git some-cmd HEAD >actual &&
	todo_test_cmp want expect actual

I.e. if you just want to throw your hands in the air and say "I don't
care how this fails and just emit a 'not ok .. TODO' line" we already
have test_expect_failure for that use-case.

I think in most other cases documenting that something is broken or
should behave differently shouldn't be synonymous with not caring *how*
that unwanted behavior works right now.

Understanding of your past commentary on this topic is that you strongly
objected to not having the test suite output reflect that a given test
was "not ok ... TODO" in some way. I.e. even though I think
"test_expect_success" has the approximate *semantics* we want we
shouldn't use those.

But I think the combination of "test_expect_todo" and the "test_todo"
primitive should satisfy that, and will give us accurate assertions
about what we're actually doing now.

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> emit certain output.  We may assert the ideal future world like so:
>>
>> 	test_expect_success 'make sure foo works the way we want' '
>> 		preparatory step &&
>> 		test_must_fail git foo --bad-option >error &&
>> 		grep "expected error message" error &&
>> 		! grep "unwanted error message" error &&
>> 		git foo >output &&
>> 		grep expected output &&
>> 		! grep unwanted output
>> 	'
>>
>> Let's also imagine that right now, option parsing in "git foo",
>> works but the main execution of the command does not work.
>>
>> With test_expect_todo, you have to write something like this
>> to document the current breakage:
>>
>> 	test_expect_todo 'document breakage' '
>> 		preparatory step &&
>> 		test_must_fail git foo --bad-option >error &&
>> 		grep "expected error message" error &&
>> 		! grep "unwanted error message" error &&
>> 		test_must_fail git foo >output &&
>> 		! grep expected output &&
>> 		grep unwanted output
>> 	'
>>
>> You can see that this makes one thing unclear.
>>
>> Among the two test_must_fail and two !, which one(s) document the
>> breakage?  In other words, which one of these four negations do we
>> wish to lift eventually?  The answer is the latter two (we said that
>> handling of "--bad-option" is already working), but it is not obvious
>> in the above test_expect_todo test sequence.
>>
>> I'd suggest we allow our test to be written this way:
>>
>> 	test_expect_success 'make sure foo works the way we want' '
>> 		preparatory step &&
>> 		test_must_fail git foo --bad-option >error &&
>> 		grep "expected error message" error &&
>> 		! grep "unwanted error message" error &&
>> 	test_ki git foo >output &&
>> 	test_ki grep expected output &&
>> 	test_ki ! grep unwanted output
>> 	'
>>
>> and teach test_expect_success that an invocation of test_ki ("known
>> issue"---a better name that is NOT test_must_fail very much welcome)
>> means we hope this test someday passes without test_ki but not
>> today, i.e. what your test_expect_todo means, and we unfortunately
>> have to expect that the lines annotated with test_ki would "fail".

> Have you had the time to look past patch 1/7 of this series? 2/7
> introduces a "test_todo" helper, the "test_expect_todo" is just the
> basic top-level primitive.

No, and I do not have to.  I care about the most basic form first,
and if you cannot get it right, it is not interesting.  You can
consider the test_ki above as the primitive form of your "test_todo"
that says "I want the command to give true, but I know it currently
gives false".

And quite honestly, I am not interested in _how_ it currently
happens to break.  We may want the command being tested to
eventually count three commits, but due to a bug, it may only count
one.  You may say "test_todo count --want 3 --expect 1 blah", but
the "expect" part is much less interesting than the fact that the
command being tested on _that_ line (not the whole sequence run with
test_expect_failure) is clearly documented to want 3 but currently
is broken, and it can be clearly distinguished from the normal
test_must_fail or ! that documents that we do want a failure out of
the command being tested there.

So with or without the "higher level" wrappers, how else, other than
the way I showed in the message you are responding to as a rewrite
of the example to use test_expect_todo, that uses two test_must_fail
and two ! and makes which ones are expected failure and which ones
are documentation of the current breakage, do you propose to write
the equivalent?  It may be that your test_todo may be a different
way to spell the test_ki marker I showed above, and if that is the
case it is perfectly fine, but I want it to be THE primitive, not
test_must_fail or !, to mark a single command in the test that
currently does not work as expected.

> 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.

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 19 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> emit certain output.  We may assert the ideal future world like so:
>>>
>>> 	test_expect_success 'make sure foo works the way we want' '
>>> 		preparatory step &&
>>> 		test_must_fail git foo --bad-option >error &&
>>> 		grep "expected error message" error &&
>>> 		! grep "unwanted error message" error &&
>>> 		git foo >output &&
>>> 		grep expected output &&
>>> 		! grep unwanted output
>>> 	'
>>>
>>> Let's also imagine that right now, option parsing in "git foo",
>>> works but the main execution of the command does not work.
>>>
>>> With test_expect_todo, you have to write something like this
>>> to document the current breakage:
>>>
>>> 	test_expect_todo 'document breakage' '
>>> 		preparatory step &&
>>> 		test_must_fail git foo --bad-option >error &&
>>> 		grep "expected error message" error &&
>>> 		! grep "unwanted error message" error &&
>>> 		test_must_fail git foo >output &&
>>> 		! grep expected output &&
>>> 		grep unwanted output
>>> 	'
>>>
>>> You can see that this makes one thing unclear.
>>>
>>> Among the two test_must_fail and two !, which one(s) document the
>>> breakage?  In other words, which one of these four negations do we
>>> wish to lift eventually?  The answer is the latter two (we said that
>>> handling of "--bad-option" is already working), but it is not obvious
>>> in the above test_expect_todo test sequence.
>>>
>>> I'd suggest we allow our test to be written this way:
>>>
>>> 	test_expect_success 'make sure foo works the way we want' '
>>> 		preparatory step &&
>>> 		test_must_fail git foo --bad-option >error &&
>>> 		grep "expected error message" error &&
>>> 		! grep "unwanted error message" error &&
>>> 	test_ki git foo >output &&
>>> 	test_ki grep expected output &&
>>> 	test_ki ! grep unwanted output
>>> 	'
>>>
>>> and teach test_expect_success that an invocation of test_ki ("known
>>> issue"---a better name that is NOT test_must_fail very much welcome)
>>> means we hope this test someday passes without test_ki but not
>>> today, i.e. what your test_expect_todo means, and we unfortunately
>>> have to expect that the lines annotated with test_ki would "fail".
>
>> Have you had the time to look past patch 1/7 of this series? 2/7
>> introduces a "test_todo" helper, the "test_expect_todo" is just the
>> basic top-level primitive.
>
> No, and I do not have to.  I care about the most basic form first,
> and if you cannot get it right, it is not interesting.  You can
> consider the test_ki above as the primitive form of your "test_todo"
> that says "I want the command to give true, but I know it currently
> gives false".

Sure, and I do have that implemented. If you're just asking that my
"test_todo" or another helper do that by default, then that's easy.

I.e. I've got that, but not as one short "test_*" verb.

> And quite honestly, I am not interested in _how_ it currently
> happens to break.  We may want the command being tested to
> eventually count three commits, but due to a bug, it may only count
> one.  You may say "test_todo count --want 3 --expect 1 blah", but
> the "expect" part is much less interesting than the fact that the
> command being tested on _that_ line (not the whole sequence run with
> test_expect_failure) is clearly documented to want 3 but currently
> is broken, and it can be clearly distinguished from the normal
> test_must_fail or ! that documents that we do want a failure out of
> the command being tested there.

Yes, if you don't want to test the exact behavior you have/want that's
also easy.

> So with or without the "higher level" wrappers, how else, other than
> the way I showed in the message you are responding to as a rewrite
> of the example to use test_expect_todo, that uses two test_must_fail
> and two ! and makes which ones are expected failure and which ones
> are documentation of the current breakage, do you propose to write
> the equivalent?  It may be that your test_todo may be a different
> way to spell the test_ki marker I showed above, and if that is the
> case it is perfectly fine, but I want it to be THE primitive, not
> test_must_fail or !, to mark a single command in the test that
> currently does not work as expected.

Sure, yes it's basically a different way to spell the same thing....

>> 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 the only sensible thing to do to fix that is to have the
semantics of test_expect_todo, within that you can always decide to
ignore individual exit codes, but you can't really do it the other way
around (which is what test_expect_failure does).

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Phillip Wood]

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

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> .... 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.

;-)

Another small detail in my suggestion that will make a huge
difference in the end is not to introduce test_expect_todo as a
separate top-level construct, and instead teach test_expect_success
to pay attention to the use of test_todo "unfortunately this does
not work yet" mark in it.  It allows us to use test_todo in a shared
test helper function and call them in test_expect_success, and when
the step the test helper has trouble with gets fixed, the "unmark"
step will be an isolated change.

Your sample change seems to already have it, which is good.

> 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

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Derrick Stolee]

On 3/20/22 2:07 PM, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> .... 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.
> 
> ;-)
> 
> Another small detail in my suggestion that will make a huge
> difference in the end is not to introduce test_expect_todo as a
> separate top-level construct, and instead teach test_expect_success
> to pay attention to the use of test_todo "unfortunately this does
> not work yet" mark in it.  It allows us to use test_todo in a shared
> test helper function and call them in test_expect_success, and when
> the step the test helper has trouble with gets fixed, the "unmark"
> step will be an isolated change.
> 
> Your sample change seems to already have it, which is good.

After reading this thread, I agree with many of the ideas that were
generated in response to this topic.

The thing I'm hoping to see from a final version is that a top-level
helper like test_expect_todo will expect at least one test_todo
helper to be executed inside of the test (perhaps communicated by
setting a special GIT_TEST_* environment variable?) and if any of
the test_todo lines change from fail to pass, then that is
communicated as a _failure_ from CI's perspective. Let us discover
if we have accidentally "fixed" any of these test cases and update
the tests accordingly.

I can predict writing a test case with multiple test_todo lines
that need to be updated to drop the test_todo helpers one-by-one
as a change is being introduced.

Thanks,
-Stolee

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> The thing I'm hoping to see from a final version is that a top-level
> helper like test_expect_todo will expect at least one test_todo
> helper to be executed inside of the test (perhaps communicated by
> setting a special GIT_TEST_* environment variable?) and if any of

I was hoping that we can do without test_expect_todo.
test_expect_success can turn itself into test_expect_todo when it
sees test_todo is invoked even once in it.  And Phillip's outline
actually implements the idea, if I am not mistaken.

> the test_todo lines change from fail to pass, then that is
> communicated as a _failure_ from CI's perspective. Let us discover
> if we have accidentally "fixed" any of these test cases and update
> the tests accordingly.

In other words, we do not want to lose the "TODO fixed" we have been
getting out of test_expect_failure, which I agree with.  I am not
sure if Phillip's outline had that feature.

> I can predict writing a test case with multiple test_todo lines
> that need to be updated to drop the test_todo helpers one-by-one
> as a change is being introduced.

Yes.

Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"

[Phillip Wood]

On 23/03/2022 22:13, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> The thing I'm hoping to see from a final version is that a top-level
>> helper like test_expect_todo will expect at least one test_todo
>> helper to be executed inside of the test (perhaps communicated by
>> setting a special GIT_TEST_* environment variable?) and if any of
> 
> I was hoping that we can do without test_expect_todo.
> test_expect_success can turn itself into test_expect_todo when it
> sees test_todo is invoked even once in it.  And Phillip's outline
> actually implements the idea, if I am not mistaken.

You are correct, there is no test_expect_todo in the outline I posted, 
test_todo lives within test_expect_success and works like test_must_fail.
>> the test_todo lines change from fail to pass, then that is
>> communicated as a _failure_ from CI's perspective. Let us discover
>> if we have accidentally "fixed" any of these test cases and update
>> the tests accordingly.
> 
> In other words, we do not want to lose the "TODO fixed" we have been
> getting out of test_expect_failure, which I agree with. 

I read Stolee's comments the other way, that we want the test harness to 
see the test fail rather passing as it does with the "TODO fixed" 
feature. In one of my early contributions I inadvertently fixed a 
submodule test and did not realize until someone pointed it out because 
the CI passes rather than fails when a test_expect_failure is fixed.

> I am not
> sure if Phillip's outline had that feature.

In my outline the test fails if any command marked by test_todo is 
successful and test_todo prints a messaging saying the command was 
unexpectedly successful. Implementing the "TODO fixed" feature gets 
tricky when there is more than one test_todo within a single 
test_expect_success - what if one is test_todo command is successful and 
the others fail?


Best Wishes

Phillip

>> I can predict writing a test case with multiple test_todo lines
>> that need to be updated to drop the test_todo helpers one-by-one
>> as a change is being introduced.
> 
> Yes.

[PATCH 2/7] test-lib-functions: add and use a "test_todo" helper

[Ævar Arnfjörð Bjarmason]

Introduce a new "test_todo" function for use with the new
"test_expect_todo" function, and convert a couple of tests to use it,
including one added in aecad374ae7 (reflog-walk: don't segfault on
non-commit sha1's in the reflog, 2016-01-05).

A good thing about "test_expect_failure" is that it allowed for
writing the sort of tests added in aecad374ae7, where we declare what
we'd like to eventually get, but just mark that we're still failing to
get that.

For cases like the one-line "t7815-grep-binary.sh" test adjusted in
the preceding commit to use "test_expect_todo" it's obvious what we'd
eventually like to do, to remove the "test_must_fail" and convert
"test_expect_todo" to "test_expect_success".

But in the cases being adjusted here it would be a regression to
simply remove the "test_line_count", as the "expect" part of it is
what we'd eventually like to have pass.

By introducing a "test_todo" we can both assert how something
currently fails, and how it should behave once the relevant feature is
implemented (or a bug is fixed).

The "test_todo" function will always run the check for what we
eventually want, and BUG() out if it succeeds. We'll also BUG() out if
neither succeed.

This way we can ensure that unlike "test_expect_failure" blocks the
assertions aren't dead or broken code. A few uses of
"test_expect_failure" in the test suite (not altered here) are
hopelessly broken, and could never pass even if a bug was fixed or a
given feature was implemented.

We just haven't noticed because we've been treating all failures in
"test_expect_failure" equally, using "test_expect_todo" with this
"test_todo" helper is a better way forward.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh        | 37 ++++++++++++++++++++
 t/t1410-reflog.sh       |  4 +--
 t/t6403-merge-file.sh   |  9 +++--
 t/test-lib-functions.sh | 77 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 4 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index e46638f1f7b..bbe48d63b16 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -204,6 +204,43 @@ test_expect_success 'subtest: mixed results: a mixture of all possible results'
 	EOF
 '
 
+test_expect_success 'subtest: test_expect_todo with test_todo' '
+	write_and_run_sub_test_lib_test test-expect-todo <<-\EOF &&
+	test_expect_todo "command not implemented yet" "
+		test_todo \
+			--want \"git unimplemented\" \
+			--expect \"test_must_fail git unimplemented\"
+	"
+	test_done
+	EOF
+	check_sub_test_lib_test test-expect-todo <<-\EOF
+	> not ok 1 - command not implemented yet # TODO known breakage
+	> # still have 1 known breakage(s)
+	> 1..1
+	EOF
+'
+
+test_expect_success 'subtest: test_expect_todo with test_todo: prefix + suffix arguments' '
+	write_and_run_sub_test_lib_test test-expect-todo-pfx <<-\EOF &&
+	test_expect_todo "prefix argument for test_todo" "
+		echo x >want &&
+		echo y >expect &&
+		cp expect actual &&
+		test_todo test_cmp \
+			--want want \
+			--expect expect \
+			-- \
+			actual
+	"
+	test_done
+	EOF
+	check_sub_test_lib_test test-expect-todo-pfx <<-\EOF
+	> not ok 1 - prefix argument for test_todo # TODO known breakage
+	> # still have 1 known breakage(s)
+	> 1..1
+	EOF
+'
+
 test_expect_success 'subtest: --verbose option' '
 	write_and_run_sub_test_lib_test_err t1234-verbose --verbose <<-\EOF &&
 	test_expect_success "passing test" true
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 68f69bb5431..fcd5ff4e313 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -369,9 +369,9 @@ test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
 	git reflog refs/tests/tree-in-reflog
 '
 
-test_expect_failure 'reflog with non-commit entries displays all entries' '
+test_expect_todo 'reflog with non-commit entries displays all entries' '
 	git reflog refs/tests/tree-in-reflog >actual &&
-	test_line_count = 3 actual
+	test_todo test_line_count --want "= 3" --expect "= 2" -- actual
 '
 
 # This test takes a lock on an individual ref; this is not supported in
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 2f421d967ab..12b334af85c 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -82,9 +82,14 @@ test_expect_success "merge without conflict (--quiet)" '
 	git merge-file --quiet test.txt orig.txt new2.txt
 '
 
-test_expect_failure "merge without conflict (missing LF at EOF)" '
+test_expect_todo "merge without conflict (missing LF at EOF)" '
 	cp new1.txt test2.txt &&
-	git merge-file test2.txt orig.txt new4.txt
+	test_todo \
+		test_expect_code \
+		--expect 1 \
+		--want 0 \
+		-- \
+		git merge-file test2.txt orig.txt new4.txt
 '
 
 test_expect_failure "merge result added missing LF" '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index a219b126d93..53335393b9b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -959,6 +959,83 @@ test_path_is_missing () {
 	fi
 }
 
+# Usage: test_todo [<common-prefix>] <options> [-- <common-suffix>...]
+#	--want <want>
+#		The condition we'd like. Injected between
+#		<common-prefix> and <common-suffix> arguments.
+#	--expect <expect>
+#		The condition we have now. Injected in the same way as
+#		the arguments to --want.
+#
+# test_todo is a wrapper for use with "test_expect_todo". It declares
+# an outcome we want, and one we currently expect:
+#
+#	test_todo --want true --expect false
+#
+# It can also take a <common-prefix> prefix along with
+# <common-suffix>... parameters after a "--", e.g.:
+#
+#	# We want 1 line, not 2
+# 	test_todo test_line_count --want "= 1" --expect "= 2" -- actual
+#
+# Here both variants of the "test_line_count" will be run to assert
+# that the "want" variant doesn't pass yet, and that the "expect"
+# variant describes the current behavior.
+#
+# Because we run both neither of them can mutate the test
+# state. I.e. they must be read-only commands such as "wc -l", and not
+# a state-altering command such as "rm".
+test_todo () {
+	local common_fn= &&
+	local have_want= &&
+	local want= &&
+	local expect= &&
+	local have_expect= &&
+	while test $# != 0
+	do
+		case "$1" in
+		--want)
+			want="$2" &&
+			have_want=t &&
+			shift
+			;;
+		--expect)
+			expect="$2" &&
+			have_expect=t &&
+			shift
+			;;
+		--)
+			shift &&
+			break
+			;;
+		*)
+			if test -n "$common_fn"
+			then
+				BUG "the <common-fn> can only be given once" &&
+				return 1
+			fi &&
+			common_fn="$1"
+			;;
+		esac
+		shift
+	done &&
+	if test "$have_want$have_expect" != "tt"
+	then
+		BUG "test_todo must get a --want <want> and --expect <expect>"
+	fi &&
+
+	if $common_fn $want "$@"
+	then
+		BUG "a test_todo succeeded with --want ('$want').  Turn it into a test_expect_success + $@ $want?" &&
+		return 1
+	elif $common_fn $expect "$@"
+	then
+		say "a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead" >&3 &&
+		return 0
+	fi &&
+	BUG "a test_todo didn't pass with either --want ('$want') or --expect ('$expect')"
+}
+
 # test_line_count checks that a file has the number of lines it
 # ought to. For example:
 #
-- 
2.35.1.1436.g756b814e59f

[PATCH 3/7] tests: allow test_* in "test_must_fail_acceptable" for "test_todo"

[Ævar Arnfjörð Bjarmason]

Convert tests added in [1] and [2] to use "test_expect_todo" to more
specifically mark the failure condition, rather then the blank check
for failure given by "test_expect_failure".

For [1] there isn't an easy way to fit the "echo world" and "test_cmp"
into a "test_todo", and in any case we might not have that output once
we fix the bug noted in [1].

For [2] we'd need add "test_with_config" (which invokes both "git" and
"test-tool" to the list in "test_must_fail_acceptable", but adding a
file-specific function to "test-lib-functions.sh" would be a bit odd.

Let's instead expand the restrictive list added in
6a67c759489 (test-lib-functions: restrict test_must_fail usage,
2020-07-07). As shown in the commits that preceded it the list came
about because we wanted to exclude the likes of "cvs" and "p4" from
"test_might_fail".

It's a fair bet that if we use it with a test_* function that that
function is being appropriately used with it (i.e. invokes one of or
own programs). It's possible that without an exhaustive list we'll get
it wrong, but I also don't think it's worth it to maintain that
exhaustive list. Let's just allow all test_* names instead.

1. 90a6464b4ad (rerere: make sure it works even in a workdir attached
   to a young repository, 2011-03-10
2. 751d3b9415f (t1309: document cases where we would want early config
   not to die(), 2017-03-13)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1021-rerere-in-workdir.sh |  9 +++++----
 t/t1309-early-config.sh      | 14 ++++++++++----
 t/test-lib-functions.sh      |  2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/t/t1021-rerere-in-workdir.sh b/t/t1021-rerere-in-workdir.sh
index 0b892894eb9..e4f4b9124c3 100755
--- a/t/t1021-rerere-in-workdir.sh
+++ b/t/t1021-rerere-in-workdir.sh
@@ -41,7 +41,7 @@ test_expect_success SYMLINKS 'rerere in workdir' '
 # For the purpose of helping contrib/workdir/git-new-workdir users, we do not
 # have to support relative symlinks, but it might be nicer to make this work
 # with a relative symbolic link someday.
-test_expect_failure SYMLINKS 'rerere in workdir (relative)' '
+test_expect_todo SYMLINKS 'rerere in workdir (relative)' '
 	rm -rf .git/rr-cache &&
 	"$SHELL_PATH" "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" . krow &&
 	(
@@ -49,9 +49,10 @@ test_expect_failure SYMLINKS 'rerere in workdir (relative)' '
 		rm -f .git/rr-cache &&
 		ln -s ../.git/rr-cache .git/rr-cache &&
 		test_must_fail git merge side &&
-		git rerere status >actual &&
-		echo world >expect &&
-		test_cmp expect actual
+		test_todo \
+			--want "git" \
+			--expect "test_must_fail git" \
+			 -- rerere status
 	)
 '
 
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 537435b90ae..fedbdac621a 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -81,13 +81,19 @@ test_expect_success 'ignore .git/ with incompatible repository version' '
 	test_i18ngrep "warning:.* Expected git repo version <= [1-9]" err
 '
 
-test_expect_failure 'ignore .git/ with invalid repository version' '
-	test_with_config "[core]repositoryformatversion = invalid"
+test_expect_todo 'ignore .git/ with invalid repository version' '
+	test_todo \
+		--want test_with_config \
+		--expect "test_must_fail test_with_config" \
+		-- "[core]repositoryformatversion = invalid"
 '
 
 
-test_expect_failure 'ignore .git/ with invalid config' '
-	test_with_config "["
+test_expect_todo 'ignore .git/ with invalid config' '
+	test_todo \
+		--want test_with_config \
+		--expect "test_must_fail test_with_config" \
+		-- "["
 '
 
 test_expect_success 'early config and onbranch' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 53335393b9b..64b9580f2bc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1127,7 +1127,7 @@ test_must_fail_acceptable () {
 	fi
 
 	case "$1" in
-	git|__git*|test-tool|test_terminal)
+	git|__git*|test-tool|test_*)
 		return 0
 		;;
 	*)
-- 
2.35.1.1436.g756b814e59f

[PATCH 4/7] test-lib-functions: add and use a "todo_test_cmp" helper

[Ævar Arnfjörð Bjarmason]

Introduce a new "todo_test_cmp" for use with the new
"test_expect_todo" function. This is a thin wrapper around the
previously introduced "test_todo". Instead of the more verbose:

    test_todo test_cmp --want want --expect expect -- actual

We can now do:

    todo_test_cmp want expect actual

Since it uses "test_todo", this "test_cmp_todo" function will BUG()
out if "want" and "expect" are the same, and likewise if the "want" is
equivalent to "actual".

Let's convert most of the tests added in 45bde58ef8f (grep:
demonstrate bug with textconv attributes and submodules, 2021-09-29)
to use it, as well as a merge test added in
6d49de414f9 (t6023-merge-file.sh: fix and mark as broken invalid
tests, 2014-06-29).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6403-merge-file.sh              | 23 +++++++++++--
 t/t7814-grep-recurse-submodules.sh | 54 +++++++++++++++++++++---------
 t/test-lib-functions.sh            | 22 ++++++++++++
 3 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 12b334af85c..d466360c41a 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -92,8 +92,27 @@ test_expect_todo "merge without conflict (missing LF at EOF)" '
 		git merge-file test2.txt orig.txt new4.txt
 '
 
-test_expect_failure "merge result added missing LF" '
-	test_cmp test.txt test2.txt
+test_expect_todo "merge result added missing LF" '
+	cat >expect <<-\EOF &&
+	Dominus regit me, et nihil mihi deerit.
+	In loco pascuae ibi me collocavit,
+	super aquam refectionis educavit me;
+	animam meam convertit,
+	deduxit me super semitas jusitiae,
+	<<<<<<< test2.txt
+	<<<<<<< test2.txt
+	propter nomen suum.
+	Nam et si ambulavero in medio umbrae mortis,
+	non timebo mala, quoniam tu mecum es:
+	virga tua et baculus tuus ipsa me consolata sunt.
+	=======
+	propter nomen suum.
+	>>>>>>> new4.txt
+	=======
+	propter nomen suum.
+	>>>>>>> new4.txt
+	EOF
+	todo_test_cmp test.txt expect test2.txt
 '
 
 test_expect_success "merge without conflict (missing LF at EOF, away from change in the other file)" '
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index a4476dc4922..8d9b53ccfed 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -442,77 +442,98 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo
 	test_must_be_empty actual
 '
 
-test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '
+test_expect_todo 'grep --textconv: superproject .gitattributes does not affect submodules' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 	echo "a diff=d2x" >.gitattributes &&
 
+	cat >want <<-\EOF &&
+	a:(1|2)x(3|4)
+	EOF
 	cat >expect <<-\EOF &&
 	a:(1|2)x(3|4)
+	submodule/a:(1|2)x(3|4)
+	submodule/sub/a:(1|2)x(3|4)
 	EOF
 	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	todo_test_cmp want expect actual
 '
 
-test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '
+test_expect_todo 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 	echo "a diff=d2x" >.gitattributes &&
 	git add .gitattributes &&
 	rm .gitattributes &&
 
+	cat >want <<-\EOF &&
+	a:(1|2)x(3|4)
+	EOF
 	cat >expect <<-\EOF &&
 	a:(1|2)x(3|4)
+	submodule/a:(1|2)x(3|4)
+	submodule/sub/a:(1|2)x(3|4)
 	EOF
 	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	todo_test_cmp want expect actual
 '
 
-test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '
+test_expect_todo 'grep --textconv: superproject .git/info/attributes does not affect submodules' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 	super_attr="$(git rev-parse --git-path info/attributes)" &&
 	test_when_finished "rm -f \"$super_attr\"" &&
 	echo "a diff=d2x" >"$super_attr" &&
 
+	cat >want <<-\EOF &&
+	a:(1|2)x(3|4)
+	EOF
 	cat >expect <<-\EOF &&
 	a:(1|2)x(3|4)
+	submodule/a:(1|2)x(3|4)
+	submodule/sub/a:(1|2)x(3|4)
 	EOF
 	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	todo_test_cmp want expect actual
 '
 
 # Note: what currently prevents this test from passing is not that the
 # .gitattributes file from "./submodule" is being ignored, but that it is being
 # propagated to the nested "./submodule/sub" files.
 #
-test_expect_failure 'grep --textconv correctly reads submodule .gitattributes' '
+test_expect_todo 'grep --textconv correctly reads submodule .gitattributes' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 	echo "a diff=d2x" >submodule/.gitattributes &&
 
+	cat >want <<-\EOF &&
+	submodule/a:(1|2)x(3|4)
+	EOF
 	cat >expect <<-\EOF &&
 	submodule/a:(1|2)x(3|4)
+	submodule/sub/a:(1|2)x(3|4)
 	EOF
 	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	todo_test_cmp want expect actual
 '
 
-test_expect_failure 'grep --textconv correctly reads submodule .gitattributes (from index)' '
+test_expect_todo 'grep --textconv correctly reads submodule .gitattributes (from index)' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 	echo "a diff=d2x" >submodule/.gitattributes &&
 	git -C submodule add .gitattributes &&
 	rm submodule/.gitattributes &&
 
-	cat >expect <<-\EOF &&
+	cat >want <<-\EOF &&
 	submodule/a:(1|2)x(3|4)
 	EOF
-	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	>expect &&
+
+	test_might_fail git grep --textconv --recurse-submodules x >actual &&
+	todo_test_cmp want expect actual
 '
 
-test_expect_failure 'grep --textconv correctly reads submodule .git/info/attributes' '
+test_expect_todo 'grep --textconv correctly reads submodule .git/info/attributes' '
 	reset_and_clean &&
 	test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
 
@@ -520,11 +541,12 @@ test_expect_failure 'grep --textconv correctly reads submodule .git/info/attribu
 	test_when_finished "rm -f \"$submodule_attr\"" &&
 	echo "a diff=d2x" >"$submodule_attr" &&
 
-	cat >expect <<-\EOF &&
+	cat >want <<-\EOF &&
 	submodule/a:(1|2)x(3|4)
 	EOF
-	git grep --textconv --recurse-submodules x >actual &&
-	test_cmp expect actual
+	>expect &&
+	test_might_fail git grep --textconv --recurse-submodules x >actual &&
+	todo_test_cmp want expect actual
 '
 
 test_expect_failure 'grep saves textconv cache in the appropriate repository' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 64b9580f2bc..4d1eca380e8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1263,6 +1263,28 @@ test_cmp () {
 	eval "$GIT_TEST_CMP" '"$@"'
 }
 
+# todo_test_cmp is a "test_cmp" for use in conjunction with
+# "test_expect_todo".
+#
+# It takes a mandatory extra first argument of "want", indicating the
+# output we'd like to have once we turn that "test_expect_todo" into a
+# "test_expect_success":
+#
+#	test_expect_todo 'foo still doesn't work' '
+#		echo yay >want &&
+#		echo error >expect &&
+#		foo >actual &&
+#		test_cmp want expect actual
+#	'
+todo_test_cmp () {
+	test "$#" -ne 3 && BUG "3 param, not $#"
+	local want=$1 &&
+	local expect=$2 &&
+	local actual=$3 &&
+
+	test_todo test_cmp --want "$want" --expect "$expect" -- "$actual"
+}
+
 # Check that the given config key has the expected value.
 #
 #    test_cmp_config [-C <dir>] <expected-value>
-- 
2.35.1.1436.g756b814e59f

[PATCH 5/7] test-lib-functions: add and use a "todo_test_path" helper

[Ævar Arnfjörð Bjarmason]

Add a new "todo_test_path" helper and convert an additional test added
in 45bde58ef8f (grep: demonstrate bug with textconv attributes and
submodules, 2021-09-29) to use it in conjunction with
"test_expect_todo".

Like the "todo_test_cmp" function introduced in a preceding commit,
this function is a trivial wrapper around "test_todo". Rather than a
more verbose:

	test_todo \
		--want "test_path_is_missing" \
		--expect "test_path_is_file" \
		-- "$super_textconv_cache"

We can do:

	todo_test_path is_missing is_file "$super_textconv_cache"

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7814-grep-recurse-submodules.sh |  6 +++---
 t/test-lib-functions.sh            | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 8d9b53ccfed..8df692ee9a0 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -549,7 +549,7 @@ test_expect_todo 'grep --textconv correctly reads submodule .git/info/attributes
 	todo_test_cmp want expect actual
 '
 
-test_expect_failure 'grep saves textconv cache in the appropriate repository' '
+test_expect_todo 'grep saves textconv cache in the appropriate repository' '
 	reset_and_clean &&
 	test_config_global diff.d2x_cached.textconv "sed -e \"s/d/x/\"" &&
 	test_config_global diff.d2x_cached.cachetextconv true &&
@@ -562,8 +562,8 @@ test_expect_failure 'grep saves textconv cache in the appropriate repository' '
 	super_textconv_cache="$(git rev-parse --git-path refs/notes/textconv/d2x_cached)" &&
 	sub_textconv_cache="$(git -C submodule rev-parse \
 			--path-format=absolute --git-path refs/notes/textconv/d2x_cached)" &&
-	test_path_is_missing "$super_textconv_cache" &&
-	test_path_is_file "$sub_textconv_cache"
+	todo_test_path is_missing is_file "$super_textconv_cache" &&
+	todo_test_path is_file is_missing "$sub_textconv_cache"
 '
 
 test_expect_success 'grep partially-cloned submodule' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4d1eca380e8..3febf4b0811 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1036,6 +1036,31 @@ test_todo () {
 	BUG "a test_todo didn't pass with either --want ('$want') or --expect ('$expect')"
 }
 
+# todo_test_path is a test_path_* for use in conjunction with
+# "test_expect_todo".
+#
+# It takes "want_fn" and "expect_fn" arguments of e.g. "is_file" or
+# "is_dir", which will be turned into corresponding "test_file_*"
+# calls. Use it like:
+#
+#	test_expect_todo 'foo should be a directory' '
+#		>foo &&
+#		todo_test_path is_dir is_file foo
+#	'
+todo_test_path () {
+	test "$#" -ne 3 && BUG "3 param, not $#"
+	local want_fn=$1
+	local expect_fn=$2
+	local path=$3 &&
+	shift 3 &&
+
+	test_todo \
+		--want "test_path_$want_fn" \
+		--expect "test_path_$expect_fn" \
+		-- \
+		"$path"
+}
+
 # test_line_count checks that a file has the number of lines it
 # ought to. For example:
 #
-- 
2.35.1.1436.g756b814e59f

[PATCH 6/7] test-lib-functions: make test_todo support a --reset

[Ævar Arnfjörð Bjarmason]

As noted in the preceding commit that introduced "test_todo" we
couldn't run something like "git rm" since we run both the --want and
--expect variants, and if --want has removed a file the --expect won't
succeed.

Let's add a --reset option to the command, this allows us to convert a
test added in 03415ca8db2 (t3600: document failure of rm across
symbolic links, 2013-04-04) to a more exact "test_expect_todo".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3600-rm.sh           | 23 ++++++++++++++++++-----
 t/test-lib-functions.sh | 26 ++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index e74a318ac33..42879e9060b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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,23 @@ 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 -h d &&
-	test_path_is_file e/f
+	test_todo \
+		--want "test_must_fail git" \
+		--reset "git reset --hard" \
+		--expect git \
+		-- \
+		rm d/f &&
+	test_todo \
+		--want git \
+		--expect "test_must_fail git" \
+		-- \
+		rev-parse --verify :d/f &&
+	test_todo \
+		--want "test -h" \
+		--expect "test_path_is_missing" \
+		-- \
+		d &&
+	todo_test_path is_file is_missing e/f
 '
 
 test_expect_success 'setup for testing rm messages' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3febf4b0811..5313ab28e72 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -966,6 +966,10 @@ test_path_is_missing () {
 #	--expect <expect>
 #		The condition we have now. Injected in the same way as
 #		the arguments to --want.
+#	--reset <reset>
+#		A command to run between the <want> and <expect>
+#		conditions to reset the repository state. Used e.g. if
+#		both run a "git rm" command that might succeed.
 #
 # test_todo is a wrapper for use with "test_expect_todo". It declares
 # an outcome we want, and one we currently expect:
@@ -985,8 +989,12 @@ test_path_is_missing () {
 # Because we run both neither of them can mutate the test
 # state. I.e. they must be read-only commands such as "wc -l", and not
 # a state-altering command such as "rm".
+#
+# To run a command that mutates the repository state supply a --reset
+# option, e.g. "git reset --hard" if you need to run "git rm".
 test_todo () {
 	local common_fn= &&
+	local reset= &&
 	local have_want= &&
 	local want= &&
 	local expect= &&
@@ -1004,6 +1012,10 @@ test_todo () {
 			have_expect=t &&
 			shift
 			;;
+		--reset)
+			reset="$2" &&
+			shift
+			;;
 		--)
 			shift &&
 			break
@@ -1028,10 +1040,16 @@ test_todo () {
 	then
 		BUG "a test_todo succeeded with --want ('$want').  Turn it into a test_expect_success + $@ $want?" &&
 		return 1
-	elif $common_fn $expect "$@"
-	then
-		say "a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead" >&3 &&
-		return 0
+	else
+		if test -n "$reset"
+		then
+			$reset
+		fi &&
+		if $common_fn $expect "$@"
+		then
+			say "a test_todo will succeed with --expect ('$expect'), we eventually want '$want' instead" >&3 &&
+			return 0
+		fi
 	fi &&
 	BUG "a test_todo didn't pass with either --want ('$want') or --expect ('$expect')"
 }
-- 
2.35.1.1436.g756b814e59f

[PATCH 7/7] sparse tests: convert a TODO test to use "test_expect_todo"

[Ævar Arnfjörð Bjarmason]

Change a test that used "test_expect_success" before 49fdd51a235 (add:
skip tracked paths outside sparse-checkout cone, 2021-09-24) to use
"test_expect_todo" instead.

Now we'll test for the exact current behavior, while documenting what
behavior we'd like to get instead.

This test is a good example of the sort of cases where we benefit most
from "test_expect_todo". In 49fdd51a235 the only change here (aside
from the "NEEDSWORK" comment) was changing "test_expect_success" to
"test_expect_failure".

We thus lost test coverage, and would not have noticed if we failed in
some unexpected place in this rather large test (it's 30 lines of
setup before getting to the "NEEDSWORK" comment). Now we can get the
test coverage back, while documenting what is and isn't desired
behavior.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1091-sparse-checkout-builtin.sh | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 9a900310186..23973bc186b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -467,7 +467,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 &&
@@ -501,11 +501,18 @@ 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 test_expect_code \
+		--want 0 \
+		--expect 1 \
+		-- \
+		git -C tweak add folder1/a &&
 	git -C tweak sparse-checkout reapply 2>err &&
-	test_must_be_empty err &&
-	test_path_is_missing tweak/deep/deeper2/a &&
-	test_path_is_missing tweak/folder1/a &&
+	test_todo \
+		--want test_must_be_empty \
+		--expect "grep warning:.*paths.*unmerged" \
+		-- err &&
+	todo_test_path is_file is_missing tweak/deep/deeper2/a &&
+	todo_test_path is_missing is_file tweak/folder1/a &&
 
 	git -C tweak sparse-checkout disable
 '
-- 
2.35.1.1436.g756b814e59f