On Thu, Feb 21, 2013 at 5:31 AM, Shivani Poddar <[email protected]>wrote:
> > > On Wed, Feb 20, 2013 at 11:41 PM, Ben Reser <[email protected]> wrote: > >> 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. >> > > Will fix.. > > >> >> 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. >> > > > The function here is a boolean function, so I assumed that checking for > int (0/1) would be sufficient. What I comprehend is that there is a need > for me to add a check to if those values being returned are 0 or 1. > > >> > + #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. >> > > What my instant intuition says is that for the svn_checksum_t* to be a > duplicate of one another the kind, digest length (since the digest value > itself will not be the same), pool of the respective objects needs to be > the same. > Would that be a correct approach?? > A possible stub could be : > > *if ( val.kind is duplicate.kind ) and (val.digest_len is > duplicate.digest_len) -> then they are duplicates.* > > Since the pool allocated remains fixed here. > Also there is a mismatch in the digest.this for the 2 svn_checksum_t > objects val and duplicate, will attributing this to the different memory > locations of (digest.this) be the correct thing to do?? > Matching the length of the digest here doesn't exactly seem the best > approach to me here ..:\ > > In light of the feedback given above, The best possible solution to me is to match the digest strings for the both the svn_checksum_t objects. But even so the svn_checksum_create() only gives me digests intialized to 0 , meaning that whatever objects I have will match with the given test. Is there any suggestion as to would doing this anyways be right??
