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

Unit test for Mersenne primes #60

Closed
wants to merge 1 commit into from
Closed

Unit test for Mersenne primes #60

wants to merge 1 commit into from

Conversation

adamantike
Copy link
Contributor

@adamantike adamantike commented Mar 28, 2016

Checks Mersenne primes' primality. Tests bigger numbers, so it also checks Miller-Rabin's correct result for bigger bitsizes (and less rounds after merging #58).

It could help with #54, by giving more testcases and reducing the random factor affecting test coverage. 9a9e08c fixed this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 90.791% when pulling 0b7e2cf on adamantike:test-mersenne-primes into e5839ee on sybrenstuvel:master.

@sybrenstuvel
Copy link
Owner

Unit testing now takes much, much longer; it increased from ± 2 seconds to ± 24 seconds on my machine, for a single Tox environment. Please reduce the size of the numbers, and the number of numbers, to the minimum that's needed to perform the tests.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.351% when pulling 811eb04 on adamantike:test-mersenne-primes into 96eaa5e on sybrenstuvel:master.

@adamantike
Copy link
Contributor Author

Could you please test execution time again? I just removed testing for product of primes, and Travis times have returned to normal.

@sybrenstuvel
Copy link
Owner

On my little Linux server (which I use for development when my desktop is booted to Windows and I don't want to reboot) this patch still increases unittest time on a single Tox environment (py34) from 13 to 21 seconds. Are all these test cases really necessary?

@adamantike
Copy link
Contributor Author

I chose these test cases for testing primes up to the 4096-bit range! Reducing it to 2048-bit range saves 4 cases. Actually, we could remove last test case and still remain in 4096-bit range (because of 2^4253 - 1)!

Testing times on my hardware (py34 env):

  • master: 5.7s
  • Current PR 4096-bit: 11.5s (+101%)
  • Proposed PR 4096-bit: 8.6s (+50%)
  • PR 2048-bit: 6.2s (+8%)

IMHO, correct prime generation and validation is essential in RSA, so I think it worths the testing overhead, especially having Travis to handle unittest.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 20c1693 on adamantike:test-mersenne-primes into * on sybrenstuvel:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0b8c914 on adamantike:test-mersenne-primes into * on sybrenstuvel:master*.

@sybrenstuvel
Copy link
Owner

Yes, let's go for "Proposed PR 4096-bit", as I fully agree we should test 4096 bits!

@sybrenstuvel
Copy link
Owner

Merged in a72efaa

@adamantike adamantike deleted the test-mersenne-primes branch April 24, 2016 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants