git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, John Cai <johncai86@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"
Date: Fri, 18 Mar 2022 13:50:12 -0700	[thread overview]
Message-ID: <xmqq8rt77zp7.fsf@gitster.g> (raw)
In-Reply-To: <xmqqczij9jeg.fsf@gitster.g> (Junio C. Hamano's message of "Fri, 18 Mar 2022 11:59:19 -0700")

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?



  reply	other threads:[~2022-03-18 20:50 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
2022-03-18 20:50     ` Junio C Hamano [this message]
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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=xmqq8rt77zp7.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=johncai86@gmail.com \
    --cc=newren@gmail.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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