bun test --isolate: opt-in global reuse fast path - #36871
Code review found 2 important issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/ZigGlobalObject.cpp:868 |
mockModule = {} zeroes LazyProperty callbacks; jest.fn/spyOn crash after global reuse |
| 🔴 Important | src/jsc/bindings/ZigGlobalObject.cpp:761-765 |
tryResetForTestIsolation calls throwing JSC APIs without a ThrowScope |
| 🟡 Nit | src/jsc/bindings/InternalModuleRegistry.cpp:65-92 |
InternalModuleExecutableCache defeats BUN_DYNAMIC_JS_LOAD_PATH mid-run reload |
| 🟡 Nit | src/jsc/VirtualMachine.rs:4463-4466 |
Baseline capture runs even when force_full_swap disables reuse |
Annotations
Check failure on line 868 in src/jsc/bindings/ZigGlobalObject.cpp
claude / Claude Code Review
mockModule = {} zeroes LazyProperty callbacks; jest.fn/spyOn crash after global reuse
`mockModule = {}` value-initializes the eight `LazyProperty<JSGlobalObject, T>` fields inside `JSMockModule`, zeroing each one's `m_pointer` and losing the `initLater` callback tag that `JSMockModule::create()` (called only from the `Zig::GlobalObject` constructor) set up. After the first successful global reuse, `mockFunctionStructure.getInitializedOnMainThread(globalObject)` returns `nullptr`, and `jest.fn()` / `vi.fn()` / `jest.spyOn()` / `mock.module()` pass a null `Structure*` into `allocat
Check failure on line 765 in src/jsc/bindings/ZigGlobalObject.cpp
claude / Claude Code Review
tryResetForTestIsolation calls throwing JSC APIs without a ThrowScope
`Zig__GlobalObject__tryResetForTestIsolation` calls throwing JSC APIs (`JSCell::deleteProperty`, `JSMapIterator::create`, `iter->next`, `toStringOrNull`/`str->view`, `requireMap->remove`) with no `DECLARE_THROW_SCOPE`/`DECLARE_CATCH_SCOPE` and no exception checks; `captureTestIsolationBaseline`'s `reifyAllStaticProperties(globalObject)` has the same gap. Per REVIEW.md's "Exception checks after every call that can enter JS" rule this trips `BUN_JSC_validateExceptionChecks=1` — add a `DECLARE_CATC
Check warning on line 92 in src/jsc/bindings/InternalModuleRegistry.cpp
claude / Claude Code Review
InternalModuleExecutableCache defeats BUN_DYNAMIC_JS_LOAD_PATH mid-run reload
nit (debug-only): under `BUN_DYNAMIC_JS_LOAD_PATH`, `initializeInternalModuleFromDisk` still reads fresh source from disk each call, but `generateModule` now returns the cached `UnlinkedFunctionExecutable` and ignores the freshly-read `SOURCE`. So on the full-swap path of `bun bd test --isolate`, mid-run edits to `src/js/*` stop taking effect after the first file (and the disk read is wasted). Consider wrapping the cache lookup/store in `#ifndef BUN_DYNAMIC_JS_LOAD_PATH` — the edit-then-rerun wo
Check warning on line 4466 in src/jsc/VirtualMachine.rs
claude / Claude Code Review
Baseline capture runs even when force_full_swap disables reuse
nit: This guard doesn't check `force_full_swap`. When `BUN_FEATURE_FLAG_DISABLE_ISOLATION_GLOBAL_REUSE=1` is set, `try_reset_for_test_isolation` is never called (short-circuited in `swap_global_for_test_isolation`), yet `captureTestIsolationBaseline` still runs on every fresh global — calling `reifyAllStaticProperties` and snapshotting all own properties into a HashMap that is never read. Consider adding `&& !self.test_isolation_state.force_full_swap` so the escape hatch stays as close to the pr