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

[Junio C Hamano <gitster@pobox.com>]

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

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.

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