Repository navigation
Conversation
|
Updated 7:20 AM PT - Oct 19th, 2026
✅ @robobun, your commit 3e2996e5d393086af7f4a3f7f44320d9dfc574cd passed in 🧪 To try this PR locally: bunx bun-pr 45790That installs a local version of the PR into your bun-45790 --bun |
StatusReproduced on tmpfs with two copies of one tree of 12 test files, created in opposite orders. bun 1.4.3 and main run On ext4 the two copies list in the same hash order, so the tests assert the literal order. CI is green on 3e2996e (181 of 181 jobs). One decision is open for a maintainer: commit 2 changes the default file order of a run with no flag. Commit 1 (the seeded shuffle) stands alone without it. If commit 2 stays, merge #38874 first or in the same release, because snapshot numbers depend on the file order until then. PR: #45790 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change defines test-file ordering by absolute-path bytes and applies it to discovery, sharding, seeded execution, and non-randomized parallel execution. CLI tests cover ordering behavior, and documentation describes ordering and seed guarantees. ChangesTest-file ordering
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: test files use path order as the basis for execution and seeded shuffling. |
| Linked Issues check | |
Active linked issue #18987 requires sequentially named test files to run in filename order. Scanner::scan sorts files appended by each scan in absolute-path byte order. sort_test_files also establ… |
| Out of Scope Changes check | |
The changes stay within #18987. The scanner sort, shared ordering helper, command integrations, documentation, and regression tests all support deterministic test-file ordering or its required schedul… |
| Description check | |
The description clearly explains the problem, implementation, scope, limitations, performance impact, and verification. It does not use the exact template headings, but it provides the required inform… |
Full details: Linked Issues check
-
Explanation
Active linked issue
#18987requires sequentially named test files to run in filename order.Scanner::scansorts files appended by each scan in absolute-path byte order.sort_test_filesalso establishes this order before seeded randomization, sharding, and parallel scheduling. The new regression tests cover01_seller,02_product,03_user, and04_order, plus directory discovery and seeded runs.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#18987. The scanner sort, shared ordering helper, command integrations, documentation, and regression tests all support deterministic test-file ordering or its required scheduling paths. No unrelated behavior is demonstrated.
Full details: Description check
-
Explanation
The description clearly explains the problem, implementation, scope, limitations, performance impact, and verification. It does not use the exact template headings, but it provides the required information in equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two things: the comparator swap in parallel/runner.rs (bun_core::order to strings::order via sort_test_files) is behavior-preserving since both are plain byte order and the is_sorted_by fast path uses the same ordering, so --shard and --parallel membership does not move on a given platform; and dropping the lowercased-basename sort that regression/issue/26851 relied on is covered, because the whole appended slice is now byte-sorted and a_* still precedes b_* under --bail.
Extended reasoning...
The change splits Scanner::scan into a walk plus a per-argument byte-order sort, removes the cached-EntryMap lowercased-basename sort, centralizes the three existing sorts into sort_test_files, and sorts before the seeded shuffle; it touches no security-sensitive surface. Not approving because inline findings on multi-argument discovery (duplicate and missing files) and Windows separator ordering remain, the default file order for every user changes, and the PR itself says commit 2 needs a maintainer decision.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/cli/test/Scanner.rs— Users who name a file and a directory that contains it run that file twice, and the new docs now promise per-argument ordering without saying so.bun test ./a.test.ts ./pushes a.test.ts at src/runtime/cli/test/Scanner.rs:158 from the ENOTDIR branch without setting entry.abs_path, so the cwd walk pushes it again via the File arm, and each scan() sorts its own slice at Scanner.rs:132, so the duplicate survives sorting, sharding and seeding. The PR's newdirectoriesandfilescases in test/cli/test/test-file-order.test.ts:89-90 never overlap arguments. Fix: a path must appear once in the run list; dedupe by path bytes after the per-scan sort (adjacent duplicates are cheap to drop there) or mark the entry's abs_path in the ENOTDIR branch.Why this was flagged
Trigger:
bun test <file> <dir-containing-file>, through TestCommand::exec calling Scanner::scan once per argument. The ENOTDIR branch at src/runtime/cli/test/Scanner.rs:150-159 appends the interned path to test_files but does not touch the resolver's fs::Entry, so the 'already seen it!' guard in next() (Scanner.rs:~356,!entry.abs_path.is_empty()) does not fire when the directory walk reaches the same file and pushes it again. The base has the same two pushes, so the duplicate run is pre-existing. What the diff changes: sort_test_files runs per scan() slice, the seeded path at src/runtime/cli/test_command.rs:2352 sorts and shuffles the list including the duplicate, and the round-robin at test_command.rs:2251-2258 can place the two copies in different shards. The new doc at docs/test/discovery.mdx:93 states Bun 'runs the files of each argument in turn' with no mention of the double run, and the new test file adds ordering cases that never overlap arguments. Remedy: dedupe the run list by path bytes once after scanning, which also fixes the --shard split.Verification: pre-existing. Trigger:
bun test ./a.test.ts ./. The ENOTDIR branch at src/runtime/cli/test/Scanner.rs:150-158 appends the interned path without touching anyfs::Entry, so the only dedup guard (Scanner.rs:380-383) sees an emptyabs_pathand pushes the path again. The base commit has the same pushes and no dedup either, so merging this PR neither introduces nor widens the duplicate. -
🟣
src/runtime/cli/test/Scanner.rs— Users who runbun test ./test ./get none of the cwd's own test files, although the new docs at docs/test/discovery.mdx:92 now promise that Bun runs the files of each argument in turn. The second scan reaches src/runtime/cli/test/Scanner.rs:173 withhas_iteratedalready true from the first argument, and the cwd listing is served from the resolver cache, so no entry is visited and nothing is appended. Fix: make the per-argument promise hold by walking a cached listing per scan (resethas_iteratedinscan()or dedupe byentry.abs_path), or drop the sentence from the docs until #42921 lands.Why this was flagged
Trigger: two or more path arguments where a later one is a directory the resolver already listed (the cwd is always cached before the scanner runs, see the comment at Scanner.rs:177-180), e.g.
bun test ./test ./through the loop at src/runtime/cli/test_command.rs:2044-2046. The first scan setshas_iterated = trueinnext()at Scanner.rs:331. The second scan getsEntriesOption::Entriesfor the cwd and skips the whole block at Scanner.rs:173-188, so the cwd's test files never reachtest_files; the sort at Scanner.rs:132 then sorts an empty slice. The base branch drops the same files, but the PR adds the sentence at docs/test/discovery.mdx:92 that each argument's files run in turn and the--shard/--seeddocs say they start from all the files, which is now a false claim users will rely on. The dismissal relied on the PR naming #42921/#43361 as follow-ups; neither is in this checkout. Remedy: reset or scopehas_iteratedperscan()and dedupe viaentry.abs_path, or remove the docs promise.Verification: Pre-existing, acknowledged in diff. Trigger: two or more path arguments where a later one is a directory the resolver already listed (e.g.
bun test ./test ./). The block atScanner.rs:173-188, gated byif !self.has_iterated, is skipped, so nothing from the cwd listing is appended totest_files. This guard is identical on the base commit, so merging does not make the base worse.
The seeded shuffle started from the list in the order the scanner found the files: the order in which each directory lists its entries, and the order of the path arguments. The listing order differs between file systems and between two checkouts of one tree, so one seed gave another file order on another machine. The shuffle now sorts the list by path first, as --shard and --parallel already do. The four places that need path order call one helper, scanner::sort_test_files. It does nothing for a list that is already in path order.
The scanner appended test files in the order each directory listed them, so a run with no flag had another file order on another file system or checkout. --bail then stopped after another set of files. Scanner::scan now sorts the files it appended by path. A run keeps the order of the path arguments, as before. The sort of the cached cwd listing is gone: the sort of the found files replaces it.
b211038 to
3e2996e
Compare
|
On the two findings outside the diff (both exist on main, as the review says):
The helper nit is fixed in aa5d1b3. The branch is also rebased on main. The description has one new line under Downsides: a |
Fixes #18987
Problem
bun testruns test files in each directory's listing order, which differs per file system and checkout.--seed=Nshuffles that list, so a seed does not replay elsewhere. The docs promise: "The same seed always produces the same test execution order."Scanner::next(src/runtime/cli/test/Scanner.rs) appends files as listed. Only--shardand--parallelsort.Fix
scanner::sort_test_filessorts for it,--shardand--parallel.Scanner::scansorts the files it appended. Path arguments keep their order.test/cli/test/test-file-order.test.ts(main fails 11 of 11) andtest-changed.test.ts. Also test-shard, isolation, 12782, 26851. Self-reviewed: 27 concerns raised, 24 addressed.Background
TestCommand::execshards, shuffles and runs.--shard.Downsides
Bsorts beforea. An order-dependent suite can flip (Async operation is blocked in workspace #6655), and a.snapnumbered under the old order fails in CI until bun test: number snapshots per test file regardless of which files ran before #38874 lands..text: -512 B.Notes
The decision. Commit 2 makes a run with no flag use flat path order. Today that run uses the cwd's files first (by lowercased name), then breadth-first, each directory in listing order. 7a5af8b sorted the whole list this way "so run order (and --bail, JUnit output) is deterministic". 36fabf3 dropped it 8 hours later "to preserve explicit file order". Commit 2 sorts per
scan()call, so it keeps that:test/regression/issue/12782.test.tspasses unchanged.If flat path order is not wanted, drop commit 2. Then only seeded runs change, and they run what
--shard=1/1 --seed=Nalready runs on main. #18987 and #36636 stay open in that case.Why the run list and not the resolver (the #36636 review).
bun testneeds no sorted directory listing. Every push to the run list happens duringScanner::walk, which onlyScanner::scancalls.src/resolver/fs.rs:565keys the map on the lowercased name, so it holds one of two names that differ only by case.--seedwith several path arguments independent of the argument order.src/router/lib.rs:492,src/runtime/cli/multi_run.rs:977,src/lint/driver/fmt/files.rs:830(per argument, keeps argument order).bun checksorts per directory (src/sema/driver/host.rs:219).node --testsorts its file list (createTestFileListends withArrayPrototypeSort(results)). Deno sorts before it shards and before it shuffles (cli/tools/test/mod.rs:specifiers.sort(); specifiers.shuffle(&mut rng);).Relation to open PRs.
bun testscans every directory #42921: its cache-hit replay is still needed.DirEntry::sorted_entries()is then not needed for order. Its test "a directory the resolver already listed is scanned in name order" expects a, B, c, D, e, F, g, H. With this PR that directory runs B, D, F, H, a, c, e, g.Scanner.rsnext to the block this PR edits. The sort runs after the walk, so it holds under each of them.#[optimize(size)]onScanner::scan. After a rebase the attribute belongs onScanner::walk. It also adds text after the--seedsentence indocs/test/index.mdx.Snapshots. Snapshot numbers depend on the files that ran before in the same process (#38874). A
.snapwritten under the old file order can miss under the new one. Example:z.test.tstakes one file snapshot, andsub/b.test.tstakes an inline snapshot and then a file snapshot in testb. 1.4.3 runsz.test.tsfirst and writesexports[`b 1`]. This branch runssub/b.test.tsfirst and looks upb 2. WithCI=truethat run fails:Snapshot creation is disabled in CI environments unless --update-snapshots is used. #38874 numbers snapshots per file, which removes the dependency. Merge it before this PR or in the same release.What moves, once.
test/): 2,345 positions change. Of 1,602,326 pairs of files at different depths, 755,205 (47%) reverse. The model for today is breadth-first with each directory in name order.bunfig.toml. The order of tests inside a file does not change.--shardand--parallelmembership does not move on any platform. The comparator is the one they had.Not fixed.
bun test ./test/*.test.ts) arrives in the shell's order, and a run with no--seed,--shardor--parallelkeeps it. Bun's own shell does not sort glob matches../test ./against./ ./test) until resolver: call the directory iterator on a cache hit, sobun testscans every directory #42921 or bun test: list a test file once when two path arguments select it #43361.--parallelabove one worker, a seed fixes the order of the list and of the tests in a file. Which worker runs a file depends on timing.\, sotest/a/b.test.tsandtest/a0.test.tsorder differently there than on POSIX (479 of 2,345 files in this repo have another index). A comparator that reads\as/would give one order on every platform. It would move Windows--shardmembership once, so it is not in this PR.Log.entity.test.tsbeforeMongoDBTransport.test.tson every machine. bun test: sort every scanned directory so test-file order is stable across filesystems #36636 found that this order gives the timeout of that issue, so it can now show on a machine where the run passed before.10_xruns before1_x. Pad the numbers.Measurements (linux x64, release builds of main 017b243 and this PR, same build directory).
bun testprocess (plain /--seed/--shard/--parallel/ both), 12 files: from the cwd main 1/1/2/2/3, PR 1/1/1/1/1. With a directory argument main 0/0/1/1/2, PR 1/1/1/1/1. Counted with a gdb breakpoint onindex_sort::sort_stable.--shard0, one file argument 0. Counted with gdb inside the sort. valgrind is not installed here.--shard+2,347 (the check that the list is in order), one file argument 0.--shard=5000/500029.1 to 28.9 ms, one file argument 48.6 to 49.0 ms. Two runs of main differ by 1.2 ms..text81,368,037 to 81,367,525 B (-512 B),.rodataequal (size -A). bloaty is not installed here.openat/getdents64/closein discovery (bun test --shard=5000/5000): 1,489 / 2,992 / 1,488 on both. Counted with gdbcatch syscall. strace is not installed here.Tests. Stock 1.4.3 and a release build of main fail 11 of 11 in
test-file-order.test.tson ext4. The debug build of this branch passes 11. Rows that also pass on main guard the sorts that moved into the helper:--shard=1/1and--shard=1/2 --timingswith directory arguments out of order, and--parallel=2 ./z.test.ts ./apple.test.ts(read from the JUnit report). The tests are in a new file because they cover one property across a plain run,--seed,--shard,--parallelandbunfig.tomlwith one fixture helper. Also run:test-changed,test-shard,isolation,pass-with-no-tests,path-ignore-patterns,test-randomize,coverage,junit,mock-module,12782,26851.parallel.test.tspasses on the release build except--parallel --dots prints one status character per test. Its check fails in 18 of 30 runs on a release build of main and in 22 of 30 on this branch, so it does not come from this change.Self-review. Not taken: folding this into one PR with #42921 and #43361 (this sort runs after the walk and holds under each of them), a sort key relative to the project root (the key stays the one
--shardhas), and a note aboutbun run --filter(a probe here did not show an order difference).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts