Repository navigation
Conversation
WalkthroughChangesThe PR adds Changed-first test execution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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 `@docs/test/parallel.mdx`:
- Around line 169-171: Update the changed-first documentation in
docs/test/parallel.mdx lines 169-171 to say that affected files are dispatched
first, removing the claim that every worker drains them before its normal chunk.
Update docs/test/index.mdx line 426 to state that --changed-first --bail can
surface affected failures sooner without promising unaffected files have not
started.
In `@test/cli/test/test-changed.test.ts`:
- Around line 408-556: Add timing-priority coverage to the changed-first test
suite around the existing fixture and run helper: create a timings file where
y.test.ts has a longer recorded duration than x.test.ts, invoke --changed-first
with --timings pointing to that file, and assert y.test.ts precedes x.test.ts in
the affected prefix while a.test.ts and b.test.ts remain in the unaffected tail.
Keep the existing membership and exit-code checks.
🪄 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: 38759e83-644b-4f48-89f0-d535197e14e3
📒 Files selected for processing (9)
docs/test/index.mdxdocs/test/parallel.mdxsrc/options_types/context.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/test-changed.test.ts
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 `@docs/test/parallel.mdx`:
- Line 171: Update the ordering description for the affected-file prefix to
qualify that the slowest-first ordering from --timings applies only when
--randomize is not enabled; with --randomize, preserve and document the
randomized order. Keep the remaining ordering, --parallel, --shard, and
option-exclusion statements 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: bcb5a185-0923-4d34-84ce-ffd717ae0af4
📒 Files selected for processing (6)
docs/test/index.mdxdocs/test/parallel.mdxsrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/test-changed.test.ts
There was a problem hiding this comment.
I didn't find any bugs, but this adds a new user-facing CLI flag and reworks how the parallel coordinator partitions and dispatches work — worth a maintainer look at the flag shape and the --shard/--timings/--parallel interactions.
What was reviewed:
scan_affected()extraction preservesfilter()'s fall-open behavior on scan failure; the dropped; running all testshint was restored in a3909dd.runner.rsrange offsets: worker chunks andt.partition()results are shifted bypriority_count, andt.costs(&sorted)still indexes the full list sofind_steal_victimslices line up.assign_work/has_undispatched_files/abort_all/abort_queued_filesall account for the newpriorityrange; priority files are never in any worker's own range so no double-dispatch.--changed+--changed-firstconflict check is order-independent (clap parses both before the check runs).
Extended reasoning...
Overview
Adds bun test --changed-first[=<ref>]: same git + module-graph detection as --changed, but affected test files are moved to the front of the run rather than the rest being skipped. Touches CLI arg parsing (Arguments.rs, context.rs), extracts a shared scan_affected() helper in ChangedFilesFilter.rs, computes and applies a stable partition in test_command.rs, and threads a shared priority: FileRange through the parallel coordinator (runner.rs, Coordinator.rs) so every worker drains it before its own chunk. Docs and 7 new tests round it out.
Security risks
None. The flag reuses the existing --changed git-spawn path (no new external input surface); the flag: &str threaded into run_git / get_changed_files is a compile-time literal, not user data.
Level of scrutiny
Medium-high. This is new user-facing API surface (per the repo's own review rubric, API-shape decisions want maintainer sign-off), and it changes how the parallel coordinator carves up and dispatches the file list — the offset arithmetic on worker ranges, the costs vector alignment, and the abort-accounting paths all had to be updated in lockstep. I traced each of those and they look correct, but the coordinator is exactly the kind of code where an off-by-one only shows up under a specific --timings + --shard + crash-recovery combination that CI doesn't exercise.
Other factors
- All prior review threads (comment-cop, CodeRabbit's docs-wording and
--timingstest coverage asks, my own warn-message nit) are resolved and the fixes are in the current diff. - Test coverage is good: serial order, parallel dispatch order (pinned to one worker via
--parallel-delay),--shardmembership invariance,--changed-first=<ref>, no-changes,--timingsslowest-first within the affected prefix, and the mutual-exclusion error. Tests are hermetic (temp git repos,gitEnv) and follow the existing--changedsuite's patterns. - The
k <= 1parallel fallback and the pure-serial path both receive the already-partitionedtest_filesslice, sopriority_countbeing dropped there is fine — order is preserved by iteration. detect_affected()doesn't consultconsume_watch_trigger()the wayfilter()does, so--changed-first --watchre-queries git on every restart rather than narrowing to the watcher's trigger set. That's a behavioral difference from--changed --watchbut arguably fine for a CI-oriented flag; flagging it as something a maintainer may want to weigh in on.
|
Updated 4:21 AM PT - Aug 4th, 2026
✅ @robobun, your commit 2eb628ad5f300afe73e4c616490cd38c9606c859 passed in 🧪 To try this PR locally: bunx bun-pr 36865That installs a local version of the PR into your bun-36865 --bun |
There was a problem hiding this comment.
No bugs found. This adds a new user-facing bun test flag and threads a shared priority range through the parallel coordinator's dispatch/steal/abort paths — worth a maintainer look at the flag semantics and the scheduler changes.
What was reviewed:
scan_affected()refactor preserves the pre-PR--changedfallback (returns full slice on scan failure) and the restored warn suffix.- Coordinator:
priorityrange is drained inassign_work, counted inhas_undispatched_files/abort_all, and cleared inabort_queued_files;find_steal_victimonly walks worker ranges so priority indices are never stolen. runner.rs: worker ranges andt.partition()results are offset bypriority_count;costsis built over the fullsortedlist so steal-by-cost indices stay aligned;rest_n = 0(all affected) yields empty worker ranges and everything goes through the shared queue.
Extended reasoning...
Overview
Adds bun test --changed-first[=<ref>]: same git + module-graph detection as --changed, but affected test files are moved to the front of the run instead of the rest being dropped. Touches CLI arg parsing (Arguments.rs, context.rs), the changed-files filter (ChangedFilesFilter.rs — extracts a shared scan_affected() helper and threads a flag string through git error messages), the serial ordering step in test_command.rs (stable-partition after --shard/--randomize), and the parallel scheduler (runner.rs + Coordinator.rs — new shared priority: FileRange at the head of the file list). Docs and 7 new tests in test-changed.test.ts.
Security risks
None identified. The git subprocess path and bundler scan are unchanged from --changed; the new code only reorders an already-discovered file list and adds a shared FileRange to the coordinator.
Level of scrutiny
Medium-high. This is new user-facing API surface (flag name, semantics, documented interactions with --shard/--parallel/--timings/--bail) and it modifies the parallel coordinator's dispatch, work-stealing, and abort accounting — code that is single-threaded but has careful raw-pointer aliasing invariants. REVIEW.md's API-design guidance applies. The implementation looks correct to me: assign_work checks priority.pop_front() before own range and before steal; has_undispatched_files, the interrupt summary, and abort_queued_files all account for the priority range; the k <= 1 fallback runs the caller-reordered files serially so priority_count is correctly unused there; t.partition(&sorted[prio..], k) returns slice-relative ranges that are then offset by priority_count before being used to index sorted/costs. But a maintainer should sign off on the flag itself and the scheduler wiring.
Other factors
All bot review threads are resolved. My earlier nit (dropped ; running all tests remedy hint on the module-graph-scan warning) was addressed in a3909dd — scan_affected() now takes an on_scan_fail string. CodeRabbit's requests (--timings test coverage, docs wording on parallel dispatch semantics) were addressed in 9eb097e; the --randomize doc caveat was declined with a reasonable justification. Test coverage is solid: serial order, parallel dispatch order (via a huge --parallel-delay to force single-worker observability), --shard membership invariance, --changed-first=<ref>, no-changes, --timings slowest-first ordering of the affected prefix, and the --changed+--changed-first conflict. CI build #88684 was still running at review time.
|
CI: build #88684 ran 190/196 lanes green; |
There was a problem hiding this comment.
No bugs found — deferring to a maintainer since this adds a new user-facing CLI flag and threads a priority range through the parallel scheduler's dispatch/steal/abort paths, which is worth a human sign-off on both the flag semantics and the Coordinator changes.
What was reviewed:
scan_affected()extraction preservesfilter()'s fallback (full slice on scan failure) and the compaction loop;detect_affected()returns an empty set on the same paths.runner.rsrange math:t.partition(&sorted[prio..], k)offsets are correct,costs(&sorted)covers the priority prefix sofind_steal_victimindexing stays aligned, and the non-timings branch'srest_nsplit still covers[priority_count, n)exactly.- Coordinator:
assign_workdrainsprioritybefore own-range and before stealing;has_undispatched_files,abort_all's not-started count, andabort_queued_filesall account for the priority range. - Both prior nits (warn-message suffix,
shardFilesstderr guard) were applied.
Extended reasoning...
Overview
Adds bun test --changed-first[=<ref>]: same git + module-graph detection as --changed, but affected test files are moved to the front of the run instead of the rest being dropped. Touches 9 files: the CLI flag/option (Arguments.rs, context.rs), a refactor of ChangedFilesFilter.rs to extract the shared bundler-scan/BFS into scan_affected() and add detect_affected(), the stable-partition in test_command.rs, a shared priority: FileRange in the parallel Coordinator/runner, two docs pages, and 7 new tests in test-changed.test.ts.
Security risks
None. The flag reuses the existing --changed git-spawn path (run_git with fixed argv, core.quotePath=off, -- separator before the user-supplied ref). No new external input surface beyond what --changed already accepts.
Level of scrutiny
Medium-high. This is new user-facing API (flag name, docs, interaction rules with --shard/--randomize/--timings/--parallel/--bail), and it modifies the parallel test scheduler — the priority range is threaded through assign_work, has_undispatched_files, abort_all, abort_queued_files, and the per-worker range/costs offset math in runner.rs. The ChangedFilesFilter.rs refactor is behavior-preserving for --changed (verified: same fallback on scan failure, same compaction, same module_graph_files return), but a maintainer should confirm the flag's semantics and the scheduler wiring.
Other factors
- Test coverage is good: serial order, parallel dispatch order (via
--parallel-delay=1000000to force single-worker), shard membership invariance + affected-first-within-shard,=<ref>, no-changes,--timingsslowest-first, and the--changed+--changed-firstconflict. The fixture deliberately names affected files (x, y) to sort after unaffected ones (a, b) so a system-bun run that ignores the flag would fail the order assertion. - All CodeRabbit and comment-cop threads are resolved; my two prior inline nits (dropped
; running all testswarn suffix,shardFilesstderr guard) were both applied. - CI build #88684: 190/196 lanes green with the touched test files passing everywhere; the 6 non-green lanes were macOS agent timeouts, not test failures.
--changed-first --watchis not wired toinit_watch_trigger()(only--changedis), so a watch restart would re-query git rather than use the trigger file — but the flag isn't documented with--watchand the behavior (prioritize by all uncommitted changes on each restart) is still coherent, so I don't consider it a bug.
--changed-first[=<ref>] uses the same git + module-graph detection as --changed, but instead of dropping unaffected files it moves the affected ones to the front of the run. The rest keep the order they would otherwise have. Useful for PR CI that still wants full coverage but wants fast feedback on what the diff touched (especially with --bail). - ChangedFilesFilter: extract the bundler scan + reverse-import BFS into scan_affected() shared by filter() and the new detect_affected(), which returns the affected test-file paths as a set so the partition can be applied after --shard/--randomize have reordered or sliced the list. Git error messages name whichever flag was passed. - test_command: compute the affected set before --shard, then stable- partition after --shard/--randomize. Among affected files, order slowest-first under --timings, otherwise by path. - parallel runner/Coordinator: new shared priority FileRange at the head of files that every worker drains (pop_front) before starting on its own chunk; per-worker ranges, stealing, and abort accounting cover the remainder unchanged (offset by the priority count). - Arguments/options: --changed-first[=<ref>] flag; rejected together with --changed. - docs: subsection in test/parallel.mdx and an entry in the Large codebases list in test/index.mdx.
2eb628a to
4c6ae09
Compare
|
Rebased onto main (4c6ae09, squashed to one commit). One conflict: main's #39324 moved the |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; the two nits I raised earlier have both been addressed and CI is green. Because this adds a new user-facing CLI flag and threads a priority range through the parallel coordinator's scheduling/stealing/abort paths, a maintainer look at the flag shape and the offset arithmetic in runner.rs would still be worthwhile.
What was reviewed:
scan_affected()extraction is behavior-preserving for--changed: same compaction, same fallback on scan failure (now with the restored "; running all tests" hint).runner.rsrange offsets:rest_n = n - priority_count, worker ranges cover[priority_count, n), timings partition is applied to the suffix and offset back;costsindexes the full sorted vec sofind_steal_victimsums correct slices.- Coordinator:
assign_workdrainsprioritybefore own range before stealing;has_undispatched_files,abort_allnot-started count, andabort_queued_filesall account for the priority range. k <= 1fallback still respects the ordering (files are already partitioned beforerun_as_coordinator).
Extended reasoning...
Overview
Adds bun test --changed-first[=<ref>]: same git + module-graph detection as --changed, but affected test files are moved to the front of the run instead of the rest being dropped. Touches 9 files: CLI arg parsing (Arguments.rs, context.rs), a refactor of ChangedFilesFilter.rs to extract the shared scan_affected() helper, the stable-partition + priority-count wiring in test_command.rs, a new shared priority: FileRange in the parallel Coordinator plus the per-worker range offset in runner.rs, two docs pages, and 7 new tests in test-changed.test.ts.
Security risks
None. The only external input is the git ref passed to --changed-first=<ref>, which flows through the same run_git path as --changed (spawned with an argv array, no shell). No auth, crypto, network, or untrusted-data parsing is touched.
Level of scrutiny
Medium-high. This is a new user-facing CLI flag (API surface — naming, interaction with --shard/--randomize/--timings/--parallel, and the mutual-exclusion with --changed are design decisions a maintainer should sign off on). It also threads a priority_count offset through the parallel coordinator's range partitioning, work-stealing cost lookup, not-started accounting, and abort paths — arithmetic that would silently skip or double-run files if off by one. I traced each of those paths (see the summary above) and they check out, and the 7 new tests cover serial/parallel/shard/timings/ref/no-changes/conflict, but the scheduling changes are the kind of thing that benefits from a second pair of eyes.
Other factors
- CI build #88743 passed on all platforms;
test-changed.test.ts,parallel.test.ts, andtest-shard.test.tsall green. - All prior review threads are resolved: my two nits (dropped "; running all tests" warn suffix;
shardFilesexit-code assertion order), CodeRabbit's docs-wording and--timingstest-coverage requests, and the comment-cop flags. - The
filter()refactor is behavior-preserving for the existing--changedpath (same compaction loop, same fall-open on bundler-scan failure); the existing 20--changedtests are untouched and still exercise it. - No CODEOWNERS coverage on the touched paths.
|
Heads up for the rebase: #41030 changes the helpers in |
What
bun test --changed-first[=<ref>]: same git + module-graph detection as--changed, but affected test files are moved to the front of the run instead of the rest being dropped. For PR CI that still wants full coverage but wants fast feedback (especially with--bail) on the files the diff actually touched.bun test --changed-first=main --parallel --bailHow
ChangedFilesFilter.rs: extracted the bundler scan + reverse-import BFS intoscan_affected(), shared by the existingfilter()and the newdetect_affected(), which returns the affected test-file paths as a set so the partition can be applied after--shard/--randomizehave reordered or sliced the list. Git error messages now name the flag the user actually passed.test_command.rs: compute the affected set before--shard, then stable-partition after--shard/--randomizeso shard membership is unchanged and the unaffected tail keeps the order it would otherwise have. Among affected files, slowest-first under--timings, otherwise path order.parallel/runner.rs+Coordinator.rs: new sharedpriority: FileRangeat the head offilesthat every worker drains (pop_front) before starting on its own chunk. Per-worker ranges, work-stealing, and abort accounting cover the remainder unchanged (now offset by the priority count).Arguments.rs/options_types/context.rs: new--changed-first[=<ref>]flag andchanged_firstfield; rejected together with--changed.docs/test/parallel.mdxand an entry in the Large codebases list indocs/test/index.mdx.Tests
test/cli/test/test-changed.test.tsgains abun test --changed-firstdescribe block (6 tests): serial order,--parallel=2 --parallel-delay=1000000dispatch order,--shardmembership unchanged with affected-first-within-shard,--changed-first=<ref>, no-changes runs everything, and--changed+--changed-firstconflict. All 26 tests in the file pass with this change; the 6 new tests fail against the released binary.no test proof · iteration 6 · 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