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

crypto/sha512: move sha512 long message tests to separate repo #22208

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

kimshrier
Copy link
Contributor

As discussed in 22187, the long message tests have been moved to a separate repo. I have left the short message tests here since the test cases are easily accommodated in the test source file.

Even though the long message tests in sha256 and sha1 did not cause any compile time errors with MSVC, I can do the same organization that I did for sha512 and move the long message tests for sha256 and sha1 over to the slower_tests repo.

@spytheman
Copy link
Member

Moving them worked.
The CI cloned the other repo, and the tests there are run successfully:
image

@spytheman spytheman merged commit 5cf4b5e into vlang:master Sep 13, 2024
67 checks passed
@kimshrier
Copy link
Contributor Author

I'm glad I didn't mess things up too badly.

Really appreciate the assistance.

@spytheman
Copy link
Member

Really appreciate the assistance.

I am happy to help when I can.
I hope that splitting the more voluminous and slower tests will be a good balance, between:

  • the size of the released packages
  • the speed for V developers, that have to run v test-all locally for unrelated changes to other modules
  • the need for running the more extensive tests before/after merging in the main V repo, to ensure a better quality of the modules, that do have more extensive tests.

@spytheman
Copy link
Member

What do you think of copying vlib/crypto/pbkdf2/pbkdf2_test.v into https://github.com/vlang/slower_tests too?
A reduced version of it (with just the lower count/faster 'test case 1' and 'test case 2'), could remain in vlib/crypto/pbkdf2/pbkdf2_test.v , to serve as a quick test of the functionality.

For me locally, the current full version, takes over 30 seconds to run without optimizations on, and over 10 seconds with -prod, while with just the first 2 test cases, it takes <1s without optimizations and ~6s with (most of the time is spend in the compilation):
image

@kimshrier
Copy link
Contributor Author

I have no problem with doing that. We should have at least 1 test in the reduced pbkdf2_test.v where the key_length is greater than the block size of the hash. That was what revealed the bug I ran in to.

I can generate a test case with a 256 byte key_length but fewer rounds. Something like c = 16 shouldn't take too long. What do you think?

@spytheman
Copy link
Member

Very well then.
I think that single _test.v files that run below 2-3 seconds are fine.

@kimshrier
Copy link
Contributor Author

I have run out of time to do this right now but I will get to it tomorrow.

@spytheman
Copy link
Member

spytheman commented Sep 13, 2024

In 56013a4, I've left 'test case 7', which has key_length > block_size for sha256 and sha224 afaik.

The full version of it, is in https://github.com/vlang/slower_tests now.

#0 10:57:35 ^ master /space/v/oo>v repeat './v vlib/crypto/pbkdf2/pbkdf2_test.v'
856.3ms ± σ:   6.8ms, 849.2ms…869.8ms, 1 series, 10 runs for ./v vlib/crypto/pbkdf2/pbkdf2_test.v

(half of that time is compilation)

@spytheman
Copy link
Member

image

#0 11:01:38 ^ master /space/v/oo>du -bs vlib/crypto/
2395214 vlib/crypto/

There are not too obvious outliers in size/runtime in the crypto module anymore.

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.

2 participants