test(vm): replace network-dependent happy-dom #16277 repro with deterministic tail-call repro - #36823
Merged
Merged
Claude / Claude Code Review
completed
Aug 3, 2026 in 13m 46s
Code review found 3 potential issues
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/node/vm/happy-dom-vm-16277.test.ts:67-69 |
GC-finalizer test drops errors so finalizeUnconditionally never runs on them |
| 🟡 Nit | test/js/node/vm/happy-dom-vm-16277.test.ts:63-72 |
Crash-regression repro runs in-process rather than as a spawned fixture |
| 🟡 Nit | test/js/node/vm/happy-dom-vm-16277.test.ts:41 |
Inconsistent error-message assertion between test 1 and test 2 |
Annotations
Check warning on line 69 in test/js/node/vm/happy-dom-vm-16277.test.ts
claude / Claude Code Review
GC-finalizer test drops errors so finalizeUnconditionally never runs on them
Test 3's design is inverted: `ErrorInstance::finalizeUnconditionally` runs via `Heap::finalizeMarkedUnconditionalFinalizers`, which iterates `forEachMarkedCell` — it fires only on errors that **survive** the collection, not on ones that are dropped. Since all 64 errors here are unreachable at `Bun.gc(true)`, they're simply swept without materializing `.stack`, so the loop + GC can be deleted without changing the outcome and the test reduces to a repeat of test 1's eager-`.stack` path. To actuall
Check warning on line 72 in test/js/node/vm/happy-dom-vm-16277.test.ts
claude / Claude Code Review
Crash-regression repro runs in-process rather than as a spawned fixture
Since this file guards a segfault, consider running at least test 3 (the pure "does not crash during GC finalizer" check) via `Bun.spawn([bunExe(), "-e", ...])` and asserting `exitCode === 0` / `signalCode === null`. As written, a regression of #16277 segfaults the test-runner process itself at address 0x5, taking every subsequent test in the shard with it rather than reporting one clean failure. A single spawn under debug+ASAN is still ~10× faster than the 16.5s baseline this replaces, so it do
Check warning on line 41 in test/js/node/vm/happy-dom-vm-16277.test.ts
claude / Claude Code Review
Inconsistent error-message assertion between test 1 and test 2
The error-message assertions in test 1 and test 2 disagree: line 41 hedges with `|| stack.includes("not callable")` while line 61 in the vm-context test asserts strictly `toContain("is not a function")`. Your own verification table shows test 2 passes on every configuration (1.1.42/1.1.44/debug+ASAN/`useJIT=0`), which proves the strict form works everywhere — so the `"not callable"` branch on line 41 is dead weight that weakens the assertion. Drop the OR and use `expect(stack).toContain("is not
Loading