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

Implemented AttachedAndroidDriver device for connecting to Genymotion #397

Merged
merged 5 commits into from
Nov 19, 2017

Conversation

vasyas
Copy link
Contributor

@vasyas vasyas commented Nov 9, 2017

As discussed in #386 and in #96 (comment) , I've implemented AttachedAndroidDriver (really a cut version of EmulatorDriver) that is able to connect to Genymotion and other adb-attached devices.

Also updated doc. Not sure if a test required.

constructor(client) {
super(client);

this.aapt = new AAPT();
Copy link
Member

Choose a reason for hiding this comment

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

I just found out AAPT was in EmulatorDriver and not in AndroidDriver, pushed a change, please remove it as well.

}

async acquireFreeDevice(name) {
const adbDevices = await this.adb.devices();
Copy link
Member

Choose a reason for hiding this comment

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

This is an almost exact copy of the logic in EmulatorDriver (and we hate duplicate code of course 😬 ) . Why not merge in into a utility function inside AndroidDriver ?

@@ -58,7 +58,19 @@ All Core APIs are 100% implemented.
## Emulator control
1. **Emulators** are fully supported, to choose an emulator to run your tests on check `emulator -list-avds`. If none exist, create one.
2. **Devices** - Coming soon!
3. **Genymotion** - Coming a bit later...
Copy link
Member

@rotemmiz rotemmiz Nov 12, 2017

Choose a reason for hiding this comment

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

This does not relate to Genymotion specifically, it can be used with any connected android device, I think it should be emphesized in the docs.

@rotemmiz
Copy link
Member

Thanks for this great PR!
please take a look at the notes, there are minor changes I think should be done before merging.

@simonracz simonracz removed their request for review November 13, 2017 09:14
@vasyas
Copy link
Contributor Author

vasyas commented Nov 13, 2017

Updated, please review.

@rotemmiz rotemmiz merged commit 06c9591 into wix:master Nov 19, 2017
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants