Skip to content

[py] Return HTTP response reason when remote connection error occurs #15942

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 4 commits into
base: trunk
Choose a base branch
from

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Jun 23, 2025

User description

🔗 Related Issues

Fixes #15931

💥 What does this PR do?

This PR updates the _request method in RemoteConnection, so an HTTP response reason is returned when a Remote WebDriver server returns an HTTP error.

Previously, if an HTTP response was returned with a status code >= 500 that had no response body, it would return a blank value. With this update, any HTTP error (response code >= 400) will return the response reason as the value. This will lead to more clear exceptions being raised when this occurs (previously it would raise an exception with a blank message).

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Return HTTP response reason for remote connection errors

  • Improve error handling for status codes >= 400

  • Provide clearer exception messages for HTTP errors


Changes walkthrough 📝

Relevant files
Bug fix
remote_connection.py
Improve HTTP error response handling                                         

py/selenium/webdriver/remote/remote_connection.py

  • Modified HTTP error handling logic to include response reason
  • Changed condition from 399 < statuscode <= 500 to statuscode >= 400
  • Added response reason to error value when no response body exists
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-py Python 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 Change

    The condition changed from handling only 400-500 status codes to all 400+ codes, which now includes 5xx server errors that were previously handled differently. This broadens the scope significantly and may change existing behavior for server errors.

    if statuscode >= 400:
        return {"status": statuscode, "value": f"{response.reason}" if not data else data.strip()}
    Potential Issue

    The response.reason might be None or empty in some HTTP implementations, which could result in an empty string being returned as the value when no data exists.

        return {"status": statuscode, "value": f"{response.reason}" if not data else data.strip()}
    content_type = []

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add fallback for None reason
    Suggestion Impact:The commit modified the same line as suggested, but implemented a different approach - removing the f-string formatting instead of adding the fallback logic

    code diff:

    -                return {"status": statuscode, "value": f"{response.reason}" if not data else data.strip()}
    +                return {"status": statuscode, "value": response.reason if not data else data.strip()}

    The response.reason might be None in some cases, which would result in "None"
    being displayed. Add a fallback to handle cases where the reason is not
    available.

    py/selenium/webdriver/remote/remote_connection.py [443]

    -return {"status": statuscode, "value": f"{response.reason}" if not data else data.strip()}
    +return {"status": statuscode, "value": f"{response.reason or statuscode}" if not data else data.strip()}

    [Suggestion processed]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that response.reason could be None and provides a sensible fallback to using the statuscode, similar to the original behavior. This improves the robustness of the error handling.

    Medium
    • Update

    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.

    [🐛 Bug]: Causes of selenium.common.exceptions.WebDriverException
    2 participants