On Tue, Dec 11, 2012 at 4:08 AM, Daniel Shahaf <[email protected]> wrote:
> Shivani Poddar wrote on Tue, Dec 11, 2012 at 02:22:28 +0530: > > Log Message: > > > > Improve support for svn_checksum.h in SWIG bindings > > * subversion/bindings/swig/python/tests/checksum.py: Improved > test_checksum > > > > Need a blank line before the * line, and to use the "* file\n (symbol)" > syntax --- 'test_checksum' is a symbol. > > > Review by: danielsh > > > > That's inappropriate; I haven't reviewed the patch yet. You might add > this field after I review it, not before. > Since you reviewed the last patch because of which i wrote this 1 i thought that i needed to attribute you at review > > > Index: subversion/bindings/swig/python/tests/checksum.py > > =================================================================== > > --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694) > > +++ subversion/bindings/swig/python/tests/checksum.py (working copy) > > @@ -20,22 +20,25 @@ > > # > > import unittest, setup_path > > import svn.core > > - > > +LENGTH = 32 or 40; > > This is wrong in two different ways: > > - "32 or 40" is a constant expression that evaluates to 32. > > - You hardcode two values, when you should be hardcoding neither of > them. (The bindings shouldn't need to change if the C library grows > a third svn_checksum_kind_t.) > > the symbolic constants in python are declared as this one. However in this test, since we are checking by only svn_checksum_md5 , we only need the length to be >= 32, i dont know why we would want to include 40 in the length here , since atleast in this test length should always be 32. so maybe LENGTH = svn.core.svn_checksum_to_cstring_display(svn_checksum_create(svn_checksum_md5)) would have been a better thing to do > class ChecksumTestCases(unittest.TestCase): > > def test_checksum(self): > > # Checking primarily the return type for the svn_checksum_create > > # function > > val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5) > > check_val = svn.core.svn_checksum_to_cstring_display(val) > > - > > # The svn_checksum_to_cstring_display should return a str type > object > > # from the check_val object passed to it > > if(type(check_val) == str): > > - # The intialized value created from a checksum should be 0 > > - if(int(check_val) != 0): > > - self.assertRaises(AssertionError) > > + #Check the length of the string check_val > > + if(len(check_val) == LENGTH): > > + # The intialized value created from a checksum should > be 0 > > + if(int(check_val) != 0): > > + raise > > This bare "raise" statement without arguments is itself an error. > > See for yourself: > > % python -c 'raise' > TypeError: exceptions must be old-style classes or derived from > BaseException, not NoneType > > This exception signifies a bug in your program. It has become > a RuntimeError in recent Pythons (and, frankly, could become > a compile-time error as well --- the compiler knows there's no except: > block surrounding this statement). It might work, but not because it's > correct. > > Yes it was working for me in the program, will check how i can fix this one > Daniel > > > + else: > > + raise > > else: > > - self.assertRaises(TypeError, test_checksum) > > + raise > > > > def suite(): > > return > unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) > > -- Shivani Poddar, Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad
