Fail fast with a clear exception instead of returning null in getSele… - #11964
Fail fast with a clear exception instead of returning null in getSele…#11964NguyenTienDat377 wants to merge 1 commit into
Conversation
|
Both this PR and #11958 (opened Aug 4) rewrite the same two I built and ran both branches locally on top of 1. The catch block is only reachable for a mapped port below -1.
The host is never a trigger:
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 2. Overriding With this PR
} 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 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 |
What does this PR do?
BrowserWebDriverContainer#getSeleniumAddress()currently catchesMalformedURLException,calls
e.printStackTrace()(marked with an open// TODO), and returnsnull.Every caller of this method in the codebase (
RemoteWebDriverconstructors in tests andexamples) passes the result straight into
new RemoteWebDriver(seleniumAddress, ...)withno null check. So a malformed URL doesn't actually fail safely today — it just turns into a
confusing
NullPointerExceptioninside Selenium'sRemoteWebDriverconstructor, severalframes 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 nullwith throwing anIllegalStateExceptionthat wraps the original
MalformedURLException, so the failure surfaces immediately, at theactual point of failure, with a clear message and full cause chain.
Applied the same fix to both copies of the class (
org.testcontainers.containers.BrowserWebDriverContainerand
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 —
MalformedURLExceptionhere would only realistically occur if the container itself werealready in a broken state.