-
Notifications
You must be signed in to change notification settings - Fork 294
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
quoting default pathToJest #523
quoting default pathToJest #523
Conversation
Pull Request Test Coverage Report for Build 673
💛 - Coveralls |
@omjadas @stephtr @seanpoulter does any of you have a few minutes to review so we can close this issue and cut a new release soon... |
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.
looks good, only thing is that the shell option should probably be removed from startRunner in JestProcess.ts
@@ -79,7 +79,8 @@ export function pathToJest({ pathToJest, rootPath }: IPluginResourceSettings) { | |||
return 'npm test --' | |||
} | |||
|
|||
return getLocalPathForExecutable(rootPath, 'jest') || 'jest' + nodeBinExtension | |||
const p = getLocalPathForExecutable(rootPath, 'jest') || 'jest' + nodeBinExtension |
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.
I don't have it in mind, if we always use the shell, do we need nodeBinExtension
on Windows?
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.
I don't have a windows machine to verify, if it worked without, I can remove it... does it matter which shell is used? I could imagine users might want to pass their favor shell in the future...
Beside the shell option it looks good to me. |
granted this is not related to the code change, but since the coverage is complaining, might as well take this opportunity to boost up a zero test covered class.
@omjadas thanks for reminding me about the deprecated shell option, it's now fixed. @stephtr I am glad you are back! I agree we should remove the |
That's true, especially since we already had a few issues related to that. |
this is to follow up jest-community/jest-editor-support#21:
with
jest-editor-support
27.0.0 now able to pass raw pathToJest to the spawn call, users can embed escape characters or quotes when they customizejest.pathToJest
and our generated default jestToPath should do the same in case there is escape sequence in user's path.related issues:
#342
#360
#426
#489