fix(vite-node,vitest): inject debugger for inspect-brk and remove fake sourcemap #6732
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@AriPerkkio I'm testing locally with vitejs/vite#16356 and it looks like this injection is not necessary anymore for |
|
Yup that would be great! If |
|
Don't forget that we will keep supporting Vite versions that don't have this fix |
hi-ogawa
force-pushed
the
fix-inject-first-line-map-for-inspect-brk
branch
from
de79736
to
db8c0c7
Compare
hi-ogawa
changed the title
--inspect-brk
|
I tried a different approach to inject |
| if (ctx.config.inspectBrk && ctx.state.filesMap.has(id)) { | ||
| return `debugger;${code}` | ||
| } |
There was a problem hiding this comment.
This is not ideal solution. Adding this will make debugger stop on every test file. When using --inspect-brk, we want to stop only on the first test file.
Could we instead use magic-string's addSourcemapLocation here?
There was a problem hiding this comment.
When using
--inspect-brk, we want to stop only on the first test file.
Isn't it intended to break each test file? On main, I tested the example like this and it breaks twice on chrome node debugger.
$ pnpm -C examples/basic test -- --inspect-brk --no-file-parallelism
> @vitest/example-test@ test /home/hiroshi/code/others/vitest/examples/basic
> vitest "--inspect-brk" "--no-file-parallelism"
DEV v2.1.3 /home/hiroshi/code/others/vitest/examples/basic
Debugger listening on ws://127.0.0.1:9229/0bdc47a0-344e-4d23-8e04-f1fd797442bf
For help, see: https://nodejs.org/en/docs/inspector
Debugger attached.
✓ test/basic.test.ts (3)
Debugger listening on ws://127.0.0.1:9229/30404ff7-fa17-4676-be16-6bbb6ad51d61
For help, see: https://nodejs.org/en/docs/inspector
✓ test/basic.test.ts (3)
✓ test/suite.test.ts (3)
Test Files 2 passed (2)
Tests 6 passed (6)
Start at 15:43:22
Duration 14.79s (transform 32ms, setup 0ms, collect 9.25s, tests 7ms, environment 0ms, prepare 5.40s)
There was a problem hiding this comment.
It depends on isolate flag I think.
We want to mimic how node --inspect-brk works. We can't simply use node --inspect-brk as it doesn't work with node:worker_threads so we need to build it ourselves like this. 🫠
There was a problem hiding this comment.
Right, --poolOptions.forks.singleFork makes it break only once on main.
But, aside from whether we should avoid injecting debugger;, I thought the idea of Vitest's --inspect-brk is to break for each file. Even for child process, actual --inspect-brk wouldn't make sense since it might break in test runner internal code, so choosing a test file as entry makes sense and --inspect-brk was just for user's familiarity.
There was a problem hiding this comment.
Yep, that's exactly the idea.
I've been looking into other solutions for a while and can't come up with one. Maybe we should make sure vite-node's injected wrapper contains mapping for debuggers to stop in?
If we want to use debugger; injection instead, we should probably do the same on browser mode.
Description
related vitejs/vite#18329, vitejs/vite#16356, #6696
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.