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.

Sign up for GitHub

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
Draft

fix(vite-node,vitest): inject debugger for inspect-brk and remove fake sourcemap #6732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46f07aa
fix(vite-node,vitest): inject debugger for inspect-brk and remove fak…
hi-ogawa Oct 18, 2024
db8c0c7
test: update test
hi-ogawa Oct 18, 2024
e4f1f8c
chore: comment
hi-ogawa Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/vite-node/src/source-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,6 @@ export function withInlineSourcemap(
code = code.replace(OTHER_SOURCE_MAP_REGEXP, '')
}

// If the first line is not present on source maps, add simple 1:1 mapping ([0,0,0,0], [1,0,0,0])
// so that debuggers can be set to break on first line
if (map.mappings.startsWith(';')) {
map.mappings = `AAAA,CAAA${map.mappings}`
}

const sourceMap = Buffer.from(JSON.stringify(map), 'utf-8').toString(
'base64',
)
Expand Down
12 changes: 11 additions & 1 deletion packages/vitest/src/node/plugins/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { UserConfig as ViteConfig, Plugin as VitePlugin } from 'vite'
import type { Plugin, UserConfig as ViteConfig, Plugin as VitePlugin } from 'vite'
import { relative } from 'pathe'
import {
deepMerge,
Expand Down Expand Up @@ -263,6 +263,16 @@ export async function VitestPlugin(
...MocksPlugins(),
VitestOptimizer(),
NormalizeURLPlugin(),
{
name: 'inject-inspect-brk-debugger',
transform(code, id) {
// TODO: this is not necessary if Vite SSR preserves source map for hoisted imports
// https://github.com/vitejs/vite/pull/16356
if (ctx.config.inspectBrk && ctx.state.filesMap.has(id)) {
return `debugger;${code}`
}
},
Comment on lines +271 to +273
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.

} satisfies Plugin,
].filter(notNullish)
}
function removeUndefinedValues<T extends Record<string, any>>(
Expand Down
17 changes: 0 additions & 17 deletions packages/vitest/src/runtime/inspector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { createRequire } from 'node:module'
import { pathToFileURL } from 'node:url'
import type { ContextRPC } from '../types/worker'
import type { SerializedConfig } from './config'

Expand All @@ -26,22 +25,6 @@ export function setupInspect(ctx: ContextRPC) {
config.inspector.host,
config.inspector.waitForDebugger,
)

if (config.inspectBrk) {
const firstTestFile = ctx.files[0]

// Stop at first test file
if (firstTestFile) {
session = new inspector.Session()
session.connect()

session.post('Debugger.enable')
session.post('Debugger.setBreakpointByUrl', {
lineNumber: 0,
url: pathToFileURL(firstTestFile),
})
}
}
}
}

Expand Down
14 changes: 8 additions & 6 deletions test/config/test/failures.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,14 @@ test('coverage.autoUpdate cannot update thresholds when configuration file doesn
})

test('boolean flag 100 should not crash CLI', async () => {
const { stderr } = await runVitestCli('--coverage.enabled', '--coverage.thresholds.100')

expect(stderr).toMatch('ERROR: Coverage for lines (0%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for functions (0%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for statements (0%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for branches (0%) does not meet global threshold (100%)')
let { stderr } = await runVitestCli('--coverage.enabled', '--coverage.thresholds.100')
// TODO: for some reason, non-zero coverage shows up, which is non-deterministic, so strip it.
stderr = stderr.replace(/\([0-9.]+%\) does/g, '(-%) does')

expect(stderr).toMatch('ERROR: Coverage for lines (-%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for functions (-%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for statements (-%) does not meet global threshold (100%)')
expect(stderr).toMatch('ERROR: Coverage for branches (-%) does not meet global threshold (100%)')
})

test('nextTick cannot be mocked inside child_process', async () => {
Expand Down
12 changes: 6 additions & 6 deletions test/coverage-test/test/vue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,16 @@ test('coverage results matches snapshot', async () => {
expect({ lines, statements }).toMatchInlineSnapshot(`
{
"lines": {
"covered": 36,
"pct": 81.81,
"covered": 34,
"pct": 80.95,
"skipped": 0,
"total": 44,
"total": 42,
},
"statements": {
"covered": 36,
"pct": 81.81,
"covered": 34,
"pct": 80.95,
"skipped": 0,
"total": 44,
"total": 42,
},
}
`)
Expand Down
Loading