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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
