fix: parsing glob strings (#404)

* fix: 🐛 consistently split comma-separated glob strings * ✅ Add tests for glob parsing util
percy · Oct 28, 2019 · d52e552 · d52e552
1 parent 22b790c
commit d52e552
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 96 deletions.
diff --git a/src/services/image-snapshot-service.ts b/src/services/image-snapshot-service.ts
@@ -7,6 +7,7 @@ import * as path from 'path'
 
 import { DEFAULT_CONFIGURATION } from '../configuration/configuration'
 import { ImageSnapshotsConfiguration } from '../configuration/image-snapshots-configuration'
+import { parseGlobs } from '../utils/configuration'
 import logger, { logError, profile } from '../utils/logger'
 import BuildService from './build-service'
 import PercyClientService from './percy-client-service'
@@ -106,9 +107,8 @@ export default class ImageSnapshotService extends PercyClientService {
   }
 
   async snapshotAll() {
-    // intentially remove '' values from because that matches every file
-    const globs = this.configuration.files.split(',').filter(Boolean)
-    const ignore = this.configuration.ignore.split(',').filter(Boolean)
+    const globs = parseGlobs(this.configuration.files)
+    const ignore = parseGlobs(this.configuration.ignore)
     const paths = await globby(globs, { cwd: this.configuration.path, ignore })
     let error
 

diff --git a/src/services/static-snapshot-service.ts b/src/services/static-snapshot-service.ts
@@ -6,6 +6,7 @@ import {Server} from 'http'
 import * as puppeteer from 'puppeteer'
 import { DEFAULT_CONFIGURATION } from '../configuration/configuration'
 import { StaticSnapshotsConfiguration } from '../configuration/static-snapshots-configuration'
+import { parseGlobs } from '../utils/configuration'
 import logger from '../utils/logger'
 import {agentJsFilename} from '../utils/sdk-utils'
 
@@ -97,28 +98,11 @@ export default class StaticSnapshotService {
   }
 
   async _buildPageUrls() {
-    // We very intentially remove '' values from these globs because that matches every file
-    const ignoreGlobs = this.configuration['ignore-files']
-      .split(',')
-      .filter((value) => value !== '')
-
-    const snapshotGlobs = this.configuration['snapshot-files']
-      .split(',')
-      .filter((value) => value !== '')
-
-    const globOptions = {
-      cwd: this.configuration.path,
-      ignore: ignoreGlobs,
-    }
-
-    const paths = await globby(snapshotGlobs, globOptions)
-    const pageUrls = [] as any
+    const globs = parseGlobs(this.configuration['snapshot-files'])
+    const ignore = parseGlobs(this.configuration['ignore-files'])
+    const paths = await globby(globs, { cwd: this.configuration.path, ignore })
     const baseUrl = this._buildLocalUrl()
 
-    for (const path of paths) {
-      pageUrls.push(baseUrl + path)
-    }
-
-    return pageUrls
+    return paths.map((path) => baseUrl + path)
   }
 }
diff --git a/src/utils/configuration.ts b/src/utils/configuration.ts
@@ -57,6 +57,16 @@ function transform(flags: any, args: any) {
   })
 }
 
+// splits on commas, but not within curly braces
+const SPLIT_REGEXP = /(?<=^|,)([^,]*{.*?}[^,]*|.*?)(?=,|$)/g
+export function parseGlobs(globStr: string): string[] {
+  return globStr.match(SPLIT_REGEXP)!
+  // trim whitespace
+    .map((str) => str.trim())
+  // empty globs would match every file
+    .filter(Boolean)
+}
+
 export default function config({ config, ...flags }: any, args: any = {}) {
   let loaded
 

diff --git a/test/utils/configuration.test.ts b/test/utils/configuration.test.ts
@@ -2,98 +2,122 @@ import { expect } from 'chai'
 import * as fs from 'fs'
 import * as path from 'path'
 import { DEFAULT_CONFIGURATION } from '../../src/configuration/configuration'
-import config, { explorer } from '../../src/utils/configuration'
+import config, { explorer, parseGlobs } from '../../src/utils/configuration'
 
 function dedent(str: string) {
   const indent = str.match(/ +/g)![0].length
   return str.replace(new RegExp(`\n {${indent}}`, 'g'), '\n').trim()
 }
 
-describe('configuration', () => {
-  let configfiles: string[]
+describe('configuration utils', () => {
+  describe('config', () => {
+    let configfiles: string[]
 
-  // helper to create a config files and cleanup on `afterEach`
-  function mkconfig(filename: string, contents: string) {
-    const filepath = path.join(process.cwd(), filename)
-    fs.writeFileSync(filepath, dedent(contents))
-    configfiles.push(filepath)
-  }
+    // helper to create a config files and cleanup on `afterEach`
+    function mkconfig(filename: string, contents: string) {
+      const filepath = path.join(process.cwd(), filename)
+      fs.writeFileSync(filepath, dedent(contents))
+      configfiles.push(filepath)
+    }
 
-  beforeEach(() => {
-    // clear caches for creating & removing files during testing
-    explorer.clearCaches()
-    configfiles = []
-  })
+    beforeEach(() => {
+      // clear caches for creating & removing files during testing
+      explorer.clearCaches()
+      configfiles = []
+    })
 
-  afterEach(() => {
-    // clean up any created config files
-    configfiles.forEach((file) => {
-      fs.unlinkSync(file)
+    afterEach(() => {
+      // clean up any created config files
+      configfiles.forEach((file) => {
+        fs.unlinkSync(file)
+      })
     })
-  })
 
-  it('returns the default configuration', () => {
-    expect(config({})).to.deep.equal(DEFAULT_CONFIGURATION)
-  })
+    it('returns the default configuration', () => {
+      expect(config({})).to.deep.equal(DEFAULT_CONFIGURATION)
+    })
 
-  it('automatically loads overrides from a `.percy.yml` config file', () => {
-    mkconfig('.percy.yml', `
-      version: 1
-      snapshot:
-        widths: [320, 1200]
-        enable-javascript: true
-      agent:
-        asset-discovery:
-          request-headers:
-            Authorization: 'Basic abc123='
-   `)
+    it('automatically loads overrides from a `.percy.yml` config file', () => {
+      mkconfig('.percy.yml', `
+        version: 1
+        snapshot:
+          widths: [320, 1200]
+          enable-javascript: true
+        agent:
+          asset-discovery:
+            request-headers:
+              Authorization: 'Basic abc123='
+      `)
 
-    expect(config({})).to.deep.equal({
-      ...DEFAULT_CONFIGURATION,
-      snapshot: {
-        ...DEFAULT_CONFIGURATION.snapshot,
-        'widths': [320, 1200],
-        'enable-javascript': true,
-      },
-      agent: {
-        ...DEFAULT_CONFIGURATION.agent,
-        'asset-discovery': {
-          ...DEFAULT_CONFIGURATION.agent['asset-discovery'],
-          'request-headers': {
-            Authorization: 'Basic abc123=',
+      expect(config({})).to.deep.equal({
+        ...DEFAULT_CONFIGURATION,
+        snapshot: {
+          ...DEFAULT_CONFIGURATION.snapshot,
+          'widths': [320, 1200],
+          'enable-javascript': true,
+        },
+        agent: {
+          ...DEFAULT_CONFIGURATION.agent,
+          'asset-discovery': {
+            ...DEFAULT_CONFIGURATION.agent['asset-discovery'],
+            'request-headers': {
+              Authorization: 'Basic abc123=',
+            },
           },
         },
-      },
+      })
+    })
+
+    it('overrides defaults and config file options with flags and args', () => {
+      mkconfig('.percy.json', `{
+        "version": 1,
+        "snapshot": {
+          "widths": [800]
+        },
+        "static-snapshots": {
+          "path": "_wrong/",
+          "ignore-files": "**/*.ignore.*"
+        }
+      }`)
+
+      const flags = { 'snapshot-files': '**/*.snapshot.html' }
+      const args = { snapshotDirectory: '_site/' }
+
+      expect(config(flags, args)).to.deep.equal({
+        ...DEFAULT_CONFIGURATION,
+        'snapshot': {
+          ...DEFAULT_CONFIGURATION.snapshot,
+          widths: [800],
+        },
+        'static-snapshots': {
+          ...DEFAULT_CONFIGURATION['static-snapshots'],
+          'path': '_site/',
+          'ignore-files': '**/*.ignore.*',
+          'snapshot-files': '**/*.snapshot.html',
+        },
+      })
     })
   })
 
-  it('overrides defaults and config file options with flags and args', () => {
-    mkconfig('.percy.json', `{
-      "version": 1,
-      "snapshot": {
-        "widths": [800]
-      },
-      "static-snapshots": {
-        "path": "_wrong/",
-        "ignore-files": "**/*.ignore.*"
-      }
-   }`)
+  describe('parseGlobs', () => {
+    it('splits glob strings with commas', () => {
+      expect(parseGlobs('*/**.html,foo/bar'))
+        .to.deep.equal(['*/**.html', 'foo/bar'])
+    })
+
+    it('does not split on commas within glob "or" lists', () => {
+      expect(parseGlobs('*/**.html,*.{png,jpg},foo'))
+        .to.deep.equal(['*/**.html', '*.{png,jpg}', 'foo'])
+    })
 
-    const flags = { 'snapshot-files': '**/*.snapshot.html' }
-    const args = { snapshotDirectory: '_site/' }
+    it('trims whitespace from globs', () => {
+      expect(parseGlobs('*/**.html,   *.{png,jpg} ,  foo'))
+        .to.deep.equal(['*/**.html', '*.{png,jpg}', 'foo'])
+    })
 
-    expect(config(flags, args)).to.deep.equal({
-      ...DEFAULT_CONFIGURATION,
-      'snapshot': {
-        ...DEFAULT_CONFIGURATION.snapshot,
-        widths: [800],
-      },
-      'static-snapshots': {
-        ...DEFAULT_CONFIGURATION['static-snapshots'],
-        'path': '_site/',
-        'ignore-files': '**/*.ignore.*',
-        'snapshot-files': '**/*.snapshot.html',
-      },
+    it('filters empty globs', () => {
+      expect(parseGlobs('*/**.html,,foo,'))
+        .to.deep.equal(['*/**.html', 'foo'])
     })
   })
 })