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

[NguyenTienDat377]

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.

[kdelay]

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.

[coderabbitai]

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

Layer / File(s)	Summary
Propagate Selenium URL failures modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java, modules/selenium/src/main/java/org/testcontainers/selenium/BrowserWebDriverContainer.java	Both getSeleniumAddress() implementations throw ContainerLaunchException with the original cause when Selenium URL construction fails.

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!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: getSeleniumAddress() now fails fast with a clear exception instead of returning null.
Description check	✅ Passed	The description explains the current behavior, the failure mode, the proposed fix, its rationale, and why both class copies changed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

Warning

⚠️ This pull request shows signs of AI-generated slop (description_diff_mismatch). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

[NguyenTienDat377]

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

[coderabbitai]

🧹 Nitpick comments (1)

modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java (1)

283-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the expected unreachability of the malformed-URL guard.

Normal mapped ports should not produce MalformedURLException. Add the same explanatory comment in both implementations.

• modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java#L283-L283: document that the guard handles an invalid mapped port and should be unreachable.
• modules/selenium/src/main/java/org/testcontainers/selenium/BrowserWebDriverContainer.java#L182-L182: add the same rationale to keep both implementations aligned.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java`
at line 283, Add the same explanatory comment above the malformed-URL guard at
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java:283-283
and
modules/selenium/src/main/java/org/testcontainers/selenium/BrowserWebDriverContainer.java:182-182,
documenting that invalid mapped ports should be unreachable but are guarded
against. No behavioral change is required.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java`:
- Line 283: Add the same explanatory comment above the malformed-URL guard at
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java:283-283
and
modules/selenium/src/main/java/org/testcontainers/selenium/BrowserWebDriverContainer.java:182-182,
documenting that invalid mapped ports should be unreachable but are guarded
against. No behavioral change is required.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d9b83e13-3dc4-4423-bf4b-8777268d88fa

📥 Commits

Reviewing files that changed from the base of the PR and between 2ac3c97 and 609a11d.

📒 Files selected for processing (2)

• modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java
• modules/selenium/src/main/java/org/testcontainers/selenium/BrowserWebDriverContainer.java