Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

better test coverage for internal modules #146

Merged
merged 13 commits into from
Oct 31, 2019

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Oct 27, 2019

preparation for #127
make the test coverage more readable and make it use pytest or unittest asserts for checks
where possible, also use pytest parametrisation or hypothesis

@tomato42 tomato42 added the maintenance issues related to making the project usable or testable label Oct 27, 2019
@tomato42 tomato42 self-assigned this Oct 27, 2019
@coveralls
Copy link

coveralls commented Oct 27, 2019

Coverage Status

Coverage increased (+1.9%) to 94.858% when pulling 89fe31d on tomato42:ecdsa-test-cov into fa7655c on warner:master.

@tomato42 tomato42 changed the title better test coverage for ecdsa module better test coverage for internal modules Oct 27, 2019
@tomato42 tomato42 force-pushed the ecdsa-test-cov branch 5 times, most recently from abc8202 to f4da621 Compare October 28, 2019 11:51
@tomato42 tomato42 mentioned this pull request Oct 28, 2019
@tomato42 tomato42 added this to the v0.14 milestone Oct 28, 2019
@tomato42 tomato42 force-pushed the ecdsa-test-cov branch 5 times, most recently from 5ac843b to 6f712e1 Compare October 30, 2019 13:28
@tomato42 tomato42 marked this pull request as ready for review October 30, 2019 13:29
don't use six.print_ - not needed on py2.6
sort imports in conventional way
make the test with different keys, messages and nonces reproducible

use all the curves/generators defined in the test (not only P-192)
use pytest.parameterize to execute the Known-Answer Tests
for the NIST P-256 curve

use a bit more sane formatting for the messages in the tests
use hypothesis (and larger numbers) for testing the inverse_mod
function
make the jacobi test into a hypothesis test
increase the range tested

add test to verify factorization, since it's used in the jacobi test
extend the testing for large primes with hypothesis

use pytest.parametrize to test with small primes
move sanity checks for gcd() to unittest

add hypothesis tests for gcd
the function is just a wrapper around pow() builtin, so don't use
it, mark it deprecated, exclude from coverage (as we're not testing it)
add hypothesis test to lcm
Copy link

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the tests sound a bit redundant, when all you are doing is testing if the math library works correctly, but besides that I see no issues with this PR.

Comment on lines 182 to 183
"Hypothesis 2.0.0 can't be made tolerant of hard to "
"meet requirements (like `is_prime()`)")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure I understand what this means, might be clarified, for people not too familiar with hypothesis

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, what is the actual issue ?

Copy link
Member Author

@tomato42 tomato42 Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you select random integers, few of them will be primes, so the test case generation takes a lot of time; hypothesis doesn't like it when it takes few seconds
with new versions, we can disable those checks, but with old (the oldest supported on py2.6) we can't

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleas mention that the issue is a time issue then.

@tomato42
Copy link
Member Author

when all you are doing is testing if the math library works correctly

yes, the maths library that is part of this package... there is no lcm(), gcd() or inverse_mod() in standard python... or am I missing something?

@simo5
Copy link

simo5 commented Oct 31, 2019

when all you are doing is testing if the math library works correctly

yes, the maths library that is part of this package... there is no lcm(), gcd() or inverse_mod() in standard python... or am I missing something?

There is math.gcd() but lcm() is indeed not there, although lcm() is so trivial to create once you have gcd(). I thought lcm() was also there but now that I look at it, it turns out I also implemented it recently for some test elsewhere :-D
So my comment I guess apply only to gcd() at this point.
It's not a big deal.

@tomato42
Copy link
Member Author

There is math.gcd()

Indeed, but not in older versions:
https://docs.python.org/3/library/math.html#math.gcd:

New in version 3.5.

probably we can use it to speed up newer versions, but the old code needs to remain

ok, thanks for the review!

@tomato42 tomato42 merged commit 8a4a1f9 into tlsfuzzer:master Oct 31, 2019
@tomato42 tomato42 deleted the ecdsa-test-cov branch October 31, 2019 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues related to making the project usable or testable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants