Both this PR and #11958 (opened Aug 4) rewrite the same two getSeleniumAddress() bodies the same way: drop printStackTrace() + return null, throw instead. The only difference is the exception type, IllegalStateException here and ContainerLaunchException there. Probably worth picking one before either lands.

I built and ran both branches locally on top of main (2ac3c97, JDK 17 toolchain) with a throwaway probe. Two things came out of it that bear on the choice.

1. The catch block is only reachable for a mapped port below -1.

new URL("http", host, port, file) throws MalformedURLException in exactly two cases: unknown protocol, and port < -1. The protocol here is the literal "http", so only the port case remains. Identical output on JDK 17.0.12, 21.0.4 and 26.0.1:

OK    proto=http       host=localhost port=-1         -> http://localhost/wd/hub
THROW proto=http       host=localhost port=-12345     -> MalformedURLException: Invalid port number :-12345
OK    proto=http       host=localhost port=0          -> http://localhost:0/wd/hub
OK    proto=http       host=localhost port=2147483647 -> http://localhost:2147483647/wd/hub
THROW proto=bogusproto host=localhost port=32768      -> MalformedURLException: unknown protocol: bogusproto

The host is never a trigger: "not a host!", "%%%", "" and null all build a URL without throwing.

ContainerState#getMappedPort has three outcomes and none of them yields a port below -1: IllegalStateException when getContainerId() == null, IllegalArgumentException("Requested port (...) is not mapped") when there is no binding, otherwise Integer.valueOf(binding[0].getHostPortSpec()) straight from Docker. Those first two also propagate through the try untouched, since they are not MalformedURLException. Probe on an unstarted container, byte-identical on main, on #11958 and on this branch:

PROBE[not-started] threw=java.lang.IllegalStateException msg=Mapped port can only be obtained after the container is started

So the failure a user actually hits today is already fail-fast, and neither PR changes it. That does not make the change useless, but it does change what the change is: a cleanup of an unreachable branch and of the // TODO, rather than a fix for a silent failure. The description argues the current code "turns into a confusing NullPointerException inside Selenium's RemoteWebDriver constructor". I grepped all seven call sites (docs/examples/junit4/generic, examples/cucumber, examples/selenium-container, BaseWebDriverContainerTest twice, LocalServerWebDriverContainerTest, and the deprecated getWebDriver() at containers/BrowserWebDriverContainer.java:339) and you are right that not one of them null-checks. But reaching that NPE still needs a mapped port below -1, and I could not find a path that produces one. Might be worth toning that part down so the change gets graded as the cleanup it is.

2. IllegalStateException collides with what getMappedPort already throws.

Overriding getMappedPort to return -12345 so the catch is actually entered, side by side:

#11958    PROBE[port=-12345] threw=org.testcontainers.containers.ContainerLaunchException msg=Could not construct Selenium address
this PR   PROBE[port=-12345] threw=java.lang.IllegalStateException msg=Failed to construct Selenium address
this PR   PROBE[not-started] threw=java.lang.IllegalStateException msg=Mapped port can only be obtained after the container is started

With this PR getSeleniumAddress() throws IllegalStateException for two unrelated reasons, and a caller writing catch (IllegalStateException e) cannot separate "container not started yet" from "URL assembly failed".

ContainerLaunchException is also what these two files already use for exactly this shape of problem, a should-never-happen exception wrapped with a comment that says so:

} catch (IOException e) {
    // should never happen as per javadoc, since we use valid prefix
    logger().error("Exception while trying to create temp directory", e);
    throw new ContainerLaunchException("Exception while trying to create temp directory", e);
}

That is selenium/BrowserWebDriverContainer.java:131 and its mirror at containers/BrowserWebDriverContainer.java:176. Repo-wide, main sources contain 30 new ContainerLaunchException(...), and these two getSeleniumAddress() methods are the only catch (MalformedURLException) in main sources at all.

So my suggestion is to converge on #11958's exception type and carry over the neighbouring catch's note about reachability:

} catch (MalformedURLException e) {
    // should never happen: the protocol is a literal and getMappedPort() cannot return a port < -1
    throw new ContainerLaunchException("Could not construct Selenium address", e);
}

I applied precisely that on top of #11958 and both :testcontainers-selenium:spotlessCheck and :testcontainers-selenium:compileJava are green, so the comment fits the formatter's line limit.

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling when the Selenium connection URL cannot be created.
  • Failures now report a clear container launch error instead of returning an invalid empty result.

Walkthrough

The Selenium address methods now throw ContainerLaunchException when URL construction fails. They no longer print the stack trace or return null.

Changes

Selenium URL error handling

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: eddumelendez, kiview, pioorg

Poem

A rabbit found a URL gone astray,
So errors now hop the proper way.
No silent null, no stack trace flight,
ContainerLaunchException makes it right.
Thump-thump, the Selenium path is clear!

• Create stacked PR
• Commit on current branch

• Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

• X
• Mastodon
• Reddit
• LinkedIn

Comment @coderabbitai help to get the list of available commands.

Both this PR and #11958 (opened Aug 4) rewrite the same two getSeleniumAddress() bodies the same way: drop printStackTrace() + return null, throw instead. The only difference is the exception type, IllegalStateException here and ContainerLaunchException there. Probably worth picking one before either lands.

I built and ran both branches locally on top of main (2ac3c97, JDK 17 toolchain) with a throwaway probe. Two things came out of it that bear on the choice.

1. The catch block is only reachable for a mapped port below -1.

new URL("http", host, port, file) throws MalformedURLException in exactly two cases: unknown protocol, and port < -1. The protocol here is the literal "http", so only the port case remains. Identical output on JDK 17.0.12, 21.0.4 and 26.0.1:

OK    proto=http       host=localhost port=-1         -> http://localhost/wd/hub
THROW proto=http       host=localhost port=-12345     -> MalformedURLException: Invalid port number :-12345
OK    proto=http       host=localhost port=0          -> http://localhost:0/wd/hub
OK    proto=http       host=localhost port=2147483647 -> http://localhost:2147483647/wd/hub
THROW proto=bogusproto host=localhost port=32768      -> MalformedURLException: unknown protocol: bogusproto

The host is never a trigger: "not a host!", "%%%", "" and null all build a URL without throwing.

ContainerState#getMappedPort has three outcomes and none of them yields a port below -1: IllegalStateException when getContainerId() == null, IllegalArgumentException("Requested port (...) is not mapped") when there is no binding, otherwise Integer.valueOf(binding[0].getHostPortSpec()) straight from Docker. Those first two also propagate through the try untouched, since they are not MalformedURLException. Probe on an unstarted container, byte-identical on main, on #11958 and on this branch:

PROBE[not-started] threw=java.lang.IllegalStateException msg=Mapped port can only be obtained after the container is started

So the failure a user actually hits today is already fail-fast, and neither PR changes it. That does not make the change useless, but it does change what the change is: a cleanup of an unreachable branch and of the // TODO, rather than a fix for a silent failure. The description argues the current code "turns into a confusing NullPointerException inside Selenium's RemoteWebDriver constructor". I grepped all seven call sites (docs/examples/junit4/generic, examples/cucumber, examples/selenium-container, BaseWebDriverContainerTest twice, LocalServerWebDriverContainerTest, and the deprecated getWebDriver() at containers/BrowserWebDriverContainer.java:339) and you are right that not one of them null-checks. But reaching that NPE still needs a mapped port below -1, and I could not find a path that produces one. Might be worth toning that part down so the change gets graded as the cleanup it is.

2. IllegalStateException collides with what getMappedPort already throws.

Overriding getMappedPort to return -12345 so the catch is actually entered, side by side:

#11958    PROBE[port=-12345] threw=org.testcontainers.containers.ContainerLaunchException msg=Could not construct Selenium address
this PR   PROBE[port=-12345] threw=java.lang.IllegalStateException msg=Failed to construct Selenium address
this PR   PROBE[not-started] threw=java.lang.IllegalStateException msg=Mapped port can only be obtained after the container is started

With this PR getSeleniumAddress() throws IllegalStateException for two unrelated reasons, and a caller writing catch (IllegalStateException e) cannot separate "container not started yet" from "URL assembly failed".

ContainerLaunchException is also what these two files already use for exactly this shape of problem, a should-never-happen exception wrapped with a comment that says so:

} catch (IOException e) {
    // should never happen as per javadoc, since we use valid prefix
    logger().error("Exception while trying to create temp directory", e);
    throw new ContainerLaunchException("Exception while trying to create temp directory", e);
}

That is selenium/BrowserWebDriverContainer.java:131 and its mirror at containers/BrowserWebDriverContainer.java:176. Repo-wide, main sources contain 30 new ContainerLaunchException(...), and these two getSeleniumAddress() methods are the only catch (MalformedURLException) in main sources at all.

So my suggestion is to converge on #11958's exception type and carry over the neighbouring catch's note about reachability:

} catch (MalformedURLException e) {
    // should never happen: the protocol is a literal and getMappedPort() cannot return a port < -1
    throw new ContainerLaunchException("Could not construct Selenium address", e);
}

I applied precisely that on top of #11958 and both :testcontainers-selenium:spotlessCheck and :testcontainers-selenium:compileJava are green, so the comment fits the formatter's line limit.

Thank you for the feedback @kdelay, I have fixed per your code review