On Tue, Feb 19, 2013 at 11:55 PM, Ben Reser <ben_at_reser.org> wrote:
> On Sat, Feb 16, 2013 at 4:26 AM, Shivani Poddar
> <shivani.poddar92_at_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=DUXfKWdmdCjO8fctODyoku9tQ@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
Received on 2013-02-19 19:59:13 CET