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 00:07:12 +0100 [thread overview]
Message-ID: <220319.86v8waetae.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq8rt77zp7.fsf@gitster.g>
On Fri, Mar 18 2022, Junio C Hamano wrote:
> 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?
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.
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
Which is quite a handful, which is why the series goes on to
e.g. introduce a todo_test_cmp, used e.g. like this (and we can easily
add more wrappers for common cases):
cat >want <<-EOF &&
$(git rev-parse HEAD)
EOF
cat >expect <<-\EOF &&
error: can't rev-parse stuff
EOF
test_might_fail git some-cmd HEAD >actual &&
todo_test_cmp want expect actual
I.e. if you just want to throw your hands in the air and say "I don't
care how this fails and just emit a 'not ok .. TODO' line" we already
have test_expect_failure for that use-case.
I think in most other cases documenting that something is broken or
should behave differently shouldn't be synonymous with not caring *how*
that unwanted behavior works right now.
Understanding of your past commentary on this topic is that you strongly
objected to not having the test suite output reflect that a given test
was "not ok ... TODO" in some way. I.e. even though I think
"test_expect_success" has the approximate *semantics* we want we
shouldn't use those.
But I think the combination of "test_expect_todo" and the "test_todo"
primitive should satisfy that, and will give us accurate assertions
about what we're actually doing now.
next prev parent reply other threads:[~2022-03-18 23:26 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 [this message]
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=220319.86v8waetae.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).