Streamline random generation #3100
Conversation
WalkthroughThe recent updates across the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
|
||
| private SafeRandoms() {} | ||
|
|
||
| public static int nextInt(int bound) { |
There was a problem hiding this comment.
Why do you use util method instead of an object?
There was a problem hiding this comment.
I didnt quite understand the concern here. Can you please elaborate?
There was a problem hiding this comment.
SafeRandom random = new SafeRandom();
int value = random.nextInt();
There was a problem hiding this comment.
Can you let me know what is the issue with the current implementation? You are suggesting inheritance and I am making use of composition to do the same thing. I am not sure if understand the benefits of having to use inheritance here.
I need a random number which is NOT zero. Why wouldnt a utility method suffice here?
There was a problem hiding this comment.
Utils are imperative programming, not composition.
Composition here will be to create your own random that create another random.
Inheritance here will be to create your own random that extends another random.
Both approach are oop and valid.
There was a problem hiding this comment.
Inheritance is not going to allow me to use another flavor of random (if there's a need for it). The current implementation works with its own random implementation if that's what is needed but it also helps with allowing a user to inject the random as a dependency. I was hinting at composition in that aspect as in I have a randomness object with me which I believe is what composition stands for (has a vs is a relationship).
My point here is that, we don't need an OOP approach here but just a plain vanilla utility that helps in generating random numbers that aren't zero.
There was a problem hiding this comment.
That's the point. Everything is possible with utils methods. But why is jdk providing an object for random instead of few static methods?
There was a problem hiding this comment.
The JDK provides classes for random because there can be multiple types of randoms being used. I am using a static method because it can take any different type of random.
I am still not understanding as to what is wrong with using a utility method for a test and why does it need to be a class that uses inheritance. Its only used in tests. Cant we not use a utility method here? If there are any functional or performance discrepancies I can understand. But this is just a different programming style. I am using a utility method to get it done while you prefer using an object. If there are no pitfalls in terms of using a utility method then I would suggest that we merge this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (2 hunks)
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (3 hunks)
- testng-core/src/test/java/test/support/SafeRandoms.java (1 hunks)
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (2 hunks)
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2 hunks)
Additional comments: 6
testng-core/src/test/java/test/support/SafeRandoms.java (1)
- 8-8: Using
SecureRandomas the default random number generator inSafeRandomsis a good choice for security-sensitive applications. However, it's worth noting thatSecureRandomcan be significantly slower thanRandom. If the primary use case ofSafeRandomsdoes not involve security-sensitive random numbers, consider allowing the use ofRandomfor scenarios where performance is more critical.testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (1)
- 14-14: The replacement of
random.nextInt(10)withSafeRandoms.nextInt(random, 10)inThread.sleepis correctly implemented and aligns with the PR's objectives. This change ensures that the sleep duration is never zero, which could be important for the test's logic.testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (1)
- 31-31: The use of
SafeRandoms.nextInt(500)to generate sleep times in milliseconds is correctly implemented. However, it's important to note that the original code likely intended to useSecureRandom, which is not directly passed toSafeRandoms.nextInthere. Ensure that the security level of random number generation is consistent with the original intent.testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (1)
- 31-31: The replacement of direct random number generation with
SafeRandoms.nextInt(500)for sleep times is correctly implemented. This ensures non-zero sleep durations, aligning with the PR's objectives. As withTestClassSample, consider the security implications of the random number generation method used.testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (1)
- 39-39: The use of
SafeRandoms.nextInt(10)to generate a multiplier for the sleep duration is correctly implemented. This change ensures that the sleep duration is never zero, which could be important for the test's logic. However, consider the impact of multiplying by a random number, especially since the range is now 1-9 instead of 0-9, which slightly alters the behavior.testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (1)
- 51-51: The replacement of direct random number generation with
SafeRandoms.nextInt(100)for determining sleep duration is correctly implemented. This ensures non-zero sleep durations, aligning with the PR's objectives. As with other files, consider the impact of the change on the test's behavior, especially in terms of the distribution of sleep durations.
I didnt understand what did you mean by this. |
krmahadevan
force-pushed
the
fix_randoms
branch
from
ec2c17b
to
b3e1f8e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (2 hunks)
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (3 hunks)
- testng-core/src/test/java/test/support/SafeRandoms.java (1 hunks)
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (2 hunks)
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java
- testng-core/src/test/java/test/support/SafeRandoms.java
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java
krmahadevan
force-pushed
the
fix_randoms
branch
from
b3e1f8e
to
9fa3167
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (2 hunks)
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (3 hunks)
- testng-core/src/test/java/test/support/SafeRandoms.java (1 hunks)
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (2 hunks)
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java
- testng-core/src/test/java/test/support/SafeRandoms.java
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java
* Add edit checks to ensure that when using Random it always gives a non zero value
krmahadevan
force-pushed
the
fix_randoms
branch
from
9fa3167
to
5769631
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (6)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (2 hunks)
- testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (3 hunks)
- testng-core/src/test/java/test/support/SafeRandoms.java (1 hunks)
- testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (2 hunks)
- testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2 hunks)
Additional Context Used
Additional comments not posted (11)
testng-core/src/test/java/test/thread/TrueParallelSampleTest.java (2)
5-5: The addition ofimport test.support.SafeRandoms;is appropriate for utilizing theSafeRandomsutility class within this test class. This change aligns with the PR's objective to standardize random number generation across tests.
14-14: Replacingrandom.nextInt(10)withSafeRandoms.nextInt(3, 7, random)in theThread.sleepcall is a good practice to ensure non-zero random values are generated within a specified range. This change enhances the predictability of test outcomes by avoiding potential zero delays that could affect the test's behavior.testng-core/src/test/java/test/dataprovider/issue3081/TestClassSample.java (2)
10-10: The addition ofimport test.support.SafeRandoms;is appropriate for utilizing theSafeRandomsutility class within this test class. This change aligns with the PR's objective to standardize random number generation across tests.
31-31: Replacing the previous random number generation mechanism withSafeRandoms.nextInt(200, 300)in theTimeUnit.MILLISECONDS.sleepcall is a good practice to ensure non-zero random values are generated within a specified range. This change enhances the predictability of test outcomes by ensuring consistent sleep durations within the specified range.testng-core/src/test/java/test/dataprovider/issue3081/TestClassWithPrioritiesSample.java (2)
10-10: The addition ofimport test.support.SafeRandoms;is appropriate for utilizing theSafeRandomsutility class within this test class. This change aligns with the PR's objective to standardize random number generation across tests.
31-31: Replacing the previous random number generation mechanism withSafeRandoms.nextInt(200, 300)in theTimeUnit.MILLISECONDS.sleepcall is a good practice to ensure non-zero random values are generated within a specified range. This change enhances the predictability of test outcomes by ensuring consistent sleep durations within the specified range.testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2)
11-11: The addition ofimport test.support.SafeRandoms;is appropriate for utilizing theSafeRandomsutility class within this test class. This change aligns with the PR's objective to standardize random number generation across tests.
39-39: Replacing the previous random number generation mechanism with500L * SafeRandoms.nextInt(1, 10)in theTimeUnit.MILLISECONDS.sleepcall is a good practice to ensure non-zero random values are generated within a specified range. This change enhances the predictability of test outcomes by ensuring consistent sleep durations within the specified range. However, it's important to ensure that the multiplication by500Laligns with the intended sleep duration range for the test.testng-core/src/test/java/test/factory/issue326/SampleTestClass.java (2)
12-12: The addition ofimport test.support.SafeRandoms;is appropriate for utilizing theSafeRandomsutility class within this test class. This change aligns with the PR's objective to standardize random number generation across tests.
51-51: Replacing the previous random number generation mechanism with10L * SafeRandoms.nextInt(30, 70)in theTimeUnit.MILLISECONDS.sleepcall is a good practice to ensure non-zero random values are generated within a specified range. This change enhances the predictability of test outcomes by ensuring consistent sleep durations within the specified range. However, it's important to ensure that the multiplication by10Laligns with the intended sleep duration range for the test.testng-core/src/test/java/test/support/SafeRandoms.java (1)
8-39: TheSafeRandomsutility class is well-designed to provide a centralized way to generate random numbers with a guarantee of non-zero values. The use of static methods allows for easy access without needing to instantiate the class. The argument checks ensure that the methods are used correctly, preventing common errors such as negative bounds. However, consider adding a more detailed JavaDoc to explain the behavior whendeltais zero, as it might not be immediately clear to users how the method behaves in this case.
Summary by CodeRabbit
SecureRandomandRandomwith a newSafeRandomsutility.SafeRandomsclass for secure and uniform random number generation within specified bounds.SecureRandomwithSafeRandoms.