Re: [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs
- From
Eric S. Raymond <esr@thyrsus.com>
- Date
- Jan 12, 2013, 15:47 UTC
- Message-ID
- <20130112154719.GA3270@thyrsus.com>
- In-Reply-To
- <7v62339du4.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com>:
> And here is what I got:
Hm. In my version of these tests, I only have one regression from the old combo (in the pathological tags test, t9602). You're seeing more breakage than that, obviously.
> A funny thing was that without cvsps-3.7 on $PATH (which means I am > getting distro packaged cvsps 2.1), I got identical errors.
That suggests that something in your test setup has gone bad and is introducing spurious errors. Which would be consistent with the above.
> Looking > at the log message, it seems that you meant to remove t960[123], so > perhaps the patch simply forgot to remove 9601 and 9602?
Yes.
Show 6 quoted lines
> As neither test runs "git cvsimport" with -o/-m/-M options, ideally > we should be able to pass them with and without having cvsps-3.x. > Not passing them without cvsps-3.x would mean that the fallback mode > of rewritten cvsimport is not working as expected. Not passing them > with cvsps-3.x may mean the tests were expecting a wrong conversion > result, or they uncover bugs in the replacement cvsimport.
That's possible, but seems unlikely. Because the new cvsimport is such a thin wrapper around the conversion engine, bugs in it should lead to obvious crashes or failure to run the engine rather than the sort of conversion error the t960* tests are designed to check. Really all it does is assemble options to pass to the conversion engines.
My test strategy is aimed at the engine, not the wrapper. I took the repos from t960* and wrote a small Python framework to check the same assertions as the git-tree tests do, but using the engine. For example, here's how my t9602 looks:
import os, cvspstest
cc = cvspstest.ConvertComparison("t9602", "module")
cc.cmp_branch_tree("test of branch", "master", True)
cc.cmp_branch_tree("test of branch", "vendorbranch", True)
cc.cmp_branch_tree("test of branch", "B_FROM_INITIALS", False)
cc.cmp_branch_tree("test of branch", "B_FROM_INITIALS_BUT_ONE", False)
cc.cmp_branch_tree("test of branch", "B_MIXED", False)
cc.cmp_branch_tree("test of branch", "B_SPLIT", True)
cc.cmp_branch_tree("test of tag", "vendortag", False)
# This is the only test new cvsps fails that old git-cvsimport passed.
cc.cmp_branch_tree("test of tag", "T_ALL_INITIAL_FILES", True)
cc.cmp_branch_tree("test of tag", "T_ALL_INITIAL_FILES_BUT_ONE", False)
cc.cmp_branch_tree("test of tag", "T_MIXED", False)
cc.cleanup()
> t9600 fails with "-a is no longer supported", even without having > cvsps-3.x on the $PATH (i.e. attempting to use the fallback). I > wonder if this is an option the updated cvsimport would want to > simply ignore?
Probably. But I don't think you should keep these tests in the git tree. That wasn't a great idea even when you were supporting just one engine; with two (and soon three) it's going to be just silly. Let sanity-checking the engines be *my* problem, since I have to do it anyway.
(I'm working towards the generalized test suite as fast as I can. First results probably in four days or so.)
Show 6 quoted lines
> It is a way to tell the old cvsps/cvsimport to disable its > heuristics to ignore commits made within the last 10 minutes (this > is done in the hope of waiting for the per-file nature of CVS > commits to stabilize, IIUC); the user tells the command that he > knows that the CVS repository is now quiescent and it is safe to > import the whole thing.
Yes, that's just what -a is supposed to do. But is should be irrelevant for testing - in the test framework CVS is running locally, so there's no network lag.
> So... does this mean that we now set the minimum required version of > Python to 2.7? I dunno.
That would be bad, IMO. I'll put backporting to 2.6 high on my to-do list.
-- <a href="http://www.catb.org/~esr/">Eric S. Raymond</a>