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?
> -
> +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.
> @@ -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.
>
> + 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.
> @@ -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?
Received on 2013-02-19 19:25:46 CET