Repository: wix/Detox (js)

Issue:
Provide API for taking screenshots on demand
The artifacts API in its current form enables screenshots to be easily taken before and after each test.
What if the test wants to take a screenshot at some arbitrary point in the middle of the test?

A few scenarios where this would be valuable:

- Using detox to take screenshots of various screens for QA purposes. If the tests could control when screenshots are taken, they could navigate, take a screenshot, and then repeat the cycle without having to restart the test to manually trigger an afterEach screenshot capture, thus saving time and effort.
- A test encounters an unexpected state in the middle of the test, and wants to capture the intermediary screen (that the afterEach screenshot may be too late to capture)
- A test just wants to capture a screenshot of some valid intermediary state for human sanity checking purposes that is not available at the end of the test.

Perhaps the API could look something like this:
```
// detox could append ".png" and put this in the same folder as beforeEach.png and afterEach.png
await device.takeScreenshot("exampleScreenShotName");
```

Would love to see what you guys think about this :)

Pull request description:
feat: device.takeScreenshot(name)
Resolves #807

Things to complete:

- [x] fix folder issue
- [x] make sure .untrackArtifact() is called
- [x] save the remainder of screenshots between `before` and `beforeEach`, `afterEach` and `afterAll`
- [x] documentation


Patch:
diff --git a/detox/src/artifacts/ArtifactsManager.js b/detox/src/artifacts/ArtifactsManager.js
index 25d39c331..5e948d29b 100644
--- a/detox/src/artifacts/ArtifactsManager.js
+++ b/detox/src/artifacts/ArtifactsManager.js
@@ -81,8 +81,9 @@ 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('beforeTerminateApp', this.onBeforeTerminateApp.bind(this));
     deviceEmitter.on('beforeUninstallApp', this.onBeforeUninstallApp.bind(this));
+    deviceEmitter.on('beforeTerminateApp', this.onBeforeTerminateApp.bind(this));
+    deviceEmitter.on('userAction', this.onUserAction.bind(this));
   }
 
   async onBootDevice(deviceInfo) {
@@ -113,6 +114,10 @@ class ArtifactsManager {
     await this._callPlugins('onLaunchApp', appLaunchInfo);
   }
 
+  async onUserAction(actionInfo) {
+    await this._callPlugins('onUserAction', actionInfo);
+  }
+
   async onBeforeAll() {
     await this._callPlugins('onBeforeAll');
   }
diff --git a/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js b/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
index 9364f9d76..f5db06c9b 100644
--- a/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
+++ b/detox/src/artifacts/screenshot/ScreenshotArtifactPlugin.js
@@ -1,4 +1,6 @@
+const _ = require('lodash');
 const argparse = require('../../utils/argparse');
+const log = require('../../utils/logger').child({ __filename });
 const TwoSnapshotsPerTestPlugin = require('../templates/plugin/TwoSnapshotsPerTestPlugin');
 
 /***
@@ -12,12 +14,50 @@ class ScreenshotArtifactPlugin extends TwoSnapshotsPerTestPlugin {
 
     this.enabled = takeScreenshots && takeScreenshots !== 'none';
     this.keepOnlyFailedTestsArtifacts = takeScreenshots === 'failing';
+    this._warnAboutNotImplementedMode = _.once(this._warnAboutNotImplementedMode.bind(this));
   }
 
-  async preparePathForSnapshot(testSummary, index) {
-    const pngName = index === 0 ? 'beforeEach.png' : 'afterEach.png';
-    return this.api.preparePathForArtifact(pngName, testSummary);
+  async onBeforeEach(testSummary) {
+    this.context.testSummary = null;
+    await this._flushScreenshots();
+
+    await super.onBeforeEach(testSummary);
+  }
+
+  async onAfterEach(testSummary) {
+    await super.onAfterEach(testSummary);
+    await this._flushScreenshots();
+  }
+
+  async onAfterAll() {
+    await super.onAfterAll();
+    await this._flushScreenshots();
+  }
+
+  async preparePathForSnapshot(testSummary, name) {
+    return this.api.preparePathForArtifact(`${name}.png`, testSummary);
+  }
+
+  async onUserAction({ type, options }) {
+    switch (type) {
+      case 'takeScreenshot':
+        return this.keepOnlyFailedTestsArtifacts
+          ? this._warnAboutNotImplementedMode()
+          : this.takeSnapshot(options.name);
+    }
+  }
+
+  _warnAboutNotImplementedMode() {
+    const message = 'Taking screenshots on-demand is not supported yet for `--take-screenshots failing` mode.';
+    log.warn({ event: 'TAKE_SCREENSHOT_IGNORED' }, message);
+  }
+
+  async _flushScreenshots() {
+    for (const name of Object.keys(this.snapshots)) {
+      this.startSavingSnapshot(name);
+      delete this.snapshots[name];
+    }
   }
 }
 
-module.exports = ScreenshotArtifactPlugin;
\ No newline at end of file
+module.exports = ScreenshotArtifactPlugin;
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
index 3cb8e778b..a82dd9cb9 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
@@ -159,6 +159,18 @@ class ArtifactPlugin {
     });
   }
 
+  /**
+   * Hook that is called on demand by some of user actions (e.g. device.takeScreeenshot())
+   *
+   * @protected
+   * @async
+   * @param {Object} event - User action event object
+   * @param {string} event.type - Action type
+   * @param {string} event.options - Action options
+   * @return {Promise<any>} - appropriate result of the action
+   */
+  async onUserAction(event) {}
+
   /**
    * Hook that is called before any test begins
    *
@@ -218,9 +230,10 @@ class ArtifactPlugin {
     this.onBootDevice = _.noop;
     this.onBeforeShutdownDevice = _.noop;
     this.onShutdownDevice = _.noop;
+    this.onBeforeTerminateApp = _.noop;
     this.onBeforeLaunchApp = _.noop;
     this.onLaunchApp = _.noop;
-    this.onBeforeTerminateApp = _.noop;
+    this.onUserAction = _.noop;
     this.onBeforeAll = _.noop;
     this.onBeforeEach = _.noop;
     this.onAfterEach = _.noop;
diff --git a/detox/src/devices/Device.js b/detox/src/devices/Device.js
index 1723d246b..7805cd5eb 100644
--- a/detox/src/devices/Device.js
+++ b/detox/src/devices/Device.js
@@ -96,6 +96,14 @@ class Device {
     }
   }
 
+  async takeScreenshot(name) {
+    if (!name) {
+      throw new Error('Cannot take a screenshot with an empty name.');
+    }
+
+    return this.deviceDriver.takeScreenshot(name);
+  }
+
   _isAppInBackground(params, _bundleId) {
     return !params.delete && !params.newInstance && this._processes[_bundleId];
   }
diff --git a/detox/src/devices/drivers/DeviceDriverBase.js b/detox/src/devices/drivers/DeviceDriverBase.js
index 34ba02888..d72bc2e5a 100644
--- a/detox/src/devices/drivers/DeviceDriverBase.js
+++ b/detox/src/devices/drivers/DeviceDriverBase.js
@@ -17,6 +17,7 @@ class DeviceDriverBase {
         'beforeUninstallApp',
         'beforeLaunchApp',
         'launchApp',
+        'userAction',
       ],
       onError: this._onEmitError.bind(this),
     });
@@ -50,6 +51,13 @@ class DeviceDriverBase {
     return await Promise.resolve('');
   }
 
+  async takeScreenshot(name) {
+    await this.emitter.emit('userAction', {
+      type: 'takeScreenshot',
+      options: { name },
+    });
+  }
+
   async sendToHome() {
     return await Promise.resolve('');
   }
diff --git a/docs/APIRef.DeviceObjectAPI.md b/docs/APIRef.DeviceObjectAPI.md
index 469fafd97..c4a265b64 100644
--- a/docs/APIRef.DeviceObjectAPI.md
+++ b/docs/APIRef.DeviceObjectAPI.md
@@ -24,6 +24,7 @@ title: The `device` Object
 - [`device.disableSynchronization()`](#devicedisablesynchronization)
 - [`device.resetContentAndSettings()`](#deviceresetcontentandsettings)
 - [`device.getPlatform()`](#devicegetplatform)
+- [`device.takeScreenshot(name)`](#devicetakescreenshotname)
 - [`device.pressBack()` **Android Only**](#devicepressback-android-only)
 - [`device.shake()` **iOS Only**](#deviceshake-ios-only)
 
@@ -308,6 +309,26 @@ if (device.getPlatform() === 'ios') {
 }
 ```
 
+### `device.takeScreenshot(name)`
+Takes a screenshot on the device and schedules putting it to
+the [artifacts folder](APIRef.Artifacts.md#enabling-artifacts) upon
+completion of the current test. Consider the example below:
+
+```js
+describe('Menu items', () => {
+  it('should have Logout', async () => {
+    // ...
+    await device.takeScreenshot('tap on menu');
+    // ...
+  });
+});
+```
+
+* If the test passes, the screenshot will be put to `<artifacts-location>/✓ Menu items should have Logout/tap on menu.png`.
+* If the test fails, the screenshot will be put to `<artifacts-location>/✗ Menu items should have Logout/tap on menu.png`.
+
+> NOTE: at the moment, taking screenshots on-demand is supported **only** in `--take-screenshots all` mode.
+
 ### `device.pressBack()` **Android Only**
 Simulate press back button.
 


Test patch:
diff --git a/detox/src/artifacts/ArtifactsManager.test.js b/detox/src/artifacts/ArtifactsManager.test.js
index e13e50fd9..6f3bffeb9 100644
--- a/detox/src/artifacts/ArtifactsManager.test.js
+++ b/detox/src/artifacts/ArtifactsManager.test.js
@@ -91,9 +91,10 @@ describe('ArtifactsManager', () => {
           onBeforeShutdownDevice: jest.fn(),
           onShutdownDevice: jest.fn(),
           onBeforeUninstallApp: jest.fn(),
+          onBeforeTerminateApp: jest.fn(),
           onBeforeLaunchApp: jest.fn(),
           onLaunchApp: jest.fn(),
-          onBeforeTerminateApp: jest.fn(),
+          onUserAction: jest.fn(),
           onBeforeAll: jest.fn(),
           onBeforeEach: jest.fn(),
           onAfterEach: jest.fn(),
@@ -252,6 +253,13 @@ describe('ArtifactsManager', () => {
           deviceId: 'testDeviceId',
         }));
 
+        itShouldCatchErrorsOnPhase('onUserAction', () => ({
+          type: 'takeScreenshot',
+          options: {
+            name: 'open app',
+          }
+        }));
+
         itShouldCatchErrorsOnPhase('onBeforeShutdownDevice', () => ({
           deviceId: 'testDeviceId'
         }));
@@ -390,6 +398,20 @@ describe('ArtifactsManager', () => {
       });
     });
 
+    describe('onUserAction', () => {
+      it('should call onUserAction in plugins', async () => {
+        const actionInfo = {
+          type: 'takeScreenshot',
+          options: {
+            name: 'open app',
+          },
+        };
+
+        await artifactsManager.onUserAction(actionInfo);
+        expect(testPlugin.onUserAction).toHaveBeenCalledWith(actionInfo);
+      });
+    });
+
     describe('onBeforeLaunchApp', () => {
       it('should call onBeforeLaunchApp in plugins', async () => {
         const launchInfo = {
diff --git a/detox/src/artifacts/__snapshots__/ArtifactsManager.test.js.snap b/detox/src/artifacts/__snapshots__/ArtifactsManager.test.js.snap
index dd9f1ac1e..e94338271 100644
--- a/detox/src/artifacts/__snapshots__/ArtifactsManager.test.js.snap
+++ b/detox/src/artifacts/__snapshots__/ArtifactsManager.test.js.snap
@@ -169,3 +169,17 @@ Array [
   ],
 ]
 `;
+
+exports[`ArtifactsManager .artifactsApi hooks error handling should catch .onUserAction errors 1`] = `
+Array [
+  Array [
+    Object {
+      "err": [Error: test onUserAction error],
+      "event": "SUPPRESS_PLUGIN_ERROR",
+      "methodName": "onUserAction",
+      "plugin": "testPlugin",
+    },
+    "Suppressed error inside function call: testPlugin.onUserAction({ type: 'takeScreenshot', options: { name: 'open app' } })",
+  ],
+]
+`;
diff --git a/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js b/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
index 68e59fa08..c6c3d79b0 100644
--- a/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
+++ b/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
@@ -2,9 +2,11 @@ jest.mock('../../../utils/argparse');
 jest.mock('../../../utils/logger');
 
 const _ = require('lodash');
+const os = require('os');
 const tempfile = require('tempfile');
 const fs = require('fs-extra');
 const path = require('path');
+const sleep = require('../../../utils/sleep');
 
 describe('SimulatorLogPlugin', () => {
   async function majorWorkflow() {
@@ -88,6 +90,10 @@ describe('SimulatorLogPlugin', () => {
     await artifactsManager.onBeforeAll();
     await logToDeviceLogs('inside before all');
 
+    if (os.platform() !== 'darwin') {
+      await sleep(1000); // HACK: till we replace `tail` with something less flaky
+    }
+
     await artifactsManager.onBeforeEach({ title: 'test', fullName: 'some test', status: 'running'});
     await logToDeviceLogs('inside before each');
 
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
index dcf04b7d2..0fc47ee46 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
@@ -140,6 +140,10 @@ describe(ArtifactPlugin, () => {
       expect(plugin.context).toMatchSnapshot();
     });
 
+    it('should have .onUserAction', async () => {
+      await expect(plugin.onUserAction()).resolves.toBe(void 0);
+    });
+
     it('should have .onBeforeAll, which resets context.testSummary if called', async () => {
       plugin.context.testSummary = {};
       await plugin.onBeforeAll();
@@ -184,6 +188,7 @@ describe(ArtifactPlugin, () => {
         expect(plugin.onBeforeEach).toBe(plugin.onTerminate);
         expect(plugin.onAfterEach).toBe(plugin.onTerminate);
         expect(plugin.onAfterAll).toBe(plugin.onTerminate);
+        expect(plugin.onUserAction).toBe(plugin.onTerminate);
       });
 
       it('should not work after the first call', async () => {
diff --git a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
index bc57a10c3..c6e46c622 100644
--- a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
+++ b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
@@ -6,30 +6,33 @@ const ArtifactPlugin = require('./ArtifactPlugin');
 class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
   constructor({ api }) {
     super({ api });
-    this._snapshots = [null, null];
+    this.snapshots = {};
   }
 
-  async onBeforeEach() {
-    await this._takeSnapshot(0);
+  async onBeforeEach(testSummary) {
+    await super.onBeforeEach(testSummary);
+    await this.takeSnapshot('beforeEach');
   }
 
   async onAfterEach(testSummary) {
+    await super.onAfterEach(testSummary);
+
     if (this.shouldKeepArtifactOfTest(testSummary)) {
-      await this._takeSnapshot(1);
-      this._startSavingSnapshot(testSummary, 0);
-      this._startSavingSnapshot(testSummary, 1);
+      await this.takeSnapshot('afterEach');
+      this.startSavingSnapshot('beforeEach');
+      this.startSavingSnapshot('afterEach');
     } else {
-      this._startDiscardingSnapshot(0);
+      this.startDiscardingSnapshot('beforeEach');
     }
 
-    this._clearSnapshotReferences();
+    this._clearAutomaticSnapshotReferences();
   }
 
   /***
    * @protected
    * @abstract
    */
-  async preparePathForSnapshot(testSummary, index) {}
+  async preparePathForSnapshot(testSummary, snapshotName) {}
 
 
   /***
@@ -41,33 +44,48 @@ class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
    */
   createTestArtifact() {}
 
-  async _takeSnapshot(index) {
+  _clearAutomaticSnapshotReferences() {
+    delete this.snapshots.beforeEach;
+    delete this.snapshots.afterEach;
+  }
+
+  /***
+   * @protected
+   */
+  async takeSnapshot(name) {
     if (!this.enabled) {
       return;
     }
 
-    const snapshot = this.createTestArtifact();
+    const snapshot = this.snapshots[name] = this.createTestArtifact();
     await snapshot.start();
     await snapshot.stop();
-    this._snapshots[index] = snapshot;
+
     this.api.trackArtifact(snapshot);
   }
 
-  _startSavingSnapshot(testSummary, index) {
-    const snapshot = this._snapshots[index];
+  /***
+   * @protected
+   */
+  startSavingSnapshot(name) {
+    const snapshot = this.snapshots[name];
     if (!snapshot) {
       return;
     }
 
+    const {testSummary} = this.context;
     this.api.requestIdleCallback(async () => {
-      const snapshotArtifactPath = await this.preparePathForSnapshot(testSummary, index);
+      const snapshotArtifactPath = await this.preparePathForSnapshot(testSummary, name);
       await snapshot.save(snapshotArtifactPath);
       this.api.untrackArtifact(snapshot);
     });
   }
 
-  _startDiscardingSnapshot(index) {
-    const snapshot = this._snapshots[index];
+  /***
+   * @protected
+   */
+  startDiscardingSnapshot(name) {
+    const snapshot = this.snapshots[name];
     if (!snapshot) {
       return;
     }
@@ -77,11 +95,6 @@ class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
       this.api.untrackArtifact(snapshot);
     });
   }
-
-  _clearSnapshotReferences() {
-    this._snapshots[0] = null;
-    this._snapshots[1] = null;
-  }
 }
 
 module.exports = TwoSnapshotsPerTestPlugin;
diff --git a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
index 240cec8f2..60644de48 100644
--- a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
@@ -110,7 +110,7 @@ describe('TwoSnapshotsPerTestPlugin', () => {
 
         await saveRequest();
 
-        expect(plugin.createdArtifacts[0].save).toBeCalledWith('test/0.png');
+        expect(plugin.createdArtifacts[0].save).toBeCalledWith('test/beforeEach.png');
         expect(api.untrackArtifact).toBeCalledWith(plugin.createdArtifacts[0]);
       });
 
@@ -122,7 +122,7 @@ describe('TwoSnapshotsPerTestPlugin', () => {
 
         await saveRequest();
 
-        expect(plugin.createdArtifacts[1].save).toBeCalledWith('test/1.png');
+        expect(plugin.createdArtifacts[1].save).toBeCalledWith('test/afterEach.png');
         expect(api.untrackArtifact).toBeCalledWith(plugin.createdArtifacts[1]);
       });
     });
diff --git a/detox/src/devices/Device.test.js b/detox/src/devices/Device.test.js
index ee58546e1..89dae6021 100644
--- a/detox/src/devices/Device.test.js
+++ b/detox/src/devices/Device.test.js
@@ -628,6 +628,17 @@ describe('Device', () => {
     expect(driverMock.driver.pressBack).toHaveBeenCalledWith(device._deviceId);
   });
 
+  it('takeScreenshot(name) should throw an exception if given name is empty', async () => {
+    await expect(validDevice().takeScreenshot()).rejects.toThrowErrorMatchingSnapshot();
+  });
+
+  it('takeScreenshot(name) should delegate the work to the driver', async () => {
+    device = validDevice();
+
+    await device.takeScreenshot('name');
+    expect(device.deviceDriver.takeScreenshot).toHaveBeenCalledWith('name');
+  });
+
   async function launchAndTestBinaryPath(configuration) {
     const device = schemeDevice(configurationsMock.pathsTests, configuration);
 
diff --git a/detox/src/devices/__snapshots__/Device.test.js.snap b/detox/src/devices/__snapshots__/Device.test.js.snap
index 8eefb59d0..a62d1ce55 100644
--- a/detox/src/devices/__snapshots__/Device.test.js.snap
+++ b/detox/src/devices/__snapshots__/Device.test.js.snap
@@ -3,3 +3,5 @@
 exports[`Device new Device() with invalid device config (no binary) should throw 1`] = `"'binaryPath' property is missing, should hold the app binary path"`;
 
 exports[`Device new Device() with invalid device config (no device name) should throw 1`] = `"'name' property is missing, should hold the device name to run on (e.g. \\"iPhone 7\\", \\"iPhone 7, iOS 10.2\\""`;
+
+exports[`Device takeScreenshot(name) should throw an exception if given name is empty 1`] = `"Cannot take a screenshot with an empty name."`;
diff --git a/detox/test/e2e/21.artifacts.test.js b/detox/test/e2e/21.artifacts.test.js
new file mode 100644
index 000000000..766328d6f
--- /dev/null
+++ b/detox/test/e2e/21.artifacts.test.js
@@ -0,0 +1,38 @@
+describe('Artifacts', () => {
+  before(async () => {
+    await device.sendToHome();
+    await device.takeScreenshot('Artifacts/before all');
+    await device.launchApp();
+  });
+
+  beforeEach(async () => {
+    // implicitly taking screenshot - beforeEach.png
+    await device.reloadReactNative();
+    await device.takeScreenshot('in main menu');
+
+    await element(by.text('Actions')).tap();
+    await device.takeScreenshot('Actions');
+  });
+
+  it('should take screenshots inside test', async () => {
+    await element(by.id('UniqueId819')).tap();
+    await device.takeScreenshot('taps - 1');
+
+    await element(by.id('UniqueId819')).tap();
+    await device.takeScreenshot('taps - 2');
+  });
+
+  afterEach(async () => {
+    await element(by.text('Tap Me')).tap();
+    await device.takeScreenshot('tap working');
+
+    await device.reloadReactNative();
+    // implicitly taking screenshot - afterEach.png
+  });
+
+  after(async () => {
+    await device.sendToHome();
+    await device.takeScreenshot('Artifacts/after all');
+    await device.launchApp();
+  });
+});