Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Mar 18, 2025

User description

Motivation and Context

This PR changes the Firefox Service class so it always passes the --websocket-port arg when starting geckodriver. The port is chosen automatically. Previously, it was only set when the --connect-existing flag was present.

This is related to #15451

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix


Description

  • Always set --websocket-port when starting geckodriver.

  • Remove conditional check for --connect-existing flag.

  • Ensure consistent port assignment for CDP connections.


Changes walkthrough 📝

Relevant files
Bug fix
service.py
Ensure consistent `--websocket-port` assignment in Firefox service

py/selenium/webdriver/firefox/service.py

  • Removed conditional check for --connect-existing flag.
  • Always append --websocket-port and assign a free port.
  • Simplified logic for setting CDP port.
  • +2/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Port Allocation

    The PR now always allocates a websocket port even when it might not be needed. Consider if this could cause any port exhaustion issues in environments with many parallel test runs.

    self.service_args.append("--websocket-port")
    self.service_args.append(f"{utils.free_port()}")

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Track websocket port value

    Store the websocket port value in an instance variable so it can be accessed
    later if needed, rather than just appending it to service_args without tracking
    the actual port value.

    py/selenium/webdriver/firefox/service.py [61-62]

    +self.websocket_port = utils.free_port()
     self.service_args.append("--websocket-port")
    -self.service_args.append(f"{utils.free_port()}")
    +self.service_args.append(f"{self.websocket_port}")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Storing the websocket port in an instance variable improves the API by making the port accessible to other methods or external code. This enhances maintainability and allows for potential future features that might need to know the websocket port value.

    Medium
    • More

    @titusfortner
    Copy link
    Member

    titusfortner commented Mar 18, 2025

    Well, just checked and it looks like it was the original behavior that was changed in 68cd181 466621d

    So we need to figure out why we (I?) needed to change it.

    @titusfortner
    Copy link
    Member

    Oh! I can read code. The current code is to send a new port number if not reusing the same driver - #10598

    So I don't think we need this PR, Python & .NET already do the right thing. 😂
    We just need to look at Ruby/Java/JS support connecting to existing

    @cgoldberg
    Copy link
    Member Author

    I read it backwards too... it only adds the port if --connect-existing is NOT passed. So yea, we don't need this.

    @cgoldberg cgoldberg deleted the py-geckodriver-websocket-port branch March 20, 2025 16:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants