[rb] Initial extensibility points for Appium #12141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you, the code seems good enough for me (at a quick glance). Let me write a code in the ruby_lib_core with this branch |
titusfortner
reviewed
Jun 5, 2023
| attr_accessor :extra_headers | ||
| attr_writer :user_agent | ||
|
|
||
| def user_agent |
There was a problem hiding this comment.
Hmm, do we need all this? Watir is just subclassing the Default client and making a tweak here. https://github.com/watir/watir/blob/main/lib/watir/http_client.rb
What is Appium doing now?
There was a problem hiding this comment.
@KazuCocoa Would implementing a custom HTTP client in Appium be better than this?
There was a problem hiding this comment.
Oh, the watir way seems good. Appium overrides DEFAULT_HEADERS, b ut the watir way is sufficient for the usage. Then, this pr can drop the user_agent related change actually
There was a problem hiding this comment.
To be clear, I'm not saying the Watir approach is "correct," if it makes more sense to do something else, Watir will do that way. 😄 Presumably we are supposed to avoid using Http::Default in case someone wants to use Http::Curb? I don't really understand why we have that one. 😂
There was a problem hiding this comment.
Yea. Actually, the Watir way is better than the current Appium's one at least :P
Hmm, I like this pr approach, to provide a way to extend headers explicitly, than overriding the parent class and calling super
There was a problem hiding this comment.
Hmm, yes, I agree. With the approach in this PR we can just inject what the user sends regardless. Would be better to minimize subclassing.
@twalpole does capybara do anything to the user agent?
titusfortner
reviewed
Jun 5, 2023
| attr_accessor :extra_commands | ||
| attr_writer :locator_converter | ||
|
|
||
| def add_command(name, verb, url, &block) |
There was a problem hiding this comment.
Would we want Selenium to use this as part of Features modules, so we aren't subclassing #commands?
There was a problem hiding this comment.
Which is to say, we probably should be doing the same thing with custom browser features as Appium does, either way.
There was a problem hiding this comment.
Yeah, I would refactor Selenium after Appium approves the changes.
|
I think we should have a place where we can track this, a project or a single issue? We need to do this for all bindings. |
|
At the very least we can start explicitly listing the pain points in each of the bindings. |
I think we'd need Appium maintainers to help list them out as we did in appium/ruby_lib_core#429. Of course, we can always read the whole Appium bindings codebase, but collaborating will probably be faster. Let's see how this PR goes and consider it a proof-of-concept - we can document other bindings afterwards. |
|
Sorry so late, but at least appium/ruby_lib_core#480 works with this. UA and additional finders/converter and add_commands |
|
@KazuCocoa No worries, do you think there is anything else on the Appium side you could simplify if Selenium provides extension points? If yes, I'll try to hack on it in this PR. If no, I'll bring this PR to the Selenium TLC for a discussion and merge. |
|
I think this PR change, modifying headers, adding a new command, and adding custom locators, is sufficient. Especially adding customer locators (and avoiding converters) is the most helpful for other languages as well. One small thing is Appium clients define its own element class like: This could be only for language-dependent things but it might be helpful to give a custom element class for find_element/s like: and it returns the given element (or default selenium's one if not provided) |
Shall we provide a setter for a default class in the driver? Selenium::WebDriver.element_class = Appium::Core::Element # defaults to Selenium::WebDriver::Element |
|
Sounds good 👍 |
|
@KazuCocoa I've rebased the branch and pushed d6e2b03 which allows you to override the returned element class, you can use it to point to Appium::Core::Element. Let me know if that's enough and I'll move on with preparation for merging this PR. |
|
Thank you, it made the appium client less override and simper. looks good |
|
@titusfortner Would you mind taking another look, if it's good we can discuss this on a TLC meeting and talk about extending to other bindings. |
|
I am moving this to the next milestone as it needs a review from @titusfortner |
|
Per discussion on DevSummit 2024, we are going to proceed with this, merge the Ruby part, and create an issue to track what needs to be done for other bindings. |
CI Failure Feedback 🧐
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the first draft that implements extensibility points that would be useful in Appium per appium/ruby_lib_core#429. With this PR, we can try to see if that would be enough to avoid monkey-patching and hopefully make Selenium break Appium less often. If this is enough, we can repeat the same for other bindings too.
@KazuCocoa Please let me know what do you think about these changes.
UPD: The Appium side is complete (appium/ruby_lib_core#480) and greatly simplified with this PR in mind. I propose we merge this and note down what needs to be done in other bindings. Python is likely going to be similar, but I am not sure about other languages.