feat: always return {} in appium_server_version for errors (#311)

* feat: always return {} in appium_server_version for errors

* revert unexpected chnage

Author: KazuCocoa

CHANGELOG.md

@@ -5,6 +5,7 @@ Read `release_notes.md` for commit level details.
 ## [Unreleased]
 
 ### Enhancements
+- Returns `{}` any errors in `Core#appium_server_version` to prevent errors in some cases
 
 ### Bug fixes
 

lib/appium_lib_core/common/base/bridge/mjsonwp.rb

@@ -42,6 +42,10 @@ def sessions
             execute :get_all_sessions
           end
 
+          def status
+            execute :status
+          end
+
           # For Appium
           def log_event(vendor, event)
             execute :post_log_event, {}, { vendor: vendor, event: event }

lib/appium_lib_core/common/base/bridge/w3c.rb

@@ -43,6 +43,10 @@ def sessions
             execute :get_all_sessions
           end
 
+          def status
+            execute :status
+          end
+
           # Perform touch actions for W3C module.
           # Generate +touch+ pointer action here and users can use this via +driver.action+
           # - https://seleniumhq.github.io/selenium/docs/api/rb/Selenium/WebDriver/W3CActionBuilder.html

lib/appium_lib_core/common/base/driver.rb

@@ -16,6 +16,7 @@
 require_relative 'search_context'
 require_relative 'screenshot'
 require_relative 'rotable'
+require_relative 'remote_status'
 
 module Appium
   module Core
@@ -29,6 +30,7 @@ class Driver < ::Selenium::WebDriver::Driver
         include ::Appium::Core::Base::Rotatable
         include ::Appium::Core::Base::SearchContext
         include ::Appium::Core::Base::TakesScreenshot
+        include ::Appium::Core::Base::HasRemoteStatus
 
         # Private API.
         # Do not use this for general use. Used by flutter driver to get bridge for creating a new element

lib/appium_lib_core/common/base/remote_status.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+module Appium
+  module Core
+    class Base
+      #
+      # @api private
+      #
+
+      module HasRemoteStatus
+        # Selenium binding has this ability only in Remote Binding,
+        # so this library has this method by own for safe.
+        def remote_status
+          bridge.status
+        end
+      end
+    end
+  end
+end

lib/appium_lib_core/driver.rb

@@ -437,7 +437,8 @@ def quit_driver
         nil
       end
 
-      # Returns the server's version info
+      # Returns the server's version info. This method calls +driver.remote_status+ internally
+      #
       # @return [Hash]
       #
       # @example
@@ -451,18 +452,18 @@ def quit_driver
       #         }
       #     }
       #
-      # Returns blank hash for Selenium Grid since 'remote_status' gets 500 error
+      # Returns blank hash in a case +driver.remote_status+ got an error
+      # such as Selenium Grid. It returns 500 error against 'remote_status'.
       #
       # @example
       #
       #   @core.appium_server_version #=> {}
       #
       def appium_server_version
-        @driver.remote_status
-      rescue Selenium::WebDriver::Error::ServerError => e
-        raise ::Appium::Core::Error::ServerError unless e.message.include?('status code 500')
-
-        # driver.remote_status returns 500 error for using selenium grid
+        @driver&.remote_status
+      rescue StandardError
+        # Ignore error case in a case the target appium server
+        # does not support `/status` API.
         {}
       end
 

test/test_helper.rb

@@ -406,6 +406,7 @@ def udid_name
 
   module Mock
     HEADER = { 'Content-Type' => 'application/json; charset=utf-8', 'Cache-Control' => 'no-cache' }.freeze
+    NOSESSION = 'http://127.0.0.1:4723/wd/hub'
     SESSION = 'http://127.0.0.1:4723/wd/hub/session/1234567890'
 
     def android_mock_create_session

test/unit/android/webdriver/mjsonwp/commands_test.rb

@@ -145,6 +145,23 @@ def test_finger_print
 
             assert_requested(:post, "#{SESSION}/appium/device/finger_print", times: 1)
           end
+
+          def test_remote
+            stub_request(:get, "#{NOSESSION}/status")
+              .to_return(headers: HEADER, status: 200, body: {
+                value: {
+                  build: {
+                    version: '1.21.0',
+                    'git-sh' => '5735c828f1ce00e99243368bd5a60acc70809dcd'
+                  }
+                }
+              }.to_json)
+
+            version = @driver.remote_status['build']['version']
+
+            assert_requested(:get, "#{NOSESSION}/status", times: 1)
+            assert version == '1.21.0'
+          end
         end # class CommandsTest
       end # module MJSONWP
     end # module WebDriver

test/unit/android/webdriver/w3c/commands_test.rb

@@ -139,6 +139,23 @@ def test_finger_print
 
             assert_requested(:post, "#{SESSION}/appium/device/finger_print", times: 1)
           end
+
+          def test_remote
+            stub_request(:get, "#{NOSESSION}/status")
+              .to_return(headers: HEADER, status: 200, body: {
+                value: {
+                  build: {
+                    version: '1.21.0',
+                    'git-sh' => '5735c828f1ce00e99243368bd5a60acc70809dcd'
+                  }
+                }
+              }.to_json)
+
+            version = @driver.remote_status['build']['version']
+
+            assert_requested(:get, "#{NOSESSION}/status", times: 1)
+            assert version == '1.21.0'
+          end
         end # class CommandsTest
       end # module W3C
     end # module WebDriver