Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Apr 16, 2025

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-merge-pro
    Copy link
    Contributor

    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-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 16, 2025

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