Added __eq__ method to classes to improve comparison abilities #161
Conversation
tomato42
left a comment
tomato42
left a comment
There was a problem hiding this comment.
sorry for insisting on 100% line coverage, but typos in python code won't be caught until the line is actually executed and the dereference fails
src/ecdsa/ellipticcurve.py
Outdated
|
|
||
| def __eq__(self, other): | ||
| if isinstance(other, CurveFp): | ||
| """Return True if the points are identical, False otherwise.""" |
src/ecdsa/test_ecdsa.py
Outdated
|
|
||
| assert not pubkey.verifies(msg - 1, signature) | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: whitespace at the end of file
src/ecdsa/test_ecdsa.py
Outdated
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| pass |
src/ecdsa/test_ellipticcurve.py
Outdated
| c = CurveFp(100, -3, 100) | ||
| p = Point(c, 100, 100, 100) | ||
| self.assertNotEqual(self.g_23, p) | ||
|
|
There was a problem hiding this comment.
nit: whitespace at end of file
| self.assertEqual(self.c_23, CurveFp(23, 1, 1)) | ||
|
|
||
| def test_inequality_curves(self): | ||
| c192 = CurveFp(p, -3, b) |
There was a problem hiding this comment.
the other test cases use global variables because they use the functional style and the pytest framework, this one uses the object-based approach from unittest, so I'd say that the use of global variables is less clean here...
There was a problem hiding this comment.
Moved to classes. Not sure if it is possible to convert test_p192_mult_tests preserving functionality.
There was a problem hiding this comment.
I mean, I'm fine with either approach, what I'm not a fan of is mixing them
|
side note: it looks like you have not associated the email you used in commits with your github account, you may want to add it Settings→Emails |
src/ecdsa/test_ellipticcurve.py
Outdated
| # From X9.62 I.1 (p. 96): | ||
| def test_add_and_mult_equivalence(self): | ||
| for n, exp in enumerate(self._add_n_times(self.g_23, 8)): | ||
| self.assertEqual(self.g_23 * n, exp) |
There was a problem hiding this comment.
the tests were using pytest decorators because then every test case is considered a separate instance, so a failure in one won't stop the others from executing
in new unittest it's the "subtest" mechanism, but I don't know if it was backported to earlier pythons...
I think it's easier to leave them as pytest tests
src/ecdsa/test_ellipticcurve.py
Outdated
|
|
||
| data = [(3, 10, 9, 7, 17, 20), # real add | ||
| (3, 10, 3, 10, 7, 12)] # double | ||
| for (x1, y1, x2, y2, x3, y3) in data: |
There was a problem hiding this comment.
same here, failure in "real add" will cause the "double" case to be skipped
| self.assertEqual(self.c_23, CurveFp(23, 1, 1)) | ||
|
|
||
| def test_inequality_curves(self): | ||
| c192 = CurveFp(p, -3, b) |
There was a problem hiding this comment.
I mean, I'm fine with either approach, what I'm not a fan of is mixing them
tomato42
left a comment
tomato42
left a comment
There was a problem hiding this comment.
test coverage for SigningKey and VerifyingKey missing for case with object of different type
4 participants
No description provided.