git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/4] Add more tests of cvsimport

From
Jeff King <peff@peff.net>
Date
Feb 20, 2009, 06:25 UTC
Message-ID
<20090220062543.GA27837@coredump.intra.peff.net>
In-Reply-To
<1235107093-32605-1-git-send-email-mhagger@alum.mit.edu>
On Fri, Feb 20, 2009 at 06:18:09AM +0100, Michael Haggerty wrote:
> The test suite for "git cvsimport" is pretty limited, and I would like
> to improve the situation.  This patch series contains the first of
> what I hope will eventually be several additions to the "git
> cvsimport" test suite.

Great. I agree the test suite is terrible for cvsimport; what little is there was added only after a regression where it was totally broken. ;)

Show 5 quoted lines
> I am the maintainer of cvs2svn/cvs2git.  Most of the new tests will
> probably use fragments from the cvs2svn test suite.  I should admit
> that part of my motivation for adding tests to the "git cvsimport"
> test suite is to document its weaknesses, which do not seem to be
> especially well known.

I don't think it is a problem to document cvsimport's weakness. It is clear from list traffic that it has shortcomings, and IMHO documenting them clearly and rigorously with test cases is the first step to fixing them (or admitting that people should just use something else ;) ).

The only downside I see is that it bloats git's test suite a bit (and cvs tests are often slow to run). We can always make them optional, I suppose.

I do wonder, though, whether it would be simpler to make a "cvs import test suite" that could pluggably test cvs2svn, git-cvsimport, or other converters. Then you could test each on the exact same set of test repos. And abstracting "OK, now make a repository from this cvsroot" wouldn't be that hard for each command (I wouldn't think, but obviously I haven't tried it :) ).

> Patch 1 splits out some code into a library usable by multiple
> CVS-related tests.

That is definitely a good first step, though the usual naming convention is t/lib-cvs.sh. See t/lib-{git-svn,httpd,rebase}.sh, for example.

> Patch 2 changes the library to add the -f option when invoking cvs (to
> make it ignore the user's ~/.cvsrc file).

The code in t9600 (which gets moved to lib-cvs in your patch 1) sets HOME explicitly. So is this really a problem?

> Patch 3 adds a new test to t9600, namely to compare the entire module
> as checked out by CVS vs. git.
Sounds reasonable.
> Patch 4 adds a new test script t9601 that tests "git cvsimport"'s
> handling of CVS vendor branches.  One of these tests fails due to an
> actual bug.
Cool. Are you volunteering to fix git-cvsimport, too? :)
Show 5 quoted lines
> The second is that the new test script uses a small CVS repository
> that is part of the test suite (i.e., the *,v files are committed
> directly into the git source tree).  This is different than the
> approach of t9600, which creates its own test CVS repository using CVS
> commands.  The reasons for this are:

I think that's fine. There are other places in the test suite where things that are a pain to produce are just included as content (e.g., see some of the SVN tests in the 9100 series).

And I think all of the reasons you gave are compelling.
Show 6 quoted lines
> Finally, the *,v files comprising the CVS repository have blank
> trailing lines, triggering a warning from "git diff --check".  I don't
> think that CVS strictly requires the blank lines, but they are always
> generated by CVS, so I left them in.  But if the "git diff --check"
> warnings are considered a serious problem, the blank lines could
> probably be removed.

It's best to leave them in, I think, to create as realistic a test as possible. But you should mark the paths as "we don't care about whitespace" using gitattributes. I.e.,:

diff --git a/t/t9601/.gitattributes b/t/t9601/.gitattributes
new file mode 100644
index 0000000..562b12e
--- /dev/null
+++ b/t/t9601/.gitattributes
@@ -0,0 +1 @@
+* -whitespace

-Peff
Previous: Michael HaggertyNext: Junio C Hamano
Message 6 of 24 in “Add more tests of cvsimport”
  1. 0/4 Add more tests of cvsimportMichael Haggerty, Feb 20, 2009
  2. 1/4 Start a library for cvsimport-related testsMichael Haggerty, Feb 20, 2009
  3. 2/4 Use CVS's -f option if available (ignore user's ~/.cvsrc file)Michael Haggerty, Feb 20, 2009
  4. 3/4 Test contents of entire cvsimported "master" tree contentsMichael Haggerty, Feb 20, 2009
  5. 4/4 Add some tests of git-cvsimport's handling of vendor branchesMichael Haggerty, Feb 20, 2009
  6. Jeff KingFeb 20, 2009
  7. Junio C HamanoFeb 20, 2009
  8. Michael HaggertyFeb 20, 2009
  9. Teach the '--exclude' option to 'diff --no-index'Johannes Schindelin, Feb 20, 2009
  10. Jeff KingFeb 20, 2009
  11. Johannes SchindelinFeb 20, 2009
  12. Jakub NarebskiFeb 20, 2009
  13. Johannes SchindelinFeb 20, 2009
  14. Junio C HamanoFeb 20, 2009
  15. Johannes SchindelinFeb 24, 2009
  16. Junio C HamanoFeb 24, 2009
  17. Michael HaggertyFeb 20, 2009
  18. Jeff KingFeb 20, 2009
  19. Samuel Lucas Vaz de MelloFeb 20, 2009
  20. Michael HaggertyFeb 21, 2009
  21. Ferry Huberts (Pelagic)Feb 20, 2009
  22. Michael HaggertyFeb 21, 2009
  23. Ferry Huberts (Pelagic)Feb 21, 2009
  24. Junio C HamanoFeb 22, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.