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

Conversation

ewencp
Copy link
Contributor

@ewencp ewencp commented Mar 12, 2018

No description provided.

@ewencp
Copy link
Contributor Author

ewencp commented Mar 12, 2018

confluentinc/schema-registry#758 and confluentinc/kafka-rest#414 are downstream changes that will rely on this being merged first.

Copy link

@edenhill edenhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some nits

@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a List

assertTrue("Should have a valid local port value", localPorts.get(0) > 0);
testApp.stop();
testApp.join();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mayeb add a test that verifies that localPorts returns the original PORT_CONFIG if != 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& <= 0xffff

@@ -352,6 +380,10 @@ public void stop() throws Exception {
server.stop();
}

public void onStarted() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// intentionally left bland

@@ -51,6 +51,7 @@
public void setUp() throws Exception {
Properties props = new Properties();
props.setProperty("debug", "false");
props.setProperty(RestConfig.PORT_CONFIG, "0");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra points to add a comment why this is necessary

@edenhill
Copy link

LGTM

Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

4 participants