code: fixes according to the code review

detox/src/artifacts/ArtifactsManager.js

@@ -1,30 +1,29 @@
 const _ = require('lodash');
 const fs = require('fs-extra');
 const path = require('path');
+const util = require('util');
 const log = require('../utils/logger').child({ __filename });
 const argparse = require('../utils/argparse');
 const DetoxRuntimeError = require('../errors/DetoxRuntimeError');
 const ArtifactPathBuilder = require('./utils/ArtifactPathBuilder');
 
 class ArtifactsManager {
   constructor(pathBuilder) {
-    this.onBootDevice = this.onBootDevice.bind(this);
-    this.onShutdownDevice = this.onShutdownDevice.bind(this);
-    this.onBeforeLaunchApp = this.onBeforeLaunchApp.bind(this);
-    this.onLaunchApp = this.onLaunchApp.bind(this);
     this.onTerminate = _.once(this.onTerminate.bind(this));
-    this._executeIdleCallback = this._executeIdleCallback.bind(this);
 
     this._idlePromise = Promise.resolve();
-    this._onIdleCallbacks = [];
+    this._idleCallbackRequests = [];
     this._activeArtifacts = [];
     this._artifactPlugins = [];
     this._pathBuilder = pathBuilder || new ArtifactPathBuilder({
       artifactsRootDir: argparse.getArgValue('artifacts-location') || 'artifacts',
     });
-    this._callersMap = new WeakMap();
+  }
+
+  _instantitateArtifactPlugin(pluginFactory) {
+    const artifactsApi = {
+      plugin: { name: '(unknown plugin)' },
 
-    this.artifactsApi = {
       preparePathForArtifact: async (artifactName, testSummary) => {
         const artifactPath = this._pathBuilder.buildPathForTestArtifact(artifactName, testSummary);
         const artifactDir = path.dirname(artifactPath);
@@ -41,41 +40,47 @@ class ArtifactsManager {
         _.pull(this._activeArtifacts, artifact);
       },
 
-      requestIdleCallback: (callback, caller) => {
-        if (caller) {
-          this._callersMap.set(callback, caller);
-        }
-
-        this._onIdleCallbacks.push(callback);
+      requestIdleCallback: (callback) => {
+        this._idleCallbackRequests.push({
+          caller: artifactsApi.plugin,
+          callback,
+        });
 
         this._idlePromise = this._idlePromise.then(() => {
-          const nextCallback = this._onIdleCallbacks.shift();
-          return this._executeIdleCallback(nextCallback);
+          const nextCallbackRequest = this._idleCallbackRequests.shift();
+
+          if (nextCallbackRequest) {
+            return this._executeIdleCallbackRequest(nextCallbackRequest);
+          }
         });
       },
     };
+
+    const plugin = pluginFactory(artifactsApi);
+    artifactsApi.plugin = plugin;
+
+    return plugin;
   }
 
-  _executeIdleCallback(callback) {
-    if (callback) {
-      return Promise.resolve()
-        .then(callback)
-        .catch(e => this._idleCallbackErrorHandle(e, callback));
-    }
+  _executeIdleCallbackRequest({ callback, caller }) {
+    return Promise.resolve()
+      .then(callback)
+      .catch(e => this._idleCallbackErrorHandle(e, caller));
   }
 
   registerArtifactPlugins(artifactPluginFactoriesMap = {}) {
-    const api = this.artifactsApi;
     const artifactPluginsFactories = Object.values(artifactPluginFactoriesMap);
 
-    this._artifactPlugins = artifactPluginsFactories.map(factory => factory(api));
+    this._artifactPlugins = artifactPluginsFactories.map((factory) => {
+      return this._instantitateArtifactPlugin(factory);
+    });
   }
 
   subscribeToDeviceEvents(deviceEmitter) {
-    deviceEmitter.on('bootDevice', this.onBootDevice);
-    deviceEmitter.on('shutdownDevice', this.onShutdownDevice);
-    deviceEmitter.on('beforeLaunchApp', this.onBeforeLaunchApp);
-    deviceEmitter.on('launchApp', this.onLaunchApp);
+    deviceEmitter.on('bootDevice', this.onBootDevice.bind(this));
+    deviceEmitter.on('shutdownDevice', this.onShutdownDevice.bind(this));
+    deviceEmitter.on('beforeLaunchApp', this.onBeforeLaunchApp.bind(this));
+    deviceEmitter.on('launchApp', this.onLaunchApp.bind(this));
   }
 
   async onBootDevice(deviceInfo) {
@@ -119,7 +124,9 @@ class ArtifactsManager {
     log.info({ event: 'TERMINATE_START' }, 'finalizing the recorded artifacts, this can take some time...');
 
     await this._callPlugins('onTerminate');
-    await Promise.all(this._onIdleCallbacks.splice(0).map(this._executeIdleCallback));
+
+    const allCallbackRequests = this._idleCallbackRequests.splice(0);
+    await Promise.all(allCallbackRequests.map(this._executeIdleCallbackRequest.bind(this)));
     await this._idlePromise;
 
     await Promise.all(this._activeArtifacts.map(artifact => artifact.discard()));
@@ -130,7 +137,8 @@ class ArtifactsManager {
   }
 
   async _callPlugins(methodName, ...args) {
-    log.trace(Object.assign({ event: 'LIFECYCLE', fn: methodName }, ...args), `${methodName}`);
+    const callSignature = this._composeCallSignature('artifactsManager', methodName, args);
+    log.trace(Object.assign({ event: 'LIFECYCLE', fn: methodName }, ...args), callSignature);
 
     await Promise.all(this._artifactPlugins.map(async (plugin) => {
       try {
@@ -141,14 +149,24 @@ class ArtifactsManager {
     }));
   }
 
-  _unhandledPluginExceptionHandler(err, { plugin, methodName }) {
-    const eventObject = { event: 'PLUGIN_ERROR', plugin: plugin.name || 'unknown', methodName, err };
-    log.error(eventObject, `Caught exception inside plugin (${eventObject.plugin}) at phase ${methodName}`);
+  _composeCallSignature(object, methodName, args) {
+    const argsString = args.map(arg => util.inspect(arg)).join(', ');
+    return `${object}.${methodName}(${argsString})`;
   }
 
-  _idleCallbackErrorHandle(err, callback) {
-    const caller = this._callersMap.get(callback) || {};
+  _unhandledPluginExceptionHandler(err, { plugin, methodName, args }) {
+    const logObject = {
+      event: 'PLUGIN_ERROR',
+      plugin: plugin.name,
+      err,
+      methodName,
+    };
+
+    const callSignature = this._composeCallSignature(plugin.name, methodName, args);
+    log.error(logObject, `Caught exception inside function call: ${callSignature}`);
+  }
 
+  _idleCallbackErrorHandle(err, caller) {
     this._unhandledPluginExceptionHandler(err, {
       plugin: caller,
       methodName: 'onIdleCallback',
@@ -158,4 +176,4 @@ class ArtifactsManager {
 }
 
 
-module.exports = ArtifactsManager;
+module.exports = ArtifactsManager;

detox/src/artifacts/ArtifactsManager.test.js

@@ -202,16 +202,6 @@ describe('ArtifactsManager', () => {
         expect(callbacks[1]).toHaveBeenCalled();
       });
 
-      it('should gracefully handle a case when plugin object is not passed and use "unknown" as a name placeholder', async () => {
-        artifactsApi.requestIdleCallback(callbacks[0]); await sleep(0);
-        await rejects[0](new Error('test onIdleCallback error'));
-
-        expect(proxy.logger.error.mock.calls).toMatchSnapshot();
-
-        artifactsApi.requestIdleCallback(callbacks[1], testPlugin); await sleep(0);
-        expect(callbacks[1]).toHaveBeenCalled();
-      });
-
       it('should work correctly even when operations are flushed on Detox termination', async () => {
         artifactsApi.requestIdleCallback(callbacks[0], testPlugin);
         artifactsApi.requestIdleCallback(callbacks[1], testPlugin);

detox/src/artifacts/__snapshots__/ArtifactsManager.test.js.snap

@@ -9,21 +9,7 @@ Array [
       "methodName": "onIdleCallback",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onIdleCallback",
-  ],
-]
-`;
-
-exports[`ArtifactsManager .artifactsApi .requestIdleCallback() should gracefully handle a case when plugin object is not passed and use "unknown" as a name placeholder 1`] = `
-Array [
-  Array [
-    Object {
-      "err": [Error: test onIdleCallback error],
-      "event": "PLUGIN_ERROR",
-      "methodName": "onIdleCallback",
-      "plugin": "unknown",
-    },
-    "Caught exception inside plugin (unknown) at phase onIdleCallback",
+    "Caught exception inside function call: testPlugin.onIdleCallback()",
   ],
 ]
 `;
@@ -37,7 +23,7 @@ Array [
       "methodName": "onAfterAll",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onAfterAll",
+    "Caught exception inside function call: testPlugin.onAfterAll()",
   ],
 ]
 `;
@@ -51,7 +37,7 @@ Array [
       "methodName": "onAfterEach",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onAfterEach",
+    "Caught exception inside function call: testPlugin.onAfterEach({ title: 'test', fullName: 'Suite test', status: 'passed' })",
   ],
 ]
 `;
@@ -65,7 +51,7 @@ Array [
       "methodName": "onBeforeAll",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onBeforeAll",
+    "Caught exception inside function call: testPlugin.onBeforeAll()",
   ],
 ]
 `;
@@ -79,7 +65,7 @@ Array [
       "methodName": "onBeforeEach",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onBeforeEach",
+    "Caught exception inside function call: testPlugin.onBeforeEach({ title: 'test', fullName: 'Suite test', status: 'running' })",
   ],
 ]
 `;
@@ -93,7 +79,7 @@ Array [
       "methodName": "onBeforeLaunchApp",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onBeforeLaunchApp",
+    "Caught exception inside function call: testPlugin.onBeforeLaunchApp({ bundleId: 'testBundleId', deviceId: 'testDeviceId' })",
   ],
 ]
 `;
@@ -107,7 +93,7 @@ Array [
       "methodName": "onBootDevice",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onBootDevice",
+    "Caught exception inside function call: testPlugin.onBootDevice({ coldBoot: false, deviceId: 'testDeviceId' })",
   ],
 ]
 `;
@@ -121,7 +107,7 @@ Array [
       "methodName": "onLaunchApp",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onLaunchApp",
+    "Caught exception inside function call: testPlugin.onLaunchApp({ bundleId: 'testBundleId', deviceId: 'testDeviceId', pid: 2018 })",
   ],
 ]
 `;
@@ -135,7 +121,7 @@ Array [
       "methodName": "onShutdownDevice",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onShutdownDevice",
+    "Caught exception inside function call: testPlugin.onShutdownDevice({ deviceId: 'testDeviceId' })",
   ],
 ]
 `;
@@ -149,7 +135,7 @@ Array [
       "methodName": "onTerminate",
       "plugin": "testPlugin",
     },
-    "Caught exception inside plugin (testPlugin) at phase onTerminate",
+    "Caught exception inside function call: testPlugin.onTerminate()",
   ],
 ]
 `;

detox/src/artifacts/log/android/ADBLogcatPlugin.js

@@ -10,7 +10,7 @@ class ADBLogcatPlugin extends LogArtifactPlugin {
   }
 
   async onLaunchApp(event) {
-    super.onLaunchApp(event);
+    await super.onLaunchApp(event);
 
     if (this.currentRecording) {
       await this.currentRecording.start({ pid: event.pid });

detox/src/artifacts/screenshot/SimulatorScreenshotPlugin.js

@@ -13,7 +13,7 @@ class SimulatorScreenshotter extends ScreenshotArtifactPlugin {
   }
 
   async onBootDevice(event) {
-    super.onBootDevice(event);
+    await super.onBootDevice(event);
 
     if (this.enabled && event.coldBoot) {
       await this.appleSimUtils.takeScreenshot(event.deviceId, '/dev/null').catch(() => {

detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.js

@@ -102,14 +102,14 @@ class StartupAndTestRecorderPlugin extends WholeTestRecorderPlugin {
       const artifactPath = await this.preparePathForStartupArtifact();
       await startupRecording.save(artifactPath);
       this.api.untrackArtifact(startupRecording);
-    }, this);
+    });
   }
 
   _startDiscardingStartupRecording(startupRecording) {
     this.api.requestIdleCallback(async () => {
       await startupRecording.discard();
       this.api.untrackArtifact(startupRecording);
-    }, this);
+    });
   }
 }
 

detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.test.js

@@ -187,7 +187,7 @@ describe('StartupAndTestRecorderPlugin', () => {
 
         it('should schedule saving of the start-up recording', () => {
           expect(api.requestIdleCallback).toHaveBeenCalledTimes(1);
-          expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function), plugin]);
+          expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function)]);
         });
 
         it('should reset .startupRecording property to null', async () => {
@@ -269,8 +269,8 @@ describe('StartupAndTestRecorderPlugin', () => {
 
     it('should schedule two operations', () => {
       expect(api.requestIdleCallback).toHaveBeenCalledTimes(2);
-      expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function), plugin]);
-      expect(api.requestIdleCallback.mock.calls[1]).toEqual([expect.any(Function), plugin]);
+      expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function)]);
+      expect(api.requestIdleCallback.mock.calls[1]).toEqual([expect.any(Function)]);
     });
 
     it('should schedule saving of the test recording', async () => {
@@ -304,7 +304,7 @@ describe('StartupAndTestRecorderPlugin', () => {
   function itShouldScheduleDiscardingAndUntrackingOfStartupArtifact() {
     it('should schedule discarding of the start-up recording', () => {
       expect(api.requestIdleCallback).toHaveBeenCalledTimes(1);
-      expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function), plugin]);
+      expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function)]);
     });
 
     it('should reset .startupRecording property to null', async () => {

detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js

@@ -63,7 +63,7 @@ class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
       const snapshotArtifactPath = await this.preparePathForSnapshot(testSummary, index);
       await snapshot.save(snapshotArtifactPath);
       this.api.untrackArtifact(snapshot);
-    }, this);
+    });
   }
 
   _startDiscardingSnapshot(index) {
@@ -75,7 +75,7 @@ class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
     this.api.requestIdleCallback(async () => {
       await snapshot.discard();
       this.api.untrackArtifact(snapshot);
-    }, this);
+    });
   }
 
   _clearSnapshotReferences() {

detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js

@@ -98,8 +98,8 @@ describe('TwoSnapshotsPerTestPlugin', () => {
 
       it('should schedule two saving operations and specify itself as an initiator', () => {
         expect(api.requestIdleCallback).toHaveBeenCalledTimes(2);
-        expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function), plugin]);
-        expect(api.requestIdleCallback.mock.calls[1]).toEqual([expect.any(Function), plugin]);
+        expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function)]);
+        expect(api.requestIdleCallback.mock.calls[1]).toEqual([expect.any(Function)]);
       });
 
       it('should schedule to save and untrack the first artifact', async () => {
@@ -143,7 +143,7 @@ describe('TwoSnapshotsPerTestPlugin', () => {
 
       it('should schedule a discard operation for the first artifact and specify itself as an initiator', () => {
         expect(api.requestIdleCallback).toHaveBeenCalledTimes(1);
-        expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function), plugin]);
+        expect(api.requestIdleCallback.mock.calls[0]).toEqual([expect.any(Function)]);
       });
 
       it('should ultimately discard and untrack the first artifact', async () => {