-
Notifications
You must be signed in to change notification settings - Fork 293
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
Fix decorators on windows by adjusting implicit jest path based on OS #324
Fix decorators on windows by adjusting implicit jest path based on OS #324
Conversation
Pull Request Test Coverage Report for Build 357
💛 - Coveralls |
Yeah, cool, this looks 👍 to me - thanks! |
@@ -61,15 +61,24 @@ function hasNodeExecutable(rootPath: string, executable: string): boolean { | |||
* @returns {string} | |||
*/ | |||
export function pathToJest(pluginSettings: IPluginSettings) { | |||
const path = normalize(pluginSettings.pathToJest) | |||
if (pluginSettings.pathToJest) { |
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.
What's your intention for changing this line @ThomasRooney? It seems to me like we've changed it from "if this is still the default setting" to "if the default setting has changed".
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 removed the default setting for pathToJest
From the package.json. I did this because I didn’t want to break any user configuration where they’d predefined it, but the default pathToJest
defined before was incorrect for windows systems (forcing Windows users to explicitly define it).
With pathToJest
now potentially being null, I wanted to maintain old behaviour for linux/osx users, and my interpretation is that each code path used for the old version is equivalent in the new version, but for the windows scenario. In this particular line, I’m mapping the path !== defaultPath
scenario for OSX users, which returned normalize(pluginSettings.pathToJest)
.
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.
E.g. path === defaultPath
=> either npm test —
or normalize(‘node_modules/.bin/jest’)
path !== defaultPath
=> normalize(path)
@stephtr good point! This additional condition came from However my understanding of #297 is it would break windows again, because even if jest supports a different shell, the |
By starting the commands in a Windows cmd (simply by supplying |
I’ve got a PR in progress if you can wait another half day. 🐌 |
…s-jest-location Fix decorators on windows by adjusting implicit jest path based on OS
The green/red dots are currently broken on windows if jest path isn't configured.
The jest path selected is always a shell
jest
(linux/mac) script, rather than a feature based on current operating system, and potentially running thejest.cmd
file. As such it crashes when attempting to run in windowsENOENT
, because windows cannot evaluate shell scripts.This fixes that.
Not tested on Mac/Linux. Please confirm this doesn't regress there if you decide to merge.
This closes quite a few bugs that are correlated because the
jest --getConfig
command wasn't working on windows. FYI for a current workaround, just define jest path explicitly.Closes #274 #320 #306 #289