From 86e37fef437e263696523ebff3cae7bedf598b79 Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Tue, 6 Oct 2026 21:44:58 +0530 Subject: [PATCH] fix: resolve hub URL on appium_lib_core 11+ and read viewportRect as a Hash (PPLT-6148) appium_lib_core 11+ (selenium-webdriver ClientConfig) no longer sets the HTTP client's @server_url, so the SDK's reflection returned "". On BrowserStack that made every session resolve to GenericProvider: Android App Percy failed with "Missing required tag name for comparison", iOS silently skipped the AppAutomate path, and Percy on Automate posted an empty executor URL ("Comparison tag details not available"). Fall back to http.client_config.server_url when @server_url is unset. AndroidMetadata read capabilities.to_json['viewportRect'], which indexes a JSON String, so the rect was never used and every call fell back to driver.get_system_bars (the Appium 2-only `mobile: getSystemBars` on core 13+). Read it through get_capability_value instead. CI adds a Ruby 3.3 / appium_lib ~> 16.0 leg (APPIUM_LIB_VERSION) and pins minitest ~> 5.0 (minitest 6 split out Minitest::Mock). Verified live on BrowserStack with appium_lib 16.3.2 / core 13.1.4: App Percy Android+iOS on Appium 1.22.0 and 2.19.0, and Percy on Automate mobile web on 2.19.0, all 3/3 snapshots. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 6 ++++++ Gemfile | 17 +++++++++------- percy/metadata/android_metadata.rb | 14 ++++++------- percy/metadata/driver_metadata.rb | 19 +++++++++++++++++- percy/metadata/metadata.rb | 3 ++- specs/android_metadata.rb | 29 ++++++++++++++++++--------- specs/driver_metadata.rb | 32 ++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dd55572..1002dce 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -21,7 +21,13 @@ jobs: matrix: os: [ubuntu-latest] ruby: ['2.6', '2.7', '3.0'] + include: + - os: ubuntu-latest + ruby: '3.3' + appium_lib: '~> 16.0' runs-on: ${{ matrix.os }} + env: + APPIUM_LIB_VERSION: ${{ matrix.appium_lib || '~> 12.0' }} steps: - uses: actions-ecosystem/action-regex-match@v2 id: regex-match diff --git a/Gemfile b/Gemfile index 87a0d44..652b814 100644 --- a/Gemfile +++ b/Gemfile @@ -2,14 +2,17 @@ source 'https://rubygems.org' -# Pin appium_lib to the 12.x line so all Ruby legs (2.6/2.7/3.0) resolve -# appium_lib_core ~> 5.0, which still exposes Appium::Core::Base::SearchContext. -# Unpinned, Ruby 3.0 resolved appium_lib 15 / appium_lib_core 9.x, which removed -# that constant and broke the specs. appium_console 3.0.0 requires appium_lib = 12.0.0. -gem 'appium_console', '~> 3.0' -gem 'appium_lib', '~> 12.0' +# appium_lib defaults to the 12.x line so the Ruby 2.6/2.7/3.0 legs resolve +# appium_lib_core ~> 5.0. CI also runs a leg on the current major via +# APPIUM_LIB_VERSION, since appium_lib_core 11+ changed where the driver keeps its +# hub URL. appium_console 3.0.0 requires appium_lib = 12.0.0, so it only joins the +# 12.x resolution. +appium_lib_version = ENV.fetch('APPIUM_LIB_VERSION', '~> 12.0') +gem 'appium_console', '~> 3.0' if appium_lib_version.include?('12.') +gem 'appium_lib', appium_lib_version gem 'dotenv' -gem 'minitest' +# minitest 6 moved Minitest::Mock into the separate minitest-mock gem. +gem 'minitest', '~> 5.0' gem 'rubocop', require: false gem 'tempfile' gem 'webmock', '~> 3.18', '>= 3.18.1' diff --git a/percy/metadata/android_metadata.rb b/percy/metadata/android_metadata.rb index 3723008..f218c5e 100644 --- a/percy/metadata/android_metadata.rb +++ b/percy/metadata/android_metadata.rb @@ -9,12 +9,11 @@ class AndroidMetadata < Percy::Metadata def initialize(driver) super(driver) @_bars = nil - # Intentionally left as the original lookup: this path already degrades to - # driver.get_system_bars consistently across all appium_lib_core versions - # (the rect read yields a non-Hash, so the rect arithmetic in - # get_system_bars rescues to nil and falls back), so it is out of scope for - # the snake_case capability fix. - @_viewport_rect = capabilities.to_json['viewportRect'] + # Read the rect as a Hash. The previous `capabilities.to_json['viewportRect']` + # indexed a JSON String, so the rect was never used and every call fell back + # to driver.get_system_bars, which appium_lib_core 13+ sends as the + # Appium 2-only `mobile: getSystemBars` command. + @_viewport_rect = viewport end def device_screen_size @@ -71,7 +70,8 @@ def navigation_bar end def viewport - capabilities.to_json['viewportRect'] + rect = get_capability_value('viewportRect') + rect.is_a?(Hash) ? rect : nil end def scale_factor diff --git a/percy/metadata/driver_metadata.rb b/percy/metadata/driver_metadata.rb index 97ee187..f4a0ca4 100644 --- a/percy/metadata/driver_metadata.rb +++ b/percy/metadata/driver_metadata.rb @@ -8,6 +8,23 @@ def initialize(driver) @driver = driver end + # Returns the hub URL the driver talks to. appium_lib_core <= 10 keeps it in + # the HTTP client's @server_url; 11+ (selenium-webdriver ClientConfig) moved it + # to client_config.server_url and leaves @server_url unset, which made every + # BrowserStack session look like a generic remote (no AppAutomate provider, + # no device name, empty executor URL for Percy on Automate). + def self.server_url(driver) + http = driver.instance_variable_get(:@bridge).instance_variable_get(:@http) + url = http.instance_variable_get(:@server_url).to_s + return url unless url.empty? + + client_config = http.respond_to?(:client_config) ? http.client_config : nil + client_config ||= http.instance_variable_get(:@client_config) + return '' unless client_config.respond_to?(:server_url) + + client_config.server_url.to_s.chomp('/') + end + def session_id @driver.session_id end @@ -15,7 +32,7 @@ def session_id def command_executor_url url = Percy::Cache.get_cache(session_id, Percy::Cache::COMMAND_EXECUTOR_URL) if url.nil? - url = @driver.instance_variable_get(:@bridge).instance_variable_get(:@http).instance_variable_get(:@server_url).to_s + url = self.class.server_url(@driver) Percy::Cache.set_cache(session_id, Percy::Cache::COMMAND_EXECUTOR_URL, url) end url diff --git a/percy/metadata/metadata.rb b/percy/metadata/metadata.rb index a4e08aa..6bdb524 100644 --- a/percy/metadata/metadata.rb +++ b/percy/metadata/metadata.rb @@ -3,6 +3,7 @@ require 'json' require 'pathname' require_relative '../common/common' +require_relative 'driver_metadata' DEVICE_INFO_FILE_PATH = File.join(File.dirname(__FILE__), '..', 'configs', 'devices.json') DEVICE_INFO = JSON.parse(File.read(DEVICE_INFO_FILE_PATH)) @@ -82,7 +83,7 @@ def os_version end def remote_url - driver.instance_variable_get(:@bridge).instance_variable_get(:@http).instance_variable_get(:@server_url).to_s + Percy::DriverMetadata.server_url(driver) end def get_orientation(**kwargs) diff --git a/specs/android_metadata.rb b/specs/android_metadata.rb index b090619..ee4994d 100644 --- a/specs/android_metadata.rb +++ b/specs/android_metadata.rb @@ -28,7 +28,7 @@ def test_viewport android_capabilities = get_android_capabilities @mock_webdriver.expect(:capabilities, android_capabilities.merge('viewportRect' => viewport)) - assert(viewport, @android_metadata.viewport) + assert_equal(viewport, @android_metadata.viewport) @mock_webdriver.verify end @@ -61,19 +61,30 @@ def test_device_screen_size_when_device_screen_size_is_present @mock_webdriver.verify end - def test_get_system_bars + def test_get_system_bars_falls_back_to_driver_without_viewport_rect system_bars = { 'statusBar' => { 'height' => 83 }, 'navigationBar' => { 'height' => 44 } } - android_capabilities = get_android_capabilities - session_id = 'session_id_123' - @mock_webdriver.expect(:session_id, session_id) - @mock_webdriver.expect(:session_id, session_id) - @mock_webdriver.expect(:capabilities, android_capabilities.merge('viewportRect' => nil)) - @mock_webdriver.expect(:get_system_bars, system_bars) + driver = Minitest::Mock.new + driver.expect(:capabilities, get_android_capabilities.merge('viewportRect' => nil)) + metadata = Percy::AndroidMetadata.new(driver) + driver.expect(:session_id, 'session_id_no_rect') + driver.expect(:session_id, 'session_id_no_rect') + driver.expect(:get_system_bars, system_bars) + + assert_equal(system_bars, metadata.get_system_bars) + driver.verify + end + + def test_get_system_bars_uses_viewport_rect + # viewportRect top=83, height=2153 on a 1080x2280 screen (see get_android_capabilities) + @mock_webdriver.expect(:session_id, 'session_id_rect') + @mock_webdriver.expect(:capabilities, get_android_capabilities) + + bars = @android_metadata.get_system_bars - assert(system_bars, @android_metadata.get_system_bars) + assert_equal({ 'statusBar' => { 'height' => 83 }, 'navigationBar' => { 'height' => 44 } }, bars) @mock_webdriver.verify end diff --git a/specs/driver_metadata.rb b/specs/driver_metadata.rb index 9ae2c5a..f618222 100644 --- a/specs/driver_metadata.rb +++ b/specs/driver_metadata.rb @@ -39,6 +39,38 @@ def test_command_executor_url assert(url, @metadata.command_executor_url) end + # appium_lib_core 11+ leaves the HTTP client's @server_url unset and keeps the + # hub URL in client_config.server_url (a URI with a trailing slash). + class ClientConfigHttp + attr_reader :client_config + + def initialize(client_config) + @client_config = client_config + end + end + + def driver_with_http(http) + bridge = Object.new + bridge.instance_variable_set(:@http, http) + driver = Object.new + driver.instance_variable_set(:@bridge, bridge) + driver.define_singleton_method(:session_id) { "session_#{object_id}" } + driver + end + + def test_command_executor_url_reads_client_config_when_server_url_unset + client_config = Struct.new(:server_url).new(URI('https://hub-cloud.browserstack.com/wd/hub/')) + driver = driver_with_http(ClientConfigHttp.new(client_config)) + + assert_equal('https://hub-cloud.browserstack.com/wd/hub', Percy::DriverMetadata.new(driver).command_executor_url) + end + + def test_server_url_is_empty_when_no_known_location_has_it + driver = driver_with_http(ClientConfigHttp.new(nil)) + + assert_equal('', Percy::DriverMetadata.server_url(driver)) + end + def test_capabilities session_id = 'session_id_123' 2.times do