-
Notifications
You must be signed in to change notification settings - Fork 58
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 port conflict when running multiple unit tests in parallel #220
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you for the PR! However, the solution can be simplified and without breaking the existing API.
HoverflyClientFactory clientFactory = mock(HoverflyClientFactory.class); | ||
when(clientFactory.createHoverflyClient(any(HoverflyConfiguration.class))).thenReturn(hoverflyClient); | ||
Whitebox.setInternalState(hoverfly, "hoverflyClientFactory", clientFactory); |
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.
Mocking hoverflyClientFactory seems redundant because you are not verifying its behaviours, and complicating some unit tests which now requires setting up:
when(hoverflyClient.getHealth()).thenReturn(true);
hoverfly.start();
HoverflyClient hoverflyClient = createMockHoverflyClient(hoverfly); | ||
when(hoverflyClient.getHealth()).thenReturn(true); |
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 becomes harder to understand and is testing both hoverfly.start()
and hoverfly.resetMode()
. We should just mock HoverflyClient only to keep test simple and less coupled to implementation details:
Whitebox.setInternalState(hoverfly, "hoverflyClient", hoverflyClient);
/** | ||
* Static factory method for creating a {@link Builder} | ||
* @return a builder for HoverflyClient | ||
*/ | ||
static Builder custom() { | ||
return new Builder(); | ||
} | ||
|
||
/** | ||
* Static factory method for default Hoverfly client | ||
* @return a default HoverflyClient | ||
*/ | ||
static HoverflyClient createDefault() { | ||
return new Builder().build(); | ||
} | ||
|
||
/** | ||
* HTTP client builder for Hoverfly admin API | ||
*/ | ||
class Builder { | ||
|
||
private String scheme = HoverflyConstants.HTTP; | ||
private String host = HoverflyConstants.LOCALHOST; | ||
private int port = HoverflyConstants.DEFAULT_ADMIN_PORT; | ||
private String authToken = null; | ||
|
||
Builder() { | ||
} | ||
|
||
public Builder scheme(String scheme) { | ||
this.scheme = scheme; | ||
return this; | ||
} | ||
|
||
public Builder host(String host) { | ||
this.host = host; | ||
return this; | ||
} | ||
|
||
public Builder port(int port) { | ||
this.port = port; | ||
return this; | ||
} | ||
|
||
/** | ||
* Get token from environment variable "HOVERFLY_AUTH_TOKEN" to authenticate with admin API | ||
* @return this Builder for further customizations | ||
*/ | ||
public Builder withAuthToken() { | ||
this.authToken = System.getenv(HoverflyConstants.HOVERFLY_AUTH_TOKEN); | ||
return this; | ||
} | ||
|
||
public HoverflyClient build() { | ||
return new OkHttpHoverflyClient(scheme, host, port, authToken); | ||
} | ||
} | ||
|
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.
We should try not to remove public method hoverfly-java users depends on. For example, people may do the following to create a hoverfly client:
HoverflyClient.createDefault()
Why not create a new builder method to set config from a HoverflyConfiguration
object, something like this:
HoverflyClient.custom().withConfig(configuration);
.scheme(hoverflyConfig.getScheme()) | ||
.host(hoverflyConfig.getHost()) | ||
.port(hoverflyConfig.getAdminPort()) | ||
.withAuthToken() |
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.
withAuthToken()
is optional. It's better to expose the builder for the user to customize the client:
HoverflyClient.custom().withConfig(configuration).withAuthToken();
if (hoverflyConfig.getProxyPort() == 0) { | ||
hoverflyConfig.setProxyPort(findUnusedPort()); | ||
} | ||
if (hoverflyConfig.getAdminPort() == 0) { | ||
hoverflyConfig.setAdminPort(findUnusedPort()); | ||
} | ||
checkPortInUse(hoverflyConfig.getProxyPort()); | ||
checkPortInUse(hoverflyConfig.getAdminPort()); |
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.
We should move any port checking and setting right before the start of hoverfly, to avoid synchronizing the whole method, only synchronizing the following is what you wanted (in pseudocode):
allOtherSetup()
synchronized() {
findAndSetPort()
checkPortInUse()
setPortToCommandArgs()
startHoverfly()
}
The "random" port selection can still lead to port conflicts, when running multiple unit tests in parallel.
This is because port selection and check is done within the config validation phase, but the actual port binding is done later when the hoverfly process is started. Example:
Fixed by moving port selection from validation to Hoverfly.startHoverflyProcess and synchronizing on Hoverfly.class.
This required some refactoring, mainly because the HoverflyClient cannot be instantiated in constructor, because the actuall port is not known yet. HoverflyClient instantiation moved to startHoverflyProcess. Introduced HoverflyClientFactory for testability.