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
Received on 2010-07-24 10:52:52 CEST