git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.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: Sat, 19 Mar 2022 12:11:10 +0100	[thread overview]
Message-ID: <220319.86ilsadw69.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqilsa76ve.fsf@gitster.g>


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



  reply	other threads:[~2022-03-19 11:22 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
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 [this message]
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=220319.86ilsadw69.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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).