From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 18 Mar 2022 18:59:19 GMT
Subject: Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"
Message-ID: <xmqqczij9jeg.fsf@gitster.g>
In-Reply-To: <patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com>

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