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

removed name locator strategy from android and ios drivers along with tests #313

Merged
merged 7 commits into from
Feb 10, 2016
Merged

removed name locator strategy from android and ios drivers along with tests #313

merged 7 commits into from
Feb 10, 2016

Conversation

SrinivasanTarget
Copy link
Member

@TikhomirovSergey please review

/**
* @throws WebDriverException This method doesn't work against native app UI.
*/
public List findElementsByName(String using) throws WebDriverException{
return super.findElementsByName(using);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be reverted

Copy link
Contributor

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

@TikhomirovSergey
Copy link
Contributor

Hi @SrinivasanTarget
First of all let's add descriptions of PR. Ok?

I'm against these changes as they are because:

@SrinivasanTarget
But you can make some improvements and help me to resolve #311

What should be done if you want this PR get merged:

  • revert changes which require reversion. Read my comments.
  • The name option of annotations should be annotated Deprecated. You can do it this way
@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

mark name parameter Decrecated as By.name selector is not supported by native app automation

will be finished.

  • You could finish this point

org.openqa.selenium.InvalidSelectorException should be handled.

The target code is here:

static boolean isInvalidSelectorRootCause(Throwable e) {
. The exception

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

private final static String INVALID_SELECTOR_PATTERN = "Invalid locator strategy:";

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.

@SrinivasanTarget
Copy link
Member Author

Thanks @TikhomirovSergey for your time.Will update the descriptions going forward. And will update the PR based on your review comments.

@bootstraponline
Copy link
Member

@bootstraponline @imurchie is By.name deprecated for Selendroid?

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.

@SrinivasanTarget
Copy link
Member Author

Thanks for clarification. Will update my piece of code.
On 05-Feb-2016 9:07 pm, "bootstraponline" notifications@github.com wrote:

@bootstraponline https://github.com/bootstraponline @imurchie
https://github.com/imurchie is By.name deprecated for Selendroid?

No.
https://github.com/selendroid/selendroid/blob/c2ec0d49f18279e751ce0500f2454e7c494657f0/selendroid-server/src/main/java/io/selendroid/server/model/internal/NativeAndroidBySelector.java#L38
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.


Reply to this email directly or view it on GitHub
#313 (comment).

@TikhomirovSergey
Copy link
Contributor

@SrinivasanTarget
Ok. I've seen your changes. They are looking much better now :). I'm going to review and merge them soom.

@SrinivasanTarget
Copy link
Member Author

I'm still working on this. will update once it's done.
On 06-Feb-2016 5:14 pm, "Sergey Tikhomirov" notifications@github.com
wrote:

@SrinivasanTarget https://github.com/SrinivasanTarget
Ok. I've seen your changes. They are looking much better now :). I'm going
to review and merge them soom.


Reply to this email directly or view it on GitHub
#313 (comment).

@TikhomirovSergey
Copy link
Contributor

ok

@SrinivasanTarget
Copy link
Member Author

@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.

@TikhomirovSergey
Copy link
Contributor

@SrinivasanTarget I'm ok. 👍
@imurchie?

@imurchie
Copy link
Contributor

imurchie commented Feb 9, 2016

Conceptually it looks fine. I can't comment on the Java code.

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.

4 participants