Repository: wix/Detox
Pull request #1984 (2020-03-30 13:19:49)

Refactor bloated emulator driver according to sub-concerns
- [x] This is a small change 
- [ ] This change has been discussed in issue #<?> and the solution has been agreed upon with maintainers.

---

**Description:**
Refactor part-1 of `EmulatorDriver`, which has become bloated. In this episode - moving away emulator acquiring logic, which is used by the device registry as its acquiring callback.

Happy to say that the episode ends with everything jolly and merry - all thanks to TDD, strictly used the main character as its main weapon!


Patch:
diff --git a/detox/src/devices/drivers/AdbDevicesHelper.js b/detox/src/devices/drivers/AdbDevicesHelper.js
new file mode 100644
index 000000000..be6cf1548
--- /dev/null
+++ b/detox/src/devices/drivers/AdbDevicesHelper.js
@@ -0,0 +1,17 @@
+class AdbDevicesHelper {
+  constructor(adb) {
+    this.adb = adb;
+  }
+
+  async lookupDevice(matchFn) {
+    const { devices } = await this.adb.devices();
+    for (const candidate of devices) {
+      if (await matchFn(candidate)) {
+        return candidate.adbName;
+      }
+    }
+    return null;
+  }
+}
+
+module.exports = AdbDevicesHelper;
diff --git a/detox/src/devices/drivers/EmulatorDriver.js b/detox/src/devices/drivers/EmulatorDriver.js
index 3a56e5d03..40141252d 100644
--- a/detox/src/devices/drivers/EmulatorDriver.js
+++ b/detox/src/devices/drivers/EmulatorDriver.js
@@ -1,9 +1,9 @@
 const _ = require('lodash');
 const fs = require('fs');
 const path = require('path');
-const os = require('os');
 const ini = require('ini');
 const AndroidDriver = require('./AndroidDriver');
+const EmulatorLookupHelper = require('./EmulatorLookupHelper');
 const DetoxRuntimeError = require('../../errors/DetoxRuntimeError');
 const DeviceRegistry = require('../DeviceRegistry');
 const Emulator = require('../android/Emulator');
@@ -44,32 +44,7 @@ class EmulatorDriver extends AndroidDriver {
     await this._validateAvd(avdName);
     await this._fixEmulatorConfigIniSkinNameIfNeeded(avdName);
 
-    log.debug({ event: ACQUIRE_DEVICE_EV }, `Looking up a device based on ${avdName}`);
-
-    const adbName = await this.deviceRegistry.allocateDevice(async () => {
-      let freeEmulatorAdbName;
-
-      const { devices } = await this.adb.devices();
-      for (const candidate of devices) {
-        const isEmulator = candidate.type === 'emulator';
-        const isBusy = this.deviceRegistry.isDeviceBusy(candidate.adbName);
-
-        if (isEmulator && !isBusy) {
-          if (await candidate.queryName() === avdName) {
-            freeEmulatorAdbName = candidate.adbName;
-            log.debug({ event: ACQUIRE_DEVICE_EV }, `Found ${candidate.adbName}!`);
-            break;
-          }
-          log.debug({ event: ACQUIRE_DEVICE_EV }, `${candidate.adbName} is available but AVD is different`);
-        } else {
-          log.debug({ event: ACQUIRE_DEVICE_EV }, `${candidate.adbName} is not a free emulator`, isEmulator, !isBusy);
-        }
-      }
-
-      return freeEmulatorAdbName || this._createDevice();
-    });
-
-    log.debug({ event: ACQUIRE_DEVICE_EV }, `Settled on ${adbName}`);
+    const adbName = await this._allocateDevice(avdName);
 
     await this._boot(avdName, adbName);
 
@@ -182,6 +157,21 @@ class EmulatorDriver extends AndroidDriver {
     }
   }
 
+  async _allocateDevice(avdName) {
+    log.debug({ event: ACQUIRE_DEVICE_EV }, `Looking up a device based on ${avdName}`);
+
+    const adbName = await this.deviceRegistry.allocateDevice(() => this._doAllocateDevice(avdName));
+
+    log.debug({ event: ACQUIRE_DEVICE_EV }, `Settled on ${adbName}`);
+    return adbName;
+  }
+
+  async _doAllocateDevice(avdName) {
+    const emulatorLookupHelper = new EmulatorLookupHelper(this.adb, this.deviceRegistry, avdName);
+    const freeEmulatorAdbName = await emulatorLookupHelper.findFreeDevice();
+    return freeEmulatorAdbName || this._createDevice();
+  }
+
   async _createDevice() {
     const {min, max} = DetoxEmulatorsPortRange;
     let port = Math.random() * (max - min) + min;
diff --git a/detox/src/devices/drivers/EmulatorLookupHelper.js b/detox/src/devices/drivers/EmulatorLookupHelper.js
new file mode 100644
index 000000000..1dcd2517d
--- /dev/null
+++ b/detox/src/devices/drivers/EmulatorLookupHelper.js
@@ -0,0 +1,35 @@
+const log = require('../../utils/logger').child({ __filename });
+const AdbDevicesHelper = require('./AdbDevicesHelper');
+
+const ACQUIRE_DEVICE_EV = 'ACQUIRE_DEVICE';
+
+class EmulatorLookupHelper {
+  constructor(adb, deviceRegistry, avdName) {
+    this.adbDevicesHelper = new AdbDevicesHelper(adb);
+    this.deviceRegistry = deviceRegistry;
+    this.avdName = avdName;
+
+    this._matcherFn = this._matcherFn.bind(this);
+  }
+
+  async findFreeDevice() {
+    return await this.adbDevicesHelper.lookupDevice(this._matcherFn);
+  }
+
+  async _matcherFn(candidate) {
+    const isEmulator = candidate.type === 'emulator';
+    const isBusy = this.deviceRegistry.isDeviceBusy(candidate.adbName);
+
+    if (isEmulator && !isBusy) {
+      if (await candidate.queryName() === this.avdName) {
+        log.debug({ event: ACQUIRE_DEVICE_EV }, `Found ${candidate.adbName}!`);
+        return true;
+      }
+      log.debug({ event: ACQUIRE_DEVICE_EV }, `${candidate.adbName} is available but AVD is different`);
+    } else {
+      log.debug({ event: ACQUIRE_DEVICE_EV }, `${candidate.adbName} is not a free emulator`, isEmulator, !isBusy);
+    }
+    return false;
+  }
+}
+module.exports = EmulatorLookupHelper;


Test patch:
diff --git a/detox/src/devices/drivers/AdbDevicesHelper.test.js b/detox/src/devices/drivers/AdbDevicesHelper.test.js
new file mode 100644
index 000000000..f8cec008f
--- /dev/null
+++ b/detox/src/devices/drivers/AdbDevicesHelper.test.js
@@ -0,0 +1,81 @@
+describe('ADB devices helper', () => {
+  let adbMock;
+  let DeviceAllocation;
+  let deviceAllocation;
+
+  beforeEach(() => {
+    adbMock = {
+      devices: jest.fn().mockResolvedValue(new Error('todo in unit test')),
+    };
+
+    DeviceAllocation = require('./AdbDevicesHelper');
+    deviceAllocation = new DeviceAllocation(adbMock);
+  });
+
+  describe('find device', () => {
+    it('should query for devices', async () => {
+      const matchFn = jest.fn().mockResolvedValue(true);
+
+      mockNoAdbDevices();
+      await deviceAllocation.lookupDevice(matchFn);
+      expect(adbMock.devices).toHaveBeenCalled();
+    });
+
+    it('should return null if there are no devices', async () => {
+      const matchFn = jest.fn().mockResolvedValue(true);
+
+      mockNoAdbDevices();
+      const device = await deviceAllocation.lookupDevice(matchFn);
+      expect(device).toEqual(null);
+    });
+
+    it('should look up a device', async () => {
+      const matchFn = jest.fn().mockResolvedValue(true);
+
+      const device = aDevice();
+      mockAdbDevices([device]);
+
+      const matchingDeviceName = await deviceAllocation.lookupDevice(matchFn);
+      expect(matchingDeviceName).toEqual(device.adbName);
+    });
+
+    it('should return null for non-matching device', async () => {
+      const matchFn = jest.fn().mockResolvedValue(false);
+
+      const device = aDevice();
+      mockAdbDevices([device]);
+
+      const matchingDeviceName = await deviceAllocation.lookupDevice(matchFn);
+      expect(matchingDeviceName).toEqual(null);
+    });
+
+    it('should return first suitable device', async () => {
+      const matchFn = jest.fn()
+        .mockResolvedValueOnce(false)
+        .mockResolvedValueOnce(true)
+        .mockResolvedValueOnce(true);
+
+      mockAdbDevices([
+        aDevice('device1'),
+        aDevice('device2'),
+        aDevice('device3'),
+      ]);
+
+      const matchinDeviceName = await deviceAllocation.lookupDevice(matchFn);
+      expect(matchinDeviceName).toEqual('device2');
+    });
+  });
+
+  const aDevice = (adbName = 'mock-name') => ({
+    type: 'mock-type',
+    adbName,
+  });
+
+  const mockAdbDevices = (devices) => {
+    adbMock.devices.mockResolvedValue({
+      devices,
+    });
+  };
+
+  const mockNoAdbDevices = () => mockAdbDevices([]);
+});
diff --git a/detox/src/devices/drivers/EmulatorLookupHelper.test.js b/detox/src/devices/drivers/EmulatorLookupHelper.test.js
new file mode 100644
index 000000000..63a690a09
--- /dev/null
+++ b/detox/src/devices/drivers/EmulatorLookupHelper.test.js
@@ -0,0 +1,107 @@
+
+describe('emulators lookup helper', () => {
+  const mockAdb = {};
+  const mockAvdName = 'mock-avd';
+
+  class MockAdbDevicesHelper {
+    constructor(...args) {
+      adbDevicesHelper.ctor(...args);
+    }
+
+    lookupDevice(...args) {
+      return adbDevicesHelper.lookupDevice(...args);
+    }
+  }
+
+  let adbDevicesHelper;
+  beforeEach(() => {
+    const AdbDevicesHelper = jest.genMockFromModule('./AdbDevicesHelper');
+    adbDevicesHelper = new AdbDevicesHelper();
+    adbDevicesHelper.ctor = jest.fn();
+    jest.mock('./AdbDevicesHelper', () => MockAdbDevicesHelper);
+  });
+
+  let mockDeviceRegistry;
+  let helper;
+  beforeEach(() => {
+    jest.mock('../../utils/logger', () => ({
+      child: jest.fn().mockReturnValue({
+        debug: jest.fn(),
+      }),
+    }));
+
+    mockDeviceRegistry = {
+      isDeviceBusy: jest.fn().mockReturnValue(false),
+    };
+
+    const Helper = require('./EmulatorLookupHelper');
+    helper = new Helper(mockAdb, mockDeviceRegistry, mockAvdName);
+  });
+
+  it('should create an adb devices helper', async () => {
+    expect(adbDevicesHelper.ctor).toHaveBeenCalledWith(mockAdb);
+  });
+
+  it('should pass a custom matcher func onto adb-devices helper for finding a free devices', async () => {
+    await helper.findFreeDevice();
+    expect(adbDevicesHelper.lookupDevice).toHaveBeenCalledWith(expect.any(Function));
+  });
+
+  it('should return the matched device', async () => {
+    const adbName = 'mock-adb-name';
+    adbDevicesHelper.lookupDevice.mockReturnValue(adbName);
+
+    const deviceName = await helper.findFreeDevice();
+    expect(deviceName).toEqual(adbName);
+  });
+
+  describe('Device lookup matcher function', () => {
+    it('should restrict matching to emulators', async () => {
+      const emuDevice = anEmulatorDevice();
+      const nonEmuDevice = aNonEmuDevice();
+
+      await helper.findFreeDevice();
+
+      const matcherFn = resolveMatcherFn();
+      expect(await matcherFn(emuDevice)).toEqual(true);
+      expect(await matcherFn(nonEmuDevice)).toEqual(false);
+    });
+
+    it('should restrict to free (unoccupied) devices', async () => {
+      const device = anEmulatorDevice();
+      mockDeviceRegistry.isDeviceBusy.mockReturnValue(true);
+
+      await helper.findFreeDevice();
+
+      const matcherFn = resolveMatcherFn();
+      expect(await matcherFn(device)).toEqual(false);
+      expect(mockDeviceRegistry.isDeviceBusy).toHaveBeenCalledWith('mock-name');
+    });
+
+    it('should restrict to the AVD name in question', async () => {
+      const device = {
+        ...anEmulatorDevice(),
+        queryName: jest.fn().mockResolvedValue('other ' + mockAvdName),
+      };
+
+      await helper.findFreeDevice();
+
+      const matcherFn = resolveMatcherFn();
+      expect(await matcherFn(device)).toEqual(false);
+    });
+
+    const resolveMatcherFn = () => adbDevicesHelper.lookupDevice.mock.calls[0][0];
+  });
+
+  const anEmulatorDevice = () => ({
+    type: 'emulator',
+    adbName: 'mock-name',
+    queryName: jest.fn().mockResolvedValue(mockAvdName),
+  });
+
+  const aNonEmuDevice = () => ({
+    type: 'non-emulator',
+    adbName: 'mock-name',
+    queryName: jest.fn().mockResolvedValue(mockAvdName),
+  });
+});