On Tue, Feb 19, 2013 at 10:01 PM, Shivani Poddar <[email protected]> wrote: > > [ [ [ > > Improving support for svn_checksum.h in SWIG bindings.
Same comment as last time about the description line. > * subversion/bindings/swig/python/tests/checksum.py: "swig-py: Adding tests > for > > svn_checksum_dup() and svn_checksum_match()" This isn't properly formatted per our commit log style. We don't use quotes and we don't leave blank lines. > Index: subversion/bindings/swig/python/tests/checksum.py > =================================================================== > --- subversion/bindings/swig/python/tests/checksum.py (revision 1448005) > +++ subversion/bindings/swig/python/tests/checksum.py (working copy) > @@ -27,15 +27,20 @@ > # function > kind, expected_length = svn.core.svn_checksum_md5, 128/8 > val = svn.core.svn_checksum_create(kind) > + duplicate = svn.core.svn_checksum_dup(val) > check_val = svn.core.svn_checksum_to_cstring_display(val) > - > + #create manual duplicate of val to check svn_checksum_match() > + dup_val = val > self.assertTrue(isinstance(check_val, str), > "Type of digest not string") > self.assertEqual(len(check_val), 2*expected_length, > "Length of digest does not match kind") > self.assertEqual(int(check_val), 0, > "Value of initialized digest is not 0") > - > + self.assertTrue(isinstance(svn.core.svn_checksum_match(val,dup_val), > int),"Type returned is not int(0/1) or boolean") Don't abuse line wrapping to deal with overly wrong lines, just continue the code on the next line and use spaces to shift it to an appropriate indentation. Your description and the test you're running don't equate. You aren't checking that it is a boolean or that if it is an int is only 0 or 1. Somewhat surprised that the Python don't seem to be mapping our svn_boolean_t to the Python boolean values but that's not really your issue. > + #check for the function svn_checksum_dup() independently > + self.assertTrue(svn.core.svn_checksum_match(val,duplicate), > + "Argument 2 is not a duplicate of argument 1") > def suite(): > return > unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) I'd say that your description for this assert could be more useful. svn_checksum_match() probably doesn't do what you think it does. It treats an all zero checksum as always matching as long as the kinds are the same. So you can't use it to make sure that svn_checksum_dup() works especially when you're only creating checksums with create() that always returns all zero checksums. Let's say that dup() is not working for some reason and it returns a checksum that has a different value, the match() function would still return true since your original checksum had all zeros. See the documentation here: http://people.apache.org/~hwright/svn/doc/api/trunk/svn__checksum_8h.html#a417244d5739a22f9a18e440f4f43d12d I think this is a good point to start accessing the values in the svn_checksum_t object directly.
