Resolve #5216 (#5269)

PR #5054 added a call to `replacePathSepForRegex` to escape values of the
`--testPathPattern` and `<regexForTestFiles>` CLI options. Since the Windows
path separator and the regular expression special character delimeter are the
same character, this can lead to ambiguous patterns (e.g.: `app\book\d*\`).

This commit:
- Removes escaping CLI args with `replacePathSepForRegex` to leave them as is
  unless it's a POSIX path separator on Windows

- Changes the tests in `normalize.test.js` to run the same test suite for
  `--testPathPattern` and `<regexForTestFiles>`

- Reverts the changes to `replacePathSepForRegex` from #5230 but keeps the tests
  for the intended behavior. It will be complicated to escape the "safe" cases
  when `\` is a path separator and not a regular expression delimeter. Instead
  of getting fancy, we can urge Windows users to use `/` or `\\` as a path
  separator.

Author: seanpoulter

packages/jest-config/src/__tests__/normalize.test.js

@@ -1031,15 +1031,6 @@ describe('testPathPattern', () => {
   const initialOptions = {rootDir: '/root'};
   const consoleLog = console.log;
 
-  function testWindowsPathSeparator(argv, expected) {
-    jest.resetModules();
-    jest.mock('path', () => require.requireActual('path').win32);
-    require('jest-resolve').findNodeModule = findNodeModule;
-
-    const {options} = require('../normalize').default(initialOptions, argv);
-    expect(options.testPathPattern).toBe(expected);
-  }
-
   beforeEach(() => {
     console.log = jest.fn();
   });
@@ -1053,62 +1044,85 @@ describe('testPathPattern', () => {
     expect(options.testPathPattern).toBe('');
   });
 
-  describe('--testPathPattern', () => {
-    it('uses testPathPattern if set', () => {
-      const {options} = normalize(initialOptions, {testPathPattern: ['a/b']});
-      expect(options.testPathPattern).toBe('a/b');
-    });
+  const cliOptions = [
+    {name: '--testPathPattern', property: 'testPathPattern'},
+    {name: '<regexForTestFiles>', property: '_'},
+  ];
+  for (const opt of cliOptions) {
+    describe(opt.name, () => {
+      it('uses ' + opt.name + ' if set', () => {
+        const argv = {[opt.property]: ['a/b']};
+        const {options} = normalize(initialOptions, argv);
 
-    it('ignores invalid regular expressions and logs a warning', () => {
-      const {options} = normalize(initialOptions, {testPathPattern: ['a(']});
-      expect(options.testPathPattern).toBe('');
-      expect(console.log.mock.calls[0][0]).toMatchSnapshot();
-    });
+        expect(options.testPathPattern).toBe('a/b');
+      });
 
-    it('escapes Windows path separators', () => {
-      testWindowsPathSeparator({testPathPattern: ['a\\b']}, 'a\\\\b');
-    });
+      it('ignores invalid regular expressions and logs a warning', () => {
+        const argv = {[opt.property]: ['a(']};
+        const {options} = normalize(initialOptions, argv);
 
-    it('joins multiple --testPathPatterns if set', () => {
-      const {options} = normalize(initialOptions, {
-        testPathPattern: ['a/b', 'c/d'],
+        expect(options.testPathPattern).toBe('');
+        expect(console.log.mock.calls[0][0]).toMatchSnapshot();
       });
-      expect(options.testPathPattern).toBe('a/b|c/d');
-    });
 
-    it('escapes Windows path separators in multiple args', () => {
-      testWindowsPathSeparator(
-        {testPathPattern: ['a\\b', 'c\\d']},
-        'a\\\\b|c\\\\d',
-      );
-    });
-  });
+      it('joins multiple ' + opt.name + ' if set', () => {
+        const argv = {testPathPattern: ['a/b', 'c/d']};
+        const {options} = normalize(initialOptions, argv);
 
-  describe('<regexForTestFiles>', () => {
-    it('uses <regexForTestFiles> if set', () => {
-      const {options} = normalize(initialOptions, {_: ['a/b']});
-      expect(options.testPathPattern).toBe('a/b');
-    });
-
-    it('ignores invalid regular expressions and logs a warning', () => {
-      const {options} = normalize(initialOptions, {_: ['a(']});
-      expect(options.testPathPattern).toBe('');
-      expect(console.log.mock.calls[0][0]).toMatchSnapshot();
-    });
+        expect(options.testPathPattern).toBe('a/b|c/d');
+      });
 
-    it('escapes Windows path separators', () => {
-      testWindowsPathSeparator({_: ['a\\b']}, 'a\\\\b');
-    });
+      describe('posix', () => {
+        it('should not escape the pattern', () => {
+          const argv = {[opt.property]: ['a\\/b', 'a/b', 'a\\b', 'a\\\\b']};
+          const {options} = normalize(initialOptions, argv);
 
-    it('joins multiple <regexForTestFiles> if set', () => {
-      const {options} = normalize(initialOptions, {_: ['a/b', 'c/d']});
-      expect(options.testPathPattern).toBe('a/b|c/d');
-    });
+          expect(options.testPathPattern).toBe('a\\/b|a/b|a\\b|a\\\\b');
+        });
+      });
 
-    it('escapes Windows path separators in multiple args', () => {
-      testWindowsPathSeparator({_: ['a\\b', 'c\\d']}, 'a\\\\b|c\\\\d');
+      describe('win32', () => {
+        beforeEach(() => {
+          jest.mock('path', () => require.requireActual('path').win32);
+          require('jest-resolve').findNodeModule = findNodeModule;
+        });
+
+        afterEach(() => {
+          jest.resetModules();
+        });
+
+        it('preserves any use of "\\"', () => {
+          const argv = {[opt.property]: ['a\\b', 'c\\\\d']};
+          const {options} = require('../normalize').default(
+            initialOptions,
+            argv,
+          );
+
+          expect(options.testPathPattern).toBe('a\\b|c\\\\d');
+        });
+
+        it('replaces POSIX path separators', () => {
+          const argv = {[opt.property]: ['a/b']};
+          const {options} = require('../normalize').default(
+            initialOptions,
+            argv,
+          );
+
+          expect(options.testPathPattern).toBe('a\\\\b');
+        });
+
+        it('replaces POSIX paths in multiple args', () => {
+          const argv = {[opt.property]: ['a/b', 'c/d']};
+          const {options} = require('../normalize').default(
+            initialOptions,
+            argv,
+          );
+
+          expect(options.testPathPattern).toBe('a\\\\b|c\\\\d');
+        });
+      });
     });
-  });
+  }
 
   it('joins multiple --testPathPatterns and <regexForTestFiles>', () => {
     const {options} = normalize(initialOptions, {

packages/jest-config/src/normalize.js

@@ -289,7 +289,14 @@ const buildTestPathPattern = (argv: Argv): string => {
     patterns.push(...argv.testPathPattern);
   }
 
-  const testPathPattern = patterns.map(replacePathSepForRegex).join('|');
+  const replacePosixSep = (pattern: string) => {
+    if (path.sep === '/') {
+      return pattern;
+    }
+    return pattern.replace(/\//g, '\\\\');
+  };
+
+  const testPathPattern = patterns.map(replacePosixSep).join('|');
   if (validatePattern(testPathPattern)) {
     return testPathPattern;
   } else {

packages/jest-regex-util/src/__tests__/__snapshots__/index.test.js.snap

@@ -4,66 +4,40 @@ import {replacePathSepForRegex} from '../index';
 import path from 'path';
 
 describe('replacePathSepForRegex()', () => {
-  const testPatternsFrom5216 = [
-    'jest-config\\/.*normalize',
-    'jest-config/.*normalize',
-    'jest-config\\.*normalize',
-    'jest-config\\\\.*normalize',
-  ];
-
   describe('posix', () => {
     beforeEach(() => (path.sep = '/'));
 
     it('should return the path', () => {
       const expected = {};
       expect(replacePathSepForRegex(expected)).toBe(expected);
     });
-
-    // Confirming existing behavior; could be changed to improve cross-platform support
-    it('should not replace Windows path separators', () => {
-      expect(replacePathSepForRegex('a\\.*b')).toBe('a\\.*b');
-      expect(replacePathSepForRegex('a\\\\.*b')).toBe('a\\\\.*b');
-    });
-
-    // Bonus: Test cases from https://github.com/facebook/jest#5216
-    it('should match the expected output from #5216', () => {
-      expect(
-        testPatternsFrom5216.map(replacePathSepForRegex),
-      ).toMatchSnapshot();
-    });
   });
 
   describe('win32', () => {
     beforeEach(() => (path.sep = '\\'));
 
-    it('should escape Windows path separators', () => {
-      expect(replacePathSepForRegex('a\\b\\c')).toBe('a\\\\b\\\\c');
-    });
-
     it('should replace POSIX path separators', () => {
       expect(replacePathSepForRegex('a/b/c')).toBe('a\\\\b\\\\c');
-    });
 
-    it('should not escape an escaped period', () => {
-      expect(replacePathSepForRegex('a\\.dotfile')).toBe('a\\.dotfile');
-      expect(replacePathSepForRegex('a\\\\\\.dotfile')).toBe('a\\\\\\.dotfile');
+      // When a regular expression is created with a string, not enclosing
+      // slashes like "/<pattern>/", the "/" character does not need to be
+      // escaped as "\/". The result is the double path separator: "\\"
+      expect(replacePathSepForRegex('a\\/b')).toBe('a\\\\\\\\b');
     });
 
-    it('should not escape an escaped Windows path separator', () => {
-      expect(replacePathSepForRegex('a\\\\b')).toBe('a\\\\b');
-      expect(replacePathSepForRegex('a\\\\.dotfile')).toBe('a\\\\.dotfile');
+    it('should escape Windows path separators', () => {
+      expect(replacePathSepForRegex('a\\b\\c')).toBe('a\\\\b\\\\c');
     });
 
-    // Confirming existing behavior; could be changed to improve cross-platform support
-    it('should not replace escaped POSIX separators', () => {
-      expect(replacePathSepForRegex('a\\/b')).toBe('a\\\\\\\\b');
-    });
+    it('should not escape an escaped dot', () => {
+      expect(replacePathSepForRegex('a\\.dotfile')).toBe('a\\.dotfile');
 
-    // Bonus: Test cases from https://github.com/facebook/jest#5216
-    it('should match the expected output from #5216', () => {
-      expect(
-        testPatternsFrom5216.map(replacePathSepForRegex),
-      ).toMatchSnapshot();
+      // If we expect Windows path separators to be escaped, one would expect
+      // the regular expression "\\\." to be unescaped as "\.". This is not the
+      // current behavior.
+      expect(replacePathSepForRegex('a\\\\\\.dotfile')).toBe(
+        'a\\\\\\\\\\.dotfile',
+      );
     });
   });
 });

packages/jest-regex-util/src/__tests__/index.test.js

@@ -22,56 +22,8 @@ export const escapeStrForRegex = (string: string) =>
   string.replace(/[[\]{}()*+?.\\^$|]/g, '\\$&');
 
 export const replacePathSepForRegex = (string: string) => {
-  if (!string || path.sep !== '\\') {
-    return string;
-  }
-
-  let result = '';
-  for (let i = 0; i < string.length; i += 1) {
-    const char = string[i];
-    if (char === '\\') {
-      const nextChar = string[i + 1];
-      /* Case: \/ -- recreate legacy behavior */
-      if (nextChar === '/') {
-        i += 1;
-        result += '\\\\\\\\';
-        continue;
-      }
-
-      /* Case: \. */
-      if (nextChar === '.') {
-        i += 1;
-        result += '\\.';
-        continue;
-      }
-
-      /* Case: \\. */
-      if (nextChar === '\\' && string[i + 2] === '.') {
-        i += 2;
-        result += '\\\\.';
-        continue;
-      }
-
-      /* Case: \\ */
-      if (nextChar === '\\') {
-        i += 1;
-        result += '\\\\';
-        continue;
-      }
-
-      /* Case: \<other> */
-      result += '\\\\';
-      continue;
-    }
-
-    /* Case: / */
-    if (char === '/') {
-      result += '\\\\';
-      continue;
-    }
-
-    result += char;
+  if (path.sep === '\\') {
+    return string.replace(/(\/|\\(?!\.))/g, '\\\\');
   }
-
-  return result;
+  return string;
 };