Repository: elastic/synthetics
Pull request #779 (2023-06-09 18:44:26)

fix: use screenshots from synthetics.config file
+ Fixes the issue when applying the screenshot config from `monitor` field in the global `synthetics.config.ts` to all pushed monitors.
+ Added a new mode which merges all the relevant monitor config from the monitor key only during the push command instead of performing extra work during the local runner mode. 
+ `screenshot` is defaulted to `on` only when the tests are executed locally to keep the backwards compatibility as before. But during the push command, we prefer the `--screenshot` flag when its configured and use the `synthetics.config.ts`. Everything can be overriden by `monitor.use` API as before. 

Slack - https://elastic.slack.com/archives/CE4MRBU48/p1686321341637449

devcorpio: LGTM! Although I have a question with the first scenario I tested, more info below:

Manually tested:

**execute the push command without setting the screenshots CLI flag**

```
options {
  ...other options...
  screenshots: undefined,
  ...other options...
}
```

Question here @vigneshshanmugam:

If the flag value is normalized as **undefined**, in the UI we will see "on". I'm assuming this is the expected behaviour - since it's the default value for the "run" command. Is this correct? I just want to be sure this is intended


<img width="580" alt="Screenshot 2023-06-12 at 12 47 29" src="https://github.com/elastic/synthetics/assets/15065076/adcef9a4-3341-4adf-b986-f49f86ee052d">




**execute the push command setting the screenshots CLI flag** (I chose only-on-failure)

```
options {
  ...other options...
  screenshots: 'only-on-failure',
  ...other options...
}
```

**execute the push command setting the screenshot config in the synthetics.config.file instead of CLI flag** (I chose off)

```
options {
  ...other options...
  screenshots: 'off',
  ...other options...
}
```


**execute the run command without setting the screenshots CLI flag**

```
run options {
  ...other options...
  screenshots: 'on',
  ...other options...
}
```

**execute the run command setting the screenshots CLI flag** (I chose only-on-failure)

```
run options {
  ...other options...
  screenshots: 'only-on-failure',
  ...other options...
}
```


vigneshshanmugam: > If the flag value is normalized as undefined, in the UI we will see "on". I'm assuming this is the expected behaviour - since it's the default value for the "run" command. Is this correct? I just want to be sure this is intended

This is the correct behaviour as you already figured out. This is why we also have it as `on` in the screenshots for Run mode. 

Thanks for testing all scenarios :) 

Patch:
diff --git a/src/cli.ts b/src/cli.ts
index ce5ea1b..3cef19a 100644
--- a/src/cli.ts
+++ b/src/cli.ts
@@ -88,9 +88,10 @@ program
     'Enable capabilities through feature flags'
   )
   .addOption(
-    new Option('--screenshots [flag]', 'take screenshots at end of each step')
-      .choices(['on', 'off', 'only-on-failure'])
-      .default('on')
+    new Option(
+      '--screenshots [flag]',
+      'take screenshots at end of each step'
+    ).choices(['on', 'off', 'only-on-failure'])
   )
   .option(
     '--dry-run',
@@ -133,7 +134,7 @@ program
   .action(async (cliArgs: CliArgs) => {
     const tearDown = await globalSetup(cliArgs, program.args);
     try {
-      const options = normalizeOptions(cliArgs);
+      const options = normalizeOptions(cliArgs, 'run');
       const results = await run(options);
       /**
        * Exit with error status if any journey fails
@@ -204,11 +205,14 @@ program
     ]);
     try {
       const settings = await loadSettings();
-      const options = normalizeOptions({
-        ...program.opts(),
-        ...settings,
-        ...cmdOpts,
-      }) as PushOptions;
+      const options = normalizeOptions(
+        {
+          ...program.opts(),
+          ...settings,
+          ...cmdOpts,
+        },
+        'push'
+      ) as PushOptions;
       validateSettings(options);
       await catchIncorrectSettings(settings, options);
       const monitors = runner.buildMonitors(options);
diff --git a/src/common_types.ts b/src/common_types.ts
index 8b5b540..17cad07 100644
--- a/src/common_types.ts
+++ b/src/common_types.ts
@@ -206,7 +206,6 @@ type BaseArgs = {
   outfd?: number;
   wsEndpoint?: string;
   pauseOnError?: boolean;
-  ignoreHttpsErrors?: boolean;
   playwrightOptions?: PlaywrightOptions;
   quietExitCode?: boolean;
   throttling?: MonitorConfig['throttling'];
@@ -234,7 +233,6 @@ export type RunOptions = BaseArgs & {
   trace?: boolean;
   filmstrips?: boolean;
   environment?: string;
-  playwrightOptions?: PlaywrightOptions;
   networkConditions?: NetworkConditions;
   reporter?: BuiltInReporterName | ReporterInstance;
 };
diff --git a/src/core/runner.ts b/src/core/runner.ts
index 8744aeb..487b561 100644
--- a/src/core/runner.ts
+++ b/src/core/runner.ts
@@ -395,6 +395,7 @@ export default class Runner {
       privateLocations: options.privateLocations,
       params: options.params,
       playwrightOptions: options.playwrightOptions,
+      screenshot: options.screenshots,
     });
 
     const monitors: Monitor[] = [];
diff --git a/src/options.ts b/src/options.ts
index 44e4fa3..bcb9ca4 100644
--- a/src/options.ts
+++ b/src/options.ts
@@ -29,7 +29,12 @@ import { readConfig } from './config';
 import type { CliArgs, RunOptions } from './common_types';
 import { THROTTLING_WARNING_MSG, warn } from './helpers';
 
-export function normalizeOptions(cliArgs: CliArgs): RunOptions {
+type Mode = 'run' | 'push';
+
+export function normalizeOptions(
+  cliArgs: CliArgs,
+  mode: Mode = 'run'
+): RunOptions {
   const options: RunOptions = {
     ...cliArgs,
     environment: process.env['NODE_ENV'] || 'development',
@@ -107,14 +112,26 @@ export function normalizeOptions(cliArgs: CliArgs): RunOptions {
   };
 
   /**
-   * Get the default monitor config from synthetics.config.ts file
+   * Merge default options based on the mode of operation whether we are running tests locally
+   * or pushing the project monitors
    */
-  const monitor = config.monitor;
-
-  options.schedule = cliArgs.schedule ?? monitor?.schedule;
-  options.locations = cliArgs.locations ?? monitor?.locations;
-  options.privateLocations =
-    cliArgs.privateLocations ?? monitor?.privateLocations;
+  switch (mode) {
+    case 'run':
+      options.screenshots = cliArgs.screenshots ?? 'on';
+      break;
+    case 'push':
+      /**
+       * Merge the default monitor config from synthetics.config.ts file
+       * with the CLI options passed via push command
+       */
+      const monitor = config.monitor;
+      options.screenshots = options.screenshots ?? monitor?.screenshot;
+      options.schedule = options.schedule ?? monitor?.schedule;
+      options.locations = options.locations ?? monitor?.locations;
+      options.privateLocations =
+        options.privateLocations ?? monitor?.privateLocations;
+      break;
+  }
 
   return options;
 }


Test patch:
diff --git a/__tests__/fixtures/synthetics.config.ts b/__tests__/fixtures/synthetics.config.ts
index dc08c7a..2a3320b 100644
--- a/__tests__/fixtures/synthetics.config.ts
+++ b/__tests__/fixtures/synthetics.config.ts
@@ -34,6 +34,12 @@ module.exports = env => {
     playwrightOptions: {
       ...devices['Galaxy S9+'],
     },
+    monitor: {
+      screenshot: 'off',
+      schedule: 10,
+      locations: ['us_east'],
+      privateLocations: ['test-location'],
+    },
   };
   if (env !== 'development' && config.params) {
     config.params.url = 'non-dev';
diff --git a/__tests__/options.test.ts b/__tests__/options.test.ts
index 5efa57f..39c23b7 100644
--- a/__tests__/options.test.ts
+++ b/__tests__/options.test.ts
@@ -46,6 +46,7 @@ describe('options', () => {
     expect(normalizeOptions({})).toMatchObject({
       environment: 'test',
       params: {},
+      screenshots: 'on',
     });
     expect(normalizeOptions(cliArgs)).toMatchObject({
       dryRun: true,
@@ -76,13 +77,27 @@ describe('options', () => {
   });
 
   it('normalize monitor configs', () => {
+    const config = join(__dirname, 'fixtures', 'synthetics.config.ts');
+    expect(normalizeOptions({ config }, 'push')).toMatchObject({
+      screenshots: 'off',
+      schedule: 10,
+      privateLocations: ['test-location'],
+      locations: ['us_east'],
+    });
+
     expect(
-      normalizeOptions({
-        schedule: 3,
-        privateLocations: ['test'],
-        locations: ['australia_east'],
-      })
+      normalizeOptions(
+        {
+          config,
+          schedule: 3,
+          screenshots: 'only-on-failure',
+          locations: ['australia_east'],
+          privateLocations: ['test'],
+        },
+        'push'
+      )
     ).toMatchObject({
+      screenshots: 'only-on-failure',
       schedule: 3,
       privateLocations: ['test'],
       locations: ['australia_east'],