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

Support getting bound local port from Application and avoid using fixed port in unit tests #90

Open
wants to merge 1 commit into
base: 3.0.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions core/src/main/java/io/confluent/rest/Application.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import com.fasterxml.jackson.jaxrs.base.JsonParseExceptionMapper;

import io.confluent.common.config.ConfigException;

import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.NetworkTrafficServerConnector;
import org.eclipse.jetty.server.Server;
Expand Down Expand Up @@ -121,6 +123,12 @@ public Server createServer() throws RestConfigException {
ServletContainer servletContainer = new ServletContainer(resourceConfig);
ServletHolder servletHolder = new ServletHolder(servletContainer);
server = new Server() {
@Override
protected void doStart() throws Exception {
super.doStart();
Application.this.onStarted();
}

@Override
protected void doStop() throws Exception {
super.doStop();
Expand Down Expand Up @@ -239,6 +247,26 @@ protected void doStop() throws Exception {
return server;
}

/**
* Get the local ports that were bound for the listeners.
* @return a List containing the local ports
*/
public List<Integer> localPorts() {
List<Integer> ports = new ArrayList<>();
for(Connector connector : server.getConnectors()) {
if (!(connector instanceof NetworkTrafficServerConnector)) {
throw new RuntimeException(
String.format(
"Expected NetworkTrafficServerConnector for listener but found %s",
connector.getClass()
)
);
}
ports.add(((NetworkTrafficServerConnector) connector).getLocalPort());
}
return ports;
}

// TODO: delete deprecatedPort parameter when `PORT_CONFIG` is deprecated. It's only used to support the deprecated
// configuration.
public static List<URI> parseListeners(List<String> listenersConfig, int deprecatedPort,
Expand Down Expand Up @@ -352,6 +380,10 @@ public void stop() throws Exception {
server.stop();
}

public void onStarted() {
// Intentionally left blank
}

/**
* Shutdown hook that is invoked after the Jetty server has processed the shutdown request,
* stopped accepting new connections, and tried to gracefully finish existing requests. At this
Expand Down
52 changes: 52 additions & 0 deletions core/src/test/java/io/confluent/rest/ApplicationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,21 @@

package io.confluent.rest;

import io.confluent.common.config.ConfigDef;
import io.confluent.common.config.ConfigException;
import org.junit.Test;

import java.net.URI;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import javax.ws.rs.core.Configurable;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class ApplicationTest {
@Test
Expand Down Expand Up @@ -79,6 +85,52 @@ public void testParseListenersNoPort() {
List<URI> listeners = Application.parseListeners(listenersConfig, -1, Arrays.asList("http", "https"), "http");
}

@Test
public void zeroForPortShouldHaveNonZeroLocalPort() throws Exception {
TestRestConfig config = new TestRestConfig(
Collections.singletonMap(RestConfig.PORT_CONFIG, "0")
);
TestApplication testApp = new TestApplication(config);
testApp.start();
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 greater than 0", localPorts.get(0) > 0);
assertTrue(
"Should have a valid local port value less than or equal to 65535",
localPorts.get(0) <= 0xFFFF
);
testApp.stop();
testApp.join();
}

private class TestApplication extends Application<TestRestConfig> {
TestApplication(TestRestConfig config) {
super(config);
}

@Override
public void setupResources(Configurable<?> config, TestRestConfig appConfig) {
// intentionally left blank
}
}

private static class TestRestConfig extends RestConfig {
private static ConfigDef config;

static {
config = baseConfigDef();
}

public TestRestConfig() {
super(config);
}

public TestRestConfig(Map<?, ?> props) {
super(config, props);
}
}

private void assertExpectedUri(URI uri, String scheme, String host, int port) {
assertEquals("Scheme should be " + scheme, scheme, uri.getScheme());
assertEquals("Host should be " + host, host, uri.getHost());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ public class ExceptionHandlingTest {
public void setUp() throws Exception {
Properties props = new Properties();
props.setProperty("debug", "false");
// Override normal port to 0 for unit tests to use a random, available, ephemeral port that
// won't conflict with locally running services or parallel tests
props.setProperty(RestConfig.PORT_CONFIG, "0");
config = new TestRestConfig(props);
app = new ExceptionApplication(config);
app.start();
Expand All @@ -65,7 +68,7 @@ public void tearDown() throws Exception {
private void testGetException(String path, int expectedStatus, int expectedErrorCode,
String expectedMessage) {
Response response = ClientBuilder.newClient(app.resourceConfig.getConfiguration())
.target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
.target("http://localhost:" + app.localPorts().get(0))
.path(path)
.request()
.get();
Expand All @@ -79,7 +82,7 @@ private void testGetException(String path, int expectedStatus, int expectedError
private void testPostException(String path, Entity entity, int expectedStatus, int expectedErrorCode,
String expectedMessage) {
Response response = ClientBuilder.newClient(app.resourceConfig.getConfiguration())
.target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
.target("http://localhost:" + app.localPorts().get(0))
.path(path)
.request()
.post(entity);
Expand Down
16 changes: 11 additions & 5 deletions core/src/test/java/io/confluent/rest/ShutdownTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ public class ShutdownTest {
public void testShutdownHook() throws Exception {
Properties props = new Properties();
props.put("shutdown.graceful.ms", "50");
// Override normal port to 0 for unit tests to use a random, available, ephemeral port that
// won't conflict with locally running services or parallel tests
props.put(RestConfig.PORT_CONFIG, "0");
ShutdownApplication app = new ShutdownApplication(new TestRestConfig(props));
app.start();

Expand All @@ -49,11 +52,14 @@ public void testShutdownHook() throws Exception {
public void testGracefulShutdown() throws Exception {
Properties props = new Properties();
props.put("shutdown.graceful.ms", "50");
// Override normal port to 0 for unit tests to use a random, available, ephemeral port that
// won't conflict with locally running services or parallel tests
props.put(RestConfig.PORT_CONFIG, "0");
final TestRestConfig config = new TestRestConfig(props);
ShutdownApplication app = new ShutdownApplication(config);
app.start();

RequestThread req = new RequestThread(config);
RequestThread req = new RequestThread(app.localPorts().get(0));
req.start();
app.resource.requestProcessingStarted.await();

Expand Down Expand Up @@ -115,12 +121,12 @@ public void run() {
};

private static class RequestThread extends Thread {
RestConfig config;
int port;
boolean finished = false;
String response = null;

RequestThread(RestConfig config) {
this.config = config;
RequestThread(int port) {
this.port = port;
}
@Override
public void run() {
Expand All @@ -131,7 +137,7 @@ public void run() {
try {
Client client = ClientBuilder.newClient();
response = client
.target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG))
.target("http://localhost:" + port)
.path("/")
.request()
.get(String.class);
Expand Down