Skip to content

[Java] refactor: replace deprecated ErrorCodes.SUCCESS_STRING #15946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

manuelsblanco
Copy link
Contributor

@manuelsblanco manuelsblanco commented Jun 23, 2025

User description

with literal "success"

Replaced usage of ErrorCodes.SUCCESS_STRING with the literal string "success" in ProtocolHandshake.java and RemotableByTest.java, as ErrorCodes is marked @deprecated(forRemoval = true).

Also removed direct use of ErrorCodes.SUCCESS and related methods like errorCodes.toStatusCode(e) in preparation for full W3C compliance, where status and state fields are no longer needed or recommended in responses.

This change aligns the codebase with the W3C WebDriver protocol by reducing reliance on deprecated JSON Wire Protocol conventions.

💥 What does this PR do?

This PR removes the use of deprecated constants and methods from the ErrorCodes class by:
• Replacing ErrorCodes.SUCCESS_STRING with the literal "success" in ProtocolHandshake.java and RemotableByTest.java.
• Removing calls to setStatus() and setState() in response objects, which are no longer required under the W3C WebDriver protocol.
• Eliminating the use of errorCodes.toStatusCode(e) and errorCodes.toState(...) when building error responses, since the W3C protocol does not rely on numeric status codes or legacy state strings.

These changes simplify the codebase and move it closer to full W3C compliance by avoiding outdated JSON Wire Protocol constructs.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Other


Description

  • Replace deprecated ErrorCodes.SUCCESS_STRING with literal "success"

  • Remove deprecated setStatus() calls from response objects

  • Eliminate unused ErrorCodes imports and instances

  • Align codebase with W3C WebDriver protocol standards


Changes walkthrough 📝

Relevant files
Cleanup
ProtocolHandshake.java
Remove deprecated ErrorCodes usage in response creation   

java/src/org/openqa/selenium/remote/ProtocolHandshake.java

  • Removed response.setStatus(ErrorCodes.SUCCESS) call
  • Replaced ErrorCodes.SUCCESS_STRING with literal "success"
  • +1/-2     
    RemotableByTest.java
    Clean up deprecated ErrorCodes usage in tests                       

    java/test/org/openqa/selenium/remote/RemotableByTest.java

  • Removed import of ErrorCodes.SUCCESS_STRING
  • Removed ErrorCodes instance variable
  • Replaced SUCCESS_STRING with literal "success"
  • Simplified error response creation by removing deprecated methods
  • +2/-5     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • …success"
    
    Replaced usage of ErrorCodes.SUCCESS_STRING with the literal string "success" in ProtocolHandshake.java and RemotableByTest.java, as ErrorCodes is marked @deprecated(forRemoval = true).
    
    Also removed direct use of ErrorCodes.SUCCESS and related methods like errorCodes.toStatusCode(e) in preparation for full W3C compliance, where status and state fields are no longer needed or recommended in responses.
    
    This change aligns the codebase with the W3C WebDriver protocol by reducing reliance on deprecated JSON Wire Protocol conventions.
    @selenium-ci selenium-ci added the C-java Java Bindings label Jun 23, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Error

    The createError method now sets state to res.getStatus().toString() but getStatus() may return null or unexpected values since setStatus() calls were removed elsewhere. This could cause runtime issues or incorrect error responses.

    res.setState(res.getStatus().toString());
    res.setValue(ErrorCodec.createDefault().encode(e));
    Inconsistent Change

    Only setState() call was replaced while setStatus() was removed entirely. This asymmetric change might break the Response object's expected state/status relationship and could affect protocol compliance.

    response.setState("success");
    return response;

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix null status reference

    The res.getStatus() is called before setting any status, which will likely
    return null or a default value. This could cause a NullPointerException or
    incorrect error state. Set the status first before using it to determine the
    state.

    java/test/org/openqa/selenium/remote/RemotableByTest.java [205-210]

     private Response createError(Exception e) {
       Response res = new Response();
    +  res.setStatus(ErrorCodes.toStatusCode(e));
       res.setState(res.getStatus().toString());
       res.setValue(ErrorCodec.createDefault().encode(e));
       return res;
     }
    • Apply / Chat
    Suggestion importance[1-10]: 10

    __

    Why: The suggestion correctly identifies a NullPointerException introduced in the PR. The Response object res is newly instantiated, and its status is not set. Calling res.getStatus() will return null, and subsequently calling .toString() on it will cause a crash. This is a critical bug in the test helper method.

    High
    • More

    @manuelsblanco
    Copy link
    Contributor Author

    If this PR is accepted, my idea is to clean up the entire project of deprecated error codes, one at a time

    @cgoldberg cgoldberg changed the title refactor: replace deprecated ErrorCodes.SUCCESS_STRING [Java] refactor: replace deprecated ErrorCodes.SUCCESS_STRING Jun 23, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants