Skip to content

test: replace vacuous jest-globals scoping test with a real one - #36132

Open
robobun wants to merge 4 commits into
mainfrom
farm/4e6d5c80/fix-vacuous-jest-globals-test
Open

test: replace vacuous jest-globals scoping test with a real one#36132
robobun wants to merge 4 commits into
mainfrom
farm/4e6d5c80/fix-vacuous-jest-globals-test

Conversation

@robobun

@robobun robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

What

The second test in test/js/bun/test/test-auto-import-jest-globals.test.js ("Jest's globals aren't available in every file") has been vacuous since it was added in 523b112:

const jestGlobals = await import("./jest-doesnt-auto-import.js");
expect(typeof jestGlobals.describe).toBe("undefined"); // etc.

The fixture only ever exported getJestGlobals (a function), never describe/it/etc., so every assertion read a nonexistent property off the module namespace object and passed unconditionally. The test could not fail.

Why not just call getJestGlobals()

Correcting it to jestGlobals.getJestGlobals().describe would make it fail: #17734 intentionally made inject_jest_globals apply to every file loaded by bun test (fixing #12034), so the original premise, that non-entrypoint files under bun test do not see the injected globals, no longer holds by design. The vacuous test simply never caught the behaviour change. Verified:

// under `bun test`, from a helper file:
{"describe":"function","it":"function","test":"function","expect":"function", ...}

The positive side of that behaviour is already covered by test/regression/issue/12034/.

Fix

Replace the test with one that checks the scoping guarantee that does still hold: ordinary bun / bun -e runs (outside of bun test) do not inject the jest globals. Delete jest-doesnt-auto-import.js, whose name is now actively misleading and which is referenced nowhere else.

This is a test-only correction with no src/ change; the runtime behaviour is already correct and intentional per #17734.

Will trivially conflict with #36125 (both add a harness import at the top of the same file).


[stamp-90s] gate passed · iteration 1 · 2 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/bun/test/test-auto-import-jest-globals.test.js'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/test/test-auto-import-jest-globals.test.js
bun test v1.4.0 (521bdc4da)

test/js/bun/test/test-auto-import-jest-globals.test.js:
(pass) Jest auto imports [4.43ms]
(pass) Jest globals are not injected outside of `bun test` > bun -e [1082.15ms]
(pass) Jest globals are not injected outside of `bun test` > bun <file> [1193.33ms]

 3 pass
 0 fail
 15 expect() calls
Ran 3 tests across 1 file. [3.18s]
Exit: 0
diff hotspot
test/js/bun/test/jest-doesnt-auto-import.js        | 12 ------
 .../bun/test/test-auto-import-jest-globals.test.js | 46 +++++++++++++++++-----
 2 files changed, 36 insertions(+), 22 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file
file                                                    reads  edits  tests
test/js/bun/test/jest-doesnt-auto-import.js                 1      0      0
test/js/bun/test/test-auto-import-jest-globals.test.js      3      2      0

The "Jest's globals aren't available in every file" test imported the
jest-doesnt-auto-import.js fixture and then asserted
`typeof jestGlobals.describe === "undefined"` on the module namespace
object. The fixture only ever exported `getJestGlobals` (a function),
never `describe`/`it`/etc., so every assertion read a nonexistent
property and passed unconditionally. The test could not fail.

Calling `getJestGlobals()` instead would have made it fail: #17734
intentionally made `inject_jest_globals` apply to every file loaded by
`bun test` (fixing #12034), so the original premise (non-entrypoint
files under `bun test` don't get the globals) no longer holds by
design. The vacuous test simply never caught the behaviour change.

Replace it with a test for the scoping guarantee that does still hold:
ordinary `bun` / `bun -e` runs do not inject the jest globals. Delete
the fixture, whose name is now actively misleading.
@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is test-only and the modified test file passes on every lane.

Remaining CI red across builds 83508 and 83528 is unrelated to this change:

Not a duplicate of #36125: that PR fixes a parser bug (partial bun:test import suppressing injection) and only adds new tests to this file. This PR removes the pre-existing vacuous test at lines 13-24 whose premise was removed by design in #17734; #36125 leaves those lines untouched. The two will need a trivial harness import merge.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The Jest globals test now runs a separate Bun process, checks the runtime types of Jest-style global names, and verifies empty stderr and a successful exit.

Changes

Jest global scope verification

Layer / File(s) Summary
Spawned runtime global check
test/js/bun/test/test-auto-import-jest-globals.test.js
The test uses bunExe() and bunEnv to execute inline code, reports each Jest global’s typeof value as JSON, and asserts all are "undefined" with no stderr and exit code 0.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title concisely matches the main change: replacing a vacuous Jest globals scoping test with a real one.
Description check ✅ Passed The description covers the change and includes verification details, even though it does not use the template headings verbatim.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/js/bun/test/test-auto-import-jest-globals.test.js`:
- Line 31: Remove the exact empty-stderr assertion from the subprocess test
while preserving the assertions on parsed output and exit status that verify
global scoping. Do not add replacement stderr validation, since diagnostic
absence is not part of this test’s behavior.
- Around line 30-33: Update the subprocess assertions around the parsed stdout
in this test to assert one combined object containing stdout, stderr, and
exitCode, following the repository’s established combined-result pattern. Keep
concurrent stream draining via Promise.all and preserve the existing expected
values.
- Around line 18-24: Extend the test around the existing bun -e spawn to also
execute the same Jest-global assertion through a temporary JavaScript file using
the ordinary bun <file> entry point. Create the file hermetically within the
test, run it with the existing bunExe() and process-output assertion flow, and
cover both entry points without changing the expected global-name results.
- Around line 15-17: Replace the explanatory comment above the injection
regression test with only the exact issue URL, removing the surrounding prose
and shorthand reference while leaving the test behavior unchanged.
🪄 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: 91bc9bd3-8b1f-4bb7-a7b6-b8bfcb540ee9

📥 Commits

Reviewing files that changed from the base of the PR and between 4eb6f99 and aa2db4a.

📒 Files selected for processing (2)
  • test/js/bun/test/jest-doesnt-auto-import.js
  • test/js/bun/test/test-auto-import-jest-globals.test.js
💤 Files with no reviewable changes (1)
  • test/js/bun/test/jest-doesnt-auto-import.js

Comment thread test/js/bun/test/test-auto-import-jest-globals.test.js
Comment thread test/js/bun/test/test-auto-import-jest-globals.test.js Outdated
Comment thread test/js/bun/test/test-auto-import-jest-globals.test.js Outdated
Comment thread test/js/bun/test/test-auto-import-jest-globals.test.js Outdated
@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. bun test: keep injecting jest globals alongside a partial bun:test import #36125 - Both PRs modify test/js/bun/test/test-auto-import-jest-globals.test.js to fix jest globals injection test coverage; bun test: keep injecting jest globals alongside a partial bun:test import #36125 also includes a runtime fix in the parser

🤖 Generated with Claude Code

@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:15 PM PT - Jul 27th, 2026

@robobun, your commit 521bdc4 has 1 failures in Build #83528 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36132

That installs a local version of the PR into your bun-36132 executable, so you can run:

bun-36132 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only correction replacing a vacuous assertion with a real subprocess check.

What was reviewed:

  • Confirmed the old test was vacuous (fixture exported only getJestGlobals, never describe/it/etc., so typeof jestGlobals.describe was unconditionally "undefined").
  • Verified jest-doesnt-auto-import.js has no remaining references in the repo.
  • New tests follow harness conventions: bunEnv/bunExe/tempDir, concurrent pipe draining, await using, stderr → parsed stdout → exitCode assertion order, test.concurrent for the two independent spawns.
  • Both bun -e and bun <file> entry points covered; all CodeRabbit threads addressed or declined with sound reasoning and marked resolved.
Extended reasoning...

Overview

Test-only PR touching two files under test/js/bun/test/: it deletes the jest-doesnt-auto-import.js fixture and rewrites the second test in test-auto-import-jest-globals.test.js. The original test asserted typeof jestGlobals.describe === "undefined" etc. against a module namespace that never exported those names — it read nonexistent properties, so it passed unconditionally and could not detect the #17734 behaviour change. The replacement spawns bun -e <src> and bun entry.js (via tempDir) and asserts every jest global reports typeof === "undefined" outside of bun test mode.

Security risks

None. Test-only; no src/ changes, no network, no external inputs. Subprocesses use bunExe()/bunEnv and a hermetic temp dir.

Level of scrutiny

Low. This is a small, mechanical test correction with a clear, verifiable premise. The vacuous-test analysis checks out by inspection of the deleted fixture. The new test follows the repo's documented spawn pattern (concurrent Promise.all drain, await using, using tempDir, stderr/stdout/exitCode ordering, test.concurrent). Grep confirms the deleted fixture is unreferenced.

Other factors

All four CodeRabbit inline threads are resolved: the bun <file> variant was added in 4667960, and the three declined suggestions (comment prose, combined-object assertion, empty-stderr gate) were declined with reasonable justifications that align with repo conventions (bunEnv sets BUN_DEBUG_QUIET_LOGS=1; this isn't a test/regression/issue/ file). The PR description notes a trivial import-line conflict with #36125, which is a merge-ordering matter and not a correctness concern here.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only correction replacing a vacuous assertion with a real subprocess check.

What was reviewed:

  • Confirmed the old test was vacuous (asserted typeof on module-namespace properties the fixture never exported) and the deleted fixture is referenced nowhere else.
  • New tests follow harness conventions: bunEnv/bunExe/tempDir, test.concurrent, concurrent pipe draining, exitCode asserted last.
  • Verified test/regression/issue/12034/ exists to cover the positive (injection-applies-under-bun test) side, so no coverage is lost.
Extended reasoning...

Overview

Test-only PR touching two files under test/js/bun/test/. It deletes jest-doesnt-auto-import.js and rewrites the second test in test-auto-import-jest-globals.test.js. The old test asserted typeof jestGlobals.describe === "undefined" etc. on the module namespace object, but the fixture only exported getJestGlobals — so every assertion read a nonexistent property and passed unconditionally. The replacement spawns bun -e and bun <file> (via tempDir) and asserts that all eight jest global names are typeof "undefined" outside bun test mode.

Security risks

None. Test-only; no src/ changes, no network, no untrusted input handling.

Level of scrutiny

Low. The change is small, self-contained, and mechanical. The PR description correctly diagnoses why the old test was vacuous and why simply calling getJestGlobals() would fail (PR #17734 intentionally made injection apply to all files under bun test). I confirmed test/regression/issue/12034/ exists, so the positive-injection behaviour remains covered. I confirmed via grep that the deleted fixture has no other references.

Other factors

The new tests follow repo conventions cleanly: bunEnv/bunExe, tempDir with using, await using on the spawned process, Promise.all draining of stdout/stderr/exited, stderr checked before parsed stdout before exitCode, and test.concurrent for the two independent spawns. All four CodeRabbit inline threads are resolved — one was addressed (added the bun <file> variant) and three were withdrawn after the author gave sound justifications (bunEnv sets BUN_DEBUG_QUIET_LOGS=1 so stderr === "" is safe; the explanatory comment is not in test/regression/issue/ so the URL-only rule doesn't apply; separate assertions are the documented pattern when stdout needs parsing). The PR notes a trivial merge conflict with #36125 on the harness import line, which is expected and easy to resolve. Tests pass per the evidence block in the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant