Repository: wix/Detox
Pull request #2150 (2020-06-25 07:47:15)

Android driver refactor 1.1: Clean up android-driver and periphery even further
- [x] This is a small change 
- [ ] This change has been discussed in issue #<?> and the solution has been agreed upon with maintainers.

---

**Description:**
Following work on #1192 -- need to break some of the AndroidDriver monolith into sub-units. This is a post-refactor step - 1.1, following #2146, #2148 and #2149: tidy-up work in driver itself, and some periphery.

coveralls: ## Pull Request Test Coverage Report for [Build 2363](https://coveralls.io/builds/31680079)

* **29** of **43**   **(67.44%)**  changed or added relevant lines in **3** files are covered.
* **1** unchanged line in **1** file lost coverage.
* Overall coverage increased (+**0.1%**) to **64.959%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [detox/src/devices/drivers/android/AndroidDriver.js](https://coveralls.io/builds/31680079/source?filename=detox%2Fsrc%2Fdevices%2Fdrivers%2Fandroid%2FAndroidDriver.js#L159) | 21 | 35 | 60.0%
<!-- | **Total:** | **29** | **43** | **67.44%** | -->

|  Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [detox/src/devices/drivers/android/AndroidDriver.js](https://coveralls.io/builds/31680079/source?filename=detox%2Fsrc%2Fdevices%2Fdrivers%2Fandroid%2FAndroidDriver.js#L273) | 1 | 62.7% |
<!-- | **Total:** | **1** |  | -->

|  Totals | [![Coverage Status](https://coveralls.io/builds/31680079/badge)](https://coveralls.io/builds/31680079) |
| :-- | --: |
| Change from base [Build 1250](https://coveralls.io/builds/31668953): |  0.1% |
| Covered Lines: | 2911 |
| Relevant Lines: | 4099 |

---
##### 💛  - [Coveralls](https://coveralls.io)

d4vidi: Merging+closing: coverage warning is since I've changed `AndroidDriver.launchApp()` and yet this doesn't run in CI (regardless of this PR).

Patch:
diff --git a/detox/src/devices/drivers/android/AndroidDriver.js b/detox/src/devices/drivers/android/AndroidDriver.js
index b55b61dde..feb3a44ef 100644
--- a/detox/src/devices/drivers/android/AndroidDriver.js
+++ b/detox/src/devices/drivers/android/AndroidDriver.js
@@ -81,37 +81,12 @@ class AndroidDriver extends DeviceDriverBase {
   }
 
   async launchApp(deviceId, bundleId, launchArgs, languageAndLocale) {
-    let notificationPayloadTargetPath;
-
     await this.emitter.emit('beforeLaunchApp', { deviceId, bundleId, launchArgs });
 
-    if (launchArgs.detoxUserNotificationDataURL) {
-      notificationPayloadTargetPath = await this._sendNotificationDataToDevice(launchArgs.detoxUserNotificationDataURL, deviceId);
-      launchArgs = {
-        ...launchArgs,
-        detoxUserNotificationDataURL: notificationPayloadTargetPath,
-      }
-    }
-
-    if (!this.instrumentation.isRunning()) {
-      await this._launchInstrumentationProcess(deviceId, bundleId, launchArgs);
-      await sleep(500);
-    } else if (launchArgs.detoxURLOverride) {
-      await this._startActivityWithUrl(launchArgs.detoxURLOverride);
-    } else if (launchArgs.detoxUserNotificationDataURL) {
-      await this._startActivityFromNotification(launchArgs.detoxUserNotificationDataURL);
-    } else {
-      await this._resumeMainActivity();
-    }
-
-    let pid = NaN;
-    try {
-      pid = await retry(() => this._queryPID(deviceId, bundleId));
-    } catch (e) {
-      log.warn(await this.adb.shell(deviceId, 'ps'));
-      throw e;
-    }
+    launchArgs = await this._modifyArgsForNotificationHandling(deviceId, bundleId, launchArgs);
+    await this._launchApp(deviceId, bundleId, launchArgs);
 
+    const pid = await this._waitForProcess(deviceId, bundleId);
     await this.emitter.emit('launchApp', { deviceId, bundleId, launchArgs, pid });
     return pid;
   }
@@ -159,20 +134,6 @@ class AndroidDriver extends DeviceDriverBase {
     await this.emitter.emit('terminateApp', { deviceId, bundleId });
   }
 
-  _getInstrumentationCrashError() {
-    return new DetoxRuntimeError({
-      message: 'Failed to run application on the device',
-      hint: this.instrumentationStackTrace
-        ? 'Most likely, your main activity has crashed prematurely.'
-        : 'Most likely, your tests have timed out and called detox.cleanup() ' +
-          'while it was waiting for "ready" message (over WebSocket) ' +
-          'from the instrumentation process.',
-      debugInfo: this.instrumentationStackTrace
-        ? `Native stacktrace dump: ${this.instrumentationStackTrace}`
-        : '',
-    });
-  }
-
   async cleanup(deviceId, bundleId) {
     await this._terminateInstrumentation();
     await super.cleanup(deviceId, bundleId);
@@ -195,18 +156,15 @@ class AndroidDriver extends DeviceDriverBase {
   }
 
   async setURLBlacklist(urlList) {
-    const call = EspressoDetoxApi.setURLBlacklist(urlList);
-    await this.invocationManager.execute(call);
+    await this.invocationManager.execute(EspressoDetoxApi.setURLBlacklist(urlList));
   }
 
   async enableSynchronization() {
-    const call = EspressoDetoxApi.setSynchronization(true);
-    await this.invocationManager.execute(call);
+    await this.invocationManager.execute(EspressoDetoxApi.setSynchronization(true));
   }
 
   async disableSynchronization() {
-    const call = EspressoDetoxApi.setSynchronization(false);
-    await this.invocationManager.execute(call);
+    await this.invocationManager.execute(EspressoDetoxApi.setSynchronization(false));
   }
 
   async takeScreenshot(deviceId, screenshotName) {
@@ -256,6 +214,31 @@ class AndroidDriver extends DeviceDriverBase {
     return testApkPath;
   }
 
+  async _modifyArgsForNotificationHandling(deviceId, bundleId, launchArgs) {
+    let _launchArgs = launchArgs;
+    if (launchArgs.detoxUserNotificationDataURL) {
+      const notificationPayloadTargetPath = await this._sendNotificationDataToDevice(launchArgs.detoxUserNotificationDataURL, deviceId);
+      _launchArgs = {
+        ...launchArgs,
+        detoxUserNotificationDataURL: notificationPayloadTargetPath,
+      }
+    }
+    return _launchArgs;
+  }
+
+  async _launchApp(deviceId, bundleId, launchArgs) {
+    if (!this.instrumentation.isRunning()) {
+      await this._launchInstrumentationProcess(deviceId, bundleId, launchArgs);
+      await sleep(500);
+    } else if (launchArgs.detoxURLOverride) {
+      await this._startActivityWithUrl(launchArgs.detoxURLOverride);
+    } else if (launchArgs.detoxUserNotificationDataURL) {
+      await this._startActivityFromNotification(launchArgs.detoxUserNotificationDataURL);
+    } else {
+      await this._resumeMainActivity();
+    }
+  }
+
   async _launchInstrumentationProcess(deviceId, bundleId, userLaunchArgs) {
     const serverPort = new URL(this.client.configuration.server).port;
     await this.adb.reverse(deviceId, serverPort);
@@ -269,17 +252,13 @@ class AndroidDriver extends DeviceDriverBase {
   async _onInstrumentationTerminated(deviceId, serverPort) {
     await this.adb.reverseRemove(deviceId, serverPort);
     this.instrumentationCloseListener();
-
-    this.instrumentation.setTerminationFn(null);
-    this.instrumentation.setLogListenFn(null);
+    this.instrumentation.clearAllCallbackFn();
   }
 
   async _terminateInstrumentation() {
     await this.instrumentation.terminate();
     this.instrumentationCloseListener();
-
-    this.instrumentation.setTerminationFn(null);
-    this.instrumentation.setLogListenFn(null);
+    this.instrumentation.clearAllCallbackFn();
   }
 
   _extractStackTraceFromInstrumLogs(logsDump) {
@@ -290,24 +269,6 @@ class AndroidDriver extends DeviceDriverBase {
     }
   }
 
-  async _queryPID(deviceId, bundleId, waitAtStart = true) {
-    if (waitAtStart) {
-      await sleep(500);
-    }
-
-    for (let attempts = 5; attempts > 0; attempts--) {
-      const pid = await this.adb.pidof(deviceId, bundleId);
-
-      if (pid > 0) {
-        return pid;
-      }
-
-      await sleep(1000);
-    }
-
-    return NaN;
-  }
-
   async _sendNotificationDataToDevice(dataFileLocalPath, deviceId) {
     await this.fileXfer.prepareDestinationDir(deviceId);
     return await this.fileXfer.send(deviceId, dataFileLocalPath, 'notification.json');
@@ -324,6 +285,42 @@ class AndroidDriver extends DeviceDriverBase {
   _resumeMainActivity() {
     return this.invocationManager.execute(DetoxApi.launchMainActivity());
   }
+
+  async _waitForProcess(deviceId, bundleId) {
+    let pid = NaN;
+    try {
+      const queryPid = () => this._queryPID(deviceId, bundleId);
+      const retryQueryPid = () => retry({ backoff: 'none', retries: 4 }, queryPid);
+      const retryQueryPidMultiple = () => retry({ backoff: 'linear' }, retryQueryPid);
+      pid = await retryQueryPidMultiple();
+    } catch (e) {
+      log.warn(await this.adb.shell(deviceId, 'ps'));
+      throw e;
+    }
+    return pid;
+  }
+
+  async _queryPID(deviceId, bundleId) {
+    const pid = await this.adb.pidof(deviceId, bundleId);
+    if (!pid) {
+      throw new Error('PID still not available');
+    }
+    return pid;
+  }
+
+  _getInstrumentationCrashError() {
+    return new DetoxRuntimeError({
+      message: 'Failed to run application on the device',
+      hint: this.instrumentationStackTrace
+        ? 'Most likely, your main activity has crashed prematurely.'
+        : 'Most likely, your tests have timed out and called detox.cleanup() ' +
+        'while it was waiting for "ready" message (over WebSocket) ' +
+        'from the instrumentation process.',
+      debugInfo: this.instrumentationStackTrace
+        ? `Native stacktrace dump: ${this.instrumentationStackTrace}`
+        : '',
+    });
+  }
 }
 
 module.exports = AndroidDriver;
diff --git a/detox/src/devices/drivers/android/tools/Instrumentation.js b/detox/src/devices/drivers/android/tools/Instrumentation.js
index 5a4a20cb0..77714e707 100644
--- a/detox/src/devices/drivers/android/tools/Instrumentation.js
+++ b/detox/src/devices/drivers/android/tools/Instrumentation.js
@@ -42,6 +42,11 @@ class Instrumentation {
     this.userLogListenFn = userLogListenFn || _.noop;
   }
 
+  clearAllCallbackFn() {
+    this.setTerminationFn(null);
+    this.setLogListenFn(null);
+  }
+
   _getSpawnArgs(userLaunchArgs) {
     const launchArgs = prepareInstrumentationArgs(userLaunchArgs);
     const additionalLaunchArgs = prepareInstrumentationArgs({ debug: false });
diff --git a/detox/src/utils/retry.js b/detox/src/utils/retry.js
index 2fd9eddf7..308ed119b 100644
--- a/detox/src/utils/retry.js
+++ b/detox/src/utils/retry.js
@@ -3,6 +3,11 @@ const sleep = require('./sleep');
 const DEFAULT_RETRIES = 9;
 const DEFAULT_INTERVAL = 500;
 const DEFAULT_CONDITION_FN = () => true;
+const DEFAULT_BACKOFF_MODE = 'linear';
+const backoffModes = {
+  'linear': () => ({ interval, totalRetries }) => totalRetries * interval,
+  'none': () => ({ interval }) => interval,
+};
 
 async function retry(options, func) {
   if (typeof options === 'function') {
@@ -13,9 +18,12 @@ async function retry(options, func) {
   const {
     retries = DEFAULT_RETRIES,
     interval = DEFAULT_INTERVAL,
+    backoff = DEFAULT_BACKOFF_MODE,
     conditionFn = DEFAULT_CONDITION_FN,
   } = options;
 
+  const backoffFn = backoffModes[backoff]();
+
   for (let totalRetries = 1; true; totalRetries++) {
     try {
       return await func(totalRetries);
@@ -23,7 +31,7 @@ async function retry(options, func) {
       if (!conditionFn(e) || (totalRetries > retries)) {
         throw e;
       }
-      await sleep(totalRetries * interval);
+      await sleep(backoffFn({ interval, totalRetries }));
     }
   }
 }


Test patch:
diff --git a/detox/src/devices/drivers/android/tools/Instrumentation.test.js b/detox/src/devices/drivers/android/tools/Instrumentation.test.js
index 9519bb297..c6b4a1233 100644
--- a/detox/src/devices/drivers/android/tools/Instrumentation.test.js
+++ b/detox/src/devices/drivers/android/tools/Instrumentation.test.js
@@ -250,6 +250,16 @@ describe('Instrumentation', () => {
     expect(logListenCallback).not.toHaveBeenCalled();
   });
 
+  it('should allow for clearing of all user-callbacks', async () => {
+    uut.clearAllCallbackFn();
+
+    await uut.launch(deviceId, bundleId, []);
+    await invokeDataCallbackWith('data');
+    await invokeTerminationCallback();
+    expect(userTerminationCallback).not.toHaveBeenCalled();
+    expect(logListenCallback).not.toHaveBeenCalled();
+  });
+
   const extractDataCallback = () => childProcess.stdout.on.mock.calls[0][1];
   const invokeDataCallbackWith = async (data) => {
     const fn = extractDataCallback();
diff --git a/detox/src/utils/retry.test.js b/detox/src/utils/retry.test.js
index 0f079df8e..6e49f7b98 100644
--- a/detox/src/utils/retry.test.js
+++ b/detox/src/utils/retry.test.js
@@ -2,6 +2,15 @@ describe('retry', () => {
   let sleep;
   let retry;
 
+  const mockFailingUserFn = () => jest.fn().mockReturnValue(Promise.reject(new Error('a thing')));
+  const mockFailingOnceUserFn = () => jest.fn()
+    .mockReturnValueOnce(Promise.reject())
+    .mockReturnValueOnce(Promise.resolve());
+  const mockFailingTwiceUserFn = () => jest.fn()
+    .mockReturnValueOnce(Promise.reject('once'))
+    .mockReturnValueOnce(Promise.reject('twice'))
+    .mockReturnValueOnce(Promise.resolve());
+
   beforeEach(() => {
     jest.mock('./sleep', () => jest.fn().mockReturnValue(Promise.resolve()));
     sleep = require('./sleep');
@@ -10,31 +19,25 @@ describe('retry', () => {
   });
 
   it('should retry once over a function that fails once', async () => {
-    const mockFnc = jest.fn()
-                        .mockReturnValueOnce(Promise.reject())
-                        .mockReturnValueOnce(Promise.resolve());
+    const mockFn = mockFailingOnceUserFn();
 
     try {
-      await retry({retries: 999, interval: 0}, mockFnc);
+      await retry({retries: 999, interval: 0}, mockFn);
     } catch (e) {
       fail('expected retry not to fail');
     }
 
-    expect(mockFnc).toHaveBeenCalledTimes(2);
+    expect(mockFn).toHaveBeenCalledTimes(2);
   });
 
   it('should retry multiple times', async () => {
-    const mockFnc = jest.fn()
-                        .mockReturnValueOnce(Promise.reject('once'))
-                        .mockReturnValueOnce(Promise.reject('twice'))
-                        .mockReturnValueOnce(Promise.resolve());
-    await retry({retries: 999, interval: 0}, mockFnc);
-    expect(mockFnc).toHaveBeenCalledTimes(3);
+    const mockFn = mockFailingTwiceUserFn();
+    await retry({retries: 999, interval: 0}, mockFn);
+    expect(mockFn).toHaveBeenCalledTimes(3);
   });
 
   it('should adhere to retries parameter', async () => {
-    const mockFn = jest.fn()
-                       .mockReturnValue(Promise.reject(new Error('a thing')));
+    const mockFn = mockFailingUserFn();
     try {
       await retry({retries: 2, interval: 1}, mockFn);
       fail('expected retry to fail and throw');
@@ -44,15 +47,52 @@ describe('retry', () => {
     }
   });
 
-  it('should adhere to interval parameter, and sleep for increasingly long intervals', async () => {
+  it('should adhere to interval parameter, and sleep for increasingly long intervals (i.e. the default backoff mode)', async () => {
+    const mockFn = mockFailingUserFn();
     const baseInterval = 111;
-    const mockFn = jest.fn().mockReturnValue(Promise.reject(new Error('a thing')));
 
     try {
       await retry({retries: 2, interval: baseInterval}, mockFn);
       fail('expected retry to fail and throw');
-    } catch (error) {
-    }
+    } catch (error) {}
+
+    expect(sleep).toHaveBeenCalledTimes(2);
+    expect(sleep).toHaveBeenCalledWith(baseInterval);
+    expect(sleep).toHaveBeenCalledWith(baseInterval * 2);
+  });
+
+  it('should allow for a constant sleep interval instead of an increasing one by setting backoff="none"', async () => {
+    const mockFn = mockFailingUserFn();
+    const baseInterval = 111;
+    const options = {
+      retries: 2,
+      interval: baseInterval,
+      backoff: 'none',
+    };
+
+    try {
+      await retry(options, mockFn);
+      fail('expected retry to fail and throw');
+    } catch (error) {}
+
+    expect(sleep).toHaveBeenCalledTimes(2);
+    expect(sleep).toHaveBeenNthCalledWith(1, baseInterval);
+    expect(sleep).toHaveBeenNthCalledWith(2, baseInterval);
+  });
+
+  it('should allow for the default linear backoff when set explicitly as "linear"', async () => {
+    const mockFn = mockFailingUserFn();
+    const baseInterval = 111;
+    const options = {
+      retries: 2,
+      interval: baseInterval,
+      backoff: 'linear',
+    };
+
+    try {
+      await retry(options, mockFn);
+      fail('expected retry to fail and throw');
+    } catch (error) {}
 
     expect(sleep).toHaveBeenCalledTimes(2);
     expect(sleep).toHaveBeenCalledWith(baseInterval);
@@ -60,8 +100,7 @@ describe('retry', () => {
   });
 
   it('should adhere to a custom condition', async () => {
-    const mockFn = jest.fn()
-                       .mockReturnValue(Promise.reject(new Error('a thing')));
+    const mockFn = mockFailingUserFn();
     const conditionFn = jest.fn()
                             .mockReturnValueOnce(true)
                             .mockReturnValueOnce(false);
@@ -69,22 +108,21 @@ describe('retry', () => {
     try {
       await retry({retries: 999, interval: 1, conditionFn}, mockFn);
       fail('expected retry to fail and throw');
-    } catch (error) {
-    }
+    } catch (error) {}
 
     expect(mockFn).toHaveBeenCalledTimes(2);
   });
 
   it('should work with default retries+interval values', async () => {
+    const mockFn = mockFailingUserFn();
     const defaultRetries = 9;
     const defaultInterval = 500;
-    const mockFn = jest.fn().mockReturnValue(Promise.reject(new Error('a thing')));
 
     try {
       await retry(mockFn);
       fail('expected retry to fail and throw');
-    } catch (error) {
-    }
+    } catch (error) {}
+
     expect(mockFn).toHaveBeenCalledTimes(defaultRetries + 1);
     expect(sleep).toHaveBeenCalledWith(defaultInterval);
   });