Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 18 Mar 2019, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >> In any case, before we get better tooling to work around these issues, I
> >> still think it makes a ton of sense to encourage proper separation of
> >> concerns: to keep patches that introduce regression tests demonstrating a
> >> breakage separate from patches that fix the breakage. It would certainly
> >> help me (e.g. when staring at a range diff).
> >
> > Then perhaps improve the tools now because these separate patches
> > enter 'master' and stay in the history forever. One of the problems I
> > have with separating tests from the actual code change is the
> > description of the problem often stays on the test commit, which I
> > can't see in git-log if I'm searching for the code change. And no
> > sometimes I can't just look at the parent commit if I filter code by
> > path (and with --full-diff)
>
> I do not think the phrase "separation of concerns" is applied
> correctly here.

You might currently still think so, so let me give you material to
re-think that stance:

- If you truly care about contributors of all stripes, you also care about
  contributors who report bugs, contributors who turn bug reports into
  reproducible regression test cases, contributors who verify regressions,
  etc.

  And if you really want to give your respect to, say, the contributors
  who provide such regression test cases, you will accept them as full
  commit authors.

  And that fact alone makes it a separate concern, notwithstanding that in
  my case, the contributor who wrote the regression test case and the
  contributor who fixed the regression happened to be the same person: me.
  That fact simply does not invalidate that there are different concerns at
  play.

  You probably even know people who are good at one of those concerns, but
  not at the other.

- If it true what you say, that a bug fix should be in the same commit as
  the regression test case designed to prevent the bug from being
  reintroduced, then both should live in the same file, because it is
  really the same concern.

  But test code lives in a separate area of the source code, is not even
  "shipped with the product", *because* it is a separate concern.

- There is an entire industry built on books and training advertising
  Test Driven Development (TDD). If you really believe what you said, that
  regression tests are really the same concern as fixing the regressions,
  then you claim that all of them are wrong. Which would be a, how can I
  say it, erm, bold statement indeed.

- You did brush aside the example I gave: Hannes Sixt applied a patch
  that demonstrated the breakage, or more correctly: that *should* have
  demonstrated a breakage.

  Let's not brush aside this example. Let's not ignore it just like that.

  This commit addressed a very specific concern: it verified that there is
  a breakage. Or actually, that in this case, there was *no* breakage.

  By not separating those concerns, as you would have me do it, I would
  have made it harder to demonstrate that this bug fix was not even necessary
  in git.git's `master`. I would have made it harder to bisect *where* in
  Git for Windows' patch thicket the regression was introduced.

  You might now say that it should have been easy to separate the concern
  after the fact, to extract it from a commit that adds both fix and
  regression test case.

  But by that token, you should squash all contributions into a single, big,
  honking commit that do way too much "because it is easy to extract what
  you need". I agree, that sounds ridiculous.

  And even saying that it would be easy to extract the test case from the
  commit already admits that there is a good chunk that you can extract
  *that can live on its own*, i.e. is a separate concern.

- We introduced, specifically, the shell function `test_expect_failure` to
  allow demonstrating breakages. If we really thought that that has no
  value, if there should not be any commit that introduces a regression
  test case without fixing any bug at the same time, if that was not a
  concern, then we should get rid of that function. But experience tells us,
  of course, that we need to keep it.

- There is *semantic* meaning in `test_expect_failure`. Depriving the
  commit history from the commits that introduce such test cases
  demonstrating bugs, and from commits that fix those bugs, makes it
  impossible to perform otherwise simple queries along the lines "how many
  regressions did <insert-name-here> fix in September 2017?".

  I know you are a fan of software archaeology as much as I am (maybe even
  a bit more), so hopefully you find at least this argument convincing.

> The concern of a simple single-patch is to fix a bug---the test that
> shows what bug was fixed and protects the code by ensuring that a
> reintroduction of the bug gets noticed is an integral part of it.

By this reasoning, it should not be possible (or allowed) to write and
contribute a patch that merely demonstrates that something is wrong with
Git, but does not fix it.

That position is untenable.

> It does not make much sense to artificially split it into two.

Let's not brush aside the above-mentioned example, okay?

Let's consider how much easier it was for Hannes (and for that matter, how
much easier it is for *me*) to grab a patch that demonstrates a breakage,
apply it, and the report back whether I can confirm the breakage or not.

It would be *even* easier, of course, if I could just fetch a branch and
cherry-pick, but that is not possible in a project that is centered around
a mailing list. So that's just not an option here.

But back to the point: there is a *lot* of value in having an easy way to
reproduce breakages. Conflating this value with the value of a bug fix
does a disservice to everybody involved.

There was nothing artificial about splitting this patch into two.

Even worse: that commit was never artificially split to begin with! What
you assumed was incorrect: those two commits were always two commits. I
came up with the regression test case first, to convince myself that there
was something that needed fixing (much in the same way as Hannes wanted to
convince himself that there was something that needed fixing, and the fact
that that commit was separate from the fix made it very easy for Hannes to
point out that there was actually nothing in git.git yet that needed
fixing).

And if you are still averse to the idea of separating the concern of
demonstrating a breakage from the concern to fix it, I have really,
really, really bad news for you: a couple of my branches that I intend to
contribute as patch series, once they are finalized, *already* have those
commits that demonstrate breakages, *without* commits to fix them!

I am sorry if that made you shudder ;-)

You even saw at least one of them:
https://public-inbox.org/git/cab7bb36eb85dbe38ad95ee02b083f11f0820e24.1533421100.git.gitgitgadget@gmail.com/

But here is a silver lining for you: There is a real advantage of having
this regression test case in a separate patch *for you* (and other
reviewers): That regression test case will most likely not change in
subsequent iterations of the patch series, while the fix will most likely
look *very* different. And by having the regression test case in a
different patch than the regression fix, you will have a very easy time
verifying that it is unchanged from before, that you do not have to
re-review it.

(Although you might have to re-review it if you do not remember that patch
from the beginning of August, last year.)

> It's like arguing for adding declarations for new functions and
> global variables in *.h files in a step before a separate step adds
> their implementations in *.c files, claiming that the first step
> designs the interface (which is sufficient for writing the client
> code) and the second one gives an implementation of the interface
> (which can be replaced later), and they address two separate
> concerns.

No, that is an apples-to-oranges comparison because the declarations
themselves cannot really be used for anything, while the regression test
cases demonstrating a breakage *can* be used for:

- validating the claim that there is a breakage,

- bisecting,

- debugging,

- implementing a fix,

- verifying the claim that a given Git version has this breakage, or
  disproving it.

> And then you would find that there are some compilers that warn
> against unimplemented functions and undefined variables.

Indeed. And I often find myself between a rock and a hard place when I
want to wrap my patch series into nice, reviewable, logically separate
patches, and I cannot, because we specifically discourage file-local
(`static`) functions without users. Although we have not the faintest
problem with *global* functions without users (which makes no sense,
either disallow both file-local *and* global functions without users, or
allow both):

In b0fa1a3f9974 (test-lib-functions.sh: add generate_zero_bytes function,
2019-02-09), a function was introduced that was then consumed in
24b451e77c7d (t5318: replace use of /dev/zero with generate_zero_bytes,
2019-02-09).

In f1f5de442faf (revision: add mark_tree_uninteresting_sparse,
2019-01-16), we introduced a function that was consumed in the following
commit 4f6d26b16703 (list-objects: consume sparse tree walk, 2019-01-16).

In 47edb649973c (hex: introduce functions to print arbitrary hashes,
2018-11-14), the hash_to_hex*() functions were added, to be used only in
subsequent commits.

And the list goes on.

So in this instance, your argument is that we have to please the compiler,
even if it does not make any logical sense for humans.

After all, a patch series has a temporal order. It has a "story arc", so
to say, or in musical terms, it is a "slur". The compiler does not know
that a function will be used, or a regression test will be marked as
fixed, in a future patch. The human reviewer would. But we actively make
it harder on the human reviewer, by combining, say, the (rather large)
implementation of a linear assignment problem solver into the same commit
as its only user (also large), just to please the compiler.

Or at least we would have made it harder on the human reviewer if we had
not used a *trick* to *fool* the compiler into not complaining: the
implementation was put into the separate file file linear-assignment.c,
*even* if it has *but a single user*!

And since you actually committed this in that form, you already proved
that you agree with me. You also did not find it better to smoosh separate
concerns into the same commit. You had no objection to apply a patch that
had no value whatsoever on its own, that only realizes its value together
with a subsequent commit actually introducing a caller to that
functionality. Which is actually worse than a commit doing nothing else
than introducing a single regression test case that demonstrates a bug,
which does have value on its own.

> The "solution" would be to enclose the whole thing that was added in the
> first "*.h only patch" inside "#if 0/#endif" to hide it from the
> compiler ;-)

Yes, if we really wanted to cater to the compiler, rather than human
readers, then we would indeed do that.

Plus, let's not forget about the fact that this is an apples to oranges
comparison. More on that below.

> That in fact looks quite similar to how "test only patch" marks the new
> tests as expecting failure in the beginning. Neither is truly useful
> when applied to a context different from where it is originally
> developed for.

If you really want to hear what the equivalent to your `.h only commit`
would be, here you go. It would look something like:

-- snip --
diff --git t/...
@@ ... @@
 '

+test_expect_failure 'demonstrate a horrible bug' '
+	false # will be implemented later
+'
+
 test_done
-- snap --

*That* is the equivalent to your example of introducing just the function
signatures, leaving the actual implementation to a later commit.

You will note that this looks very different from what I am advertising,
where the actual implementation that does demonstrate a horrible bug (and
will hopefully gain a second life after the bug is fixed by keeping the
bug fixed) is added at the same time as the `test_expect_failure`.

And if you really, really, really needed another argument in favor of
accepting regression test-only patches as standalone commits: take the
unfortunate example of
https://public-inbox.org/git/CA+h-Bnuf6u=hkPBcxhMm06FbfkS+jtrozu+inqqmUY1cNkXrWQ@mail.gmail.com/

It might not have mattered in this case had you stopped discouraging
contributions of patches that merely add test cases demonstrating
breakages. But it could not have helped, for sure.

If we keep discouraging such patches (and note that I specifically
*encouraged* such a patch instead [*1*]), if we keep insisting that a
regression test case on its own is of no value, we will get even less of
those contributions.

And we will be repeating the same type of story again, where a bug was
reported, no patch was contributed (let alone applied) to demonstrate the
bug, the bug will be fixed, only to be broken *just in time* for the next
official Git version.

That is not what I want.

And that is why I keep considering it a bad practice to discourage
separating commits adding regression tests from the corresponding commits
that fix the regressions.

As I said, it might not have mattered in this instance. Or in the next
one. Or even in many cases.

But encouraging that separation of concerns, acknowledging that there is a
real, measurable value not only in knowing about a bug, but in addition
having an automated way to reproduce the bug (and to verify a bug fix),
*this* will get us more of those contributions rather than less.

Phew. What a long mail. I am glad you made it this far.

Let me end by saying that laying out all of the above, I do not intend to
annoy you or anything. I simply care about this project, and I want it to
run as smoothly as possible. And I really think that the separation of
concerns that we are talking about is one of the ways to make it run
smoother. To make it easier on contributors. To make it easier on
reviewers. To make it easier on developers validating bugs.

Ciao,
Dscho

Footnote *1*:
https://public-inbox.org/git/nycvar.QRO.7.76.6.1901091501320.41@tvgsbejvaqbjf.bet/