better errors, better logging, update all specs to new methods

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

@@ -27,12 +27,14 @@ def results(options, klass)
           exe = klass::EXECUTABLE
 
           if path
-            validate_files(exe, driver_path: path)
+            validate_files(driver_path: path)
           else
             begin
-              validate_files(exe, **SeleniumManager.results(to_args(options)))
+              validate_files(**SeleniumManager.results(to_args(options)))
             rescue StandardError => e
-              raise Error::NoSuchDriverError("Unable to obtain #{exe} using Selenium Manager"), e.message, e.backtrace
+              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} using Selenium Manager"
             end
           end
         end
@@ -61,13 +63,9 @@ def to_args(options)
           args
         end
 
-        def validate_files(exe, **opts)
+        def validate_files(**opts)
           opts.each_value do |value|
-            raise Error::NoSuchDriverError("Unable to locate or obtain #{exe})") if value.nil?
-
             Platform.assert_executable(value)
-          rescue Error::WebDriverError => e
-            raise Error::NoSuchDriverError("#{exe} located, but has problems"), e.message, e.backtrace
           end
           opts
         end

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

@@ -51,38 +51,31 @@ def results(*arguments)
         # @return [String] the path to the correct selenium manager
         def binary
           @binary ||= begin
-            location = ENV.fetch('SE_MANAGER_PATH', begin
+            location = ENV.fetch('SE_MANAGER_PATH', nil)
+            if location
+              WebDriver.logger.debug("Selenium Manager set by ENV['SE_MANAGER_PATH']: #{location}")
+            else
               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)
+              location = 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
+              WebDriver.logger.debug("Looking for Selenium Manager at: #{location}")
+            end
 
-            validate_location(location)
-            location
-          end
-        end
-
-        def validate_location(location)
-          raise Error::WebDriverError('Unable to locate or obtain Selenium Manager binary') if value.nil?
-
-          begin
             Platform.assert_executable(location)
-          rescue Error::WebDriverError => e
-            raise Error::WebDriverError, "Selenium Manager binary located, but has problems: #{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)
@@ -103,7 +96,7 @@ def run(*command)
           result = json_output['result']
           return result unless status.exitstatus.positive?
 
-          raise Error::WebDriverError, "Unsuccessful command executed: #{command}\n#{result}#{stderr}"
+          raise Error::WebDriverError, "Unsuccessful command executed: #{command}\n#{result}\n#{stderr}"
         end
       end
     end # SeleniumManager

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

@@ -45,25 +45,25 @@ 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(:results).and_return({})
           expect_request
           options = Options.new
 
           described_class.new(service: service, options: options)
-          expect(DriverFinder).to have_received(:path).with(options, service.class)
+          expect(DriverFinder).to have_received(:results).with(options, service.class)
         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(:results).and_return({})
 
           described_class.new(service: service)
-          expect(DriverFinder).not_to have_received(:path)
+          expect(DriverFinder).not_to have_received(:results)
         end
 
         it 'does not require any parameters' do
-          allow(DriverFinder).to receive(:path).and_return('path')
+          allow(DriverFinder).to receive(:results).and_return({})
           allow(Platform).to receive(:assert_file)
           allow(Platform).to receive(:assert_executable)
 
@@ -73,7 +73,7 @@ def expect_request(body: nil, endpoint: nil)
         end
 
         it 'accepts provided Options as sole parameter' do
-          allow(DriverFinder).to receive(:path).and_return('path')
+          allow(DriverFinder).to receive(:results).and_return({})
           allow(Platform).to receive(:assert_file)
           allow(Platform).to receive(:assert_executable)
 
@@ -83,6 +83,14 @@ def expect_request(body: nil, endpoint: nil)
           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_messages(path: 'path', results: {})
+          allow(Platform).to receive(:assert_file)
+          allow(Platform).to receive(:assert_executable)
+          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

@@ -104,7 +104,7 @@ module Chrome
           end
 
           it 'is created when :url is not provided' do
-            allow(DriverFinder).to receive(:path).and_return('path')
+            allow(DriverFinder).to receive(:results).and_return({})
             allow(Platform).to receive(:assert_file)
             allow(Platform).to receive(:assert_executable)
             allow(described_class).to receive(:new).and_return(service)
@@ -114,7 +114,7 @@ module Chrome
           end
 
           it 'accepts :service without creating a new instance' do
-            allow(DriverFinder).to receive(:path).and_return('path')
+            allow(DriverFinder).to receive(:results).and_return({})
             allow(Platform).to receive(:assert_file)
             allow(Platform).to receive(:assert_executable)
             allow(described_class).to receive(:new)