appium / ruby_lib_core

Core library for the Ruby client
Apache License 2.0
31 stars 24 forks source link

support a custom listener #455

Closed KazuCocoa closed 1 year ago

KazuCocoa commented 1 year ago

This is a

Summary

Selenium EventFiringBridge could return selenium element, while current appium element has different structure https://github.com/appium/ruby_lib_core/blob/master/lib/appium_lib_core/element.rb

It causes no reference error in https://github.com/appium/ruby_lib_core/blob/4a3f289c4b970c433079f6156bb6adbb47b51614/lib/appium_lib_core/common/base/bridge.rb#L216-L230 It should be elemenet.ref[1] base.

I also need to check the compatibility of element.ref and .id call in this module

Environment

Actual behaviour and steps to reproduce

Expected behaviour

Link to Appium/Ruby logs

Create a GIST which is a paste of your full Appium logs, and link them here.

Any additional comments

KazuCocoa commented 1 year ago

https://github.com/SeleniumHQ/selenium/commit/b618499adcc3a9f667590652c5757c0caa703289

TODO: add test to handle selenium element as appium bridge's element

tayrawr commented 1 year ago

@KazuCocoa, I was doing some initial testing around the suggested fix(element.ref[1]) and encountered issues when invoking Appium element attribute methods (i.e. .value). It appears that this would normally pass through method_missing, then subsequently transformed into a method call to attribute. Whereas, this causes errors for Selenium elements because the method does not exist from what I can tell.

https://github.com/appium/ruby_lib_core/blob/4a3f289c4b970c433079f6156bb6adbb47b51614/lib/appium_lib_core/element.rb#L53-L58

I don't possess much exposure/experience to the code here, but would it be possible to instead typecast the Selenium element that's returned in the SearchContext back to an Appium element? I'm not sure how viable or clean of a solution this might be or what further issues this might entail, though.

https://github.com/appium/ruby_lib_core/blob/e13b0efae6b964cc6b0e7b0ed10816eaf6e007d4/lib/appium_lib_core/common/base/search_context.rb#L131-L139

KazuCocoa commented 1 year ago

find_element_by is https://github.com/appium/ruby_lib_core/blob/4a3f289c4b970c433079f6156bb6adbb47b51614/lib/appium_lib_core/common/base/bridge.rb#L256 , btw.

Thanks for the info. yea, the missing method should be considered.

tayrawr commented 1 year ago

My apologies for the lack of clarification, but my understanding was that when a listener is passed into the driver initialization, the bridge will be created as an EventFiringBridge instead.

https://github.com/SeleniumHQ/selenium/blob/trunk/rb/lib/selenium/webdriver/common/driver.rb#L71-L77

So when find_element_by is called, that bridge is what's returning the Selenium element to the SearchContext.

KazuCocoa commented 1 year ago

Actually, in the case, maybe the result could be the selenium's one as: https://github.com/SeleniumHQ/selenium/blob/trunk/rb/lib/selenium/webdriver/support/event_firing_bridge.rb#L79

I'll explore more tonight, to this weekend.

https://github.com/SeleniumHQ/selenium/blob/f28144eb72ae1df18f267a5250db6b9b41dc1fdc/rb/spec/unit/selenium/webdriver/support/event_firing_spec.rb#L25

Maybe having a class like AppiumEventFiringBridge and use it as this lib would help to use appium's element, although I need to write test, and implementation to check the behavior

KazuCocoa commented 1 year ago

What about https://github.com/appium/ruby_lib_core/pull/456?

I haven't tested it yet, but I wonder the PR approach is sufficient at this time

tayrawr commented 1 year ago

Thanks for the quick turnaround and help! I did leave a comment on the PR about a possible concern I had.

KazuCocoa commented 1 year ago

https://github.com/appium/ruby_lib_core/pull/456 was sufficient for this purpose