-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Escape Windows path separator in testPathPattern CLI arguments #5054
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5054 +/- ##
==========================================
+ Coverage 60.72% 60.74% +0.01%
==========================================
Files 198 198
Lines 6605 6605
Branches 4 3 -1
==========================================
+ Hits 4011 4012 +1
+ Misses 2594 2593 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean fix, thanks!
Yay! If you're in the |
Sorry for closing it there. Hit the wrong comment button. 🤦♂️ |
Yep, sounds good. |
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.
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.
Sorry I don't understand this fix, as I am fairly new to jest. But I have the same problem with no fix currently. I don't want to open an new issue for this, so I will ask here. Basically this is the line that is causing the issue (I think)
My test directory confirms to that
The error I get is this:
|
What's your rationale to think this is the same problem @samayo? Have you tracked it down to the same I don't remember specifics but I say this PR changed the way we handle the separators in the command line arguments. I'm not sure if it changed how we handle the configuration file. |
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. |
Summary
This fixes #3793 by adding the path separator escaping that is used in watch mode to the
--testPathPattern
and default<regexForTestFiles>
command line arguments.Test plan
Since there were no existing tests for
testPathPattern
, this PR adds tests for the existing behavior as well as the fix for path separator escaping on Windows systems.I have confirmed this works on my Windows setup:
This would previously fail with:
But now runs the added tests: