Change in vdsm[master]: storageServer: Add tests for equality and hash

nsoffer at redhat.com nsoffer at redhat.com
Sun Jul 26 10:49:47 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storageServer: Add tests for equality and hash
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/43988/2//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2015-07-25 22:51:48 +0300
Line 6: 
Line 7: storageServer: Add tests for equality and hash
Line 8: 
Line 9: Current __eq__ and __hash__ are wrong and have no tets. Add tests
> tets->tests
Will fix in next version
Line 10: showing what works and what not.
Line 11: 
Line 12: To make the tests results more clear, implement __str__. This can also
Line 13: be useful when logging the objects.


https://gerrit.ovirt.org/#/c/43988/2/tests/storageServerTests.py
File tests/storageServerTests.py:

Line 120:         ("s1", "s2", "t", "t", "o", "o"),
Line 121:         ("s", "s", "t1", "t2", "o", "o"),
Line 122:         ("s", "s", "t", "t", "o1", "o2"),
Line 123:     ])
Line 124:     def test_not_equal_diffrent_hash(self, s1, s2, t1, t2, o1, o2):
> Why do we need this test ?
The contract is right, but this hash function is ok according to this contract:

    def __hash__(self):
        return 0

So whats missing in the contract is:

   3. hash should use all relevant object state and use a good hash function.

These tests are very low bar, if the code cannot pass them either it does not use all object state, or the hash function is probably horrible.

The implementation I added elsewhere (see https://gerrit.ovirt.org/43978)  use hash((object.state1, object.state2, ...)), which use the underlying Python builtin hash functions (tuple, str...), which are pretty good.

It is true that this test may fail because of hash collision, but this is highly unlikely, so I think we will never get false negatives here.
Line 125:         c1 = MountConnection(s1, t1, o1)
Line 126:         c2 = MountConnection(s2, t2, o2)
Line 127:         self.assertNotEqual(hash(c1), hash(c2))
Line 128: 


Line 120:         ("s1", "s2", "t", "t", "o", "o"),
Line 121:         ("s", "s", "t1", "t2", "o", "o"),
Line 122:         ("s", "s", "t", "t", "o1", "o2"),
Line 123:     ])
Line 124:     def test_not_equal_diffrent_hash(self, s1, s2, t1, t2, o1, o2):
> diffrent->different
Will fix in next version.
Line 125:         c1 = MountConnection(s1, t1, o1)
Line 126:         c2 = MountConnection(s2, t2, o2)
Line 127:         self.assertNotEqual(hash(c1), hash(c2))
Line 128: 


-- 
To view, visit https://gerrit.ovirt.org/43988
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1dc9510a9f0f6c44534fd568cbcda161f36497c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list