Repository: elastic/synthetics (ts)

Issue:
screenshots docs are duplicated across journeys
+ As we utilize the global cache path, on each journey end we end up writing the same docs that was already written on the previous journey resulting in N*M screenshot docs where N is the number of journeys and M is the number of steps. 

Pull request:
fix: avoid writing duplicate screenshot docs
+ fix #320 
+ We clear the global screenshot cache once each journey end. We do not need to worry about the parallel execution problem here as our screenshot path contains the process id which would be different for each worker if we end up running journeys in parallel. 
+ This feels much simpler than creating a hash for every journey where we need to pass that hash across every journey context. 
+ Also cleaned up some deprecated API and moved to promise based FS API. 

Patch:
diff --git a/src/core/runner.ts b/src/core/runner.ts
index 81d0c3b..abad2d2 100644
--- a/src/core/runner.ts
+++ b/src/core/runner.ts
@@ -25,7 +25,7 @@
 
 import { once, EventEmitter } from 'events';
 import { join } from 'path';
-import { mkdirSync, rmdirSync, writeFileSync } from 'fs';
+import { mkdir, rm, writeFile } from 'fs/promises';
 import { Journey } from '../dsl/journey';
 import { Step } from '../dsl/step';
 import { reporters, Reporter } from '../reporters';
@@ -33,8 +33,8 @@ import {
   CACHE_PATH,
   monotonicTimeInSeconds,
   getTimestamp,
-  now,
   runParallel,
+  generateUniqueId,
 } from '../helpers';
 import {
   StatusValue,
@@ -131,13 +131,18 @@ export default class Runner extends EventEmitter {
   currentJourney?: Journey = null;
   journeys: Journey[] = [];
   hooks: SuiteHooks = { beforeAll: [], afterAll: [] };
-  screenshotPath = join(CACHE_PATH, 'screenshots');
   hookError: Error | undefined;
+  static screenshotPath = join(CACHE_PATH, 'screenshots');
 
   static async createContext(options: RunOptions): Promise<JourneyContext> {
     const start = monotonicTimeInSeconds();
     const driver = await Gatherer.setupDriver(options);
     const pluginManager = await Gatherer.beginRecording(driver, options);
+    /**
+     * For each journey we create the screenshots folder for
+     * caching all screenshots and clear them at end of each journey
+     */
+    await mkdir(this.screenshotPath, { recursive: true });
     return {
       start,
       params: options.params,
@@ -160,9 +165,9 @@ export default class Runner extends EventEmitter {
      * each journey without impacting the step timing information
      */
     if (buffer) {
-      const fileName = now().toString() + '.json';
-      writeFileSync(
-        join(this.screenshotPath, fileName),
+      const fileName = `${generateUniqueId()}.json`;
+      await writeFile(
+        join(Runner.screenshotPath, fileName),
         JSON.stringify({
           step,
           data: buffer.toString('base64'),
@@ -312,6 +317,8 @@ export default class Runner extends EventEmitter {
     if (options.reporter === 'json') {
       await once(this, 'journey:end:reported');
     }
+    // clear screenshots cache after each journey
+    await rm(Runner.screenshotPath, { recursive: true, force: true });
   }
 
   /**
@@ -376,7 +383,7 @@ export default class Runner extends EventEmitter {
     return result;
   }
 
-  init(options: RunOptions) {
+  async init(options: RunOptions) {
     const { reporter, outfd } = options;
     /**
      * Set up the corresponding reporter and fallback
@@ -390,7 +397,7 @@ export default class Runner extends EventEmitter {
     /**
      * Set up the directory for caching screenshots
      */
-    mkdirSync(this.screenshotPath, { recursive: true });
+    await mkdir(CACHE_PATH, { recursive: true });
   }
 
   async run(options: RunOptions) {
@@ -428,16 +435,16 @@ export default class Runner extends EventEmitter {
       env: options.environment,
       params: options.params,
     });
-    this.reset();
+    await this.reset();
     return result;
   }
 
-  reset() {
+  async reset() {
     /**
      * Clear all cache data stored for post processing by
      * the current synthetic agent run
      */
-    rmdirSync(CACHE_PATH, { recursive: true });
+    await rm(CACHE_PATH, { recursive: true, force: true });
     this.currentJourney = null;
     this.journeys = [];
     this.active = false;
diff --git a/src/helpers.ts b/src/helpers.ts
index 35deba6..a0f7318 100644
--- a/src/helpers.ts
+++ b/src/helpers.ts
@@ -48,8 +48,12 @@ export const symbols = {
   failed: red('✖'),
 };
 
+export function generateUniqueId() {
+  return `${Date.now() + Math.floor(Math.random() * 1e13)}`;
+}
+
 export function generateTempPath() {
-  return join(os.tmpdir(), `synthetics-${process.hrtime().toString()}`);
+  return join(os.tmpdir(), `synthetics-${generateUniqueId()}`);
 }
 
 /**


Test patch:
diff --git a/__tests__/core/index.test.ts b/__tests__/core/index.test.ts
index 4afb423..4c4e751 100644
--- a/__tests__/core/index.test.ts
+++ b/__tests__/core/index.test.ts
@@ -33,7 +33,7 @@ import {
   afterAll,
 } from '../../src/core/index';
 
-beforeEach(() => runner.reset());
+beforeEach(async () => await runner.reset());
 
 const noop = () => {};
 const name = 'journey';
diff --git a/__tests__/core/runner.test.ts b/__tests__/core/runner.test.ts
index d4ba7d9..e37cb18 100644
--- a/__tests__/core/runner.test.ts
+++ b/__tests__/core/runner.test.ts
@@ -515,4 +515,49 @@ describe('runner', () => {
       fd: expect.any(Number),
     });
   });
+
+  const readAndCloseStreamJson = () => {
+    const fd = fs.openSync(dest, 'r');
+    const buffer = fs.readFileSync(fd, 'utf-8');
+    const out = [];
+    buffer.split('\n').forEach(l => {
+      try {
+        out.push(JSON.parse(l));
+      } catch (e) {
+        return; // ignore empty lines
+      }
+    });
+    return out;
+  };
+
+  it('run api - verify screenshots', async () => {
+    const j1 = new Journey({ name: 'j1' }, noop);
+    const s1 = j1.addStep('j1s1', noop);
+    const j2 = new Journey({ name: 'j2' }, noop);
+    const s2 = j2.addStep('j2s2', noop);
+    runner.addJourney(j1);
+    runner.addJourney(j2);
+
+    await runner.run({
+      wsEndpoint,
+      reporter: 'json',
+      screenshots: 'on',
+      outfd: fs.openSync(dest, 'w'),
+    });
+
+    const screenshotJson = readAndCloseStreamJson().filter(
+      ({ type }) => type === 'step/screenshot'
+    );
+    expect(screenshotJson.length).toEqual(2);
+    expect(screenshotJson).toMatchObject([
+      {
+        journey: { name: j1.name },
+        step: { name: s1.name, index: 1 },
+      },
+      {
+        journey: { name: j2.name },
+        step: { name: s2.name, index: 1 },
+      },
+    ]);
+  });
 });