Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 3, 2016, 05:12 UTC
Message-ID: <CAPig+cR7pPHZv_z3G+BsLPqP7WYSVUb_7c2qmM+0y-TFeWjaSg@mail.gmail.com>
In-Reply-To: <CAFZEwPOAWh48YCxA3B+kRxVpkwN32OHW7Qrb9ajs2Cy0S8sjLw@mail.gmail.com>

On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 33 quoted lines

> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>> and git-commit so if a new verbose related behavior is introduced in
>>> git-commit, then it should not affect the behavior of git-status.
>>>
>>> One previous commit (title: commit: add a commit.verbose config
>>> variable) introduced a new config variable named commit.verbose,
>>> so care should be taken that it would not affect the behavior of
>>> status.
>>>
>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>> respect "unspecified" values") changes the initial value of verbose
>>> from 0 to -1. This can cause git-status to display a verbose output even
>>> when it isn't supposed to.
>>>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> If these are documenting what your previous patches broke, then
>> there test body should describe what should happen, and then if it
>> is broken, use test_expect_failure, no?
>>
>> Your first test does "run status with commit.verbose is set, and
>> make sure the "diff --git" does not appear", which is correct, so if
>> it does not work, test_expect_failure would be the right thing to
>> use.
>>
>> These, especially the latter, look rather unpleasant regressions to
>> me, and the main commit.verbose change would need to be held back
>> before they are fixed.
>
> I agree that using test_expect_failure would be a better way of going
> with this thing. Thanks. Will send an updated patch for this.

Please don't. test_expect_failure() is not warranted.

Step back a moment and recall why these tests were added. Earlier rounds of this series were buggy and caused regressions in git-status. As a consequence, reviewers suggested[1,2] that you improve test coverage to ensure that such breakage is caught early.

The problems which caused the regressions were addressed in later versions of the series, thus using test_expect_success() is indeed correct, whereas test_expect_failure(), which illustrates broken behavior, would be the wrong choice.

The point of these new tests is to prevent regressions caused by *subsequent* changes, which is why it was suggested that these tests be added early (as a "preparatory patch"[3]), not at the very end of the series as done here in v15.

This patch's commit message is perhaps a bit too detailed about what could have gone wrong in earlier patches in this series; indeed, it misled Junio into thinking that patches in this series did break behavior, when in fact, it was instead previous rounds of this series which were buggy. If you instead make this a preparatory patch[3], then you can sell it more simply by explaining that git-commit and git-status share implementation (without necessarily going into detail about exactly what is shared), and that you're improving test coverage to ensure that changes specific to git-commit don't accidentally impact git-status, as well.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648 [2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730 [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468