From 4c6ae09e480aaa57ef0d94dcc797c8800595c8c9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 17 Aug 2026 08:53:05 +0000 Subject: [PATCH] test: add --changed-first to run affected test files first --changed-first[=] 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[=] flag; rejected together with --changed. - docs: subsection in test/parallel.mdx and an entry in the Large codebases list in test/index.mdx. --- docs/test/index.mdx | 4 +- docs/test/parallel.mdx | 11 + src/options_types/context.rs | 5 + src/runtime/cli/Arguments.rs | 12 ++ src/runtime/cli/test/ChangedFilesFilter.rs | 216 ++++++++++++++----- src/runtime/cli/test/parallel/Coordinator.rs | 29 ++- src/runtime/cli/test/parallel/runner.rs | 30 ++- src/runtime/cli/test_command.rs | 77 +++++++ test/cli/test/test-changed.test.ts | 179 +++++++++++++++ 9 files changed, 504 insertions(+), 59 deletions(-) diff --git a/docs/test/index.mdx b/docs/test/index.mdx index e9b67f22611f..f28fb72efe17 100644 --- a/docs/test/index.mdx +++ b/docs/test/index.mdx @@ -423,7 +423,9 @@ jobs: Every shard must read the _same set_ of timings files for the shards to add up to the whole suite. That is why a run reads the previous run's files (restored from the cache), and why it writes its own where sibling shards still in flight won't pick them up (`next/` above). Add `--no-isolate` to the `bun test` line if step 2 applies to you. -**6. Within a file: [`test.concurrent`](#concurrent-test-execution)** for I/O-bound tests that spend their time awaiting. +**6. Fail PRs fast: [`--changed-first`](/test/parallel#running-affected-files-first-with---changed-first).** Same detection as `--changed`, but the affected files run first and the rest still run. Add `--bail` and a PR that breaks what it touched fails without waiting on the rest of the suite. + +**7. Within a file: [`test.concurrent`](#concurrent-test-execution)** for I/O-bound tests that spend their time awaiting. ## Performance diff --git a/docs/test/parallel.mdx b/docs/test/parallel.mdx index ee82ae508ad0..96fb2a2004ab 100644 --- a/docs/test/parallel.mdx +++ b/docs/test/parallel.mdx @@ -159,6 +159,17 @@ The file is plain JSON, slowest first, so it doubles as a "what's slow" report: You can pass `--timings` more than once. Bun reads the files as one table and skips paths that don't exist yet. `--update-timings` writes to the **first** path. Under `--shard` that output contains only the files the shard ran, so the shards' outputs are disjoint. Read together on the next run, they add up to the whole suite with no merge step. [Large codebases](/test/index#large-codebases) on the main page has the full CI workflow. +## Running affected files first with `--changed-first` + +```sh terminal icon="terminal" +bun test --changed-first # uncommitted + untracked vs HEAD +bun test --changed-first=main # vs a branch or commit +``` + +`--changed-first` uses the same git + module-graph detection as `--changed` to find which test files are affected by your edits, but instead of skipping the rest it moves the affected files to the front of the run. On a PR CI job that still wants full coverage, this surfaces failures in what you touched early; combined with `--bail` the run stops on the first such failure instead of working through the rest of the suite. + +Affected files are ordered slowest-first when `--timings` is available, otherwise by path; the remaining files keep the order they would otherwise have. Under `--parallel` the affected files are dispatched from a shared queue first, so every affected file is handed out before any worker starts on its own chunk (an unaffected file may already be running when the last affected one finishes). Under `--shard` each file stays in the shard it would normally belong to and is simply run first within that shard. `--changed` and `--changed-first` cannot be passed together. + ## How it compares 2 000 TypeScript test files × 8 small tests each, all importing a small app built on `zod`, `date-fns` and `lodash`, with a shared setup file (custom matcher + `beforeEach`/`afterEach`) loaded via each runner's preload mechanism — the shape of a large application's unit-test suite ([`bench/test/app`](https://github.com/oven-sh/bun/tree/main/bench/test/app), `bun app/setup.ts 2000 20`). 16-core Apple M4 Max: diff --git a/src/options_types/context.rs b/src/options_types/context.rs index 4190731b8ed8..f22acdd7958e 100644 --- a/src/options_types/context.rs +++ b/src/options_types/context.rs @@ -438,6 +438,10 @@ pub struct TestOptions { /// Otherwise the value is a git ref (commit, branch, tag) to diff /// against. pub changed: Option>, + /// `bun test --changed-first[=]`. Same detection as `changed`, + /// but affected test files are moved to the front of the run instead + /// of the rest being dropped. Mutually exclusive with `changed`. + pub changed_first: Option>, /// `bun test --shard=M/N`. When set, test files are sorted by path /// and only every Nth file (starting from M-1) is run. index is /// 1-based; both are validated at parse time so `1 <= index <= count`. @@ -509,6 +513,7 @@ impl Default for TestOptions { parallel_delay_ms: None, test_worker: false, changed: None, + changed_first: None, shard: None, timings_files: Vec::new(), update_timings: false, diff --git a/src/runtime/cli/Arguments.rs b/src/runtime/cli/Arguments.rs index ab0fc39e9b37..c9f83ded3030 100644 --- a/src/runtime/cli/Arguments.rs +++ b/src/runtime/cli/Arguments.rs @@ -613,6 +613,9 @@ pub(crate) const TEST_ONLY_PARAMS: &[ParamType] = &[ parse_param!( "--changed ? Only run test files affected by changed files according to git. Optionally pass a commit or branch to compare against." ), + parse_param!( + "--changed-first ? Run test files affected by changed files first, then the rest. Same detection as --changed. Optionally pass a commit or branch to compare against." + ), parse_param!( "--isolate Run each test file in a fresh global object. Leaked handles from one file cannot affect another." ), @@ -1902,6 +1905,15 @@ fn parse_test_command_options(args: &clap::Args, ctx: Context<'_>) { if let Some(since) = args.option(b"--changed") { ctx.test_options.changed = Some(since.into()); } + if let Some(since) = args.option(b"--changed-first") { + if ctx.test_options.changed.is_some() { + bun_core::pretty_errorln!( + "error: --changed and --changed-first cannot be used together" + ); + Global::exit(1); + } + ctx.test_options.changed_first = Some(since.into()); + } if let Some(shard) = args.option(b"--shard") { let Some(sep) = strings::index_of_char(shard, b'/') else { bun_core::pretty_errorln!( diff --git a/src/runtime/cli/test/ChangedFilesFilter.rs b/src/runtime/cli/test/ChangedFilesFilter.rs index a2b1be85e98e..eae965bd9079 100644 --- a/src/runtime/cli/test/ChangedFilesFilter.rs +++ b/src/runtime/cli/test/ChangedFilesFilter.rs @@ -43,6 +43,14 @@ use crate::api::bun_process::sync as spawn_sync; // `core::result::Result` is fully qualified throughout this file to avoid the // shadow. +pub(crate) struct AffectedSet { + /// Absolute paths of test entry points whose module graph reaches a + /// changed file. + pub(crate) paths: StringSet, + /// Number of files git reported as changed. + pub(crate) changed_count: usize, +} + pub(crate) struct Result<'a> { /// The filtered list of test files. Slice of the original `test_files` /// allocation, owned by the caller. @@ -81,7 +89,7 @@ pub(crate) fn filter<'a>( let changed_files = if let Some(trigger_set) = consume_watch_trigger() { trigger_set } else { - match get_changed_files(top_level_dir, changed_since) { + match get_changed_files(top_level_dir, changed_since, "--changed") { Ok(set) => set, Err(GitError::GitNotFound) => { Output::err_generic( @@ -118,6 +126,112 @@ pub(crate) fn filter<'a>( }); } + let changed_count = changed_files.count(); + let total = test_files.len(); + let Some((affected, graph_files)) = scan_affected( + ctx, + vm, + test_files, + &changed_files, + "--changed", + "running all tests", + ) else { + return Ok(Result { + test_files, + changed_count, + total_tests: total, + module_graph_files: Vec::new(), + }); + }; + + let mut write: usize = 0; + for i in 0..total { + if affected[i] { + test_files[write] = test_files[i]; + write += 1; + } + } + + Ok(Result { + test_files: &mut test_files[0..write], + changed_count, + total_tests: total, + module_graph_files: graph_files, + }) +} + +/// Same module-graph walk as [`filter`] but returns the set of affected test +/// file paths instead of compacting in place. For `--changed-first`, which +/// needs to look up affectedness by path after `--shard`/`--randomize` have +/// reordered or sliced the test-file list. +pub(crate) fn detect_affected( + ctx: &Command::Context, + vm: &mut VirtualMachine, + test_files: &[Interned], + changed_since: &[u8], +) -> core::result::Result { + let top_level_dir: &[u8] = bun_resolver::fs::FileSystem::get().top_level_dir; + + let changed_files = match get_changed_files(top_level_dir, changed_since, "--changed-first") { + Ok(set) => set, + Err(GitError::GitNotFound) => { + Output::err_generic( + "--changed-first requires git to be installed and in PATH", + (), + ); + Global::exit(1); + } + Err(GitError::GitFailed) => { + Global::exit(1); + } + }; + + let changed_count = changed_files.count(); + if test_files.is_empty() || changed_count == 0 { + return Ok(AffectedSet { + paths: StringSet::new(), + changed_count, + }); + } + + let Some((affected, _)) = scan_affected( + ctx, + vm, + test_files, + &changed_files, + "--changed-first", + "running tests in the usual order", + ) else { + return Ok(AffectedSet { + paths: StringSet::new(), + changed_count, + }); + }; + + let mut paths = StringSet::new(); + for (i, &is) in affected.iter().enumerate() { + if is { + let _ = paths.insert(test_files[i].as_bytes()); + } + } + Ok(AffectedSet { + paths, + changed_count, + }) +} + +/// Run the bundler scan and reverse-import BFS. Returns, per `test_files` +/// index, whether that entry point transitively imports a changed file, plus +/// every on-disk source path encountered (for `--changed --watch` to seed the +/// watcher). `None` on bundler-scan failure (already warned). +fn scan_affected( + ctx: &Command::Context, + vm: &mut VirtualMachine, + test_files: &[Interned], + changed_files: &StringSet, + flag: &str, + on_scan_fail: &str, +) -> Option<(Vec, Vec>)> { // Convert the interned-path list to []const []const u8 for the bundler. let entry_points: Vec<&[u8]> = test_files.iter().map(|p| p.as_bytes()).collect(); @@ -137,8 +251,8 @@ pub(crate) fn filter<'a>( Ok(t) => t, Err(err) => { Output::err_generic( - "Failed to initialize module graph scanner for --changed: {s}", - (err.name(),), + "Failed to initialize module graph scanner for {s}: {s}", + (flag, err.name()), ); Global::exit(1); } @@ -174,19 +288,14 @@ pub(crate) fn filter<'a>( ) { Ok(b) => b, Err(err) => { - // Fall back to running every test rather than aborting the run. bun_core::warn!( - "--changed: failed to build module graph ({}); running all tests", - err.name() + "{}: failed to build module graph ({}); {}", + flag, + err.name(), + on_scan_fail ); Output::flush(); - let total = test_files.len(); - return Ok(Result { - test_files, - changed_count: changed_files.count(), - total_tests: total, - module_graph_files: Vec::new(), - }); + return None; } }; @@ -280,20 +389,14 @@ pub(crate) fn filter<'a>( // A test file is selected if (a) its entry point source index is marked // affected, or (b) the test file itself is in the changed set (covers // test files that failed to enter the graph for any reason). - let mut write: usize = 0; - // reshaped for borrowck — capture len before re-borrowing test_files let total = test_files.len(); - debug_assert_eq!(test_files.len(), slot_to_source.len()); + let mut result: Vec = vec![false; total]; + debug_assert_eq!(total, slot_to_source.len()); for i in 0..total { let tf = test_files[i]; let maybe_source = slot_to_source[i]; - let keep = changed_files.contains(tf.as_bytes()) + result[i] = changed_files.contains(tf.as_bytes()) || maybe_source.is_some_and(|src| affected.is_set(src as usize)); - - if keep { - test_files[write] = tf; - write += 1; - } } // `to_ast()` materializes `Vec` / `Vec` / `Vec` on the @@ -308,12 +411,7 @@ pub(crate) fn filter<'a>( // above — its `Drop` never runs, so `deinit` cannot lead to a double-drop. unsafe { bundle.transpiler.deinit() }; - Ok(Result { - test_files: &mut test_files[0..write], - changed_count: changed_files.count(), - total_tests: total, - module_graph_files: graph_files, - }) + Some((result, graph_files)) } #[cfg(not(windows))] @@ -452,6 +550,7 @@ pub(crate) enum GitError { fn get_changed_files( top_level_dir: &[u8], since: &[u8], + flag: &str, ) -> core::result::Result { let mut which_buf = CorePathBuffer([0u8; bun_core::MAX_PATH_BYTES]); let Some(git_path) = which( @@ -466,22 +565,26 @@ fn get_changed_files( // Find the git repository root so we can make the paths git prints // absolute (git prints paths relative to the repo toplevel with these // commands). - let git_root: Box<[u8]> = - match run_git(git_path, top_level_dir, &[b"rev-parse", b"--show-toplevel"]) { - GitResult::SpawnFailed => return Err(GitError::GitFailed), - GitResult::ExitError { stderr } => { - if !stderr.is_empty() { - Output::err_generic( - "--changed: {s}", - (BStr::new(strings::trim(&stderr, b" \r\n\t")),), - ); - } else { - Output::err_generic("--changed requires running inside a git repository", ()); - } - return Err(GitError::GitFailed); + let git_root: Box<[u8]> = match run_git( + git_path, + top_level_dir, + &[b"rev-parse", b"--show-toplevel"], + flag, + ) { + GitResult::SpawnFailed => return Err(GitError::GitFailed), + GitResult::ExitError { stderr } => { + if !stderr.is_empty() { + Output::err_generic( + "{s}: {s}", + (flag, BStr::new(strings::trim(&stderr, b" \r\n\t"))), + ); + } else { + Output::err_generic("{s} requires running inside a git repository", (flag,)); } - GitResult::Ok { stdout } => Box::<[u8]>::from(strings::trim(&stdout, b" \r\n\t")), - }; + return Err(GitError::GitFailed); + } + GitResult::Ok { stdout } => Box::<[u8]>::from(strings::trim(&stdout, b" \r\n\t")), + }; let mut set = StringSet::new(); @@ -493,11 +596,17 @@ fn get_changed_files( git_path, top_level_dir, &[b"diff", b"--name-only", b"HEAD", b"--"], + flag, ) { GitResult::SpawnFailed => return Err(GitError::GitFailed), GitResult::Ok { stdout } => append_paths(&mut set, &git_root, &stdout), GitResult::ExitError { .. } => { - match run_git(git_path, top_level_dir, &[b"diff", b"--name-only", b"--"]) { + match run_git( + git_path, + top_level_dir, + &[b"diff", b"--name-only", b"--"], + flag, + ) { GitResult::SpawnFailed => return Err(GitError::GitFailed), GitResult::Ok { stdout } => append_paths(&mut set, &git_root, &stdout), GitResult::ExitError { .. } => {} @@ -507,6 +616,7 @@ fn get_changed_files( git_path, top_level_dir, &[b"diff", b"--name-only", b"--cached", b"--"], + flag, ) { GitResult::SpawnFailed => return Err(GitError::GitFailed), GitResult::Ok { stdout } => append_paths(&mut set, &git_root, &stdout), @@ -519,18 +629,19 @@ fn get_changed_files( git_path, top_level_dir, &[b"diff", b"--name-only", since, b"--"], + flag, ) { GitResult::SpawnFailed => return Err(GitError::GitFailed), GitResult::ExitError { stderr } => { if !stderr.is_empty() { Output::err_generic( - "--changed: {s}", - (BStr::new(strings::trim(&stderr, b" \r\n\t")),), + "{s}: {s}", + (flag, BStr::new(strings::trim(&stderr, b" \r\n\t"))), ); } else { Output::err_generic( - "--changed: git diff against {f} failed", - (bun_fmt::quote(since),), + "{s}: git diff against {f} failed", + (flag, bun_fmt::quote(since)), ); } return Err(GitError::GitFailed); @@ -554,6 +665,7 @@ fn get_changed_files( b"--exclude-standard", b"--full-name", ], + flag, ) { GitResult::SpawnFailed => return Err(GitError::GitFailed), GitResult::Ok { stdout } => append_paths(&mut set, &git_root, &stdout), @@ -574,7 +686,7 @@ pub(crate) enum GitResult { }, } -fn run_git(git_path: &[u8], cwd: &[u8], args: &[&[u8]]) -> GitResult { +fn run_git(git_path: &[u8], cwd: &[u8], args: &[&[u8]], flag: &str) -> GitResult { let mut argv: Vec<&[u8]> = Vec::with_capacity(args.len() + 3); argv.push(git_path); // `core.quotePath` (on by default) wraps non-ASCII filenames in quotes @@ -604,7 +716,7 @@ fn run_git(git_path: &[u8], cwd: &[u8], args: &[&[u8]]) -> GitResult { }) { Ok(p) => p, Err(err) => { - Output::err_generic("--changed: failed to spawn git: {s}", (err.name(),)); + Output::err_generic("{s}: failed to spawn git: {s}", (flag, err.name())); return GitResult::SpawnFailed; } }; @@ -612,8 +724,8 @@ fn run_git(git_path: &[u8], cwd: &[u8], args: &[&[u8]]) -> GitResult { match proc { sys::Result::Err(err) => { Output::err_generic( - "--changed: failed to spawn git: {f}", - format_args!("{}", err), + "{s}: failed to spawn git: {f}", + (flag, format_args!("{}", err)), ); GitResult::SpawnFailed } diff --git a/src/runtime/cli/test/parallel/Coordinator.rs b/src/runtime/cli/test/parallel/Coordinator.rs index 0f4acc80348b..d8d4a080ca4e 100644 --- a/src/runtime/cli/test/parallel/Coordinator.rs +++ b/src/runtime/cli/test/parallel/Coordinator.rs @@ -15,6 +15,7 @@ use bun_core::{Global, Output}; use bun_jsc::virtual_machine::VirtualMachine; use bun_ptr::Interned; +use super::file_range::FileRange; use super::frame::{self, Frame}; use super::worker::{Worker, WorkerPipe}; use crate::test_command::CommandLineReporter; @@ -31,6 +32,9 @@ pub struct Coordinator<'a> { pub(crate) event_loop_handle: bun_jsc::EventLoopHandle, pub(crate) reporter: &'a mut CommandLineReporter, pub(crate) files: Vec, + /// `--changed-first`: head of `files` that every worker drains before + /// touching its own `range`. Empty when the flag was not passed. + pub(crate) priority: FileRange, /// `--timings`: recorded cost per `files` index; stealing then goes by remaining time and takes the victim's slowest file. pub(crate) costs: Option>, pub(crate) cwd: &'a [u8], @@ -85,6 +89,9 @@ impl<'a> Coordinator<'a> { } fn has_undispatched_files(&self) -> bool { + if !self.priority.is_empty() { + return true; + } for w in self.workers.iter() { if !w.range.is_empty() { return true; @@ -189,9 +196,13 @@ impl<'a> Coordinator<'a> { running_ms / 1000 ); } - let not_started: u32 = self.workers.iter().map(|w| w.range.len()).sum(); + let not_started: u32 = + self.priority.len() + self.workers.iter().map(|w| w.range.len()).sum::(); if not_started > 0 { bun_core::pretty_errorln!("{} file(s) had not started:", not_started); + for idx in self.priority.lo..self.priority.hi { + bun_core::pretty_errorln!(" {}", bstr::BStr::new(self.rel_path(idx))); + } for w in self.workers.iter() { for idx in w.range.lo..w.range.hi { bun_core::pretty_errorln!(" {}", bstr::BStr::new(self.rel_path(idx))); @@ -287,6 +298,9 @@ impl<'a> Coordinator<'a> { if self.stop_reason.is_some() { return w.shutdown(); } + if let Some(idx) = self.priority.pop_front() { + return w.dispatch(idx, self.files[idx as usize].as_bytes()); + } if let Some(idx) = w.range.pop_front() { return w.dispatch(idx, self.files[idx as usize].as_bytes()); } @@ -769,6 +783,19 @@ impl<'a> Coordinator<'a> { /// Mark every not-yet-dispatched file as failed so `drive()` can exit /// instead of spinning when no live worker remains to make progress. fn abort_queued_files(&mut self, reason: &[u8]) { + while let Some(idx) = self.priority.pop_front() { + bun_core::pretty_error!( + "✗ {} ({})\n", + bstr::BStr::new(bun_paths::resolve_path::relative( + bun_paths::fs::FileSystem::instance().top_level_dir(), + self.files[idx as usize].as_bytes(), + )), + bstr::BStr::new(reason), + ); + self.reporter.summary().fail += 1; + self.reporter.summary().files += 1; + self.files_done += 1; + } // Reachable from reap_worker/abort_on_worker_panic with the // caller's `w: &mut Worker` still live and used afterward; iter_mut() // would create a second `&mut Worker` for `w`'s slot (UB). Iterate via diff --git a/src/runtime/cli/test/parallel/runner.rs b/src/runtime/cli/test/parallel/runner.rs index 7430871524cc..ab773ea8467f 100644 --- a/src/runtime/cli/test/parallel/runner.rs +++ b/src/runtime/cli/test/parallel/runner.rs @@ -34,10 +34,15 @@ const DEFAULT_SCALE_UP_AFTER_MS: i64 = 5; /// Returns true if files were actually run via the worker pool, false if it /// fell back to the sequential path (≤1 effective worker). The caller uses /// this to decide whether to run the serial coverage/JUnit reporters. +/// +/// `priority_count`: the first N entries of `files` are the `--changed-first` +/// priority set and are dispatched from a shared range before any worker +/// starts on its own chunk. The remaining entries are scheduled as before. pub(crate) fn run_as_coordinator( reporter: &mut CommandLineReporter, vm: *mut VirtualMachine, files: &[Interned], + priority_count: u32, ctx: Command::Context, coverage_opts: &mut CodeCoverageOptions, ) -> crate::Result { @@ -90,10 +95,14 @@ pub(crate) fn run_as_coordinator( // Sort lexicographically so adjacent indices share parent directories. // Each worker owns a contiguous chunk; co-located files share imports, so // this keeps each worker's isolation SourceProvider cache hot. --randomize - // explicitly opts out of locality (the caller already shuffled). + // explicitly opts out of locality (the caller already shuffled). The + // [0..priority_count) prefix is the --changed-first set, already ordered. let mut sorted: Vec = files.to_vec(); + let prio = priority_count as usize; + debug_assert!(prio <= sorted.len()); + let rest_n = n - priority_count; if !ctx.test_options.randomize { - index_sort::sort_slice_by(&mut sorted, |a, b| { + index_sort::sort_slice_by(&mut sorted[prio..], |a, b| { bun_core::order(a.as_bytes(), b.as_bytes()) }); } @@ -103,7 +112,14 @@ pub(crate) fn run_as_coordinator( let mut costs: Option> = None; let ranges: Vec = match reporter.timings.as_ref() { Some(t) if !t.is_empty() && !ctx.test_options.randomize => { - let ranges = t.partition(&sorted, k); + let ranges: Vec = t + .partition(&sorted[prio..], k) + .into_iter() + .map(|r| FileRange { + lo: r.lo + priority_count, + hi: r.hi + priority_count, + }) + .collect(); for r in &ranges { t.sort_slowest_first(&mut sorted[r.lo as usize..r.hi as usize]); } @@ -112,8 +128,8 @@ pub(crate) fn run_as_coordinator( } _ => (0..k) .map(|idx| FileRange { - lo: idx * n / k, - hi: (idx + 1) * n / k, + lo: priority_count + idx * rest_n / k, + hi: priority_count + (idx + 1) * rest_n / k, }) .collect(), }; @@ -157,6 +173,10 @@ pub(crate) fn run_as_coordinator( }, reporter, files: sorted, + priority: FileRange { + lo: 0, + hi: priority_count, + }, costs, // SAFETY: FileSystem singleton is initialized before any test runner code runs. cwd: FileSystem::get().top_level_dir, diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index d27f34019e08..2466ee40f653 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -2568,6 +2568,51 @@ impl TestCommand { } else { &mut all_test_files[..] }; + + // --changed-first: compute the affected set now (on the full + // discovered list) so it can be applied after --shard/--randomize + // have reordered or sliced it. + let changed_first_affected: Option = match &ctx + .test_options + .changed_first + { + Some(since) if !test_files.is_empty() => { + let result = match ChangedFilesFilter::detect_affected(&ctx, vm, test_files, since) + { + Ok(r) => r, + Err(err) => { + Output::err( + err, + "--changed-first: unable to determine affected tests", + (), + ); + Global::exit(1); + } + }; + if result.changed_count == 0 { + pretty_error!("--changed-first: no changed files\n"); + } else if result.paths.count() == 0 { + pretty_error!( + "--changed-first: {} changed file{}, no test files affected\n", + result.changed_count, + if result.changed_count == 1 { "" } else { "s" } + ); + } else { + pretty_error!( + "--changed-first: {} changed file{}, {}/{} test file{} affected\n", + result.changed_count, + if result.changed_count == 1 { "" } else { "s" }, + result.paths.count(), + test_files.len(), + if test_files.len() == 1 { "" } else { "s" } + ); + } + Output::flush(); + Some(result) + } + _ => None, + }; + // --shard=M/N: sort the test files for determinism, then keep only // every Nth file starting at M-1. This round-robin distribution // keeps shards roughly balanced regardless of how many files there @@ -2681,11 +2726,43 @@ impl TestCommand { } } + // --changed-first: stable-partition affected files to the front. + // After --shard (membership unchanged) and --randomize (shuffle + // still covers the whole list). + let mut priority_count: u32 = 0; + if let Some(affected) = &changed_first_affected + && affected.paths.count() > 0 + { + let mut front: Vec = Vec::with_capacity(test_files.len()); + let mut back: Vec = Vec::with_capacity(test_files.len()); + for &f in test_files.iter() { + if affected.paths.contains(f.as_bytes()) { + front.push(f); + } else { + back.push(f); + } + } + if !ctx.test_options.randomize { + if let Some(t) = reporter.timings.as_ref().filter(|t| !t.is_empty()) { + t.sort_slowest_first(&mut front); + } else { + index_sort::sort_slice_by(&mut front, |a, b| { + strings::order(a.as_bytes(), b.as_bytes()) + }); + } + } + priority_count = u32::try_from(front.len()).unwrap(); + for (i, f) in front.into_iter().chain(back).enumerate() { + test_files[i] = f; + } + } + if ctx.test_options.parallel > 0 { ran_parallel = ParallelRunner::run_as_coordinator( &mut reporter, vm, test_files, + priority_count, &mut *ctx, &mut coverage_options, )?; diff --git a/test/cli/test/test-changed.test.ts b/test/cli/test/test-changed.test.ts index abea2aadcd25..1641c5bff6f8 100644 --- a/test/cli/test/test-changed.test.ts +++ b/test/cli/test/test-changed.test.ts @@ -68,6 +68,16 @@ function ranFiles(stderr: string, names: string[]): string[] { return names.filter(n => stderr.includes(n + ":")).sort(); } +/** The given test-file basenames in the order their file header first appears + * in bun test's stderr. Names that never appear are omitted. */ +function fileOrder(stderr: string, names: string[]): string[] { + return names + .map(n => [stderr.indexOf(n + ":"), n] as const) + .filter(([i]) => i !== -1) + .sort(([a], [b]) => a - b) + .map(([, n]) => n); +} + // The --watch test at the end is the slow one; everything else is independent // git repos so run them concurrently. describe.concurrent("bun test --changed", () => { @@ -395,6 +405,175 @@ describe.concurrent("bun test --changed", () => { }); }); +describe.concurrent("bun test --changed-first", () => { + // x.test.ts + y.test.ts import shared.ts; a.test.ts and b.test.ts do not. + // Affected files sort after unaffected ones so an ignored flag (system bun) + // does not accidentally produce the expected order. + const fixture = { + "package.json": JSON.stringify({ name: "changed-first", type: "module" }), + "shared.ts": `export const v = 1;\n`, + "a.test.ts": `import { test, expect } from "bun:test";\ntest("a", () => expect(1).toBe(1));\n`, + "b.test.ts": `import { test, expect } from "bun:test";\ntest("b", () => expect(1).toBe(1));\n`, + "x.test.ts": `import { test, expect } from "bun:test";\nimport { v } from "./shared";\ntest("x", () => expect(v).toBe(1));\n`, + "y.test.ts": `import { test, expect } from "bun:test";\nimport { v } from "./shared";\ntest("y", () => expect(v).toBe(1));\n`, + }; + const names = ["a.test.ts", "b.test.ts", "x.test.ts", "y.test.ts"]; + const affected = ["x.test.ts", "y.test.ts"]; + const unaffected = ["a.test.ts", "b.test.ts"]; + + async function run(cwd: string, extra: string[] = []) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "--changed-first", ...extra], + cwd, + env: gitEnv, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + test("serial: affected files run first, nothing is dropped", async () => { + using dir = tempDir("changed-first-serial", fixture); + initRepo(String(dir)); + appendFileSync(join(String(dir), "shared.ts"), "// touched\n"); + + const { stderr, exitCode } = await run(String(dir)); + // All four files ran. + expect(ranFiles(stderr, names)).toEqual(names); + // Affected (x, y) print before unaffected (a, b). + expect(fileOrder(stderr, names)).toEqual([...affected, ...unaffected]); + expect(stderr).toContain("--changed-first:"); + expect(stderr).toContain("2/4 test files affected"); + expect(exitCode).toBe(0); + }); + + test("parallel: affected files dispatched before any worker starts its own chunk", async () => { + using dir = tempDir("changed-first-parallel", fixture); + initRepo(String(dir)); + appendFileSync(join(String(dir), "shared.ts"), "// touched\n"); + + // A huge --parallel-delay keeps the pool at one worker, so file headers + // appear strictly in dispatch order and the priority range is observable. + const { stderr, exitCode } = await run(String(dir), ["--parallel=2", "--parallel-delay=1000000"]); + expect(ranFiles(stderr, names)).toEqual(names); + expect(fileOrder(stderr, names)).toEqual([...affected, ...unaffected]); + expect(exitCode).toBe(0); + }); + + test("no changed files: runs everything in the usual order", async () => { + using dir = tempDir("changed-first-none", fixture); + initRepo(String(dir)); + + const { stderr, exitCode } = await run(String(dir)); + expect(ranFiles(stderr, names)).toEqual(names); + expect(stderr).toContain("--changed-first:"); + expect(stderr).toContain("no changed files"); + expect(exitCode).toBe(0); + }); + + test("--changed-first= compares against a commit", async () => { + using dir = tempDir("changed-first-ref", fixture); + initRepo(String(dir)); + appendFileSync(join(String(dir), "shared.ts"), "// v2\n"); + git(String(dir), "add", "-A"); + git(String(dir), "commit", "-q", "-m", "v2"); + + // Working tree is clean, so bare --changed-first prioritizes nothing. + { + const { stderr, exitCode } = await run(String(dir)); + expect(stderr).toContain("no changed files"); + expect(ranFiles(stderr, names)).toEqual(names); + expect(exitCode).toBe(0); + } + + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "--changed-first=HEAD~1"], + cwd: String(dir), + env: gitEnv, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(fileOrder(stderr, names)).toEqual([...affected, ...unaffected]); + expect(ranFiles(stderr, names)).toEqual(names); + expect(exitCode).toBe(0); + }); + + test("--shard: membership is unchanged; affected files run first within the shard", async () => { + using dir = tempDir("changed-first-shard", fixture); + initRepo(String(dir)); + appendFileSync(join(String(dir), "shared.ts"), "// touched\n"); + + async function shardFiles(extra: string[]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", ...extra], + cwd: String(dir), + env: gitEnv, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + if (exitCode !== 0) expect(stderr).toBe(""); + expect(exitCode).toBe(0); + return { ran: ranFiles(stderr, names), order: fileOrder(stderr, names) }; + } + + for (const shard of ["1/2", "2/2"]) { + const without = await shardFiles([`--shard=${shard}`]); + const withFlag = await shardFiles(["--changed-first", `--shard=${shard}`]); + // Same files either way. + expect(withFlag.ran).toEqual(without.ran); + // Within the shard, affected files come first. + const aff = withFlag.order.filter(n => affected.includes(n)); + const unaff = withFlag.order.filter(n => unaffected.includes(n)); + expect(withFlag.order).toEqual([...aff, ...unaff]); + // Each 1/2 shard of {a, b, x, y} holds one affected and one unaffected + // file, so this reordering is observable. + expect(aff.length).toBeGreaterThan(0); + expect(unaff.length).toBeGreaterThan(0); + } + }); + + test("--timings: affected files are ordered slowest-first", async () => { + using dir = tempDir("changed-first-timings", { + ...fixture, + "timings.json": JSON.stringify({ + version: 1, + files: { "y.test.ts": 100, "x.test.ts": 10, "a.test.ts": 50, "b.test.ts": 50 }, + }), + }); + initRepo(String(dir)); + appendFileSync(join(String(dir), "shared.ts"), "// touched\n"); + + const { stderr, exitCode } = await run(String(dir), ["--timings=timings.json"]); + expect(ranFiles(stderr, names)).toEqual(names); + // y before x (slower), then the unaffected tail. + expect(fileOrder(stderr, names)).toEqual(["y.test.ts", "x.test.ts", ...unaffected]); + expect(exitCode).toBe(0); + }); + + test("--changed and --changed-first together is an error", async () => { + using dir = tempDir("changed-first-conflict", fixture); + initRepo(String(dir)); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "--changed", "--changed-first"], + cwd: String(dir), + env: gitEnv, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toContain("cannot be used together"); + expect(exitCode).not.toBe(0); + }); +}); + // On Windows, `bun test --watch` runs as a parent watcher-manager that // respawns a child process on change (rather than exec()-in-place), which // makes this test's stderr-stream sync points racy there. The 15 cases