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

Escape Windows path separator in testPathPattern CLI arguments #5054

Merged
merged 4 commits into from Dec 11, 2017
Merged

Escape Windows path separator in testPathPattern CLI arguments #5054

merged 4 commits into from Dec 11, 2017

Conversation

seanpoulter
Copy link
Contributor

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:

yarn build
yarn jest jest-config/src/__tests__/normalize

This would previously fail with:

Pattern: jest-config/src/__tests__/normalize - 0 matches

But now runs the added tests:

Ran all test suites matching /jest-config\\src\\__tests__\\normalize/i.

@codecov-io
Copy link

Codecov Report

Merging #5054 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 91.95% <ø> (+1.14%) ⬆️

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 3ea2e10...5a6f756. Read the comment docs.

@cpojer cpojer requested a review from SimenB December 11, 2017 18:20
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix, thanks!

@seanpoulter
Copy link
Contributor Author

Yay! If you're in the testPathPattern headspace, I'm happy to build out the testPathPattern tests with some clarification on how to handle multiple args (#5037).

@seanpoulter seanpoulter reopened this Dec 11, 2017
@seanpoulter
Copy link
Contributor Author

Sorry for closing it there. Hit the wrong comment button. 🤦‍♂️

@SimenB
Copy link
Member

SimenB commented Dec 11, 2017

I think your suggestion in #5037 makes sense, @cpojer WDYT?

@cpojer cpojer merged commit 6049858 into jestjs:master Dec 11, 2017
@cpojer
Copy link
Member

cpojer commented Dec 11, 2017

Yep, sounds good.

cpojer pushed a commit that referenced this pull request Jan 4, 2018
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.
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.
@samayo
Copy link

samayo commented Jun 13, 2019

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)

// jest.config.js
testMatch: [
   '<rootDir>/(test/unit/*.spec.(js|jsx|ts|tsx)|**/__test__/*.(js|jsx|ts|tsx))'
 ],

My test directory confirms to that testMatch rule. This is what it looks like:

node_modules
public
src
  test
    unit
      test.spec.js

The error I get is this:

$ npm test  

> my-app@0.1.0 test C:\_\playground\my-app
> jest

No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In C:\_\playground\my-app
  11 files checked.
  testMatch: C:/_/playground/my-app\(test/unit/*.spec.(js|jsx|ts|tsx)|**/__test__/*.(js|jsx|ts|tsx)) - 0 matches
  testPathIgnorePatterns: \\node_modules\\ - 11 matches
  testRegex:  - 0 matches
Pattern:  - 0 matches
npm ERR! Test failed.  See above for more details.

@seanpoulter
Copy link
Contributor Author

What's your rationale to think this is the same problem @samayo? Have you tracked it down to the same buildTestPathPattern function?

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.

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

Can't run a single test file specified by name via the CLI
6 participants