-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Android Appium test tweaks #244
Conversation
Oh! I totally forgot to ping @alsorokin for a review :) If you find the time, please review good sir! |
// it is not necessary to do a full CI setup to run these tests | ||
// just run "node cordova-medic/medic/medic.js appium --platform android --plugins cordova-plugin-camera" | ||
// for android 5.1, run: | ||
// node cordova-paramedic/main.js --platform android --plugin cordova-plugin-camera --skipMainTests --saucePlatformVersion "5.1" --target api22 |
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.
I think --saucePlatformVersion
is meant to be used together with --shouldUseSauce
For a local test run, I think it makes sense to use --target
arg, which specifies the emulator name to run on.
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.
So, while that may be how paramedic right now was designed, that's actually counter-productive when working with appium. Appium will actually select the correct emulator (and even start it for you) based on the platformName
, platformVersion
and deviceName
capabilities you provide. These capabilities apply regardless if you are running the test locally or on Sauce Labs.
I think, moving forward, for a paramedic refactor I am gathering use cases for / noting issues to address, we would want to expose the appium/selenium capabilities more completely. This would also reduce the difference between local test executions and executions on Sauce Labs, by allowing Appium to worry about managing the emulator, rather than managing it in paramedic.
I will change the instructions back, but just a heads up that I think we should change this behaviour in the future.
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.
I still think that the user should know better which emulator he wants to use in the test run. For example, one of the emulator may not have camera enabled. I doubt that Appium would know that when selecting "best" emulator.
Also please don't forget that --target
arg specifies not only emulator for Appium tests, but also for our regular "mobilespec" ones. It just seems natural to me that Appium should use already opened and ready-for-action emulator instead of choosing another one.
I'm not against any changes in paramedic, just letting you know how it works right now. 😉
So my point is, we still need to manage the emulator in paramedic, at least in case of local "main" tests run.
@@ -373,7 +384,6 @@ describe('Camera tests Android.', function () { | |||
return driver | |||
.waitForElementByXPath('//android.widget.TextView[@text="Gallery"]', 20000) | |||
.elementByXPath('//android.widget.TextView[@text="Gallery"]') | |||
.elementByXPath('//android.widget.TextView[@text="Gallery"]') |
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.
Please do not delete duplicate element selections.
They are needed to work around the following bug: on Android emulator under Windows, element selection doesn't work the first time on some elements. And sometimes even a second or third time - wrong elements are being clicked.
The only way to work around this issue I've found is to select the element multiple times like this.
Maybe selecting element by accessibility id could resolve this issue. I'll make a note to myself to look into it a bit later, but for now please leave it as is.
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.
Do you have any details around the emulator bug on Windows? Ideally:
- any Appium issues filed that sound similar?
- any Android issues filed relevant?
- Windows version?
- Appium version?
- Android emulator version?
I'm also up for looking into it more closely.
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.
I've tried to find any similar issues but with no luck.
Windows: Windows 10 Enterprise, Version 1607, Build 14393.577
Appium: 1.5.0 (maybe even earlier) to 1.6.3
Android SDK Tools (i.e. emulator version) 25.2.2, pretty sure the latest one will also expose the issue
@alsorokin thanks for the feedback, I've addressed your points. Let me know if you have any other concerns or if you are good with merging it. |
LGTM 👍 |
However, it would be nice if first two checklist items were fulfilled. |
Filed CB-12312 for this one. |
- updated comments on how to run the tests. extra comments around functionality at certain points in the automation. - stub of a resolution checker on test startup - still need to figure out acceptable values. - moved session shutdown to an afterAll clause. - changed resolution determiner from using webview-based values to using the native windows dimensions - this helps as the webview values may be scaled down intentionally by manufacturers (via changing devicePixelRatio). furthermore, since the screen dimension automation is used purely for native UI automation, better to use the dimensions reported by the native context rather than the web context. - when finding elements by XPath, use multiple calls to avoid a Windows emulator + Android bug. Made this pattern consistent in the entire test.
4e26e2a
to
05594c4
Compare
Rebased and squashed commit and updated message to have the same format as you pointed out @alsorokin. Thanks for the review! I'll merge upon tests passing. |
Platforms affected
Android
What does this PR do?
afterAll
clause.window.devicePixelRatio
, for example). furthermore, since the screen dimensions are used purely for native UI automation using direct coordinates, better to use the dimensions reported by the native context rather than the web context.What testing has been done on this change?
Tested on local appium 1.6.3 environment on Android 4.4 and 5.1 stock emulators.
Checklist