Re: [PATCH] Add tests for rev-list --graph with options that simplify history

[Adam Simpkins <simpkins@facebook.com>]

On Fri, Aug 21, 2009 at 01:15:27PM -0700, Junio C Hamano wrote:
> Adam Simpkins <simpkins@facebook.com> writes:
> 
> > +# There's more than one "correct" way to represent the history graphically.
> > +# These tests depend on the current behavior of the graphing code.  If the
> > +# graphing code is ever changed to draw the output differently, these tests
> > +# cases will need to be updated to know about the new layout.
> 
> An ideal solution to such a problem would be not to write the tests that
> way to require _the exact layout_ of the output.

Yeah.  In the past I've been hesitant to submit tests for the graph
behavior for precisely this reason.  However, having tests that check
the exact layout seems better than not having tests at all.

> What was the bug you were trying to fix?  Was it that in a simplified
> history some arcs are not connected whey they should be?

It was an issue with a missing arc between two commits that should
have been connected.  In the past, other bugs (e.g., the one fixed in
2ecbd0a0) have caused arcs to appear connected to the wrong commit.

> Can you test that without relying on other aspect (say, commits are marked
> with '*' right now but a patch might change it to '^' for some commits) of
> the output?
> 
> I am just wondering how feasible it is the problem you are trying to
> solve, not demanding you to solve it.

In general, it seems like its not worthwhile trying to solve this
problem.  I don't expect changes to the graph layout to occur often.
Modifying this test case if and when they do occur seems simpler and
less error-prone than trying to write code that attempts to anticipate
changes we might make in the future.

When I wrote this comment, I was thinking more about potential changes
in the way arcs are drawn in the output, or in the amount of padding.
The problem you mentioned (accepting other characters other than '*'
for commits) is easier, but I'm still not convinced we should try to
solve it.  For example, it's nice that the current code also tests
that boundary commits are represented differently than non-boundary
commits.  Being too permissive in what we accept could also
potentially hide bugs in the future.

-- 
Adam Simpkins
simpkins@facebook.com