Re: [PATCH 1/7] test-lib: add a "test_expect_todo", similar to "test_expect_failure"
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Add an alternative to the "test_expect_failure" function. Like
> "test_expect_failure" it will marks a test as "not ok ... TODO" in the
Show 5 quoted lines
> TAP output, and thus document that it's a "TODO" test that fails
> currently.
>
> Unlike "test_expect_failure" it asserts that the tested code in
> exactly one manner, and not any other.
ECANNOTPARSE due to a verb missing. For now I'll assume "It asserts that the tested code fails in exactly one matter" and keep going.
> We'll thus stop conflating
> e.g. segfaults with other sorts of errors, and generally be able to
> tell if our "expected to fail" tests start failing in new and
> unexpected ways.
The above makes it sound wonderful but I got somewhat confused when I tried to see how well it conveys what it wants to tell by picking an example from this patch. Say, for example:
Show 5 quoted lines
> -test_expect_failure 'git grep .fi a' '
> - git grep .fi a
> +test_expect_todo 'git grep .fi a' '
> + test_must_fail git grep .fi a
> '
So, we used to say
"We eventually, when things are fixed, want 'git grep' to find a
string '.fi' in file 'a', but currently this does not work".Now the updated one says
"We know 'git grep' fails to find string '.fi' in file 'a'"
If it is a trivial single statement test like the above, the distinction does not matter, but if it were a test with multiple steps, the readability would become quite different. It would turn
test_expect_failure 'sample test' '
preparation 1 &&
preparation 2 &&
git command invocation that we want to succeed
'
into
test_expect_todo 'sample test' '
preparation 1 &&
preparation 2 &&
test_must_fail git command invocation that we want to succeed
'
"expect_failure" expects the whole thing to fail, so we will miss if any preparation steps fail, but "expect_todo" is like "expect_success" so it will help us debuging the test by catching a silly mistake in preparation steps.
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.
> * More generally, "test_expect_failure" by design doesn't test what
> does, but just asserts that the test fails in some way.
Exactly.
It matters in complex tests that !(A&B&C) is different from (A&B&!C), the latter of which is what we want to do with tests that document what currently does not work.
Show 10 quoted lines
> - test_expect_failure [<prereq>] <message> <script>
>
> This is NOT the opposite of test_expect_success, but is used
> - to mark a test that demonstrates a known breakage. Unlike
> + to mark a test that demonstrates a known breakage whose exact failure
> + behavior isn't predictable.
> +
> + If the exact breakage is known the "test_expect_todo" function
> + should be used instead. Usethis function if it's hard to pin down
> + the exact nature of the failure. Unlike
Show 5 quoted lines
> -test_expect_failure 'clone --local detects misnamed objects' '
> - test_must_fail git clone --local misnamed misnamed-checkout
> +test_expect_todo 'clone --local detects misnamed objects' '
> + git clone --local misnamed misnamed-checkout
> '
Just like too many negatives confuse readers, I have to say this is quite confusing. Do we or do we not want "git clone" invocation to succeed?
Show 7 quoted lines
> +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.
Looks like a good first step. I'd stop reading the series for now at this one.
Ævar Arnfjörð Bjarmason
