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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2022, 18:59 UTC
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".

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

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

Show 7 quoted lines

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