Christian Couder <christian.couder@xxxxxxxxx> writes: > 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. >> 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. > >> - 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. 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? 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 ;-). >> 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. So taking the above three points, perhaps cat >expect <<EOF && ... as written in one of the expect list in Tiago's patch EOF # 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 # 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.
