Re: [JGIT PATCH 6/5 v2] Add tests for RevWalk and its supporting code

From: Shawn O. Pearce <spearce@spearce.org>
Date: Mar 16, 2009, 14:33 UTC
Message-ID: <20090316143359.GM22920@spearce.org>
In-Reply-To: <200903151234.36812.robin.rosenberg.lists@dewire.com>

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:

Show 7 quoted lines

> 
> A few /09 interesting/ places in StartGenerator are not covered by the tests.
> 
> 		if (q instanceof DateRevQueue)
> 			pending = (DateRevQueue)q;
> 		else
> -->			pending = new DateRevQueue(q);

This one is difficult to test. Near as I can tell from the code in the revwalk package, it never happens. But if we did get here with a non date rev queue, DateRevQueue()'s constructor can copy the data from q over to one with no risk.

Perhaps just test this constructor in isolation?

Show 6 quoted lines

> and
> 
> 		if (tf != TreeFilter.ALL) {
> -->			rf = AndRevFilter.create(rf, new RewriteTreeFilter(w, tf));
> -->			pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE;
> 		}

That says we have no tests on the path filter code. Adding one is likely to identify at least one bug. Clearly we need additional tests on the path filter sections. And it isn't just for this one little conditional; the entire RewriteTreeFilter and RewriteGenerator are untested right now.

I can work on adding more test coverage here, but its not in the
critical path for me at day-job right now.  The other problems I
tried to address in this series were.  So its lower in my queue
so-to-speak.  But I'll see what I can do in the near future to
write additional tests for this area of the revwalk package.

Show 8 quoted lines

> PendingGenerator
> 						if (n != null && n.commitTime >= last.commitTime) {
> 							// This is too close to call. The next commit we
> 							// would pop is dated after the last one produced.
> 							// We have to keep going to ensure that we carry
> 							// flags as much as necessary.
> 							//
> -->							overScan = OVER_SCAN;

*sigh*  We never get here in the test suite? I missed that.  I'll try to
work out a test case today and send an additional patch to get coverage
here.  I thought RevWalkCullTest testProperlyCullAllAncestors1() would
be hitting here.  It doesn't.

> I'll merge it anyway since this is still a huge improvement compared to the previous
> state.

Thanks.

A co-worker and I are also currently trying to track down a deadlock between C git-fetch and JGit upload-pack. Both get stuck waiting for the other to answer. Clearly its JGit's fault.

-- 
Shawn.