Repository: elastic/synthetics (ts)

Issue:
`--screenshots` option is overridden by `--rich-events`
It seems that if using `--rich-events` in conjunction with `--screenshots` the latter option is fully ignored, meaning https://github.com/elastic/beats/pull/26455#pullrequestreview-692291735 doesn't actually have any effect. 

I propose we simply remove `--screenshots` from `--rich-events` as a simple solution.

Pull request:
feat: default to ssblocks for richevents
+ We now default to spitting out screenshot blocks whenever `--rich-events` flag is enabled which mimicks the heartbeat mode. 
+ fixes #319 - Allow screenshots option to be overridden per heartbeat monitor config. 
+ remove `-j ` flag in favor of using `--reporter`
+ `network` is now moved inside `capability` to match with other tracing, metrics and filmstrip capability. 

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()}`);
 }
 
 /**
diff --git a/src/parse_args.ts b/src/parse_args.ts
index d5d3208..f478aaa 100644
--- a/src/parse_args.ts
+++ b/src/parse_args.ts
@@ -38,7 +38,6 @@ program
     'configuration path (default: synthetics.config.js)'
   )
   .option('-s, --suite-params <jsonstring>', 'suite variables', '{}')
-  .option('-j, --json', 'output newline delimited JSON')
   .addOption(
     new Option('--reporter <value>', `output repoter format`).choices(
       Object.keys(reporters)
@@ -63,7 +62,6 @@ program
       .choices(['on', 'off', 'only-on-failure'])
       .default('on')
   )
-  .option('--network', 'capture network information for all journeys')
   .option(
     '--dry-run',
     "don't actually execute anything, report only registered journeys"
@@ -101,14 +99,21 @@ const options = command.opts() as CliArgs;
  */
 if (options.richEvents) {
   options.reporter = options.reporter ?? 'json';
-  options.screenshots = 'on';
+  options.ssblocks = true;
   options.network = true;
 }
 
 if (options.capability) {
-  const supportedCapabilities = ['trace', 'filmstrips', 'metrics', 'ssblocks'];
+  const supportedCapabilities = [
+    'trace',
+    'network',
+    'filmstrips',
+    'metrics',
+    'ssblocks',
+  ];
   /**
    * trace - record chrome trace events(LCP, FCP, CLS, etc.) for all journeys
+   * network - capture network information for all journeys
    * filmstrips - record detailed filmstrips for all journeys
    * metrics - capture performance metrics (DOM Nodes, Heap size, etc.) for each step
    * ssblocks - Dedupes the screenshots in to blocks to save storage space


Test patch:
diff --git a/__tests__/cli.test.ts b/__tests__/cli.test.ts
index 40eeba3..bc5a9ed 100644
--- a/__tests__/cli.test.ts
+++ b/__tests__/cli.test.ts
@@ -42,7 +42,11 @@ describe('CLI', () => {
   });
 
   it('produce json output via --json flag', async () => {
-    const cli = new CLIMock([join(FIXTURES_DIR, 'fake.journey.ts'), '--json']);
+    const cli = new CLIMock([
+      join(FIXTURES_DIR, 'fake.journey.ts'),
+      '--reporter',
+      'json',
+    ]);
     await cli.waitFor('fake journey');
     const output = cli.output();
     expect(JSON.parse(output).journey).toEqual({
@@ -52,17 +56,41 @@ describe('CLI', () => {
     expect(await cli.exitCode).toBe(0);
   });
 
-  it('enables rich events on `--rich-events` flag', async () => {
+  it('mimick heartbeat with `--rich-events` flag', async () => {
     const cli = new CLIMock([
       join(FIXTURES_DIR, 'fake.journey.ts'),
       '--rich-events',
     ]);
-    await cli.waitFor('fake journey');
-    const output = cli.output();
-    expect(JSON.parse(output).journey).toEqual({
-      id: 'fake journey',
-      name: 'fake journey',
+    await cli.waitFor('journey/end');
+    const screenshotRef = cli
+      .buffer()
+      .map(data => JSON.parse(data))
+      .find(({ type }) => type === 'step/screenshot_ref');
+
+    expect(screenshotRef).toMatchObject({
+      journey: {
+        id: 'fake journey',
+        name: 'fake journey',
+      },
+      root_fields: expect.any(Object),
     });
+
+    expect(await cli.exitCode).toBe(0);
+  });
+
+  it('override screenshots with `--rich-events` flag', async () => {
+    const cli = new CLIMock([
+      join(FIXTURES_DIR, 'fake.journey.ts'),
+      '--rich-events',
+      '--screenshots',
+      'off',
+    ]);
+    await cli.waitFor('journey/end');
+    const screenshots = cli
+      .buffer()
+      .map(data => JSON.parse(data))
+      .find(({ type }) => type === 'step/screenshot_ref');
+    expect(screenshots).not.toBeDefined();
     expect(await cli.exitCode).toBe(0);
   });
 
@@ -72,7 +100,8 @@ describe('CLI', () => {
     const output = async () => {
       const cli = new CLIMock([
         join(FIXTURES_DIR, 'fake.journey.ts'),
-        '--json',
+        '--reporter',
+        'json',
         '--config',
         join(FIXTURES_DIR, 'synthetics.config.ts'),
       ]);
@@ -94,7 +123,8 @@ describe('CLI', () => {
   it('suite params wins over config params', async () => {
     const cli = new CLIMock([
       join(FIXTURES_DIR, 'fake.journey.ts'),
-      '--json',
+      '--reporter',
+      'json',
       '--config',
       join(FIXTURES_DIR, 'synthetics.config.ts'),
       '-s',
@@ -111,7 +141,8 @@ describe('CLI', () => {
   it('throw error on modifying params', async () => {
     const cli = new CLIMock([
       join(FIXTURES_DIR, 'params-error.journey.ts'),
-      '-j',
+      '--reporter',
+      'json',
     ]);
     expect(await cli.exitCode).toBe(1);
     const output = cli.output();
@@ -124,7 +155,8 @@ describe('CLI', () => {
   it('support capability flag', async () => {
     const cli = new CLIMock([
       join(FIXTURES_DIR, 'example.journey.ts'),
-      '-j',
+      '--reporter',
+      'json',
       '--capability',
       'metrics',
     ]);
@@ -137,7 +169,8 @@ describe('CLI', () => {
   it('show warn for unknown capability flag', async () => {
     const cli = new CLIMock([
       join(FIXTURES_DIR, 'fake.journey.ts'),
-      '-j',
+      '--reporter',
+      'json',
       '--capability',
       'unknown',
     ]);
@@ -159,13 +192,13 @@ describe('CLI', () => {
 
 class CLIMock {
   private process: ChildProcess;
-  private data: string;
+  private data = '';
+  private chunks: Array<string> = [];
   private waitForText: string;
   private waitForPromise: () => void;
   exitCode: Promise<number>;
 
   constructor(args: string[]) {
-    this.data = '';
     this.process = spawn(
       'node',
       [join(__dirname, '..', 'dist', 'cli.js'), ...args],
@@ -176,6 +209,7 @@ class CLIMock {
     );
     const dataListener = data => {
       this.data = data.toString();
+      this.chunks.push(...this.data.split('\n').filter(Boolean));
       if (this.waitForPromise && this.data.includes(this.waitForText)) {
         this.process.stdout.off('data', dataListener);
         this.waitForPromise();
@@ -197,4 +231,8 @@ class CLIMock {
   output() {
     return this.data;
   }
+
+  buffer() {
+    return this.chunks;
+  }
 }
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 },
+      },
+    ]);
+  });
 });