-
-
Notifications
You must be signed in to change notification settings - Fork 765
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
removed name locator strategy from android and ios drivers along with tests #313
removed name locator strategy from android and ios drivers along with tests #313
Conversation
/** | ||
* @throws WebDriverException This method doesn't work against native app UI. | ||
*/ | ||
public List findElementsByName(String using) throws WebDriverException{ | ||
return super.findElementsByName(using); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...By.nane is still supported by browser automation tools
Hi @SrinivasanTarget I'm against these changes as they are because:
@SrinivasanTarget What should be done if you want this PR get merged:
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.FIELD, ElementType.TYPE})
public @interface AndroidFindBy {
String uiAutomator() default "";
String accessibility() default "";
String id() default "";
@Deprecated
/**
* By.name selector is not supported by Appium server node since 1.5.x.
* So this option is going to be removed further. Be careful.
*/
String name() default "";
String className() default "";
String tagName() default "";
String xpath() default "";
} So users will be warned and they will have time to improve their projects before migration to 1.5.x. We will remove it later. But I still don't know which name options should be deprecated. @bootstraponline @imurchie is By.name deprecated for Selendroid? One more question. Why By.name is deprecated for iOS? iOS-elements have the name tag, I think. So the point
will be finished.
The target code is here:
org.openqa.selenium.InvalidSelectorException: Locator Strategy 'css selector' is not supported for this session (WARNING: The server did not provide any stacktrace information) is thrown and it is not being handled here. We should keep it backward compatible with Appium node 1.4x for some time. So you could improve this method the following way: 1 firstly it checks the excepion type. If it is org.openqa.selenium.InvalidSelectorException then method should return true. 2 if 1) has a negative result then it checks exception message. It could use regexps. If message is convenient to the previous pattern
or Locator Strategy 'xxxr' is not supported bla-bla-bla then then method should return true. 3 if both 1) and 2) have negative results then these steps are repeated recursively with the cause of exception. |
Thanks @TikhomirovSergey for your time.Will update the descriptions going forward. And will update the PR based on your review comments. |
No. Even if it was deprecated (it isn't), we'd still want to wait for support to be removed from the server. If the server supports it then I don't see why clients should remove the feature. |
Thanks for clarification. Will update my piece of code.
|
@SrinivasanTarget |
I'm still working on this. will update once it's done.
|
ok |
@TikhomirovSergey I have handled both the scenario's mentioned above.Please review. Also i have deprecated iOSFindBy "name" locator strategy. @imurchie Please correct me if i'm wrong. |
@SrinivasanTarget I'm ok. 👍 |
Conceptually it looks fine. I can't comment on the Java code. |
@TikhomirovSergey please review