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

Fix for NPE in case of retry. #77

Merged
merged 3 commits into from
Jan 25, 2018

Conversation

alobodzki
Copy link
Contributor

In case of retry RetryOnErrorHandler was recreated without required fields.
Additionally this change contains refactoring to reuse OriginStatsFactory in
StyxHttpClient.

In case of retry RetryOnErrorHandler was recreated without required fields.
Additionally this change contains refactoring to reuse OriginStatsFactory in
StyxHttpClient.
Copy link
Contributor

@mikkokar mikkokar left a 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);
Copy link
Contributor

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:

  1. adding a helper method to set up context
  2. convert the mocks like connectionPool, lbContext, loadBalancingStrategy into fields and initialise in test setup method before each test.

}

@Test
public void shouldRetry() {
Copy link
Contributor

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() {
Copy link
Contributor

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.

@mikkokar mikkokar merged commit 3c3002f into ExpediaGroup:master Jan 25, 2018
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