Skip to content

Let firefox choose the bidi port by default #15727

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

Merged
merged 2 commits into from
May 10, 2025

Conversation

tomhughes
Copy link
Contributor

@tomhughes tomhughes commented May 9, 2025

User description

If the user hasn't asked for a specific port then just let firefox choose a random one.

🔗 Related Issues

Fixes #15707.

💥 What does this PR do?

Prevents race conditions choosing a bidi port by letting firefox choose.

🔧 Implementation Notes

Implementation is as suggested in #15707 (comment).

💡 Additional Considerations

None.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Let Firefox choose BiDi port by default to avoid race conditions

  • Update service initialization to use '--websocket-port=0' unless specified

  • Remove and revise tests for websocket port assignment logic

  • Add new unit tests for argument handling in service initialization


Changes walkthrough 📝

Relevant files
Bug fix
service.rb
Default to Firefox-chosen BiDi port in service initialization

rb/lib/selenium/webdriver/firefox/service.rb

  • Change default '--websocket-port' argument to '0' unless specified
  • Prevents manual port probing and potential race conditions
  • Adjust logic to skip adding argument if already present
  • +2/-2     
    Tests
    service_spec.rb
    Remove integration test for random websocket port assignment

    rb/spec/integration/selenium/webdriver/firefox/service_spec.rb

  • Remove test asserting different websocket ports for two services
  • Clean up BiDi port assignment expectations in integration tests
  • +0/-15   
    service_spec.rb
    Revise and add unit tests for websocket port argument handling

    rb/spec/unit/selenium/webdriver/firefox/service_spec.rb

  • Update test to expect websocket port '0' by default
  • Add test to ensure custom websocket port is respected
  • Add test to ensure '--websocket-port' is not duplicated
  • +10/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • If the user hasn't asked for a specific port then just let
    firefox choose a random one.
    @CLAassistant
    Copy link

    CLAassistant commented May 9, 2025

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    qodo-merge-pro bot commented May 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15707 - Fully compliant

    Compliant requirements:

    • Fix issue with Firefox 136+ failing when multiple instances are started in parallel
    • Address race condition where multiple threads can allocate the same BiDi port
    • Ensure Firefox instances can start properly in parallel testing environments
    • Fix regression in Selenium 4.32.0 ruby bindings

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

    Argument Handling

    The condition to check for existing arguments could be more robust. Currently it checks if any argument "includes" the string, which might match partial arguments. Consider using exact matching or regex patterns for more precise detection.

    unless args.any? { |arg| arg.include?('--connect-existing') || arg.include?('--websocket-port') }
      args << '--websocket-port'

    Copy link
    Contributor

    qodo-merge-pro bot commented May 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve argument matching precision

    The current implementation only checks if arguments contain the string
    '--websocket-port', which could match partial arguments like
    '--websocket-port-timeout'. Use a more precise check to ensure exact matching of
    the '--websocket-port' argument.

    rb/lib/selenium/webdriver/firefox/service.rb [31-34]

    -unless args.any? { |arg| arg.include?('--connect-existing') || arg.include?('--websocket-port') }
    +unless args.any? { |arg| arg.include?('--connect-existing') || arg == '--websocket-port' || arg.start_with?('--websocket-port=') }
       args << '--websocket-port'
       args << '0'
     end
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a potential bug where partial matches like --websocket-port-timeout would incorrectly be treated as the --websocket-port argument. The improved code with exact matching using arg == '--websocket-port' or arg.start_with?('--websocket-port=') prevents this issue, making the argument handling more robust.

    Medium
    • Update

    @godofcodeeyes
    Copy link

    The documentation in this repository is excellent! Very clear and comprehensive.

    @aguspe aguspe merged commit 1e2945d into SeleniumHQ:trunk May 10, 2025
    22 checks passed
    @aguspe
    Copy link
    Contributor

    aguspe commented May 10, 2025

    Thank you for your contribution @tomhughes !

    @tomhughes tomhughes deleted the bidi-port branch May 10, 2025 15:47
    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]: Selenium 4.32.0 ruby bindings fail to start firefox in parallel testing
    5 participants