[rb] Allow driver path to be set using ENV variables

[aguspe]

Description

I added a new method on the common service class that sets the path equal to the value of the environment variable for the respective driver or returns nil

Motivation and Context

As explained in #14045 the goal of this PR is to provide users with an alternative option to set the path to their own drivers if they do not want to use selenium manager

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.

[qodo-merge-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Potential Performance Issue The env_path method is called in the initialize method, which could lead to unnecessary environment variable lookups even when a path is provided. Code Duplication The find_driver_path method duplicates logic that already exists in the DriverFinder class. Consider refactoring to avoid duplication.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add a guard clause to handle potential nil value and prevent NoMethodError Consider using a guard clause to handle the case when class_name is nil to avoid potential NoMethodError when calling upcase on nil. rb/lib/selenium/webdriver/common/service.rb [104-109] def env_path class_name = self.class.name&.split('::')&.[](2)&.downcase + return nil if class_name.nil? driver_name = class_name == 'firefox' ? 'gecko' : class_name - parsed_driver = "SE_#{driver_name&.upcase}DRIVER" + parsed_driver = "SE_#{driver_name.upcase}DRIVER" ENV.fetch(parsed_driver, nil) end Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding a guard clause to handle potential nil values is a significant improvement that prevents runtime errors, enhancing the robustness of the code.	9
Enhancement	Rename method to better reflect its purpose of retrieving the driver path from environment variables Consider using a more descriptive name for the env_path method, such as get_driver_path_from_env, to better reflect its purpose of retrieving the driver path from environment variables. rb/lib/selenium/webdriver/common/service.rb [104-109] -def env_path +def get_driver_path_from_env class_name = self.class.name&.split('::')&.[](2)&.downcase driver_name = class_name == 'firefox' ? 'gecko' : class_name parsed_driver = "SE_#{driver_name&.upcase}DRIVER" ENV.fetch(parsed_driver, nil) end Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to rename the method to get_driver_path_from_env improves code readability by making the method's purpose clearer. However, this is a minor enhancement and not critical to functionality.	7
	Rename method to better reflect its action of retrieving a driver path Consider using fetch_driver_path instead of find_driver_path to better reflect that the method is retrieving a path rather than searching for it. rb/lib/selenium/webdriver/common/service.rb [99-102] -def find_driver_path +def fetch_driver_path default_options = WebDriver.const_get("#{self.class.name&.split('::')&.[](2)}::Options").new DriverFinder.new(default_options, self).driver_path end Apply this suggestion Suggestion importance[1-10]: 6 Why: Renaming the method to fetch_driver_path provides a clearer understanding of its functionality, but it is a minor readability improvement and not essential to the code's operation.	6
Best practice	Improve readability of ternary operator condition Consider using a more explicit condition in the ternary operator for better readability and maintainability. rb/lib/selenium/webdriver/common/service.rb [106] -driver_name = class_name == 'firefox' ? 'gecko' : class_name +driver_name = (class_name == 'firefox') ? 'gecko' : class_name Apply this suggestion Suggestion importance[1-10]: 5 Why: While the suggestion slightly improves readability, the change is minimal and does not significantly impact the code's clarity or maintainability.	5

[aguspe]

The only test failing is an unrelated Edge test, which I will create a PR for but I need #14433 to be merged first

[p0deje]

Few comments from my side:

1. What do you think about making the code more explicit rather than relying on class names? For example, we can make every service class have a constant specifying which environment variable can be used to override the finder behavior. This would make it easier for anyone looking at the code and we would be able to document it.

module Selenium
  module WebDriver
    module Safari
      class Service
        # You can explicitly specify the location of the driver
        # using SE_SAFARIDRIVER="..." environment variable.
        DRIVER_PATH_ENV_KEY = "SE_SAFARIDRIVER"

2. Looking at the specs, I feel like they belong more to the unit tests, rather than integration ones because they don't need to start the driver. Can you update accordingly?

[aguspe]

DRIVER_PATH_ENV_KEY = "SE_SAFARIDRIVER"

1 - That's a great idea with the constant to have the env variable, I just changed that
2 - I think this is more of an integration test also because when I try to run it as a unit test locally I have this error related to selenium manager:

0) Selenium::WebDriver::Chrome::Service when initializing driver with a path env variable updates the path after setting the environment variable
     Failure/Error: raise Error::NoSuchDriverError, "Unable to obtain #{exe}"

     Selenium::WebDriver::Error::NoSuchDriverError:
       Unable to obtain chromedriver; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors/driver_location
     # ./lib/selenium/webdriver/common/driver_finder.rb:75:in `rescue in paths'
     # ./lib/selenium/webdriver/common/driver_finder.rb:49:in `paths'
     # ./lib/selenium/webdriver/common/driver_finder.rb:39:in `driver_path'
     # ./spec/unit/selenium/webdriver/chrome/service_spec.rb:124:in `block (4 levels) in <module:Chrome>'
     # ------------------
     # --- Caused by: ---
     # Selenium::WebDriver::Error::WebDriverError:
     #   not a file: "/Users/apeque01/Desktop/main_folder/Projects/open_source/selenium/rb/bin/macos/selenium-manager"
     ```

[p0deje]

I think this is more of an integration test also because when I try to run it as a unit test locally I have this error related to selenium manager:

Yes, if you want to test Selenium Manager - it would be an integration test. However, if you only want to test the logic for fetching driver path via ENV - it should be enough to write a unit test.

[aguspe]

I think this is more of an integration test also because when I try to run it as a unit test locally I have this error related to selenium manager:

Yes, if you want to test Selenium Manager - it would be an integration test. However, if you only want to test the logic for fetching driver path via ENV - it should be enough to write a unit test.

That's a great point, I moved everything as a unit test know because you are right the main core of this test is to fetch the ENV value

I ran all the unit tests locally and all seem to be good so it's ready for re-review @p0deje

[diemol]

Are there docs for this?

[p0deje]

Are there docs for this?

Not really, we probably need to add it somewhere in https://www.selenium.dev/documentation/selenium_manager/?

[aguspe]

@p0deje @diemol I can create a PR to update the docs tonight :) I will ping it here too

[diemol]

@aguspe do you have time to add docs for this?

[diemol]

@aguspe I wrote a comment and I did not see your reply, sorry!

[aguspe]

Sorry for the delay but here is the PR for the doc update: SeleniumHQ/seleniumhq.github.io#1950

Let me know if any changes or more info is needed!
I did not add any translations since I can only help with Spanish, Italian and Danish