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 19, 2022, 07:12 UTC
Message-ID: <xmqqilsa76ve.fsf@gitster.g>
In-Reply-To: <220319.86v8waetae.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 53 quoted lines

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

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.

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.

Show 11 quoted lines

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