Repository navigation
Conversation
…nd events When TestReporter.enable arrives after test collection has started, the retroactive walker emits TestReporter.found for each already-discovered test using the line number stored on BaseScope. That field was only populated when the JUnit reporter was enabled, so retroactively reported tests carried line: 0 (and so landed at the top of the file in IDE test explorers) unless --reporter=junit happened to be on. Capture the line number at registration time whenever an inspector is attached as well, so the retroactive path has a real source location to report.
|
Status: diff is green. Remaining CI failures are all tagged Ready for review. |
WalkthroughThe test runner now captures test line numbers when inspector mode is enabled. The inspector reporter regression test verifies source URLs and line numbers for retroactively reported tests. ChangesInspector test location reporting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — one-line condition widening so capture_test_line_number also runs when the inspector is attached, fixing line: 0 in retroactive TestReporter.found events.
What was reviewed:
- Verified
is_inspector_enabled()readsvm.debugger.is_some(), which is set for--inspect-waitbefore collection runs. - Confirmed the retroactive walk in
jsc_hooks.rs:1759,1786readsbase.line_no, so populating it is the right layer. - Checked the exact-line assertion (
testA1?.line === 6) against the fixture's template-literal line numbering. - No-inspector/no-junit path still skips the stack walk (perf-neutral for the common case).
Extended reasoning...
Overview
This PR widens a single condition in capture_test_line_number (src/runtime/test_runner/jest.rs:821) from reporters.junit to reporters.junit || vm.is_inspector_enabled(), so that test/describe registration captures the source line number whenever an inspector is attached — not only when the JUnit reporter is on. It also updates the doc comment on BaseScope::line_no to match, and extends the existing retroactive-enable test in test/cli/inspect/test-reporter.test.ts to assert that every retroactively reported TestReporter.found event carries a real url and line > 0, plus an exact-line spot check.
Security risks
None. This is a debugger/inspector metadata path; the added call is a boolean read (vm.debugger.is_some()) and a stack-frame line-number lookup that already ran under --reporter=junit.
Level of scrutiny
Low. The functional change is one boolean disjunct in a gate that already existed. I verified:
is_inspector_enabled()atsrc/jsc/VirtualMachine.rs:984returnsself.debugger.is_some(), which is populated when--inspect/--inspect-waitinitializes the debugger — so it is true during collection in the fixture.- The consumer (
retroactively_report_scopeinsrc/runtime/jsc_hooks.rs:1753,1780) readsbase.line_noand passes it toreport_test_found_with_location, so populatingline_nois the correct fix layer. - The common case (
bun testwith no inspector, no junit) still short-circuits before the FFI stack walk, so there is no perf regression on the hot path. - The
Jest::runner()borrow followed byglobal_this.bun_vm()is the pre-existing pattern here; the new call only reads a disjoint VM field and does not touchTestRunner.
Other factors
The test extension is added alongside (not replacing) existing assertions in the retroactive-enable test, and includes both a broad invariant (line > 0 for all 5 entries, correct url) and an exact-line spot check to catch off-by-one regressions. The PR body documents fail-before/pass-after with released vs. debug bun. The comment update on BaseScope::line_no keeps the field doc accurate.
There was a problem hiding this comment.
LGTM — one-line gate widening with clear fail-before/pass-after evidence.
What was reviewed:
- Confirmed
is_inspector_enabled()isdebugger.is_some(), which is set before collection under--inspect-wait/--inspect, soline_nois captured for the retroactive walk injsc_hooks.rs:1759/1786. - Checked the gate is intentionally broader than the sibling
test_id_for_debuggergate (agent.is_enabled()) — that's the point: capture must happen beforeTestReporter.enablelands. - Verified the exact-line assertion (
test A1on line 6) against the fixture's leading newline.
Extended reasoning...
Overview
Two-line runtime change plus test assertions. capture_test_line_number in src/runtime/test_runner/jest.rs previously only walked the stack for a source line when --reporter=junit was on; it now also does so when vm.debugger.is_some(). The doc comment on BaseScope::line_no in bun_test.rs is updated to match. The existing retroactive-enable test in test/cli/inspect/test-reporter.test.ts gains assertions that every retroactively reported TestReporter.found event carries url ending in the fixture filename and line > 0, plus an exact-line spot check.
Security risks
None. This only affects when a stack-walk for a source line number is performed during test()/describe() registration, gated on the presence of a debugger. No user-controlled input, no new FFI surface.
Level of scrutiny
Low. The runtime change is a one-token OR added to an existing conditional; the extra work (Bun__CallFrame__getLineNumber) is the same call the JUnit path already exercises, and it only fires under an attached inspector — a debugging scenario where the extra stack walk per test registration is negligible. I traced the retroactive consumer in src/runtime/jsc_hooks.rs:retroactively_report_scope and confirmed it reads base.line_no for both describe and test entries, so the fix lands at the layer that owns the invariant.
Other factors
- The gate correctly uses
debugger.is_some()rather thantest_reporter_agent.is_enabled(): the whole bug is that the agent enables after collection, so line capture must be unconditional on debugger presence. - Evidence in the PR body shows fail-before (line 0 received) on both ASAN debug and release without the fix, pass-after on both with it. The inner spawned subprocess does not pass
--reporter=junit, so the fail-before is genuine. - Test additions extend the existing retroactive test in place (no new file), assert on already-collected event data, and include an exact-line check whose arithmetic I verified against the template-literal fixture.
- The comment-cop bot's concern about a paragraph comment was addressed in 7498925 and the thread is resolved.
|
Closing this PR because it has been inactive for more than 90 days. |
|
The stale workflow closed this with no maintainer review. The work continues in #47868. The gate here ( |
Problem
When a debugger client sends
TestReporter.enableafter test collection has already started,bun testwalks the already-discovered scope tree and emits aTestReporter.foundevent for each test. Every one of those events carriedline: 0(and noscriptId), so IDE test explorers placed them at the top of the file.The same tests carry real line numbers when
TestReporter.enableis sent beforeInspector.initialized, and also on the late path if--reporter=junithappens to be on.Cause
The live path (
InspectorTestReporterAgent::reportTestFound) derives the source location directly from the liveCallFrame. The retroactive path has no call frame, so it readsBaseScope::line_no, which is populated bycapture_test_line_numberinsrc/runtime/test_runner/jest.rs. That function only walked the stack whentest_options.reporters.junitwas set; line capture was originally added for the JUnit reporter and the inspector path inherited the zero.Fix
Also capture the line number at test registration time when an inspector is attached (
vm.debugger.is_some()), so the retroactive walker has a real source location to emit. The common case (no inspector, no junit) still skips the stack walk.Verification
Extended the existing retroactive-enable test in
test/cli/inspect/test-reporter.test.tsto assert that every retroactively reportedTestReporter.foundevent hasline > 0and the correcturl, with an exact-line spot check.[stamp-90s] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file