✨ Add core dry-run ability (#575)

* ✅ Add soft reset to logger test helpers

* ✨ Rename queue length property to size

* ♻ Refactor snapshot method and config into a dedicated module

* 🔊 Use consistent nesting character for discovery logs

* ✨ Add core dry-run ability

* ✨ Add common dry-run flag

* ♻ Rely on core dry-run for the cli-snapshot command

* 🏷 Add type defs for core config property and method

* 👷 Increment CI cache-key

* 📝 Update dry-run flag description

.github/.cache-key

@@ -1 +1 @@
-Times we have broken CI: 1
+Times we have broken CI: 2

packages/cli-command/src/command.js

@@ -81,6 +81,7 @@ export default class PercyCommand extends Command {
     // set config: false to prevent core from reloading config
     return Object.assign(config, {
       skipUploads: this.flags.debug,
+      dryRun: this.flags['dry-run'],
       config: false
     });
   }

packages/cli-command/src/flags.js

@@ -35,6 +35,10 @@ const discovery = {
     description: 'disable asset discovery caches',
     percyrc: 'discovery.disableCache'
   }),
+  'dry-run': flags.boolean({
+    char: 'd',
+    description: 'print logs only, do not run asset discovery or upload snapshots'
+  }),
   debug: flags.boolean({
     description: 'debug asset discovery and do not upload snapshots'
   })

packages/cli-command/test/command.test.js

@@ -127,6 +127,7 @@ describe('PercyCommand', () => {
         '--allowed-hostname', '*.percy.io',
         '--network-idle-timeout', '150',
         '--disable-cache',
+        '--dry-run',
         '--debug',
         'foo', 'bar'
       ])).toBeResolved();
@@ -135,6 +136,7 @@ describe('PercyCommand', () => {
         version: 2,
         config: false,
         skipUploads: true,
+        dryRun: true,
         snapshot: {
           widths: [375, 1280],
           minHeight: 1024,

packages/cli-snapshot/src/commands/snapshot.js

@@ -41,10 +41,6 @@ export class Snapshot extends Command {
       description: 'one or more globs/patterns matching snapshots to exclude',
       multiple: true
     }),
-    'dry-run': flags.boolean({
-      description: 'prints a list of snapshots without processing them',
-      char: 'd'
-    }),
 
     // static only flags
     'clean-urls': flags.boolean({
@@ -105,27 +101,15 @@ export class Snapshot extends Command {
       (isStatic && await this.loadStaticSnapshots(arg)) ||
       await this.loadSnapshotsFile(arg));
 
-    let l = snapshots.length;
-    if (!l) this.error('No snapshots found');
+    if (!snapshots.length) {
+      this.error('No snapshots found');
+    }
 
     // start processing snapshots
-    let dry = this.flags['dry-run'];
-    if (!dry) await this.percy.start();
-    else this.log.info(`Found ${l} snapshot${l === 1 ? '' : 's'}`);
-
-    for (let snap of snapshots) {
-      if (dry) {
-        this.log.info(`Snapshot found: ${snap.name}`);
-        this.log.debug(`-> url: ${snap.url}`);
-
-        for (let s of (snap.additionalSnapshots || [])) {
-          let name = s.name || `${s.prefix || ''}${snap.name}${s.suffix || ''}`;
-          this.log.info(`Snapshot found: ${name}`);
-          this.log.debug(`-> url: ${snap.url}`);
-        }
-      } else {
-        this.percy.snapshot(snap);
-      }
+    await this.percy.start();
+
+    for (let snapshot of snapshots) {
+      this.percy.snapshot(snapshot);
     }
   }
 

packages/cli-snapshot/test/directory.test.js

@@ -83,13 +83,16 @@ describe('percy snapshot <directory>', () => {
   it('does not take snapshots and prints a list with --dry-run', async () => {
     await Snapshot.run(['./tmp', '--dry-run']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 4 snapshots',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: /test-1.html',
       '[percy] Snapshot found: /test-2.html',
       '[percy] Snapshot found: /test-3.html',
-      '[percy] Snapshot found: /test-index/index.html'
+      '[percy] Snapshot found: /test-index/index.html',
+      '[percy] Found 4 snapshots'
     ]);
   });
 
@@ -106,30 +109,36 @@ describe('percy snapshot <directory>', () => {
 
     await Snapshot.run(['./tmp', '--dry-run']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 4 snapshots',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: First',
-      '[percy] Snapshot found: First (2)',
+      '[percy] Additional snapshot: First (2)',
       '[percy] Snapshot found: /test-2.html',
-      '[percy] Snapshot found: /test-2.html (2)',
+      '[percy] Additional snapshot: /test-2.html (2)',
       '[percy] Snapshot found: /test-3.html',
-      '[percy] Snapshot found: /test-3.html (2)',
+      '[percy] Additional snapshot: /test-3.html (2)',
       '[percy] Snapshot found: /test-index/index.html',
-      '[percy] Snapshot found: /test-index/index.html (2)'
+      '[percy] Additional snapshot: /test-index/index.html (2)',
+      '[percy] Found 8 snapshots'
     ]);
   });
 
   it('rewrites file and index URLs with --clean-urls', async () => {
     await Snapshot.run(['./tmp', '--dry-run', '--clean-urls']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 4 snapshots',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: /test-1',
       '[percy] Snapshot found: /test-2',
       '[percy] Snapshot found: /test-3',
-      '[percy] Snapshot found: /test-index'
+      '[percy] Snapshot found: /test-index',
+      '[percy] Found 4 snapshots'
     ]);
   });
 
@@ -144,13 +153,16 @@ describe('percy snapshot <directory>', () => {
 
     await Snapshot.run(['./tmp', '--dry-run']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 4 snapshots',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: /test/1',
       '[percy] Snapshot found: /test/2',
       '[percy] Snapshot found: /test/3',
-      '[percy] Snapshot found: /test-index'
+      '[percy] Snapshot found: /test-index',
+      '[percy] Found 4 snapshots'
     ]);
   });
 
@@ -168,11 +180,14 @@ describe('percy snapshot <directory>', () => {
 
     await Snapshot.run(['./tmp', '--dry-run']);
 
-    expect(logger.stderr).toEqual([]);
-    expect(logger.stdout).toEqual(jasmine.arrayContaining([
-      '[percy] Found 2 snapshots',
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
+    expect(logger.stdout).toEqual([
+      '[percy] Percy has started!',
       '[percy] Snapshot found: /test-1',
-      '[percy] Snapshot found: /test-3'
-    ]));
+      '[percy] Snapshot found: /test-3',
+      '[percy] Found 2 snapshots'
+    ]);
   });
 });

packages/cli-snapshot/test/file.test.js

@@ -186,21 +186,28 @@ describe('percy snapshot <file>', () => {
 
   it('does not take snapshots and prints a list with --dry-run', async () => {
     await Snapshot.run(['./pages.yml', '--dry-run']);
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 1 snapshot',
-      '[percy] Snapshot found: YAML Snapshot'
+      '[percy] Percy has started!',
+      '[percy] Snapshot found: YAML Snapshot',
+      '[percy] Found 1 snapshot'
     ]);
 
     logger.reset();
 
     await Snapshot.run(['./pages.js', '--dry-run']);
-    expect(logger.stderr).toEqual([]);
+
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 1 snapshot',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: JS Snapshot',
-      '[percy] Snapshot found: JS Snapshot 2',
-      '[percy] Snapshot found: Other JS Snapshot'
+      '[percy] Additional snapshot: JS Snapshot 2',
+      '[percy] Additional snapshot: Other JS Snapshot',
+      '[percy] Found 3 snapshots'
     ]);
   });
 

packages/cli-snapshot/test/sitemap.test.js

@@ -42,12 +42,15 @@ describe('percy snapshot <sitemap>', () => {
   it('snapshots URLs listed by a sitemap', async () => {
     await Snapshot.run(['http://localhost:8000/sitemap.xml', '--dry-run']);
 
-    expect(logger.stderr).toEqual([]);
+    expect(logger.stderr).toEqual([
+      '[percy] Build not created'
+    ]);
     expect(logger.stdout).toEqual([
-      '[percy] Found 3 snapshots',
+      '[percy] Percy has started!',
       '[percy] Snapshot found: /',
       '[percy] Snapshot found: /test-1/',
-      '[percy] Snapshot found: /test-2/'
+      '[percy] Snapshot found: /test-2/',
+      '[percy] Found 3 snapshots'
     ]);
   });
 

packages/cli-upload/src/commands/upload.js

@@ -138,9 +138,8 @@ export class Upload extends Command {
     if (this.closing) return;
     this.closing = true;
 
-    await this.queue.empty(len => {
-      this.log.progress(`Uploading ${len}` + (
-        ` snapshot${len !== 1 ? 's' : ''}...`), !!len);
+    await this.queue.empty(s => {
+      this.log.progress(`Uploading ${s} snapshot${s !== 1 ? 's' : ''}...`, !!s);
     });
 
     await this.client.finalizeBuild(this.build.id);

packages/core/src/config.js

@@ -1,7 +1,3 @@
-import { strict as assert } from 'assert';
-import PercyConfig from '@percy/config';
-import { merge } from '@percy/config/dist/utils';
-
 // Common config options used in Percy commands
 export const configSchema = {
   snapshot: {
@@ -263,52 +259,3 @@ export const migrations = [
   ['/config', configMigration],
   ['/snapshot', snapshotMigration]
 ];
-
-// Validate and merge per-snapshot configuration options with global configuration options.
-export function getSnapshotConfig(options, { snapshot, discovery }, log) {
-  // prune client and env info from being validated
-  let { clientInfo, environmentInfo, ...config } = PercyConfig.migrate(options, '/snapshot');
-
-  // throw an error when missing required options
-  assert(config.url, 'Missing required URL for snapshot');
-  assert((config.widths ?? snapshot.widths)?.length, 'Missing required widths for snapshot');
-
-  // validate and scrub according to dom snaphot presence
-  let errors = PercyConfig.validate(config, (
-    config.domSnapshot ? '/snapshot/dom' : '/snapshot'));
-
-  if (errors) {
-    log.warn('Invalid snapshot options:');
-    for (let e of errors) log.warn(`- ${e.path}: ${e.message}`);
-  }
-
-  // parse the URL to construct defaults
-  let url = new URL(options.url);
-
-  // inherit options from the config
-  return merge([snapshot, {
-    // default to the URL /pathname?search#hash
-    name: `${url.pathname}${url.search}${url.hash}`,
-    // add back client and environment information
-    clientInfo,
-    environmentInfo,
-    // only specific discovery options are used per-snapshot
-    discovery: {
-      allowedHostnames: [url.hostname, ...discovery.allowedHostnames],
-      requestHeaders: discovery.requestHeaders,
-      authorization: discovery.authorization,
-      disableCache: discovery.disableCache,
-      userAgent: discovery.userAgent
-    }
-  }, config], (path, prev, next) => {
-    switch (path.join('.')) {
-      case 'widths': // override and sort widths
-        return [path, next.sort((a, b) => a - b)];
-      case 'percyCSS': // concatenate percy css
-        return [path, [prev, next].filter(Boolean).join('\n')];
-      case 'execute': // shorthand for execute.beforeSnapshot
-        return (Array.isArray(next) || typeof next !== 'object')
-          ? [path.concat('beforeSnapshot'), next] : [path];
-    }
-  });
-}

packages/core/src/discovery.js

@@ -17,10 +17,10 @@ export function createRequestHandler(network, { disableCache, getResource }) {
       let resource = getResource(url);
 
       if (resource?.root) {
-        log.debug('-> Serving root resource', meta);
+        log.debug('- Serving root resource', meta);
         await request.respond(resource);
       } else if (resource && !disableCache) {
-        log.debug('-> Resource cache hit', meta);
+        log.debug('- Resource cache hit', meta);
         await request.respond(resource);
       } else {
         await request.continue();
@@ -41,7 +41,7 @@ export function createRequestFinishedHandler(network, {
   allowedHostnames,
   disableCache,
   getResource,
-  addResource
+  saveResource
 }) {
   let log = logger('core:discovery');
 
@@ -63,17 +63,17 @@ export function createRequestFinishedHandler(network, {
 
         /* istanbul ignore next: sanity check */
         if (!response) {
-          return log.debug('-> Skipping no response', meta);
+          return log.debug('- Skipping no response', meta);
         } else if (!capture) {
-          return log.debug('-> Skipping remote resource', meta);
+          return log.debug('- Skipping remote resource', meta);
         } else if (!body.length) {
-          return log.debug('-> Skipping empty response', meta);
+          return log.debug('- Skipping empty response', meta);
         } else if (body.length > MAX_RESOURCE_SIZE) {
-          return log.debug('-> Skipping resource larger than 15MB', meta);
+          return log.debug('- Skipping resource larger than 15MB', meta);
         } else if (!ALLOWED_STATUSES.includes(response.status)) {
-          return log.debug(`-> Skipping disallowed status [${response.status}]`, meta);
+          return log.debug(`- Skipping disallowed status [${response.status}]`, meta);
         } else if (!enableJavaScript && !ALLOWED_RESOURCES.includes(request.type)) {
-          return log.debug(`-> Skipping disallowed resource type [${request.type}]`, meta);
+          return log.debug(`- Skipping disallowed resource type [${request.type}]`, meta);
         }
 
         resource = createResource(url, body, response.mimeType, {
@@ -86,11 +86,11 @@ export function createRequestFinishedHandler(network, {
             ), {})
         });
 
-        log.debug(`-> sha: ${resource.sha}`, meta);
-        log.debug(`-> mimetype: ${resource.mimetype}`, meta);
+        log.debug(`- sha: ${resource.sha}`, meta);
+        log.debug(`- mimetype: ${resource.mimetype}`, meta);
       }
 
-      addResource(resource);
+      saveResource(resource);
     } catch (error) {
       log.debug(`Encountered an error processing resource: ${url}`, meta);
       log.debug(error);

packages/core/src/network.js

@@ -88,7 +88,7 @@ export default class Network {
         if (this.log.shouldLog('debug')) {
           msg += `\n\n  ${['Active requests:',
             ...requests.map(r => r.url)
-          ].join('\n  -> ')}\n`;
+          ].join('\n  - ')}\n`;
         }
 
         throw new Error(msg);

packages/core/src/page.js

@@ -162,6 +162,8 @@ export default class Page {
     execute,
     ...options
   }) {
+    this.log.debug(`Taking snapshot: ${name}`, this.meta);
+
     // wait for any specified timeout
     if (waitForTimeout) {
       this.log.debug(`Wait for ${waitForTimeout}ms timeout`, this.meta);