Skip to content

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
    @qodo-merge-pro
    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;

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 23, 2025

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

    @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
    Copy link
    Contributor

    @asolntsev asolntsev left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Ok, deprecated was integer constant ErrorCodes.SUCCESS and method response.setStatus(int).
    But method response.setState(string) was not deprecated.

    Maybe it's still a good idea to hold constant SUCCESS_STRING = "success" instead of duplicating string "success" in multiple places?

    @manuelsblanco manuelsblanco deleted the error-codes branch October 11, 2025 17:29
    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.

    3 participants