Repository: wix/Detox
Pull request #2476 (2020-11-22 15:44:29)

Improve Genymotion leakage warning by including instance names
## Description

Improve Genymotion leakage warnings by including instance names, in addition to meaningless UUID's.
This incorporates 2 main things:
1. Enhancing the device registry such that it would allow for managing object, and not just strings (!)
2. Managing the device cleanup registry using objects, containing both instance `uuid`'s _and_ names (thus allowing for the instance names to be logged, eventually, in global-cleanup).

Related to #2429.


Patch:
diff --git a/detox/ios/Detox/Invocation/Predicate.swift b/detox/ios/Detox/Invocation/Predicate.swift
index 303984c0a..91fbac034 100644
--- a/detox/ios/Detox/Invocation/Predicate.swift
+++ b/detox/ios/Detox/Invocation/Predicate.swift
@@ -204,7 +204,7 @@ class ValuePredicate : Predicate {
 	]
 	
 	static let translator : [String: String] = [
-		"accessibilityIdentifier": "identifier:",
+		"accessibilityIdentifier": "identifier",
 		"accessibilityLabel": "label",
 		"dtx_text": "text",
 		"accessibilityValue": "value"
diff --git a/detox/src/devices/DeviceRegistry.js b/detox/src/devices/DeviceRegistry.js
index 1aeec9f63..e11a41509 100644
--- a/detox/src/devices/DeviceRegistry.js
+++ b/detox/src/devices/DeviceRegistry.js
@@ -3,6 +3,9 @@ const environment = require('../utils/environment');
 const ExclusiveLockfile = require('../utils/ExclusiveLockfile');
 const safeAsync = require('../utils/safeAsync');
 
+const getDeviceEqualsFn = (deviceHandle) => (otherDeviceHandle) => _.isEqual(otherDeviceHandle, deviceHandle);
+const getDeviceDifferFn = (deviceHandle) => (otherDeviceHandle) => !_.isEqual(otherDeviceHandle, deviceHandle);
+
 class DeviceRegistry {
   constructor({ lockfilePath }) {
     /***
@@ -22,30 +25,31 @@ class DeviceRegistry {
   }
 
   /***
-   * @param {string|Function} getDeviceId
+   * @param {string|Function} getDeviceHandle
    * @returns {Promise<string>}
    */
-  async allocateDevice(getDeviceId) {
+  async allocateDevice(getDeviceHandle) {
     return this._lockfile.exclusively(async () => {
-      const deviceId = await safeAsync(getDeviceId);
-      this._toggleDeviceStatus(deviceId, true);
-      return deviceId;
+      const deviceHandle = await safeAsync(getDeviceHandle);
+      this._toggleDeviceStatus(deviceHandle, true);
+      return deviceHandle;
     });
   }
 
   /***
-   * @param {string|Function} getDeviceId
+   * @param {string|Function} getDeviceHandle
    * @returns {void}
    */
-  async disposeDevice(getDeviceId) {
+  async disposeDevice(getDeviceHandle) {
     await this._lockfile.exclusively(async () => {
-      const deviceId = await safeAsync(getDeviceId);
+      const deviceId = await safeAsync(getDeviceHandle);
       this._toggleDeviceStatus(deviceId, false);
     });
   }
 
-  includes(deviceId) {
-    return this._lockfile.read().includes(deviceId);
+  includes(deviceHandle) {
+    const deviceEqualsFn = getDeviceEqualsFn(deviceHandle);
+    return !!_.find(this._lockfile.read(), deviceEqualsFn);
   }
 
   getRegisteredDevices() {
@@ -70,11 +74,12 @@ class DeviceRegistry {
   /***
    * @private
    */
-  _toggleDeviceStatus(deviceId, busy) {
+  _toggleDeviceStatus(deviceHandle, busy) {
+    const deviceDifferFn = getDeviceDifferFn(deviceHandle);
     const state = this._lockfile.read();
     const newState = busy
-      ? _.concat(state, deviceId)
-      : _.without(state, deviceId);
+      ? _.concat(state, deviceHandle)
+      : _.filter(state, deviceDifferFn);
     this._lockfile.write(newState);
   }
 
diff --git a/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.js b/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.js
index 4bd081982..84713181a 100644
--- a/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.js
+++ b/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.js
@@ -1,4 +1,5 @@
 const AndroidDeviceAllocator = require('../AndroidDeviceAllocator');
+const GenyCloudInstanceHandle = require('./GenyCloudInstanceHandle');
 const retry = require('../../../../utils/retry');
 const logger = require('../../../../utils/logger').child({ __filename });
 
@@ -13,8 +14,10 @@ class GenyCloudDeviceAllocator extends AndroidDeviceAllocator {
 
   async _doAllocateDevice(recipe) {
     let { instance, isNew } = await this._doSynchronizedAllocation(recipe);
+    const instanceHandle = new GenyCloudInstanceHandle(instance);
+
     if (isNew) {
-      await this.deviceCleanupRegistry.allocateDevice(instance.uuid);
+      await this.deviceCleanupRegistry.allocateDevice(instanceHandle);
     }
 
     instance = await this._waitForInstanceBoot(instance);
@@ -22,7 +25,7 @@ class GenyCloudDeviceAllocator extends AndroidDeviceAllocator {
     return {
       instance,
       isNew,
-      toString: () => `GenyCloud:${instance.name} (${instance.uuid})`,
+      toString: () => instanceHandle.toString(),
     }
   }
 
diff --git a/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.js b/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.js
index abc686067..3177b07ee 100644
--- a/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.js
+++ b/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.js
@@ -94,11 +94,11 @@ class GenyCloudDriver extends AndroidDriver {
 
   static async globalCleanup() {
     const deviceCleanupRegistry = GenyDeviceRegistryFactory.forGlobalShutdown();
-    const deviceUUIDs = await deviceCleanupRegistry.readRegisteredDevices();
-    if (deviceUUIDs.length) {
+    const deviceHandles = await deviceCleanupRegistry.readRegisteredDevices();
+    if (deviceHandles.length) {
       const exec = new GenyCloudExec(environment.getGmsaasPath());
       const instanceLifecycleService = new InstanceLifecycleService(exec, null);
-      await doCleanup(instanceLifecycleService, deviceUUIDs);
+      await doCleanup(instanceLifecycleService, deviceHandles);
     }
   }
 }
@@ -107,13 +107,13 @@ const cleanupLogData = {
   event: 'GENYCLOUD_TEARDOWN',
 };
 
-async function doCleanup(instanceLifecycleService, deviceUUIDs) {
+async function doCleanup(instanceLifecycleService, deviceHandles) {
   logger.info(cleanupLogData, 'Initiating Genymotion cloud instances teardown...');
 
   const deletionLeaks = [];
-  const killPromises = deviceUUIDs.map((uuid) =>
+  const killPromises = deviceHandles.map(({ uuid, name }) =>
     instanceLifecycleService.deleteInstance(uuid)
-      .catch((error) => deletionLeaks.push({ uuid, error })));
+      .catch((error) => deletionLeaks.push({ uuid, name, error })));
 
   await Promise.all(killPromises);
   reportGlobalCleanupSummary(deletionLeaks);
@@ -123,8 +123,8 @@ function reportGlobalCleanupSummary(deletionLeaks) {
   if (deletionLeaks.length) {
     logger.warn(cleanupLogData, 'WARNING! Detected a Genymotion cloud instance leakage, for the following instances:');
 
-    deletionLeaks.forEach(({ uuid, error }) =>
-      logger.warn(cleanupLogData, `Instance UUID ${uuid}: ${error}`));
+    deletionLeaks.forEach(({ uuid, name, error }) =>
+      logger.warn(cleanupLogData, `Instance ${name} (${uuid}): ${error}`));
 
     logger.info(cleanupLogData, 'Instances teardown completed with warnings');
   } else {
diff --git a/detox/src/devices/drivers/android/genycloud/GenyCloudInstanceHandle.js b/detox/src/devices/drivers/android/genycloud/GenyCloudInstanceHandle.js
new file mode 100644
index 000000000..2e58c6278
--- /dev/null
+++ b/detox/src/devices/drivers/android/genycloud/GenyCloudInstanceHandle.js
@@ -0,0 +1,12 @@
+class GenyCloudInstanceHandle {
+  constructor(instance) {
+    this.uuid = instance.uuid;
+    this.name = instance.name;
+  }
+
+  toString() {
+    return `GenyCloud:${this.name} (${this.uuid})`;
+  }
+}
+
+module.exports = GenyCloudInstanceHandle;
diff --git a/detox/src/devices/drivers/android/genycloud/services/dto/GenyInstance.js b/detox/src/devices/drivers/android/genycloud/services/dto/GenyInstance.js
index 307c03b3a..a84e741b9 100644
--- a/detox/src/devices/drivers/android/genycloud/services/dto/GenyInstance.js
+++ b/detox/src/devices/drivers/android/genycloud/services/dto/GenyInstance.js
@@ -1,4 +1,3 @@
-const _ = require('lodash');
 const Recipe = require('./GenyRecipe');
 
 const STATE_ONLINE = 'ONLINE';
@@ -13,7 +12,9 @@ const initStates = [
 
 class GenyInstance {
   constructor(rawInstance) {
-    Object.assign(this, _.pick(rawInstance, 'uuid', 'name', 'state'));
+    this.uuid = rawInstance.uuid;
+    this.name = rawInstance.name;
+    this.state = rawInstance.state;
     this.adb = {
       name: rawInstance.adb_serial,
       port: rawInstance.adb_serial_port,
diff --git a/detox/src/devices/drivers/android/genycloud/services/dto/GenyRecipe.js b/detox/src/devices/drivers/android/genycloud/services/dto/GenyRecipe.js
index 19d196b75..792d692e2 100644
--- a/detox/src/devices/drivers/android/genycloud/services/dto/GenyRecipe.js
+++ b/detox/src/devices/drivers/android/genycloud/services/dto/GenyRecipe.js
@@ -1,8 +1,7 @@
-const _ = require('lodash');
-
-class Recipe {
+class GenyRecipe {
   constructor(rawRecipe) {
-    Object.assign(this, _.pick(rawRecipe, 'uuid', 'name'));
+    this.uuid = rawRecipe.uuid;
+    this.name = rawRecipe.name;
   }
 
   toString() {
@@ -10,4 +9,4 @@ class Recipe {
   }
 }
 
-module.exports = Recipe;
+module.exports = GenyRecipe;


Test patch:
diff --git a/detox/src/devices/DeviceRegistry.test.js b/detox/src/devices/DeviceRegistry.test.js
index e2857ed90..06d2fc6d0 100644
--- a/detox/src/devices/DeviceRegistry.test.js
+++ b/detox/src/devices/DeviceRegistry.test.js
@@ -19,37 +19,37 @@ describe('DeviceRegistry', () => {
       await fs.remove(lockfilePath);
     });
 
-    async function allocateDevice(deviceId) {
-      return registry.allocateDevice(() => deviceId);
+    async function allocateDevice(deviceHandle) {
+      return registry.allocateDevice(() => deviceHandle);
     }
 
-    function expectRegisteredDevices(...deviceIds) {
+    function expectRegisteredDevices(...deviceHandles) {
       return registry.allocateDevice(() => {
-        expect(registry.getRegisteredDevices()).toEqual([ ...deviceIds ]);
+        expect(registry.getRegisteredDevices()).toEqual([ ...deviceHandles ]);
         throw new Error('ignored'); // So it wouldn't really allocate anything
       }).catch((e) => { if (e.message !== 'ignored') throw e });
     }
 
-    async function checkDeviceRegisteredAndDispose(deviceId) {
+    async function checkDeviceRegisteredAndDispose(deviceHandle) {
       return registry.disposeDevice(async () => {
-        expect(registry.includes(deviceId)).toBe(true);
-        return deviceId;
+        expect(registry.includes(deviceHandle)).toBe(true);
+        return deviceHandle;
       });
     }
 
-    async function disposeDevice(deviceId) {
-      return registry.disposeDevice(() => deviceId);
+    async function disposeDevice(deviceHandle) {
+      return registry.disposeDevice(() => deviceHandle);
     }
 
-    async function checkDeviceNotRegistered(deviceId) {
+    async function checkDeviceIsNotRegistered(deviceHandle) {
       return registry.allocateDevice(async () => {
-        expect(registry.includes(deviceId)).toBe(false);
+        expect(registry.includes(deviceHandle)).toBe(false);
         throw new Error('ignored'); // So it wouldn't really allocate anything
       }).catch((e) => { if (e.message !== 'ignored') throw e });
     }
 
-    async function checkRegisteredDevicesEqual(...deviceIds) {
-      expect(await registry.readRegisteredDevices()).toEqual([ ...deviceIds ]);
+    async function checkRegisteredDevicesEqual(...deviceHandles) {
+      expect(await registry.readRegisteredDevices()).toEqual([ ...deviceHandles ]);
     }
 
     const assertForbiddenOutOfContextRegistryQuery = () =>
@@ -59,51 +59,78 @@ describe('DeviceRegistry', () => {
       expect(() => registry.getRegisteredDevices()).toThrowError();
 
     it('should be able to tell whether a device is registered', async () => {
-      const deviceId = 'emulator-5554';
-      await allocateDevice(deviceId);
-      await checkDeviceRegisteredAndDispose(deviceId);
-      await checkDeviceNotRegistered(deviceId);
+      const deviceHandle = 'emulator-5554';
+      await allocateDevice(deviceHandle);
+      await checkDeviceRegisteredAndDispose(deviceHandle);
+      await checkDeviceIsNotRegistered(deviceHandle);
+    });
+
+    it('should be able to tell whether a device is registered, given objects for handles', async () => {
+      const rawDeviceHandle = {
+        type: 'mocked-device-type',
+        adbName: 'localhost:11111',
+      };
+      const deviceHandle = {
+        ...rawDeviceHandle,
+        mockFunc: () => 'mocked-func-result',
+      };
+
+      await allocateDevice(deviceHandle);
+      await checkDeviceRegisteredAndDispose(rawDeviceHandle);
+      await checkDeviceIsNotRegistered(deviceHandle);
     });
 
     it('should throw on attempt of checking whether a device is registered outside of allocation/disposal context', async () => {
-      const deviceId = 'emulator-5554';
+      const deviceHandle = 'emulator-5554';
 
       assertForbiddenOutOfContextRegistryQuery();
 
-      await allocateDevice(deviceId);
+      await allocateDevice(deviceHandle);
       assertForbiddenOutOfContextRegistryQuery();
     });
 
     it('should be able to fast-get a valid list of registered devices', async () => {
-      const deviceId = 'emulator-5554';
-      const anotherDeviceId = {
+      const deviceHandle = 'emulator-5554';
+      const anotherDeviceHandle = {
         type: 'mocked-device-type',
         adbName: 'emulator-5556',
       };
 
-      await allocateDevice(deviceId);
-      await allocateDevice(anotherDeviceId);
-      await expectRegisteredDevices(deviceId, anotherDeviceId);
+      await allocateDevice(deviceHandle);
+      await allocateDevice(anotherDeviceHandle);
+      await expectRegisteredDevices(deviceHandle, anotherDeviceHandle);
     });
 
     it('should throw on attempt of fast-getting registered devices list outside of allocation/disposal context', async () => {
-      const deviceId = 'emulator-5554';
+      const deviceHandle = 'emulator-5554';
 
       assertForbiddenOutOfContextDeviceListQuery();
 
-      await allocateDevice(deviceId);
+      await allocateDevice(deviceHandle);
       assertForbiddenOutOfContextDeviceListQuery();
     });
 
     it('should be able to read a valid list of registered devices', async () => {
-      const deviceId = 'emulator-5554';
-      const anotherDeviceId = 'emulator-5556';
+      const deviceHandle = 'emulator-5554';
+      const anotherDeviceHandle = 'emulator-5556';
+
+      await allocateDevice(deviceHandle);
+      await allocateDevice(anotherDeviceHandle);
+      await checkRegisteredDevicesEqual(deviceHandle, anotherDeviceHandle);
+      await disposeDevice(deviceHandle);
+      await checkRegisteredDevicesEqual(anotherDeviceHandle);
+    });
+
+    it('should be able to dispose devices with object-like handles', async () => {
+      const deviceId = {
+        type: 'mocked-device-type',
+        adbName: 'emulator-5554',
+      };
 
       await allocateDevice(deviceId);
-      await allocateDevice(anotherDeviceId);
-      await checkRegisteredDevicesEqual(deviceId, anotherDeviceId);
+      await checkRegisteredDevicesEqual(deviceId);
       await disposeDevice(deviceId);
-      await checkRegisteredDevicesEqual(anotherDeviceId);
+      await checkRegisteredDevicesEqual();
     });
 
     describe('.reset() method', () => {
diff --git a/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.test.js b/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.test.js
index 5ba399705..61c386760 100644
--- a/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.test.js
+++ b/detox/src/devices/drivers/android/genycloud/GenyCloudDeviceAllocator.test.js
@@ -220,6 +220,26 @@ describe('Genymotion-Cloud device allocator', () => {
     expect(deviceRegistry.allocateDevice).toHaveBeenCalled();
   });
 
+  it('should register a new device for cleanup', async () => {
+    const instance = aFullyConnectedInstance();
+    givenNoFreeInstances();
+    givenCreatedInstance(instance);
+
+    await uut.allocateDevice(aRecipe());
+    expect(deviceCleanupRegistry.allocateDevice).toHaveBeenCalledWith({
+      uuid: instance.uuid,
+      name: instance.name,
+    });
+  });
+
+  it('should not register an existing device for cleanup', async () => {
+    const instance = aFullyConnectedInstance();
+    givenFreeInstance(instance);
+
+    await uut.allocateDevice(aRecipe());
+    expect(deviceCleanupRegistry.allocateDevice).not.toHaveBeenCalled();
+  });
+
   it('should log pre-allocate event', async () => {
     givenFreeInstance(aFullyConnectedInstance());
     await uut.allocateDevice(aRecipe());
diff --git a/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.test.js b/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.test.js
index d5c69a491..170e15852 100644
--- a/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.test.js
+++ b/detox/src/devices/drivers/android/genycloud/GenyCloudDriver.test.js
@@ -20,6 +20,7 @@ describe('Genymotion-cloud driver', () => {
 
   const anInstance = () => ({
     uuid: 'mock-instance-uuid',
+    name: 'mock-instance-name',
     adbName: 'mock-instance-adb-name',
   });
 
@@ -141,10 +142,10 @@ describe('Genymotion-cloud driver', () => {
         const deviceQuery = aDeviceQuery();
         const result = await uut.acquireFreeDevice(deviceQuery);
 
-        expect(result).toEqual({
+        expect(result).toEqual(expect.objectContaining({
           adbName: instance.adbName,
           uuid: instance.uuid,
-        });
+        }));
         expect(deviceQueryHelper().getRecipeFromQuery).toHaveBeenCalledWith(deviceQuery);
         expect(deviceAllocator().allocateDevice).toHaveBeenCalledWith(recipe);
       });
@@ -326,34 +327,39 @@ describe('Genymotion-cloud driver', () => {
     });
 
     describe('global clean-up', () => {
-      const givenDeletionPendingDevices = (deviceUUIDs) => deviceCleanupRegistry.readRegisteredDevices.mockResolvedValue(deviceUUIDs);
+      const givenDeletionPendingDevices = (devicesHandles) => deviceCleanupRegistry.readRegisteredDevices.mockResolvedValue(devicesHandles);
       const givenNoDeletionPendingDevices = () => givenDeletionPendingDevices([]);
-      const givenDeletionPendingInstances = (instances) => givenDeletionPendingDevices(_.map(instances, 'uuid'));
+      const givenDeletionPendingInstances = (instances) => givenDeletionPendingDevices( _.map(instances, ({ uuid, name }) => ({ uuid, name }) ));
       const givenDeletionResult = (deletedInstance) => instanceLifecycleService.deleteInstance.mockResolvedValue(deletedInstance);
 
-      const assertablePendingPromise = () => {
+      const anAssertablePendingPromise = () => {
         let promiseAck = jest.fn();
         const promise = new Promise(resolve => setTimeout(resolve, 1)).then(promiseAck);
         promise.assertResolved = () => expect(promiseAck).toHaveBeenCalled();
         return promise;
       };
 
+      const aPendingDevice = (name, uuid) => ({ name, uuid });
+
       it('should kill all deletion-pending device', async () => {
-        const killPromise1 = assertablePendingPromise();
-        const killPromise2 = assertablePendingPromise();
+        const killPromise1 = anAssertablePendingPromise();
+        const killPromise2 = anAssertablePendingPromise();
         instanceLifecycleService.deleteInstance
           .mockReturnValueOnce(killPromise1)
           .mockReturnValueOnce(killPromise2);
 
-        const deviceUUIDs = ['device1-uuid', 'device2-uuid'];
-        givenDeletionPendingDevices(deviceUUIDs);
+        const deviceHandles = [
+          aPendingDevice('device1', 'uuid1'),
+          aPendingDevice('device2', 'uuid2'),
+        ];
+        givenDeletionPendingDevices(deviceHandles);
 
         await GenyCloudDriver.globalCleanup();
 
         killPromise1.assertResolved();
         killPromise2.assertResolved();
-        expect(instanceLifecycleService.deleteInstance).toHaveBeenCalledWith(deviceUUIDs[0]);
-        expect(instanceLifecycleService.deleteInstance).toHaveBeenCalledWith(deviceUUIDs[1]);
+        expect(instanceLifecycleService.deleteInstance).toHaveBeenCalledWith('uuid1');
+        expect(instanceLifecycleService.deleteInstance).toHaveBeenCalledWith('uuid2');
       });
 
       it('should warn of instances deletion rejects', async () => {
@@ -362,13 +368,17 @@ describe('Genymotion-cloud driver', () => {
           .mockResolvedValueOnce(anInstance())
           .mockRejectedValueOnce(new Error('mock-error2'));
 
-        givenDeletionPendingDevices(['failing-uuid1', 'nonfailing-uuid', 'failing-uuid2']);
+        givenDeletionPendingDevices([
+          aPendingDevice('failing1', 'uuid1'),
+          aPendingDevice('nonfailing', 'uuid'),
+          aPendingDevice('failing2', 'uuid2'),
+        ]);
 
         await GenyCloudDriver.globalCleanup();
 
         expect(logger.warn).toHaveBeenCalledWith({ event: 'GENYCLOUD_TEARDOWN' }, 'WARNING! Detected a Genymotion cloud instance leakage, for the following instances:');
-        expect(logger.warn).toHaveBeenCalledWith({ event: 'GENYCLOUD_TEARDOWN' }, expect.stringMatching(/failing-uuid1:.*mock-error1/));
-        expect(logger.warn).toHaveBeenCalledWith({ event: 'GENYCLOUD_TEARDOWN' }, expect.stringMatching(/failing-uuid2:.*mock-error2/));
+        expect(logger.warn).toHaveBeenCalledWith({ event: 'GENYCLOUD_TEARDOWN' }, expect.stringMatching(/failing1 \(uuid1\): .*mock-error1/));
+        expect(logger.warn).toHaveBeenCalledWith({ event: 'GENYCLOUD_TEARDOWN' }, expect.stringMatching(/failing2 \(uuid2\): .*mock-error2/));
         expect(logger.warn).toHaveBeenCalledTimes(3);
       });