Skip to content

Fail fast with a clear exception instead of returning null in getSele… - #11964

Open
NguyenTienDat377 wants to merge 1 commit into
testcontainers:mainfrom
NguyenTienDat377:fix/selenium-address-illegal-state
Open

Fail fast with a clear exception instead of returning null in getSele…#11964
NguyenTienDat377 wants to merge 1 commit into
testcontainers:mainfrom
NguyenTienDat377:fix/selenium-address-illegal-state

Conversation

@NguyenTienDat377

Copy link
Copy Markdown

What does this PR do?

BrowserWebDriverContainer#getSeleniumAddress() currently catches MalformedURLException,
calls e.printStackTrace() (marked with an open // TODO), and returns null.

Every caller of this method in the codebase (RemoteWebDriver constructors in tests and
examples) passes the result straight into new RemoteWebDriver(seleniumAddress, ...) with
no null check. So a malformed URL doesn't actually fail safely today — it just turns into a
confusing NullPointerException inside Selenium's RemoteWebDriver constructor, several
frames away from the real cause, with the original exception only visible via a stderr dump
instead of the test's actual logs.

This PR replaces the printStackTrace + return null with throwing an IllegalStateException
that wraps the original MalformedURLException, so the failure surfaces immediately, at the
actual point of failure, with a clear message and full cause chain.

Applied the same fix to both copies of the class (org.testcontainers.containers.BrowserWebDriverContainer
and org.testcontainers.selenium.BrowserWebDriverContainer), since both had the identical TODO.

Why is it important?

This method effectively never returns null safely in practice (no call site checks for it),
so the current behavior only delays and obscures a real failure. Failing fast with a clear
message is strictly more debuggable and doesn't change behavior for any passing case —
MalformedURLException here would only realistically occur if the container itself were
already in a broken state.

@NguyenTienDat377
NguyenTienDat377 marked this pull request as ready for review August 10, 2026 12:12
@NguyenTienDat377
NguyenTienDat377 requested a review from a team as a code owner August 10, 2026 12:12
@kdelay

kdelay commented Aug 12, 2026

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants