Repository: wix/Detox
Pull request #1885 (2020-01-30 18:36:34)

code: cleanup redundant artifacts code
- [x] This is a small change 

---

**Description:**

1. There was code in the artifacts manager that has never been in use since 2018, so I removed a good part of it.

2. `pathBuilder` in the configuration object, IMO, should be already some class instance, not a string. ArtifactsManager should not take this responsibility for resolving paths and modules. I move that to `configuration.js`.

3. Unit tests fixes.

rotemmiz: More lines deleted than added == automerge
LeoNatan: 
![puny](https://user-images.githubusercontent.com/2270433/73531678-a1f5b500-4423-11ea-8813-ab2cf813dd0e.png)


You are all puny.
noomorph: ![image](https://user-images.githubusercontent.com/1962469/73535473-4c71d600-442c-11ea-8242-a43b2000b96b.png)


Patch:
diff --git a/detox/src/artifacts/ArtifactsManager.js b/detox/src/artifacts/ArtifactsManager.js
index 999cb8448..f2ad13825 100644
--- a/detox/src/artifacts/ArtifactsManager.js
+++ b/detox/src/artifacts/ArtifactsManager.js
@@ -4,31 +4,14 @@ const path = require('path');
 const util = require('util');
 const FileArtifact = require('./templates/artifact/FileArtifact');
 const log = require('../utils/logger').child({ __filename });
-const ArtifactPathBuilder = require('./utils/ArtifactPathBuilder');
 
 class ArtifactsManager {
-  constructor({ rootDir, pathBuilder, plugins } = {}) {
-    this.onTerminate = _.once(this.onTerminate.bind(this));
-
+  constructor({ pathBuilder, plugins } = {}) {
     this._pluginConfigs = plugins;
     this._idlePromise = Promise.resolve();
     this._idleCallbackRequests = [];
-    this._activeArtifacts = [];
     this._artifactPlugins = {};
-
-    this._pathBuilder = this._instantiatePathBuilder(pathBuilder, rootDir);
-  }
-
-  _instantiatePathBuilder(pathBuilderFactory, rootDir) {
-    if (typeof pathBuilderFactory === 'function') {
-      return pathBuilderFactory({ rootDir });
-    }
-
-    if (pathBuilderFactory) {
-      return pathBuilderFactory;
-    }
-
-    return new ArtifactPathBuilder({ rootDir });
+    this._pathBuilder = pathBuilder;
   }
 
   _instantitateArtifactPlugin(pluginFactory, pluginUserConfig) {
@@ -45,13 +28,8 @@ class ArtifactsManager {
         return artifactPath;
       },
 
-      trackArtifact: (artifact) => {
-        this._activeArtifacts.push(artifact);
-      },
-
-      untrackArtifact: (artifact) => {
-        _.pull(this._activeArtifacts, artifact);
-      },
+      trackArtifact: _.noop,
+      untrackArtifact: _.noop,
 
       requestIdleCallback: (callback) => {
         this._idleCallbackRequests.push({
@@ -62,6 +40,7 @@ class ArtifactsManager {
         this._idlePromise = this._idlePromise.then(() => {
           const nextCallbackRequest = this._idleCallbackRequests.shift();
 
+          /* istanbul ignore else  */
           if (nextCallbackRequest) {
             return this._executeIdleCallbackRequest(nextCallbackRequest);
           }
@@ -152,29 +131,6 @@ class ArtifactsManager {
     await this._idlePromise;
   }
 
-  async onTerminate() {
-    if (_.isEmpty(this._artifactPlugins)) {
-      return;
-    }
-
-    log.info({ event: 'TERMINATE_START' }, 'finalizing the recorded artifacts, this can take some time...');
-
-    await this._callPlugins('plain', 'onTerminate');
-
-    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()));
-    await this._idlePromise;
-
-    for (const key of Object.keys(this._activeArtifacts)) {
-      delete this._artifactPlugins[key];
-    }
-
-    log.info({ event: 'TERMINATE_SUCCESS' }, 'done.');
-  }
-
   async _callSinglePlugin(pluginId, methodName, ...args) {
     const callSignature = this._composeCallSignature('artifactsManager', methodName, args);
     log.trace(Object.assign({ event: 'LIFECYCLE', fn: methodName }, ...args), callSignature);
@@ -252,5 +208,4 @@ class ArtifactsManager {
   }
 }
 
-
 module.exports = ArtifactsManager;
diff --git a/detox/src/configuration.js b/detox/src/configuration.js
index 8254f8772..db138a714 100644
--- a/detox/src/configuration.js
+++ b/detox/src/configuration.js
@@ -10,6 +10,7 @@ const InstrumentsArtifactPlugin = require('./artifacts/instruments/InstrumentsAr
 const LogArtifactPlugin = require('./artifacts/log/LogArtifactPlugin');
 const ScreenshotArtifactPlugin = require('./artifacts/screenshot/ScreenshotArtifactPlugin');
 const VideoArtifactPlugin = require('./artifacts/video/VideoArtifactPlugin');
+const ArtifactPathBuilder = require('./artifacts/utils/ArtifactPathBuilder');
 
 async function defaultSession() {
   return {
@@ -121,9 +122,7 @@ function composeArtifactsConfig({
     artifactsConfig.rootDir
   );
 
-  if (typeof artifactsConfig.pathBuilder === 'string') {
-    artifactsConfig.pathBuilder = resolveModuleFromPath(artifactsConfig.pathBuilder);
-  }
+  artifactsConfig.pathBuilder = resolveArtifactsPathBuilder(artifactsConfig);
 
   artifactsConfig.plugins = _.mapValues(artifactsConfig.plugins, (value, key) => {
     switch (key) {
@@ -137,6 +136,28 @@ function composeArtifactsConfig({
   return artifactsConfig;
 }
 
+function resolveArtifactsPathBuilder(artifactsConfig) {
+  let { rootDir, pathBuilder } = artifactsConfig;
+
+  if (typeof pathBuilder === 'string') {
+    pathBuilder = resolveModuleFromPath(pathBuilder);
+  }
+
+  if (typeof pathBuilder === 'function') {
+    try {
+      pathBuilder = pathBuilder({ rootDir });
+    } catch (e) {
+      pathBuilder = new pathBuilder({ rootDir });
+    }
+  }
+
+  if (!pathBuilder) {
+    pathBuilder = new ArtifactPathBuilder({ rootDir });
+  }
+
+  return pathBuilder;
+}
+
 module.exports = {
   defaultSession,
   validateSession,


Test patch:
diff --git a/detox/src/artifacts/ArtifactsManager.test.js b/detox/src/artifacts/ArtifactsManager.test.js
index d37cf5588..3e6ce780a 100644
--- a/detox/src/artifacts/ArtifactsManager.test.js
+++ b/detox/src/artifacts/ArtifactsManager.test.js
@@ -33,43 +33,6 @@ describe('ArtifactsManager', () => {
     };
   });
 
-  describe('when created', () => {
-    let artifactsManager;
-
-    beforeEach(() => {
-      artifactsManager = new proxy.ArtifactsManager({
-        rootDir: '/tmp',
-        plugins: {},
-      });
-    });
-
-    it('should provide artifacts location to path builder', async () => {
-      expect(proxy.ArtifactPathBuilder).toHaveBeenCalledWith({
-        rootDir: '/tmp',
-      });
-    });
-
-    it('should correctly terminate itself (without errors)', async () => {
-      await artifactsManager.onTerminate();
-    });
-
-    describe('with { pathBuilder } instance', () => {
-      it('should require that module as pathBuilder', () => {
-        const pathBuilder = new FakePathBuilder();
-        artifactsManager = new proxy.ArtifactsManager({ pathBuilder });
-        expect(artifactsManager._pathBuilder).toBe(pathBuilder);
-      });
-    })
-
-    describe('with { pathBuilder } function', () => {
-      it('should require that module as pathBuilder', () => {
-        const pathBuilder = jest.fn(() => new FakePathBuilder());
-        artifactsManager = new proxy.ArtifactsManager({ pathBuilder, rootDir: '/tmp/42' });
-        expect(pathBuilder).toHaveBeenCalledWith({ rootDir: '/tmp/42' });
-      });
-    })
-  });
-
   describe('when plugin factory is registered', () => {
     let artifactsManager, factory, plugin;
 
@@ -79,7 +42,9 @@ describe('ArtifactsManager', () => {
       });
 
       artifactsManager = new proxy.ArtifactsManager({
-        rootDir: '/tmp',
+        pathBuilder: new proxy.ArtifactPathBuilder({
+          rootDir: '/tmp',
+        }),
         plugins: {
           mock: { setting: 'value' },
         },
@@ -124,7 +89,6 @@ describe('ArtifactsManager', () => {
           onTestStart: jest.fn(),
           onTestDone: jest.fn(),
           onBeforeCleanup: jest.fn(),
-          onTerminate: jest.fn(),
         });
       };
 
@@ -167,34 +131,6 @@ describe('ArtifactsManager', () => {
       });
     });
 
-    describe('.trackArtifact()', () => {
-      it('should mark artifact to be discarded when Detox is being terminated', async () => {
-        const artifact = {
-          discard: jest.fn(),
-        };
-
-        artifactsApi.trackArtifact(artifact);
-        expect(artifact.discard).not.toHaveBeenCalled();
-        await artifactsManager.onTerminate();
-        expect(artifact.discard).toHaveBeenCalled();
-      });
-    });
-
-    describe('.untrackArtifact()', () => {
-      it('should mark artifact as the one that does not have to be discarded when Detox is being terminated', async () => {
-        const artifact = {
-          discard: jest.fn(),
-        };
-
-        artifactsApi.trackArtifact(artifact);
-        artifactsApi.untrackArtifact(artifact);
-
-        expect(artifact.discard).not.toHaveBeenCalled();
-        await artifactsManager.onTerminate();
-        expect(artifact.discard).not.toHaveBeenCalled();
-      });
-    });
-
     describe('.requestIdleCallback()', () => {
       let callbacks, resolves, rejects;
 
@@ -203,57 +139,55 @@ describe('ArtifactsManager', () => {
         resolves = new Array(3);
         rejects = new Array(3);
 
-        [0, 1, 2].forEach((index) => {
-          callbacks[index] = jest.fn().mockImplementation(() => {
-            return new Promise((resolve, reject) => {
-              resolves[index] = async (value) => { resolve(value); await sleep(0); };
-              rejects[index] = async (value) => { reject(value); await sleep(0); };
-            });
+        for (const index of [0, 1, 2]) {
+          const promise = new Promise((resolve, reject) => {
+            resolves[index] = resolve;
+            rejects[index] = reject;
           });
-        });
+
+          callbacks[index] = jest.fn(() => promise);
+        }
       });
 
       it('should enqueue an async operation to a queue that executes operations only one by one', async () => {
-        artifactsApi.requestIdleCallback(callbacks[0], testPlugin); await sleep(0);
-        expect(callbacks[0]).toHaveBeenCalled();
-
+        artifactsApi.requestIdleCallback(callbacks[0], testPlugin);
         artifactsApi.requestIdleCallback(callbacks[1], testPlugin);
-        artifactsApi.requestIdleCallback(callbacks[2], testPlugin);
 
-        expect(callbacks[1]).not.toHaveBeenCalled();
-        expect(callbacks[2]).not.toHaveBeenCalled();
+        expect(callbacks[0]).toHaveBeenCalledTimes(0);
+        expect(callbacks[1]).toHaveBeenCalledTimes(0);
 
-        await resolves[0]();
-        expect(callbacks[1]).toHaveBeenCalled();
-        expect(callbacks[2]).not.toHaveBeenCalled();
+        await sleep(0);
+        expect(callbacks[0]).toHaveBeenCalledTimes(1);
+        expect(callbacks[1]).toHaveBeenCalledTimes(0);
 
-        await resolves[1]();
-        expect(callbacks[2]).toHaveBeenCalled();
+        await (resolves[0](), sleep(0));
+        expect(callbacks[1]).toHaveBeenCalledTimes(1);
       });
 
       it('should catch errors, report them if callback fails, and move on ', async () => {
-        artifactsApi.requestIdleCallback(callbacks[0], testPlugin); await sleep(0);
-        await rejects[0](new Error('test onIdleCallback error'));
+        artifactsApi.requestIdleCallback(callbacks[0], testPlugin);
+        artifactsApi.requestIdleCallback(callbacks[1], testPlugin);
+        rejects[0](new Error('test onIdleCallback error'));
+        resolves[1]();
 
-        expect(proxy.logger.error.mock.calls.length).toBe(0);
+        expect(callbacks[0]).not.toHaveBeenCalled();
+        expect(callbacks[1]).not.toHaveBeenCalled();
+        expect(proxy.logger.warn.mock.calls.length).toBe(0);
+
+        await sleep(0);
 
-        artifactsApi.requestIdleCallback(callbacks[1], testPlugin); await sleep(0);
+        expect(callbacks[0]).toHaveBeenCalled();
         expect(callbacks[1]).toHaveBeenCalled();
+        expect(proxy.logger.warn.mock.calls.length).toBe(1);
       });
 
-      it('should work correctly even when operations are flushed on Detox termination', async () => {
+      it('should work correctly for onBeforeCleanup', async () => {
+        resolves[0](); resolves[1](); resolves[2]();
         artifactsApi.requestIdleCallback(callbacks[0], testPlugin);
         artifactsApi.requestIdleCallback(callbacks[1], testPlugin);
         artifactsApi.requestIdleCallback(callbacks[2], testPlugin);
-        artifactsManager.onTerminate();
-        await sleep(0);
-
-        expect(callbacks[0]).toHaveBeenCalledTimes(1);
-        expect(callbacks[1]).toHaveBeenCalledTimes(1);
-        expect(callbacks[2]).toHaveBeenCalledTimes(1);
-
-        await Promise.all(resolves.map(r => r()));
 
+        await artifactsManager.onBeforeCleanup();
         expect(callbacks[0]).toHaveBeenCalledTimes(1);
         expect(callbacks[1]).toHaveBeenCalledTimes(1);
         expect(callbacks[2]).toHaveBeenCalledTimes(1);
@@ -287,8 +221,6 @@ describe('ArtifactsManager', () => {
 
         itShouldCatchErrorsOnPhase('onBeforeCleanup', () => undefined);
 
-        itShouldCatchErrorsOnPhase('onTerminate', () => undefined);
-
         itShouldCatchErrorsOnPhase('onBootDevice', () => ({
           coldBoot: false,
           deviceId: 'testDeviceId',
@@ -363,14 +295,6 @@ describe('ArtifactsManager', () => {
         });
       });
 
-      describe('onTerminate', () => {
-        it('should call onTerminate in plugins', async () => {
-          expect(testPlugin.onTerminate).not.toHaveBeenCalled();
-          await artifactsManager.onTerminate();
-          expect(testPlugin.onTerminate).toHaveBeenCalled();
-        });
-      });
-
       describe('onBootDevice', () => {
         it('should call onBootDevice in plugins', async () => {
           const bootInfo = {
diff --git a/detox/src/configuration.test.js b/detox/src/configuration.test.js
index 39ea59cbd..d483179ba 100644
--- a/detox/src/configuration.test.js
+++ b/detox/src/configuration.test.js
@@ -4,6 +4,7 @@ const schemes = require('./configurations.mock');
 const LogArtifactPlugin = require('./artifacts/log/LogArtifactPlugin');
 const ScreenshotArtifactPlugin = require('./artifacts/screenshot/ScreenshotArtifactPlugin');
 const VideoArtifactPlugin = require('./artifacts/video/VideoArtifactPlugin');
+const ArtifactPathBuilder = require('./artifacts/utils/ArtifactPathBuilder');
 
 describe('configuration', () => {
   let configuration;
@@ -43,9 +44,10 @@ describe('configuration', () => {
         configurationName: 'abracadabra',
         deviceConfig: {},
         detoxConfig: {},
-      })).toEqual({
-        rootDir: expect.stringMatching(/^artifacts[\\\/]abracadabra\.\d{4}/),
-        pathBuilder: null,
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^artifacts[\\\/]abracadabra\.\d{4}/),
+        }),
         plugins: schemes.pluginsDefaultsResolved,
       });
     });
@@ -62,9 +64,10 @@ describe('configuration', () => {
         },
         detoxConfig: {},
         cliConfig: {}
-      })).toEqual({
-        rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
-        pathBuilder: _.noop,
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
+        }),
         plugins: schemes.pluginsAllResolved,
       });
     });
@@ -81,9 +84,10 @@ describe('configuration', () => {
           }
         },
         cliConfig: {}
-      })).toEqual({
-        rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
-        pathBuilder: _.noop,
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
+        }),
         plugins: schemes.pluginsAllResolved,
       });
     });
@@ -100,9 +104,10 @@ describe('configuration', () => {
           recordVideos: 'all',
           recordPerformance: 'all',
         }
-      })).toEqual({
-        rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
-        pathBuilder: null,
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^otherPlace[\\\/]abracadabra\.\d{4}/),
+        }),
         plugins: schemes.pluginsAllResolved,
       });
     });
@@ -131,9 +136,10 @@ describe('configuration', () => {
             },
           },
         },
-      })).toEqual({
-        rootDir: expect.stringMatching(/^cli[\\\/]priority\.\d{4}/),
-        pathBuilder: _.identity,
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^cli[\\\/]priority\.\d{4}/),
+        }),
         plugins: {
           log: schemes.pluginsFailingResolved.log,
           screenshot: schemes.pluginsAllResolved.screenshot,
@@ -144,6 +150,7 @@ describe('configuration', () => {
     });
 
     it('should resolve path builder from string (absolute path)', () => {
+      const FakePathBuilder = require('./artifacts/__mocks__/FakePathBuilder');
       expect(configuration.composeArtifactsConfig({
         configurationName: 'customization',
         deviceConfig: {
@@ -152,9 +159,7 @@ describe('configuration', () => {
           },
         },
         detoxConfig: {},
-      })).toEqual(expect.objectContaining({
-        pathBuilder: require('./artifacts/__mocks__/FakePathBuilder'),
-      }));
+      }).pathBuilder).toBeInstanceOf(FakePathBuilder);
     });
 
     it('should resolve path builder from string (relative path)', () => {
@@ -166,9 +171,12 @@ describe('configuration', () => {
           },
         },
         detoxConfig: {},
-      })).toEqual(expect.objectContaining({
-        pathBuilder: require(path.join(process.cwd(), 'package.json')),
-      }));
+      })).toMatchObject({
+        pathBuilder: expect.objectContaining({
+          "name": expect.any(String),
+          "version": expect.any(String),
+        }),
+      });
     });
 
     it('should not append configuration with timestamp if rootDir ends with slash', () => {
@@ -180,9 +188,9 @@ describe('configuration', () => {
           },
         },
         detoxConfig: {},
-      })).toEqual(expect.objectContaining({
+      })).toMatchObject({
         rootDir: '.artifacts/',
-      }));
+      });
     });
   });
 
diff --git a/detox/src/index.test.js b/detox/src/index.test.js
index e037b321c..a6dc18f9d 100644
--- a/detox/src/index.test.js
+++ b/detox/src/index.test.js
@@ -86,11 +86,12 @@ describe('index', () => {
     await detox.init(schemes.validOneDeviceNoSession);
 
     expect(Detox).toHaveBeenCalledWith({
-      artifactsConfig: {
-        ...schemes.defaultArtifactsConfiguration,
+      artifactsConfig: expect.objectContaining({
         plugins: schemes.pluginsDefaultsResolved,
-        rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.release/),
-      },
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.release/),
+        }),
+      }),
       deviceConfig: schemes.validOneDeviceNoSession.configurations['ios.sim.release'],
       session: undefined,
     });
@@ -103,11 +104,12 @@ describe('index', () => {
     await detox.init(schemes.validTwoDevicesNoSession);
 
     expect(Detox).toHaveBeenCalledWith({
-      artifactsConfig: {
-        ...schemes.defaultArtifactsConfiguration,
+      artifactsConfig: expect.objectContaining({
         plugins: schemes.pluginsDefaultsResolved,
-        rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.debug/),
-      },
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.debug/),
+        }),
+      }),
       deviceConfig: schemes.validTwoDevicesNoSession.configurations['ios.sim.debug'],
       session: undefined,
     });
@@ -138,11 +140,12 @@ describe('index', () => {
     }
 
     expect(Detox).toHaveBeenCalledWith({
-      artifactsConfig: {
-        ...schemes.defaultArtifactsConfiguration,
+      artifactsConfig: expect.objectContaining({
         plugins: schemes.pluginsDefaultsResolved,
-        rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.release/),
-      },
+        pathBuilder: expect.objectContaining({
+          rootDir: expect.stringMatching(/^artifacts[\\\/]ios\.sim\.release/),
+        }),
+      }),
       deviceConfig: expectedConfig,
       session: undefined,
     });