Repository navigation
inspector: share TestReporter test ID counter between live and retroactive paths - #36522
Conversation
…ctive paths The live-registration path in ScopeFunctions::call used a function-local static AtomicI32; the retroactive path in retroactively_report_discovered_tests seeded a fresh local at 0 on every call. When TestReporter.enable arrives mid-collection both paths run in the same process and hand out overlapping 1..N ranges, so two unrelated tests can be reported under the same ID. Move the counter to Debugger::next_test_id_for_debugger. ScopeFunctions::call increments it directly; the retroactive hook takes the current value and returns the advanced counter, which test_reporter_agent_enable writes back. Co-authored-by: Craig R. Hughes <craig.git@rungie.com>
WalkthroughChangesTest reporter ID allocation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced and verified locally:
After review the counter now lives on CI: |
|
Updated 1:42 AM PT - Jul 31st, 2026
❌ @robobun, your commit 532af42 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36522That installs a local version of the PR into your bun-36522 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review: colocate the counter with the agent handle to match HTTPServerAgent.next_server_id / ErasedAgentSlot.sequence. Trim redundant comments. Drain proc.stdout alongside stderr in the mid-collection test. Co-authored-by: Craig R. Hughes <craig.git@rungie.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/Debugger.rs`:
- Around line 810-815: Rename TestReporterAgent.next_test_id to last_test_id and
centralize pre-increment allocation in a public alloc_test_id method that
returns the newly assigned ID, starting at 1. Update both ScopeFunctions and the
retroactive jsc_hooks path to use this method while preserving the existing
parameter/return threading and ID behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4b9c916-dcac-4635-a875-335167e6349a
📒 Files selected for processing (5)
src/jsc/Debugger.rssrc/jsc/VirtualMachine.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/ScopeFunctions.rstest/cli/inspect/test-reporter.test.ts
There was a problem hiding this comment.
LGTM — the stdout-drain nit from the earlier pass is addressed, and the shared-counter fix looks correct.
What was reviewed:
- Both ID-assignment paths now draw from
TestReporterAgent::next_test_id; all early returns inretroactively_report_discovered_teststhreadnext_test_idback unchanged, andretroactively_report_scopeonly increments for scopes withtest_id_for_debugger == 0, so a re-enable can't double-assign. - Moving from a process-global
static AtomicI32to a per-Debuggerfield is the right placement (per-VM state on the VM); single-JS-thread so plaini32is fine. - New test genuinely interleaves the two paths (async describe checkpoint → retroactive walk → live registration) and asserts uniqueness on a raw non-deduped ID list, so a collision can't hide behind the Map.
The three unresolved comment-cop notes are firing on ordinary 2-line doc comments, not workaround justifications.
Extended reasoning...
Overview
This PR fixes a TestReporter inspector-protocol bug where test/describe IDs could collide when TestReporter.enable arrives mid-collection. Two independent counters (a function-local static AtomicI32 in ScopeFunctions::call for live registration, and a fresh let mut max_id = 0 in retroactively_report_discovered_tests) each handed out 1..N, so tests reported via different paths in the same run could share an ID. The fix unifies them into a single next_test_id: i32 field on TestReporterAgent, threaded by value/return through the RuntimeHooks boundary so the low-tier crate can seed and re-store it without a forward-dep.
Files touched: src/jsc/Debugger.rs (+field, write-back on enable), src/jsc/VirtualMachine.rs (hook signature), src/runtime/jsc_hooks.rs (thread the counter through, all early returns preserve it), src/runtime/test_runner/ScopeFunctions.rs (drop the static, mutate the field via &mut Debugger), plus a new regression test in test/cli/inspect/test-reporter.test.ts.
Security risks
None. This is inspector/debugger metadata (integer IDs correlating found/start/end events for IDE tooling). No user-controlled input reaches the changed code paths; no auth, crypto, filesystem, or network surface.
Level of scrutiny
Moderate. ~30 lines of native change across four files with a clear, mechanical shape: replace two counters with one, thread it across a crate boundary. The state relocation (process-global static → per-Debugger field) is an architectural improvement per REVIEW.md ("per-VM state goes on VirtualMachine, never process globals"). I traced retroactively_report_scope to confirm it pre-increments *max_id before use and skips scopes that already carry an ID, so seeding from a nonzero next_test_id cannot double-assign or regress the existing all-retroactive test.
Other factors
- My prior review flagged the new test piping stdout without draining it; commit
532af423addressed that (Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])), and the inline comment is now resolved. - robobun confirmed the test fails under
USE_SYSTEM_BUN=1and passes on the debug build — satisfies the "prove the test fails for the right reason" bar. - The three still-open
comment-copbot comments target the 2–3-line doc comments onnext_test_idand the hook signature. Those are ordinary field/function docs, not workaround justifications, so I'm treating them as bot false positives. - The new test follows the sibling test's harness pattern (unix-socket inspector, gate files, Console-domain checkpoint) and asserts on a raw
rawFoundIdsarray specifically so a duplicate ID surfaces asSet.size < lengthrather than being coalesced by the session'sMap.
Adopts the fix from #34221 by @hughescr.
Problem
TestReportertest/describe IDs came from two independent counters:ScopeFunctions::call, which assigns an ID as eachdescribe()/test()is collected) used a function-localstatic AtomicI32that only advances while the agent is enabledretroactively_report_discovered_tests, which walks the already-discovered scope tree whenTestReporter.enablearrives after collection has started) seeded a fresh localmax_idat0on every callWhen
enablelands mid-collection (some scopes already discovered, others still being registered live) both paths run in the same process and each hands out its own1..Nrange, so two unrelated tests can be reported under the same ID. Inspector clients keying state by ID (correlatingfound/start/endevents) silently conflate them.Fix
Move the counter to a
next_test_id: i32field onTestReporterAgent(matchingHTTPServerAgent::next_server_id/ErasedAgentSlot::sequence):ScopeFunctions::callincrements it directly through its&mut Debuggerborrowretroactively_report_discovered_teststakes the counter by value and returns the advanced value through theRuntimeHookshook;test_reporter_agent_enablewrites the return back into the fieldBoth paths now draw from the same monotone counter. Single JS thread, so no synchronization needed.
Verification
New test in
test/cli/inspect/test-reporter.test.tscreates a genuine mid-collection window by pausing inside an asyncdescribe()callback (suite A's sync callback has already registered 2 tests; suite B'stest()call has not yet run).TestReporter.enableis sent in that window so suite A + suite B's describe go through the retroactive walk andtest B1goes through the live path in the same run. The test asserts everyfoundID across both paths is unique via a raw non-deduped list (theMap-keyed session bookkeeping would coalesce a collision).Supersedes #34221 (same fix, adopted with co-author credit).
[review] gate passed · iteration 0 · 5 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