Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Oct 17, 2024

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:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Oct 17, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e4f1f8c
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6711dbf207566d00086896e1
😎 Deploy Preview https://deploy-preview-6732--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa
Copy link
Contributor Author

@AriPerkkio I'm testing locally with vitejs/vite#16356 and it looks like this injection is not necessary anymore for --inspect-brk to stop inside the test file nicely. I guess it might still break if other transform breaks source map, but then that would be other plugins to blame. Maybe we can remove this if this is fixed on Vite? (that should be probably soon)

@AriPerkkio
Copy link
Member

Yup that would be great! If 'Debugger.setBreakpointByUrl' with { lineNumber: 0 } doesn't work automatically, we might need to get breakpoints with Debugger.getPossibleBreakpoints first.

@sheremet-va
Copy link
Member

Don't forget that we will keep supporting Vite versions that don't have this fix

@hi-ogawa hi-ogawa force-pushed the fix-inject-first-line-map-for-inspect-brk branch from de79736 to db8c0c7 Compare October 18, 2024 03:52
@hi-ogawa hi-ogawa mentioned this pull request Oct 18, 2024
6 tasks
@hi-ogawa hi-ogawa changed the title fix(vite-node): inject first line mapping only when --inspect-brk fix(vite-node,vitest): inject debugger for inspect-brk and remove fake sourcemap Oct 18, 2024
@hi-ogawa
Copy link
Contributor Author

I tried a different approach to inject debugger at the start of the test files. This seems to achieve virtually same behavior as manually calling Debugger.setBreakpointByUrl. What do you think?

@hi-ogawa hi-ogawa marked this pull request as ready for review October 18, 2024 04:32
Comment on lines +271 to +273
if (ctx.config.inspectBrk && ctx.state.filesMap.has(id)) {
return `debugger;${code}`
}
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

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. 🫠

Copy link
Contributor Author

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.

Copy link
Member

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.

@hi-ogawa hi-ogawa marked this pull request as draft October 26, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants