-
Notifications
You must be signed in to change notification settings - Fork 295
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
Improve debugging of tests #271
Changes from all commits
6c71475
a08fad4
921a243
fd1a4bc
851a0f1
8b3e785
6ee4d8d
a564d07
b395384
7bf457a
76a6b0d
f199f2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { | |
TestResult, | ||
resultsWithLowerCaseWindowsDriveLetters, | ||
} from './TestResults' | ||
import { pathToJestPackageJSON, pathToJest, pathToConfig } from './helpers' | ||
import { pathToJestPackageJSON, pathToJest, pathToConfig, testCommandIsCreateReactApp } from './helpers' | ||
import { readFileSync } from 'fs' | ||
import { CoverageMapProvider } from './Coverage' | ||
import { updateDiagnostics, resetDiagnostics, failedSuiteCount } from './diagnostics' | ||
|
@@ -445,79 +445,121 @@ export class JestExt { | |
/** | ||
* Primitive way to resolve path to jest.js | ||
*/ | ||
private resolvePathToJestBin() { | ||
private resolveTestProgram(): { program: string; args: string[]; isCreateReactApp: boolean } { | ||
// Let's start with the given command | ||
let jest = this.workspace.pathToJest | ||
let basename = path.basename(jest) | ||
let isCreateReactApp = false | ||
|
||
if (basename.indexOf('npm test') === 0 || basename.indexOf('npm.cmd test') === 0) { | ||
// We can't always debug running npm test, therefore we have to dig deeper. | ||
try { | ||
const packagePath = path.join(vscode.workspace.rootPath, 'package.json') | ||
const packageJSON = JSON.parse(readFileSync(packagePath, 'utf8')) | ||
if (!packageJSON || !packageJSON.scripts || !packageJSON.scripts.test) { | ||
throw 'invalid package.json' | ||
} | ||
// If the initial command contained additional parameters (separated by `--`), | ||
// add them to the test command as if npm would do it. | ||
const args = basename.split('0').slice(3) | ||
basename = packageJSON.scripts.test + ' ' + args.join(' ') | ||
// If the test script given in package.json hasn't the form of a relative path, | ||
// we assume that it resides in `node_modules/.bin`. | ||
if (basename.substr(0, 1) !== '.') { | ||
jest = 'node_modules/.bin/' + basename | ||
} | ||
isCreateReactApp = testCommandIsCreateReactApp(packageJSON.scripts.test) | ||
} catch { | ||
vscode.window.showErrorMessage("package.json couldn't be read!") | ||
return undefined | ||
} | ||
} | ||
|
||
if (!path.isAbsolute(jest)) { | ||
jest = path.join(vscode.workspace.rootPath, jest) | ||
} | ||
|
||
const basename = path.basename(jest) | ||
switch (basename) { | ||
case 'jest.js': { | ||
return jest | ||
} | ||
// We hope for the best that the filename doesn't contain any spaces and separate | ||
// the basename from the arguments. | ||
const args = basename.split(' ') | ||
basename = args.shift() | ||
const extension = path.extname(basename) | ||
jest = path.join(path.dirname(jest), basename) | ||
|
||
case 'jest.cmd': { | ||
/* i need to extract '..\jest-cli\bin\jest.js' from line 2 | ||
let program = '' | ||
|
||
@IF EXIST "%~dp0\node.exe" ( | ||
"%~dp0\node.exe" "%~dp0\..\jest-cli\bin\jest.js" %* | ||
) ELSE ( | ||
@SETLOCAL | ||
@SET PATHEXT=%PATHEXT:;.JS;=;% | ||
node "%~dp0\..\jest-cli\bin\jest.js" %* | ||
) | ||
*/ | ||
const line = fs.readFileSync(jest, 'utf8').split('\n')[1] | ||
const match = /^\s*"[^"]+"\s+"%~dp0\\([^"]+)"/.exec(line) | ||
return path.join(path.dirname(jest), match[1]) | ||
} | ||
try { | ||
// We would like to run a JavaScript file, so let us look for one. | ||
switch (extension) { | ||
case 'js': { | ||
program = jest | ||
break | ||
} | ||
|
||
case 'jest': { | ||
/* file without extension uses first line as file type | ||
in case of node script i can use this file directly, | ||
in case of linux shell script i need to extract path from line 9 | ||
#!/bin/sh | ||
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") | ||
|
||
case `uname` in | ||
*CYGWIN*) basedir=`cygpath -w "$basedir"`;; | ||
esac | ||
|
||
if [ -x "$basedir/node" ]; then | ||
"$basedir/node" "$basedir/../jest-cli/bin/jest.js" "$@" | ||
ret=$? | ||
else | ||
node "$basedir/../jest-cli/bin/jest.js" "$@" | ||
ret=$? | ||
fi | ||
exit $ret | ||
*/ | ||
const lines = fs.readFileSync(jest, 'utf8').split('\n') | ||
switch (lines[0]) { | ||
case '#!/usr/bin/env node': { | ||
return jest | ||
} | ||
case 'cmd': { | ||
/* i need to extract '..\jest-cli\bin\jest.js' from line 2 | ||
|
||
@IF EXIST "%~dp0\node.exe" ( | ||
"%~dp0\node.exe" "%~dp0\..\jest-cli\bin\jest.js" %* | ||
) ELSE ( | ||
@SETLOCAL | ||
@SET PATHEXT=%PATHEXT:;.JS;=;% | ||
node "%~dp0\..\jest-cli\bin\jest.js" %* | ||
) | ||
*/ | ||
const line = fs.readFileSync(jest, 'utf8').split('\n')[1] | ||
const match = /^\s*"[^"]+"\s+"%~dp0\\([^"]+)"/.exec(line) | ||
program = path.join(path.dirname(jest), match[1]) | ||
break | ||
} | ||
|
||
case '#!/bin/sh': { | ||
const line = lines[8] | ||
const match = /^\s*"[^"]+"\s+"\$basedir\/([^"]+)"/.exec(line) | ||
if (match) { | ||
return path.join(path.dirname(jest), match[1]) | ||
case '': { | ||
/* file without extension uses first line as file type | ||
in case of node script i can use this file directly, | ||
in case of linux shell script i need to extract path from line 9 | ||
#!/bin/sh | ||
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") | ||
|
||
case `uname` in | ||
*CYGWIN*) basedir=`cygpath -w "$basedir"`;; | ||
esac | ||
|
||
if [ -x "$basedir/node" ]; then | ||
"$basedir/node" "$basedir/../jest-cli/bin/jest.js" "$@" | ||
ret=$? | ||
else | ||
node "$basedir/../jest-cli/bin/jest.js" "$@" | ||
ret=$? | ||
fi | ||
exit $ret | ||
*/ | ||
const lines = fs.readFileSync(jest, 'utf8').split('\n') | ||
switch (lines[0]) { | ||
case '#!/usr/bin/env node': { | ||
program = jest | ||
break | ||
} | ||
|
||
break | ||
case '#!/bin/sh': { | ||
const line = lines[8] | ||
const match = /^\s*"[^"]+"\s+"\$basedir\/([^"]+)"/.exec(line) | ||
if (match) { | ||
program = path.join(path.dirname(jest), match[1]) | ||
} | ||
break | ||
} | ||
} | ||
} | ||
|
||
break | ||
break | ||
} | ||
} | ||
} catch { | ||
vscode.window.showErrorMessage('Cannot debug jest command!') | ||
return undefined | ||
} | ||
|
||
case 'npm test --': | ||
case 'npm.cmd test --': { | ||
vscode.window.showErrorMessage('Debugging of tasks is currently only available when directly running jest!') | ||
return undefined | ||
} | ||
if (program !== '') { | ||
return { program, args, isCreateReactApp } | ||
} | ||
|
||
vscode.window.showErrorMessage('Cannot find jest.js file!') | ||
|
@@ -527,32 +569,54 @@ export class JestExt { | |
public runTest = (fileName: string, identifier: string) => { | ||
const restart = this.jestProcessManager.numberOfProcesses > 0 | ||
this.jestProcessManager.stopAll() | ||
const program = this.resolvePathToJestBin() | ||
if (!program) { | ||
const launcher = this.resolveTestProgram() | ||
if (!launcher) { | ||
console.log("Could not find Jest's CLI path") | ||
return | ||
} | ||
|
||
const escapedIdentifier = JSON.stringify(identifier).slice(1, -1) | ||
|
||
const args = ['--runInBand', fileName, '--testNamePattern', escapedIdentifier] | ||
let args = launcher.args | ||
// These settings identify the test we would like to run. | ||
args.push('--runInBand', fileName, '--testNamePattern', escapedIdentifier) | ||
if (this.pluginSettings.pathToConfig.length) { | ||
args.push('--config', this.pluginSettings.pathToConfig) | ||
} | ||
|
||
const port = Math.floor(Math.random() * 20000) + 10000 | ||
let runtimeArgs = ['--inspect-brk=' + port] | ||
const env: any = {} | ||
|
||
if (launcher.isCreateReactApp) { | ||
// If the project has been created by create-react-app, | ||
// things get a little bit complicated. | ||
// Running `react-scripts test` starts Jest within a separately spawned | ||
// process and we want to debug that process, not the outer | ||
// `react-scripts` process. | ||
// We therefore run `react-scripts --inspect-brk=... test`, because | ||
// `react-scripts` interprets the arguments before `test` as launching | ||
// arguments for generating the new node process. | ||
args = runtimeArgs.concat(args) | ||
runtimeArgs = [] | ||
// `react-scripts` automatically appends the `--watch` flag | ||
// if CI isn't set. | ||
env.CI = 'VSCode-test-debugger' | ||
} | ||
|
||
const configuration = { | ||
name: 'TestRunner', | ||
type: 'node', | ||
request: 'launch', | ||
program, | ||
program: launcher.program, | ||
args, | ||
runtimeArgs: ['--inspect-brk=' + port], | ||
runtimeArgs: runtimeArgs, | ||
port, | ||
protocol: 'inspector', | ||
console: 'integratedTerminal', | ||
smartStep: true, | ||
sourceMaps: true, | ||
env, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be helpful to also let us override (example use case: https://github.com/nevir/lib-boundless/blob/master/scripts/jest.js) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be the plan, since that is also what we need for supporting create-react-app apps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sweet |
||
|
||
const handle = vscode.debug.onDidTerminateDebugSession(_ => { | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(For example https://www.npmjs.com/package/string-argv)
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.
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 tochild_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with three possible cases:
Project has been generated by
create-react-app
:In that case
pathToJest
should be simplynpm test --
, which runs (defined within package.json)react-scripts test
(or similar). The problem is that runningreact-scripts
with a debugger attached isn't enough since it spawns a distinctive node process (without debugger). But sincereact-scripts
forwards all parameters beforetest
to 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 sets
pathToJest
tonpm 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)