test: cover a throwing export getter in the plugin object loader - #39998
Conversation
#39804 made generateObjectModuleSourceCode propagate an exception thrown by a getter on the exports object of a loader: "object" result. It added tests for the mock.module() entry point only. This adds the Bun.plugin entry points: import() and require() of a build.module() result, and import() of a build.onLoad() result. Each case checks that the importer receives the getter's own error object. Before #39804 the three cases print boom=undefined. They pass on main.
|
Updated 12:09 PM PT - Aug 21st, 2026
✅ @robobun, your commit 2e7029c86575c26f97c5de8e098f37f417098a80 passed in 🧪 To try this PR locally: bunx bun-pr 39998That installs a local version of the PR into your bun-39998 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesObject-loader getter error propagation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This PR adds localized coverage for throwing export getters without changing product or runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: this PR carries the plugin test case from #33793, which is now closed as superseded by #39804. Ready for review. How I checked the behavior:
Both automated reviews and the self-review below reported no findings. No open review threads. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only addition that mirrors the existing throwing exports getter block right above it.
What was reviewed:
- Subprocess pattern matches the neighboring #37026 block:
await using proc, concurrent pipe drain viaPromise.all, exact stdout/stderr/exitCode assertions in the right order. - Identity check (
e === globalThis.sentinel) is stronger than a message match; the success-pathns.boomprint ensures the pre-#39804undefined-export behavior produces distinguishable output so the test fails for the right reason. it.concurrentis appropriate — each case is an independent hermetic subprocess with no shared state.
Extended reasoning...
Overview
Test-only PR adding one describe block with three it.concurrent cases to test/js/bun/plugin/plugins.test.ts. Covers the Bun.plugin entry points (build.module via import/require, and build.onLoad via import) into generateObjectModuleSourceCode when an individual export property's getter throws. #39804 fixed the behavior and tested only the mock.module() entry point; this fills in the plugin-side coverage that #33793 originally proposed.
Security risks
None. No production code touched. Tests spawn hermetic subprocesses via bunExe()/bunEnv with inline -e code, no filesystem writes, no network.
Level of scrutiny
Low. This is a near-verbatim structural copy of the adjacent describe("object loader with a throwing exports getter") block (from #37026), with the getter moved one level deeper (onto a property of exports rather than exports itself). The shared expectSentinel helper and report catch-block template are local to the new describe and don't affect other tests.
Other factors
- Assertions are exact and can fail:
expect(stdout).toBe("failed with sentinel\n")distinguishes the fixed behavior from the oldboom=undefinedoutput the PR description documents underUSE_SYSTEM_BUN=1. - The sentinel identity check (
e === globalThis.sentinel) verifies the thrown object reaches the importer as-is, not wrapped or stringified — a tighter invariant than the neighboring block's message check. - Pipes drained concurrently,
await usingfor the subprocess, exit code asserted last — all per harness conventions. - The
before/afterplain exports around the throwingboomgetter ensure the loop ingenerateObjectModuleSourceCodeis mid-iteration when the throw happens, which is the interesting case. - No prior reviews from me on this PR; only the robobun build comment in the timeline.
|
Self-review of this PR, done after the two automated reviews above. Questions I checked:
No changes needed from this pass. The PR is ready for review. |
3 participants
Problem
generateObjectModuleSourceCode(src/jsc/modules/ObjectModule.cpp:25). A throwing getter on the exports object of aloader: "object"result now fails the import. Before, the loader exportedundefinedfor it.mock.module()entry point only. Its description lists this change under "no repro", but a plugin reaches it: bun1.4.0-canary.1+6e906e468(before Propagate JS exceptions instead of clearing them where the caller is fallible #39804) printsboom=undefinedfor the cases below.Fix
describe("object loader with a throwing getter on an export")totest/js/bun/plugin/plugins.test.ts, next to the Bun.plugin: fix segfault when an object loader result's exports getter throws #37026 block for a throwing getter on the result'sexportsproperty.import()andrequire()of abuild.module()result, andimport()of abuild.onLoad()result. Each checks that the caught error is the object the getter threw. The getter sits between two plain exports.USE_SYSTEM_BUN=1(1.4.0-canary.1+6e906e468) and pass with a debug build of main at 40ef811. The full file passes there (45 tests).Background
loader: "object"result becomes a synthetic module.ModuleLoader.cpp:442passes itsexportsobject togenerateObjectModuleSourceCode, which reads each own enumerable property once into the namespace.require()takes the same path after the__esModulecheck atModuleLoader.cpp:422.mock.module()of a module that is not loaded yet uses the same generator. That is the entry point Propagate JS exceptions instead of clearing them where the caller is fallible #39804 tests.exportsproperty of the result object (ModuleLoader.cpp:155).Notes
imported boom=undefinedfor the twoimport()cases andrequired boom=undefinedfor therequire()case.import()andrequire()in one subprocess. This version mirrors the shape of the Bun.plugin: fix segfault when an object loader result's exports getter throws #37026 block: one subprocess per entry point, exactstdout, emptystderr, exit code 0.mock.module()test is not carried over. Propagate JS exceptions instead of clearing them where the caller is fallible #39804 added two tests for that entry point intest/js/bun/test/mock/mock-module.test.ts(the import failure and the untouched namespace). Both of Bun.plugin: propagate getter exceptions from loader: "object" exports #33793's test cases pass on a debug build of main with nosrc/change. That is the basis for closing Bun.plugin: propagate getter exceptions from loader: "object" exports #33793.bun bd test test/js/bun/plugin/plugins.test.ts(45 pass) andUSE_SYSTEM_BUN=1 bun test test/js/bun/plugin/plugins.test.ts -t "throwing getter on an export"(3 fail).