On Sat, 24 Jun 2006 00:15:05 +0530, David James <djames@collab.net> wrote:
> On 6/23/06, Madan U Sreenivasan <madan@collab.net> wrote:
>> Hi,
>>
>> Pl. find attached some more tests for the svn.wc python bindings.
>
> Nice patch, Madan! A few minor issues:
>
> 1. Instead of using failUnless and failUnlessRaises, please use
> assert_ and assertRaises. This is more consistent with our existing
> test cases.
I had a discussion with Jelmer on this on irc (We had discussed a bit of
this on the dev@ list too, in the thread starting
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=116865, and
specifically at
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=116911)
The summary : Jelmer wanted to use assert...() functions, and I wanted to
use fail...() functions wherever available. My rationale behind it is
simple: fail...() makes more sense when we write a test case. Its self
explanatory to say failIf() than to say assert_(). The statment "A test
case failed" is more natural than the computing jargon "A test case
asserted". :)
> 2. Instead of saying 'failUnless(x == y)", use "assertEqual(x, y)".
didn't we decide not to use assertEqual in favor of assert_() because its
available only in the recent versions of python?
> This outputs a better error message if the test fails.
Really? I thought these were synonyms.
http://docs.python.org/lib/testcase-objects.html uses the same description
for both these functions.
> 3. For the test case which throws SubversionException, please add a
> comment. It should say something like "Ensure that setting svn:entry
> props is not allowed". I trust that this behavior is correct (if not,
> add a TODO explaining how it should normally behave.)
Okay, shall I assume this has to be specifically done for all failure test
cases in future?
> After these issues are fixed, +1 to commit.
Cool. Thanks for the review.
>> btw, I almost completed implementing XFail() for the bindings, when
>> I
>> realized that assertRaises()/failUnlessRaises() do the same thing. Why
>> do
>> we need XFail for the python bindings tests?
>
> Good point. Actually, now that I think about it, assertRaises is
> probably better than XFail, because you can test what exception is
> raised. All tests which test for incorrect behaviour should contain a
> TODO marker explaining what's wrong and how they should be fixed.
yeah, the TODO marker is important. Sometime next week, I will write a
small convention document for the python tests (or maybe add it to the
existing README) including this point.
Regards,
Madan U S
www.symonds.net/~madan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jun 23 21:16:25 2006