-
Notifications
You must be signed in to change notification settings - Fork 79
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
Fix for NPE in case of retry. #77
Conversation
In case of retry RetryOnErrorHandler was recreated without required fields. Additionally this change contains refactoring to reuse OriginStatsFactory in StyxHttpClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking some small stylistic improvements.
|
||
RetryPolicy.Context context = mock(RetryPolicy.Context.class); | ||
ConnectionPool connectionPool = mock(ConnectionPool.class); | ||
LoadBalancingStrategy.Context lbContext = mock(LoadBalancingStrategy.Context.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock setup adds lots of verbosity in the tests in this class. Please consider:
- adding a helper method to set up context
- convert the mocks like
connectionPool
,lbContext
,loadBalancingStrategy
into fields and initialise in test setup method before each test.
} | ||
|
||
@Test | ||
public void shouldRetry() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test name is not informative. We need to make the cause+effect clear.
} | ||
|
||
@Test | ||
public void shouldNotRetryBasedOnWrongException() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes an exception wrong? I think the test name should mention the marker interface.
In case of retry RetryOnErrorHandler was recreated without required fields.
Additionally this change contains refactoring to reuse OriginStatsFactory in
StyxHttpClient.