fixed handling of quoted commands with spaces

[omjadas]

fixes jest-community/vscode-jest#342
fixes jest-community/vscode-jest#360
fixes jest-community/vscode-jest#426
fixes jest-community/vscode-jest#489
#trivial

[connectdotz]

This is one of the high priority bugs on our todo list for a while, thanks for tackling it 👍

For the most part, the regex pattern worked, we could merge as it is but figure might as well take the opportunity to see if we can simplify it a bit to make future maintenance easier...

While the code change is trivial, the impact is not, just see how many issues it could resolve! I would encourage you do update the CHANGELOG.md to record this change, there will be many users want to thank you for it! 😄

[connectdotz]

I think I get what you are trying to do, but it is a bit complex for the mere mortal: nested non-capture-groups, [\S\s] (isn't white-space + non-white-space = well, everything?)... I was having a hard time following, wondering if it is possible to simplify it, came up with this

/(\S*".*?")|(\S*'.*?')|(\S*\\\s\S+)|\S+/g

there are really just 3 patterns we want to capture beyond the non-white-space: single, double quotes and backslash escape... tested with the jest and the following, both seem to work:

"/a/b is a directory/c" --without /x/y/z useThis="I am a 'nested single quote' test" single='this is a single quote'  backslash=a\\ space/b/c 

however, the nested quotes cause problems for both regex patterns:

--nestedQuote="before \\"after this\\" end" --nestedSingleQuote='before \\'after this\\' end'

while this can be easily fixed with lookbehind (I am sure you can also fix your regex pattern), it does start to get more complicated and ugly... I can be talked into not to support it as I don't see how this will occur in jest command line...

/(\S*".*?(?<!\\)")|(\S*'.*?(?<!\\)')|(\S*\\\s\S+)|\S+/g

anyway, just to throw some idea out there to see if we can get it simplified a bit...

BTW, to make future maintenance easier, how about adding some comments to explain the regex patterns?

[omjadas]

The reason for [\S\s] is that it will match newlines, whereas . will not. This is in case of something like this

"/a/b is a directory/c" --without /x/y/z useThis="I am a 'nested 
single quote' test" single='this is a single quote'  backslash=a\\ space/b/c

Also, I'm pretty sure that

--nestedQuote="before \\"after this\\" end"

should be two matches. Whereas

--nestedQuote="before \"after this\" end"

should be one match.

I noticed that the regex you suggested split

"/a/b is\" adirectory/c"

into two matches. It's unlikely that someone would have a quote in their path, but it is possible.

Not sure how to simplify it and still support these cases.

[connectdotz]

just to add one more note so we won't forget. once this PR is merged, we should patch the vscode-jest to make sure the default jest.pathToJest path is quoted so space will not be an issue and people don't need to override jest.pathToJest to work around this issue anymore...

[connectdotz]

I noticed that the regex you suggested split
"/a/b is" adirectory/c"
into two matches.

hmmm, did you use the first regex pattern that I stated doesn't support nested quote? or the 2nd pattern that does: /(\S*".*?(?<!\\)")|(\S*'.*?(?<!\\)')|(\S*\\\s\S+)|\S+/g ? because the later seems to work just fine, ins't it?

The reason for [\S\s] is that it will match newlines

I see. I think it is pretty safe not to support the newline for our use case...

---

In any case, optimization or not, the most important thing is that the process needs to run... I tested this PR with vscode-jest on mac and looks like we have another issue...The arguments are indeed no longer split on space, but the spawned process failed:

for example, for the settings: "jest.pathToJest": "yarn test --config='/Users/x/dir with space/jest.config.js'",.

Before spawning, the arguments looked all right to me:

spawning process with command=yarn, args=test,--config='/Users/x/dir with space/jest.config.js',--testLocationInResults,--json,--useStderr,--outputFile,/var/folders/vg/yw9mltwd1mj_29xbvhwywc980000gn/T/jest_runner_jest_editor_support.json,--watch,--no-color

but the spanwed process error out:

$ jest '--config='\''/Users/vsun/github/sandbox/dir with space/jest-editor-support/jest.config.js'\' --testLocationInResults --json --useStderr --outputFile /var/folders/vg/yw9mltwd1mj_29xbvhwywc980000gn/T/jest_runner_jest_editor_support.json --no-color

Usage: jest [--config=<pathToConfigFile>] [TestPathPattern]
...

Apparently child_process.spawn is quoting the whole config argument as a string, and escaping the inner quotes that jest simply doesn't understand...

also tried (single/double) escape space hoping to by-pass quote problem above but without success...

thoughts?

[omjadas]

I think setting shell: true might fix it. I can give it a go over the weekend, don't have access to macOS though.

[omjadas]

Setting shell: true seems to fix it for the case that you have given. Is there a reason that is only being set to true for Windows in vscode-jest at the moment?

[seanpoulter]

To say it again, thank you @omjadas for tackling this bug! 👍

--

Is there a reason that is only being set to true for Windows in vscode-jest at the moment?

That change sounds familiar to me. I think the change was introduced to solve a problem that was only on Windows. We didn't think about non-Windows systems or have as much capacity to test it. Here's the PR that added it.

--

If we're still having problems trying to be clever and parse a user-provided string into the command and args for spawn(command: string, args: string[], ...), I'd like to make a few suggestions:

• We should extract this transformation into a small testable function.
• We should capture all your great examples in the PR comments in our tests.

[connectdotz]

Is there a reason that is only being set to true for Windows in vscode-jest at the moment?

the shell option comes with its overhead (the shell) and security risk:

If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

Therefore, no reason to use it unless we absolutely need it...

Having say that, if we can't find a better alternative and must use shell, I think it might be all right, provided we do some tests:

1. the security risk is probably acceptable since the processes are executed in users' own environment.
2. we can probably remove the command args parsing all together and just pass the full command string to spawn (test needed)
3. not sure how much overhead with the extra shell, especially in multi-root workspace where we alreadh spawn quite a few jest processes... (test needed)

[connectdotz]

@omjadas not sure if you have a chance to test the shell option yet, I did a quick and dirty test with your suggestion: enable "shell" option, then removing all parsing/splitting logic, appending the extra args at the end of the runtimeExecutable, and simply invoke spawn like spawn(commandWithArgs, [], spawnOptions), it seems to work just fine.

do you have any concerns? do you think you will have some time to try this new approach?

[omjadas]

No immediate concerns jump out to me. I'll be able to do some testing over the weekend.

[omjadas]

I've done some testing and I was not able to determine a difference in performance when setting the shell option in multi-root workspaces.

Did we want to remove the options argument, or leave it in case we want to pass other options to spawn in the future?

[connectdotz]

Did we want to remove the options argument, or leave it in case we want to pass other options to spawn in the future?

I think we can leave it and maybe even check if the options.shell is set explicitly to "false", we could override the default "yes" as a way to turn-off the shell... but I admit that I can't think of an actual use case, other than testing and emergency workaround, if needed...

---

@omjadas btw, did you get a chance to test this approach on windows?

@seanpoulter and @stephtr, this is sort of a high impact change, do you guys have anything else to add? To be safe, it would be great if we can all test it in our local build for a few days before merge...

[omjadas]

@connectdotz I was able to test with both windows and linux.

If we want to be able to explicitly set the shell option to false, should we re-add the arg splitting on spaces in that case?

[connectdotz]

If we want to be able to explicitly set the shell option to false, should we re-add the arg splitting on spaces in that case?

great point, this changed my mind, let's just remove the options. Please also clean up types such as index.d.ts.

[omjadas]

I've removed spawnOptions and also the shell option from a few other places it occurred

[connectdotz]

looks good to me with just a minor suggestion. I have tested this in my repo for a few days, it seems to work fine, although we do need some other PR to fix the default pathToJest function, but that is beyond the scope of this PR.

[connectdotz]

while using string += worked, one has to be mindful about adding space separators manually. It might be simpler to just push all command and arguments into an array, then use the array.join(' ') to construct the final command line?

[omjadas]

thanks for the suggestion