-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(vite-node,vitest): inject debugger for inspect-brk and remove fake sourcemap #6732
base: main
Are you sure you want to change the base?
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 |
de79736
to
db8c0c7
Compare
--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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.