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

Resolve #5216 #5269

Merged
merged 1 commit into from
Jan 10, 2018
Merged

Resolve #5216 #5269

merged 1 commit into from
Jan 10, 2018

Conversation

seanpoulter
Copy link
Contributor

Summary
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 Fix --testPathPattern escaping for '\\' on Windows #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.


Test plan
Tests have been updated.

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.
@seanpoulter
Copy link
Contributor Author

The previous test case results don't change:

image

This does re-introduce the regression in the escaping of the Watch Usage path pattern. If you enter jest-config\src on Windows, it is escaped and matches the jest-config\src path. If you enter jest-config\\src is also escaped and will not match. 😖 We can clear that one up later, and urge folks to use / instead of \ which is ambiguous as a path sep or regexp special character delimter.

@codecov-io
Copy link

Codecov Report

Merging #5269 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5269      +/-   ##
==========================================
- Coverage   61.14%   60.98%   -0.16%     
==========================================
  Files         203      203              
  Lines        6843     6816      -27     
  Branches        4        4              
==========================================
- Hits         4184     4157      -27     
  Misses       2658     2658              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 93.1% <ø> (ø) ⬆️
packages/jest-regex-util/src/index.js 60% <100%> (-29.19%) ⬇️

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 e95ec14...f901aa4. Read the comment docs.

@cpojer cpojer merged commit 3ffc99e into jestjs:master Jan 10, 2018
@cpojer
Copy link
Member

cpojer commented Jan 10, 2018

Thanks for following up on this @seanpoulter. Ugh, path separators and regex.

@SimenB
Copy link
Member

SimenB commented Jan 11, 2018

@seanpoulter mind sending a quick PR for the changelog? It's been released in 22.0.6

@seanpoulter
Copy link
Contributor Author

Would you like a note in the docs to say / is a valid path sep on Windows too, @SimenB?

@seanpoulter seanpoulter deleted the issue-5216 branch January 11, 2018 13:36
@SimenB
Copy link
Member

SimenB commented Jan 11, 2018

Yeah, I think that's a good idea.

@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.

5 participants