archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: "Ævar Arnfjörð Bjarmason" <>
Cc:, John Cai <>,
	Elijah Newren <>,
	Derrick Stolee <>
Subject: Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"
Date: Fri, 18 Mar 2022 11:59:19 -0700	[thread overview]
Message-ID: <xmqqczij9jeg.fsf@gitster.g> (raw)
In-Reply-To: <> (=?utf-8?B?IsOGdmFyCUFybmZqw7Zyw7A=?= Bjarmason"'s message of "Fri, 18 Mar 2022 01:33:56 +0100")

Ævar Arnfjörð Bjarmason  <> 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


	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.


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

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

  reply	other threads:[~2022-03-18 18:59 UTC|newest]

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

Reply instructions:

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

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

  Avoid top-posting and favor interleaved quoting:

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

  git send-email \
    --in-reply-to=xmqqczij9jeg.fsf@gitster.g \ \ \ \ \ \ \

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