Re: [RFC] test-lib: detect common misuse of test_expect_failure

Jeff King [off-list ref] writes:

I like the general idea, but I'm not sure how this would interact with
the tests in t0000 that test the test suite.

I tried but gave up adding a new test for this to t0000 ;-)

quoted

 test_expect_failure () {
+	if test "$test_in_progress" = 1
+	then
+		error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
+	fi

This follows existing practice for things like the &&-lint-checker, and
bails out on the whole test script.

Yes, you guessed correctly where the above came from.

That sometimes makes it hard to find
the problematic test, especially if you're running via something like
"prove", because it doesn't make valid TAP output.

Yeah, true.

It might be nicer if we just said "this test is malformed, and therefore
fails", and then you get all the usual niceties for recording and
finding the failed test.

I don't think it would be robust enough to try to propagate the error up
to the outer test_expect_success block (and anyway, you'd also want to
know about it in a test_expect_failure block; it's a bug in the test,
not a known breakage). But perhaps error() could dump some TAP-like
output with a "virtual" failed test.

Something like:
...
which lets "make prove" collect the broken test number.

It would perhaps need to cover the case when $test_count is "0"
separately. I dunno. It would be nicer still if we could continue
running other tests in the script, but I think it's impossible to
robustly jump back to the outer script.

These kinds of "bug in the test suite" are presumably rare enough that
the niceties don't matter that much, but I trigger the &&-checker
reasonably frequently (that and test_line_count, because I can never
remember the correct invocation).

Anyway. That's all orthogonal to your patch. I just wondered if we could
do better, but AFAICT the right way to do better is to hook into
error(), which means your patch would not have to care exactly how it
fails.

Yeah, the change to error() may be a good thing to do, but it has
quite a many callers in t/*lib*.sh and definitely deserves to be a
separate patch, not tied to this single test.