Re: [PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

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

Jon Simons <jon@jonsimons.org> writes:

> +When fixing a bug, you may find it useful to first write a test using
> +`test_expect_failure` to confirm that the test exposes the bug.
> +However, unless there is a strong reason not to, submit the test as
> +`test_expect_success` in the same commit as your code change that
> +fixes the bug.  A separate commit that only changes `test_expect_failure`
> +to `test_expect_success` can leave the behavior being tested out of the
> +diff context, making the fix harder to review.  Reviewers who want to
> +verify that your test reproduces the bug can temporarily undo the code
> +changes while keeping the test.

This indeed matches what contributors have been told to do.

But do we even need to say the first two and a half lines?  One
might find it convenient not to write any tests, or to be satisfied
with manual testing, or to send test_expect_failure() separately
and early.  None of these is what we recommend.

As you described, adding a test_expect_failure() test with a large
body alone in one patch, and fixing the code in another patch would
show a change from test_expect_failure() to test_expect_success()
without showing much of the body of the test in the second patch,
making the review unnecessarily cumbersome.  If the test is trivial
and small, it may fit within the diff context and avoid such a
problem, but then there is no need to add such a small test as a
failing one in a separate patch in the first place, unless the goal
is to inflate the commit count.

So, unless we can think of concrete cases that qualify as a
"strong reason", I do not think we want to write everything before
and up to "a strong reason not to" in your text.

How about a short and sweet sentence like this instead?

    A bugfix patch should include tests in test_expect_success()
    blocks that demonstrate the breakage when the code change that
    implements the fix is omitted.