Repository: wix/Detox
Pull request #1846 (2020-01-10 13:29:58)

code(artifacts): remove onBeforeAll, rename onAfterAll -> onBeforeCleanup
- [x] This is a small change 

---

**Description:**

This is the second step to implement: https://github.com/wix/Detox/issues/1661 .

This pull request returns the original meaning to `before all` and `after all` events from Detox terminology, which is `onAfterInit` and `onBeforeCleanup`, if we are honest.

However, after looking at the code, I have found out that the former `onBeforeAll` is no longer in use, apparently, after the SimulatorLogPlugin refactoring in the past year. That's why I just remove the unused code.

The new naming reflects much more closely the actual workflow in Detox artifacts manager.

The full solution of #1661 will be too voluminous for one pull request and involve way too many files for a human reviewer's eye, hence I'll be splitting it into micro-PRs like #1845 and this one.



Patch:
diff --git a/detox/src/Detox.js b/detox/src/Detox.js
index 0002700d0..5b7f4eeef 100644
--- a/detox/src/Detox.js
+++ b/detox/src/Detox.js
@@ -81,13 +81,11 @@ class Detox {
       Object.assign(global, globalsToExport);
     }
 
-    await this._artifactsManager.onBeforeAll();
-
     return this;
   }
 
   async cleanup() {
-    await this._artifactsManager.onAfterAll();
+    await this._artifactsManager.onBeforeCleanup();
 
     if (this._client) {
       this._client.dumpPendingRequests();
diff --git a/detox/src/artifacts/ArtifactsManager.js b/detox/src/artifacts/ArtifactsManager.js
index 1f8399dff..999cb8448 100644
--- a/detox/src/artifacts/ArtifactsManager.js
+++ b/detox/src/artifacts/ArtifactsManager.js
@@ -139,10 +139,6 @@ class ArtifactsManager {
     });
   }
 
-  async onBeforeAll() {
-    await this._callPlugins('ascending', 'onBeforeAll');
-  }
-
   async onTestStart(testSummary) {
     await this._callPlugins('ascending', 'onTestStart', testSummary);
   }
@@ -151,8 +147,8 @@ class ArtifactsManager {
     await this._callPlugins('descending', 'onTestDone', testSummary);
   }
 
-  async onAfterAll() {
-    await this._callPlugins('descending', 'onAfterAll');
+  async onBeforeCleanup() {
+    await this._callPlugins('descending', 'onBeforeCleanup');
     await this._idlePromise;
   }
 
diff --git a/detox/src/artifacts/log/LogArtifactPlugin.js b/detox/src/artifacts/log/LogArtifactPlugin.js
index c36c3f627..dc7bd46a9 100644
--- a/detox/src/artifacts/log/LogArtifactPlugin.js
+++ b/detox/src/artifacts/log/LogArtifactPlugin.js
@@ -11,8 +11,8 @@ class LogArtifactPlugin extends StartupAndTestRecorderPlugin {
     super({ api });
   }
 
-  async onAfterAll() {
-    await super.onAfterAll();
+  async onBeforeCleanup() {
+    await super.onBeforeCleanup();
 
     if (this.shouldKeepArtifactOfSession()) {
       this.api.requestIdleCallback(async () => {
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
index 22a91de65..ea80013d0 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.js
@@ -11,7 +11,7 @@ const log = require('../../../utils/logger').child({ __filename });
 class ArtifactPlugin {
   constructor({ api }) {
     this.api = api;
-    this.context = {};
+    this.context = { testSummary: null };
     this.enabled = api.userConfig.enabled;
     this.keepOnlyFailedTestsArtifacts = api.userConfig.keepOnlyFailedTestsArtifacts;
     this.priority = 16;
@@ -170,19 +170,6 @@ class ArtifactPlugin {
    */
   async onCreateExternalArtifact(event) {}
 
-  /**
-   * Hook that is called before any test begins
-   *
-   * @protected
-   * @async
-   * @return {Promise<void>} - when done
-   */
-  async onBeforeAll() {
-    this.context.testSummary = null;
-    this._hasFailingTests = false;
-    this._finishedTests = false;
-  }
-
   /**
    * Hook that is called before a test begins
    *
@@ -210,13 +197,13 @@ class ArtifactPlugin {
   }
 
   /**
-   * Hook that is called after all tests run
+   * Hook that is called when detox.cleanup() just begins
    *
    * @protected
    * @async
    * @return {Promise<void>} - when done
    */
-  async onAfterAll() {
+  async onBeforeCleanup() {
     this.context.testSummary = null;
     this._finishedTests = true;
     this._logDisableWarning();
@@ -241,10 +228,9 @@ class ArtifactPlugin {
     this.onBeforeLaunchApp = _.noop;
     this.onLaunchApp = _.noop;
     this.onUserAction = _.noop;
-    this.onBeforeAll = _.noop;
     this.onTestStart = _.noop;
     this.onTestDone = _.noop;
-    this.onAfterAll = _.noop;
+    this.onBeforeCleanup = _.noop;
   }
 
   _logDisableWarning() {


Test patch:
diff --git a/detox/src/Detox.test.js b/detox/src/Detox.test.js
index fac315120..556b231d1 100644
--- a/detox/src/Detox.test.js
+++ b/detox/src/Detox.test.js
@@ -198,10 +198,6 @@ describe('Detox', () => {
       artifactsManager = detox._artifactsManager; // TODO: rewrite to avoid accessing private fields
     });
 
-    it(`Calling detox.init() should trigger artifactsManager.beforeAll()`, async () => {
-      expect(artifactsManager.onBeforeAll).toHaveBeenCalledTimes(1);
-    });
-
     it(`Calling detox.beforeEach() will trigger artifacts manager .onBeforeEach`, async () => {
       const testSummary = { title: 'test', fullName: 'suite - test', status: 'running' };
       await detox.beforeEach(testSummary);
@@ -236,9 +232,9 @@ describe('Detox', () => {
       expect(artifactsManager.onTestDone).toHaveBeenCalledWith(testSummary);
     });
 
-    it(`Calling detox.cleanup() should trigger artifactsManager.afterAll()`, async () => {
+    it(`Calling detox.cleanup() should trigger artifactsManager.onBeforeCleanup()`, async () => {
       await detox.cleanup();
-      expect(artifactsManager.onAfterAll).toHaveBeenCalledTimes(1);
+      expect(artifactsManager.onBeforeCleanup).toHaveBeenCalledTimes(1);
     });
   });
 });
diff --git a/detox/src/artifacts/ArtifactsManager.test.js b/detox/src/artifacts/ArtifactsManager.test.js
index 7d3d61df7..d37cf5588 100644
--- a/detox/src/artifacts/ArtifactsManager.test.js
+++ b/detox/src/artifacts/ArtifactsManager.test.js
@@ -121,10 +121,9 @@ describe('ArtifactsManager', () => {
           onBeforeLaunchApp: jest.fn(),
           onLaunchApp: jest.fn(),
           onCreateExternalArtifact: jest.fn(),
-          onBeforeAll: jest.fn(),
           onTestStart: jest.fn(),
           onTestDone: jest.fn(),
-          onAfterAll: jest.fn(),
+          onBeforeCleanup: jest.fn(),
           onTerminate: jest.fn(),
         });
       };
@@ -282,13 +281,11 @@ describe('ArtifactsManager', () => {
           });
         }
 
-        itShouldCatchErrorsOnPhase('onBeforeAll', () => undefined);
-
         itShouldCatchErrorsOnPhase('onTestStart', () => testSummaries.running());
 
         itShouldCatchErrorsOnPhase('onTestDone', () => testSummaries.passed());
 
-        itShouldCatchErrorsOnPhase('onAfterAll', () => undefined);
+        itShouldCatchErrorsOnPhase('onBeforeCleanup', () => undefined);
 
         itShouldCatchErrorsOnPhase('onTerminate', () => undefined);
 
@@ -338,14 +335,6 @@ describe('ArtifactsManager', () => {
         }));
       });
 
-      describe('onBeforeAll', () => {
-        it('should call onBeforeAll in plugins', async () => {
-          expect(testPlugin.onBeforeAll).not.toHaveBeenCalled();
-          await artifactsManager.onBeforeAll();
-          expect(testPlugin.onBeforeAll).toHaveBeenCalled();
-        });
-      });
-
       describe('onTestStart', () => {
         it('should call onTestStart in plugins with the passed argument', async () => {
           const testSummary = testSummaries.running();
@@ -366,11 +355,11 @@ describe('ArtifactsManager', () => {
         });
       });
 
-      describe('onAfterAll', () => {
-        it('should call onAfterAll in plugins', async () => {
-          expect(testPlugin.onAfterAll).not.toHaveBeenCalled();
-          await artifactsManager.onAfterAll();
-          expect(testPlugin.onAfterAll).toHaveBeenCalled();
+      describe('onBeforeCleanup', () => {
+        it('should call onBeforeCleanup in plugins', async () => {
+          expect(testPlugin.onBeforeCleanup).not.toHaveBeenCalled();
+          await artifactsManager.onBeforeCleanup();
+          expect(testPlugin.onBeforeCleanup).toHaveBeenCalled();
         });
       });
 
diff --git a/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js b/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
index cae1576c0..58d11021b 100644
--- a/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
+++ b/detox/src/artifacts/log/ios/SimulatorLogPlugin.test.js
@@ -1,4 +1,3 @@
-jest.mock('../../../utils/argparse');
 jest.mock('../../../utils/logger');
 
 const _ = require('lodash');
@@ -9,7 +8,6 @@ const path = require('path');
 
 describe('SimulatorLogPlugin', () => {
   async function majorWorkflow() {
-    let argparse = null;
     let fakePathBuilder = null;
     let fakeSources = null;
     let fakeAppleSimUtils = null;
@@ -19,24 +17,16 @@ describe('SimulatorLogPlugin', () => {
     let createdArtifacts = null;
 
     function init() {
-      argparse = require('../../../utils/argparse');
-      argparse.getArgValue.mockImplementation((key) => {
-        switch (key) {
-          case 'record-logs': return 'all';
-          case 'loglevel': return 'trace';
-          case 'artifacts-location': return path.dirname(tempfile(''));
-          default: throw new Error(`unexpected argparse.getArgValue mock call: ${key}`);
-        }
-      });
-
       SimulatorLogPlugin = require('./SimulatorLogPlugin');
       ArtifactsManager = require('../../ArtifactsManager');
 
       createdArtifacts = [];
       fakePathBuilder = {
-        buildPathForTestArtifact: jest.fn((_, summary) => {
+        buildPathForTestArtifact: jest.fn((artifactName, summary) => {
           const artifactPath = tempfile(summary ? '.log' : '.startup.log');
-          createdArtifacts.push(artifactPath);
+          if (!artifactName.includes('detox_')) {
+            createdArtifacts.push(artifactPath);
+          }
 
           return artifactPath;
         }),
@@ -48,7 +38,7 @@ describe('SimulatorLogPlugin', () => {
 
       fakeAppleSimUtils = {
         logStream({ udid, processImagePath, level, stdout, stderr }) {
-          fs.writeFileSync(fakeSources.stdin, '');
+          // fs.writeFileSync(fakeSources.stdin, '');
 
           const handle = fs.openSync(fakeSources.stdin, 'r');
           const process = exec.spawnAndLog('cat', [], {
@@ -65,7 +55,15 @@ describe('SimulatorLogPlugin', () => {
         },
       };
 
-      artifactsManager = new ArtifactsManager({ pathBuilder: fakePathBuilder });
+      artifactsManager = new ArtifactsManager({
+        pathBuilder: fakePathBuilder,
+        plugins: {
+          log: {
+            enabled: true,
+            keepOnlyFailedTestsArtifacts: false,
+          },
+        },
+      });
       artifactsManager.registerArtifactPlugins({
         log: (api) => new SimulatorLogPlugin({
           api,
@@ -88,26 +86,23 @@ describe('SimulatorLogPlugin', () => {
     await artifactsManager.onLaunchApp({ device: 'booted', bundleId: 'com.test', pid: 8000 });
     await logToDeviceLogs('omit - after launch inside detox.init()');
 
-    await artifactsManager.onBeforeAll();
-    await logToDeviceLogs('take - inside before all');
-
     await artifactsManager.onTestStart({ title: 'test', fullName: 'some test', status: 'running'});
-    await logToDeviceLogs('take - inside before each');
-
     await logToDeviceLogs('take - before relaunch inside test');
+
     await artifactsManager.onBeforeLaunchApp({ device: 'booted', bundleId: 'com.test' });
     await logToDeviceLogs('take - during relaunch inside test');
     await artifactsManager.onLaunchApp({ device: 'booted', bundleId: 'com.test', pid: 8001 });
-    await logToDeviceLogs('take - after relaunch inside test');
 
+    await logToDeviceLogs('take - after relaunch inside test');
     await artifactsManager.onTestDone({ title: 'test', fullName: 'some test', status: 'passed'});
-    await logToDeviceLogs('omit - after afterEach');
-    await artifactsManager.onAfterAll();
-    await logToDeviceLogs('omit - after afterAll');
+
+    await logToDeviceLogs('omit - before cleanup');
+    await artifactsManager.onBeforeCleanup();
+    await logToDeviceLogs('omit - after cleanup');
 
     const result = {};
 
-    expect(fakePathBuilder.buildPathForTestArtifact).toHaveBeenCalledTimes(2);
+    expect(fakePathBuilder.buildPathForTestArtifact).toHaveBeenCalledTimes(4);
     for (const artifact of createdArtifacts) {
       const contents = (await fs.readFile(artifact, 'utf8')).trim().split('\n');
       const extension = path.basename(artifact).split('.').slice(1).join('.');
diff --git a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
index efd1a8e58..c77c0b7e4 100644
--- a/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/ArtifactPlugin.test.js
@@ -199,12 +199,6 @@ describe('ArtifactPlugin', () => {
       })).resolves.toBe(void 0);
     });
 
-    it('should have .onBeforeAll, which resets context.testSummary if called', async () => {
-      plugin.context.testSummary = {};
-      await plugin.onBeforeAll();
-      expect(plugin.context.testSummary).toBe(null);
-    });
-
     it('should have .onTestStart, which updates context.testSummary if called', async () => {
       const testSummary = testSummaries.running();
       await plugin.onTestStart(testSummary);
@@ -217,9 +211,9 @@ describe('ArtifactPlugin', () => {
       expect(plugin.context.testSummary).toBe(testSummary);
     });
 
-    it('should have .onAfterAll, which resets context.testSummary if called', async () => {
+    it('should have .onBeforeCleanup, which resets context.testSummary if called', async () => {
       plugin.context.testSummary = {};
-      await plugin.onAfterAll();
+      await plugin.onBeforeCleanup();
       expect(plugin.context.testSummary).toBe(null);
     });
 
@@ -240,10 +234,9 @@ describe('ArtifactPlugin', () => {
         expect(plugin.onLaunchApp).toBe(plugin.onTerminate);
         expect(plugin.onBeforeTerminateApp).toBe(plugin.onTerminate);
         expect(plugin.onTerminateApp).toBe(plugin.onTerminate);
-        expect(plugin.onBeforeAll).toBe(plugin.onTerminate);
         expect(plugin.onTestStart).toBe(plugin.onTerminate);
         expect(plugin.onTestDone).toBe(plugin.onTerminate);
-        expect(plugin.onAfterAll).toBe(plugin.onTerminate);
+        expect(plugin.onBeforeCleanup).toBe(plugin.onTerminate);
         expect(plugin.onUserAction).toBe(plugin.onTerminate);
       });
 
diff --git a/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.js b/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.js
index a42081326..cca9ddbca 100644
--- a/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.js
+++ b/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.js
@@ -53,14 +53,14 @@ class StartupAndTestRecorderPlugin extends WholeTestRecorderPlugin {
     }
   }
 
-  async onAfterAll() {
-    await super.onAfterAll();
+  async onBeforeCleanup() {
+    await super.onBeforeCleanup();
 
     if (this.startupRecording) {
       this._tryToFinalizeStartupRecording();
     }
 
-    await super.onAfterAll();
+    await super.onBeforeCleanup();
   }
 
   /***
diff --git a/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.test.js b/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.test.js
index 91fe8fca1..41e428dfe 100644
--- a/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/StartupAndTestRecorderPlugin.test.js
@@ -48,11 +48,11 @@ describe('StartupAndTestRecorderPlugin', () => {
       it('should end correctly, but do nothing', expectThatNothingActuallyHappens);
     });
 
-    describe('onAfterAll', () => {
+    describe('onBeforeCleanup', () => {
       beforeEach(async () => {
         await plugin.onTestStart(testSummaries.running());
         await plugin.onTestDone(testSummaries.failed());
-        await plugin.onAfterAll();
+        await plugin.onBeforeCleanup();
       });
 
       it('should end correctly, but do nothing', expectThatNothingActuallyHappens);
@@ -184,7 +184,7 @@ describe('StartupAndTestRecorderPlugin', () => {
     });
   });
 
-  describe('onAfterAll', () => {
+  describe('onBeforeCleanup', () => {
     describe('when the plugin is configured to keep all artifacts', () => {
       beforeEach(() => {
         plugin.keepOnlyFailedTestsArtifacts = false;
@@ -193,7 +193,7 @@ describe('StartupAndTestRecorderPlugin', () => {
       describe('when there were no calls to .onTestStart and .onTestDone', () => {
         beforeEach(async () => {
           await plugin.onReadyToRecord();
-          await plugin.onAfterAll();
+          await plugin.onBeforeCleanup();
         });
 
         it('should schedule saving of the start-up recording', () => {
@@ -252,7 +252,7 @@ describe('StartupAndTestRecorderPlugin', () => {
           await plugin.onReadyToRecord();
 
           api.requestIdleCallback.mockClear();
-          await plugin.onAfterAll();
+          await plugin.onBeforeCleanup();
         });
 
         itShouldScheduleDiscardingAndUntrackingOfStartupArtifact();
@@ -265,7 +265,7 @@ describe('StartupAndTestRecorderPlugin', () => {
           await plugin.onTestDone(testSummaries.passed());
 
           api.requestIdleCallback.mockClear();
-          await plugin.onAfterAll();
+          await plugin.onBeforeCleanup();
         });
 
         itShouldScheduleDiscardingAndUntrackingOfStartupArtifact();
diff --git a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
index 91aa6f803..c72754c10 100644
--- a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
+++ b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.js
@@ -37,8 +37,8 @@ class TwoSnapshotsPerTestPlugin extends ArtifactPlugin {
     this._flushSessionSnapshots();
   }
 
-  async onAfterAll() {
-    await super.onAfterAll();
+  async onBeforeCleanup() {
+    await super.onBeforeCleanup();
     this._flushSessionSnapshots();
   }
 
diff --git a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
index e02137d91..866b93a2f 100644
--- a/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
+++ b/detox/src/artifacts/templates/plugin/TwoSnapshotsPerTestPlugin.test.js
@@ -193,7 +193,7 @@ describe('TwoSnapshotsPerTestPlugin', () => {
       });
     });
 
-    describe('when an external snapshot is created before afterAll', function() {
+    describe('when an external snapshot is created before Detox cleanup', function() {
       let artifact;
 
       beforeEach(async () => {
@@ -205,7 +205,7 @@ describe('TwoSnapshotsPerTestPlugin', () => {
           artifact,
           name: 'final_name',
         });
-        await plugin.onAfterAll();
+        await plugin.onBeforeCleanup();
       });
 
       it('should be saved using the suggested name and untracked', async () => {