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

Refactor source XML tree generation #208

Merged
merged 6 commits into from
Oct 27, 2018

Conversation

mykola-mokhnach
Copy link
Contributor

Previously there were two separate classes for xpath search and for source tree building. I have merged these two implementations, so now the actual xpath source should be executed on the same source, which is visible to getPageSource output

@mykola-mokhnach
Copy link
Contributor Author

@vmaxim Would you like to take a look?

Copy link
Contributor

@dpgraham dpgraham left a comment

Choose a reason for hiding this comment

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

This looks good, as long as it passes the CI tests.

@mykola-mokhnach mykola-mokhnach merged commit b4b655e into appium:master Oct 27, 2018
@mykola-mokhnach mykola-mokhnach deleted the xpath_fix branch October 27, 2018 16:41
rajdeepv pushed a commit to rajdeepv/appium-uiautomator2-server that referenced this pull request Jan 13, 2019
@abasau
Copy link

abasau commented Feb 11, 2019

@mykola-mokhnach I have a couple of questions about this PR. It changed the behavior of XPath searches in scope of an element (relative selectors, e.g. ./*). The following screenshot shows two issues (old and new):
image
According to XPath specification, . should refer to the current element but it used to be that ./* referred to it in Appium for UiAutomator2 (e.g. in 1.9.1). With these changes, ./hierarchy/* refers to the current element. So there are two additional levels before the current element.

Could you please tell which behavior is correct?

I would think that . as a reference to the current element should be correct but, at the same time, a lot of people expect the previous behavior (./*).

@mykola-mokhnach
Copy link
Contributor Author

@aliaksandrbasau Can you please check if #245 works for you?

@abasau
Copy link

abasau commented Feb 11, 2019

@mykola-mokhnach Will try right now. Although it may take time for me to setup the environment for testing. Thank you for so prompt fix and response!

@mykola-mokhnach
Copy link
Contributor Author

I did some more testing and it looks like the behaviour that you've described above (./hierarchy/* refers to the current element) is the expected one for the JDOM2-based parser.
Previously we were using the standard xpath implementation, which is bundled with Android. That one is buggy and has not been updated for ages. It was the main reason to do that switch.

@abasau
Copy link

abasau commented Feb 11, 2019

@mykola-mokhnach Thanks! The new behavior should be fine as long as everyone knows about it. Although it isn't obvious for people coming from web automation. It would be great if it was documented somewhere.

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.

None yet

3 participants