[rb] process output in driver finder not selenium manager

Author: titusfortner

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

@@ -26,17 +26,22 @@ def result(options, klass)
           path = path.call if path.is_a?(Proc)
           exe = klass::EXECUTABLE
 
-          results = if path
-                      WebDriver.logger.debug("Skipping Selenium Manager; user provided #{exe} location: #{path}")
-                      {driver_path: path}
-                    else
-                      SeleniumManager.results(*to_args(options))
-                    end
-          validate_files(**results)
+          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.result(*to_args(options))
+            result = {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(result[:driver_path])
+            Platform.assert_executable(result[:browser_path])
+            result
+          end
         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 #{exe} using Selenium Manager"
+          raise Error::NoSuchDriverError, "Unable to obtain #{exe}"
         end
 
         def path(options, klass)
@@ -62,11 +67,6 @@ def to_args(options)
           end
           args
         end
-
-        def validate_files(**opts)
-          opts.each_value { |value| Platform.assert_executable(value) }
-          opts
-        end
       end
     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

@@ -36,14 +36,12 @@ def bin_path
 
         # @param [Array] arguments what gets sent to to Selenium Manager binary.
         # @return [Hash] paths to the requested assets.
-        def results(*arguments)
+        def result(*arguments)
           arguments += %w[--language-binding ruby]
           arguments += %w[--output json]
           arguments << '--debug' if WebDriver.logger.debug?
-          output = run(binary, *arguments)
 
-          {driver_path: Platform.cygwin? ? Platform.cygwin_path(output['driver_path']) : output['driver_path'],
-           browser_path: Platform.cygwin? ? Platform.cygwin_path(output['browser_path']) : output['browser_path']}
+          run(binary, *arguments)
         end
 
         private
@@ -71,16 +69,17 @@ 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}\n#{stderr}"
+          raise Error::WebDriverError,
+                "Unsuccessful command executed: #{command} - Code #{status.exitstatus}\n#{result}\n#{stderr}"
         end
 
         def platform_location

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb

@@ -45,18 +45,15 @@ module WebDriver
       end
 
       it 'validates all returned files' do
-        allow(SeleniumManager).to receive(:result).and_return({browser_path: '/path/to/browser',
-                                                                driver_path: '/path/to/driver',
-                                                                anything: '/path/to/random'})
+        allow(SeleniumManager).to receive(:result).and_return({'browser_path' => '/path/to/browser',
+                                                               'driver_path' => '/path/to/driver'})
         allow(Platform).to receive(:assert_executable).with('/path/to/browser').and_return(true)
         allow(Platform).to receive(:assert_executable).with('/path/to/driver').and_return(true)
-        allow(Platform).to receive(:assert_executable).with('/path/to/random').and_return(true)
 
         described_class.result(Options.chrome, Chrome::Service)
 
         expect(Platform).to have_received(:assert_executable).with('/path/to/browser')
         expect(Platform).to have_received(:assert_executable).with('/path/to/driver')
-        expect(Platform).to have_received(:assert_executable).with('/path/to/random')
       end
 
       it 'wraps error with NoSuchDriverError' do
@@ -67,24 +64,27 @@ module WebDriver
             described_class.result(Options.chrome, Chrome::Service)
           }.to output(/Exception occurred: an error/).to_stderr_from_any_process
         }.to raise_error(WebDriver::Error::NoSuchDriverError,
-                         /Unable to obtain chromedriver using Selenium Manager; For documentation on this error/)
+                         /Unable to obtain chromedriver; For documentation on this error/)
       end
 
       it 'creates arguments' do
-        allow(SeleniumManager).to receive(:result).and_return({})
+        allow(SeleniumManager).to receive(:result).and_return({'browser_path' => '/path/to/browser',
+                                                               'driver_path' => '/path/to/driver'})
         proxy = instance_double(Proxy, ssl: 'proxy')
         options = Options.chrome(browser_version: 'stable', proxy: proxy, binary: 'path/to/browser')
+        allow(Platform).to receive(:assert_executable).with('/path/to/browser').and_return(true)
+        allow(Platform).to receive(:assert_executable).with('/path/to/driver').and_return(true)
 
         described_class.result(options, Chrome::Service)
 
         expect(SeleniumManager).to have_received(:result).with('--browser',
-                                                                options.browser_name,
-                                                                '--browser-version',
-                                                                options.browser_version,
-                                                                '--browser-path',
-                                                                options.binary,
-                                                                '--proxy',
-                                                                options.proxy.ssl)
+                                                               options.browser_name,
+                                                               '--browser-version',
+                                                               options.browser_version,
+                                                               '--browser-path',
+                                                               options.binary,
+                                                               '--proxy',
+                                                               options.proxy.ssl)
       end
     end
   end

rb/spec/unit/selenium/webdriver/common/selenium_manager_spec.rb

@@ -68,7 +68,7 @@ module WebDriver
       describe '.run' do
         it 'returns result if positive exit status' do
           status = instance_double(Process::Status, exitstatus: 0)
-          stdout = '{"result": "value"}'
+          stdout = '{"result": "value", "logs": []}'
           allow(Open3).to receive(:capture3).and_return([stdout, 'stderr', status])
 
           expect(described_class.send(:run, 'anything')).to eq 'value'
@@ -84,21 +84,21 @@ module WebDriver
 
         it 'errors if exit status greater than 0' do
           status = instance_double(Process::Status, exitstatus: 1)
-          stdout = '{"result": "value"}'
+          stdout = '{"result": "value", "logs": []}'
           allow(Open3).to receive(:capture3).and_return([stdout, 'stderr', status])
 
+          msg = /Unsuccessful command executed: \["anything"\] - Code 1\nvalue\nstderr/
           expect {
             described_class.send(:run, 'anything')
-          }.to raise_error(Error::WebDriverError, /Unsuccessful command executed: \["anything"\]\nvalue\nstderr/)
+          }.to raise_error(Error::WebDriverError, msg)
         end
       end
 
       describe '.results' do
-        it 'returns asset paths' do
-          allow(described_class).to receive_messages(binary: 'binary', run: {'browser_path' => '/path/to/browser',
-                                                                             'driver_path' => '/path/to/driver'})
-          expect(described_class.result('something')).to eq({browser_path: '/path/to/browser',
-                                                              driver_path: '/path/to/driver'})
+        it 'returns exact output from #run' do
+          return_map = {}
+          allow(described_class).to receive_messages(binary: 'binary', run: return_map)
+          expect(described_class.result('something')).to eq(return_map)
         end
       end
     end