Support getting bound local port from Application and avoid using fixed port in unit tests #90
Conversation
|
confluentinc/schema-registry#758 and confluentinc/kafka-rest#414 are downstream changes that will rely on this being merged first. |
| @@ -239,6 +247,26 @@ protected void doStop() throws Exception { | |||
| return server; | |||
| } | |||
|
|
|||
| /** | |||
| * Get the local ports that were bound for the listeners. | |||
| * @return an List containing the local ports | |||
| assertTrue("Should have a valid local port value", localPorts.get(0) > 0); | ||
| testApp.stop(); | ||
| testApp.join(); | ||
| } |
There was a problem hiding this comment.
Mayeb add a test that verifies that localPorts returns the original PORT_CONFIG if != 0.
There was a problem hiding this comment.
Heh, I considered it but this has the exact problem that I was trying to remove by removing the use of choosePort() -- you have to somehow choose a fixed value before allocating the port, which makes it a flakey test because there is no reliable way of choosing this without the potential for conflicts. I can try to pick something in a range that is unlikely to cause conflicts, in the hope that this will be the only test that will use a fixed port, but it will still be error prone.
I guess something very high in the IANA registered port range and below the ephemeral range could possibly work, at least for a single test...
There was a problem hiding this comment.
Ha, yeah of course, you are right :)
You could try a range of high ports, skipping to the next if binding fails, but it might be overkill for something like this. 👎
| List<Integer> localPorts = testApp.localPorts(); | ||
| assertEquals(1, localPorts.size()); | ||
| // Validate not only that it isn't zero, but also a valid value | ||
| assertTrue("Should have a valid local port value", localPorts.get(0) > 0); |
| @@ -352,6 +380,10 @@ public void stop() throws Exception { | |||
| server.stop(); | |||
| } | |||
|
|
|||
| public void onStarted() { | |||
|
|
|||
| @@ -51,6 +51,7 @@ | |||
| public void setUp() throws Exception { | |||
| Properties props = new Properties(); | |||
| props.setProperty("debug", "false"); | |||
| props.setProperty(RestConfig.PORT_CONFIG, "0"); | |||
There was a problem hiding this comment.
extra points to add a comment why this is necessary
…ed port in unit tests
|
LGTM |
There was a problem hiding this comment.
Hey @ewencp,
Noticed your PR and thought I'd mention that KSQL already has functionality similar to this in its Application subsclass.
KSQL's rest app has a method to get the listeners:
public List<URL> getListeners() {
return Arrays.stream(server.getConnectors())
.filter(connector -> connector instanceof ServerConnector)
.map(ServerConnector.class::cast)
.map(connector -> {
try {
final String protocol = new HashSet<>(connector.getProtocols())
.stream()
.map(String::toLowerCase)
.anyMatch(s -> s.equals("ssl")) ? "https" : "http";
final int localPort = connector.getLocalPort();
return new URL(protocol, "localhost", localPort, "");
} catch (final Exception e) {
throw new RuntimeException("Malformed listener", e);
}
})
.collect(Collectors.toList());
}
Which may be more flexible / useful than just getting the ports, (and not knowing the protocol).
Then KSQL has a JUnit ExternalResource class that has a method to find the http end point:
public URI getHttpListener() {
final URL url = getListeners().stream()
.filter(l -> l.getProtocol().equals("http"))
.findFirst()
.orElseThrow(() -> new RuntimeException("No HTTP Listener found: "));
try {
return url.toURI();
} catch (final Exception e) {
throw new RuntimeException("Invalid REST listener", e);
}
}
Which can then be used directly in the tests to set the server local for HTTP requests.
No description provided.