Avoid null pointer exceptions when calling getCapabilities #1094
Conversation
|
|
| Capabilities capabilities = getCapabilities(); | ||
| return String.format("%s, Capabilities: %s", getClass().getCanonicalName(), | ||
| getCapabilities().asMap().toString()); | ||
| capabilities != null ? capabilities.asMap().toString() : "null"); |
There was a problem hiding this comment.
I'd rather use checkNotNull macros everywhere
There was a problem hiding this comment.
or this is normal that getCapabilities can sometimes return null?
There was a problem hiding this comment.
getCapabilities will be null until the response of the new session request, where the real capabilities are returned by the server.
As I mentioned in the description, if the new session command fails (which can be pretty often when you are trying to set up the proper version combination of Appium, Automation and device), this NullPointerException masks the real failure (with no exception nesting).
So, overall, this makes the initial set up incredibly frustrating.
The idea of this change is to avoid NPEs altogether.
There was a problem hiding this comment.
yep, I've checked the Selenium code and it's how it currently works. Although I would like to get the response to SeleniumHQ/selenium#6903 first. What if this is just a bug and not the expected behaviour?
There was a problem hiding this comment.
I'm ok about learning the official posture. However, if RemoteWebDriver does indeed have a typo (missing a this.), then the exception masking would still remain. But instead of NullPointerException, a ClassCastException would be thrown:
In such a case, the fix would be much uglier.
The WebDriver capabilities field remains as null until the new-session command succeeds, returning the capabilities stated by the server. If an exception happens during session creation, some handling code may try to convert the WebDriver instance to string. In which case, the conversion would throw a NullPointerException, masking the real failure. This change just protects against the NullPointerException, so the real session initiation failures can be easily troubleshooted.
3 participants
Change list
Add null checks to
getCapabilities()that prevent spurious null pointer exceptions.Types of changes
What types of changes are you proposing/introducing to Java client?
Put an
xin the boxes that applyDetails
The
capabilitiesfield inRemoteWebDriverremains null until a new session is successfully created. If something fails before the Selenium client can consolidate the returned capabilities, aNullPointerExceptionis thrown from the Appium overridengetCapabilitiesmethods that masks the real failure.In other words:
getCapabilitiesgetCapabilitiesis calledRemoteWebDriverreturns null, the Appium override tries to call a method on a null valueThis exception masking results in very confusing and misleading troubleshooting.
The fix proposed by this pull request should fix the problem.