Windows watch mode fix by scottambroseio · Pull Request #3563 · jestjs/jest

scottambroseio · 2017-05-13T18:23:23Z

Summary

Test plan

Run the 'can select a specific file name from the typeahead results' test in watch-filename-pattern-mode-test.js on both Windows and Unix based systems

gaearon · 2017-05-16T16:17:27Z

This fixes the issue in my testing. Can we please get it in?

thymikee · 2017-05-16T16:34:26Z

Feel free to stamp it and @cpojer will merge soon. We could cut a patch release for that I think, but would need to double check for breaking changes

cpojer · 2017-05-16T16:48:18Z

Yes we can do a patch with this tomorrow. Seems like this diff has lint errors, though. Can you fix them up and make CI pass?

thymikee · 2017-05-16T16:57:24Z

Although I'm not super happy we use a third party until just for that, but didn't have time to check what it does, maybe it's reasonable.

cpojer · 2017-05-16T16:58:30Z

Yeah, it's only used in the test file – can we inline it rather than adding a dependency?

Why can't we use require('jest-regex-util').replacePathSepForRegex; in the test as well?

cpojer · 2017-05-16T16:58:45Z

-const createContext = require('./lib/createContext');
 const isValidPath = require('./lib/isValidPath');
 const preRunMessage = require('./preRunMessage');
+const replacePathSepForRegex = require('jest-regex-util').replacePathSepForRegex;


nit: const {replacePathSepForRegex} = require('jest-regex-util');

scottambroseio · 2017-05-16T17:49:55Z

Hey, just caught up on this.

You can see my last two comments at #3516 which explains the idea behind regex-slash. I wasn't sure whether to inline it, or make it a dependency like slash. I went the dependency route as I figured being like slash, other's might find it useful. The source is here https://github.com/scottrangerio/regex-slash/blob/master/index.js, it's just a one line function,

module.exports = (pathPattern) => pathPattern.replace(/\\\\/g, '/');

so it would be super easy to inline. I'm not a master at regex, so I'm not sure if there's a more optimal / better regex pattern?

As for using replacePathSepForRegex in the test, this is the result.

The test uses a snapshot of a Unix style path pattern. So on windows, for the test to pass, we need a function of the form

// path\\to\\file.js -> path/to/file.js

to match the snapshot so the test passes on Windows.

I'll push the code review fix in a moment. I'm happy to add that helper function to jest-regex-util if that's preferred.

cpojer · 2017-05-16T18:21:26Z

-const createContext = require('./lib/createContext');
 const isValidPath = require('./lib/isValidPath');
 const preRunMessage = require('./preRunMessage');
+const { replacePathSepForRegex } = require('jest-regex-util');


nit: remove the spaces.

cpojer · 2017-05-16T18:21:57Z

Can we just inline this in the test? The function is not needed outside the test itself.

Proposal:

Inline the function.
Run yarn lint locally and fix the CI issues.

gaearon · 2017-05-16T20:18:57Z

@scottrangerio If you don't mind I'll fix up the nits and push to your branch.

scottambroseio · 2017-05-16T20:44:55Z

@gaearon Ah sorry, I only just saw this comment! I just pushed some code but feel free to take over from here on out! Cheers

gaearon · 2017-05-16T20:45:33Z

@@ -2,6 +2,27 @@
 # yarn lockfile v1



You can probably revert this file?

gaearon · 2017-05-16T20:47:41Z

  });

  it('can select a specific file name from the typeahead results', () => {
+    const toUnixPathPattern = pathPattern => pathPattern.replace(/\\\\/g, '/');


Can you explain why we need to replace two backslashes with a single slash? Where does the second backslash come from?

I guess because we're dealing with a pattern, so it needs an escape.

This was the original problem the function is trying to fix

Due to the backslashs needing escaping, the way I understand it is that behind the hood path\\to\\file is actually path\\\\to\\\\file, if that makes sense?

Edit: Yeah, you summarised it better than I could.

gaearon · 2017-05-16T23:03:56Z

Seems like this is ready. I would really appreciate a patch for this.

codecov-io · 2017-05-17T00:27:20Z

Codecov Report

Merging #3563 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3563      +/-   ##
=========================================
+ Coverage   62.39%   62.4%   +<.01%     
=========================================
  Files         181     181              
  Lines        6646    6647       +1     
  Branches        6       6              
=========================================
+ Hits         4147    4148       +1     
  Misses       2496    2496              
  Partials        3       3

Impacted Files	Coverage Δ
packages/jest-cli/src/watch.js	`76.29% <100%> (+0.17%)`	⬆️

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 6cf16f1...99d807b. Read the comment docs.

* 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 · 2021-05-13T15:07:20Z

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.

Scott Ranger added 4 commits May 8, 2017 18:08

Fix interactive watch arrow selection on Windows jestjs#3516

832822a

Refactored fix for jestjs#3516 to use replacePathSepForRegex

1100811

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

3ef22a0

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

347752a

facebook-github-bot added the CLA Signed ✔️ label May 13, 2017

gaearon added 2 commits May 16, 2017 17:16

Merge branch 'master' into windows-watch-mode-fix

c72f000

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

1303f8a

gaearon mentioned this pull request May 16, 2017

Fix Jest 20 Windows bug for test selection facebook/create-react-app#2169

Closed

cpojer reviewed May 16, 2017

View reviewed changes

Code review feedback

b71f1e7

cpojer reviewed May 16, 2017

View reviewed changes

Fixed lint errors and implemented PR feedback

5fa008f

gaearon reviewed May 16, 2017

View reviewed changes

Comment thread packages/jest-cli/yarn.lock

@@ -2,6 +2,27 @@

# yarn lockfile v1

Copy link
Copy Markdown

Contributor

gaearon May 16, 2017

Choose a reason for hiding this comment

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

You can probably revert this file?

gaearon reviewed May 16, 2017

View reviewed changes

Revert yarn.lock changes

99d807b

gaearon approved these changes May 16, 2017

View reviewed changes

cpojer merged commit d53f7c0 into jestjs:master May 17, 2017

renovate-bot mentioned this pull request May 17, 2017

Update dependency jest to version 20.0.4 renovatebot/renovate#208

Merged

renovate-bot mentioned this pull request May 17, 2017

Update dependency babel-jest to version 20.0.3 renovatebot/renovate#207

Merged

github-actions Bot locked as resolved and limited conversation to collaborators May 13, 2021

Uh oh!

Conversation

scottambroseio commented May 13, 2017

Uh oh!

gaearon commented May 16, 2017

Uh oh!

thymikee commented May 16, 2017

Uh oh!

cpojer commented May 16, 2017

Uh oh!

thymikee commented May 16, 2017

Uh oh!

cpojer commented May 16, 2017

Uh oh!

cpojer May 16, 2017

Choose a reason for hiding this comment

Uh oh!

scottambroseio commented May 16, 2017

Uh oh!

cpojer May 16, 2017

Choose a reason for hiding this comment

Uh oh!

cpojer commented May 16, 2017

Uh oh!

gaearon commented May 16, 2017

Uh oh!

scottambroseio commented May 16, 2017

Uh oh!

gaearon May 16, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon May 16, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon May 16, 2017

Choose a reason for hiding this comment

Uh oh!

scottambroseio May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented May 16, 2017

Uh oh!

codecov-io commented May 17, 2017

Codecov Report

Uh oh!

github-actions Bot commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scottambroseio May 16, 2017 •

edited

Loading