[rb] separate concerns between Service, DriverFinder, and Options (#17564)

Author: titusfortner

rb/lib/selenium/webdriver/common/driver_finder.rb

@@ -26,6 +26,8 @@ def self.path(options, service_class)
         new(options, service_class.new).driver_path
       end
 
+      # @param options [Options, nil] when nil driver parsed from Service::EXECUTABLE
+      # @param service [Service]
       def initialize(options, service)
         @options = options
         @service = service
@@ -46,24 +48,19 @@ def browser_path?
       private
 
       def paths
-        @paths ||= resolve_paths
-      rescue StandardError => e
-        WebDriver.logger.error("Exception occurred: #{e.message}")
-        WebDriver.logger.error("Backtrace:\n\t#{e.backtrace&.join("\n\t")}")
-        raise Error::NoSuchDriverError, "Unable to obtain #{@service.class::EXECUTABLE}"
+        @paths ||= begin
+          path = @service.executable_path || resolve_class_path
+          path ? paths_from_service(path) : paths_from_manager
+        rescue StandardError => e
+          WebDriver.logger.error("Exception occurred: #{e.message}")
+          WebDriver.logger.error("Backtrace:\n\t#{e.backtrace&.join("\n\t")}")
+          raise Error::NoSuchDriverError, "Unable to obtain #{@service.class::EXECUTABLE}"
+        end
       end
 
-      def resolve_paths
+      def resolve_class_path
         path = @service.class.driver_path
-        path = path.call if path.is_a?(Proc)
-        result = path ? paths_from_service(path) : paths_from_manager
-
-        # A binary (whether user-supplied or resolved by Selenium Manager) is the
-        # source of truth for the browser version, so drop any named version that
-        # would otherwise conflict with what the binary actually is.
-        @options.browser_version = nil if @options.respond_to?(:binary) && @options.binary
-
-        result
+        path.is_a?(Proc) ? path.call : path
       end
 
       def paths_from_service(path)
@@ -79,11 +76,12 @@ def paths_from_manager
                      browser_path: Platform.cygwin_path(output['browser_path'], only_cygwin: true)}
         Platform.assert_executable(formatted[:driver_path])
         Platform.assert_executable(formatted[:browser_path])
-        @options.binary ||= formatted[:browser_path] if @options.respond_to?(:binary)
         formatted
       end
 
       def to_args
+        return ['--driver', @service.class::EXECUTABLE] unless @options
+
         args = ['--browser', @options.browser_name]
         if @options.browser_version
           args << '--browser-version'

rb/lib/selenium/webdriver/common/local_driver.rb

@@ -48,14 +48,10 @@ def process_options(options, service)
           raise ArgumentError, ":options must be an instance of #{default_options.class}"
         end
 
-        service.executable_path ||= begin
-          finder = WebDriver::DriverFinder.new(options, service)
-          if options.respond_to?(:binary) && finder.browser_path?
-            options.binary = finder.browser_path
-            options.browser_version = nil
-          end
-          finder.driver_path
-        end
+        finder = WebDriver::DriverFinder.new(options, service)
+        options.binary = finder.browser_path if options.respond_to?(:binary) && finder.browser_path?
+        service.executable_path = finder.driver_path
+        options.browser_version = nil if options.respond_to?(:binary) && options.binary
         options.as_json
       end
     end

rb/lib/selenium/webdriver/common/service.rb

@@ -88,19 +88,14 @@ def initialize(path: nil, port: nil, log: nil, args: nil)
       end
 
       def launch
-        @executable_path ||= env_path || find_driver_path
+        @executable_path ||= env_path || DriverFinder.new(nil, self).driver_path
         ServiceManager.new(self).tap(&:start)
       end
 
       def shutdown_supported
         self.class::SHUTDOWN_SUPPORTED
       end
 
-      def find_driver_path
-        default_options = WebDriver.const_get("#{self.class.name&.split('::')&.[](2)}::Options").new
-        DriverFinder.new(default_options, self).driver_path
-      end
-
       def env_path
         ENV.fetch(self.class::DRIVER_PATH_ENV_KEY, nil)
       end

rb/sig/lib/selenium/webdriver/common/driver_finder.rbs

@@ -20,7 +20,7 @@ module Selenium
 
       def paths: -> untyped
 
-      def resolve_paths: -> Hash[Symbol, String]
+      def resolve_class_path: -> String?
 
       def paths_from_service: (String path) -> Hash[Symbol, String]
 

rb/sig/lib/selenium/webdriver/common/service.rbs

@@ -55,8 +55,6 @@ module Selenium
 
       def initialize: (?path: untyped?, ?port: untyped?, ?log: untyped?, ?args: untyped?) -> void
 
-      def find_driver_path: -> untyped
-
       def launch: () -> untyped
 
       def shutdown_supported: () -> untyped

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

@@ -31,7 +31,7 @@ module Chrome
         after { service_manager.stop }
 
         it 'auto uses chromedriver' do
-          service.executable_path = DriverFinder.new(Options.new, described_class.new).driver_path
+          service.executable_path = DriverFinder.new(nil, described_class.new).driver_path
 
           expect(service_manager.uri).to be_a(URI)
         end

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

@@ -31,7 +31,7 @@ module Edge
         after { service_manager.stop }
 
         it 'auto uses edgedriver' do
-          service.executable_path = DriverFinder.new(Options.new, described_class.new).driver_path
+          service.executable_path = DriverFinder.new(nil, described_class.new).driver_path
 
           expect(service_manager.uri).to be_a(URI)
         end

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

@@ -31,7 +31,7 @@ module Firefox
         after { service_manager.stop }
 
         it 'auto uses geckodriver' do
-          service.executable_path = DriverFinder.new(Options.new, described_class.new).driver_path
+          service.executable_path = DriverFinder.new(nil, described_class.new).driver_path
 
           expect(service_manager.uri).to be_a(URI)
         end

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

@@ -31,7 +31,7 @@ module IE
         after { service_manager.stop }
 
         it 'auto uses iedriver' do
-          service.executable_path = DriverFinder.new(Options.new, described_class.new).driver_path
+          service.executable_path = DriverFinder.new(nil, described_class.new).driver_path
 
           expect(service_manager.uri).to be_a(URI)
         end

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

@@ -31,7 +31,7 @@ module Safari
         after { service_manager.stop }
 
         it 'auto uses safaridriver' do
-          service.executable_path = DriverFinder.new(Options.new, described_class.new).driver_path
+          service.executable_path = DriverFinder.new(nil, described_class.new).driver_path
 
           expect(service_manager.uri).to be_a(URI)
         end