[java] resolve exceptions from the W3C state in ErrorHandler

[yashp676]

Related Issues

• Track A, first item, of [🚀 Feature]: Remove deprecated JSON Wire ErrorCodes / Response.status (java) #17638

What does this PR do?

ErrorHandler resolved exception types from the JSON Wire integer Response.getStatus(). It now uses the W3C state string via the existing non-deprecated ErrorCodes.getExceptionType(String), so ErrorHandler no longer reads Response.getStatus() at all.

Also removes a dead block that tried to construct exceptions with a (String, Throwable, Integer) signature. Nothing under java/src/org/openqa/selenium/ declares that constructor, and createThrowable swallows the NoSuchMethodException and returns null, so it always fell through to the (String, Throwable) case below.

Implementation Notes

The issue suggests ErrorCodec.decode(). That returns a built WebDriverException, but ErrorHandler needs the exception class so it can attach its own message, cause and screenshot handling. getExceptionType(String) fits better and W3CHandshakeResponse already uses it this way. Happy to redo it through ErrorCodec if you'd prefer.

state is set on every inbound path: W3CHttpResponseCodec sets it on each error branch and derives status from it, ProtocolHandshake and Response.fromJson both set it, and ErrorHandler already relied on it for the success check.

Two entries dropped from ErrorHandlerTest: XPATH_LOOKUP_ERROR and INVALID_XPATH_SELECTOR. Both are non-canonical for W3C in ErrorCodes, so toState maps them to "unhandled error" and no spec-compliant remote end can send them as a state. They only asserted InvalidSelectorException because the int lookup skipped the state mapping. INVALID_SELECTOR_ERROR and INVALID_XPATH_SELECTOR_RETURN_TYPER still cover that exception.

ResponseConverter is on the issue's list but is out of scope — its getStatus() calls are HttpResponse's HTTP status, not Response.status.

ErrorHandler falls back to WebDriverException when the ErrorCodes lookup returns null. Appium's ErrorCodesMobile returns null for non-mobile states, which would otherwise throw a NullPointerException.

Types of changes

• Cleanup (no functional change intended)

Checklist

• I have read the contributing document.
• All new and existing tests passed.

ErrorHandlerTest, ErrorCodecTest, W3CHttpResponseCodecTest, W3CHandshakeResponseTest, ProtocolHandshakeTest and RemoteWebDriverUnitTest pass locally on Linux.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. State-based errors lack a focused test ✗ Dismissed 📘 Rule violation ☼ Reliability

Description

testThrowsCorrectExceptionTypes supplies an integer to createResponse, which derives a matching
state and sets both fields before invoking throwIfResponseFailed. If the handler returns to
resolving exceptions from the integer status, every remaining mapping assertion still passes, so the
central behavior change can regress undetected.

Code

java/test/org/openqa/selenium/remote/ErrorHandlerTest.java[R88-89]

+    assertThrowsCorrectExceptionType(
+        ErrorCodes.INVALID_SELECTOR_ERROR, InvalidSelectorException.class);

Evidence

Compliance rule 6 requires focused coverage for behavioral changes. The production handler now
selects the exception from responseState, but the replacement assertion uses a helper that sets
both status and its derived state, leaving the new lookup source unverified.

AGENTS.md: Use Focused, Reliable Tests and Avoid Mocks That Misrepresent Contracts
java/src/org/openqa/selenium/remote/ErrorHandler.java[100-101]
java/test/org/openqa/selenium/remote/ErrorHandlerTest.java[88-89]
java/test/org/openqa/selenium/remote/ErrorHandlerTest.java[478-482]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The changed tests continue to construct responses with matching integer status and W3C state values, so they do not prove that `ErrorHandler` resolves exception types from the state.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/ErrorHandlerTest.java[88-89]
- java/src/org/openqa/selenium/remote/ErrorHandler.java[100-101]

## Recommended Fix
Add a focused unit test that constructs a response whose W3C state identifies a specific exception while its integer status is absent or deliberately conflicting, then assert that the state determines the thrown exception type.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: 🧠 Deep: This push adds substantial vendor BiDi schema, normalization, projection, and Ruby/JavaScript generation logic across multiple independent paths, creating a dense set of easy-to-miss integration risks beyond a shallow generated diff.

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[diemol]

Can you check the AI review comment?

Also, I think getExceptionType(int) won't be used anymore. Can you check if it has references and if not, it should be removed.

[yashp676]

Thanks for the review, @diemol.

I've added focused tests for the AI review point: state set with no status, state taking precedence over a conflicting status, and an ErrorCodes subclass returning null.

On removing getExceptionType(int): I'd suggest holding off. Appium's ErrorCodesMobile overrides it and passes itself into ErrorHandler via new ErrorHandler(new ErrorCodesMobile(), true): https://javadoc.io/static/io.appium/java-client/10.1.1/io/appium/java_client/ErrorCodesMobile.html Removing it would break their build, which I think is what Track B in #17638 is sequencing around. Happy to mark it @Deprecated(forRemoval = true) here instead if you'd prefer, or leave it for Track B.

Checking that also turned up a bug in my original change: ErrorCodesMobile's getExceptionType(String) returns null for anything that isn't a mobile-specific error, so ErrorHandler would have thrown a NullPointerException for Appium users on this path. I've added a fallback to WebDriverException. The new test fails with exactly that NPE without it.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[yashp676]

Hi @diemol, The one failing check is RBE's BrowsingContextInspectorTest-remote, which failed on both attempts. I don't think it's related: BiDi events come over the WebSocket rather than through ErrorHandler, the Java browser and remote jobs all passed, and BrowsingContextInspectorTest-chrome passes locally on this branch. The output was too large for the Actions log though, so I can't see which case failed in the remote run. Happy to dig in if it looks related to you.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 144bb6b

[qodo-code-review]

No code changes since the last review — review skipped