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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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