Hi Junio,

On Tue, 26 Jun 2018, Junio C Hamano wrote:

Christian Couder [off-list ref] writes:

quoted

Obviousness is often not the same for everybody.

... which you just learned---what you thought obvious turns out to
be not so obvious after all, so you adjust to help your readers.

Indeed. And trying to tell the reader that they should find it obvious is
not exactly productive. It only causes bad feelings and can be easily
avoided.

quoted

quoted

In this particular case it even feels as if this test is not even
testing what it should test at all:

- it should verify that all of the commits in the first parent
lineage are part of the list

It does that.

quoted

- it should verify that none of the other commits are in the list

It does that too.

But the point is it does a lot more by insisting exact output.

Indeed. One thing it does in addition is to make the test code a lot less
obvious for readers in general.

Let me summarize again what good regression tests have to deliver, because
I think it cannot be stressed enough, especially in this context where we
run the danger of adding poor regression tests:

- a regression test needs to catch regressions (d'oh)

- a regression test needs to be *quick*. Otherwise developers will skip
  running them, which is worse than having no tests at all (because the
  effort to develop them is wasted)

- a regression test must make it as easy as possible to fix regressions

There are quite a few corollaries to that last point, some of which are:

- a regression test that fails for anything but an obvious reason is
  *useless*

- a regression test that tries to test for *everything* (including dogs and
  cats) not only breaks the quickness requirement, but it also makes it
  confusing where to start fixing things. "The test talked about --bisect
  but now I am stuck somewhere in XYZ.c, what has *that* to do with
  --bisect?" is *not* something you want to risk any developer yelling at
  the test code that you authored

- less is more. If you can use commits that were already generated in the
  common "setup" step, there is literally a negative value in generating a
  new set of commits instead

- less is more. If you can catch the same regressions in three, concise
  *and* understandable lines, avoid using thirty lines instead

- regression tests should not need to be adjusted when the logic changes
  in an intended way. It is a strong sign that a regression test was
  written badly if it starts failing for any reason other than a
  regression

The overzealous "I want this output to be exactly this" stance of the
tests we are discussing here is a very obvious violation here.

Regression tests should fail only to indicate regressions.

Really. Let me repeat that because it is so obvious when you think about
it, but it is easy to forget when writing regression tests:

Regression tests should fail only to indicate regressions.

For example, the version I reviewed had a two "expected output", and
said that the actual output must match either one of them.  I guess it
was because there were two entries with the same distance and we cannot
rely on which order they appear in the result?  If a test history gained
another entry with the same distance, then would we need 6 possible
expected output because we cannot rely on the order in which these three
come out?

It is totally unnecessary to go there, as it would make those regression
tests a lot less valuable than they could otherwise be. Let me elaborate
further below.

That was the only thing I was complaining about.  Dscho gave me too
much credit and read a lot more good things than what I actually
meant to say ;-).

Why don't you just accept my praise gracefully? ;-)

It's not that I gave you a lot of praise recently, even if you clearly
deserve it.

quoted

quoted

And that is really all there is to test.

Another is that "rev-list --bisect-all" promises that the entries
are ordered by the distance value.

Yes! And you know what we can do there? We can test *precisely* that!

	# verify that the output is sorted by `dist` (descending)
	sed "s/.*dist=\([0-9]*\).*/\1/" <revs >dist &&
	sort -n -r <dist >dist.sorted &&
	test_cmp dist dist.sorted

This extracts the distance numbers into their own file, then verifies that
they were already sorted.

The really big advantage here is that any future change that might result
in a different order of entries with the same "dist" value *will not cause
this regression test to fail*. And for a good reason: because ordering
identical "dist" values differently is not a regression at all.

So taking the above three points, perhaps

	cat >expect <<EOF &&
	... as written in one of the expect list in Tiago's patch
	EOF

Please no. Please let's *not* generate more commits when we already have a
perfectly fine set of commits generated by the setup phase of the very
same script. Please let's not use confusing names for those commits.
Please let's not add a diagram whos layout deviates for no good reason
from the layout used by the existing diagram in the same file.

	# Make sure we have the same entries, nothing more, nothing less
	git rev-list --bisect-all $other_args >actual &&
	sort actual >actual.sorted &&
	sort expect >expect.sorted &&
	test_cmp expect.sorted actual.sorted

With plenty experience in investigating test failures of Git's test suite
under my belt, let me tell you how much faster it is for me to start
debugging when reading this code instead:

	git rev-list --bisect-all --first-parent F..E >revs &&
	# only E, e1..e8 should be listed, nothing else
	test_line_count = 9 revs &&
	for rev in E e1 e2 e3 e4 e5 e6 e7 e8
	do
		grep "^$(git rev-parse $rev) " revs || return
	done

I am faster by... a lot. Like, seconds instead of minutes.

As a bonus, it is also shorter. Quicker to read. Easier to grok. Quicker
to validate manually.

	# Make sure the entries are sorted in the dist order
	sed -e 's/.*(dist=\([1-9]*[0-9]\)).*/\1/' actual >actual.dists &&
	sort actual.dists >actual.dists.sorted &&
	test_cmp actual.dists.sorted actual.dists

is what I would have expected.

... but without the [1-9] because the distance can be 0. And with a star
after the [0-9] instead.

And then you have essentially the very same thing I suggested above. A
readable regression test, that only triggers failures upon the regression
we want to prevent, is fast to grok, actionable, and leads to fast
debugging.

Just to make sure: I am utterly disinterested in pushing "my" code
through. I am very much interested in having a good patch with a good
regression test and a good commit message. Otherwise I would not put up
with what it takes to review it.

So Chris, if you feel that you must "win" against me, just take Junio's
code already. In that case, however, do change his code to avoid
generating those unnecessary commits.

Thanks,
Dscho