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

Build with Java 17 LTS #665

Merged
merged 3 commits into from
Sep 29, 2021
Merged

Build with Java 17 LTS #665

merged 3 commits into from
Sep 29, 2021

Conversation

brenuart
Copy link
Collaborator

Close #654 and #664

…sues during test

Cfr Issue #664

Instead of exposing and mocking the Random used by the strategy, hide it behind a `nextInt()` member method that can safely by intercepted by Mockito during tests.
@brenuart
Copy link
Collaborator Author

brenuart commented Sep 26, 2021

Some comments about RandomDestinationConnectionStrategy...

(1)
The class used to expose a public getRandom() method. The only reason these method was made public is to allow mocking by Mockito during test case (some tests are not in the same package so the method could not have been made protected instead).
Issue #664 is about Mockito unable to Mock the java.util.Random class with Java 17. To work around the issue, this PR does not expose the Random anymore but instead defines a nextInt() method directly on the strategy class. Test cases are modified to mock/intercept that method instead of mocking the Random itself.
However, this new method must still be public which is definitely not "good". IMHO a better approach is to make it protected and let testcases extend the class and override the method with the desired behaviour for the test.

(2)
RandomDestinationConnectionStrategyTest is about testing the random strategy - but there is no "random" anymore: each test starts by giving the sequence of "random" number that should be returned by subsequent calls to Random#nextInt()... According to me, we are not testing the "RandomConnectionStrategy" here but rather the DestinationConnectionStrategyWithTtl...

(3)
The strategy makes use of a "ThreadLocal Random" to support concurrency. However, it seems that connection strategies are always used from the same thread (the event handler thread)... Concurrent access is therefore not a requirement here. Your opinion?

@brenuart brenuart requested a review from philsttr September 26, 2021 21:43
@philsttr
Copy link
Collaborator

(1) Feel free to make protected and extend as you mentioned.

(2) The test does not need to test that Random/ThreadLocalRandom behave properly, since they have been thoroughly tested by the jdk release process. The test just needs to verify that the RandomDestinationConnectionStrategyTest calls Random/ThreadLocalRandom properly and returns their values. So mocking the Random return values, and checking that the RandomDestinationConnectionStrategy returns those values is a valid test.

(3) I'd have to dig in to see if it is safe to remove. I don't think there's a harm in using a thread local Random to ensure the strategy is threadsafe.

Use a UnaryOperator as random generator instead of a public method whose only purpose is to serve as a hook for Mockito during test cases.
@brenuart
Copy link
Collaborator Author

(1) Feel free to make protected and extend as you mentioned.

I chose to pass a UnaryOperator function as argument to the constructor to generate random numbers. The default implementation makes use of ThreadLocalRandom.current().nextInt(). Test cases can provide their own implementation to control the sequence of generated random numbers.

(3) I'd have to dig in to see if it is safe to remove. I don't think there's a harm in using a thread local Random to ensure the strategy is threadsafe.

I kept the ThreadLocalRandom. The "thread-safety" question can be raised for the other connection strategies as well. I propose to open a separate issue to cover them all if we feel the need to investigate any further.

@brenuart brenuart merged commit bdad979 into main Sep 29, 2021
@brenuart brenuart deleted the gh654-java17 branch September 29, 2021 10:35
@philsttr philsttr added this to the 7.0 milestone Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants