Fix port conflict when running multiple unit tests in parallel#220
Fix port conflict when running multiple unit tests in parallel#220cretzel wants to merge 1 commit intoSpectoLabs:masterfrom
Conversation
| 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.
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();
| hoverfly = new Hoverfly(localConfigs().captureAllHeaders().enableStatefulCapture(), CAPTURE); | ||
|
|
||
| HoverflyClient hoverflyClient = createMockHoverflyClient(hoverfly); | ||
| when(hoverflyClient.getHealth()).thenReturn(true); |
There was a problem hiding this comment.
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.
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.
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.
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()
}
3 participants
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.