Re: [PATCH 2/3] t9010-svn-fi: Add tests for svn-fi

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Create a test-svn-fi in toplevel directory, add rules to build it, and
> add some basic tests.

Thanks.  Probably this should be squashed with patch #1?

> --- /dev/null
> +++ b/t/t9010-svn-fi.sh

Test number's already taken.  9012 is free, though.

> @@ -0,0 +1,303 @@
> +#!/bin/sh
> +
> +test_description='check svn dumpfile exporter'
> +
> +. ./test-lib.sh
> +
> +if ! svnadmin -h >/dev/null 2>&1
> +then
> +	skip_all='skipping svn-fi tests, svn not available'
> +	test_done
> +fi

Is it impossible to test without svn present or otherwise speed
these up?  Some random ideas:

 - feed svn-fi output into svn-fe and compare git repos
 - check if "svn-fi after svn-fe" is idempotent (should it be?)
 - check for individual salient features from svn-fi output, using
   "grep", "sed", or "awk" to ignore the rest
 - use svnrdump in place of "svnadmin load" if available (is it
   faster?)
 - find the bottleneck in svnadmin and fix it

Of course at least one test of the "svn-fe | svnadmin load"
pipeline seems worthwhile; I'm just thinking we should avoid
habits that slow down the test suite too much.

> +svn_look () {
> +	subcommand=$1 &&
> +	shift &&
> +	svnlook "$subcommand" "$svnrepo" "$@"
> +}

Needed?  The svn_cmd function exists to point to a configuration
directory.

> +test_expect_success 'normal empty files' '
> +	reinit_svn &&
> +	cat >expect.tree <<-\EOF &&
> +	/
> +	 foo
> +	 bar
> +	EOF
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <nobody@localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100

Where do the dates come from?  Why should I (the reader) expect
them?

> +# TODO: How to test date? Need to convert from local timestamp

Ah, you saw. :)

One possibility is to fuzz away whatever is unimportant when
comparing results (and using tools like "awk" to extract the
date to perform whatever tests on it are appropriate where it
matters).

> +test_expect_success 'malformed fast-import stream: author 2' '
> +	reinit_svn &&
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100
> +	data 0
> +	M 100644 inline foo
> +	data 0
> +
> +	EOF
> +	try_load input must_fail

What's wrong with this stream?

> +test_expect_success 'svn:special and svn:executable' '
> +	reinit_svn &&
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <nobody@localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100
> +	data 7
> +	nothing
> +	M 100755 inline foo
> +	data 0
> +	M 755 inline moo
> +	data 0
> +	M 120000 inline bar
> +	data 0
> +
> +	EOF
> +	try_load input &&
> +	svn_look propget svn:executable foo &&
> +	svn_look propget svn:executable moo &&
> +	svn_look propget svn:special bar

Maybe worth checking for the absence of svn:executable +
svn:special from an ordinary file, too?

> +test_expect_success 'replace symlink with normal file' '

Nice.  "replace symlink with executable" and "replace symlink
with directory" might also be interesting.

Regards,
Jonathan