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

On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:

On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano [off-list ref] wrote:

quoted

Pranit Bauva [off-list ref] writes:

quoted

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 <redacted>

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