Improve debugging of tests #271
Conversation
Merge pull request #270 from stephtr/master
…into debug-npm-support
|
OK, I can't quite grok this in my head, will try find time over the weekend and test it locally |
| } | ||
| // We hope for the best that the filename doesn't contain any spaces and separate | ||
| // the basename from the arguments. | ||
| const args = basename.split(' ') |
There was a problem hiding this comment.
Adding an external dependency would probably be the best way for a correct separation of arguments (including quotes)?
There was a problem hiding this comment.
(For example https://www.npmjs.com/package/string-argv)
There was a problem hiding this comment.
The root cause of this problem is jest.pathToJest is ambiguous as a string, and hard to parse into the parameters we need to provide to child_process.spawn. Why not remove the problem by asking for the type signature we need: { cmd: String, args: String[] }.
You're going to see this again soon as a suggestion to fix the regression we've introduced on Windows.
There was a problem hiding this comment.
That would work for the settings, but not, if one sets it to npm test, in which case we would still end with the command string from package.json.
Which regression do you mean?
There was a problem hiding this comment.
I'm not sure I understand the issue. Is the problem that a user who has requested that we run the "npm test" script to start Jest is forcing us to find the path to the JavaScript file?
Would the args be passed to node if we spawned that command?
There was a problem hiding this comment.
I came up with three possible cases:
-
Project has been generated by
create-react-app:
In that casepathToJestshould be simplynpm test --, which runs (defined within package.json)react-scripts test(or similar). The problem is that runningreact-scriptswith a debugger attached isn't enough since it spawns a distinctive node process (without debugger). But sincereact-scriptsforwards all parameters beforetestto the node process it spawns, we can inject the correct debugger settings into that process by callingreact-scripts --inspect-brk=... test ...instead of justreact-scripts test .... Therefore in that specific case we have to parse the package.json file and run a modified (with an injected argument, not just appended ones) test command. -
Other projects which should use the test script from package.json:
If the user manually setspathToJesttonpm test --, we just have to run the test script. I think it should be possible to directly runnpm test -- ...with the debugger attached, but since the code for extracting the test script is already in place from (1), running it directly should be less complicate than dealing with npm. -
Projects which specify a script (relative path to a JS file or a npm binary, like jest)
In that case we just have to resolve the JS file which should be used (depending on the file extension; .js: run directly; .cmd: extract path to js file; no extension: extract parth to js file from shell script)
|
Hi, sorry I was late for this thread and many others earlier... Looks like recently there are a few CRA related issues (#244, #239), it might be a good time to take a step back and discuss if keep adding extra patterns and logic to handle this ever-widening landscape is the only way we should consider... JavaScript/Typescripts environments will always have lots of variations, platforms, libraries, and that is what made them great. I think we can all agree that vscode-jest can't resolve every possible variation people can run or debug jest tests. We need to draw a clear boundary for what vscode-jest can do, and provide flexible hooks to allow users adapt beyond. Otherwise, this tool might be risking in growing complexity, reducing stability and still falling short of addressing users' needs... in #244, I think @seanpoulter suggestion to use Furthermore, in this comment, @rrousselGit already had a working debug config. Similar to the argument above: why guess when the user already had the answer? Instead of keep fixing our internally generated debug config, I think we should focus on launching the debug session with the ability to take user provided debug config. This issue is even more obvious in #233 where @justinlevi used the debug config suggested in react-scripts doc. In that spirit, I think a more efficient "catch-all" solution is not to rely on can it be done for debug? I hope so... vscode recommended debug plugin implementation via DebugConfigurationProvider interface that can intercept and inject/modify debug config. The official example seems pretty straightforward and can satisfy our requirements:
A little bit more thoughts on possible implementation:
Sorry for a rather lengthy comment, just to provide an alternative way to reason about these issues/solutions, in hopes of keeping vscode-jest simple, lite, and stable while serving more use cases... |
I'd be glad to.
In my opinion that boundary would be to include support for the most common project generators. People mainly use CRA because of the unnecessity of dealing with configurations, in contrast to those using monorepos.
Ah, in the meantime the configuration has changed, so even for CRA apps it would be enough to just run
In which case would
That would be an additional option, but independently I still would like to achieve that this extension works for 90% of the most common setups without any configuration, if that would be possible by using the test script from package.json.
In my opinion (that was also my first impression when using this extension) documentation should be definitely expanded. One alternative way I could think of is to ask the user a few questions when opening a project containing jest for the first time, similar to trying to debug a new project for the first time, like: Independent of the decision I would like to add a setting |
To keep up with external library/platform/generator is not trivial. I still remember when jest 21.0.0 moved to the new config format, all vscode-jest users with new jest were broken and blocked until we can release the patch. Imagine after we commit this PR and CRA changed the config, now do we need to add more logic to detect CRA version in order to pass the right params? It's a slippery slope... dependency is expensive, lesser the better. Jest is a necessary dependency, but nothing else is. While we all want to be as user-friendly as possible, we need to be conscious of the price we will be paying.
Does this mean we can rollback #266 and
perfect, I am perfectly fine with whatever default value we decide for pathToJest. Does this mean that we can also get rid of
if pathToJest defaults to
Not sure what is the problem here? The user doesn't have to set
custom debug config, like pathToJest.debug, is optional:
This might be a reasonable alternative. However, users might change their packages after the initial opening, so would need to update the config accordingly. Thus you might want to make it a standalone executable, like |
One of the ideas of CRA is that they hide all of the new features and configuration changes behind their
Concerning running the tests (in background)
I'm fine with adding such a setting (or including it in the debug-config), I just thought that in such case
Unfortunately not. For most targets we still have to resolve the path to the executable js file. CRA apps are the exception, those scripts can be run directly.
I would reckon to have a default setting, which would work with plain jest and with CRA apps. However comparing the debug config for jest only and jest on CRA (one time
Those input dialogs would only configure VSCode's settings, if one changes packages, he still would be able to change those settings by hand. |
|
Let's get back to the code... @stephtr, will you be interested in taking a shot at clean-up and moving the debug functionality into DebugConfigurationProvider? This will pave the way for further debug enhancement such as supporting user-defined debug config. |
|
Wouldn't it be less work to directly include user-defined debug configurations? Did I understand it correctly, we would create a DebugConfigurationProvider which automatically appends the flags specifying the test to be run, the user creates a suitable debug configuration with a specific name and then we would run that configuration as soon as the user clicks "Debug test"? Unfortunately I won't be available the coming days, but I could try it the week after next. |
|
@stephtr yes you got the idea. For people who already have the debug config, that is exactly what it should do. However, like you have rightfully argued that many others will not have debug config and we should provide a solution for them as well. Good news is we already have most of that in the current implementation, just need to clean up and consolidate to DebugConfigurationProvider, so we have a single entry for debug functionality. I am looking forward to seeing the updates 👍 |
| port, | ||
| protocol: 'inspector', | ||
| console: 'integratedTerminal', | ||
| smartStep: true, | ||
| sourceMaps: true, | ||
| env, | ||
| } |
There was a problem hiding this comment.
It'd be helpful to also let us override runtimeExecutable, for cases where we need to pass --inspect-brk through to a child process (rather than attempting to debug a wrapper script)
(example use case: https://github.com/nevir/lib-boundless/blob/master/scripts/jest.js)
There was a problem hiding this comment.
That would be the plan, since that is also what we need for supporting create-react-app apps.
|
Sorry that it took me so long for finding time. |
Closed in favor of #287
Previously debugging of tests was only possible when
pathToJestreferenced to the jest binary.This PR should enable support for other JS files or
npm test --, including projects which have been generated usingcreate-react-app. (fixes #239 and in most cases #193)Since this change misses quite for sure some corner cases, I would be happy for some pairs of eyes looking over it or trying it out, even though this change shouldn't degrade support.111