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

Allow to specify the random number generator seed in unit tests #1751

Merged
merged 8 commits into from
Feb 20, 2025

Conversation

rjd15372
Copy link
Member

In this commit, we add a new parameter to the unit tests binary, called --seed, that allows to specify the seed used by the several random numbers generators used in the Valkey server code.

We also now print the seed information right before starting the unit tests execution, so that someone that needs to reproduce an error, can use the same seed to re-run the failed tests.

In this commit, we add a new parameter to the unit tests binary, called
`--seed`, that allows to specify the seed used by the several random
numbers generators used in the Valkey server code.

We also now print the seed information right before starting the unit
tests execution, so that someone that needs to reproduce an error, can
use the same seed to re-run the failed tests.

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@rjd15372 rjd15372 requested a review from zuiderkwast February 19, 2025 10:53
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 32.14286% with 19 lines in your changes missing coverage. Please review.

Project coverage is 71.11%. Comparing base (206f7f6) to head (c31fc79).
Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/util.c 32.14% 19 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1751      +/-   ##
============================================
- Coverage     71.14%   71.11%   -0.03%     
============================================
  Files           123      123              
  Lines         65534    65550      +16     
============================================
- Hits          46622    46614       -8     
- Misses        18912    18936      +24     
Files with missing lines Coverage Δ
src/server.h 100.00% <ø> (ø)
src/util.c 69.97% <32.14%> (-1.40%) ⬇️

... and 16 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Should we remove the random seed we do in test_hashtable.c and maybe some other test suites? Otherwise, it's still not reproducible...

@artikell
Copy link
Contributor

Maybe we can support a hook point for beforeTest and afterTest, which will help us restore the scene without affecting other cases.

@rjd15372
Copy link
Member Author

Should we remove the random seed we do in test_hashtable.c and maybe some other test suites? Otherwise, it's still not reproducible...

I think it does not matter. We are setting up the seed used by getRandomBytes in the beginning of the test. Therefore, the bytes generated in randomSeed of the test_hashtable.c test, will always be the same if we set the same seed.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Feb 19, 2025

We are setting up the seed used by getRandomBytes in the beginning of the test. Therefore, the bytes generated in randomSeed of the test_hashtable.c test, will always be the same if we set the same seed.

Are you sure? test_hashtable.c uses randomSeed uses getRandomBytes which takes from /dev/random, so i would guess it does matter.

@rjd15372
Copy link
Member Author

Are you sure? test_hashtable.c uses randomSeed uses getRandomBytes which takes from /dev/random, so i would guess it does matter.

But getRandomBytes only uses /dev/random to initialize the seed in the case that the seed has not been initialized before.
Since we are initializing the seed in the beginning of the test, the following calls to getRandomBytes will use the existing seed.

@zuiderkwast
Copy link
Contributor

Maybe we can support a hook point for beforeTest and afterTest, which will help us restore the scene without affecting other cases.

Do we need that? It seem like overkill for this purpose. I'd suggest we just remove all random seeding in each test suite and only do it once by the test framework. And print it in the beginning.

It's nice that this test framework is extremely simple, without scaffolding like beforeSuite, beforeTestCase.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Feb 19, 2025

@rjd15372 I see now that getRandomBytes only reads /dev/random once, so I think you're right that it's reproducible, but only if we run all tests together so getRandomBytes is called the same number of times. If we want to run a single test suite with --single and --seed, there can be a difference, right?

The test framework could can re-seed using the same seed before each test suite, maybe....

@rjd15372
Copy link
Member Author

Correct. It's only reproducible if you run the test with the same CLI arguments plus the seed arg. If you use the --single arg, it will not be the same.

Yeah, maybe re-seeding before each test run would allow us to run a single test. I'll update the PR.

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@rjd15372
Copy link
Member Author

rjd15372 commented Feb 20, 2025

@zuiderkwast what do you think of doing something similar to this PR for TCL tests?

@zuiderkwast
Copy link
Contributor

@zuiderkwast what do you think of doing something similar to this PR for TCL tests?

That's be great but it seems much harder. It would need to involve some server config or debug command to set the seed, right? And Lua's and TCL's internal seeds too?

A fixed hashtable seed per slot that's transferred on failover and slot migrations is something we've been thinking about in #4. It's a different purpose but it seems to be related and somewhat overlapping in parts of implementation I guess.

@rjd15372
Copy link
Member Author

@zuiderkwast what do you think of doing something similar to this PR for TCL tests?

That's be great but it seems much harder. It would need to involve some server config or debug command to set the seed, right? And Lua's and TCL's internal seeds too?

I was thinking about something simpler. Just set the seed on the TCL side. The server would still be random but it would be better than nothing.

Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@zuiderkwast zuiderkwast merged commit f3361cc into valkey-io:unstable Feb 20, 2025
51 checks passed
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.

3 participants