Conversation
58 tests in 27 files were marked todo but pass at HEAD (bun test --todo flags them). Remove the markers, and fix the bodies that were the actual problem: the Readable _events test asserted Writable's keys, the express name.txt fixture had a trailing newline that upstream's does not, the bunfig plugin preload fixture no longer proved the preload ran once .yaml imports became native, the serve Range tests accepted both outcomes, the spawn cancel test relied on sleeps, and the CONNECT socket timeout test wrote after end() when the idle timer re-fired during a slow close.
Contributor
|
Warning Review limit reached
Next review available in: 18 minutes 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 (29)
Comment |
Collaborator
Author
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it enables 58 tests across 27 files — including several timing-sensitive ones (grpc drain/connection-injector, spawn cancel, socket timeout, client-abort) and rewrites six test bodies — a human look and green CI across all lanes would still be worthwhile before merging.
What was reviewed:
- The six test-body rewrites: spawn-stdin cancel now awaits the round-trip instead of sleeping; node-http-connect uses
onceto avoid write-after-end; Readable_eventskeys corrected; Range assertions tightened; preload fixture checksrequire.cache;describe.todonarrowed to innerit.todowhere one case still fails. fixtures/name.txtnewline removal — only referenced byres.sendFile.test.ts, no other consumers.- The
describe.todo→describeblocks (inflate: false,rootoption, Connection injector, revokeObjectURL) — verified the enabled inner tests match what the description claims and no still-failing test lost its marker.
Extended reasoning...
Overview
This is a test-only housekeeping PR that removes stale .todo markers from 58 tests across 27 files whose bodies now pass at HEAD, following the same pattern as #34857. No src/ files are touched. Most changes are pure marker removals (it.todo → it, describe.todo → describe, or dropping todo: true from itBundled). Six test bodies were adjusted where the body itself was buggy, and one fixture (name.txt) had a trailing newline removed to match upstream express.
Security risks
None. This is purely test-suite maintenance with no runtime, build, or dependency changes.
Level of scrutiny
Moderate. Each individual change is mechanical, but the aggregate is large and includes judgment calls about flakiness for timing-sensitive tests (grpc drain and TLS/http2 connection injection, spawn-stdin cancel, socket timeout, the two express client-abort cases, fork IPC). REVIEW.md is emphatic about not enabling flaky tests and about tests failing for the right reason. The PR description documents per-platform verification (debug Linux, Windows canary) and 5–20× loop runs for the timing-sensitive cases, but CI across all lanes is the real gate.
Other factors
- The body rewrites all follow REVIEW.md guidance: the spawn-stdin rewrite replaces
setInterval+sleepwith awaiting the actual round-trip and thecancel()promise; the node-http-connecton→oncefix prevents a write-after-end when the idle timer re-fires during a slow close; the Readable_eventstest was previously asserting Writable's keys on a Readable; the Range-request assertions were tightened from either/or to exact 206/416 +Content-Range. - I confirmed
fixtures/name.txtis only referenced byres.sendFile.test.ts, so the newline removal cannot break other tests. - Where a
describe.todowas un-todoed with one still-failing case inside (express.textwhen "text/html",res.sendFileasync local storage), the marker was correctly narrowed onto the failingit, matching neighboring blocks. - The removed macOS comment on
bun-test.test.tsabsolute-path tests is covered by the PR's stated cross-platform verification; CI will confirm. - Given the scale (29 files) and the number of newly-live timing-sensitive tests, this is beyond the "simple, mechanical, obvious" bar for auto-approval — a maintainer should confirm CI is green on all platforms and sanity-check the rewrites.
Collaborator
Author
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit d6c08bb has some failures in 🧪 To try this PR locally: bunx bun-pr 39058That installs a local version of the PR into your bun-39058 --bun |
This was referenced Aug 29, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 30, 2026
…rkers (#40894) ### Problem - `test/config/bunfig/preload.test.ts` took 42.28s on the windows 2019 x64 lane of build #108217. It runs 18 `bun` child processes one after the other, so its wall time is the sum of 18 process starts. - Most tests asserted `expect(out).toBeEmpty()`. The fixture entry files did the real check with `node:assert`, so the test only proved that the child printed nothing and exited 0. ### Fix - Every `describe` is `describe.concurrent` and the last test is `test.concurrent`. With no hooks, all 18 spawns form one concurrent group. No test writes to a fixture directory. - Every entry fixture prints `globalThis.preload`, the marker each preload pushes. Each test matches the exact stdout with `toMatchInlineSnapshot` and asserts `stderr` is `""` before the exit code. The `bun test` case also snapshots its report. - `multi/empty.ts` and `multi/cli-merge.ts` are gone (`index.ts` prints the list). The `many` and `relative` markers name their own directory. - Verified: `bun bd test test/config/bunfig/preload.test.ts`, 3 runs each. Linux debug (ASAN): 8.2 to 8.5s before, 3.3s after. Windows Server 2019 debug: 6.2s before, 1.9s after. Windows release: 0.33s before, 0.10s after. ### Background - The 42s did not reproduce on a Windows Server 2019 machine. CI runs this file in a `bun test --parallel` batch of 167 files, so the per-file timer includes contention. - `--max-concurrency` is 20 by default and 5 under ASAN, hence 3.3s on the Linux debug build. - The `it.skip` and FIXME cases still fail on main and stay as they are. PR #39058 enables the plugin `it.todo`. The merge with it is clean and the merged file passes. <details><summary>Notes</summary> Probes of the FIXME cases with the debug build at 1ab272b: - `bun --config=<abs>/simple/bunfig.toml <abs>/simple/index.ts` from another cwd: `error: preload not found "./preload.ts"`. - `relative` fixture (`preload = "preload.ts"`): `globalThis.preload` is `undefined`. - `bun --preload=./preload3.ts run index.ts`, `bun run --preload ./preload3.ts index.ts`, `bun run --preload=./preload3.ts index.ts`: preload3 is missing from the list (#38599 covers this). - `bun --preload ./preload3.ts run index.ts`: prints the `bun run` usage text and exits 0. Per-test times on Windows debug are still 0.4 to 0.6s each. The file total of 1.9s is 1.2s of fixed cost (a test file that only imports `harness` takes 1.22s on that build) plus one round of overlapping spawns. Spawning itself is cheap there: 18 `Bun.spawn` calls of the debug binary complete in 81ms. `bun test` in the `simple` fixture does not load the top-level `preload` (only `[test].preload` applies to `bun test`, see `src/bunfig/bunfig.rs`). `simple/index.fixture-test.ts` and `relative/index.fixture-test.ts` are unreferenced and left alone. CI timings for this file in build #108300 (release builds, the file runs alone as a modified test): windows 2019 x64 161ms (median before this PR: 290ms in `test/expected-durations.json`), x64-asan 733ms (before: 1550ms), linux lanes 57 to 133ms, darwin 85 to 168ms. 18 pass on every lane. Merge check: `git merge-tree --write-tree HEAD pr-39058` is clean. The merged `preload.test.ts` with the plugin test enabled passes (19 pass) with the `bun-plugin-yaml` fixture dependency installed. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 2 · 14 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/config/bunfig/preload.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/config/bunfig/preload.test.ts bun test v1.4.1 (d578a8c) test/config/bunfig/preload.test.ts: (skip) Given a single universal preload > When `bun run` is run from a different directory but bunfig.toml is explicitly used, preloads are run (pass) Given a single universal preload > When `bun run` is run and `bunfig.toml` is implicitly loaded, preloads are run [308.06ms] (pass) Given a bunfig.toml with both universal and test-only preloads > `bun run index.ts` only loads the universal preload [291.20ms] (pass) Given a `bunfig.toml` with a list of preloads > When `bun run` is run, preloads are run [285.80ms] (todo) Given a `bunfig.toml` with a plugin preload > When `bun run` is run, preloads are run (pass) Given a `bunfig.toml` with a list of preloads > when passed `--config=bunfig.empty.toml`, preloads are not run [285.38ms] (skip) Given a `bunfig.toml` file with a relative path without a leading './' > preload = 'preload.ts' is treated like a relative path and loaded (pass) Given a bunfig.toml with both universal and test-only preloads > `bun test` only loads test-only preloads, clobbering the universal ones [320.57ms] (pass) Given a `bunfig.toml` with a list of preloads > When `bun --preload ./preload3.ts index.ts` is run, `--preload` adds the target file to the list of preloads [295.79ms] (pass) Given a `bunfig.toml` file with a relative path to a preload in a parent directory > When `bun run` is run, preloads are run [281.08ms] (pass) Test that all the aliases for --preload work > When `bun run` is run with --preload=./preload1.ts, the preload is executed [286.61ms] (pass) Test that all the aliases for --preload work > When `bun run` is run with --require=./preload1.ts, the preload is executed [281.16ms] (pass) Given a `bunfig.toml` with a list of preloads > When `bun --preload=./preload3.ts index.ts` is run, `--preload` adds the target file to the list of preloads [402. ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .../bunfig/fixtures/preload/many/preload1.ts | 2 +- .../bunfig/fixtures/preload/many/preload2.ts | 2 +- .../bunfig/fixtures/preload/many/preload3.ts | 2 +- .../fixtures/preload/mixed/index.fixture-test.ts | 6 +- test/config/bunfig/fixtures/preload/mixed/index.ts | 3 +- .../bunfig/fixtures/preload/multi/cli-merge.ts | 2 - test/config/bunfig/fixtures/preload/multi/empty.ts | 3 - test/config/bunfig/fixtures/preload/multi/index.ts | 3 +- .../bunfig/fixtures/preload/parent/foo/index.ts | 3 +- .../preload/relative/index.fixture-test.ts | 2 +- .../bunfig/fixtures/preload/relative/index.ts | 3 +- .../bunfig/fixtures/preload/relative/preload.ts | 2 +- .../config/bunfig/fixtures/preload/simple/index.ts | 3 +- test/config/bunfig/preload.test.ts | 142 +++++++++++---------- 14 files changed, 89 insertions(+), 89 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/config/bunfig/fixtures/preload/many/preload1.ts 0 0 0 test/config/bunfig/fixtures/preload/many/preload2.ts 0 0 0 test/config/bunfig/fixtures/preload/many/preload3.ts 0 0 0 …fig/bunfig/fixtures/preload/mixed/index.fixture-test.ts 1 1 0 test/config/bunfig/fixtures/preload/mixed/index.ts 1 1 0 test/config/bunfig/fixtures/preload/multi/cli-merge.ts 0 0 0 test/config/bunfig/fixtures/preload/multi/empty.ts 0 0 0 test/config/bunfig/fixtures/preload/multi/index.ts 1 1 0 test/config/bunfig/fixtures/preload/parent/foo/index.ts 1 1 0 …/bunfig/fixtures/preload/relative/index.fixture-test.ts 1 1 0 test/config/bunfig/fixtures/preload/relative/index.ts 1 1 0 test/config/bunfig/fixtures/preload/relative/preload.ts 1 1 0 test/config/bunfig/fixtures/preload/simple/index.ts 1 3 0 test/config/bunfig/preload.test.ts 2 3 0 ``` </details> <!-- robobun:evidence:end -->
This was referenced Aug 31, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
bun test <file> --todoreportsthis test is marked as todo but passesfor 58 tests in 27 files on main. CI never passes--todo(scripts/runner.node.mjsbuilds thebun testargv without it), so when the behavior behind a todo gets fixed without the marker being removed, the marker goes stale silently and the assertions stop running.Fix
todomarker from every test whose body passes today, adjusting the few whose body itself was the problem (table below). Test-only change; nosrc/involved.bun bd test <file> --todoon the 27 files no longer flags anything except the empty-bodiedcheck formatting for %pplaceholder inbun-test.test.tsthat bun test: fix inline snapshot conflict, beforeAll skip, and test.each title reporting #32817 deletes (left alone here, see below). The same 27 files also pass on Windows x64 with the current canary (for the sixitBundledcases, with the one-line registration fix from test/bundler: stop silently dropping every itBundled test on Windows #34552 applied locally, sinceitBundledtests do not register on Windows today), so nothing here needs a platform guard.bun bd test <file>passes on 25 of the 27 files; the other two only have pre-existing debug-build timeouts in tests this PR does not touch (listed in the details below), and pass in full on Windows and with the release binary.Enabled (58 tests, 27 files)
test/js/node/url/url-pathtofileurl.test.jstest/js/node/url/url-revokeobjecturl.test.jsdescribe.todo)test/config/bunfig/preload.test.tstest/cli/run/preload-test.test.jstest/cli/test/bun-test.test.tstest/js/node/http/node-http-connect.test.tstest/js/node/stream/node-stream.test.js_eventsfor Readable, Writable, Duplex, Transform, PassThroughtest/js/node/child_process/child_process-node.test.jstest/js/bun/spawn/spawn-stdin-readable-stream-edge-cases.test.tstest/js/bun/http/bun-serve-file.test.tsdescribe.todo)test/js/bun/yaml/yaml.test.tstest/bundler/transpiler/transpiler.test.jstest/bundler/esbuild/dce.test.tstest/bundler/esbuild/ts.test.tstest/bundler/esbuild/packagejson.test.tstest/bundler/esbuild/splitting.test.tstest/bundler/esbuild/default.test.tstest/js/third_party/jsonwebtoken/{jwt.asymmetric_signing,validateAsymmetricKey,verify}.test.jsUnknown key type "dsa", validation with no algorithm)test/js/third_party/express/express.json.test.tsapplication/vnd.api+jsontest/js/third_party/express/express.text.test.tsdescribe.todo), custom typetext/html, type array parsestext/htmltest/js/third_party/express/res.location.test.tstest/js/third_party/express/res.send.test.tstest/js/third_party/express/res.sendFile.test.tsrootoption casestest/js/third_party/grpc-js/test-channel-credentials.test.tstest/js/third_party/grpc-js/test-server.test.tsdescribe.todo; TLS/http2 connection injection works now)Adjusted while enabling
node-stream.test.jsReadable_eventsprefinish/finish/drain). Node pre-populates a Readable's_eventswithclose/error/data/end/readable(checked against node v26), and Bun matches; the other four classes were already asserting the right keys.res.sendFile.test.ts(6 of the 13)fixtures/name.txthad picked up a trailing newline, so every assertion on a"tobi"body failed; upstream express's fixture has no newline (the other fixtures in that directory have none either). Fixing the fixture is what un-blocks transfer/ETag/304/rootcases.preload.test.tsplugin preloadbun-plugin-yamlonly exports a factory and.yamlimports are native now, so the fixture's yaml import no longer proved the preload ran. The fixture now also asserts the package is inrequire.cache, which is true only when bunfig loaded it (checked both ways). This is the only test covering a bare package specifier inpreload.bun-serve-file.test.tsRange requests/partial.txt), so these now assert 206 +Content-Range: bytes 0-4/13+Accept-Rangesand 416 +bytes */13against the per-method{ GET, HEAD }route.spawn-stdin-...-edge-cases.test.tscancel callbacksetIntervalplus two sleeps, and itscancelcleanup readthis.intervalwhich was never set. Rewritten: a stream that never closes, kill once its chunk has round-tripped through the child, then await thecancel()call. 10/10 under the debug build.node-http-connect.test.tssocket timeoutsocket.on("timeout")re-fires while the ended socket is still closing (the 408 write re-arms the idle timer), and the second run wrote afterend(); this reproduced 5/5 under the debug build with other tests running.oncemakes it independent of how long the close takes (8/8 afterwards).express.text.test.tswhen "text/html",res.sendFile.test.tsasync local storagedescribe.todoblocks with one passing test each: the block marker moved onto the single failing test, matching how the neighboring blocks in those files are marked.Left as todo (still fail at HEAD)
preload-test.test.jsworks/works from CLI: a runtime plugin returningloader: "json"still has its contents evaluated as JS (Missing 'default' export, same symptom as SyntaxError: Missing 'default' export in module... [yamlloader - example Runtime plugins] #9987).{}instead of empty when a parser skips,charset=UTF-8casing from send 0.18,app.disable("etag")not honored by v4sendFile, v5 route syntax inapp.router), or a real remaining failure. The threeapp.routercases that pass under--tododo so incidentally under v4 semantics and were left inside their v5 blocks.grpc-js/test-resolver.test.tsshould not keep repeating successful resolutions: passes, but is a 10 second test that also requireslocalhostto resolve to both127.0.0.1and::1; not worth enabling as-is.node-http-connect.test.tspause/resume,bun-serve-file.test.tshandles ETag, the remaining transpiler macro cases, the jsonwebtoken ES256/RSA-PSS cases,child_process-node.test.jsabort-signal: still failing.Not touched
test/cli/run/env.test.ts(test: un-todo the process.env string coercion test on POSIX #38864),snapshot-tests/.../different-directory.test.ts(test: give 22 matcher-less expect() statements a matcher and lint for new ones #38646),test/bundler/esbuild/lower.test.ts(file-widedescribe.todo; test/bundler: fail the file when itBundled registration throws anything but an auto-skip #38655 changes which of its cases register) andbundler_jsx.test.tsjsx/PragmaMultiple(only the Dev half passes; needs theprodTodowiring from test/bundler: fail the file when itBundled registration throws anything but an auto-skip #38655),extra.test.tsCaseSensitiveImport2/3 (resolver: keep sibling entries whose names differ only in case #38011 area, and they would need a case-insensitive filesystem guard),bun-test.test.tscheck formatting for %p(bun test: fix inline snapshot conflict, beforeAll skip, and test.each title reporting #32817 deletes it).can specify <option>cases intest/js/node/vm/vm.test.ts, the http2 set-cookie case incookies.test.ts. They need bodies, not un-todoing.bunshell.test.tsported from GNU bash(4 of ~40, two of them only because$TDIRis unset) and the sinonfake-timers.test.tsblocks (3 cases).How the list was produced
--todousing the released binary to find candidates; every candidate above was then re-run with a debug build (bun bd test <file>andbun bd test <file> --todo), and the touched files were run again on Windows x64 with the current canary.spawn-stdin-readable-stream-edge-cases.test.tsspawn options variations(not a todo) spawns three debug binaries back to back and exceeds 5s; it is left as is because spawn: surface stdin ReadableStream producer errors via onExit / unhandledRejection #36236 is already rewriting that test.node-http-connect.test.tstests should run on bunruns a whole nestedbun testwith the debug binary and exceeds 5s too (and its child-spawning neighbors sit close to the limit). The--todoruns also report a few todo bodies that time out as failures rather than todos, which is what bun test --todo: report a todo body that times out as todo instead of a failure #38153 is about; none of them are tests touched here.res.sendFile.test.ts(should error missing path) intermittently sees a thrownTypeError's.stackheader rendered as bareError, anddefault.test.ts --todoonce hit a debug-onlyZStr::as_cstr: interior NULassertion in the resolver. Neither involves a test touched here.