Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add webdrivererror handler instead of calling just name #66

Merged
merged 1 commit into from
Nov 13, 2022

Conversation

KazuCocoa
Copy link
Member

Instead of #62, maybe we could try the rescue clause when the primary method gets Selenium WebDriver exception.

the tag name is https://w3c.github.io/webdriver/#get-element-tag-name in the w3c spec while the "name" is Appium specific. So, when we consider the WebContext as well, it would be safe to keep the primary method in capybara. Instead, the rescue clause can have ::Selenium::WebDriver::Error::WebDriverError as well so that the method can try out the tag_name. (in fact of the tag_name, Ruby client calls /session/{session id}/element/{element id}/name endpoint as the spec.

This case also get passed for the given scenario https://github.com/appium/appium_capybara/pull/62/files

cc @teyamagu

@KazuCocoa KazuCocoa merged commit 4072558 into master Nov 13, 2022
@KazuCocoa KazuCocoa deleted the tag-path-catch branch November 13, 2022 06:38
@teyamagu
Copy link
Contributor

teyamagu commented Nov 13, 2022

Thanks! I think your fix is good. I'll check this

@teyamagu
Copy link
Contributor

teyamagu commented Nov 13, 2022

@KazuCocoa I tried this change in my use-case. iOS is OK, but android is wrong.

% bundle exec rspec spec/android_example_spec.rb
{:caps=>{:platformName=>"Android", :deviceName=>"Samsung Galaxy S20 Ultra", :platformVersion=>"10.0", :automationName=>"UiAutomator2", :autoGrantPermissions=>true, :app=>"/Users/teyamagu/appium_capybara/example/api.apk"}, :appium_lib=>{:server_url=>"http://localhost:4723/wd/hub"}, :global_driver=>false}
F

Failures:

  1) API app smoke test samle test
     Failure/Error: expect(home_page.list.has_missed_item?).to be_falsey
     NoMethodError:
       undefined method `downcase' for nil:NilClass
     # /Users/teyamagu/appium_capybara/lib/appium_capybara/ext/element_ext.rb:4:in `inspect'
     # ./spec/android_example_spec.rb:7:in `block (2 levels) in <top (required)>'

Finished in 8.27 seconds (files took 2.54 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/android_example_spec.rb:4 # API app smoke test samle test

Maybe when we cann't get element existence, we can't get tag_name in android.

example/vendor/bundle/ruby/2.7.0/gems/capybara-3.38.0/lib/capybara/selenium/node.rb

  def tag_name
    @tag_name ||=
      if native.respond_to? :tag_name
        native.tag_name.downcase
      else
        shadow_root? ? 'ShadowRoot' : 'Unknown'
      end
  end

====
repro codes
App: https://github.com/appium/ruby_lib_core/blob/master/test/functional/app/api.apk

spec/android_example_spec.rb

require_relative 'spec_helper'

describe 'API app smoke test' do
  it 'samle test' do
    home_page = Pages::Android.new
    expect(home_page.list.media_item).to be_truthy
    expect(home_page.list.has_missed_item?).to be_falsey #<- error occurred
  end
end

android_page.rb

module Pages
  class Android < ::SitePrism::Page
    section :list, :class, 'android.widget.ListView' do
      elements :media_item, :class, 'android.widget.TextView', text: 'Media' #exist_item
      elements :missed_item, :class, 'android.widget.TextView', text: 'missed' #no_exist item
    end
end

=====
more detail

  • home_page.list.has_media_item?: Expected: true, Actual: true
  • home_page.list.has_no_media_item?: Expected: false, Actual: Error
  • home_page.list.has_no_missed_item?: Expected: true, Actual: true
  • home_page.list.has_missed_item?: Expected: false, Actual: Error
From: /Users/teyamagu/appium_capybara/example/spec/android_example_spec.rb:8 :

     3: describe 'API app smoke test' do
     4:   it 'samle test' do
     5:     home_page = Pages::Android.new
     6:     expect(home_page.list.media_item).to be_truthy
     7:     binding.pry
 =>  8:     expect(home_page.list.has_media_item?).to be_truthy
     9:     expect(home_page.list.has_no_media_item?).to be_falsey
    10:     expect(home_page.list.has_no_missed_item?).to be_truthy
    11:     expect(home_page.list.has_missed_item?).to be_falsey
    12:   end
    13: end

[1] pry(#<RSpec::ExampleGroups::APIAppSmokeTest>)> home_page.list.has_media_item?
=> true
[2] pry(#<RSpec::ExampleGroups::APIAppSmokeTest>)> home_page.list.has_no_media_item?
NoMethodError: undefined method `downcase' for nil:NilClass
from /Users/teyamagu/appium_capybara/example/vendor/bundle/ruby/2.7.0/gems/capybara-3.38.0/lib/capybara/selenium/node.rb:185:in `tag_name'
[3] pry(#<RSpec::ExampleGroups::APIAppSmokeTest>)> home_page.list.has_no_missed_item?
=> true
[4] pry(#<RSpec::ExampleGroups::APIAppSmokeTest>)> home_page.list.has_missed_item?
NoMethodError: undefined method `downcase' for nil:NilClass
from /Users/teyamagu/appium_capybara/example/vendor/bundle/ruby/2.7.0/gems/capybara-3.38.0/lib/capybara/selenium/node.rb:185:in `tag_name'
[5] pry(#<RSpec::ExampleGroups::APIAppSmokeTest>)>

@KazuCocoa
Copy link
Member Author

Let me check Android later.

The endpoint (tag_name) called /name endpoint in yesterday's look. Then, Android UIA2 gives the response of https://github.com/appium/appium-uiautomator2-driver/blob/42eb3d7b59ffebd24e61536a6161a8f8f6f5b249/lib/commands/element.js#L31-L33 (https://github.com/appium/appium-uiautomator2-server/blob/d3cd4a3baa4209d02afcdd2fc0007717890bf138/app/src/main/java/io/appium/uiautomator2/handler/GetName.java#L33). The value might depend on the content description.

Or I missed something in the capybara. Let me check further

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Nov 14, 2022

so... this capybara node overrided the node as https://github.com/appium/appium_capybara/blob/master/lib/appium_capybara/driver/appium/node.rb#L27-L29 . Then, your change %(#<Capybara::Node::Element name=#{@base.name}>) was proper in this context. I'll revert this

@teyamagu
Copy link
Contributor

Thank you for your checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants