Repository navigation
test runner: drop the allow_in_preload flag from GetActiveCfg - #39143
Conversation
The flag was constant per caller: hooks passed true, test/describe passed false. Move the preload check into clone_active_strong (the test/describe path) and have the hook path call the runner lookup directly, so there is no flag to keep in sync with the caller. Error messages and their order are unchanged; tests pin them. Removes the derived_field baseline entry for bun_test.rs.
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit 7d480ec has some failures in 🧪 To try this PR locally: bunx bun-pr 39143That installs a local version of the PR into your bun-39143 --bun |
|
Warning Review limit reached
Next review available in: 23 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review.
|
There was a problem hiding this comment.
LGTM — behavior-preserving removal of a redundant flag.
What was reviewed:
- Traced both callers:
call_as_function(test/describe, wasallow_in_preload: false) now hits the inlined preload check inclone_active_strong;generic_hook_impl(hooks, wastrue) callsget_test_rootwhich never had the check apply — same three throws in the same order on both paths. - Confirmed no other references to
GetActiveCfg/get_active_test_root/js_fns::clone_active_strongexist;SignatureisCopyso pass-by-value is fine. - New tests drain pipes concurrently, snapshot exact messages, and assert the preload
beforeAllactually runs — they pin existing behavior (expected for a pure refactor).
Extended reasoning...
Overview
Removes the two-field GetActiveCfg struct from src/runtime/test_runner/bun_test.rs to satisfy a mordant derived_field lint: allow_in_preload was a constant per caller (hooks always true, test/describe always false), so the flag encoded nothing beyond "which of the two callers am I". get_active_test_root becomes get_test_root (runner-exists check only); the preload check moves inline into clone_active_strong, the sole path that ever needed it. ScopeFunctions.rs updates its one call site and drops the GetActiveCfg import. One line removed from mordant-baseline.toml. Two new subprocess tests in bun_test.test.ts pin the three error messages and verify preload hooks still register.
Security risks
None. Internal test-runner control flow; no untrusted input handling, no auth/crypto, no new unsafe blocks or lifetime changes.
Level of scrutiny
Low. This is a small mechanical refactor (~30 lines net) that inlines a boolean whose value was statically determined per call site. I verified equivalence by tracing both call paths: for call_as_function, old = runner check → (is_in_preload && !false) preload throw → active-file check; new = runner check → is_in_preload preload throw → active-file check — identical. For generic_hook_impl, old = runner check → (is_in_preload && !true) never throws → return root; new = runner check → return root — identical. Grep confirms these are the only two callers and no stale references remain.
Other factors
The two new tests follow harness conventions (tempDir, bunEnv, test.concurrent, Promise.all on stdout/stderr/exited, exact snapshots, exit-code asserted last). The PR description correctly notes they pass on release too — appropriate since this is a no-behavior-change refactor and the tests are regression pins, not proof-of-fix. The author scoped the baseline change to just the fixed entry and explicitly left two unrelated stale entries for a separate PR, which is the right call. No prior human review comments to address.
Problem
derived_fieldonGetActiveCfg(src/runtime/test_runner/bun_test.rs:79):allow_in_preloadhas the same value for a givensignatureat both places the struct is built.generic_hook_impl) always passestrue, thetest/describeentry point (ScopeFunctions::call_as_function) always passesfalse. The Zig original had the same two callers with the same two constants, so the flag only ever encoded "which caller am I".Signature, though:Signatureis the enum used to name the caller in error messages (Signature::Stris any static name, it happens to be used only by hooks today), so aSignature::allow_in_preload()method would be an arbitrary rule hung on a display type.Fix
GetActiveCfgand the flag.get_test_root(wasget_active_test_root) only does the runner lookup and takes theSignaturefor its error message;clone_active_strong, thetest/describepath, does the preload check inline before looking up the active file. The hook path callsget_test_rootdirectly, as it already handles preload itself (get_active_file_unless_in_preload, a few lines down).outside of the test runner,during preload,outside of a test file) are raised in the same order with the same messages on both paths.mordant-baseline.toml: dropped thederived_fieldentry forbun_test.rs. A fullbun run rust:mordant:baselineadditionally drops two entries unrelated to this change that went stale when install: let the walker own the cache dir it walks and build InstallDirState in one go #38271 and crypto: store PBKDF2's key length as usize #37648 landed (always_unwrapped_optioninPackageInstall.rs,narrowed_two_waysinnode_crypto_binding.rs); left out of this PR on purpose.test/js/bun/test/bun_test.test.ts: two new tests pin theduring preloadandoutside of the test runnermessages fortest/describe/beforeAll, plusbeforeAllworking from a preload. They pass on the release build too, as expected for a refactor.bun bd testontest/js/bun/test/{bun_test,test-test,preload-test,expect-extend-preload,jest-hooks,describe,jest-each,nested-describes,test-only,concurrent}andtest/cli/test/bun-test.test.ts: 0 failures.cargo dylint --all -p bun_runtimewith the baseline line removed: reports exactly thisderived_fieldfinding on the old source, nothing on the new source.Background
bun test --preload <file>runs the preload script before any test file is loaded. At that point there is no active test file, sotest()/describe()have nothing to attach to and throw; hooks are allowed and attach toBunTestRoot.hook_scope, becoming global hooks for every file.Signature(js_fns::Signature) is how the test runner names the calling function in those error messages: either aScopeFunctionsobject, which prints as the chain the user called (test,test.skip,describe.only, ...), or a static string such asbeforeAll().bun run rust:mordant;mordant-baseline.tomlrecords pre-existing findings per (lint, file) so CI fails only on new ones, and an entry is removed once its site is fixed.