Repository: wix/Detox
Pull request #1971 (2020-03-24 14:17:28)

feat(artifacts): take appNotReady.png screenshot if needed
- [x] This is a small change 

---

**Description:**

Supplements #1914 . Will be taking an automatic screenshot `appNotReady.png`, when there was an issue with launching the app, when it never signalled back that it is ready and active.

@d4vidi, here the question is mostly whether `device.driver.emitter` makes you smell _Feature envy_, one of the code smells, or it is fine?


d4vidi: >here the question is mostly whether device.driver.emitter makes you smell Feature envy
😄 
TBH it's weird for me to begin with that the emitter is a strict property of the driver. Feels like it should be an object shared among the designated emitters. You know what I mean?
noomorph: I have rewritten emitters code according to what you suggested earlier, so, now we have a single emitter instance per one Detox, and it is passed down to devices and device drivers.

Patch:
diff --git a/detox/src/Detox.js b/detox/src/Detox.js
index c98ef2e41..9535b9c1d 100644
--- a/detox/src/Detox.js
+++ b/detox/src/Detox.js
@@ -9,6 +9,7 @@ const EmulatorDriver = require('./devices/drivers/android/EmulatorDriver');
 const AttachedAndroidDriver = require('./devices/drivers/android/AttachedAndroidDriver');
 const DetoxRuntimeError = require('./errors/DetoxRuntimeError');
 const argparse = require('./utils/argparse');
+const AsyncEmitter = require('./utils/AsyncEmitter');
 const MissingDetox = require('./utils/MissingDetox');
 const configuration = require('./configuration');
 const Client = require('./client/Client');
@@ -37,6 +38,21 @@ class Detox {
     this._client = null;
     this._server = null;
     this._artifactsManager = new ArtifactsManager(artifactsConfig);
+    this._eventEmitter = new AsyncEmitter({
+      events: [
+        'bootDevice',
+        'beforeShutdownDevice',
+        'shutdownDevice',
+        'beforeTerminateApp',
+        'terminateApp',
+        'beforeUninstallApp',
+        'beforeLaunchApp',
+        'launchApp',
+        'appReady',
+        'createExternalArtifact',
+      ],
+      onError: this._onEmitError.bind(this),
+    });
 
     this.device = null;
   }
@@ -67,13 +83,15 @@ class Detox {
 
     const deviceDriver = new DeviceDriverClass({
       client: this._client,
+      emitter: this._eventEmitter,
     });
 
-    this._artifactsManager.subscribeToDeviceEvents(deviceDriver);
+    this._artifactsManager.subscribeToDeviceEvents(this._eventEmitter);
     this._artifactsManager.registerArtifactPlugins(deviceDriver.declareArtifactPlugins());
 
     const device = new Device({
       deviceConfig: this._deviceConfig,
+      emitter: this._eventEmitter,
       deviceDriver,
       sessionConfig,
     });
@@ -204,6 +222,14 @@ class Detox {
 
     return session;
   }
+
+  _onEmitError({ error, eventName, eventObj }) {
+    log.error(
+      { event: 'EMIT_ERROR', fn: eventName },
+      `Caught an exception in: emitter.emit("${eventName}", ${JSON.stringify(eventObj)})\n\n`,
+      error
+    );
+  }
 }
 
 Detox.none = new MissingDetox();
diff --git a/detox/src/artifacts/ArtifactsManager.js b/detox/src/artifacts/ArtifactsManager.js
index 8854a56ec..0028183f8 100644
--- a/detox/src/artifacts/ArtifactsManager.js
+++ b/detox/src/artifacts/ArtifactsManager.js
@@ -73,6 +73,7 @@ class ArtifactsManager {
     deviceEmitter.on('shutdownDevice', this.onShutdownDevice.bind(this));
     deviceEmitter.on('beforeLaunchApp', this.onBeforeLaunchApp.bind(this));
     deviceEmitter.on('launchApp', this.onLaunchApp.bind(this));
+    deviceEmitter.on('appReady', this.onAppReady.bind(this));
     deviceEmitter.on('beforeUninstallApp', this.onBeforeUninstallApp.bind(this));
     deviceEmitter.on('beforeTerminateApp', this.onBeforeTerminateApp.bind(this));
     deviceEmitter.on('terminateApp', this.onTerminateApp.bind(this));
@@ -111,6 +112,10 @@ class ArtifactsManager {
     await this._callPlugins('plain', 'onLaunchApp', appLaunchInfo);
   }
 
+  async onAppReady(appInfo) {
+    await this._callPlugins('plain', 'onAppReady', appInfo);
+  }
+
   async onCreateExternalArtifact({ pluginId, artifactName, artifactPath }) {
     await this._callSinglePlugin(pluginId, 'onCreateExternalArtifact', {
       artifact: new FileArtifact({ temporaryPath: artifactPath }),
diff --git a/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js b/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
index 2d9acf8e1..c35a1cc30 100644
--- a/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
+++ b/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
@@ -12,6 +12,15 @@ class ScreenshotArtifactPlugin extends TwoSnapshotsPerTestPlugin {
     return this.api.preparePathForArtifact(`${name}.png`, testSummary);
   }
 
+  async onBeforeCleanup(e) {
+    if (this.context.isAppReady === false) {
+      this._hasFailingTests = true;
+      await this._takeAutomaticSnapshot('appNotReady');
+    }
+
+    await super.onBeforeCleanup(e);
+  }
+
   /** @param {string} config */
   static parseConfig(config) {
     switch (config) {
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
index a656f3511..1bcdfb844 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
@@ -46,7 +46,11 @@ class ArtifactPlugin {
    * @param {Object} event.launchArgs - Mutable key-value pairs of args before the launch
    * @return {Promise<void>} - when done
    */
-  async onBeforeLaunchApp(event) {}
+  async onBeforeLaunchApp(event) {
+    Object.assign(this.context, {
+      isAppReady: false,
+    });
+  }
 
   /**
    * Hook that is called inside device.launchApp() and
@@ -70,6 +74,24 @@ class ArtifactPlugin {
    });
   }
 
+  /**
+   * Hook that is called after app sends ready signal
+   * over the web socket.
+   *
+   * @protected
+   * @async
+   * @param {Object} event - App ready event object
+   * @param {string} event.deviceId - Current deviceId
+   * @param {string} event.bundleId - Current bundleId
+   * @param {number} event.pid - Process id of the running app
+   * @return {Promise<void>} - when done
+   */
+  async onAppReady(event) {
+    Object.assign(this.context, {
+      isAppReady: true,
+    });
+  }
+
   /**
    * Hook that is supposed to be called from device.boot()
    *
@@ -113,6 +135,7 @@ class ArtifactPlugin {
       bundleId: '',
       launchArgs: null,
       pid: NaN,
+      isAppReady: undefined,
     });
   }
 
@@ -154,6 +177,7 @@ class ArtifactPlugin {
       bundleId: '',
       launchArgs: null,
       pid: NaN,
+      isAppReady: undefined,
     });
   }
 
diff --git a/detox/src/devices/Device.js b/detox/src/devices/Device.js
index f252bad5a..bed113e04 100644
--- a/detox/src/devices/Device.js
+++ b/detox/src/devices/Device.js
@@ -5,9 +5,10 @@ const argparse = require('../utils/argparse');
 const debug = require('../utils/debug'); //debug utils, leave here even if unused
 
 class Device {
-  constructor({ deviceConfig, deviceDriver, sessionConfig }) {
+  constructor({ deviceConfig, deviceDriver, emitter, sessionConfig }) {
     this._deviceConfig = deviceConfig;
     this._sessionConfig = sessionConfig;
+    this._emitter = emitter;
     this._processes = {};
     this.deviceDriver = deviceDriver;
     this.deviceDriver.validateDeviceConfig(deviceConfig);
@@ -86,6 +87,11 @@ class Device {
 
     await this.deviceDriver.waitUntilReady();
     await this.deviceDriver.waitForActive();
+    await this._emitter.emit('appReady', {
+      deviceId: this._deviceId,
+      bundleId: _bundleId,
+      pid: processId,
+    });
 
     if(params.detoxUserNotificationDataURL) {
       await this.deviceDriver.cleanupRandomDirectory(params.detoxUserNotificationDataURL);
diff --git a/detox/src/devices/drivers/DeviceDriverBase.js b/detox/src/devices/drivers/DeviceDriverBase.js
index db7951e4c..270e178ed 100644
--- a/detox/src/devices/drivers/DeviceDriverBase.js
+++ b/detox/src/devices/drivers/DeviceDriverBase.js
@@ -2,41 +2,19 @@ const _ = require('lodash');
 const fs = require('fs-extra');
 const os = require('os');
 const path = require('path');
-const AsyncEmitter = require('../../utils/AsyncEmitter');
 const log = require('../../utils/logger').child({ __filename });
 
 class DeviceDriverBase {
-  constructor({ client }) {
+  constructor({ client, emitter }) {
     this.client = client;
+    this.emitter = emitter;
     this.matchers = null;
-    this.emitter = new AsyncEmitter({
-      events: [
-        'bootDevice',
-        'beforeShutdownDevice',
-        'shutdownDevice',
-        'beforeTerminateApp',
-        'terminateApp',
-        'beforeUninstallApp',
-        'beforeLaunchApp',
-        'launchApp',
-        'createExternalArtifact',
-      ],
-      onError: this._onEmitError.bind(this),
-    });
   }
 
   get name() {
     return 'UNSPECIFIED_DEVICE';
   }
 
-  off(event, listener) {
-    this.emitter.off(event, listener);
-  }
-
-  on(event, listener) {
-    this.emitter.on(event, listener);
-  }
-
   declareArtifactPlugins() {
     return {};
   }
@@ -210,14 +188,6 @@ class DeviceDriverBase {
     return await Promise.resolve('');
   }
 
-  _onEmitError({ error, eventName, eventObj }) {
-    log.error(
-      { event: 'EMIT_ERROR', className: this.constructor.name, eventName },
-      `Caught an exception in: emitter.emit("${eventName}", ${JSON.stringify(eventObj)})\n\n`,
-      error
-    );
-  }
-
   async setStatusBar(deviceId, flags) {
   }
 
diff --git a/docs/Introduction.GettingStarted.md b/docs/Introduction.GettingStarted.md
index 6a1bbda72..38f7a43b9 100644
--- a/docs/Introduction.GettingStarted.md
+++ b/docs/Introduction.GettingStarted.md
@@ -12,7 +12,7 @@ Running Detox (on iOS) requires the following:
 
 * Mac with macOS (at least macOS High Sierra 10.13.6)
 
-* Xcode 10.1+ with Xcode command line tools
+* Xcode 10.2+ with Xcode command line tools
 > TIP: Verify Xcode command line tools is installed by typing `gcc -v` in terminal (shows a popup if not installed)
 
 * A working [React Native](https://facebook.github.io/react-native/docs/getting-started.html) app you want to test


Test patch:
diff --git a/detox/src/Detox.test.js b/detox/src/Detox.test.js
index e1631dd21..77bd7da87 100644
--- a/detox/src/Detox.test.js
+++ b/detox/src/Detox.test.js
@@ -236,6 +236,21 @@ describe('Detox', () => {
     }
   });
 
+  it(`should log EMIT_ERROR if the internal emitter throws an error`, async () => {
+    Detox = require('./Detox');
+    detox = new Detox({deviceConfig: validDeviceConfigWithSession});
+
+    const emitter = detox._eventEmitter;
+    emitter.on('launchApp', () => { throw new Error('TestError'); });
+    await emitter.emit('launchApp');
+
+    expect(mockLogger.error).toHaveBeenCalledWith(
+      { event: 'EMIT_ERROR', fn: 'launchApp' },
+      expect.stringMatching(/Caught an exception in.*emit.*launchApp.*undefined/),
+      expect.any(Error),
+    )
+  });
+
   describe('.artifactsManager', () => {
     let artifactsManager;
 
diff --git a/detox/src/artifacts/ArtifactsManager.test.js b/detox/src/artifacts/ArtifactsManager.test.js
index 1ad0b4555..2dd587a7d 100644
--- a/detox/src/artifacts/ArtifactsManager.test.js
+++ b/detox/src/artifacts/ArtifactsManager.test.js
@@ -86,6 +86,7 @@ describe('ArtifactsManager', () => {
           onTerminateApp: jest.fn(),
           onBeforeLaunchApp: jest.fn(),
           onLaunchApp: jest.fn(),
+          onAppReady: jest.fn(),
           onCreateExternalArtifact: jest.fn(),
           onTestStart: jest.fn(),
           onTestDone: jest.fn(),
@@ -258,6 +259,12 @@ describe('ArtifactsManager', () => {
           pid: 2018,
         }));
 
+        itShouldCatchErrorsOnPhase('onAppReady', () => ({
+          bundleId: 'testBundleId',
+          deviceId: 'testDeviceId',
+          pid: 2018,
+        }));
+
         itShouldCatchErrorsOnPhase('onBeforeTerminateApp', () => ({
           bundleId: 'testBundleId',
           deviceId: 'testDeviceId',
@@ -442,5 +449,19 @@ describe('ArtifactsManager', () => {
         expect(testPlugin.onLaunchApp).toHaveBeenCalledWith(launchInfo);
       });
     });
+
+    describe('onAppReady', () => {
+      it('should call onAppReady in plugins', async () => {
+        const appInfo = {
+          deviceId: 'testDeviceId',
+          bundleId: 'testBundleId',
+          pid: 2020,
+        };
+
+        expect(testPlugin.onAppReady).not.toHaveBeenCalled();
+        await artifactsManager.onAppReady(appInfo);
+        expect(testPlugin.onAppReady).toHaveBeenCalledWith(appInfo);
+      });
+    });
   });
 });
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
index de32cdefe..f321c6ddf 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
@@ -79,7 +79,7 @@ describe('ArtifactPlugin', () => {
       };
     });
 
-    it('should not update context on .onBeforeLaunchApp', async () => {
+    it('should set isAppReady = false on .onBeforeLaunchApp', async () => {
       await expect(plugin.onBeforeLaunchApp({
         deviceId: 'testDeviceId',
         bundleId: 'testBundleId',
@@ -90,7 +90,8 @@ describe('ArtifactPlugin', () => {
 
       expect(plugin.context).toEqual({
         deviceId: 'someOriginalDeviceId',
-        shouldNotBeDeletedFromContext: 'extraProperty'
+        shouldNotBeDeletedFromContext: 'extraProperty',
+        isAppReady: false,
       });
     });
 
@@ -115,6 +116,20 @@ describe('ArtifactPlugin', () => {
       });
     });
 
+    it('should set isAppReady = true on .onAppReady', async () => {
+      await expect(plugin.onAppReady({
+        deviceId: 'testDeviceId',
+        bundleId: 'testBundleId',
+        pid: 2020
+      }));
+
+      expect(plugin.context).toEqual({
+        deviceId: 'someOriginalDeviceId',
+        shouldNotBeDeletedFromContext: 'extraProperty',
+        isAppReady: true,
+      });
+    });
+
     it('should not update context on .onBeforeUninstallApp', async () => {
       await expect(plugin.onBeforeUninstallApp({
         deviceId: 'testDeviceId',
@@ -149,6 +164,7 @@ describe('ArtifactPlugin', () => {
         bundleId: '',
         launchArgs: null,
         pid: NaN,
+        isAppReady: undefined,
         deviceId: 'someOriginalDeviceId',
         shouldNotBeDeletedFromContext: 'extraProperty',
       });
@@ -187,6 +203,7 @@ describe('ArtifactPlugin', () => {
         bundleId: '',
         launchArgs: null,
         pid: NaN,
+        isAppReady: undefined,
         shouldNotBeDeletedFromContext: 'extraProperty',
       });
     });
diff --git a/detox/src/devices/Device.test.js b/detox/src/devices/Device.test.js
index 49d9db697..3b24fa9dd 100644
--- a/detox/src/devices/Device.test.js
+++ b/detox/src/devices/Device.test.js
@@ -10,6 +10,7 @@ describe('Device', () => {
   let fs;
   let DeviceDriverBase;
   let SimulatorDriver;
+  let emitter;
   let Device;
   let argparse;
   let Client;
@@ -33,6 +34,10 @@ describe('Device', () => {
     jest.mock('../client/Client');
     Client = require('../client/Client');
 
+    jest.mock('../utils/AsyncEmitter');
+    const AsyncEmitter = require('../utils/AsyncEmitter');
+    emitter = new AsyncEmitter({});
+
     Device = require('./Device');
   });
 
@@ -47,7 +52,10 @@ describe('Device', () => {
 
   class DeviceDriverMock {
     constructor() {
-      this.driver = new DeviceDriverBase(client);
+      this.driver = new DeviceDriverBase({
+        client,
+        emitter,
+      });
     }
 
     expectLaunchCalled(device, expectedArgs, languageAndLocale) {
@@ -86,6 +94,7 @@ describe('Device', () => {
       deviceConfig: scheme.configurations[configuration],
       deviceDriver: driverMock.driver,
       sessionConfig: scheme.session,
+      emitter,
     });
 
     device.deviceDriver.acquireFreeDevice.mockReturnValue('mockDeviceId');
@@ -166,6 +175,19 @@ describe('Device', () => {
       driverMock.expectLaunchCalled(device, expectedArgs);
     });
 
+    it(`args should launch app and emit appReady`, async () => {
+      driverMock.driver.launchApp = async () => 42;
+
+      const device = validDevice();
+      await device.launchApp();
+
+      expect(emitter.emit).toHaveBeenCalledWith('appReady', {
+        deviceId: device._deviceId,
+        bundleId: device._bundleId,
+        pid: 42,
+      })
+    });
+
     it(`(relaunch) with no args should use defaults`, async () => {
       const expectedArgs = expectedDriverArgs;
       const device = validDevice();
@@ -697,6 +719,7 @@ describe('Device', () => {
       deviceConfig: invalidDeviceNoBinary.configurations['ios.sim.release'],
       deviceDriver: new SimulatorDriver(client),
       sessionConfig: validScheme.session,
+      emitter,
     })).toThrowError(/binaryPath.* is missing/);
   });
 
diff --git a/detox/src/devices/drivers/android/AndroidDriver.test.js b/detox/src/devices/drivers/android/AndroidDriver.test.js
index b6b709279..b320134af 100644
--- a/detox/src/devices/drivers/android/AndroidDriver.test.js
+++ b/detox/src/devices/drivers/android/AndroidDriver.test.js
@@ -12,7 +12,6 @@ describe('Android driver', () => {
     }));
 
     jest.mock('./tools/ADB', () => mockADBClass);
-    jest.mock('../../../utils/AsyncEmitter', () => mockAsyncEmitter);
     jest.mock('../../../utils/sleep', () => jest.fn().mockResolvedValue(''));
     jest.mock('../../../utils/retry', () => jest.fn().mockResolvedValue(''));
 
@@ -56,6 +55,7 @@ describe('Android driver', () => {
     const AndroidDriver = require('./AndroidDriver');
     uut = new AndroidDriver({
       client,
+      emitter: new mockAsyncEmitter(),
     });
   });
 
@@ -252,4 +252,5 @@ class mockInstrumentationLogsParserClass {
     this.getStackTrace = () => mockInstrumentationLogsParserClass.INSTRUMENTATION_STACKTRACE_MOCK;
   }
 }
+
 mockInstrumentationLogsParserClass.INSTRUMENTATION_STACKTRACE_MOCK = 'Stacktrace mock';
diff --git a/detox/src/devices/drivers/ios/SimulatorDriver.test.js b/detox/src/devices/drivers/ios/SimulatorDriver.test.js
index 1b597b95f..12bcba2b5 100644
--- a/detox/src/devices/drivers/ios/SimulatorDriver.test.js
+++ b/detox/src/devices/drivers/ios/SimulatorDriver.test.js
@@ -1,11 +1,18 @@
+const AsyncEmitter = require('../../../utils/AsyncEmitter');
+
 describe('IOS simulator driver', () => {
-  let uut, sim;
+  let uut, sim, emitter;
 
   const deviceId = 'device-id-mock';
   const bundleId = 'bundle-id-mock';
 
   beforeEach(() => {
     jest.mock('./tools/AppleSimUtils', () => mockAppleSimUtils);
+
+    emitter = new AsyncEmitter({
+      events: ['beforeLaunchApp'],
+      onError: (e) => { throw e },
+    });
   });
 
   describe('launch args', () => {
@@ -20,7 +27,7 @@ describe('IOS simulator driver', () => {
       languageAndLocale = '';
 
       const SimulatorDriver = require('./SimulatorDriver');
-      uut = new SimulatorDriver({ client: {} });
+      uut = new SimulatorDriver({ client: {}, emitter });
     });
 
     it('should be passed to AppleSimUtils', async () => {
@@ -29,7 +36,7 @@ describe('IOS simulator driver', () => {
     });
 
     it('should be passed to AppleSimUtils even if some of them were received from `beforeLaunchApp` phase', async () => {
-      uut.emitter.on('beforeLaunchApp', ({ launchArgs }) => {
+      emitter.on('beforeLaunchApp', ({ launchArgs }) => {
         launchArgs.dog3 = 'Chika, from plugin';
       });
 
@@ -46,7 +53,7 @@ describe('IOS simulator driver', () => {
       languageAndLocale = '';
 
       const SimulatorDriver = require('./SimulatorDriver');
-      sim = new SimulatorDriver({ client: {} });
+      sim = new SimulatorDriver({ client: {}, emitter });
     });
 
     it('enrolls in biometrics by passing to AppleSimUtils', async () => {
@@ -85,7 +92,7 @@ describe('IOS simulator driver', () => {
 
     beforeEach(() => {
       const SimulatorDriver = require('./SimulatorDriver');
-      uut = new SimulatorDriver({ client: {} });
+      uut = new SimulatorDriver({ client: {}, emitter });
       jest.spyOn(uut.deviceRegistry, 'isDeviceBusy').mockReturnValue(false);
       applesimutils = uut.applesimutils;
       applesimutils.list.mockImplementation(async () => require('./tools/applesimutils.mock')['--list']);