Skip to content

Replace deterministic random values with actual randomness in tests #3000

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bvkatwijk
Copy link
Contributor

@bvkatwijk bvkatwijk commented Mar 12, 2025

  • Use random numbers instead of fixed sequence in loop

Two tests used identical fixed random instances inside a loop, causing the extact same sequence of numbers being generated:

        for (int i = 0; i < 10; i++) {
            final Random random = getRandom(123456789);
                // ...
                final int value = random.nextInt(size);
                // ...
        }

Each iteration will generate a fixed sequence (in this case: 2554, 3297, 4789, ...)
Unless I am missing the purpose, this means that each loop is identical and thus redundant.
Generating the same sequence of numbers via a Random does not seem to be the intented design here.

If my assumptions are correct, this fixes both issues by using an unseeded random instance, creating a test run with actual randomness.

If each loop iteration should be different but deterministic values are desired, a different fix could be to move the random instance out of the loop:

        final Random random = getRandom(123456789);
        for (int i = 0; i < 10; i++) {
                // ...
                final int value = random.nextInt(size);
                // ...
        }

@bvkatwijk bvkatwijk requested a review from pivovarit as a code owner March 12, 2025 08:59
@pivovarit pivovarit force-pushed the make-random-actually-random branch from 04fbcf3 to 552efc7 Compare May 24, 2025 07:31
@pivovarit
Copy link
Member

I agree that there's a problem, but I don't agree with the solution - this is an attempt to introduce property-based-like tests, but it's missing the point: those tests should be random, but with an ability to make them deterministic during debugging by passing a custom seed.

So the solution would be to:

  • pass negative seed to let it randomize runs
  • introduce parameterless getRandom() alongside the existing method and use it in tests

Your call :)

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