On Tue, Feb 19, 2013 at 11:55 PM, Ben Reser <b...@reser.org> wrote: > On Sat, Feb 16, 2013 at 4:26 AM, Shivani Poddar > <shivani.podda...@gmail.com> wrote: > > [ [ [ > > > > Improving support for svn_checksum.h in SWIG bindings. > > This isn't how I'd word this commit description. You haven't actually > improved support at all with this patch, but rather you've added tests > for a function that already works. That doesn't mean it's not a > worthwhile contribution (it is), it just isn't what is described. > > > Index: subversion/bindings/swig/python/tests/checksum.py > > =================================================================== > > --- subversion/bindings/swig/python/tests/checksum.py (revision 1446877) > > +++ subversion/bindings/swig/python/tests/checksum.py (working copy) > > @@ -18,9 +18,10 @@ > > # under the License. > > # > > # > > +import sys > > import unittest, setup_path > > import svn.core > > Why import sys you're not using it anywhere? > > Will check the way the above is worded in the patch and also, yes, I need not import sys. Maybe writing "adding tests for svn_checksum_dup() function in svn_checksum.h would be the correct thing to do.
> > - > > +LENGTH = > svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) > > class ChecksumTestCases(unittest.TestCase): > > def test_checksum(self): > > # Checking primarily the return type for the svn_checksum_create > > I'd have probably put LENGTH at least inside the ChecksumTestCases > class. There's no reason for it to be a global. But really I don't > understand why it's here at all, see below for more on that. > > Since in the earlier patches we had LENGTH as a global variable, I did not feel the need to change it here. > > @@ -28,7 +29,12 @@ class ChecksumTestCases(unittest.TestCase): > > kind, expected_length = svn.core.svn_checksum_md5, 128/8 > > val = svn.core.svn_checksum_create(kind) > > check_val = svn.core.svn_checksum_to_cstring_display(val) > > + is_duplicate = svn.core.svn_checksum_dup(val); > > is_duplicate implies to me that this is the result of a test to check > if something is a duplicate, so on a glance I'd be expecting > is_duplicate to be a boolean. But rather it's a svn_checksum_t. I'd > have just named it duplicate. > > Yes, didn't perceive that naming might indicate otherwise. (Will rectify) > > > > + self.assertEqual(type(check_val),str,"Type of digest not > string") > > + self.assertEqual(len(check_val)%LENGTH,0,"Length of digest does > not match kind") > > + self.assertEqual(int(check_val),0,"Value of initialized digest > is not 0") > > + > > self.assertTrue(isinstance(check_val, str), > > "Type of digest not string") > > self.assertEqual(len(check_val), 2*expected_length, > > Why is this here? Thought we were testing the duplicate, this code is > testing the check_val and is essentially a duplicate of the the > assertions it precedes. Modulus is not the right operation to test > the length. In fact now that I think about this. This looks > familiar... > > Yes these lines and the LENGTH appears to be from this patch: > > http://mail-archives.apache.org/mod_mbox/subversion-dev/201212.mbox/%3CCAFFzEcM2B_T55m2BQ95_2K_W=duxfkwdmdcjo8fctodyoku...@mail.gmail.com%3E > > Which was committed by danielsh with some modifications and then > ultimately modified further based on some feedback on the list. > > Yes, this is merely the same. I did not rewrite it this time around. I am not sure why would these lines come with a (+) in the patch. Using modulus here was to tackle the different types of svn_checksum_kind_t we have. This was deliberated at earlier when danielsh reviewed that patch. > > @@ -36,6 +42,8 @@ class ChecksumTestCases(unittest.TestCase): > > self.assertEqual(int(check_val), 0, > > "Value of initialized digest is not 0") > > > > + self.assertTrue(type(val) is type(is_duplicate), > > + "Type of return value not svn_checksum_t*") > > I'd think you'd want to test more than just that the duplicated value > has the same type, but you'd also want to know that it has the same > values. Your error message says that the type returned is not a > svn_checksum_t * but you've never actually tested that. What if both > the create and the dup both return some other incorrect type? > Writing this test was on the assumption that we have checked that the type returned by svn_checksum_create() is already svn_checksum_t* , and so for any type(x) being equal to it's type returns us svn_checksum_t* only. Also equating the values of the 2 also and not just the type would have been a better call. (Will rectify that) Thanks a lot for the review :) Regards, Shivani
