On 04/01/2013 06:42 PM, Shivani Poddar wrote:
>
> Log:
> [[[ Followup to r1420334: Adding new tests.
>
> * subversiom/bindings/swig/python/tests/checksum.py
> Writing new tests to ensure robustness of svn_checksum_match(),
> svn_checksum_dup() and svn_checksum_empty_checksum().
>
> Patch by: Shivani Poddar <shivani.poddar92_at_gmail.com
> <mailto:shivani.poddar92_at_gmail.com>> ]]]
>
>
> Regards,
> Shivani Poddar,
> irc-nick - Pods
>
> 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)
> @@ -20,7 +20,7 @@
> #
> import unittest, setup_path
> import svn.core
> -
> +import random
Please preserve the blank line between the import statements and other code.
> class ChecksumTestCases(unittest.TestCase):
> def test_checksum(self):
> # Checking primarily the return type for the svn_checksum_create
> @@ -28,14 +28,56 @@
> 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)
> -
> + rand_checksum = svn.core.svn_checksum_t()
> + rand_checksum.kind = svn.core.svn_checksum_md5
> + #Generating a random digest value, length of which is 16 bytes for checkum kind = md5 checksum.
Local convention is to leave a space between '#' and start of comment.
"# Generate a random digest value ..."
> + rand_checksum.digest = ''.join(chr(random.randrange(256)) for x in xrange(16)).encode('base64')
This seems wrong. You got out of your way to to make 16-byte random string,
but then you base64-encode it -- which makes longer. What are you trying to
accomplish here?
> + rand_checksum2 = svn.core.svn_checksum_t()
> + rand_checksum2.kind = svn.core.svn_checksum_md5
> + rand_checksum2.digest = ''.join(chr(random.randrange(256)) for x in xrange(16)).encode('base64')
Same thing here.
> + #To check if both the random checksums are not identical
Comment style again.
> + while rand_checksum.digest == rand_checksum2.digest:
> + rand_checksum2.digest = ''.join(chr(random.randrange(256)) for x in xrange(16)).encode('base64')
I don't understand this block. In the event that the loop is entered, that
means ... what? That random.randrange() is pretty broken? So the code
says, "If my 'random' module is broken, keep looping until it isn't."
Doesn't make sense to me. But maybe I'm missing something obvious here.
> +
> + check_rand = svn.core.svn_checksum_to_cstring(rand_checksum)
> + duplicate = svn.core.svn_checksum_dup(rand_checksum)
> + duplicate_val = svn.core.svn_checksum_dup(val)
> + check_rand2 = svn.core.svn_checksum_to_cstring(rand_checksum2)
> + check_dup = svn.core.svn_checksum_to_cstring(duplicate)
> self.assertTrue(isinstance(check_val, str),
> - "Type of digest not string")
> + "Type of digest not string")
This formatting correction looks good ...
> self.assertEqual(len(check_val), 2*expected_length,
> - "Length of digest does not match kind")
> + "Length of digest does not match kind")
> self.assertEqual(int(check_val), 0,
> - "Value of initialized digest is not 0")
> + "Value of initialized digest is not 0")
... but here you've actually messed up the code alignment. These two
changed lines should have been left alone.
> + #Checking the svn_checksum_dup() function's returned object's attributes
> + self.assertEqual(type(rand_checksum),type(duplicate),
> + "Duplicate checksum returned is not of the type svn_checksum_t*")
> + self.assertEqual(rand_checksum.kind,duplicate.kind,
> + "Duplicate returned is not of the same svn_checksum_kind_t as the parent checksum")
> + self.assertEqual(check_rand,check_dup,
> + "Duplicate returned does not have the same digest value as the parent checksum")
> + #Checking svn_checksum_match()
> + #Unequal checksums rand_checksum and rand_checksum2
> + self.assertEqual(str(svn.core.svn_checksum_match(rand_checksum2,rand_checksum)),'0',
> + "Unequal checksums return match")
> + #Duplicate non zero checksums
> + self.assertIs(str(svn.core.svn_checksum_match(rand_checksum,duplicate)),'1',
> + "Equal Checksums return false for checksum match")
> + #Border case to check if match returns a zero for all checksums
> + self.assertIs(str(svn.core.svn_checksum_match(val,duplicate_val)),'1',
> + "Zero checksums should return 1 in svn_checksum_match")
> + #Checking for 1 zero and 1 non zero checksum
> + self.assertIs(str(svn.core.svn_checksum_match(val,rand_checksum)),'1',
> + "0 not returned for zero checksum argument in svn_checksum_match")
>
> + #Testing for svn_checksum_is_empty_checksum()
> + #Checking for an empty checksum
> + self.assertEqual(str(svn.core.svn_checksum_is_empty_checksum(val)),'1',
> + "False is returned for svn_checksum_is_empty_checksum for empty checksum")
> + #Checking for a non-empty checksum
> + self.assertEqual(str(svn.core.svn_checksum_is_empty_checksum(rand_checksum)),'0',
> + "True is returned for svn_checksum_is_empty_checksum for non-empty checksum")
Here we have more comment formatting issue, plus a bunch of lines which
exceed our 80-column preferred line limit and which don't have spaces
between function parameters. Do something such as this instead:
self.assertEqual(check_rand, check_dup,
"Duplicate returned does not have the same digest "
"value as the parent checksum")
I confess I'm a bit confused by the early parts of the test changes, which
appear to be more about testing the 'random' module and less about testing
Subversion. Further, we generally try to avoid non-repeatable tests, so we
like to be able to get pseudo-random data which can be reliable re-created
with a known seed. Here, if something fails in a way that happens to be
specific to your random data, I'm not sure how we'd go about debugging the
problem.
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Received on 2013-04-02 23:19:21 CEST