Enable run-detect-module-type test and fix its import.cjs fixtures - #37182
Enable run-detect-module-type test and fix its import.cjs fixtures#37182robobun wants to merge 6 commits into
Conversation
The file lacked .test. in its name, so the test runner never collected it. Its two import.cjs fixtures also ended with +fs; on a module namespace object, which throws TypeError (ToPrimitive on a namespace has no default value, same as Node), so the test failed whenever it was run explicitly. Replace the coercion with a plain property access, rename the file so CI runs it, and run the 18 fixtures concurrently.
WalkthroughChangesModule type and main-module tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only change that renames a never-collected test file so CI runs it, fixes a broken fixture line, and swaps spawnSync for concurrent async spawns. The inline nit about the === "false" ternary is minor and carried over from the original file; fine to address or leave.
Extended reasoning...
Overview
This PR touches four files under test/cli/run/: it deletes run-detect-module-type.ts (which used bun:test but was never collected because it lacked .test. in its name), adds run-detect-module-type.test.ts with the same expectation table, and changes +fs; → fs.constants; in two import.cjs fixtures. The new test file also converts 18 sequential spawnSync calls into concurrent Bun.spawn + Promise.all, following the harness convention of draining stdout/stderr/exited together.
Security risks
None. This is test-only; no runtime code, no auth/crypto/permissions, no untrusted input handling.
Level of scrutiny
Low. The expectation table is byte-for-byte identical to what was already committed (just reformatted with double quotes and as const). The fixture fix is well-justified: +fs on a module namespace object throws TypeError in both Bun and Node because namespace objects have no ToPrimitive path; fs.constants; keeps the import referenced without coercion. All 18 fixture files referenced in the table exist on disk, and the two package.json files have the expected type fields. The concurrent-spawn rewrite follows the CLAUDE.md pattern (await using proc, Promise.all([stdout.text(), stderr.text(), proc.exited])).
Other factors
- The one finding is a nit about the lossy
stdout.trim() === "false" ? "commonjs" : "module"mapping. This is real but low-impact: the fixtures are controlled one-liners that can only print"true"or"false", the exit-code check catches crashes, and the identical ternary existed in the deleted original — it was carried over, not introduced. Not a blocker. - The
USE_SYSTEM_BUN=1must-fail rule does not apply here: there is no accompanying runtime fix; this PR is enabling a pre-existing behavior-pinning test that was silently never running. - No prior human or bot reviews to reconcile with; no outstanding comments.
|
Addressed the review nit in 0bb1e9a: the stdout mapping is now exact, "true" -> module, "false" -> commonjs, and anything else surfaces the raw output in the toEqual diff instead of silently counting as module. |
![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 `@test/cli/run/run-detect-module-type.test.ts`:
- Around line 47-50: Update the subprocess result handling around proc.stdout,
proc.stderr, and proc.exited so successful fixtures also fail when stderr
contains unexpected output. Assert the combined result according to the existing
bunEnv fixture subprocess convention, while preserving the current nonzero-exit
error reporting.
🪄 Autofix
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: 6d69975d-3f92-4759-8858-2b7824a7c865
📒 Files selected for processing (4)
test/cli/run/module-type-fixture/cjs/import.cjstest/cli/run/module-type-fixture/esm/import.cjstest/cli/run/run-detect-module-type.test.tstest/cli/run/run-detect-module-type.ts
💤 Files with no reviewable changes (1)
- test/cli/run/run-detect-module-type.ts
There was a problem hiding this comment.
LGTM — test-only change enabling a previously uncollected test; both earlier review nits (exact stdout mapping, stderr rejection) are addressed.
What was reviewed:
- Fixture change
+fs;→fs.constants;keeps the import binding referenced without ToPrimitive coercion; no other+fssites remain in the fixture tree. - Expectation table is byte-identical to the deleted file; all 18 fixture filenames exist on disk.
- Subprocess handling drains stdout/stderr/exited concurrently, uses
bunEnv/bunExe, and now rejects both non-"true"/"false" stdout and any stderr.
Extended reasoning...
Overview
This PR touches only test infrastructure: it renames test/cli/run/run-detect-module-type.ts to .test.ts so the runner collects it, replaces +fs; with fs.constants; in the two import.cjs fixtures (unary-plus on a module namespace object throws in both Bun and Node, so the fixture itself was broken), and rewrites the test body to spawn the 18 fixtures concurrently via Bun.spawn + Promise.all instead of sequential spawnSync. The expectation table pinning module-type detection is unchanged from the deleted file.
Security risks
None. No runtime, native, or built-in module code is touched — only a test file and two three-line fixture scripts under test/cli/run/.
Level of scrutiny
Low. This is a self-contained test-enablement change with no production code paths involved. The interesting question is whether the newly-enabled test is sound, and the two prior review rounds already tightened it: stdout is now mapped exactly ("true"→module, "false"→commonjs, anything else surfaces raw in the toEqual diff) and any non-empty stderr on a zero-exit run throws. The fixture directory listing confirms all 18 referenced files exist and no sibling +fs sites were missed.
Other factors
Both prior review threads (my stdout-mapping nit and CodeRabbit's stderr check) are marked resolved and the diff reflects the fixes. Harness conventions are followed: bunEnv/bunExe, await using for the subprocess, concurrent pipe draining, no sleeps or hardcoded ports, no network. The stderr check is ordered after the exit-code check rather than before, but the exit-code branch already includes stderr in its message so diagnostics are preserved either way.
There was a problem hiding this comment.
LGTM — test-only change enabling two previously-uncollected test files and fixing a broken fixture; earlier review nits are addressed.
What was reviewed:
+fs;→fs.constants;fixture fix — namespace ToPrimitive does throw in both Bun and Node, so the fixture was the bug.- Rewritten test: concurrent
Bun.spawnwithawait using, pipes drained viaPromise.all, exact tri-valued stdout mapping, stderr rejected on success — matches harness conventions. - New
exports.{js,mjs}rows and therun-importmetamainrename (100% content-identical) — fixture set matches the 22-row table; no runtime code touched.
Extended reasoning...
Overview
This PR is entirely under test/cli/run/. It renames two test files that used bun:test but lacked .test. in their names (so CI never collected them), fixes a broken fixture line (+fs; on a module namespace object, which throws in both Bun and Node), rewrites run-detect-module-type to spawn fixtures concurrently with proper pipe draining and exact assertions, and adds four exports.{js,mjs} fixtures pinning the CommonJS-marker side of Bun's module-type sniff. No source code under src/ is touched.
Security risks
None. Test-only; spawns local fixtures with bunExe()/bunEnv, no network, no filesystem writes outside the fixture tree, no untrusted input.
Level of scrutiny
Low. The original test never ran, so enabling it cannot regress anything already covered. The fixture fix is a one-token change whose rationale (namespace ToPrimitive) is spec-correct and verified against Node. The rewrite follows the repo's documented subprocess-test patterns (await using, Promise.all on stdout/stderr/exited, bunEnv). The run-importmetamain rename is a git similarity 100% move with no content change.
Other factors
Two prior review threads (my stdout-mapping nit and CodeRabbit's stderr check) are both addressed and resolved in the current diff — stdout is now mapped tri-valued so garbage output fails visibly, and non-empty stderr on a zero exit throws. The 22-row table matches the on-disk fixture set exactly (11 files × 2 package types). The added exports.mjs → commonjs rows pin Bun-specific detection behavior that the PR description states was verified against current main and a recent canary, so they document existing behavior rather than assert new semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/cli/run/run-importmetamain.test.ts`:
- Around line 2-3: Replace tmpdirSync-based fixture setup with disposable
tempDir usage at both setup sites: import tempDir, remove the mkdirSync and
tmpdirSync imports, and declare each directory as using dir = tempDir("...",
{}). Ensure the fixtures rely on tempDir’s automatic creation and disposal.
- Around line 13-20: Update both Bun.spawnSync calls in the import-meta tests to
destructure exitCode, and assert each exitCode equals 0 alongside its
corresponding stdout assertion, including the CommonJS fixture test.
🪄 Autofix
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: d428e4ee-1ac7-455a-9274-94732a399445
📒 Files selected for processing (6)
test/cli/run/module-type-fixture/cjs/exports.jstest/cli/run/module-type-fixture/cjs/exports.mjstest/cli/run/module-type-fixture/esm/exports.jstest/cli/run/module-type-fixture/esm/exports.mjstest/cli/run/run-detect-module-type.test.tstest/cli/run/run-importmetamain.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 `@test/cli/run/run-importmetamain.test.ts`:
- Around line 2-3: Replace tmpdirSync-based fixture setup with disposable
tempDir usage at both setup sites: import tempDir, remove the mkdirSync and
tmpdirSync imports, and declare each directory as using dir = tempDir("...",
{}). Ensure the fixtures rely on tempDir’s automatic creation and disposal.
- Around line 13-20: Update both Bun.spawnSync calls in the import-meta tests to
destructure exitCode, and assert each exitCode equals 0 alongside its
corresponding stdout assertion, including the CommonJS fixture test.
🪄 Autofix
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: d428e4ee-1ac7-455a-9274-94732a399445
📒 Files selected for processing (6)
test/cli/run/module-type-fixture/cjs/exports.jstest/cli/run/module-type-fixture/cjs/exports.mjstest/cli/run/module-type-fixture/esm/exports.jstest/cli/run/module-type-fixture/esm/exports.mjstest/cli/run/run-detect-module-type.test.tstest/cli/run/run-importmetamain.test.ts
🛑 Comments failed to post (2)
test/cli/run/run-importmetamain.test.ts (2)
2-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail repo_root="$(pwd)" rg -n -C 3 'tempDir\(|tmpdirSync\(' \ "$repo_root/test/harness.ts" "$repo_root/test" --glob '*.test.ts'Repository: oven-sh/bun
Length of output: 2031
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- target file ---' sed -n '1,120p' test/cli/run/run-importmetamain.test.ts printf '%s\n' '--- tempDir definitions and nearby usage ---' rg -n -C 4 'function tempDir|const tempDir|tempDir\(|tmpdirSync\(' test/harness.ts test/cli/run --glob '*.test.ts' | head -240Repository: oven-sh/bun
Length of output: 2002
🌐 Web query:
oven-sh/bun test/harness.ts tempDir("run-importmetamain💡 Result:
In the context of the oven-sh/bun repository, the components mentioned relate to testing infrastructure for ensuring correct bundler and runtime behavior regarding
import.meta.mainandrequire.main[1]. 1.tempDir: This is a utility function exported fromtest/harness.ts[2][3]. It creates a temporary directory for test isolation and returns a path that also acts as a disposable resource (supportingSymbol.disposeandSymbol.asyncDispose) to automatically clean up the directory after the test completes [2][3]. It is frequently used in conjunction withtempDirWithFilesto set up fixture files [4]. 2.run-importmetamain: This refers to a test file, specificallytest/cli/run/run-importmetamain.test.ts(originally namedrun-importmetamain.tsbut renamed to be collected by the test runner) [1]. This test suite was created to verify how Bun handlesimport.meta.mainandrequire.main === moduleacross different execution contexts (CJS, ESM, and bundled code) [1]. It specifically addresses precedence issues in code generation [1] and ensures that these properties behave consistently when code is run directly versus when it is bundled [1][5]. These components are part of Bun's standard testing practices, where developers useharness.tsto manage temporary test environments and create specific test cases (likerun-importmetamain.test.ts) to ensure that Bun's transpiler and bundler correctly maintain the semantics of Node.js-compatible features [1][6][4].Citations:
- 1: #33447
- 2: https://github.com/oven-sh/bun/blob/7e57e529/test/harness.ts
- 3: https://github.com/oven-sh/bun/blob/7abe6c38/test/harness.ts
- 4: https://github.com/oven-sh/bun/blob/74e191b3/test/CLAUDE.md
- 5: #30085
- 6: #22646
🌐 Web query:
site:github.com/oven-sh/bun/blob test/harness.ts "export function tempDir" "Symbol.dispose"💡 Result:
In the Bun test harness (test/harness.ts), the tempDir function is designed to return a string representing a temporary directory path that also implements the Disposable and AsyncDisposable interfaces [1][2]. This allows the directory to be automatically cleaned up when using the using keyword in TypeScript [3]. The implementation achieves this by wrapping the temporary directory path in a class called DisposableString, which extends String [1][2]. This class implements: 1. [Symbol.dispose]: Uses fs.rmSync to synchronously remove the directory and its contents recursively [1][2]. 2. [Symbol.asyncDispose]: Uses fs.promises.rm to asynchronously remove the directory and its contents recursively [1][2]. Because the returned object is an instance of DisposableString (which behaves like a string) and implements these disposal methods, developers can write code such as: using dir = tempDir("my-test", { "file.txt": "content" }); // The directory is automatically deleted when 'dir' goes out of scope.
Citations:
- 1: https://github.com/oven-sh/bun/blob/75e94b04/test/harness.ts
- 2: https://github.com/oven-sh/bun/blob/main/test/harness.ts
- 3: https://github.com/oven-sh/bun/blob/88417471/test/js/bun/cron/cron.test.ts
Replace
tmpdirSync()with disposabletempDir().At both fixture setup sites, import
tempDir, removemkdirSync, and useusing dir = tempDir("...", {});.tempDir()creates and disposes the directory;tmpdirSync()is prohibited in test files.🤖 Prompt for 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. In `@test/cli/run/run-importmetamain.test.ts` around lines 2 - 3, Replace tmpdirSync-based fixture setup with disposable tempDir usage at both setup sites: import tempDir, remove the mkdirSync and tmpdirSync imports, and declare each directory as using dir = tempDir("...", {}). Ensure the fixtures rely on tempDir’s automatic creation and disposal.Sources: Coding guidelines, Learnings
13-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail repo_root="$(pwd)" rg -n -C 4 'Bun\.spawnSync\(|\bexitCode\b' \ "$repo_root/test" --glob '*.test.ts'Repository: oven-sh/bun
Length of output: 2031
🏁 Script executed:
#!/bin/bash set -euo pipefail target="test/cli/run/run-importmetamain.test.ts" if [ -f "$target" ]; then printf '%s\n' '--- target ---' cat -n "$target" else printf 'missing target: %s\n' "$target" >&2 exit 1 fi printf '%s\n' '--- focused spawnSync conventions ---' rg -n -C 5 'Bun\.spawnSync\(' test --glob '*.test.ts' | head -n 500 printf '%s\n' '--- focused exitCode assertions ---' rg -n -C 3 'exitCode' test/cli test --glob '*.test.ts' | head -n 500Repository: oven-sh/bun
Length of output: 2002
🌐 Web query:
oven-sh/bun test/cli/run/run-importmetamain.test.ts Bun.spawnSync exitCode💡 Result:
The file
test/cli/run/run-importmetamain.test.tsin theoven-sh/bunrepository is a test file designed to verify the behavior ofimport.meta.mainandrequire.main === moduleacross different execution contexts [1]. It was renamed to include the.test.tssuffix to ensure it is correctly collected and executed by the Bun test runner [1]. In this test file,Bun.spawnSyncis used to execute Bun processes synchronously [2][3]. TheexitCodeproperty returned byBun.spawnSyncis then used to verify that the executed process finished with the expected status code (e.g.,0for success) [2][4]. While specific usage withinrun-importmetamain.test.tsfocuses on validating the logic of module main-entry detection, the general pattern for checking exit codes withBun.spawnSyncis [2][5]: const { exitCode } = Bun.spawnSync({ cmd: ["bun", "run", "some-file.ts"], }); expect(exitCode).toBe(0); TheSyncSubprocessobject returned byBun.spawnSyncalso includes asuccessboolean property, which is a shorthand for checking ifexitCode === 0[3][4].Citations:
- 1: #33447
- 2: https://bun.sh/reference/bun/spawnSync
- 3: https://bun.sh/docs/runtime/child-process
- 4: https://bun.com/docs/runtime/child-process
- 5: https://github.com/oven-sh/bun/blob/7e57e529/test/js/bun/spawn/spawn.test.ts
🌐 Web query:
site:github.com/oven-sh/bun/blob test/cli/run/run-importmetamain.test.ts "const { stdout"💡 Result:
The file test/cli/run/run-importmetamain.test.ts appears to be part of the Bun runtime test suite. It is designed to verify the behavior of the import.meta.main property [1][2]. In Bun, import.meta.main is a boolean property used to determine if the current module is the entry point of the application [1][2]. When a script is executed directly (e.g., bun run script.ts), import.meta.main evaluates to true [1][2]. While the specific content of test/cli/run/run-importmetamain.test.ts is not fully detailed in the provided search results, related test files within the Bun repository—such as test/bundler/bundler_compile.test.ts—demonstrate how this property is tested [1][2]. These tests typically spawn a Bun process and capture the stdout to assert that import.meta.main behaves as expected: const { stdout } = Bun.spawnSync({... }); // Example logic used in tests: // console.log(( => import.meta.main).toString.includes('true')); The code snippet in your query, const { stdout }, is a common pattern in these tests for extracting the output from a spawned Bun process, allowing the test to assert that the runtime's internal checks for the entry point are correct [3][4][5].
Citations:
- 1: https://github.com/oven-sh/bun/blob/a0e221e0/test/bundler/bundler_compile.test.ts
- 2: https://github.com/oven-sh/bun/blob/88417471/test/bundler/bundler_compile.test.ts
- 3: https://github.com/oven-sh/bun/blob/1cc83768/test/cli/run/run-eval.test.ts
- 4: https://github.com/oven-sh/bun/blob/7abe6c38/test/cli/run/run-eval.test.ts
- 5: https://github.com/oven-sh/bun/blob/7abe6c38/test/cli/install/bun-run.test.ts
🌐 Web query:
raw.githubusercontent.com oven-sh bun test/cli/run/run-importmetamain.test.ts💡 Result:
The file
test/cli/run/run-importmetamain.test.tsis a test file within the Bun runtime repository [1]. It was created by renaming a previously existing file (run-importmetamain.ts) to ensure it would be correctly collected and executed by the Bun test runner, which specifically looks for files containing.testin their filename [1]. The primary purpose of this test file is to verify the behavior ofimport.meta.mainandrequire.main === moduleacross different module systems (CommonJS and ESM) [1]. It was specifically used to ensure that these expressions are handled correctly by the runtime, including fixing precedence bugs in the transpiler/printer that could incorrectly affect these checks [1]. In essence, the file contains tests that cover: * CommonJS entry points [1]. * Required non-main CommonJS modules [1]. * ESM inverted form [1]. * Syntax error handling for specific patterns [1]. This file is part of the broader test suite in theoven-sh/bunrepository, helping ensure consistency in how Bun handles module entry points and environment variables [1][2].Citations:
Assert each subprocess exit status.
Destructure
exitCodefrom bothBun.spawnSyncresults and assertexitCodeis0after each stdout assertion, including the CommonJS test. Otherwise, a fixture can print the expected JSON and still exit non-zero while the test passes.🤖 Prompt for 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. In `@test/cli/run/run-importmetamain.test.ts` around lines 13 - 20, Update both Bun.spawnSync calls in the import-meta tests to destructure exitCode, and assert each exitCode equals 0 alongside its corresponding stdout assertion, including the CommonJS fixture test.Sources: Coding guidelines, Learnings
|
Both review findings on run-importmetamain.test.ts are addressed in d3e5b9e: fixture setup now uses the disposable tempDir helper from harness, and both tests assert empty stderr, exact stdout, and a zero exit code. Verified passing with the release binary and a debug build. |
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 `@test/cli/run/run-importmetamain.test.ts`:
- Line 4: Extend the run-importmetamain test beyond the entry-module probe by
adding an imported ESM child and a required CommonJS child, each reporting and
asserting import.meta.main === false. Update the test setup and expectations to
cover both child-module paths while preserving the existing entry-module checks.
🪄 Autofix
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: ec544e89-842e-4350-98f2-f892998a8ed8
📒 Files selected for processing (1)
test/cli/run/run-importmetamain.test.ts
There was a problem hiding this comment.
LGTM — test-only change that enables two previously-uncollected test files and fixes their fixtures. My earlier nit and the CodeRabbit findings are all addressed.
What was reviewed:
- Confirmed all 22 table entries have matching fixture files on disk; the
+fs;→fs.constants;fix is correct (ToPrimitive on a module namespace throws in both Bun and Node). - Checked the tri-valued stdout mapping and stderr guard land the earlier review feedback;
run-importmetamain.test.tsnow usestempDir, drains pipes concurrently, and asserts stderr/stdout/exitCode in order. - No production code touched; the new
exports.{js,mjs}rows pin existing detection behavior verified against main.
Extended reasoning...
Overview
This PR renames two bun:test files under test/cli/run/ that lacked the .test. infix and were therefore never collected by CI: run-detect-module-type.ts and run-importmetamain.ts. It fixes the import.cjs fixtures (which threw TypeError: No default value on +fs; because module namespace objects have no ToPrimitive), adds four exports.{js,mjs} fixtures to pin the CommonJS-marker-beats-extension direction of detection, and rewrites both test files to use concurrent Bun.spawn, tempDir, and exact stdout/stderr/exit assertions. Ten files changed, all under test/.
Security risks
None. No production code, no network, no external input — the tests spawn local fixture files with bunEnv and compare stdout to a fixed table.
Level of scrutiny
Low-to-moderate. Test-only, mechanical rename plus fixture repair. The main thing to check is that enabling a never-run test doesn't introduce CI flakiness or pin wrong behavior. The assertions are deterministic (no sleeps, no ports, no timing), pipes are drained concurrently per REVIEW.md, and the PR description confirms the pinned detection results match both current main and a prior canary. The new exports.mjs -> commonjs rows codify Bun's existing marker-sniffing behavior rather than changing it.
Other factors
All three prior review findings on this PR are resolved: my nit on the lossy === "false" ternary (now tri-valued so garbage stdout fails visibly), CodeRabbit's stderr-on-zero-exit check (now throws), and the run-importmetamain cleanup (now tempDir + full {stderr, stdout, exitCode} assertions). The evidence block shows 3 tests / 22 fixtures passing under debug+ASAN. No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — test-only change enabling two previously-uncollected test files; the earlier nit on the lossy stdout mapping and the stderr/tempDir follow-ups are all addressed.
What was reviewed:
+fs;→fs.constants;fixture fix — confirmed the original line throws identically in Node, so the fixture was the bug, not the runtime.- New
exports.{js,mjs}rows pin existing CommonJS-marker detection; pre-existing table rows are unchanged. - Both test files now drain stdout/stderr/exited concurrently, reject non-empty stderr, and assert exact output before exit code.
- Checked that all 22 fixture filenames referenced in the table exist on disk under both
cjs/andesm/.
Extended reasoning...
Overview
This PR renames two test files under test/cli/run/ that used bun:test but lacked .test. in their filenames, so CI never collected them. run-detect-module-type.ts (from #18562/#18686) and run-importmetamain.ts (from #12867) become .test.ts files. Along the way it fixes the import.cjs fixtures whose trailing +fs; line threw TypeError: No default value (ToPrimitive on a module namespace object — same behavior in Node), replaces spawnSync with concurrent Bun.spawn + Promise.all pipe draining, and adds four exports.{js,mjs} fixtures pinning the mirror case where a module.exports marker beats package.json type and file extension. run-importmetamain is rewritten to use the disposable tempDir helper with full stderr/stdout/exitCode assertions. No production code is touched.
Security risks
None. All changes are confined to test/, spawn only the local bunExe() against local fixture files under bunEnv, and touch no network, auth, crypto, or permission surface.
Level of scrutiny
Low-to-medium. It's a pure test-infrastructure fix: enabling dormant tests, repairing a broken fixture line, and modernizing harness usage per test/CLAUDE.md conventions (tempDir, concurrent pipe draining, test.concurrent, exit-code-last assertions). The pre-existing expectation rows are unchanged, and the four new rows pin behavior the PR author verified identical on main and 1.4.0-canary — they document current detection semantics rather than change them.
Other factors
All prior review feedback is resolved: my earlier nit about the lossy === "false" ternary was fixed in 0bb1e9a (now tri-valued so unexpected stdout surfaces in the toEqual diff); CodeRabbit's stderr-rejection request was applied in 3612059; the tempDir/exit-status follow-up on run-importmetamain landed in d3e5b9e; and the child-module-coverage suggestion was withdrawn after confirming test/js/bun/resolve/import-meta.test.js already covers it. I confirmed all 22 fixture files referenced in the table exist under both module-type-fixture/cjs/ and module-type-fixture/esm/. The PR description shows both files passing under bun bd test (3 tests, 22 spawned fixtures).
1 participant
Problem
test/cli/run/run-detect-module-type.ts(added in #18562, re-landed in #18686) usesbun:testbut its filename lacks.test., so the test runner never collects it and it has never run in CI.Running it explicitly fails on every Bun version, including current main:
Cause
Both
module-type-fixture/{cjs,esm}/import.cjsend with+fs;, wherefsis a module namespace object. ToPrimitive on a namespace object throws, because it has a null prototype and noSymbol.toPrimitive/valueOf/toString. Node 26 throws the same way on that line (TypeError: Cannot convert object to primitive value), so Bun's runtime behavior is correct and the fixture line is the bug. The line only exists to reference the import; the detection result (theconsole.log) prints before the throw.Fix
+fs;withfs.constants;in both fixtures, keeping the binding referenced without coercing it.run-detect-module-type.test.tsso CI actually runs it.spawnSync, assert stdout is exactlytrue/false, and reject any stderr on a zero exit.Review follow-ups folded in:
test/for the same class of bug (abun:testfile whose name the runner never collects). The only other instance wastest/cli/run/run-importmetamain.ts(added in feat(bundler): inlining/dead-code-elimination forimport.meta.main(and --compile) #12867, same directory, uncollected since then); its tests pass as-is, so it is renamed here too..cjsrows, leaving the opposite direction of the sniff untested. Addedexports.js/exports.mjsfixtures with amodule.exportsmarker under both package types: Bun detects these as CommonJS, beating both package.json"type"and the file extension, which is the mirror image of the existingimport.cjs -> modulerows.The pre-existing expectation rows are unchanged; the detection they pin already behaves as expected (verified identical output on current main and 1.4.0-canary.1+45ee9556a).
Verification
bun test ./test/cli/run/run-detect-module-type.tsfails with the TypeError above on main.bun bd test test/cli/run/run-detect-module-type.test.ts test/cli/run/run-importmetamain.test.ts, 3 tests, 22 spawned fixtures).This is a test-only change: the bugs were in the test infrastructure itself (never collected, broken fixture), not in Bun.
[stamp-90s] gate passed · iteration 1 · 10 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file