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.

Sign up for GitHub

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

Jump to bottom

Return empty string if null #212

Closed
wants to merge 5 commits into from
Closed

Return empty string if null #212

wants to merge 5 commits into from

Conversation

imurchie
Copy link
Contributor

In certain circumstances (such as Java 1.7, Android-21 in our appium-uiautomator2-driver Travis build (see https://travis-ci.org/appium/appium-uiautomator2-driver/jobs/447774226#L3630-L3637)) the framework returns null, and we pass that back, leading to failures where "" is expected.

info HTTP --> GET /wd/hub/session/a31a2b91-2d5b-45e5-8b82-fbe8a5e13061/element/e0205769-e494-4f91-bcc8-76c1b2cdbb8b/text
info HTTP {}
info MJSONWP (a31a2b91) Driver proxy active, passing request on via HTTP proxy
dbug JSONWP Proxy Matched '/wd/hub/session/a31a2b91-2d5b-45e5-8b82-fbe8a5e13061/element/e0205769-e494-4f91-bcc8-76c1b2cdbb8b/text' to command name 'getText'
dbug JSONWP Proxy Proxying [GET /wd/hub/session/a31a2b91-2d5b-45e5-8b82-fbe8a5e13061/element/e0205769-e494-4f91-bcc8-76c1b2cdbb8b/text] to [GET http://localhost:8200/wd/hub/session/a6cdb841-e905-49ad-ae43-51a9ac773de4/element/e0205769-e494-4f91-bcc8-76c1b2cdbb8b/text] with body: {}
dbug JSONWP Proxy Got response with status 200: "{\"sessionId\":\"a6cdb841-e905-49ad-ae43-51a9ac773de4\",\"status\":0,\"value\":null}"
info JSONWP Proxy Replacing sessionId a6cdb841-e905-49ad-ae43-51a9ac773de4 with a31a2b91-2d5b-45e5-8b82-fbe8a5e13061
info HTTP <-- GET /wd/hub/session/a31a2b91-2d5b-45e5-8b82-fbe8a5e13061/element/e0205769-e494-4f91-bcc8-76c1b2cdbb8b/text 200 39

@mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented Oct 29, 2018 •
edited
Loading

This was actually a deliberate change in #208 to distinguish cases where text attribute is not present and where it equals to an empty string.

This result should be consistent in all places: xml tree, xpath search, getAttribute output

@imurchie
Copy link
Contributor Author

I guess that's why the tests just started failing.

@mykola-mokhnach
Copy link
Contributor

@imurchie does the standard define some rules on the text value?

@mykola-mokhnach
Copy link
Contributor

appium-uiautomator2-server/app/src/main/java/io/appium/uiautomator2/utils/ElementHelpers.java

Line 225 in cc9b767

public static String getText(@Nullable AccessibilityNodeInfo nodeInfo) {

appium-uiautomator2-server/app/src/main/java/io/appium/uiautomator2/utils/ElementHelpers.java

Line 238 in cc9b767

public static String getText(Object element) throws UiObjectNotFoundException {

These are the places to patch in order to make getText output consistent

mykola-mokhnach
mykola-mokhnach reviewed Oct 29, 2018
View reviewed changes
app/src/main/java/io/appium/uiautomator2/handler/SendKeysToElement.java
@@ -45,7 +45,8 @@ public SendKeysToElement(String mappedUri) {
}

private static boolean isTextFieldClear(AndroidElement element) throws UiObjectNotFoundException {
return element.getText() == null || element.getText().isEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

actually the previous one was also fine ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just two calls to element.getText().

@imurchie
Copy link
Contributor Author

Unfortunately, this breaks finding element by androidUiAutomator. Alas.

@mykola-mokhnach
Copy link
Contributor

@imurchie I don't see any connection between this change and lookup by androidUiAutomator %/

@mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented Oct 29, 2018 •
edited
Loading

We could patch getAttribute calls for UiObject2Element/UiObjectElement plus replace element.getText with getAttribute(TEXT) call in getText handler if that helps to resolve the search issue above.

@imurchie
Copy link
Contributor Author

Me neither. :(

But as far as I can tell right now, the right object is found, but reports as non-existent at https://github.com/appium/appium-uiautomator2-server/blob/master/app/src/main/java/io/appium/uiautomator2/model/internal/CustomUiDevice.java#L124

@mykola-mokhnach
Copy link
Contributor

@imurchie I believe it was something else that caused this test to fail, because I observe the same error in https://travis-ci.org/appium/appium-uiautomator2-server/builds/448241629?utm_source=github_status&utm_medium=notification, however it does not contain this fix. How about merging this PR as is and then we try to address the uia location issue in the other one?

@mykola-mokhnach
Copy link
Contributor

@imurchie I assume this PR should be fine if you rebase it with master

mykola-mokhnach
mykola-mokhnach reviewed Oct 31, 2018
View reviewed changes
app/src/main/java/io/appium/uiautomator2/model/CustomUiSelector.java Outdated
@@ -44,7 +44,9 @@ public UiSelector getUiSelector(AccessibilityNodeInfo node) {
}
put(Attribute.PACKAGE, charSequenceToString(uiAutomationElement.getPackageName()));
put(Attribute.CLASS, charSequenceToString(uiAutomationElement.getClassName()));
put(Attribute.TEXT, charSequenceToString(uiAutomationElement.getText()));
// the element will give empty strings as "" but for searching we need null
String text = charSequenceToString(uiAutomationElement.getText());
Copy link
Contributor

Choose a reason for hiding this comment

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

this is too hacky and not very reliable as for my taste. Can we then patch the stuff in GetAttribute and GetText instead and leave getText inside ElementHelpers in their previous state (return null)?

Copy link
Contributor

Choose a reason for hiding this comment

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

or probably have a separate getText endpoint for uiAutomationElement, which does not replace null and call it here?

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

like getNullableText and a "normal" getText

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

mykola-mokhnach
mykola-mokhnach reviewed Oct 31, 2018
View reviewed changes
app/src/main/java/io/appium/uiautomator2/model/UiAutomationElement.java
@Nullable
public String getNullableText() {
String text = getText();
return (text != null && text.isEmpty()) ? null : text;
Copy link
Contributor

Choose a reason for hiding this comment

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

better, but still not ideal. I would prefer to have a special argument for ElementHelper methods, which defines whether null should be replaced or not, so we don't need to perform "double transformation"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to open a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure will do. Thanks for the hint

@mykola-mokhnach mykola-mokhnach mentioned this pull request Oct 31, 2018
Merged
@mykola-mokhnach
Copy link
Contributor

Superseeded by #218

@imurchie imurchie deleted the isaac-gettext branch May 8, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@imurchie @mykola-mokhnach