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

Implementation of the 'scroll' command #244

Merged

Conversation

Anton659011
Copy link
Contributor

This change fixes implementation of the ScrollTo command, so that now it supports different strategies to search for a GUI element (by accessibility id, by class name, and by using the Android uiautomator selectors). The fix also addresses issue #98. Please let me know if I missed something in the implementation, and/or more files need to be updated.

Command: POST /session/:sessionId/touch/scroll
Here are a few examples of the supported JSON objects in the body of the POST request:

Scroll to an element using its accessibility id:

{
  "params":
  {
    "strategy":"accessibility id",
    "selector":"MyTestApp.OptionsPage.Option1.Label"
  }
}

Scroll to an element using its class name:

{
  "params":
  {
    "strategy":"class name",
    "selector":"android.widget.TextView"
  }
}

Scroll to an element identified by an Android uiautomator selector - 'element's text contains given string':

{
  "params":
  {
    "strategy":"-android uiautomator",
    "selector":"new UiSelector().textContains(\"Radio Group\")"
  }
}

Scroll to an element identified by an Android uiautomator selector - 'element's content-desc is the given string':

{
  "params":
  {
    "strategy":"-android uiautomator",
    "selector":"new UiSelector().description(\"MyTestApp.OptionsPage.Option2.Label\")"
  }
}

@jsf-clabot
Copy link

jsf-clabot commented Jan 23, 2019

CLA assistant check
All committers have signed the CLA.

@KazuCocoa
Copy link
Member

KazuCocoa commented Jan 24, 2019

Can you add some test cases if you have a time?
It might help to what format is acceptable for the command.
https://github.com/appium/appium-uiautomator2-server/blob/ce4b766cf702e9d0b8b2304b06a8fd00aaff8fd6/app/src/androidTestE2eTest/java/io/appium/uiautomator2/unittest/test/DeviceCommandsTest.java

@mykola-mokhnach
Copy link
Contributor

Nicely done. Having a test would be perfect.

…ccessfully that might fail automated build jobs. Removed @ignore attribute added by mistake to test shouldShutdownServerOnPowerDisconnect.
@mykola-mokhnach
Copy link
Contributor

It looks like unittest.test.DeviceCommandsTest.shouldBeAbleToFindElementViaUiScrollableScrollTextIntoView is not stable on CI

… 23 and earlier. Simplified test cases scrollVeryLongListSuccessfully and scrollVeryLongListUnsuccessfully. Fixed some code-formatting issues.
@mykola-mokhnach
Copy link
Contributor

The tests are still failing on Travis. Please note that Travis performance is not very good, since it can only run arm-based AVD, which requires to put longer timeouts or more retries for e2e tests in comparison to a local run.

I like the recent source fixes.

@Anton659011
Copy link
Contributor Author

Hmmm, the CI tests that fail were not touched by this change at all.
Probably the new tests that I introduced, which happen to run right before the failing ones, put the target device in some bad state. I am looking into this.

@Anton659011
Copy link
Contributor Author

Anton659011 commented Feb 4, 2019

In the latest push I replaced calls to navigateTo() - a new method introduced in this pull request - with startActivity() - a method used by other tests that were not touched by this change.

UPDATE: The same tests still fail. So, the culprit is not the navigateTo() method. When run locally on fast and slow (ARM) emulators, all tests run with no problems and pass consistently 100%. My latest guess is that there is something with the API Demos app that is used in CI tests on Travis. Is there a way to check if I am using the right target app in my local setup?

@mykola-mokhnach
Copy link
Contributor

There is only one failing test. If it passes locally then I assume we might mark it as ignored for now and add a comment that it's unstable on CI.

@Anton659011
Copy link
Contributor Author

There are two tests that fail, and the reason for the failures seem to be the small number of maximum search swipes set in one of the tests that runs before the failing ones. The number of search swipes is held in a static property of the UiScrollable class. That is, whenever a non-default number of search swipes is used during the scroll, we must always restore the setting after the operation. Otherwise all subsequent scrolls will use the new value, which might not be good. I believe, this is exactly what happens in the failing CI tests. The latest update addresses this issue.

…calls to a more efficient startActivity(). Removed the 'import' statements that became unnecessary because of this change. Updated logic in the scrollByClassName() test to properly handle premature failures.
…ntees proper restoration of the maxSearchSwipes value.
@mykola-mokhnach mykola-mokhnach merged commit 809f19f into appium:master Feb 5, 2019
rajdeepv pushed a commit to rajdeepv/appium-uiautomator2-server that referenced this pull request Feb 15, 2019
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

5 participants