Skip to content

[rb] ensure driver process is always stopped#15635

Merged
titusfortner merged 2 commits intotrunkfrom
stop_driver
Dec 10, 2025
Merged

[rb] ensure driver process is always stopped#15635
titusfortner merged 2 commits intotrunkfrom
stop_driver

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Apr 16, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

one part of #15634

💥 What does this PR do?

  • moves #service_url method from superclass to the applicable module (removes it from Remote driver where it isn't needed)
  • add rescue to all local driver initialization to ensure drivers are stopped

🔧 Implementation Notes

It might be more obvious to duplicate this in each local driver, but I actually like adding the block here because it also removes the array return that I hate anyway.

💡 Additional Considerations

This should be good as is

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix

Description

  • Refactored initialize_local_driver to ensure driver processes are stopped.

  • Moved service_url method from common/driver.rb to common/local_driver.rb.

  • Updated driver initialization for Chrome, Edge, Firefox, IE, and Safari to use a block for error handling.

  • Added error handling to stop service manager on WebDriver errors.

Changes walkthrough 📝

Relevant files
Bug fix
6 files
driver.rb
Refactored Chrome driver initialization for error handling
 +3/-2     
local_driver.rb
Added error handling and moved `service_url` method           
+11/-1   
driver.rb
Refactored Edge driver initialization for error handling 
+3/-2     
driver.rb
Refactored Firefox driver initialization for error handling
 +3/-2     
driver.rb
Refactored IE driver initialization for error handling     
+3/-2     
driver.rb
Refactored Safari driver initialization for error handling
 +3/-2     
Cleanup
1 files
driver.rb
Removed `service_url` method from common driver                   
+0/-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.
    •

    @selenium-ci selenium-ci added the C-rb Ruby Bindings label Apr 16, 2025
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 16, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()
    • Ensure JavaScript events are properly triggered in Firefox 42.0

    5678 - PR Code Verified

    Compliant requirements:

    • Ensure proper cleanup of ChromeDriver processes between instantiations

    Requires further human verification:

    • Fix "ConnectFailure (Connection refused)" errors when instantiating ChromeDriver multiple times - need to verify if this PR completely resolves the issue

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

    Error Handling

    The rescue block only handles WebDriverError exceptions. Consider if other exception types should also trigger service manager cleanup to prevent orphaned processes.

     
    begin
  yield(caps, url) if block_given?
rescue Selenium::WebDriver::Error::WebDriverError
  @service_manager&.stop
  raise
end

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 16, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @titusfortner titusfortner requested a review from p0deje April 16, 2025 22:04
    @github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Oct 15, 2025
    @titusfortner titusfortner requested a review from aguspe October 22, 2025 15:26
    @titusfortner titusfortner removed the J-stale Applied to issues that become stale, and eventually closed. label Oct 22, 2025
    @titusfortner titusfortner merged commit b154cc7 into trunk Dec 10, 2025
    31 checks passed
    @titusfortner titusfortner deleted the stop_driver branch December 10, 2025 21:31
    This was referenced Jan 19, 2026
    Closed
    Closed
    Closed
    Merged
    Merged
    Closed
    Merged
    Open
    Open
    Closed
    Closed
    Closed
    Merged
    Merged
    Merged
    Closed
    Closed
    This was referenced Jan 19, 2026
    Merged
    Closed
    Merged
    Closed
    Merged
    Open
    Merged
    Closed
    Closed
    Merged
    This was referenced Feb 1, 2026
    Closed
    Closed
    This was referenced Feb 11, 2026
    Closed
    Closed
    Closed
    This was referenced Feb 20, 2026
    Merged
    Open
    Closed
    Open
    Closed
    Open
    Open
    Open
    Open
    Open
    Open
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @p0deje p0deje Awaiting requested review from p0deje

    @aguspe aguspe Awaiting requested review from aguspe

    Assignees

    No one assigned

    Labels

    C-rb Ruby Bindings Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @titusfortner @selenium-ci