refactor: after review with the stakeholder

detox/src/Detox.js

@@ -10,7 +10,7 @@ const Client = require('./client/Client');
 const DetoxServer = require('detox-server');
 const URL = require('url').URL;
 const _ = require('lodash');
-const createArtifactsManager = require('./artifacts/index');
+const ArtifactsManager = require('./artifacts/ArtifactsManager');
 
 log.level = argparse.getArgValue('loglevel') || 'info';
 log.addLevel('wss', 999, {fg: 'blue', bg: 'black'}, 'wss');
@@ -59,38 +59,13 @@ class Detox {
       global.device = this.device;
     }
 
-    this.pluginApi = this._createPluginApi({
-      deviceId: this.device._deviceId,
-      deviceClass: this.deviceConfig.type,
+    this.artifactsManager = new ArtifactsManager({
+      artifactCapabilities: deviceDriver.getArtifactCapabilities(this.device.id),
     });
 
-    this.artifactsManager = createArtifactsManager(this.pluginApi);
     await this.artifactsManager.onStart();
   }
 
-  _createPluginApi({ deviceId, deviceClass }) {
-    return Object.freeze({
-      _config: Object.freeze({
-        artifactsLocation: argparse.getArgValue('artifacts-location') || 'artifacts',
-        recordLogs: argparse.getArgValue('record-logs') || 'none',
-        takeScreenshots: argparse.getArgValue('take-screenshots') || 'none',
-        recordVideos: argparse.getArgValue('record-videos') || 'none',
-      }),
-
-      getDeviceId() {
-        return deviceId;
-      },
-
-      getDeviceClass() {
-        return deviceClass;
-      },
-
-      getConfig() {
-        return this._config;
-      },
-    });
-  }
-
   async cleanup() {
     await this.artifactsManager.onExit();
 
@@ -111,6 +86,11 @@ class Detox {
     }
   }
 
+  async shutdown() {
+    console.error('emergency detox shutdown');
+    await this.artifactsManager.onShutdown();
+  }
+
   async beforeEach(testSummary) {
     await this._handleAppCrashIfAny(testSummary.fullName);
     await this.artifactsManager.onBeforeTest(testSummary);

detox/src/artifacts/ArtifactsManager.js

@@ -1,42 +1,111 @@
 const _ = require('lodash');
-const fs = require('fs-extra');
+const argparse = require('../utils/argparse');
+
+const ArtifactPathBuilder =  require('./core/ArtifactPathBuilder');
+const RecorderLifecycle = require('./core/RecorderLifecycle');
+const SnapshotterLifecycle = require('./core/SnapshotterLifecycle');
 
 class ArtifactsManager {
-  constructor({ artifactsRootDir }) {
-    this.artifactsRootDir = artifactsRootDir;
-    this.hooks = [];
- }
+  constructor({ artifactCapabilities }) {
+    this._hooks = [];
+    this._recordings = [];
+    this._registerRecording = this._registerRecording.bind(this);
 
-  registerHooks(hooks) {
-    this._ensureMethodExists(hooks, 'onStart');
-    this._ensureMethodExists(hooks, 'onBeforeTest');
-    this._ensureMethodExists(hooks, 'onAfterTest');
-    this._ensureMethodExists(hooks, 'onExit');
+    this._pathBuilder = new ArtifactPathBuilder({
+      artifactsRootDir: argparse.getArgValue('artifacts-location') || 'artifacts',
+    });
 
-    this.hooks.push(hooks);
-    return this;
-  }
+    if (artifactCapabilities) {
+      const { log, screenshot, video } = artifactCapabilities;
 
-  _ensureMethodExists(obj, method) {
-    if (!(method in obj)) {
-      obj[method] = _.noop;
+      this._hooks = _.compact([
+        this._createLogRecorderLifecycle(log()),
+        this._createScreenshotterLifecycle(screenshot()),
+        this._createVideoRecorderLifecycle(video()),
+      ]);
     }
   }
 
   async onStart() {
-    await Promise.all(this.hooks.map(hook => hook.onStart()));
+    await Promise.all(this._hooks.map(hook => hook.onStart()));
   }
 
   async onBeforeTest(testSummary) {
-    await Promise.all(this.hooks.map(hook => hook.onBeforeTest(testSummary)));
+    await Promise.all(this._hooks.map(hook => hook.onBeforeTest(testSummary)));
   }
 
   async onAfterTest(testSummary) {
-    await Promise.all(this.hooks.map(hook => hook.onAfterTest(testSummary)));
+    await Promise.all(this._hooks.map(hook => hook.onAfterTest(testSummary)));
   }
 
   async onExit() {
-    await Promise.all(this.hooks.map(hook => hook.onExit()));
+    await Promise.all(this._hooks.map(hook => hook.onExit()));
+  }
+
+  async onShutdown() {
+    await Promise.all(this._recordings.map(r => r.stop()));
+  }
+
+  _createLogRecorderLifecycle(logRecorder) {
+    if (!logRecorder) {
+      return null;
+    }
+
+    const recordLogs = argparse.getArgValue('record-logs') || 'none';
+
+    if (recordLogs === 'none') {
+      return null;
+    }
+
+    return new RecorderLifecycle({
+      shouldRecordStartup: true,
+      keepOnlyFailedTestsRecordings: recordLogs === 'failing',
+      pathBuilder: this._pathBuilder,
+      registerRecording: this._registerRecording,
+      recorder: logRecorder,
+    });
+  }
+
+  _createScreenshotterLifecycle(screenshotter) {
+    if (!screenshotter) {
+      return null;
+    }
+
+    const takeScreenshots = argparse.getArgValue('take-screenshots') || 'none';
+
+    if (takeScreenshots === 'none') {
+      return null;
+    }
+
+    return new SnapshotterLifecycle({
+      keepOnlyFailedTestsSnapshots: takeScreenshots === 'failing',
+      pathBuilder: this._pathBuilder,
+      snapshotter: screenshotter,
+    });
+  }
+
+  _createVideoRecorderLifecycle(videoRecorder) {
+    if (!videoRecorder) {
+      return null;
+    }
+
+    const recordVideos = argparse.getArgValue('record-videos') || 'none';
+
+    if (recordVideos === 'none') {
+      return null;
+    }
+
+    return new RecorderLifecycle({
+      shouldRecordStartup: true,
+      keepOnlyFailedTestsRecordings: recordVideos === 'failing',
+      pathBuilder: this._pathBuilder,
+      registerRecording: this._registerRecording,
+      recorder: videoRecorder,
+    });
+  }
+
+  _registerRecording(recording) {
+    this._recordings.push(recording);
   }
 }
 

detox/src/artifacts/pathStrategies/NoConflictPathStrategy.js → detox/src/artifacts/core/ArtifactPathBuilder.js

@@ -2,10 +2,10 @@ const path = require('path');
 const DetoxRuntimeError = require('../../errors/DetoxRuntimeError');
 const constructSafeFilename = require('../../utils/constructSafeFilename');
 
-class NoConflictPathStrategy {
+class ArtifactPathBuilder {
   constructor({
     artifactsRootDir,
-    getUniqueSubdirectory = NoConflictPathStrategy.generateTimestampBasedSubdirectoryName
+    getUniqueSubdirectory = ArtifactPathBuilder.generateTimestampBasedSubdirectoryName
   }) {
     this._nextIndex = -1;
     this._lastTestTitle = undefined;
@@ -16,19 +16,33 @@ class NoConflictPathStrategy {
     return this._currentTestRunDir;
   }
 
-  constructPathForTestArtifact(testSummary, artifactName) {
-    const testIndexPrefix = this._getTestIndex(testSummary) + '. ';
-    const testArtifactsDirname = constructSafeFilename(testIndexPrefix, testSummary.fullName);
-    const artifactFilename = constructSafeFilename(artifactName);
+  buildPathForRunArtifact(artifactName) {
+    return path.join(
+      this._currentTestRunDir,
+      constructSafeFilename(artifactName),
+    );
+  }
 
-    const artifactPath = path.join(
+  buildPathForTestArtifact(testSummary, artifactName) {
+    const testArtifactPath = path.join(
       this._currentTestRunDir,
-      testArtifactsDirname,
-      artifactFilename
+      this._constructDirectoryNameForCurrentRunningTest(testSummary),
+      constructSafeFilename(artifactName),
     );
 
-    this._assertConstructedPathIsStillInsideArtifactsRootDir(artifactPath, testSummary);
-    return artifactPath;
+    this._assertConstructedPathIsStillInsideArtifactsRootDir(testArtifactPath, testSummary);
+    return testArtifactPath;
+  }
+
+  _constructDirectoryNameForCurrentRunningTest(testSummary) {
+    if (testSummary == null) {
+      return '';
+    }
+
+    const testIndexPrefix = this._getTestIndex(testSummary) + '. ';
+    const testArtifactsDirname = constructSafeFilename(testIndexPrefix, testSummary.fullName);
+
+    return testArtifactsDirname;
   }
 
   _assertConstructedPathIsStillInsideArtifactsRootDir(artifactPath, testSummary) {
@@ -54,6 +68,6 @@ class NoConflictPathStrategy {
   }
 }
 
-NoConflictPathStrategy.generateTimestampBasedSubdirectoryName = () => `detox_artifacts.${new Date().toISOString()}`;
+ArtifactPathBuilder.generateTimestampBasedSubdirectoryName = () => `detox_artifacts.${new Date().toISOString()}`;
 
-module.exports = NoConflictPathStrategy;
+module.exports = ArtifactPathBuilder;

detox/src/artifacts/pathStrategies/NoConflictPathStrategy.test.js → detox/src/artifacts/core/ArtifactPathBuilder.test.js

@@ -1,13 +1,13 @@
 const _ = require('lodash');
 const path = require('path');
-const NoConflictPathStrategy = require('./NoConflictPathStrategy');
+const ArtifactPathBuilder = require('./ArtifactPathBuilder');
 
-describe(NoConflictPathStrategy, () => {
+describe(ArtifactPathBuilder, () => {
   let strategy;
 
   describe('happy paths', () => {
     beforeEach(() => {
-      strategy = new NoConflictPathStrategy({
+      strategy = new ArtifactPathBuilder({
         artifactsRootDir: '/tmp'
       });
     });
@@ -16,9 +16,16 @@ describe(NoConflictPathStrategy, () => {
       expect(strategy.rootDir).toMatch(/^[\\/]tmp[\\/]detox_artifacts\.\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/);
     });
 
+    it('should provide path for unique (per test runner run) artifacts', () => {
+      const artifactPath1 = strategy.buildPathForRunArtifact('before-tests-began.log');
+      const expectedPath1 = path.join(strategy.rootDir, 'before-tests-began.log');
+
+      expect(artifactPath1).toBe(expectedPath1);
+    });
+
     it('should provide indexed and nested path for test artifact', () => {
       const test1 = {title: 'test 1', fullName: 'some test 1', status: 'running' };
-      const artifactPath1 = strategy.constructPathForTestArtifact(test1, '1.log');
+      const artifactPath1 = strategy.buildPathForTestArtifact(test1, '1.log');
       const expectedPath1 = path.join(strategy.rootDir, '0. ' + test1.fullName, '1.log');
 
       expect(artifactPath1).toBe(expectedPath1);
@@ -27,8 +34,8 @@ describe(NoConflictPathStrategy, () => {
     it('should give different indices for different tests', () => {
       const createTestSummary = (i) => ({ title: `test ${i}`, fullName: `suite - test ${i}` });
 
-      const path1 = strategy.constructPathForTestArtifact(createTestSummary(0), 'artifact');
-      const path2 = strategy.constructPathForTestArtifact(createTestSummary(1), 'artifact');
+      const path1 = strategy.buildPathForTestArtifact(createTestSummary(0), 'artifact');
+      const path2 = strategy.buildPathForTestArtifact(createTestSummary(1), 'artifact');
 
       expect(path1).not.toBe(path2);
       expect(path1).toMatch(/0\. suite - test 0[/\\]artifact$/);
@@ -39,16 +46,16 @@ describe(NoConflictPathStrategy, () => {
       const testSummary1 = { title: 'test', fullName: 'suite - test' };
       const testSummary2 = { title: 'test', fullName: 'suite - test' };
 
-      const path1 = strategy.constructPathForTestArtifact(testSummary1, 'artifact');
-      const path2 = strategy.constructPathForTestArtifact(testSummary2, 'artifact');
+      const path1 = strategy.buildPathForTestArtifact(testSummary1, 'artifact');
+      const path2 = strategy.buildPathForTestArtifact(testSummary2, 'artifact');
 
       expect(path1).toBe(path2);
     });
   });
 
   describe('edge cases', () => {
     beforeEach(() => {
-      strategy = new NoConflictPathStrategy({
+      strategy = new ArtifactPathBuilder({
         artifactsRootDir: '/tmp',
         getUniqueSubdirectory: _.constant('subdir'),
       });
@@ -57,13 +64,13 @@ describe(NoConflictPathStrategy, () => {
     it('should defend against accidental resolving outside of root directory', () => {
       const maliciousName = 'some/../../../../../../home/build-server';
 
-      expect(() => strategy.constructPathForTestArtifact({ title: '', fullName: maliciousName }, '.bashrc')).toThrowErrorMatchingSnapshot();
-      expect(() => strategy.constructPathForTestArtifact({ title: '', fullName: 'test' }, maliciousName)).toThrowErrorMatchingSnapshot();
-      expect(() => strategy.constructPathForTestArtifact({ title: '', fullName: maliciousName }, maliciousName)).toThrowErrorMatchingSnapshot();
+      expect(() => strategy.buildPathForTestArtifact({ title: '', fullName: maliciousName }, '.bashrc')).toThrowErrorMatchingSnapshot();
+      expect(() => strategy.buildPathForTestArtifact({ title: '', fullName: 'test' }, maliciousName)).toThrowErrorMatchingSnapshot();
+      expect(() => strategy.buildPathForTestArtifact({ title: '', fullName: maliciousName }, maliciousName)).toThrowErrorMatchingSnapshot();
     });
 
     it('should trim too long filenames', () => {
-      const actualPath = strategy.constructPathForTestArtifact({ title: 'test', fullName: '1'.repeat(512) }, '2'.repeat(256));
+      const actualPath = strategy.buildPathForTestArtifact({ title: 'test', fullName: '1'.repeat(512) }, '2'.repeat(256));
       const expectedPath = path.join(strategy.rootDir, '0. ' + '1'.repeat(252), '2'.repeat(255));
 
       expect(actualPath).toBe(expectedPath);