You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ramkumar Ramachandra <ar...@gmail.com> on 2010/07/23 22:03:53 UTC
svnrdump: New unittest
Hi,
Here's a patch for a simple unittest I wrote out- the zeroth revision
of the ASF repository passes with no issues. Since this is outside my
area, I need a +1 to commit this.
Thanks!
Index: subversion/tests/cmdline/svnrdump_tests.py
===================================================================
--- subversion/tests/cmdline/svnrdump_tests.py (revision 967194)
+++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
@@ -41,25 +41,75 @@ XFail = svntest.testcase.XFail
Item = svntest.wc.StateItem
Wimp = svntest.testcase.Wimp
-def basic_svnrdump(sbox):
- "dump the standard sbox repos"
- sbox.build(read_only = True, create_wc = False)
+######################################################################
+# Helper routines
+def build_repos(sbox):
+ """Build an empty sandbox repository"""
+ # Cleanup after the last run by removing any left-over repository.
+ svntest.main.safe_rmtree(sbox.repo_dir)
+
+ # Create an empty repository.
+ svntest.main.create_repos(sbox.repo_dir)
+
+def run_test(sbox, dumpfile_name = None, dumpfile_url = None):
+ """Load either a local or remote dumpfile using svnadmin load, dump
+ it with svnrdump and check that the same dumpfile is produced"""
+
+ # Create the empty master repository.
+ build_repos(sbox)
+
+ # Sanity check
r, out, err = svntest.main.run_svnrdump(sbox.repo_url)
if (r != 0):
raise svntest.Failure('Result code not 0')
if not out[0].startswith('SVN-fs-dump-format-version:'):
- raise svntest.Failure('No valid output')
+ raise svntest.Failure('Sanity check failed')
+ # This directory contains all the dump files
+ svnsync_tests_dir = os.path.join(os.path.dirname(sys.argv[0]),
+ 'svnrdump_tests_data')
+
+ # Load the specified dump file into the master repository.
+ if (dumpfile_name):
+ svnadmin_dumpfile = open(os.path.join(svnsync_tests_dir,
+ dumpfile_name),
+ 'rb').readlines()
+ else:
+ raise svntest.Failure('Unsupported test')
+
+ # Create the revprop-change hook for this test
+ svntest.actions.enable_revprop_changes(sbox.repo_dir)
+
+ # Load dumpfile_contents into the sbox repository
+ svntest.actions.run_and_verify_load(sbox.repo_dir, svnadmin_dumpfile)
+
+ # Create a dump file using svnrdump
+ r, svnrdump_dumpfile, err = svntest.main.run_svnrdump(sbox.repo_url)
+ if (r != 0):
+ raise svntest.Failure('Result code not 0')
+
+ svntest.verify.compare_and_display_lines(
+ "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
+
+######################################################################
+# Tests
+
+def asf_0(sbox):
+ "dump the zeroth revision of the ASF repository"
+
+ build_repos(sbox)
+ run_test(sbox, dumpfile_name = "asf-0.dump")
+
########################################################################
# Run the tests
# list all tests here, starting with None:
test_list = [ None,
- basic_svnrdump,
+ asf_0,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump
===================================================================
--- subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (revision 0)
+++ subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (working copy)
@@ -0,0 +1,30 @@
+SVN-fs-dump-format-version: 3
+
+UUID: 95f5730d-843b-4fe3-8879-f46da52f2123
+
+Revision-number: 0
+Prop-content-length: 258
+Content-length: 258
+
+K 8
+svn:date
+V 27
+2003-01-08T10:33:40.549533Z
+K 26
+svn:sync-currently-copying
+V 2
+15
+K 17
+svn:sync-from-url
+V 31
+http://svn.apache.org/repos/asf
+K 18
+svn:sync-from-uuid
+V 36
+13f79535-47bb-0310-9956-ffa450edef68
+K 24
+svn:sync-last-merged-rev
+V 2
+14
+PROPS-END
+
Re: svnrdump: New unittest
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi,
After Bert pointed out some problems with my patch on IRC, I've
updated it. Here's the new patch including the original basic_svnrdump
test as well.
Thanks.
Index: subversion/tests/cmdline/svnrdump_tests.py
===================================================================
--- subversion/tests/cmdline/svnrdump_tests.py (revision 978817)
+++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
@@ -41,6 +41,54 @@ XFail = svntest.testcase.XFail
Item = svntest.wc.StateItem
Wimp = svntest.testcase.Wimp
+######################################################################
+# Helper routines
+
+def build_repos(sbox):
+ """Build an empty sandbox repository"""
+
+ # Cleanup after the last run by removing any left-over repository.
+ svntest.main.safe_rmtree(sbox.repo_dir)
+
+ # Create an empty repository.
+ svntest.main.create_repos(sbox.repo_dir)
+
+def run_test(sbox, dumpfile_name):
+ """Load a dumpfile using svnadmin load, dump it with svnrdump and
+ check that the same dumpfile is produced"""
+
+ # Create an empty sanbox repository
+ build_repos(sbox)
+
+ # This directory contains all the dump files
+ svnsync_tests_dir = os.path.join(os.path.dirname(sys.argv[0]),
+ 'svnrdump_tests_data')
+
+ # Load the specified dump file into the repository
+ svnadmin_dumpfile = open(os.path.join(svnsync_tests_dir,
+ dumpfile_name),
+ 'rb').readlines()
+
+ # Create the revprop-change hook for this test
+ svntest.actions.enable_revprop_changes(sbox.repo_dir)
+
+ # Load dumpfile_contents into the sbox repository
+ svntest.actions.run_and_verify_load(sbox.repo_dir, svnadmin_dumpfile)
+
+ # Create a dump file using svnrdump
+ r, svnrdump_dumpfile, err = svntest.main.run_svnrdump(sbox.repo_url)
+
+ # Check error code
+ if (r != 0):
+ raise svntest.Failure('Result code not 0')
+
+ # Compare the output
+ svntest.verify.compare_and_display_lines(
+ "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
+
+######################################################################
+# Tests
+
def basic_svnrdump(sbox):
"dump the standard sbox repos"
sbox.build(read_only = True, create_wc = False)
@@ -53,6 +101,10 @@ def basic_svnrdump(sbox):
if not out[0].startswith('SVN-fs-dump-format-version:'):
raise svntest.Failure('No valid output')
+def asf_0(sbox):
+ "dump the zeroth revision of the ASF repository"
+ run_test(sbox, dumpfile_name = "asf-0.dump")
+
########################################################################
# Run the tests
@@ -60,6 +112,7 @@ def basic_svnrdump(sbox):
# list all tests here, starting with None:
test_list = [ None,
basic_svnrdump,
+ asf_0,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump
===================================================================
--- subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (revision 0)
+++ subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (working copy)
@@ -0,0 +1,30 @@
+SVN-fs-dump-format-version: 3
+
+UUID: 95f5730d-843b-4fe3-8879-f46da52f2123
+
+Revision-number: 0
+Prop-content-length: 258
+Content-length: 258
+
+K 8
+svn:date
+V 27
+2003-01-08T10:33:40.549533Z
+K 26
+svn:sync-currently-copying
+V 2
+15
+K 17
+svn:sync-from-url
+V 31
+http://svn.apache.org/repos/asf
+K 18
+svn:sync-from-uuid
+V 36
+13f79535-47bb-0310-9956-ffa450edef68
+K 24
+svn:sync-last-merged-rev
+V 2
+14
+PROPS-END
+
Re: svnrdump: New unittest
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sat, Jul 24, 2010 at 15:21:41 +0530:
> Daniel Shahaf writes:
> > Have you run 'make check'? :-)
>
> Yeah, but I usually execute the Python script raw from command-line
> when I want to test my changes. It currently doesn't really need all
> the setup that `make check` provides.
Yup. Running the tests manually over ra_local or ra_svn is trivial; for ra_dav,
you need to write a config file and start httpd manually, but that's about it.
(I can share the config file I use if you like)
Re: svnrdump: New unittest
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> > > First of all, please don't verify a 100k dumpfile during every 'make check'.
> > > It would take too long.
> >
> > Ah, I didn't think about that.
> >
>
> Have you run 'make check'? :-)
Yeah, but I usually execute the Python script raw from command-line
when I want to test my changes. It currently doesn't really need all
the setup that `make check` provides.
> > > Second, is it really necessary to have all of these? I'd assume that all edge
> > > cases you need to test can be fitted quite roomly in a 50-revision dumpfile.
> >
> > Do we already have a dumpfile with all the possible edge cases
> > somewhere in the tree?
> >
>
> svnsync_tests.py is probably what comes closest. (it also has some
> svnsync-with-authz tests, IIRC)
Okay.
-- Ram
Re: svnrdump: New unittest
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sat, Jul 24, 2010 at 13:37:35 +0530:
> I'll commit it now.
>
Please don't "Approved by: danielsh"; I haven't +1'd the patch yet.
(e.g., I haven't run it yet)
> > First of all, please don't verify a 100k dumpfile during every 'make check'.
> > It would take too long.
>
> Ah, I didn't think about that.
>
Have you run 'make check'? :-)
> > Second, is it really necessary to have all of these? I'd assume that all edge
> > cases you need to test can be fitted quite roomly in a 50-revision dumpfile.
>
> Do we already have a dumpfile with all the possible edge cases
> somewhere in the tree?
>
svnsync_tests.py is probably what comes closest. (it also has some
svnsync-with-authz tests, IIRC)
> note that I've written special code for handling revision 0, so it makes
> sense to test it.
Agreed.
Re: svnrdump: New unittest
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Good start. But you don't have a first sentence before the bullets (which we
> want for most non-small changes), and you aren't using the standard (function_name)
> syntax. So, perhaps along these lines:
>
> [[[
> Add a unit test for svnrdump dumping r0.
>
> * subversion/tests/cmdline/svnrdump_tests_data: Add a new directory to
> keep test data for svnrdump.
>
> * subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: Add a
> dumpfile of the zeroth revision of the ASF repository.
>
> * subversion/tests/cmdline/svnrdump_tests.py:
> (build_repos, run_test): New helper functions. ("Factored out from XXX()", if applicable)
> (asf_0): New test ("for XXX", if you like).
> (test_list): Run the new test.
> ]]]
Right. I'm slowly getting used to it :)
I'll commit it now.
> First of all, please don't verify a 100k dumpfile during every 'make check'.
> It would take too long.
Ah, I didn't think about that.
> Second, is it really necessary to have all of these? I'd assume that all edge
> cases you need to test can be fitted quite roomly in a 50-revision dumpfile.
Do we already have a dumpfile with all the possible edge cases
somewhere in the tree?
I'll rename asf-0 as a generic zeroth-revision test then; note that
I've written special code for handling revision 0, so it makes sense
to test it.
> (Specifically, I think that having a 10k and 100k tests is unnecessary given
> that you have a 1k test; and that the 1k test in itself could be much reduced.
> It's not the number of revisions that counts!)
Got it.
-- Ram
Re: svnrdump: New unittest
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sat, Jul 24, 2010 at 13:16:02 +0530:
> Daniel Shahaf writes:
> > Where is the log message?
>
> My proposed svn:log
> * subversion/tests/cmdline/svnrdump_tests_data: Add a new directory to
> keep test data for svnrdump.
> * subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: Add a
> dumpfile of the zeroth revision of the ASF repository.
> * subversion/tests/cmdline/svnrdump_tests.py: Add a new asf_0 test to
> test the dumpfile along with a run_test helper function.
>
Good start. But you don't have a first sentence before the bullets (which we
want for most non-small changes), and you aren't using the standard (function_name)
syntax. So, perhaps along these lines:
[[[
Add a unit test for svnrdump dumping r0.
* subversion/tests/cmdline/svnrdump_tests_data: Add a new directory to
keep test data for svnrdump.
* subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: Add a
dumpfile of the zeroth revision of the ASF repository.
* subversion/tests/cmdline/svnrdump_tests.py:
(build_repos, run_test): New helper functions. ("Factored out from XXX()", if applicable)
(asf_0): New test ("for XXX", if you like).
(test_list): Run the new test.
]]]
> > Why are you using r0 *of the ASF repository* --- is anything special about that
> > repository?
>
> No, but I plan to build up on this to verify 1k, 10k, 100k
> ... revisions of different repositories.
>
First of all, please don't verify a 100k dumpfile during every 'make check'.
It would take too long.
Second, is it really necessary to have all of these? I'd assume that all edge
cases you need to test can be fitted quite roomly in a 50-revision dumpfile.
(Specifically, I think that having a 10k and 100k tests is unnecessary given
that you have a 1k test; and that the 1k test in itself could be much reduced.
It's not the number of revisions that counts!)
> > Should we have run_and_verify_svnrdump()?
>
> Excellent suggestion. I'll get started working on it as soon as I
> commit this patch.
>
> -- Ram
Re: svnrdump: New unittest
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Stefan,
Stefan Sperling writes:
> (I guess you're getting the idea, but I'd still like to point out that,
> for new functions, it's OK to just say 'New function.'. But if you make
> further changes to the functions, just saying 'I changed this function.'
> is not detailed enough... :)
>
> Note also that writing good log messages needs a lot of practice and time.
> And there's a lot of personal opinion involved in deciding what makes a
> log message "good". At the same time, as a project we're trying to be very
> consistent in the way we write log messages. So the ground is a bit shaky
> for newcomers. Don't be offended when we criticise your log messages.
> Just try to follow existing examples and get used to the project's style
> of writing log messages. And soon you'll reach peoples' log message comfort
> level and they will stop nitpicking.
Thanks for the nitpick. I now see what's wrong with my log messages-
I'll pay more attention next time, and make sure that I don't write my
log messages in a hurry.
> Looks like you forgot to attach the new diff?
Oops :p
> Please refrain from adding regression tests that take a lot of time
> to execute, if possible. Our test suite already takes a long time to
> run as is. We want such tests, but they should be optional.
>
> But don't let this stop you from writing more tests.
> Unfortunately, we don't have a nice way of adding optional regression
> tests to our test suite, and I don't expect you to start working on
> that, too (there's enough on your plate as is).
> When you have new tests, post a diff, and we'll see what to do with it.
> Maybe it's time to finally address this problem in our test suite.
Got it. Yes, I'm doing too many things at once. Daniel taught me how
to use changelists, so I'll use them to keep track of what all I'm
doing.
-- Ram
Re: svnrdump: New unittest
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jul 24, 2010 at 01:16:02PM +0530, Ramkumar Ramachandra wrote:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Where is the log message?
>
> My proposed svn:log
> * subversion/tests/cmdline/svnrdump_tests_data: Add a new directory to
> keep test data for svnrdump.
> * subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: Add a
> dumpfile of the zeroth revision of the ASF repository.
> * subversion/tests/cmdline/svnrdump_tests.py: Add a new asf_0 test to
> test the dumpfile along with a run_test helper function.
I think your log message needs more granularity and syntax.
Please read
http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
again. We like having function and struct names affected by a commit
listed in log messages so you can easily search log messages for changes
people made to particular functions and structs.
I'd suggest something like:
[[[
Add a regression test for svnrdump which verifies that revision 0 of a
repository is dumped correctly. Revision 0 only has revision properties,
so it's somewhat special.
* subversion/tests/cmdline/svnrdump_tests.py
(build_repos, run_test): New helper functions.
(asf_0): New test.
(test_list): Add the new test.
* subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: A dump of
revision zero of the ASF repository as of late July 2010. Used as
test data in asf_0 test.
]]]
(I guess you're getting the idea, but I'd still like to point out that,
for new functions, it's OK to just say 'New function.'. But if you make
further changes to the functions, just saying 'I changed this function.'
is not detailed enough... :)
Note also that writing good log messages needs a lot of practice and time.
And there's a lot of personal opinion involved in deciding what makes a
log message "good". At the same time, as a project we're trying to be very
consistent in the way we write log messages. So the ground is a bit shaky
for newcomers. Don't be offended when we criticise your log messages.
Just try to follow existing examples and get used to the project's style
of writing log messages. And soon you'll reach peoples' log message comfort
level and they will stop nitpicking.
Also, what about renaming asf_0 to dump_revision_0?
> > Why are you removing basic_svnrdump()?
>
> My bad. Re-added.
Looks like you forgot to attach the new diff?
> > Why are you using r0 *of the ASF repository* --- is anything special about that
> > repository?
>
> No, but I plan to build up on this to verify 1k, 10k, 100k
> ... revisions of different repositories.
Please refrain from adding regression tests that take a lot of time
to execute, if possible. Our test suite already takes a long time to
run as is. We want such tests, but they should be optional.
But don't let this stop you from writing more tests.
Unfortunately, we don't have a nice way of adding optional regression
tests to our test suite, and I don't expect you to start working on
that, too (there's enough on your plate as is).
When you have new tests, post a diff, and we'll see what to do with it.
Maybe it's time to finally address this problem in our test suite.
> > Should we have run_and_verify_svnrdump()?
>
> Excellent suggestion. I'll get started working on it as soon as I
> commit this patch.
That's great. Please post an updated version of the patch, with a log
message (you can use one similar to what I wrote above if you want to).
Oh, and with [PATCH] in the subject line :)
Thanks,
Stefan
Re: svnrdump: New unittest
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Where is the log message?
My proposed svn:log
* subversion/tests/cmdline/svnrdump_tests_data: Add a new directory to
keep test data for svnrdump.
* subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump: Add a
dumpfile of the zeroth revision of the ASF repository.
* subversion/tests/cmdline/svnrdump_tests.py: Add a new asf_0 test to
test the dumpfile along with a run_test helper function.
> Why are you removing basic_svnrdump()?
My bad. Re-added.
> Why are you using r0 *of the ASF repository* --- is anything special about that
> repository?
No, but I plan to build up on this to verify 1k, 10k, 100k
... revisions of different repositories.
> Should we have run_and_verify_svnrdump()?
Excellent suggestion. I'll get started working on it as soon as I
commit this patch.
-- Ram
Re: svnrdump: New unittest
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Where is the log message?
Why are you removing basic_svnrdump()?
Why are you using r0 *of the ASF repository* --- is anything special about that
repository?
Should we have run_and_verify_svnrdump()?
Ramkumar Ramachandra wrote on Sat, Jul 24, 2010 at 03:33:53 +0530:
> Hi,
>
> Here's a patch for a simple unittest I wrote out- the zeroth revision
> of the ASF repository passes with no issues. Since this is outside my
> area, I need a +1 to commit this.
>
> Thanks!
>
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 967194)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -41,25 +41,75 @@ XFail = svntest.testcase.XFail
> Item = svntest.wc.StateItem
> Wimp = svntest.testcase.Wimp
>
> -def basic_svnrdump(sbox):
> - "dump the standard sbox repos"
> - sbox.build(read_only = True, create_wc = False)
> +######################################################################
> +# Helper routines
>
> +def build_repos(sbox):
> + """Build an empty sandbox repository"""
> + # Cleanup after the last run by removing any left-over repository.
> + svntest.main.safe_rmtree(sbox.repo_dir)
> +
> + # Create an empty repository.
> + svntest.main.create_repos(sbox.repo_dir)
> +
> +def run_test(sbox, dumpfile_name = None, dumpfile_url = None):
> + """Load either a local or remote dumpfile using svnadmin load, dump
> + it with svnrdump and check that the same dumpfile is produced"""
> +
> + # Create the empty master repository.
> + build_repos(sbox)
> +
> + # Sanity check
> r, out, err = svntest.main.run_svnrdump(sbox.repo_url)
>
> if (r != 0):
> raise svntest.Failure('Result code not 0')
>
> if not out[0].startswith('SVN-fs-dump-format-version:'):
> - raise svntest.Failure('No valid output')
> + raise svntest.Failure('Sanity check failed')
>
> + # This directory contains all the dump files
> + svnsync_tests_dir = os.path.join(os.path.dirname(sys.argv[0]),
> + 'svnrdump_tests_data')
> +
> + # Load the specified dump file into the master repository.
> + if (dumpfile_name):
> + svnadmin_dumpfile = open(os.path.join(svnsync_tests_dir,
> + dumpfile_name),
> + 'rb').readlines()
> + else:
> + raise svntest.Failure('Unsupported test')
> +
> + # Create the revprop-change hook for this test
> + svntest.actions.enable_revprop_changes(sbox.repo_dir)
> +
> + # Load dumpfile_contents into the sbox repository
> + svntest.actions.run_and_verify_load(sbox.repo_dir, svnadmin_dumpfile)
> +
> + # Create a dump file using svnrdump
> + r, svnrdump_dumpfile, err = svntest.main.run_svnrdump(sbox.repo_url)
> + if (r != 0):
> + raise svntest.Failure('Result code not 0')
> +
> + svntest.verify.compare_and_display_lines(
> + "Dump files", "DUMP", svnadmin_dumpfile, svnrdump_dumpfile)
> +
> +######################################################################
> +# Tests
> +
> +def asf_0(sbox):
> + "dump the zeroth revision of the ASF repository"
> +
> + build_repos(sbox)
> + run_test(sbox, dumpfile_name = "asf-0.dump")
> +
> ########################################################################
> # Run the tests
>
>
> # list all tests here, starting with None:
> test_list = [ None,
> - basic_svnrdump,
> + asf_0,
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (revision 0)
> +++ subversion/tests/cmdline/svnrdump_tests_data/asf-0.dump (working copy)
> @@ -0,0 +1,30 @@
> +SVN-fs-dump-format-version: 3
> +
> +UUID: 95f5730d-843b-4fe3-8879-f46da52f2123
> +
> +Revision-number: 0
> +Prop-content-length: 258
> +Content-length: 258
> +
> +K 8
> +svn:date
> +V 27
> +2003-01-08T10:33:40.549533Z
> +K 26
> +svn:sync-currently-copying
> +V 2
> +15
> +K 17
> +svn:sync-from-url
> +V 31
> +http://svn.apache.org/repos/asf
> +K 18
> +svn:sync-from-uuid
> +V 36
> +13f79535-47bb-0310-9956-ffa450edef68
> +K 24
> +svn:sync-last-merged-rev
> +V 2
> +14
> +PROPS-END
> +