Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --testPathPattern escaping for '\\' on Windows #5230

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Fix --testPathPattern escaping for '\\' on Windows #5230

merged 1 commit into from
Jan 4, 2018

Conversation

seanpoulter
Copy link
Contributor

Summary
PR #5054 introduced a regression when handling escaped Windows path separators in the
--testPathPattern. The PR applied the same escaping as the "Watch Usage" prompt, which incorrectly escapes path\\.*file as path\\\.*file.

This PR fixes the common regular expression escaping used in the "Watch Usage" prompt and the --testPathPattern CLI argument with unit tests.

Test plan
New unit tests have been added to confirm the regular expression escaping behavior. I've also added a snapshot to test the 8 test cases documented in #5216. I'm happy to record GIFs from a Linux and Windows system if you feel it's necessary.

Lessons learned
Sorry folks. I hastily submitted #5054 without adding enough tests to be confident in the regular expression escaping. I was wrong to assume that the "Watch Usage" prompt would behave the same way.

@seanpoulter
Copy link
Contributor Author

As a heads up, here's the manual testing results with this PR.
Looks good, but there's still something else modifying the pattern for the \ case on Linux:

image

PR #5054 introduced a regression when handling escaped Windows path
separators in the `--testPathPattern`. The PR applied the same escaping
as the "Watch Usage" prompt, which incorrectly escapes `path\\.*file` as
`path\\\.*file`.

This commit fixes the regular expression used in "Watch Usage" and the
`--testPathPattern` CLI argument with unit tests.
@codecov-io
Copy link

codecov-io commented Jan 4, 2018

Codecov Report

Merging #5230 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5230      +/-   ##
==========================================
+ Coverage   60.91%   61.13%   +0.22%     
==========================================
  Files         202      202              
  Lines        6731     6760      +29     
  Branches        4        4              
==========================================
+ Hits         4100     4133      +33     
+ Misses       2630     2626       -4     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-regex-util/src/index.js 89.18% <100%> (+89.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e25eceb...c98c84c. Read the comment docs.

@cpojer cpojer merged commit f3fce7b into jestjs:master Jan 4, 2018
@cpojer
Copy link
Member

cpojer commented Jan 4, 2018

Thank you for the thorough investigation and fix, this is really awesome and I greatly appreciate it. I merged this PR and let you consider the posix difference separately.

@seanpoulter
Copy link
Contributor Author

... there's still something else modifying the pattern for the \ case on Linux

All good, I just forgot to escape the \ in Bash. 😞

image

@seanpoulter seanpoulter deleted the issue-5216 branch January 9, 2018 19:02
@seanpoulter seanpoulter mentioned this pull request Jan 9, 2018
cpojer pushed a commit that referenced this pull request Jan 10, 2018
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.
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants