Find devices for 'android.attached' based on 'adbName' pattern #2169

DmytroKost · 2020-06-30T12:03:28Z

For running parallel tests on attached devices

This is a small change

Description:

I want to be able to run parallel tests on multiple attached Android devices (e.g. Genymotion). Currently, the android.attached configuration type accepts only one adbName.

noomorph · 2020-06-30T12:50:28Z

@d4vidi, let's look together into this.

noomorph · 2020-06-30T12:56:34Z

Preliminary speaking, looks good to me, except maybe for 1-2 very minor things. I'll give it a try locally.

d4vidi · 2020-06-30T14:38:26Z

nice one - hope to have a look soon!

DmytroKost · 2020-07-02T14:48:10Z

I noticed that when enabling artifacts (logs, videos), Detox fails to save them when running in parallel mode.

tests_1                | detox[188] DEBUG: [exec.js/SPAWN_CMD, #14] [pid=260] /opt/sdk/platform-tools/adb shell "logcat --pid=1930 -f /sdcard/62339204_0.log"
tests_1                | detox[188] DEBUG: [DetoxServer.js/CANNOT_FORWARD] role=testee not connected, cannot fw action (sessionId=c2967c73-58e6-887c-834b-4cba63136e9b)
tests_1                | detox[188] ERROR: [exec.js/SPAWN_STDERR, #14] error: more than one device/emulator


tests_1                | detox[174] ERROR: [exec.js/EXEC_FAIL, #22] ""/opt/sdk/platform-tools/adb"  shell du /sdcard/62743283_0.mp4" failed with code = 1, stdout and stderr:
tests_1                | 
tests_1                | detox[174] ERROR: [exec.js/EXEC_FAIL, #22] 
tests_1                | detox[174] ERROR: [exec.js/EXEC_FAIL, #22] error: more than one device/emulator

From the output I can see that it doesn't pass -s device_name.
That doesn't happen when running tests on local emulators. Did I miss something? I haven't yet spent time investigating the cause of the issue.

d4vidi · 2020-07-05T09:17:40Z

@DmytroKost I've revisited this now and I feel there are 2 things to consider.

Implementation
Should we decide to accept this as a feature (and I personally think it could definitely be the right evolution for attached devices support!), there are two drawbacks in putting support inside a driver -- bloatedness of drivers and unit-test coverage. Therefore, we could only accept this after refactoring the device lookup onto a separate, full-tested (tdd) finder class - much like FreeEmulatorFinder, which is the equivalent one for emulators. See how it's tested, here. Note how it easily utilizes the more generic AdbDevicesHelper for this. Do you think you could try to work out something like this over this PR?
Configuration contract
I'm still not certain whether an adb-names array is the right way to represent this data. For that, I'll need @noomorph's insights...

noomorph · 2020-07-06T07:10:18Z

@d4vidi, I'm about to review this today or maximum — tomorrow.

As for your question 2 — well, I was considering also RegExp-based solution. This way we can elegantly solve many cases:

"emulator-\\d+" // use only spawned emulators 
".*" // use anything that is free
"device_1|device_2|device_3" // use this kinda array of devices

What do you think?

DmytroKost · 2020-07-08T08:04:44Z

@d4vidi Yes, I can do that. Any insight into why artifacts don't work properly with multi-attached devices? #2169 (comment)

noomorph · 2020-07-08T10:20:17Z

Cannot deliver my promise to review it the PR yesterday, obviously, have to repromise that for tomorrow.
Regarding that comment, I cannot really say without a deeper look, @DmytroKost. Sorry for the long wait, thanks for the patience.

DmytroKost · 2020-07-09T14:46:57Z

@d4vidi @noomorph I've made the following changes:

Implemented FreeAdbDeviceFinder and added unit tests for it.
Extracted a parent classFreeDeviceFinderBase from FreeEmulatorFinder and reused it for FreeAdbDeviceFinder. Same for the unit tests.
Implemented lookup strategy based or adbName being a regular expression.
Fixed the problem with artifacts by emitting a bootDevice event with the devideId.

coveralls · 2020-07-09T14:52:16Z

Pull Request Test Coverage Report for Build 2499

77 of 94 (81.91%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 85.641%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
detox/src/devices/drivers/DeviceDriverBase.js	0	1	0.0%
detox/src/devices/drivers/android/AndroidDriver.js	9	10	90.0%
detox/src/devices/drivers/android/tools/DeviceHandle.js	7	10	70.0%
detox/src/devices/drivers/android/attached/AttachedAndroidDriver.js	4	16	25.0%

Totals
Change from base Build 1315:	0.02%
Covered Lines:	3642
Relevant Lines:	4088

💛 - Coveralls

noomorph · 2020-07-13T14:06:36Z

@DmytroKost , if you don't mind, could you remerge master again into your branch, please?

DmytroKost · 2020-07-13T14:16:39Z

@DmytroKost , if you don't mind, could you remerge master again into your branch, please?

Done.

noomorph · 2020-07-13T15:40:45Z

@DmytroKost, I think it makes sense to add:

A mention about the RegExp matching in the docs:

a. Introduction.Android.md
```
 Connect to already-attached android device. The device should be listed in the output of adb devices command under provided name. Use this type to connect to Genymotion emulator.
```
b. APIRef.Configuration.md — see example with android.att.release, maybe you could add right there an example of how you can specify multiple devices.
An example in the configs:
https://github.com/wix/Detox/blob/cbf6966b640948db7c92035229294a6621e5319c/examples/demo-react-native/detox.config.js

At the very least, let's add some emulator-.* configuration with android.attached type, that can be used for testing that your PR works. I wonder, maybe it will make sense to start running a smoke test with that attached configuration in CI, @d4vidi , what do you think?

DmytroKost · 2020-07-14T09:38:27Z

@noomorph Updated the docs and the detox.config.js example as requested.

noomorph

Looks fairly good to me. I think I'll have today time to go through all the pull request. @d4vidi, don't forget to look too, if possible - and if not, I hope you don't mind if I approve it soon.

noomorph · 2020-07-14T19:47:06Z

@DmytroKost , see my suggestions in DmytroKost#1

d4vidi · 2020-07-15T08:02:38Z

Sorry, 'been overly booked in the last couple of days. I'll try my best to review asap.

DmytroKost · 2020-07-15T08:23:26Z

@noomorph Thank you for the feedback. I'll wait for the review from @d4vidi before making changes so that I don't have to go back and forth.

d4vidi · 2020-07-19T13:44:33Z

'Promise to review this this week, probably within a day or 2!

d4vidi · 2020-07-21T14:29:31Z

examples/demo-react-native/detox.config.js

@@ -51,6 +51,14 @@ module.exports = {
      "device": {
        "avdName": "Pixel_API_28"
      }
+    },
+    "android.att.release": {


Perhaps change to android.genymotion.release? a bit confusing to see both attached and the emulator-.* regexp in the same config

I asked @DmytroKost first of all to add it for the testing purposes. I am sure that Genymotion emulators do not follow emulator-<port> naming (!). But we can add some Genymotionish configuration as well, separately.

@d4vidi, probably, you are right in a sense that it could be better if we rename it to, let's say, android.any.release (att -> any), and use .* instead of emulator-.*. 🤔

We connect to Genymotion via SSH tunnel, the names of devices will be localhost:5555, localhost:5556, .etc.

I can make an example for android.genymotion.release to demonstrate that, and one for android.any.release with a .* mark to demonstrate connection to any attached device/emulator. I thought detox.config.js for also for demo purposes. But since it's for testing, I'm not sure I should make any changes to it.

I also mentioned the example with a remote device in https://github.com/DmytroKost/Detox/blob/uic/adb-name-array/docs/APIRef.Configuration.md#device-configuration

@d4vidi, your call.

@d4vidi , @DmytroKost , let's change it to ".*" and rename to any instead of att. It is just for local testing at the moment.

Any progress here? @d4vidi, is the .* suggestion okay?

Why .*? does it really make sense to match all-the-things? How about we negate emulator-? (?!(emulator)), I believe...

Anyways on 2nd thought no point in adding an extra genymotion config here - I agree.

Or iso negating, assert that it's a remotely connected device with this pattern

@d4vidi @noomorph Reverted changes to the detox.config.js file. Moved the example with android.genymotion.release to docs/APIRef.Configuration.md. What do you think?

detox/src/devices/drivers/android/emulator/FreeEmulatorFinder.test.js

detox/src/devices/drivers/android/emulator/EmulatorDriver.js

noomorph · 2020-07-27T10:35:38Z

@DmytroKost , I think @d4vidi suggests to copy FreeDeviceFinderBase.test.js twice and delete. The copies will be called FreeDeviceFinder.test.js and FreeEmulatorFinder.test.js. Both copies will run very similar test suites except for a few emulator-specific and device-specific cases, e.g.: FreeDeviceFinder is indifferent about whether a device is an emulator or not — it just should match the specified regular expression, while FreeEmulatorFinder cares exclusively about emulators and a correct AVD name; however, both test suites will make sure that those finders omit offline and busy devices — that is the common part for both implementations.

I hope I've managed to convey the message.

detox/src/devices/drivers/android/attached/AttachedAndroidDriver.js

d4vidi · 2020-07-27T11:29:30Z

@DmytroKost please rebase your branch over Detox' latest master. I believe this should solve CI issues.

DmytroKost · 2020-07-27T12:38:20Z

@d4vidi I merged FreeAdbDeviceFinder to FreeDeviceFinder with the following changes:

Added an underscore to _isDeviceMatching
Renamed FreeDeviceFinderBase to FreeDeviceFinder
Removed FreeAdbDeviceFinder
FreeDeviceFinder._isDeviceMatching tests candidate's adbName against a regex by default (copied the implementation from FreeAdbDeviceFinder._isDeviceMatching)
FreeEmulatorFinder extends FreeDeviceFinder and overrides _isDeviceMatching
Moved the unit test from FreeAdbDeviceFinder.test.js to FreeDeviceFinder.test.js and removed FreeAdbDeviceFinder.test.js
FreeEmulatorFinder.test.js tests only the _isDeviceMatching override through the public method findFreeDevice

For running parallel tests on attached devices

Ignore emulators and devices that aren't on the list

Artifact manager needs deviceId

… on 'adbName' pattern" - src/devices/drivers - DeviceDriverBase.js - android - AndroidDriver.js - attached - AttachedAndroidDriver.js - FreeAdbDeviceFinder.js - emulator - EmulatorDriver.js - FreeEmulatorFinder.js DriverRegistry resolves: - 'android.emulator': EmulatorDriver - EmulatorDriver.acquireFreeDevice - AndroidDriver.allocateDevice - ABSTRACT AndroidDriver.doAllocateDevice -> IMPL EmulatorDriver.doAllocateDevice - FreeEmulatorFinder.findFreeDevice -> SUPER FreeDeviceFinderBase.findFreeDevice - ABSTRACT FreeDeviceFinderBase.isDeviceMatching -> IMPL FreeEmulatorFinder.isDeviceMatching - 'android.attached': AttachedAndroidDriver - AttachedAndroidDriver.acquireFreeDevice - AndroidDriver.allocateDevice - ABSTRACT AndroidDriver.doAllocateDevice -> IMPL AttachedAndroidDriver.doAllocateDevice - FreeAdbDeviceFinder.findFreeDevice -> SUPER FreeDeviceFinderBase.findFreeDevice - ABSTRACT FreeDeviceFinderBase.isDeviceMatching -> IMPL FreeAdbDeviceFinder.isDeviceMatching

- Renamed FreeDeviceFinderBase to FreeDeviceFinder - Removed FreeAdbDeviceFinder - FreeDeviceFinder._isDeviceMatching tests candidate's adbName against a regex by default (copied the implementation from FreeAdbDeviceFinder._isDeviceMatching) - FreeEmulatorFinder extends FreeDeviceFinder and overrides _isDeviceMatching - Moved the unit test from FreeAdbDeviceFinder.test.js to FreeDeviceFinder.test.js and removed FreeAdbDeviceFinder.test.js - FreeEmulatorFinder.test.js tests only the _isDeviceMatching override through the public method findFreeDevice

DmytroKost · 2020-07-27T12:42:58Z

@d4vidi Rebased on master

detox/src/devices/drivers/android/emulator/EmulatorDriver.js

Hopefully fix CI stability by "creating" emulator devices from within device-registry's user allocation func

d4vidi · 2020-07-27T14:39:40Z

Green at last!!! I will continue going over this tomorrow, and - hopefully, merge, as well.

noomorph · 2020-07-27T15:01:11Z

DmytroKost requested a review from rotemmiz as a code owner June 30, 2020 12:03

DmytroKost mentioned this pull request Jun 30, 2020

Allow "android.attached" to accept an array of "adbName" for parallelisation #2166

Closed

DmytroKost changed the title ~~Allow 'android.attached' to accept an array of 'adbName'~~ Find devices for 'android.attached' whose 'adbName' matches a regular expression Jul 9, 2020

DmytroKost force-pushed the uic/adb-name-array branch from cbaee76 to 5f5d220 Compare July 9, 2020 12:05

DmytroKost changed the title ~~Find devices for 'android.attached' whose 'adbName' matches a regular expression~~ Find devices for 'android.attached' based on 'adbName' pattern Jul 9, 2020

DmytroKost force-pushed the uic/adb-name-array branch from bdd2ff5 to aeb47da Compare July 13, 2020 14:15

DmytroKost force-pushed the uic/adb-name-array branch from 6cfca86 to 41ef564 Compare July 14, 2020 09:37

noomorph reviewed Jul 14, 2020

View reviewed changes

noomorph added a commit that referenced this pull request Jul 14, 2020

code: suggestions for #2169

08fbd6f

d4vidi self-assigned this Jul 15, 2020

d4vidi reviewed Jul 21, 2020

View reviewed changes

detox/src/devices/drivers/android/emulator/FreeEmulatorFinder.test.js Show resolved Hide resolved

d4vidi reviewed Jul 27, 2020

View reviewed changes

detox/src/devices/drivers/android/emulator/EmulatorDriver.js Outdated Show resolved Hide resolved

d4vidi reviewed Jul 27, 2020

View reviewed changes

detox/src/devices/drivers/android/attached/AttachedAndroidDriver.js Outdated Show resolved Hide resolved

DmytroKost and others added 15 commits July 27, 2020 14:41

Allow 'android.attached' to accept an array of 'adbName'

bb95e98

For running parallel tests on attached devices

Allocate attached devices based on the provided list of adb names

abf9e77

Ignore emulators and devices that aren't on the list

Implement FreeAdbDeviceFinder

5813490

Emit 'bootDevice' after an attached device's been found

d10a10e

Artifact manager needs deviceId

Mention regex-based 'adbName' property in docs and examples

f6c9365

Review: duck test parrent matching iso mocking RegExp

90772e8

Review: _createDevice should return a string iso a Promise

26c5aee

Review: move DeviceHandle and EmulatorHandle out of ADB

7e44a04

Review: skip offline devices while searching for a free device

637b243

Review: mark method with underscore to signify it's private

be42d10

Add missing await in emulator driver

b088b69

Add missing await in attached-driver

c9e0690

Review: mark method with underscore to signify it's private

1c25acf

DmytroKost force-pushed the uic/adb-name-array branch from a452c6e to ecc785d Compare July 27, 2020 12:42

Move example from detox.config.js to docs

7476656

d4vidi reviewed Jul 27, 2020

View reviewed changes

detox/src/devices/drivers/android/emulator/EmulatorDriver.js Outdated Show resolved Hide resolved

d4vidi reviewed Jul 27, 2020

View reviewed changes

detox/src/devices/drivers/android/emulator/EmulatorDriver.js Outdated Show resolved Hide resolved

Apply suggestions from code review

1c9b3e9

Hopefully fix CI stability by "creating" emulator devices from within device-registry's user allocation func

d4vidi merged commit a4852f8 into wix:master Jul 28, 2020

d4vidi mentioned this pull request Jul 28, 2020

Apply some post-processing over the multi ADB-name PR #2235

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Find devices for 'android.attached' based on 'adbName' pattern #2169

Find devices for 'android.attached' based on 'adbName' pattern #2169

DmytroKost commented Jun 30, 2020 •

edited

noomorph commented Jun 30, 2020

noomorph commented Jun 30, 2020

d4vidi commented Jun 30, 2020

DmytroKost commented Jul 2, 2020

d4vidi commented Jul 5, 2020 •

edited

noomorph commented Jul 6, 2020

DmytroKost commented Jul 8, 2020

noomorph commented Jul 8, 2020 •

edited

DmytroKost commented Jul 9, 2020

coveralls commented Jul 9, 2020 •

edited

noomorph commented Jul 13, 2020

DmytroKost commented Jul 13, 2020

noomorph commented Jul 13, 2020

DmytroKost commented Jul 14, 2020

noomorph left a comment

noomorph commented Jul 14, 2020

d4vidi commented Jul 15, 2020

DmytroKost commented Jul 15, 2020

d4vidi commented Jul 19, 2020

d4vidi Jul 21, 2020

noomorph Jul 22, 2020 •

edited

DmytroKost Jul 22, 2020

noomorph Jul 22, 2020

noomorph Jul 23, 2020

noomorph Jul 26, 2020

d4vidi Jul 27, 2020

DmytroKost Jul 27, 2020

DmytroKost Jul 27, 2020

noomorph commented Jul 27, 2020

d4vidi commented Jul 27, 2020

DmytroKost commented Jul 27, 2020

DmytroKost commented Jul 27, 2020 •

edited

d4vidi commented Jul 27, 2020 •

edited

noomorph commented Jul 27, 2020

Find devices for 'android.attached' based on 'adbName' pattern #2169

Find devices for 'android.attached' based on 'adbName' pattern #2169

Conversation

DmytroKost commented Jun 30, 2020 • edited

noomorph commented Jun 30, 2020

noomorph commented Jun 30, 2020

d4vidi commented Jun 30, 2020

DmytroKost commented Jul 2, 2020

d4vidi commented Jul 5, 2020 • edited

noomorph commented Jul 6, 2020

DmytroKost commented Jul 8, 2020

noomorph commented Jul 8, 2020 • edited

DmytroKost commented Jul 9, 2020

coveralls commented Jul 9, 2020 • edited

Pull Request Test Coverage Report for Build 2499

💛 - Coveralls

noomorph commented Jul 13, 2020

DmytroKost commented Jul 13, 2020

noomorph commented Jul 13, 2020

DmytroKost commented Jul 14, 2020

noomorph left a comment

Choose a reason for hiding this comment

noomorph commented Jul 14, 2020

d4vidi commented Jul 15, 2020

DmytroKost commented Jul 15, 2020

d4vidi commented Jul 19, 2020

Choose a reason for hiding this comment

noomorph Jul 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noomorph commented Jul 27, 2020

d4vidi commented Jul 27, 2020

DmytroKost commented Jul 27, 2020

DmytroKost commented Jul 27, 2020 • edited

d4vidi commented Jul 27, 2020 • edited

noomorph commented Jul 27, 2020

DmytroKost commented Jun 30, 2020 •

edited

d4vidi commented Jul 5, 2020 •

edited

noomorph commented Jul 8, 2020 •

edited

coveralls commented Jul 9, 2020 •

edited

noomorph Jul 22, 2020 •

edited

DmytroKost commented Jul 27, 2020 •

edited

d4vidi commented Jul 27, 2020 •

edited