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

Interactive watch arrow selection doesn't work on Windows. #3516

Closed
bookman25 opened this issue May 8, 2017 · 8 comments · Fixed by #3563
Closed

Interactive watch arrow selection doesn't work on Windows. #3516

bookman25 opened this issue May 8, 2017 · 8 comments · Fixed by #3563

Comments

@bookman25
Copy link
Contributor

Do you want to request a feature or report a bug?
bug

What is the current behavior?
Using the new arrow selection for patterns in watch mode does not work on Windows. It correctly selects the entry but I believe because of the slashes it's not finding any tests.

Arrow selection pattern: /_Common/Upload/Html5/FileUploadSpec.ts/
Actual pattern: /_Common\\Upload\\Html5\\FileUploadSpec.ts/

What is the expected behavior?
It should find the tests based off the arrow selection.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.
Jest 20
Yarn 0.23.4
Windows 10

@thymikee
Copy link
Collaborator

thymikee commented May 8, 2017

Would you like to send a PR with a fix? 🙂

@bookman25
Copy link
Contributor Author

If I find some time, I will. Created the ticket in case someone else got to it before I do.

scottambroseio pushed a commit to scottambroseio/jest that referenced this issue May 8, 2017
@scottambroseio
Copy link
Contributor

I had some free time today so I took a bash at fixing this, here's my findings. Here's a diff with a simple but working solution.

I don't know the code base that well at all so I'm not sure if there's a better place to do this logic.

Secondly, this change breaks one of the tests but I'm not sure how best to resolve it, as the test passing or failing depends on the operating system the test is being run on.

err

@thymikee
Copy link
Collaborator

thymikee commented May 8, 2017

@scottrangerio in jest-regex-util there's a replacePathSepForRegex function, I think you'll be better off using it :)

The test may be fixed with slash:

const slash = require('slash');
expect(slash(argv.testPathPattern)).toMatchSnapshot();

We use it to normalize paths in tests on Unix/Windows platforms.

@bookman25
Copy link
Contributor Author

I'm wondering if we can normalize it instead so that watch mode would support / for windows. Right now you have to use \\ to escape slashes on windows and it's a pain. But that might involve making changes in the way the meta data about the files is stored.

scottambroseio pushed a commit to scottambroseio/jest that referenced this issue May 8, 2017
@scottambroseio
Copy link
Contributor

scottambroseio commented May 8, 2017

@thymikee thanks for the suggestions / input!

The replacePathSepForRegex works like a charm, but there's still an issue in the test when using slash

The test isn't actually testing a path, but a regular expression which represents a path. So rather than calling slash with a valid windows path string such as src\index.js, it's being called with the equivalent regex string src\\index.js which results in an extra / per separator.

err2

@scottambroseio
Copy link
Contributor

scottambroseio commented May 9, 2017

I've been thinking some more about a non hack way to fix the test on windows. I really like the slash idea, so I was thinking that something simple like this could do the job.

// regex-slash.js
// could be hosted as a npm module like slash, or simply included somewhere like in jest-regex-util

'use strict';

// path\\to\\file.js -> path/to/file.js
module.exports = (str: string): string => {
	return str.replace(/\\\\/g, '/');
};

usage:

const rslash = require('regex-slash');
expect(rslash(argv.testPathPattern)).toMatchSnapshot();

scottambroseio pushed a commit to scottambroseio/jest that referenced this issue May 9, 2017
cpojer pushed a commit that referenced this issue May 17, 2017
* Fix interactive watch arrow selection on Windows #3516

* Refactored fix for #3516 to use replacePathSepForRegex

* Fixed the failing test on Windows for the #3516 fix

* Bump regex-slash to 1.0.1 (provides a flow lib def)

* Update watch-filename-pattern-mode-test.js

* Code review feedback

* Fixed lint errors and implemented PR feedback

* Revert yarn.lock changes
orta pushed a commit to orta/jest that referenced this issue Jul 7, 2017
* Fix interactive watch arrow selection on Windows jestjs#3516

* Refactored fix for jestjs#3516 to use replacePathSepForRegex

* Fixed the failing test on Windows for the jestjs#3516 fix

* Bump regex-slash to 1.0.1 (provides a flow lib def)

* Update watch-filename-pattern-mode-test.js

* Code review feedback

* Fixed lint errors and implemented PR feedback

* Revert yarn.lock changes
tushardhole pushed a commit to tushardhole/jest that referenced this issue Aug 21, 2017
* Fix interactive watch arrow selection on Windows jestjs#3516

* Refactored fix for jestjs#3516 to use replacePathSepForRegex

* Fixed the failing test on Windows for the jestjs#3516 fix

* Bump regex-slash to 1.0.1 (provides a flow lib def)

* Update watch-filename-pattern-mode-test.js

* Code review feedback

* Fixed lint errors and implemented PR feedback

* Revert yarn.lock changes
@github-actions
Copy link

This issue 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 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants