[rb] update SOC for driver finder and selenium manager classes (#13386)

[rb] move driver finding logic out of selenium manager class

Co-authored-by: Diego Molina <diemol@users.noreply.github.com>

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

@@ -20,25 +20,64 @@
 module Selenium
   module WebDriver
     class DriverFinder
-      def self.path(options, klass)
-        path = klass.driver_path
-        path = path.call if path.is_a?(Proc)
+      def initialize(options, service)
+        @options = options
+        @service = service
+      end
+
+      def browser_path
+        paths[:browser_path]
+      end
+
+      def driver_path
+        paths[:driver_path]
+      end
+
+      def browser_path?
+        !browser_path.nil? && !browser_path.empty?
+      end
+
+      private
 
-        path ||= begin
-          SeleniumManager.driver_path(options) unless options.is_a?(Remote::Capabilities)
+      def paths
+        @paths ||= begin
+          path = @service.class.driver_path
+          path = path.call if path.is_a?(Proc)
+          exe = @service.class::EXECUTABLE
+          if path
+            WebDriver.logger.debug("Skipping Selenium Manager; path to #{exe} specified in service class: #{path}")
+            Platform.assert_executable(path)
+            {driver_path: path}
+          else
+            output = SeleniumManager.binary_paths(*to_args)
+            formatted = {driver_path: Platform.cygwin_path(output['driver_path'], only_cygwin: true),
+                         browser_path: Platform.cygwin_path(output['browser_path'], only_cygwin: true)}
+            Platform.assert_executable(formatted[:driver_path])
+            Platform.assert_executable(formatted[:browser_path])
+            formatted
+          end
         rescue StandardError => e
-          raise Error::NoSuchDriverError, "Unable to obtain #{klass::EXECUTABLE} using Selenium Manager; #{e.message}"
+          WebDriver.logger.error("Exception occurred: #{e.message}")
+          WebDriver.logger.error("Backtrace:\n\t#{e.backtrace&.join("\n\t")}")
+          raise Error::NoSuchDriverError, "Unable to obtain #{exe}"
         end
+      end
 
-        begin
-          Platform.assert_executable(path)
-        rescue TypeError
-          raise Error::NoSuchDriverError, "Unable to locate or obtain #{klass::EXECUTABLE}"
-        rescue Error::WebDriverError => e
-          raise Error::NoSuchDriverError, "#{klass::EXECUTABLE} located, but: #{e.message}"
+      def to_args
+        args = ['--browser', @options.browser_name]
+        if @options.browser_version
+          args << '--browser-version'
+          args << @options.browser_version
         end
-
-        path
+        if @options.respond_to?(:binary) && !@options.binary.nil?
+          args << '--browser-path'
+          args << @options.binary.gsub('\\', '\\\\\\')
+        end
+        if @options.proxy
+          args << '--proxy'
+          args << (@options.proxy.ssl || @options.proxy.http)
+        end
+        args
       end
     end
   end

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

@@ -38,7 +38,14 @@ def process_options(options, service)
           raise ArgumentError, ":options must be an instance of #{default_options.class}"
         end
 
-        service.executable_path ||= WebDriver::DriverFinder.path(options, service.class)
+        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
         options.as_json
       end
     end

rb/lib/selenium/webdriver/common/platform.rb

@@ -111,7 +111,9 @@ def wrap_in_quotes_if_necessary(str)
         windows? && !cygwin? ? %("#{str}") : str
       end
 
-      def cygwin_path(path, **opts)
+      def cygwin_path(path, only_cygwin: false, **opts)
+        return path if only_cygwin && !cygwin?
+
         flags = []
         opts.each { |k, v| flags << "--#{k}" if v }
 

rb/lib/selenium/webdriver/common/selenium_manager.rb

@@ -34,88 +34,33 @@ def bin_path
           @bin_path ||= '../../../../../bin'
         end
 
-        # @param [Options] options browser options.
-        # @return [String] the path to the correct driver.
-        def driver_path(options)
-          command = generate_command(binary, options)
-
-          output = run(*command)
-
-          browser_path = Platform.cygwin? ? Platform.cygwin_path(output['browser_path']) : output['browser_path']
-          driver_path = Platform.cygwin? ? Platform.cygwin_path(output['driver_path']) : output['driver_path']
-          Platform.assert_executable driver_path
-
-          if options.respond_to?(:binary) && browser_path && !browser_path.empty?
-            options.binary = browser_path
-            options.browser_version = nil
-          end
-
-          driver_path
+        # @param [Array] arguments what gets sent to to Selenium Manager binary.
+        # @return [Hash] paths to the requested assets.
+        def binary_paths(*arguments)
+          arguments += %w[--language-binding ruby]
+          arguments += %w[--output json]
+          arguments << '--debug' if WebDriver.logger.debug?
+
+          run(binary, *arguments)
         end
 
         private
 
-        def generate_command(binary, options)
-          command = [binary, '--browser', options.browser_name]
-          if options.browser_version
-            command << '--browser-version'
-            command << options.browser_version
-          end
-          if options.respond_to?(:binary) && !options.binary.nil?
-            command << '--browser-path'
-            command << options.binary.squeeze('\\').gsub('\\', '\\\\\\')
-          end
-          if options.proxy
-            command << '--proxy'
-            command << (options.proxy.ssl || options.proxy.http)
-          end
-          command
-        end
-
         # @return [String] the path to the correct selenium manager
         def binary
           @binary ||= begin
-            location = ENV.fetch('SE_MANAGER_PATH', begin
-              directory = File.expand_path(bin_path, __FILE__)
-              if Platform.windows?
-                "#{directory}/windows/selenium-manager.exe"
-              elsif Platform.mac?
-                "#{directory}/macos/selenium-manager"
-              elsif Platform.linux?
-                "#{directory}/linux/selenium-manager"
-              elsif Platform.unix?
-                WebDriver.logger.warn('Selenium Manager binary may not be compatible with Unix; verify settings',
-                                      id: %i[selenium_manager unix_binary])
-                "#{directory}/linux/selenium-manager"
-              end
-            rescue Error::WebDriverError => e
-              raise Error::WebDriverError, "Unable to obtain Selenium Manager binary for #{e.message}"
-            end)
+            if (location = ENV.fetch('SE_MANAGER_PATH', nil))
+              WebDriver.logger.debug("Selenium Manager set by ENV['SE_MANAGER_PATH']: #{location}")
+            end
+            location ||= platform_location
 
-            validate_location(location)
-            location
-          end
-        end
-
-        def validate_location(location)
-          begin
-            Platform.assert_file(location)
             Platform.assert_executable(location)
-          rescue TypeError
-            raise Error::WebDriverError,
-                  "Unable to locate or obtain Selenium Manager binary; #{location} is not a valid file object"
-          rescue Error::WebDriverError => e
-            raise Error::WebDriverError, "Selenium Manager binary located, but #{e.message}"
+            WebDriver.logger.debug("Selenium Manager binary found at #{location}", id: :selenium_manager)
+            location
           end
-
-          WebDriver.logger.debug("Selenium Manager binary found at #{location}", id: :selenium_manager)
         end
 
         def run(*command)
-          command += %w[--language-binding ruby]
-          command += %w[--output json]
-          command << '--debug' if WebDriver.logger.debug?
-
           WebDriver.logger.debug("Executing Process #{command}", id: :selenium_manager)
 
           begin
@@ -124,16 +69,34 @@ def run(*command)
             raise Error::WebDriverError, "Unsuccessful command executed: #{command}; #{e.message}"
           end
 
-          json_output = stdout.empty? ? {} : JSON.parse(stdout)
-          (json_output['logs'] || []).each do |log|
+          json_output = stdout.empty? ? {'logs' => [], 'result' => {}} : JSON.parse(stdout)
+          json_output['logs'].each do |log|
             level = log['level'].casecmp('info').zero? ? 'debug' : log['level'].downcase
             WebDriver.logger.send(level, log['message'], id: :selenium_manager)
           end
 
           result = json_output['result']
-          return result unless status.exitstatus.positive?
+          return result unless status.exitstatus.positive? || result.nil?
 
-          raise Error::WebDriverError, "Unsuccessful command executed: #{command}\n#{result}#{stderr}"
+          raise Error::WebDriverError,
+                "Unsuccessful command executed: #{command} - Code #{status.exitstatus}\n#{result}\n#{stderr}"
+        end
+
+        def platform_location
+          directory = File.expand_path(bin_path, __FILE__)
+          if Platform.windows?
+            "#{directory}/windows/selenium-manager.exe"
+          elsif Platform.mac?
+            "#{directory}/macos/selenium-manager"
+          elsif Platform.linux?
+            "#{directory}/linux/selenium-manager"
+          elsif Platform.unix?
+            WebDriver.logger.warn('Selenium Manager binary may not be compatible with Unix',
+                                  id: %i[selenium_manager unix_binary])
+            "#{directory}/linux/selenium-manager"
+          else
+            raise Error::WebDriverError, "unsupported platform: #{Platform.os}"
+          end
         end
       end
     end # SeleniumManager

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

@@ -89,7 +89,7 @@ def initialize(path: nil, port: nil, log: nil, args: nil)
       def launch
         @executable_path ||= begin
           default_options = WebDriver.const_get("#{self.class.name&.split('::')&.[](2)}::Options").new
-          DriverFinder.path(default_options, self.class)
+          DriverFinder.new(default_options, self).driver_path
         end
         ServiceManager.new(self).tap(&:start)
       end

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

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

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

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

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

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

rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb

@@ -33,6 +33,7 @@ module Chrome
            body: {value: {sessionId: 0, capabilities: {browserName: 'chrome'}}}.to_json,
            headers: {content_type: 'application/json'}}
         end
+        let(:finder) { instance_double(DriverFinder, browser_path?: false, driver_path: '/path/to/driver') }
 
         def expect_request(body: nil, endpoint: nil)
           body = (body || {capabilities: {alwaysMatch: {browserName: 'chrome', 'goog:chromeOptions': {}}}}).to_json
@@ -45,44 +46,46 @@ def expect_request(body: nil, endpoint: nil)
         end
 
         it 'uses DriverFinder when provided Service without path' do
-          allow(DriverFinder).to receive(:path)
+          allow(DriverFinder).to receive(:new).and_return(finder)
           expect_request
           options = Options.new
 
           described_class.new(service: service, options: options)
-          expect(DriverFinder).to have_received(:path).with(options, service.class)
+          expect(finder).to have_received(:driver_path)
         end
 
         it 'does not use DriverFinder when provided Service with path' do
           expect_request
           allow(service).to receive(:executable_path).and_return('path')
-          allow(DriverFinder).to receive(:path)
+          allow(DriverFinder).to receive(:new).and_return(finder)
 
           described_class.new(service: service)
-          expect(DriverFinder).not_to have_received(:path)
+          expect(finder).not_to have_received(:driver_path)
         end
 
         it 'does not require any parameters' do
-          allow(DriverFinder).to receive(:path).and_return('path')
-          allow(Platform).to receive(:assert_file)
-          allow(Platform).to receive(:assert_executable)
-
+          allow(DriverFinder).to receive(:new).and_return(finder)
           expect_request
 
           expect { described_class.new }.not_to raise_exception
         end
 
         it 'accepts provided Options as sole parameter' do
-          allow(DriverFinder).to receive(:path).and_return('path')
-          allow(Platform).to receive(:assert_file)
-          allow(Platform).to receive(:assert_executable)
+          allow(DriverFinder).to receive(:new).and_return(finder)
 
           opts = {args: ['-f']}
           expect_request(body: {capabilities: {alwaysMatch: {browserName: 'chrome', 'goog:chromeOptions': opts}}})
 
           expect { described_class.new(options: Options.new(**opts)) }.not_to raise_exception
         end
 
+        it 'raises an ArgumentError if parameter is not recognized' do
+          allow(DriverFinder).to receive(:new).and_return(finder)
+
+          msg = 'unknown keyword: :invalid'
+          expect { described_class.new(invalid: 'foo') }.to raise_error(ArgumentError, msg)
+        end
+
         it 'does not accept Options of the wrong class' do
           expect {
             described_class.new(options: Options.firefox)

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

@@ -91,6 +91,7 @@ module Chrome
           end
           let(:service_manager) { instance_double(ServiceManager, uri: 'http://example.com') }
           let(:bridge) { instance_double(Remote::Bridge, quit: nil, create_session: {}) }
+          let(:finder) { instance_double(DriverFinder, browser_path?: false, driver_path: '/path/to/driver') }
 
           before do
             allow(Remote::Bridge).to receive(:new).and_return(bridge)
@@ -104,19 +105,15 @@ module Chrome
           end
 
           it 'is created when :url is not provided' do
-            allow(DriverFinder).to receive(:path).and_return('path')
-            allow(Platform).to receive(:assert_file)
-            allow(Platform).to receive(:assert_executable)
+            allow(DriverFinder).to receive(:new).and_return(finder)
             allow(described_class).to receive(:new).and_return(service)
 
             driver.new
             expect(described_class).to have_received(:new).with(no_args)
           end
 
           it 'accepts :service without creating a new instance' do
-            allow(DriverFinder).to receive(:path).and_return('path')
-            allow(Platform).to receive(:assert_file)
-            allow(Platform).to receive(:assert_executable)
+            allow(DriverFinder).to receive(:new).and_return(finder)
             allow(described_class).to receive(:new)
 
             driver.new(service: service)