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

feat: Add locator types supported by flutter integration driver #2201

Merged
merged 9 commits into from
Jul 20, 2024

Conversation

sudharsan-selvaraj
Copy link
Contributor

@sudharsan-selvaraj sudharsan-selvaraj commented Jul 16, 2024

Change list

Please provide briefly described change list which are you going to propose.

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Please provide more details about changes if it is necessary. If there are new features you can provide code samples which show the way they
work and possible use cases. Also you can create gists with pasted java code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
lforst Luca Forstner
@@ -53,6 +56,13 @@ protected AppiumBy(String selector, String locatorString, String locatorName) {
return String.format("%s.%s: %s", AppiumBy.class.getSimpleName(), locatorName, remoteParameters.value());
}

public Map<String, Object> toJson() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure this method is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently flutter driver supports custom commands like scrollTillVisble, waitForVisible, waitForVisible etc, which accepts raw locator info(By) as arguments. So users can directly pass the locator info by calling toJson() method instead of constructing the JsonMap manually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can observe that Remotable.Parameters class already contains toJson method. It's private though. Lets add it to the list in SeleniumHQ/selenium#13949 and leave a TODO there.

Also consider using Map.of() factory instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also provide an example of any of the above calls where toJson method may be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One such example is using flutter: scrollTillVisible method.

  1. Without toJson()
driver.executeScript("flutter: scrollTillVisible", Map.of("finder", Map.of("using", "-flutter key", "value", "login_button")));
  1. With toJson()
driver.executeScript("flutter: scrollTillVisible", Map.of("finder", AppiumBy.flutterKey("login_button").toJson());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably make sense to create FlutterIosDriver and FlutterAndroidDriver inherited from iOSDriver and AndroidDriver, so the above extensions may be implemented as first-class methods, e.g.

flutter. scrollTillVisible(AppiumBy.flutterKey("login_button"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We initially had a similar idea. After reviewing the implementation, we found that the logic is consistent for both iOS and Android drivers, so we decided to maintain this approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sudharsan-selvaraj Agree we wanted to make it consistent but nothing is stopping us implementing the initial approach if everyone is fine. It gets easier to implement the tests.
cc: @mykola-mokhnach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saikrishna321 @mykola-mokhnach That sounds good! Let's proceed by creating separate Flutter-specific driver classes to abstract the underlying commands. I'll prepare a patch with the changes and tag for the review.

@sudharsan-selvaraj sudharsan-selvaraj marked this pull request as ready for review July 20, 2024 16:06
@sudharsan-selvaraj
Copy link
Contributor Author

@mykola-mokhnach @SrinivasanTarget I have updated the PR to just include the changes related to locators. I'll create separate PR for driver implementation along with the e2e tests.

@mykola-mokhnach mykola-mokhnach merged commit bb4ee2d into appium:master Jul 20, 2024
4 of 5 checks passed
@KazuCocoa KazuCocoa added the size:S contribution size: S label Sep 6, 2024
@jlipps
Copy link
Member

jlipps commented Sep 6, 2024

Hi @sudharsan-selvaraj, congrats, the Appium project wants to compensate you for this (and perhaps other) contribution(s) this month! Please reply to this comment mentioning me and sharing your OpenCollective account name, so that we can initiate payment! Or let me know if you decline to receive compensation via OpenCollective. Thank you!

@sudharsan-selvaraj
Copy link
Contributor Author

Hey @jlipps , I would like to express my gratitude to the Appium team for the recognition and I'm pleased to be a part of the project. For the fund transfers, please use the OpenCollective account associated with https://opencollective.com/appium-device-farm. If you need any additional information, please let me know.

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

Successfully merging this pull request may close these issues.

None yet

6 participants