From e09b5cf0ec02c8186fb5d37dee4ab3b8217513d8 Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 00:19:13 +0300 Subject: [PATCH 1/9] fix(lang-ts): attribute class-field initializer refs to the class class_declaration nodes had no scope_of entry, so refs inside field initializers (e.g. `x = compute();`) fell through to the enclosing current_def instead of the class def -- module-level code (from_def=0) when the class was top-level. Register the class node itself in scope_of; method bodies still resolve correctly since walk_refs re-checks scope_of at every node and method_definition's own (more specific) entry overrides the class-level one for nodes inside it. --- crates/lang-ts/src/lib.rs | 8 ++++++++ crates/lang-ts/tests/refs.rs | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/crates/lang-ts/src/lib.rs b/crates/lang-ts/src/lib.rs index b67da86..44bc6d9 100644 --- a/crates/lang-ts/src/lib.rs +++ b/crates/lang-ts/src/lib.rs @@ -713,6 +713,14 @@ fn walk_top_level( if let Some(name) = node.child_by_field_name("name") { let class_idx = push_def(node, name, DefKind::Class, src, defs, parent); def_name_ids.insert(name.id()); + // The class's own scope: catches refs in class-body members + // that don't open their own scope (field initializers, e.g. + // `x = compute();`), so they attribute to the class def + // rather than falling through to ``. Method bodies + // still win: `walk_refs` re-resolves `scope_of` at every + // node, so a method_definition's own (more specific) entry, + // inserted below, overrides this one for nodes inside it. + scope_of.insert(node.id(), class_idx); if let Some(body) = node.child_by_field_name("body") { let mut cursor = body.walk(); for member in body.children(&mut cursor) { diff --git a/crates/lang-ts/tests/refs.rs b/crates/lang-ts/tests/refs.rs index 9c921c5..65a0f7f 100644 --- a/crates/lang-ts/tests/refs.rs +++ b/crates/lang-ts/tests/refs.rs @@ -39,6 +39,28 @@ fn test_bodies_attribute_calls_to_testcase() { .any(|c| c.name == "add" && c.from_def == neg)); } +#[test] +fn class_field_initializer_calls_attribute_to_class_not_module() { + let src = r#" +import { compute } from "./util"; +export class Foo { + x = compute(); + bar() { return other(); } +} +"#; + let ex = extract(src); + let foo = ex.defs.iter().position(|d| d.name == "Foo").unwrap(); + let bar = ex.defs.iter().position(|d| d.name == "bar").unwrap(); + assert!(ex + .calls + .iter() + .any(|c| c.name == "compute" && c.from_def == foo)); + assert!(ex + .calls + .iter() + .any(|c| c.name == "other" && c.from_def == bar)); +} + #[test] fn method_calls_carry_qualifier() { let src = r#" From e8f27b998549f85203d4268a6020932401204f6c Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 00:21:13 +0300 Subject: [PATCH 2/9] fix(lang-go): include import aliases in read known-names build_known_names derived a package qualifier only from the raw import path's last segment, so `import foo "example.com/x/bar"` allow-listed "bar" instead of "foo" -- reads via the actual alias qualifier were silently dropped. collect_imports now also collects each import_spec's alias (its `name` field, when it's a genuine package_identifier rather than a blank/dot import) and threads it into build_known_names. ImportRef itself is left untouched since it's a shared cross-language type. --- crates/lang-go/src/lib.rs | 44 ++++++++++++++++++++++++++++-------- crates/lang-go/tests/refs.rs | 17 ++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/crates/lang-go/src/lib.rs b/crates/lang-go/src/lib.rs index 0d29fe1..3359e2f 100644 --- a/crates/lang-go/src/lib.rs +++ b/crates/lang-go/src/lib.rs @@ -100,9 +100,10 @@ impl Language for GoLanguage { } let mut imports = Vec::new(); - collect_imports(root, src_bytes, &mut imports); + let mut import_aliases = Vec::new(); + collect_imports(root, src_bytes, &mut imports, &mut import_aliases); - let known_names = build_known_names(&defs, &imports); + let known_names = build_known_names(&defs, &imports, &import_aliases); let ctx = RefCtx { src: src_bytes, @@ -534,8 +535,21 @@ fn push_def( /// Recursively collect `import_declaration` specs: both the single-spec /// form (`import "fmt"`) and the parenthesized list form (`import (...)`, -/// an `import_spec_list` of `import_spec`). -fn collect_imports(node: Node, src: &[u8], imports: &mut Vec) { +/// an `import_spec_list` of `import_spec`). Also collects each spec's alias +/// (`import foo "example.com/x/bar"`'s `name` field, when it's a genuine +/// package alias rather than `_` (blank import) or `.` (dot import)) into +/// `aliases`, so `build_known_names` can allow-list the alias qualifier +/// actually used at call sites instead of guessing it from the raw path's +/// last segment (which is wrong whenever the import is aliased). +/// `ImportRef` itself stays alias-unaware: it's a shared cross-language type, +/// and every other caller of `raw` needs the resolvable path, not the local +/// binding name. +fn collect_imports( + node: Node, + src: &[u8], + imports: &mut Vec, + aliases: &mut Vec, +) { if node.kind() == "import_spec" { if let Some(path) = node.child_by_field_name("path") { if path.kind() == "interpreted_string_literal" { @@ -546,19 +560,30 @@ fn collect_imports(node: Node, src: &[u8], imports: &mut Vec) { }); } } + if let Some(name) = node.child_by_field_name("name") { + if name.kind() == "package_identifier" { + if let Ok(alias) = name.utf8_text(src) { + aliases.push(alias.to_string()); + } + } + } } let mut cursor = node.walk(); for child in node.children(&mut cursor) { - collect_imports(child, src, imports); + collect_imports(child, src, imports, aliases); } } /// The cheap allow-list used to filter read extraction down to non-local /// noise: same-file top-level function names, plus each import's local -/// package qualifier (the last `/`-separated segment of its raw path; -/// aliases aren't tracked, an acceptable simplification since none of this -/// crate's fixtures use them). -fn build_known_names(defs: &[ExtractedDef], imports: &[ImportRef]) -> HashSet { +/// package qualifier -- the alias, when the import declares one (`import foo +/// "example.com/x/bar"` is referenced at call sites as `foo.Something`, not +/// `bar.Something`), else the last `/`-separated segment of its raw path. +fn build_known_names( + defs: &[ExtractedDef], + imports: &[ImportRef], + import_aliases: &[String], +) -> HashSet { let mut names: HashSet = defs .iter() .filter(|d| d.kind == DefKind::Function) @@ -571,6 +596,7 @@ fn build_known_names(defs: &[ExtractedDef], imports: &[ImportRef]) -> HashSet Date: Sat, 25 Jul 2026 00:25:57 +0300 Subject: [PATCH 3/9] fix(core): seed deleted-file importers instead of run_all (#13) A deleted source file no longer forces RunAll unconditionally. Deleted files (indexed or not) are now routed through the same raw-import stem scan used for unrecognized changed files: any surviving importer whose raw import text still references the deleted path gets its ModuleInit seeded, since it will fail to resolve/compile. A deleted file with no remaining importer contributes zero seeds. --- crates/cli/tests/changes.rs | 65 ++++++++++++++++++++++++++++++++----- crates/core/src/classify.rs | 28 +++++++++++----- 2 files changed, 76 insertions(+), 17 deletions(-) diff --git a/crates/cli/tests/changes.rs b/crates/cli/tests/changes.rs index e7a0e76..6aa5e8b 100644 --- a/crates/cli/tests/changes.rs +++ b/crates/cli/tests/changes.rs @@ -363,10 +363,62 @@ fn added_test_file_seeds_test_cases_and_module_init_as_added() { } } -/// (e) A deleted, previously-indexed source file forces `RunAll`: the -/// documented sound-but-coarse decision for Plan 3. +/// (e) A deleted, previously-indexed source file that still has a live +/// importer (its former importer's raw import text still references it, +/// e.g. `import { helper } from "./removed"`) seeds that importer's +/// `ModuleInit`, not `RunAll`: the importer will fail to resolve/compile +/// against the missing module, so its tests need to run, but nothing +/// unrelated does (issue #13). #[test] -fn deleted_source_file_forces_run_all() { +fn deleted_source_file_with_importer_seeds_importer_module_init() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + // src/removed.ts existed before this change and has since been deleted; + // only its former importer remains on disk for the new-tree index. + write( + root, + "src/consumer.ts", + "import { helper } from \"./removed\";\nexport function useHelper(): unknown { return helper(); }\n", + ); + + let registry = registry(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let consumer_id = testless_core::FileId( + graph + .files + .iter() + .position(|f| f.path.ends_with("consumer.ts")) + .unwrap() as u32, + ); + let consumer_module_init = graph.module_init(consumer_id).expect("module_init present"); + + let changed = vec![ChangedFile { + path: PathBuf::from("src/removed.ts"), + status: FileStatus::Deleted, + }]; + + let mode = classify( + root, + &graph, + ®istry, + &changed, + &extractions, + &no_old_content, + ); + assert_eq!( + mode, + ChangeMode::Selection(vec![Seed { + def: consumer_module_init, + kind: SeedKind::ModuleInit, + }]) + ); +} + +/// (e2) A deleted, previously-indexed source file with no remaining +/// importer contributes zero seeds: its own tests died along with it, and +/// nothing else referenced it (issue #13). +#[test] +fn deleted_source_file_with_no_importers_yields_empty_selection() { let tmp = tempfile::tempdir().unwrap(); let root = tmp.path(); write(root, "src/math.ts", MATH_ADD_ORIGINAL); @@ -387,12 +439,7 @@ fn deleted_source_file_forces_run_all() { &extractions, &no_old_content, ); - match &mode { - ChangeMode::RunAll { reason } => { - assert!(reason.contains("removed.ts"), "reason: {reason}"); - } - other => panic!("expected RunAll, got {other:?}"), - } + assert_eq!(mode, ChangeMode::Selection(vec![])); } /// (f) An unindexed file (no registered `Language`) with no importer diff --git a/crates/core/src/classify.rs b/crates/core/src/classify.rs index fd943cc..ef1b7fc 100644 --- a/crates/core/src/classify.rs +++ b/crates/core/src/classify.rs @@ -7,9 +7,14 @@ //! //! Rule precedence (highest first): //! 1. Any changed path matching [`is_config_file`] -> `RunAll` immediately. -//! 2. Deleted/renamed-away *indexed* source file -> `RunAll { reason: -//! "deleted source file" }`: sound if coarse; a later plan can narrow -//! this to just the file's former importers. +//! 2. Deleted file (indexed or not) -> routed through the same raw-import +//! stem scan as rule 5: any still-present file whose raw import text +//! references the deleted path (by basename or extensionless stem) +//! seeds that importer's `ModuleInit` (`SeedKind::ModuleInit`). A +//! deleted file with no remaining importer contributes zero seeds — +//! its own tests died with it. (A `Renamed` file is handled by rule 4 +//! instead: its old content is diffed directly against its new-path +//! content, so rename semantics are unaffected.) //! 3. Added indexed file -> seed its `TestCase` defs and its `ModuleInit`, //! both `SeedKind::Added` (new exports; nothing referenced them before). //! 4. Modified/Renamed indexed file -> re-parse old vs. new content with @@ -142,8 +147,11 @@ pub fn classify( enum PerFile { Seeds(Vec), - /// `path` isn't recognized by any registered `Language`: batched up for - /// a single pass over every indexed file's raw imports. + /// `path` needs a raw-import stem scan rather than def-level diffing: + /// either it isn't recognized by any registered `Language`, or it was + /// deleted (so there's nothing on disk left to parse, indexed language + /// or not). Batched up for a single pass over every indexed file's raw + /// imports. ScanImporters(PathBuf), } @@ -155,9 +163,10 @@ fn classify_one( old_src_of: &dyn Fn(&Path) -> anyhow::Result>, ) -> Result { if c.status == FileStatus::Deleted { - if registry.for_path(&c.path).is_some() { - return Err(format!("deleted source file: {}", c.path.display())); - } + // Whether or not `path` was itself indexable, it no longer exists + // to parse: fall back to the raw-import stem scan so any surviving + // importer's now-dangling reference still seeds that importer's + // `ModuleInit` (see rule 2's doc comment above). return Ok(PerFile::ScanImporters(c.path.clone())); } @@ -261,6 +270,9 @@ fn seed_added_file(new_graph: &Graph, file_id: FileId) -> Vec { /// its extension stripped, e.g. `config`, so an extensionless import /// specifier like `import cfg from "./config"` still matches a changed /// `config.json`). Each match seeds that importing file's `ModuleInit`. +/// `changed_paths` may be files with no registered `Language` *or* deleted +/// files (indexed or not) — either way there's no def-level diff to run, so +/// this stem scan is the only way to find who's affected. fn scan_importers( new_graph: &Graph, extractions: &[CachedExtraction], From 850b7c6c5fa24f8b2e28d4a8d2e8954b1fc640ce Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 00:40:39 +0300 Subject: [PATCH 4/9] fix(core): scope unknown-call widening to import-reachable files (#20) Unknown-by-short-name widening previously enqueued every Calls{Unknown(n)} caller repo-wide, exploding selection on dense single-crate repos where a common method name matches unrelated defs across the whole codebase. Now an Unknown caller C only widens through to a visited def D when D's file is in C's forward transitive import closure, or they share a file: tier-1 resolution scope for a ref in C is C's file plus its direct imports, so a def outside that reachable set could never have been C's actual (unresolved) callee under our model. Exposed two pre-existing indexer gaps the old repo-wide sweep was papering over: Go same-package cross-file calls (no `import` needed within a package) were never resolved to `Resolved` edges -- fixed by extending a Go file's tier-1 scope to its package siblings. The Rust fixture relying on a bare `crate::`-qualified call with no local `use` was adjusted to use an explicit `use`, an equally idiomatic style, rather than extending Rust's import model further. --- crates/cli/tests/select.rs | 4 +- crates/core/src/indexer.rs | 29 +++++--- crates/core/src/walk.rs | 134 ++++++++++++++++++++++++++++++++++++- 3 files changed, 155 insertions(+), 12 deletions(-) diff --git a/crates/cli/tests/select.rs b/crates/cli/tests/select.rs index 46c8f53..3082389 100644 --- a/crates/cli/tests/select.rs +++ b/crates/cli/tests/select.rs @@ -591,7 +591,9 @@ mod tests { "; const RUST_FMT_RS: &str = "\ -pub fn fmt(a: i64, b: i64) -> String { format!(\"{}\", crate::math::add(a, b)) } +use crate::math::add; + +pub fn fmt(a: i64, b: i64) -> String { format!(\"{}\", add(a, b)) } #[cfg(test)] mod tests { diff --git a/crates/core/src/indexer.rs b/crates/core/src/indexer.rs index 7598bc3..72febd5 100644 --- a/crates/core/src/indexer.rs +++ b/crates/core/src/indexer.rs @@ -185,14 +185,19 @@ pub fn index_repo_incremental( // Pass 3: resolve calls/reads to tier-1 candidates. Scope for a ref in // file F is F itself plus every file F `Imports` (reusing `seen`, which - // pass 2 already built as exactly that from/to set). Defs are indexed - // under their *short* name: a method def like `Calc.push` is indexed - // under `push` too, so a bare-identifier ref matches both plain - // functions and qualified methods whether or not `ref.qualifier` is - // set (the receiver variable rarely equals the type name, so this is a - // deliberate over-approximation). `ModuleInit` defs (name ``) - // are excluded from candidacy since nothing ever references them by - // name. + // pass 2 already built as exactly that from/to set) plus, for Go only, + // every sibling file in F's own package directory (see the `lang.id() + // == "go"` scope extension below): Go's unit of visibility is the + // package, not the file, and a file can never `import` its own + // package, so cross-file same-package calls would otherwise always + // resolve as `Unknown` even though they're entirely unambiguous. Defs + // are indexed under their *short* name: a method def like `Calc.push` + // is indexed under `push` too, so a bare-identifier ref matches both + // plain functions and qualified methods whether or not `ref.qualifier` + // is set (the receiver variable rarely equals the type name, so this + // is a deliberate over-approximation). `ModuleInit` defs (name + // ``) are excluded from candidacy since nothing ever + // references them by name. let mut imports_of: HashMap> = HashMap::new(); for (from, to) in &seen { imports_of.entry(*from).or_default().push(*to); @@ -222,11 +227,19 @@ pub fn index_repo_incremental( for (file_idx, extraction) in extractions.iter().enumerate() { let file_id = FileId(file_idx as u32); let base = file_def_base[file_idx]; + let (rel_path, lang) = &files[file_idx]; let mut scope: Vec = vec![file_id]; if let Some(targets) = imports_of.get(&file_id) { scope.extend(targets.iter().copied()); } + if lang.id() == "go" { + if let Some(dir) = rel_path.parent() { + if let Some(siblings) = dir_to_files.get(dir) { + scope.extend(siblings.iter().copied().filter(|&f| f != file_id)); + } + } + } let candidates_for = |name: &str| -> Vec { scope .iter() diff --git a/crates/core/src/walk.rs b/crates/core/src/walk.rs index abf5ffa..356c4da 100644 --- a/crates/core/src/walk.rs +++ b/crates/core/src/walk.rs @@ -16,7 +16,23 @@ //! file's (and its transitive importers') tests. //! - `Calls{from, to: Unknown(name)}` where `name == short_name(D)` -> //! `from` is impacted (an unresolved call that could dynamically dispatch -//! to D). +//! to D), PROVIDED D's file is reachable from the caller's file through +//! the forward transitive import closure (the caller's file imports, +//! directly or indirectly, D's file), or they're the same file. Why this +//! scoping is sound: tier-1 name resolution for a ref in file C only +//! ever looks at C's own file plus its direct imports; a ref is left +//! `Unknown` either because the callee is external (not in the repo +//! graph at all) or because the name would only resolve through an +//! import chain longer than one hop that tier-1 didn't attempt, or +//! through re-export indirection. In every one of those cases, the only +//! files C could possibly be naming a callee in are C's own file and +//! every file reachable from C by following `Imports` edges forward +//! (transitively) — that's precisely our import-edge model's full +//! picture of "names C could plausibly see". A def living in a file C's +//! import graph can't reach is a def whose name could never have entered +//! C's resolution scope under our model, so excluding it drops only +//! noise (an unrelated same-named def somewhere unconnected in the +//! repo), never a real ambiguous callee. //! - If D is a `ModuleInit`: every file in the transitive importer closure //! of D's file (including D's own file) has its `ModuleInit` enqueued and //! its `TestCase` defs enqueued too (not just collected; a widened test @@ -29,6 +45,7 @@ //! through them: a test helper (itself a `TestCase`) that's called by other //! tests must keep propagating impact to those callers. +use std::cell::RefCell; use std::collections::{HashMap, HashSet, VecDeque}; use crate::classify::Seed; @@ -96,7 +113,10 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { } if let Some(unknown_callers) = index.unknown_by_name.get(short_name(&def.name)) { for &c in unknown_callers { - enqueue(&mut queue, &mut visited, c); + let caller_file = graph.def(c).file; + if index.imports_reach(caller_file, def.file) { + enqueue(&mut queue, &mut visited, c); + } } } @@ -153,6 +173,16 @@ struct ReverseIndex<'g> { /// `Imports{from, to}` -> `importers[to] = [from, ...]` (files that /// import `to`, i.e. `to`'s importers). importers: HashMap>, + /// `Imports{from, to}` -> `imports_fwd[from] = [to, ...]` (files that + /// `from` imports directly). The forward counterpart of `importers`; + /// used to compute the forward transitive import closure for + /// `Unknown`-call widening (see `imports_reach`). + imports_fwd: HashMap>, + /// Lazy, memoized cache of forward transitive import closures, keyed + /// by the file the closure was computed from. Populated on first use + /// by `imports_reach`; `RefCell` because `ReverseIndex` is otherwise + /// borrowed immutably for the whole walk. + forward_closures: RefCell>>, } impl<'g> ReverseIndex<'g> { @@ -162,6 +192,7 @@ impl<'g> ReverseIndex<'g> { let mut containers: HashMap = HashMap::new(); let mut unknown_by_name: HashMap<&str, Vec> = HashMap::new(); let mut importers: HashMap> = HashMap::new(); + let mut imports_fwd: HashMap> = HashMap::new(); for edge in &graph.edges { match edge { @@ -180,7 +211,10 @@ impl<'g> ReverseIndex<'g> { Edge::Contains { parent, child } => { containers.insert(*child, *parent); } - Edge::Imports { from, to } => importers.entry(*to).or_default().push(*from), + Edge::Imports { from, to } => { + importers.entry(*to).or_default().push(*from); + imports_fwd.entry(*from).or_default().push(*to); + } } } @@ -190,7 +224,46 @@ impl<'g> ReverseIndex<'g> { containers, unknown_by_name, importers, + imports_fwd, + forward_closures: RefCell::new(HashMap::new()), + } + } + + /// Whether `to_file` is reachable from `from_file` through the forward + /// transitive import closure of `from_file` (`from_file` imports, + /// directly or indirectly, `to_file`), or they're the same file. Used + /// to scope `Unknown`-call widening to files the caller could + /// plausibly have resolved a name into; see the module doc comment. + /// The closure is computed by BFS over `imports_fwd` on first use per + /// `from_file` and memoized for the rest of the walk. + fn imports_reach(&self, from_file: FileId, to_file: FileId) -> bool { + if from_file == to_file { + return true; + } + if let Some(closure) = self.forward_closures.borrow().get(&from_file) { + return closure.contains(&to_file); } + + let mut visited: HashSet = HashSet::new(); + let mut queue: VecDeque = VecDeque::new(); + visited.insert(from_file); + queue.push_back(from_file); + while let Some(f) = queue.pop_front() { + if let Some(imps) = self.imports_fwd.get(&f) { + for &imp in imps { + if visited.insert(imp) { + queue.push_back(imp); + } + } + } + } + visited.remove(&from_file); + + let reaches = visited.contains(&to_file); + self.forward_closures + .borrow_mut() + .insert(from_file, visited); + reaches } /// The transitive closure of files that (directly or indirectly) import @@ -504,6 +577,61 @@ mod tests { assert_eq!(result, vec![t]); } + #[test] + fn unknown_widening_respects_import_reachability() { + // File A: test T with Calls{Unknown("foo")}. File B: def foo. With + // no import edge from A to B, B is not in A's forward transitive + // import closure and A != B, so T must NOT be selected. Adding an + // Imports{from: A, to: B} edge brings B into A's closure, and T + // must then be selected. + let build = |with_import: bool| { + let mut g = g(); + let fa = file(&mut g, "a.ts"); + let fb = file(&mut g, "b.ts"); + let foo = def(&mut g, "foo", DefKind::Function, fb); + let t = def(&mut g, "t", DefKind::TestCase, fa); + g.add_edge(Edge::Calls { + from: t, + to: CallTarget::Unknown("foo".into()), + }); + if with_import { + g.add_edge(Edge::Imports { from: fa, to: fb }); + } + (g, foo, t) + }; + + let (g_no_import, foo, _t) = build(false); + let result = impacted_tests(&g_no_import, &[seed(foo)]); + assert_eq!(result, Vec::::new()); + + let (g_with_import, foo2, t2) = build(true); + let result = impacted_tests(&g_with_import, &[seed(foo2)]); + assert_eq!(result, vec![t2]); + } + + #[test] + fn unknown_widening_transitive_imports() { + // A imports B imports C. Test T (in A) has Calls{Unknown("bar")}; + // def bar lives in C. A's forward transitive import closure + // reaches C via A -> B -> C, even though A doesn't import C + // directly, so T must be selected. + let mut g = g(); + let fa = file(&mut g, "a.ts"); + let fb = file(&mut g, "b.ts"); + let fc = file(&mut g, "c.ts"); + let bar = def(&mut g, "bar", DefKind::Function, fc); + let t = def(&mut g, "t", DefKind::TestCase, fa); + g.add_edge(Edge::Calls { + from: t, + to: CallTarget::Unknown("bar".into()), + }); + g.add_edge(Edge::Imports { from: fa, to: fb }); + g.add_edge(Edge::Imports { from: fb, to: fc }); + + let result = impacted_tests(&g, &[seed(bar)]); + assert_eq!(result, vec![t]); + } + #[test] fn deterministic_order_and_no_dupes() { // two seeds reaching same test -> once, sorted From 0f3314f8411d230e902729e675bce38a37b331d2 Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 01:03:15 +0300 Subject: [PATCH 5/9] feat(cli): why command explains selection paths (#14) Add `walk::impacted_tests_with_paths`, which records each def's first-discovery predecessor during the impact BFS so the shortest seed -> test hop path can be reconstructed for any selected test. `impacted_tests` becomes a thin wrapper that discards the paths, so all prior selection semantics and tests are unchanged. Wire this up as `testless why `: runs the same analyze+walk pipeline as `select`, forgivingly substring-matches `test_id` against selected tests' ` :: `, and prints the hop path (human text on a terminal, JSON when piped). Zero matches -> not selected (exit 1); multiple matches -> lists candidates (exit 1); run-all classification mirrors select/changes' exit-2 contract. --- crates/cli/src/main.rs | 360 +++++++++++++++++++++++++++++++++++++++- crates/cli/tests/why.rs | 184 ++++++++++++++++++++ crates/core/src/walk.rs | 187 ++++++++++++++++++++- 3 files changed, 720 insertions(+), 11 deletions(-) create mode 100644 crates/cli/tests/why.rs diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index aa6e8f4..c690906 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -9,16 +9,16 @@ mod format; use testless_core::cache::{Cache, CachedExtraction}; use testless_core::classify::{classify, ChangeMode, SeedKind}; use testless_core::gitio; -use testless_core::graph::{CallTarget, DefKind, Edge, Graph}; +use testless_core::graph::{CallTarget, DefId, DefKind, Edge, Graph}; use testless_core::indexer::index_repo_incremental; -use testless_core::walk::impacted_tests; +use testless_core::walk::{impacted_tests, impacted_tests_with_paths, Hop, HopKind}; use testless_core::Registry; #[derive(Parser)] #[command( name = "testless", version, - after_help = "Examples:\n testless index\n testless stats\n testless changes --from origin/main\n testless select --from origin/main\n testless select --from origin/main --format args\n testless completion zsh > _testless" + after_help = "Examples:\n testless index\n testless stats\n testless changes --from origin/main\n testless select --from origin/main\n testless select --from origin/main --format args\n testless why \"formats a sum\"\n testless completion zsh > _testless" )] struct Cli { #[command(subcommand)] @@ -74,6 +74,17 @@ enum Cmd { #[arg(long, value_enum)] format: Option, }, + /// Explain why a test was selected: prints the hop path from a change + /// seed to the matching test. + Why { + /// Which test to explain. Forgiving substring match against + /// ` :: ` (e.g. a bare test name, a file path + /// prefix, or the full `file :: chain` string all work). + test_id: String, + /// Revision to diff from. Compared against the current worktree. + #[arg(long, default_value = "HEAD")] + from: String, + }, /// Generate a shell completion script and print it to stdout. Completion { /// Shell to generate completions for (bash, zsh, fish, elvish, powershell). @@ -513,6 +524,234 @@ fn cmd_select(from: String, to: Option, format: Option) -> Resul Ok(0) } +/// A selected test that matched a `why` query, along with its +/// seed -> test hop path (from `walk::impacted_tests_with_paths`) and the +/// ` :: ` string `test_id` was matched against. +struct WhyCandidate { + file: std::path::PathBuf, + name: Vec, + path: Vec, + match_str: String, +} + +/// Human-facing verb phrase for a hop's edge kind, used as the prefix of +/// every non-final line in `print_why_text` (e.g. `called by fmt (...)`). +fn hop_kind_label(kind: &HopKind) -> String { + match kind { + HopKind::Calls => "called by".to_string(), + HopKind::Reads => "read by".to_string(), + HopKind::Contains => "contained in".to_string(), + HopKind::ImportCloses => "imported by".to_string(), + HopKind::UnknownName(name) => format!("possibly called by (unresolved name \"{name}\")"), + } +} + +/// Machine-readable label for a hop's edge kind, matching the lowercase +/// snake_case convention `seed_kind_label` already established for the +/// wire format. `UnknownName` carries its matched name, so it serializes +/// as a small object instead of a bare string. +fn hop_kind_json(kind: &HopKind) -> serde_json::Value { + match kind { + HopKind::Calls => serde_json::json!("calls"), + HopKind::Reads => serde_json::json!("reads"), + HopKind::Contains => serde_json::json!("contains"), + HopKind::ImportCloses => serde_json::json!("import_closes"), + HopKind::UnknownName(name) => serde_json::json!({ "unknown_name": name }), + } +} + +/// A def's display name and file path, looked up by `DefId`. Shared by both +/// `why` renderers so a hop's endpoints are always rendered identically. +fn def_display(graph: &Graph, id: DefId) -> (String, std::path::PathBuf) { + let def = graph.def(id); + ( + def.name.clone(), + graph.files[def.file.0 as usize].path.clone(), + ) +} + +/// `why`'s human output, one line per hop: the seed (path's first hop's +/// `from`) renders as `changed ()`; every intermediate hop +/// renders as ` ()` (e.g. ` called by fmt (...)`, +/// ` read by ...`); the final hop (arriving at the matched test itself) +/// always renders as ` = test "" ()` regardless of its +/// edge kind, since it's the destination, not another impacted def. A test +/// that's itself a seed (empty path, e.g. a newly `Added` test) has no +/// preceding `changed` line: just the bare `= test ...` line. Returns lines +/// rather than printing directly so it's unit-testable without capturing +/// stdout; `print_why_text` is the printing wrapper callers actually use. +fn why_text_lines(graph: &Graph, candidate: &WhyCandidate) -> Vec { + let test_name = candidate.name.join(" > "); + if candidate.path.is_empty() { + return vec![format!( + "= test \"{test_name}\" ({})", + candidate.file.display() + )]; + } + + let mut lines = Vec::with_capacity(candidate.path.len() + 1); + let (seed_name, seed_file) = def_display(graph, candidate.path[0].from); + lines.push(format!("changed {seed_name} ({})", seed_file.display())); + + let last = candidate.path.len() - 1; + for (i, hop) in candidate.path.iter().enumerate() { + if i == last { + lines.push(format!( + " = test \"{test_name}\" ({})", + candidate.file.display() + )); + } else { + let (name, file) = def_display(graph, hop.to); + lines.push(format!( + " {} {name} ({})", + hop_kind_label(&hop.edge), + file.display() + )); + } + } + lines +} + +fn print_why_text(graph: &Graph, candidate: &WhyCandidate) { + for line in why_text_lines(graph, candidate) { + println!("{line}"); + } +} + +/// `why`'s JSON output for an unambiguous match: `{"version":1,"test": +/// {...},"path":[{"from":{...},"edge":...,"to":{...}}, ...]}`, each hop +/// endpoint rendered as `{"name":..., "file":...}` via `def_display`. +fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { + let path_json: Vec<_> = candidate + .path + .iter() + .map(|hop| { + let (from_name, from_file) = def_display(graph, hop.from); + let (to_name, to_file) = def_display(graph, hop.to); + serde_json::json!({ + "from": { "name": from_name, "file": from_file }, + "edge": hop_kind_json(&hop.edge), + "to": { "name": to_name, "file": to_file }, + }) + }) + .collect(); + + serde_json::json!({ + "version": 1, + "mode": "explained", + "test": { + "file": candidate.file, + "name": candidate.name, + }, + "path": path_json, + }) +} + +/// `testless why `: explain why a test was (or wasn't) selected, +/// by running the same analyze+walk pipeline as `select` and printing the +/// seed -> test hop path for whichever selected test matches `test_id`. +/// +/// Matching is deliberately forgiving: `test_id` is a substring match +/// against ` :: ` (so a bare test name, a file path, or +/// the full string all work). Zero matches means the test wasn't among +/// this change's selected tests (exit 1); more than one lists every +/// matching candidate rather than guessing (exit 1). Exactly one match +/// prints its path (exit 0). A run-all classification short-circuits with +/// the same reason/exit-code (2) contract as `select`/`changes`: there's no +/// specific walk to explain when everything runs. +fn cmd_why(test_id: String, from: String) -> Result { + let (graph, _extractions, mode, _changed_count) = analyze(&from)?; + let is_tty = std::io::stdout().is_terminal(); + + let seeds = match mode { + ChangeMode::Selection(seeds) => seeds, + ChangeMode::RunAll { reason } => { + if is_tty { + println!("run all: {reason}"); + } else { + let out = serde_json::json!({ + "version": 1, + "mode": "run_all", + "reason": reason, + }); + println!("{out}"); + } + return Ok(2); + } + }; + + let with_paths = impacted_tests_with_paths(&graph, &seeds); + let candidates: Vec = with_paths + .into_iter() + .map(|(id, path)| { + let def = graph.def(id); + let file = graph.files[def.file.0 as usize].path.clone(); + let name = def + .test_id + .clone() + .unwrap_or_else(|| vec![def.name.clone()]); + let match_str = format!("{} :: {}", file.display(), name.join(" > ")); + WhyCandidate { + file, + name, + path, + match_str, + } + }) + .collect(); + + let matches: Vec<&WhyCandidate> = candidates + .iter() + .filter(|c| c.match_str.contains(&test_id)) + .collect(); + + match matches.len() { + 0 => { + if is_tty { + println!("not selected: no impacted test matches \"{test_id}\""); + } else { + let out = serde_json::json!({ + "version": 1, + "mode": "not_selected", + "query": test_id, + }); + println!("{out}"); + } + Ok(1) + } + 1 => { + let candidate = matches[0]; + if is_tty { + print_why_text(&graph, candidate); + } else { + println!("{}", why_json(&graph, candidate)); + } + Ok(0) + } + _ => { + if is_tty { + println!("ambiguous match for \"{test_id}\", candidates:"); + for c in &matches { + println!(" {}", c.match_str); + } + } else { + let candidates_json: Vec<_> = matches + .iter() + .map(|c| serde_json::json!({ "file": c.file, "name": c.name })) + .collect(); + let out = serde_json::json!({ + "version": 1, + "mode": "ambiguous", + "query": test_id, + "candidates": candidates_json, + }); + println!("{out}"); + } + Ok(1) + } + } +} + fn cmd_completion(shell: clap_complete::Shell) -> Result<()> { clap_complete::generate( shell, @@ -530,6 +769,7 @@ fn main() { Cmd::Stats => cmd_stats().map(|()| 0), Cmd::Changes { from, to } => cmd_changes(from, to), Cmd::Select { from, to, format } => cmd_select(from, to, format), + Cmd::Why { test_id, from } => cmd_why(test_id, from), Cmd::Completion { shell } => cmd_completion(shell).map(|()| 0), }; @@ -548,3 +788,117 @@ fn main() { } } } + +#[cfg(test)] +mod why_tests { + use super::*; + use std::path::PathBuf; + use testless_core::graph::{Def, DefKind, FileNode}; + + fn file(g: &mut Graph, path: &str) -> testless_core::graph::FileId { + g.add_file(FileNode { + path: PathBuf::from(path), + hash: [0; 32], + lang: "ts".into(), + }) + } + + fn def(g: &mut Graph, name: &str, kind: DefKind, file: testless_core::graph::FileId) -> DefId { + g.add_def(Def { + name: name.into(), + kind, + file, + start_line: 1, + end_line: 2, + test_id: None, + computed_name: false, + }) + } + + #[test] + fn why_text_lines_two_hop_path() { + // changed add (src/math.ts) -> called by fmt (src/format.ts) -> + // = test "formats a sum" (src/format.test.ts) + let mut g = Graph::default(); + let math_file = file(&mut g, "src/math.ts"); + let format_file = file(&mut g, "src/format.ts"); + let test_file = file(&mut g, "src/format.test.ts"); + let add = def(&mut g, "add", DefKind::Function, math_file); + let fmt = def(&mut g, "fmt", DefKind::Function, format_file); + + let candidate = WhyCandidate { + file: PathBuf::from("src/format.test.ts"), + name: vec!["formats a sum".to_string()], + path: vec![ + Hop { + from: add, + edge: HopKind::Calls, + to: fmt, + }, + Hop { + from: fmt, + edge: HopKind::Reads, + to: def(&mut g, "formats a sum", DefKind::TestCase, test_file), + }, + ], + match_str: "src/format.test.ts :: formats a sum".to_string(), + }; + + let lines = why_text_lines(&g, &candidate); + assert_eq!( + lines, + vec![ + "changed add (src/math.ts)".to_string(), + " called by fmt (src/format.ts)".to_string(), + " = test \"formats a sum\" (src/format.test.ts)".to_string(), + ] + ); + } + + #[test] + fn why_text_lines_empty_path_is_bare_test_line() { + let mut g = Graph::default(); + let test_file = file(&mut g, "src/a.test.ts"); + def(&mut g, "t", DefKind::TestCase, test_file); + + let candidate = WhyCandidate { + file: PathBuf::from("src/a.test.ts"), + name: vec!["t".to_string()], + path: vec![], + match_str: "src/a.test.ts :: t".to_string(), + }; + + let lines = why_text_lines(&g, &candidate); + assert_eq!(lines, vec!["= test \"t\" (src/a.test.ts)".to_string()]); + } + + #[test] + fn hop_kind_label_covers_every_variant() { + assert_eq!(hop_kind_label(&HopKind::Calls), "called by"); + assert_eq!(hop_kind_label(&HopKind::Reads), "read by"); + assert_eq!(hop_kind_label(&HopKind::Contains), "contained in"); + assert_eq!(hop_kind_label(&HopKind::ImportCloses), "imported by"); + assert_eq!( + hop_kind_label(&HopKind::UnknownName("add".to_string())), + "possibly called by (unresolved name \"add\")" + ); + } + + #[test] + fn hop_kind_json_labels() { + assert_eq!(hop_kind_json(&HopKind::Calls), serde_json::json!("calls")); + assert_eq!(hop_kind_json(&HopKind::Reads), serde_json::json!("reads")); + assert_eq!( + hop_kind_json(&HopKind::Contains), + serde_json::json!("contains") + ); + assert_eq!( + hop_kind_json(&HopKind::ImportCloses), + serde_json::json!("import_closes") + ); + assert_eq!( + hop_kind_json(&HopKind::UnknownName("add".to_string())), + serde_json::json!({ "unknown_name": "add" }) + ); + } +} diff --git a/crates/cli/tests/why.rs b/crates/cli/tests/why.rs new file mode 100644 index 0000000..e305963 --- /dev/null +++ b/crates/cli/tests/why.rs @@ -0,0 +1,184 @@ +//! `testless why` CLI e2e coverage (#14): mirrors the git/tempdir harness in +//! `select.rs`, but drives the `why` subcommand end to end: index -> diff -> +//! classify -> walk `impacted_tests_with_paths` -> hop-path output. +//! +//! Fixture (TS, one repo, same shape as `select.rs`'s primary scenario): +//! - `src/math.ts`: `add`, called by `format.ts`'s `fmt` and by +//! `math.test.ts`'s `describe("add")` tests. +//! - `src/format.ts`: `fmt`, which calls `add`. +//! - `src/math.test.ts`: `describe("add")` with an `it("handles negatives")`. +//! - `src/format.test.ts`: a single `it("formats a sum")` that calls `fmt`. +//! +//! An `add`-body edit selects both, so `why formats` explains the +//! `add -> fmt -> "formats a sum"` path (exit 0, stdout mentions both +//! `add` and `formats`), while `why nonexistent` matches no selected test +//! (exit 1). + +use assert_cmd::Command; + +fn git(dir: &std::path::Path, args: &[&str]) { + let status = std::process::Command::new("git") + .arg("-C") + .arg(dir) + .args([ + "-c", + "user.email=t@t", + "-c", + "user.name=t", + "-c", + "commit.gpgsign=false", + ]) + .args(args) + .status() + .expect("failed to spawn git"); + assert!(status.success(), "git {args:?} failed in {}", dir.display()); +} + +const MATH_TS: &str = "\ +export function add(a: number, b: number): number { return a + b; } +"; + +const MATH_TS_BODY_EDITED: &str = "\ +export function add(a: number, b: number): number { return a + b + 1; } +"; + +const FORMAT_TS: &str = "\ +import { add } from \"./math\"; +export function fmt(a: number, b: number): string { return `${add(a, b)}`; } +"; + +const MATH_TEST_TS: &str = "\ +import { describe, it, expect } from \"vitest\"; +import { add } from \"./math\"; + +describe(\"add\", () => { + it(\"handles negatives\", () => { expect(add(-1, -2)).toBe(-3); }); +}); +"; + +const FORMAT_TEST_TS: &str = "\ +import { it, expect } from \"vitest\"; +import { fmt } from \"./format\"; +it(\"formats a sum\", () => { expect(fmt(1, 2)).toBe(\"3\"); }); +"; + +fn init_repo() -> tempfile::TempDir { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + std::fs::write(root.join("package.json"), "{ \"name\": \"why-fixture\" }\n").unwrap(); + std::fs::write(root.join("src/math.ts"), MATH_TS).unwrap(); + std::fs::write(root.join("src/format.ts"), FORMAT_TS).unwrap(); + std::fs::write(root.join("src/math.test.ts"), MATH_TEST_TS).unwrap(); + std::fs::write(root.join("src/format.test.ts"), FORMAT_TEST_TS).unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "initial"]); + tmp +} + +/// `why formats` (a substring of `"formats a sum"`) explains the impacted +/// path from `add`'s body edit, through `fmt`, to the matching test. Exit +/// 0; stdout is human text (not piped through anything, so it's the `Text` +/// default) and mentions both the changed def and the destination test. +#[test] +fn why_explains_path_to_selected_test() { + let tmp = init_repo(); + let root = tmp.path(); + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["why", "formats"]) + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + + assert!(out.contains("add"), "expected the changed def in {out:?}"); + assert!( + out.contains("formats a sum"), + "expected the matched test's name in {out:?}" + ); + assert!( + out.contains("src/math.ts"), + "expected the changed def's file in {out:?}" + ); + assert!( + out.contains("src/format.test.ts"), + "expected the matched test's file in {out:?}" + ); +} + +/// A query matching no selected test exits 1 and says so, rather than +/// silently printing nothing or crashing. +#[test] +fn why_nonexistent_test_exits_1() { + let tmp = init_repo(); + let root = tmp.path(); + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + + Command::cargo_bin("testless") + .unwrap() + .args(["why", "nonexistent"]) + .current_dir(root) + .assert() + .code(1); +} + +/// JSON output (piped stdout) for an unambiguous match: `version`, `mode`, +/// a `test` object naming the matched test, and a non-empty `path` whose +/// hops both mention `add` and `fmt` along the way. +#[test] +fn why_json_output_when_piped() { + let tmp = init_repo(); + let root = tmp.path(); + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + + // assert_cmd pipes stdout by default (it's captured, not a TTY), so + // `why`'s TTY-sniffing already selects JSON here, same convention as + // `select`/`changes`. + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["why", "formats"]) + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + + assert_eq!(json["version"], 1); + assert_eq!(json["mode"], "explained"); + assert_eq!(json["test"]["name"][0], "formats a sum"); + assert_eq!(json["test"]["file"], "src/format.test.ts"); + + let path = json["path"].as_array().expect("path array"); + assert!(!path.is_empty(), "expected a non-empty hop path"); + let path_str = format!("{path:?}"); + assert!( + path_str.contains("add"), + "expected the changed def somewhere in the path: {path_str}" + ); + assert!( + path_str.contains("fmt"), + "expected the intermediate caller somewhere in the path: {path_str}" + ); +} + +/// A config-file edit forces `run_all`; `why` mirrors `select`/`changes`'s +/// exit-2 contract rather than pretending to explain a walk that never ran. +#[test] +fn why_config_file_edit_forces_run_all_exit_2() { + let tmp = init_repo(); + let root = tmp.path(); + std::fs::write(root.join("package.json"), "{}\n").unwrap(); + + Command::cargo_bin("testless") + .unwrap() + .args(["why", "formats"]) + .current_dir(root) + .assert() + .code(2); +} diff --git a/crates/core/src/walk.rs b/crates/core/src/walk.rs index 356c4da..31b3986 100644 --- a/crates/core/src/walk.rs +++ b/crates/core/src/walk.rs @@ -48,9 +48,34 @@ use std::cell::RefCell; use std::collections::{HashMap, HashSet, VecDeque}; +use serde::{Deserialize, Serialize}; + use crate::classify::Seed; use crate::graph::{CallTarget, DefId, DefKind, Edge, FileId, Graph}; +/// Which reverse edge a `Hop` crossed; mirrors the module doc comment's +/// edge-by-edge rule list above, plus `ImportCloses` for the module-init +/// importer-closure widening step (crossing from a changed module's init +/// into a file that transitively imports it, or into that file's own +/// tests). +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub enum HopKind { + Calls, + Reads, + Contains, + UnknownName(String), + ImportCloses, +} + +/// One step of a reconstructed seed -> test impact path: `from` (already +/// impacted) reached `to` (newly impacted) via `edge`. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub struct Hop { + pub from: DefId, + pub edge: HopKind, + pub to: DefId, +} + /// All `TestCase` defs impacted by `seeds`, per the spec's reverse- /// reachability rules. Deterministic order (ascending `DefId`). /// @@ -62,14 +87,32 @@ use crate::graph::{CallTarget, DefId, DefKind, Edge, FileId, Graph}; /// eventually skip re-walking callers) and for richer reporting; it's /// reserved, not dead weight. pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { + impacted_tests_with_paths(graph, seeds) + .into_iter() + .map(|(d, _path)| d) + .collect() +} + +/// Same selection semantics as `impacted_tests`, but additionally +/// reconstructs, for each selected test, the shortest seed -> test hop path +/// (the BFS's first-discovery parent pointer chain). Used by `testless why` +/// to explain a selection; `impacted_tests` is a thin wrapper around this +/// that discards the paths. +pub fn impacted_tests_with_paths(graph: &Graph, seeds: &[Seed]) -> Vec<(DefId, Vec)> { let index = ReverseIndex::build(graph); let mut visited: HashSet = HashSet::new(); let mut queue: VecDeque = VecDeque::new(); let mut tests: HashSet = HashSet::new(); + // First-discovery parent pointer per def, recorded the moment a def is + // newly enqueued via some reverse edge (never for seeds, which are the + // roots of the walk's implicit forest). Since `visited` blocks + // rediscovery, the first hop recorded here is always the shortest + // (fewest-hops) path from *some* seed back to that def. + let mut predecessors: HashMap = HashMap::new(); for seed in seeds { - enqueue(&mut queue, &mut visited, seed.def); + enqueue_root(&mut queue, &mut visited, seed.def); } while let Some(d) = queue.pop_front() { @@ -83,12 +126,26 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { if let Some(callers) = index.callers.get(&d) { for &c in callers { - enqueue(&mut queue, &mut visited, c); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::Calls, + c, + ); } } if let Some(readers) = index.readers.get(&d) { for &r in readers { - enqueue(&mut queue, &mut visited, r); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::Reads, + r, + ); } } if let Some(&parent) = index.containers.get(&d) { @@ -108,14 +165,28 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { // closure loop enqueuing `ModuleInit`s directly (never through // this `containers` edge). if graph.def(parent).kind != DefKind::ModuleInit { - enqueue(&mut queue, &mut visited, parent); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::Contains, + parent, + ); } } if let Some(unknown_callers) = index.unknown_by_name.get(short_name(&def.name)) { for &c in unknown_callers { let caller_file = graph.def(c).file; if index.imports_reach(caller_file, def.file) { - enqueue(&mut queue, &mut visited, c); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::UnknownName(short_name(&def.name).to_string()), + c, + ); } } } @@ -123,7 +194,14 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { if def.kind == DefKind::ModuleInit { for file in index.importer_closure(def.file) { if let Some(m) = graph.module_init(file) { - enqueue(&mut queue, &mut visited, m); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::ImportCloses, + m, + ); } for (id, file_def) in graph.defs_in_file(file) { if file_def.kind == DefKind::TestCase { @@ -133,7 +211,14 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { // otherwise a test helper collected here would // silently stop the walk short of any external // caller of that helper. - enqueue(&mut queue, &mut visited, id); + enqueue_hop( + &mut queue, + &mut visited, + &mut predecessors, + d, + HopKind::ImportCloses, + id, + ); } } } @@ -143,14 +228,60 @@ pub fn impacted_tests(graph: &Graph, seeds: &[Seed]) -> Vec { let mut result: Vec = tests.into_iter().collect(); result.sort(); result + .into_iter() + .map(|t| { + let path = reconstruct_path(&predecessors, t); + (t, path) + }) + .collect() } -fn enqueue(queue: &mut VecDeque, visited: &mut HashSet, d: DefId) { +/// Enqueue a seed: the root of a BFS tree, so it never gets a `predecessors` +/// entry (that's how `reconstruct_path` knows to stop climbing). +fn enqueue_root(queue: &mut VecDeque, visited: &mut HashSet, d: DefId) { if visited.insert(d) { queue.push_back(d); } } +/// Enqueue `to`, discovered via a reverse edge of kind `kind` from the +/// already-impacted `from`. Records `to`'s first-discovery parent pointer +/// (skipped if `to` was already visited, e.g. a seed or a def reached +/// earlier via a shorter path). +fn enqueue_hop( + queue: &mut VecDeque, + visited: &mut HashSet, + predecessors: &mut HashMap, + from: DefId, + kind: HopKind, + to: DefId, +) { + if visited.insert(to) { + predecessors.insert( + to, + Hop { + from, + edge: kind, + to, + }, + ); + queue.push_back(to); + } +} + +/// Walks `predecessors` backward from `d` (a selected test) to its seed +/// root, collecting each `Hop` crossed, then reverses the result into +/// seed -> test order. +fn reconstruct_path(predecessors: &HashMap, mut d: DefId) -> Vec { + let mut hops = Vec::new(); + while let Some(hop) = predecessors.get(&d) { + hops.push(hop.clone()); + d = hop.from; + } + hops.reverse(); + hops +} + /// The last `.`-separated segment of a def's name (methods are recorded as /// e.g. `Class.method`; a plain function's short name is its whole name). /// Used to widen the walk through `Calls{to: Unknown(name)}` edges, since an @@ -632,6 +763,46 @@ mod tests { assert_eq!(result, vec![t]); } + /// `impacted_tests_with_paths` records the shortest seed -> test hop + /// chain: `add <- calculate <- t1` (seed `add`) reconstructs as exactly + /// two `Calls` hops, in seed-to-test order. + #[test] + fn path_recording_two_hops_in_order() { + let mut g = g(); + let f = file(&mut g, "a.ts"); + let add = def(&mut g, "add", DefKind::Function, f); + let calculate = def(&mut g, "calculate", DefKind::Function, f); + let t1 = def(&mut g, "t1", DefKind::TestCase, f); + g.add_edge(Edge::Calls { + from: calculate, + to: CallTarget::Resolved(add), + }); + g.add_edge(Edge::Calls { + from: t1, + to: CallTarget::Resolved(calculate), + }); + + let result = impacted_tests_with_paths(&g, &[seed(add)]); + assert_eq!(result.len(), 1); + let (test_id, path) = &result[0]; + assert_eq!(*test_id, t1); + assert_eq!( + path, + &vec![ + Hop { + from: add, + edge: HopKind::Calls, + to: calculate, + }, + Hop { + from: calculate, + edge: HopKind::Calls, + to: t1, + }, + ] + ); + } + #[test] fn deterministic_order_and_no_dupes() { // two seeds reaching same test -> once, sorted From a2bde1f52b38a43cbc45231ed02b7a4dcfec6511 Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 01:16:15 +0300 Subject: [PATCH 6/9] feat: testless.toml with always-run and ignore globs (#15) Adds an optional repo-root testless.toml escape hatch: `ignore` globs (globset) drop matching files at discovery time so they're never indexed; `always-run` globs are unioned into the selection post-walk, so a smoke test always runs even when the walk itself seeds nothing. A malformed testless.toml (bad TOML, invalid glob, wrong value type) is a hard error (exit 1) in every command that loads it, never a silent run_all degrade. `why` explains an always-run-only selection with a dedicated "selected by always-run glob ''" line/field instead of the ordinary hop path, since the walk never reached that test at all. --- Cargo.lock | 56 +++++++++ README.md | 9 ++ crates/cli/src/main.rs | 160 ++++++++++++++++++++++--- crates/cli/tests/changes.rs | 22 ++-- crates/cli/tests/index.rs | 5 +- crates/cli/tests/select.rs | 142 ++++++++++++++++++++++ crates/core/Cargo.toml | 2 + crates/core/src/config.rs | 228 ++++++++++++++++++++++++++++++++++++ crates/core/src/discover.rs | 47 +++++++- crates/core/src/indexer.rs | 14 ++- crates/core/src/lib.rs | 1 + 11 files changed, 649 insertions(+), 37 deletions(-) create mode 100644 crates/core/src/config.rs diff --git a/Cargo.lock b/Cargo.lock index 3a44077..49b4860 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -568,6 +568,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "shlex" version = "2.0.1" @@ -641,9 +650,11 @@ dependencies = [ "anyhow", "bincode", "blake3", + "globset", "ignore", "serde", "tempfile", + "toml", "tree-sitter", "tree-sitter-typescript", ] @@ -675,6 +686,45 @@ dependencies = [ "tree-sitter-typescript", ] +[[package]] +name = "toml" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53c96ecdfa941c8fc4fcaed14f99ada8ebed502eef533015095a07e3301d4c3c" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2abe9b86193656635d2411dc43050282ca48aa31c2451210f4202550afb7526" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tree-sitter" version = "0.26.11" @@ -780,6 +830,12 @@ dependencies = [ "windows-link", ] +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "zmij" version = "1.0.23" diff --git a/README.md b/README.md index e514ebf..5434c0f 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,15 @@ testless select --from origin/main --format args > cmds.txt || RUN_ALL=1 Live recordings of these runs rotate on [the website](https://testless.itaywol.tools). +## Config + +Optional `testless.toml` at the repo root, for the cases static inference can't cover: + +```toml +always-run = ["tests/smoke/**", "**/*.e2e.test.ts"] # always select these tests +ignore = ["**/generated/**", "*.pb.go"] # never index these files +``` + ## Languages TypeScript / JavaScript (vitest, jest `-t` patterns) · Go (`go test -run`) · Rust (`cargo test`) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index c690906..de7181f 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -8,6 +8,7 @@ mod format; use testless_core::cache::{Cache, CachedExtraction}; use testless_core::classify::{classify, ChangeMode, SeedKind}; +use testless_core::config::{self, Config}; use testless_core::gitio; use testless_core::graph::{CallTarget, DefId, DefKind, Edge, Graph}; use testless_core::indexer::index_repo_incremental; @@ -106,6 +107,25 @@ fn cache_for(cwd: &std::path::Path) -> Cache { } } +/// Loads `{cwd}/testless.toml` (see `testless_core::config::Config`) and +/// eagerly validates both glob lists, so a malformed config (bad TOML, or a +/// syntactically invalid glob pattern in either `ignore` or `always-run`) +/// fails loudly right here, in every command that reaches it, rather than +/// only surfacing later when a glob is actually evaluated. This is a hard +/// error (bubbles up through `main`'s `Err` branch, exit 1): a `testless.toml` +/// the user wrote and got wrong deserves a clear failure, not a silent +/// `run_all` degrade. +fn load_config(cwd: &std::path::Path) -> Result { + let config = Config::load(cwd).context("loading testless.toml")?; + config + .ignore_globset() + .context("parsing testless.toml `ignore` globs")?; + config + .always_run_globset() + .context("parsing testless.toml `always-run` globs")?; + Ok(config) +} + fn count_tests(graph: &Graph) -> usize { graph .defs @@ -147,11 +167,15 @@ fn cmd_index(full: bool) -> Result<()> { let cwd = std::env::current_dir().context("getting current directory")?; let cache = cache_for(&cwd); let prev = if full { None } else { cache.load() }; + let config = load_config(&cwd)?; + let ignore = config + .ignore_globset() + .context("parsing testless.toml ignore globs")?; eprintln!("indexing {}...", cwd.display()); let start = Instant::now(); let (graph, extractions, stats) = - index_repo_incremental(&cwd, ®istry(), prev).context("indexing repo")?; + index_repo_incremental(&cwd, ®istry(), Some(&ignore), prev).context("indexing repo")?; let ms = start.elapsed().as_millis() as u64; cache.save(&graph, &extractions).context("saving cache")?; @@ -269,16 +293,23 @@ fn seed_kind_label(kind: SeedKind) -> &'static str { /// still map that to a distinct exit code, not a `main`-reported `Err`. /// /// Returns the graph, its cached per-file extractions, the classification, -/// and the count of files `changed_files` reported (0 on the degrade-to- -/// run-all path); the last is used only for stats reporting by callers. -fn analyze(from: &str) -> Result<(Graph, Vec, ChangeMode, usize)> { +/// the count of files `changed_files` reported (0 on the degrade-to- +/// run-all path, used only for stats reporting by callers), and the loaded +/// `testless.toml` config (empty defaults if none exists), so callers that +/// need `always_run` (namely `select`/`why`) don't have to reload it. +fn analyze(from: &str) -> Result<(Graph, Vec, ChangeMode, usize, Config)> { let cwd = std::env::current_dir().context("getting current directory")?; let reg = registry(); let cache = cache_for(&cwd); let prev = cache.load(); + let config = load_config(&cwd)?; + let ignore = config + .ignore_globset() + .context("parsing testless.toml ignore globs")?; + let (graph, extractions, _stats) = - index_repo_incremental(&cwd, ®, prev).context("indexing repo")?; + index_repo_incremental(&cwd, ®, Some(&ignore), prev).context("indexing repo")?; let (mode, changed_count) = match gitio::changed_files(&cwd, from, None) { Ok(changed) => { @@ -295,7 +326,7 @@ fn analyze(from: &str) -> Result<(Graph, Vec, ChangeMode, usiz cache.save(&graph, &extractions).context("saving cache")?; - Ok((graph, extractions, mode, changed_count)) + Ok((graph, extractions, mode, changed_count, config)) } /// `--from ` diffed against the current worktree, classified into @@ -309,7 +340,7 @@ fn cmd_changes(from: String, to: Option) -> Result { anyhow::bail!("--to is not yet supported (v1 only diffs --from against the worktree)"); } - let (graph, _extractions, mode, changed_count) = analyze(&from)?; + let (graph, _extractions, mode, changed_count, _config) = analyze(&from)?; let exit_code = match &mode { ChangeMode::Selection(_) => 0, @@ -374,6 +405,26 @@ fn cmd_changes(from: String, to: Option) -> Result { Ok(exit_code) } +/// The impact walk's selected `TestCase` defs, unioned with every +/// `TestCase` whose file matches one of `config`'s `always-run` globs (see +/// `testless_core::config::always_run_matches`). This is the escape hatch's +/// selection-level half: a smoke test matching an `always-run` glob is +/// selected even when the walk itself found nothing to seed (e.g. a +/// comment-only edit), because it's never reached via any `Seed` at all. +/// Ascending `DefId` order, matching `impacted_tests`'s own determinism. +fn selected_test_defs( + graph: &Graph, + seeds: &[testless_core::classify::Seed], + config: &Config, +) -> Result> { + let mut selected: std::collections::BTreeSet = + impacted_tests(graph, seeds).into_iter().collect(); + for (id, _glob) in config::always_run_matches(graph, config)? { + selected.insert(id); + } + Ok(selected.into_iter().collect()) +} + /// The test-runner label for a def's file language, per the `select` wire /// contract: `ts` -> `vitest`, `go` -> `gotest`, `rust` -> `cargo`. Any /// other/future registered language degrades to `"unknown"` rather than @@ -413,7 +464,7 @@ fn cmd_select(from: String, to: Option, format: Option) -> Resul anyhow::bail!("--to is not yet supported (v1 only diffs --from against the worktree)"); } - let (graph, _extractions, mode, changed_count) = analyze(&from)?; + let (graph, _extractions, mode, changed_count, config) = analyze(&from)?; // `--format` always wins; omitted, it sniffs the TTY like `changes` // does. `Args` is never the sniffed default; it must be requested. let resolved_format = format.unwrap_or_else(|| { @@ -449,7 +500,7 @@ fn cmd_select(from: String, to: Option, format: Option) -> Resul let total_known = count_tests(&graph); let seed_count = seeds.len(); - let test_defs = impacted_tests(&graph, &seeds); + let test_defs = selected_test_defs(&graph, &seeds, &config)?; let tests: Vec = test_defs .into_iter() .map(|id| { @@ -527,11 +578,18 @@ fn cmd_select(from: String, to: Option, format: Option) -> Resul /// A selected test that matched a `why` query, along with its /// seed -> test hop path (from `walk::impacted_tests_with_paths`) and the /// ` :: ` string `test_id` was matched against. +/// +/// `always_run_glob` is `Some()` exactly when this test was selected +/// *only* via `testless.toml`'s `always-run` list (the impact walk itself +/// never reached it at all, not even as a bare/empty-path seed): in that +/// case `path` is empty and the `always-run` glob is the sole explanation +/// for the selection, rendered distinctly from an ordinary empty-path seed. struct WhyCandidate { file: std::path::PathBuf, name: Vec, path: Vec, match_str: String, + always_run_glob: Option, } /// Human-facing verb phrase for a hop's edge kind, used as the prefix of @@ -577,11 +635,22 @@ fn def_display(graph: &Graph, id: DefId) -> (String, std::path::PathBuf) { /// always renders as ` = test "" ()` regardless of its /// edge kind, since it's the destination, not another impacted def. A test /// that's itself a seed (empty path, e.g. a newly `Added` test) has no -/// preceding `changed` line: just the bare `= test ...` line. Returns lines -/// rather than printing directly so it's unit-testable without capturing -/// stdout; `print_why_text` is the printing wrapper callers actually use. +/// preceding `changed` line: just the bare `= test ...` line. A test +/// selected only via a `testless.toml` `always-run` glob (also an empty +/// path, but distinguished by `always_run_glob` being `Some`) instead gets a +/// `selected by always-run glob ''` line, since "no walk path" means +/// something different there: the walk never reached this test by any means +/// at all. Returns lines rather than printing directly so it's unit-testable +/// without capturing stdout; `print_why_text` is the printing wrapper +/// callers actually use. fn why_text_lines(graph: &Graph, candidate: &WhyCandidate) -> Vec { let test_name = candidate.name.join(" > "); + if let Some(glob) = &candidate.always_run_glob { + return vec![ + format!("selected by always-run glob '{glob}'"), + format!(" = test \"{test_name}\" ({})", candidate.file.display()), + ]; + } if candidate.path.is_empty() { return vec![format!( "= test \"{test_name}\" ({})", @@ -620,7 +689,10 @@ fn print_why_text(graph: &Graph, candidate: &WhyCandidate) { /// `why`'s JSON output for an unambiguous match: `{"version":1,"test": /// {...},"path":[{"from":{...},"edge":...,"to":{...}}, ...]}`, each hop -/// endpoint rendered as `{"name":..., "file":...}` via `def_display`. +/// endpoint rendered as `{"name":..., "file":...}` via `def_display`. A test +/// selected only via an `always-run` glob (see `WhyCandidate::always_run_glob`) +/// carries an empty `path` plus an extra top-level `"always_run"` string +/// field naming the matched glob, instead of the usual hop chain. fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { let path_json: Vec<_> = candidate .path @@ -636,7 +708,7 @@ fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { }) .collect(); - serde_json::json!({ + let mut out = serde_json::json!({ "version": 1, "mode": "explained", "test": { @@ -644,7 +716,11 @@ fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { "name": candidate.name, }, "path": path_json, - }) + }); + if let Some(glob) = &candidate.always_run_glob { + out["always_run"] = serde_json::json!(glob); + } + out } /// `testless why `: explain why a test was (or wasn't) selected, @@ -660,7 +736,7 @@ fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { /// the same reason/exit-code (2) contract as `select`/`changes`: there's no /// specific walk to explain when everything runs. fn cmd_why(test_id: String, from: String) -> Result { - let (graph, _extractions, mode, _changed_count) = analyze(&from)?; + let (graph, _extractions, mode, _changed_count, config) = analyze(&from)?; let is_tty = std::io::stdout().is_terminal(); let seeds = match mode { @@ -680,10 +756,25 @@ fn cmd_why(test_id: String, from: String) -> Result { } }; - let with_paths = impacted_tests_with_paths(&graph, &seeds); - let candidates: Vec = with_paths + // The walk's own selection (possibly with an empty path, for a def + // that's itself a seed) takes precedence over `always-run`: a test is + // only explained via its `always-run` glob when the walk didn't reach + // it by any means at all. + let with_paths: std::collections::HashMap> = + impacted_tests_with_paths(&graph, &seeds) + .into_iter() + .collect(); + let always_run: std::collections::HashMap = + config::always_run_matches(&graph, &config)? + .into_iter() + .collect(); + + let mut all_ids: std::collections::BTreeSet = with_paths.keys().copied().collect(); + all_ids.extend(always_run.keys().copied()); + + let candidates: Vec = all_ids .into_iter() - .map(|(id, path)| { + .map(|id| { let def = graph.def(id); let file = graph.files[def.file.0 as usize].path.clone(); let name = def @@ -691,11 +782,16 @@ fn cmd_why(test_id: String, from: String) -> Result { .clone() .unwrap_or_else(|| vec![def.name.clone()]); let match_str = format!("{} :: {}", file.display(), name.join(" > ")); + let (path, always_run_glob) = match with_paths.get(&id) { + Some(path) => (path.clone(), None), + None => (Vec::new(), always_run.get(&id).cloned()), + }; WhyCandidate { file, name, path, match_str, + always_run_glob, } }) .collect(); @@ -842,6 +938,7 @@ mod why_tests { }, ], match_str: "src/format.test.ts :: formats a sum".to_string(), + always_run_glob: None, }; let lines = why_text_lines(&g, &candidate); @@ -866,12 +963,37 @@ mod why_tests { name: vec!["t".to_string()], path: vec![], match_str: "src/a.test.ts :: t".to_string(), + always_run_glob: None, }; let lines = why_text_lines(&g, &candidate); assert_eq!(lines, vec!["= test \"t\" (src/a.test.ts)".to_string()]); } + #[test] + fn why_text_lines_always_run_glob_overrides_empty_path() { + let mut g = Graph::default(); + let test_file = file(&mut g, "tests/smoke/login.test.ts"); + def(&mut g, "logs in", DefKind::TestCase, test_file); + + let candidate = WhyCandidate { + file: PathBuf::from("tests/smoke/login.test.ts"), + name: vec!["logs in".to_string()], + path: vec![], + match_str: "tests/smoke/login.test.ts :: logs in".to_string(), + always_run_glob: Some("tests/smoke/**".to_string()), + }; + + let lines = why_text_lines(&g, &candidate); + assert_eq!( + lines, + vec![ + "selected by always-run glob 'tests/smoke/**'".to_string(), + " = test \"logs in\" (tests/smoke/login.test.ts)".to_string(), + ] + ); + } + #[test] fn hop_kind_label_covers_every_variant() { assert_eq!(hop_kind_label(&HopKind::Calls), "called by"); diff --git a/crates/cli/tests/changes.rs b/crates/cli/tests/changes.rs index 6aa5e8b..afe1133 100644 --- a/crates/cli/tests/changes.rs +++ b/crates/cli/tests/changes.rs @@ -117,7 +117,7 @@ fn body_edit_seeds_exactly_that_defs_body() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let add = find_def(&graph, "add"); let changed = vec![ChangedFile { @@ -160,7 +160,7 @@ fn exported_arrow_const_body_edit_does_not_seed_module_init() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let mul = find_def(&graph, "mul"); let changed = vec![ChangedFile { @@ -209,7 +209,7 @@ fn export_const_value_edit_seeds_module_init() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let changed = vec![ChangedFile { path: PathBuf::from("src/config.ts"), @@ -253,7 +253,7 @@ fn comment_only_edit_yields_empty_selection() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let changed = vec![ChangedFile { path: PathBuf::from("src/math.ts"), @@ -280,7 +280,7 @@ fn config_file_change_forces_run_all() { write(root, "src/math.ts", MATH_ADD_ORIGINAL); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let changed = vec![ChangedFile { path: PathBuf::from("package.json"), @@ -314,7 +314,7 @@ fn added_test_file_seeds_test_cases_and_module_init_as_added() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let file_id = testless_core::FileId( graph .files @@ -382,7 +382,7 @@ fn deleted_source_file_with_importer_seeds_importer_module_init() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let consumer_id = testless_core::FileId( graph .files @@ -424,7 +424,7 @@ fn deleted_source_file_with_no_importers_yields_empty_selection() { write(root, "src/math.ts", MATH_ADD_ORIGINAL); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let changed = vec![ChangedFile { path: PathBuf::from("src/removed.ts"), @@ -452,7 +452,7 @@ fn unindexed_file_with_no_importers_yields_empty_selection() { write(root, "README.md", "# hello\n"); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let changed = vec![ChangedFile { path: PathBuf::from("README.md"), @@ -486,7 +486,7 @@ fn unresolved_json_import_seeds_importers_module_init() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let consumer_id = testless_core::FileId( graph .files @@ -534,7 +534,7 @@ fn extensionless_import_matches_changed_file_by_stem() { ); let registry = registry(); - let (graph, extractions, _) = index_repo_incremental(root, ®istry, None).unwrap(); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); let consumer_id = testless_core::FileId( graph .files diff --git a/crates/cli/tests/index.rs b/crates/cli/tests/index.rs index 49736d0..502e6e7 100644 --- a/crates/cli/tests/index.rs +++ b/crates/cli/tests/index.rs @@ -132,7 +132,8 @@ fn incremental_reindex_reuses_unchanged_files_and_reparses_only_the_changed_one( let files_count = |g: &testless_core::Graph| g.files.len(); - let (graph, extractions, stats) = index_repo_incremental(root, ®istry(), None).unwrap(); + let (graph, extractions, stats) = + index_repo_incremental(root, ®istry(), None, None).unwrap(); assert_eq!(stats.parsed, files_count(&graph)); assert_eq!(stats.reused, 0); @@ -144,7 +145,7 @@ fn incremental_reindex_reuses_unchanged_files_and_reparses_only_the_changed_one( std::fs::write(&math_path, src).unwrap(); let (graph2, _extractions2, stats2) = - index_repo_incremental(root, ®istry(), Some((graph, extractions))).unwrap(); + index_repo_incremental(root, ®istry(), None, Some((graph, extractions))).unwrap(); // Only math.ts changed, so only it should have been re-parsed. assert_eq!(stats2.parsed, 1); diff --git a/crates/cli/tests/select.rs b/crates/cli/tests/select.rs index 3082389..652466f 100644 --- a/crates/cli/tests/select.rs +++ b/crates/cli/tests/select.rs @@ -219,6 +219,148 @@ fn comment_only_edit_yields_empty_tests() { assert_eq!(json["stats"]["total_known"], 5); } +// --------------------------------------------------------------------- +// `testless.toml` escape hatch (issue #15): `always-run` (selection-level) +// and `ignore` (discovery-level) globs. +// --------------------------------------------------------------------- + +const SMOKE_TEST_TS: &str = "\ +import { it, expect } from \"vitest\"; +it(\"smoke check\", () => { expect(true).toBe(true); }); +"; + +const ALWAYS_RUN_TESTLESS_TOML: &str = "always-run = [\"src/smoke.test.ts\"]\n"; + +/// Same base fixture as `init_repo`, plus an unrelated `src/smoke.test.ts` +/// and a `testless.toml` marking it `always-run`, all committed together +/// (so the config file itself is never part of a later diff). +fn init_repo_with_always_run() -> tempfile::TempDir { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + std::fs::write( + root.join("package.json"), + "{ \"name\": \"select-fixture\" }\n", + ) + .unwrap(); + std::fs::write(root.join("src/math.ts"), MATH_TS).unwrap(); + std::fs::write(root.join("src/format.ts"), FORMAT_TS).unwrap(); + std::fs::write(root.join("src/math.test.ts"), MATH_TEST_TS).unwrap(); + std::fs::write(root.join("src/format.test.ts"), FORMAT_TEST_TS).unwrap(); + std::fs::write(root.join("src/unrelated.test.ts"), UNRELATED_TEST_TS).unwrap(); + std::fs::write(root.join("src/smoke.test.ts"), SMOKE_TEST_TS).unwrap(); + std::fs::write(root.join("testless.toml"), ALWAYS_RUN_TESTLESS_TOML).unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "initial"]); + tmp +} + +/// `always-run` in `testless.toml` selects a matching test even when the +/// walk itself found nothing to seed: a comment-only edit of `add` (zero +/// seeds, see `comment_only_edit_yields_empty_tests`) still selects exactly +/// `src/smoke.test.ts`'s test, because that file's path matches the +/// `always-run` glob regardless of the walk's own result. +#[test] +fn always_run_glob_selects_smoke_test_even_with_zero_seeds() { + let tmp = init_repo_with_always_run(); + let root = tmp.path(); + std::fs::write(root.join("src/math.ts"), MATH_TS_COMMENT_EDITED).unwrap(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .arg("select") + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + + assert_eq!(json["mode"], "selection"); + assert_eq!(json["stats"]["seeds"], 0, "comment-only edit seeds nothing"); + // 5 fixture tests (as in comment_only_edit_yields_empty_tests) plus the + // smoke test itself. + assert_eq!(json["stats"]["total_known"], 6); + assert_eq!(json["stats"]["selected"], 1); + + let tests = json["tests"].as_array().expect("tests array"); + assert_eq!( + tests.len(), + 1, + "expected exactly the smoke test, got {tests:?}" + ); + assert_eq!(tests[0]["file"], "src/smoke.test.ts"); + assert_eq!(tests[0]["name"].as_array().unwrap()[0], "smoke check"); +} + +const GENERATED_TEST_TS: &str = "\ +import { it, expect } from \"vitest\"; +it(\"generated check\", () => { expect(1).toBe(1); }); +"; + +const IGNORE_TESTLESS_TOML: &str = "ignore = [\"src/generated/**\"]\n"; + +/// Same base fixture as `init_repo`, plus a `src/generated/models.test.ts` +/// (a file that, absent any config, would be indexed and add one more +/// known test) and a `testless.toml` marking `src/generated/**` `ignore`d. +fn init_repo_with_ignore() -> tempfile::TempDir { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src/generated")).unwrap(); + std::fs::write( + root.join("package.json"), + "{ \"name\": \"select-fixture\" }\n", + ) + .unwrap(); + std::fs::write(root.join("src/math.ts"), MATH_TS).unwrap(); + std::fs::write(root.join("src/format.ts"), FORMAT_TS).unwrap(); + std::fs::write(root.join("src/math.test.ts"), MATH_TEST_TS).unwrap(); + std::fs::write(root.join("src/format.test.ts"), FORMAT_TEST_TS).unwrap(); + std::fs::write(root.join("src/unrelated.test.ts"), UNRELATED_TEST_TS).unwrap(); + std::fs::write(root.join("src/generated/models.test.ts"), GENERATED_TEST_TS).unwrap(); + std::fs::write(root.join("testless.toml"), IGNORE_TESTLESS_TOML).unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "initial"]); + tmp +} + +/// `ignore` in `testless.toml` drops a matching file at discovery time: its +/// test never becomes part of `total_known`, and it's never selectable, +/// even though it's present on disk and would otherwise be indexed. +#[test] +fn ignore_glob_excludes_generated_file_from_total_known() { + let tmp = init_repo_with_ignore(); + let root = tmp.path(); + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .arg("select") + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + + // Same total as add_body_edit_selects_add_and_formats_tests_excludes_unrelated's + // fixture (5 tests): the generated file's test is excluded entirely, not + // just unselected. + assert_eq!(json["stats"]["total_known"], 5); + + let tests = json["tests"].as_array().expect("tests array"); + assert!( + tests + .iter() + .all(|t| t["file"] != "src/generated/models.test.ts"), + "generated file's test must never be selectable, got {tests:?}" + ); +} + /// (c) A `package.json` edit forces `run_all`, exit 2. #[test] fn config_file_edit_forces_run_all_exit_2() { diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 1256703..0e40488 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -13,8 +13,10 @@ categories.workspace = true anyhow = "1.0.104" bincode = "1" blake3 = "1.8.5" +globset = "0.4.19" ignore = "0.4.31" serde = { version = "1.0.229", features = ["derive"] } +toml = "1.1.3" tree-sitter = "0.26.11" [dev-dependencies] diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs new file mode 100644 index 0000000..016f258 --- /dev/null +++ b/crates/core/src/config.rs @@ -0,0 +1,228 @@ +//! `testless.toml`: an optional, repo-root escape-hatch config for two knobs +//! the spec calls out as needing manual override rather than static +//! inference: +//! +//! ```toml +//! always-run = ["tests/smoke/**", "**/*.e2e.test.ts"] +//! ignore = ["**/generated/**", "*.pb.go"] +//! ``` +//! +//! - `ignore`: discovery-level. Matched (via `globset`) against repo-relative +//! paths; a matching file is dropped before indexing ever sees it, so +//! nothing it defines is a candidate for anything (selection, `why`, +//! `total_known`). +//! - `always-run`: selection-level. After the impact walk, every `TestCase` +//! def whose *file* matches one of these globs is added to the selection +//! regardless of whether the walk reached it; see +//! [`always_run_matches`]. +//! +//! A missing `testless.toml` is not an error: [`Config::load`] returns +//! [`Config::default`] (both lists empty, i.e. a no-op). A `testless.toml` +//! that exists but fails to parse (bad TOML syntax, wrong value types) *is* +//! an error: the file is something the user deliberately wrote, so a broken +//! one deserves a loud failure rather than a silent `run_all` degrade (that +//! degrade is reserved for transient/environmental failures like a missing +//! `git`, not user-authored config). + +use std::path::Path; + +use anyhow::{Context, Result}; +use globset::{Glob, GlobSet, GlobSetBuilder}; +use serde::Deserialize; + +use crate::graph::{DefId, DefKind, Graph}; + +/// Parsed `testless.toml`. Both fields default to empty (a no-op config), +/// so a config with only one of the two keys set is valid. +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct Config { + #[serde(default)] + pub always_run: Vec, + #[serde(default)] + pub ignore: Vec, +} + +impl Config { + /// Loads `{repo}/testless.toml`. A missing file yields + /// `Ok(Config::default())`; any other read failure or a parse error + /// (bad TOML, wrong value shapes) yields `Err` with the file path in + /// context, so callers should treat this as a hard failure, not a + /// `run_all` degrade. + pub fn load(repo: &Path) -> Result { + let path = repo.join("testless.toml"); + let src = match std::fs::read_to_string(&path) { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Config::default()), + Err(e) => return Err(e).with_context(|| format!("reading {}", path.display())), + }; + toml::from_str(&src).with_context(|| format!("parsing {}", path.display())) + } + + /// Compiles `ignore` into a `GlobSet` for discovery-time filtering. + /// `Err` on any invalid glob pattern (also a "malformed config" failure). + pub fn ignore_globset(&self) -> Result { + build_globset(&self.ignore) + } + + /// Compiles `always_run` into a `GlobSet`. Exposed mainly so callers can + /// eagerly validate the config (fail fast on a bad pattern) even before + /// [`always_run_matches`] is needed. + pub fn always_run_globset(&self) -> Result { + build_globset(&self.always_run) + } +} + +fn build_globset(patterns: &[String]) -> Result { + let mut builder = GlobSetBuilder::new(); + for pattern in patterns { + let glob = Glob::new(pattern) + .with_context(|| format!("invalid glob pattern in testless.toml: {pattern:?}"))?; + builder.add(glob); + } + builder + .build() + .context("building globset from testless.toml") +} + +/// Every `TestCase` def in `graph` whose file matches one of `config`'s +/// `always-run` globs, paired with the glob pattern (as written in +/// `testless.toml`) that matched it. Selection-level: called after the +/// impact walk, so its results get unioned into the walk's own selection +/// rather than replacing it. When a file matches more than one pattern, the +/// first matching pattern (in `always_run`'s declared order) is reported. +pub fn always_run_matches(graph: &Graph, config: &Config) -> Result> { + if config.always_run.is_empty() { + return Ok(Vec::new()); + } + let set = config.always_run_globset()?; + + let mut out = Vec::new(); + for (idx, def) in graph.defs.iter().enumerate() { + if def.kind != DefKind::TestCase { + continue; + } + let path = &graph.files[def.file.0 as usize].path; + if let Some(&first) = set.matches(path).first() { + out.push((DefId(idx as u32), config.always_run[first].clone())); + } + } + Ok(out) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::graph::{Def, FileNode}; + + #[test] + fn missing_file_yields_default() { + let tmp = tempfile::tempdir().unwrap(); + let config = Config::load(tmp.path()).unwrap(); + assert_eq!(config, Config::default()); + assert!(config.always_run.is_empty()); + assert!(config.ignore.is_empty()); + } + + #[test] + fn valid_file_parses_both_lists() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("testless.toml"), + "always-run = [\"tests/smoke/**\", \"**/*.e2e.test.ts\"]\n\ + ignore = [\"**/generated/**\", \"*.pb.go\"]\n", + ) + .unwrap(); + + let config = Config::load(tmp.path()).unwrap(); + assert_eq!( + config.always_run, + vec!["tests/smoke/**".to_string(), "**/*.e2e.test.ts".to_string()] + ); + assert_eq!( + config.ignore, + vec!["**/generated/**".to_string(), "*.pb.go".to_string()] + ); + } + + #[test] + fn valid_file_with_only_one_key_defaults_the_other() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("testless.toml"), "ignore = [\"*.pb.go\"]\n").unwrap(); + + let config = Config::load(tmp.path()).unwrap(); + assert!(config.always_run.is_empty()); + assert_eq!(config.ignore, vec!["*.pb.go".to_string()]); + } + + #[test] + fn malformed_toml_is_an_error() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("testless.toml"), + "always-run = [not valid toml\n", + ) + .unwrap(); + + let err = Config::load(tmp.path()).unwrap_err(); + assert!( + format!("{err:#}").contains("testless.toml"), + "error should mention testless.toml: {err:#}" + ); + } + + #[test] + fn wrong_value_type_is_an_error() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("testless.toml"), "always-run = \"nope\"\n").unwrap(); + + assert!(Config::load(tmp.path()).is_err()); + } + + fn file(g: &mut Graph, path: &str) -> crate::graph::FileId { + g.add_file(FileNode { + path: path.into(), + hash: [0; 32], + lang: "ts".into(), + }) + } + + fn def(g: &mut Graph, name: &str, kind: DefKind, file: crate::graph::FileId) -> DefId { + g.add_def(Def { + name: name.into(), + kind, + file, + start_line: 1, + end_line: 2, + test_id: None, + computed_name: false, + }) + } + + #[test] + fn always_run_matches_selects_test_files_by_glob() { + let mut g = Graph::default(); + let smoke_file = file(&mut g, "tests/smoke/login.test.ts"); + let other_file = file(&mut g, "src/math.test.ts"); + let smoke_test = def(&mut g, "logs in", DefKind::TestCase, smoke_file); + let _other_test = def(&mut g, "adds", DefKind::TestCase, other_file); + + let config = Config { + always_run: vec!["tests/smoke/**".to_string()], + ignore: vec![], + }; + + let matches = always_run_matches(&g, &config).unwrap(); + assert_eq!(matches, vec![(smoke_test, "tests/smoke/**".to_string())]); + } + + #[test] + fn empty_always_run_matches_nothing() { + let mut g = Graph::default(); + let f = file(&mut g, "src/math.test.ts"); + def(&mut g, "adds", DefKind::TestCase, f); + + let config = Config::default(); + assert_eq!(always_run_matches(&g, &config).unwrap(), Vec::new()); + } +} diff --git a/crates/core/src/discover.rs b/crates/core/src/discover.rs index f7371dd..ce672f4 100644 --- a/crates/core/src/discover.rs +++ b/crates/core/src/discover.rs @@ -1,12 +1,24 @@ use std::path::{Path, PathBuf}; +use globset::GlobSet; use ignore::WalkBuilder; use crate::language::{Language, Registry}; /// Walk `root`, respecting `.gitignore` and hard-skipping known noise /// directories, returning repo-relative sorted paths matched by `registry`. -pub fn discover<'r>(root: &Path, registry: &'r Registry) -> Vec<(PathBuf, &'r dyn Language)> { +/// +/// `ignore`, when given, is `testless.toml`'s `ignore` glob list (see +/// `crate::config`): any repo-relative path it matches is dropped here, at +/// discovery time, so an ignored file never becomes a `FileNode`/`Def` +/// candidate at all (not indexed, not selectable, not counted in +/// `total_known`). `None` means no `testless.toml`-level filtering (every +/// existing caller that predates the config feature). +pub fn discover<'r>( + root: &Path, + registry: &'r Registry, + ignore: Option<&GlobSet>, +) -> Vec<(PathBuf, &'r dyn Language)> { const SKIP_DIRS: &[&str] = &["node_modules", "vendor", "target", ".testless"]; let mut out = Vec::new(); @@ -37,6 +49,9 @@ pub fn discover<'r>(root: &Path, registry: &'r Registry) -> Vec<(PathBuf, &'r dy .strip_prefix(root) .expect("entry under root") .to_path_buf(); + if ignore.is_some_and(|set| set.is_match(&rel)) { + continue; + } out.push((rel, lang)); } } @@ -66,7 +81,10 @@ mod tests { std::fs::write(root.join(".gitignore"), "ignored.fk\n").unwrap(); let r = Registry::new(vec![Box::new(Fake)]); - let found: Vec<_> = discover(root, &r).into_iter().map(|(p, _)| p).collect(); + let found: Vec<_> = discover(root, &r, None) + .into_iter() + .map(|(p, _)| p) + .collect(); assert_eq!( found, vec![ @@ -76,4 +94,29 @@ mod tests { ] ); } + + /// A `testless.toml`-style `ignore` glob (`**/generated/**`) excludes a + /// matching file at discovery time: it never becomes a candidate, so it + /// doesn't show up in `discover`'s output at all, unlike a + /// `.gitignore`d file (already covered above) which is excluded for a + /// different reason but the same observable effect. + #[test] + fn ignore_globset_excludes_matching_files() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src/generated")).unwrap(); + std::fs::write(root.join("src/a.fk"), "").unwrap(); + std::fs::write(root.join("src/generated/models.fk"), "").unwrap(); + + let r = Registry::new(vec![Box::new(Fake)]); + let mut builder = globset::GlobSetBuilder::new(); + builder.add(globset::Glob::new("**/generated/**").unwrap()); + let ignore = builder.build().unwrap(); + + let found: Vec<_> = discover(root, &r, Some(&ignore)) + .into_iter() + .map(|(p, _)| p) + .collect(); + assert_eq!(found, vec![std::path::PathBuf::from("src/a.fk")]); + } } diff --git a/crates/core/src/indexer.rs b/crates/core/src/indexer.rs index 72febd5..47df7f5 100644 --- a/crates/core/src/indexer.rs +++ b/crates/core/src/indexer.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use anyhow::{anyhow, Context, Result}; +use globset::GlobSet; use tree_sitter::Parser; use crate::cache::CachedExtraction; @@ -19,9 +20,10 @@ pub struct IndexStats { } /// Walk `root`, parse every file `registry` matches, and build the full -/// `Graph` from scratch (no previous run to reuse). +/// `Graph` from scratch (no previous run to reuse, no `testless.toml` +/// `ignore` filtering). pub fn index_repo(root: &Path, registry: &Registry) -> Result { - let (graph, _extractions, _stats) = index_repo_incremental(root, registry, None)?; + let (graph, _extractions, _stats) = index_repo_incremental(root, registry, None, None)?; Ok(graph) } @@ -32,12 +34,18 @@ pub fn index_repo(root: &Path, registry: &Registry) -> Result { /// there's no in-place patching and no risk of dangling ids. Deleted files /// drop out naturally since they're no longer in the discovered set; renames /// are just a delete + an add. +/// +/// `ignore` is `testless.toml`'s compiled `ignore` glob set (see +/// `crate::config::Config::ignore_globset`), forwarded straight to +/// `discover`; `None` skips that filtering entirely (no config, or a caller +/// that predates the config feature). pub fn index_repo_incremental( root: &Path, registry: &Registry, + ignore: Option<&GlobSet>, prev: Option<(Graph, Vec)>, ) -> Result<(Graph, Vec, IndexStats)> { - let files = discover(root, registry); + let files = discover(root, registry, ignore); let mut prev_extractions: HashMap = prev .map(|(_, extractions)| { diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 1dc6cc5..9e3c21f 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,5 +1,6 @@ pub mod cache; pub mod classify; +pub mod config; pub mod diffdef; pub mod discover; pub mod fingerprint; From b3a63b58f9443a851627e5b87f5dedca499e04ce Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 01:29:18 +0300 Subject: [PATCH 7/9] feat: select/changes/why across arbitrary rev ranges via --to (#17) Materializes --to in a temporary git worktree (git worktree add --detach, removed on Drop) and runs the whole analysis pipeline rooted there, so --from/--to can name any two revisions instead of only --from vs. the live worktree. changed_files/show_file still run against the main repo (it has the objects); a bad/unfetched --to rev degrades to the existing run_all/exit-2 fallback, same as a bad --from. --- crates/cli/src/main.rs | 125 +++++++++++++------ crates/cli/tests/changes.rs | 66 ++++++++-- crates/cli/tests/select.rs | 236 ++++++++++++++++++++++++++++++++++-- crates/cli/tests/why.rs | 57 +++++++++ crates/core/src/gitio.rs | 146 ++++++++++++++++++++++ 5 files changed, 576 insertions(+), 54 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index de7181f..8f308f0 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -19,7 +19,7 @@ use testless_core::Registry; #[command( name = "testless", version, - after_help = "Examples:\n testless index\n testless stats\n testless changes --from origin/main\n testless select --from origin/main\n testless select --from origin/main --format args\n testless why \"formats a sum\"\n testless completion zsh > _testless" + after_help = "Examples:\n testless index\n testless stats\n testless changes --from origin/main\n testless select --from origin/main\n testless select --from origin/main --format args\n testless select --from v1.0.0 --to v1.1.0\n testless why \"formats a sum\"\n testless completion zsh > _testless" )] struct Cli { #[command(subcommand)] @@ -52,22 +52,30 @@ enum Cmd { /// Classify what changed since `--from` into impacted-def seeds (or a /// run-all fallback) and print them. Changes { - /// Revision to diff from. Compared against the current worktree. + /// Revision to diff from. Compared against `--to` (or, when `--to` + /// is omitted, the current worktree). #[arg(long, default_value = "HEAD")] from: String, - /// Revision to diff to. Not yet supported: v1 always compares - /// `--from` against the worktree. + /// Revision to diff to. When omitted, `--from` is compared against + /// the current worktree. When given, `--from`..`--to` is analyzed + /// as a snapshot: `--to` is checked out into a temporary git + /// worktree (removed again once the command finishes), so it must + /// already exist in the local object store (fetched, if remote). #[arg(long)] to: Option, }, /// Select the tests impacted by what changed since `--from` (or a /// run-all fallback) and print them. Select { - /// Revision to diff from. Compared against the current worktree. + /// Revision to diff from. Compared against `--to` (or, when `--to` + /// is omitted, the current worktree). #[arg(long, default_value = "HEAD")] from: String, - /// Revision to diff to. Not yet supported: v1 only diffs - /// `--from` against the worktree. + /// Revision to diff to. When omitted, `--from` is compared against + /// the current worktree. When given, `--from`..`--to` is analyzed + /// as a snapshot: `--to` is checked out into a temporary git + /// worktree (removed again once the command finishes), so it must + /// already exist in the local object store (fetched, if remote). #[arg(long)] to: Option, /// Output format. Defaults to `json` when stdout is piped, `text` @@ -82,9 +90,13 @@ enum Cmd { /// ` :: ` (e.g. a bare test name, a file path /// prefix, or the full `file :: chain` string all work). test_id: String, - /// Revision to diff from. Compared against the current worktree. + /// Revision to diff from. Compared against `--to` (or, when `--to` + /// is omitted, the current worktree). #[arg(long, default_value = "HEAD")] from: String, + /// Revision to diff to (see `select --to`/`changes --to`). + #[arg(long)] + to: Option, }, /// Generate a shell completion script and print it to stdout. Completion { @@ -275,45 +287,86 @@ fn seed_kind_label(kind: SeedKind) -> &'static str { } } -/// Shared `--from ` pipeline for `changes` and `select`: incrementally -/// (re)indexes the repo, diffs the worktree against `from`, classifies the -/// change into a `ChangeMode`, and saves the (possibly freshly-parsed) -/// cache, deliberately in that order. +/// Shared `--from `[`--to `] pipeline for `changes`, `select`, and +/// `why`: incrementally (re)indexes the repo, diffs against `from`, +/// classifies the change into a `ChangeMode`, and saves the (possibly +/// freshly-parsed) cache, deliberately in that order. +/// +/// When `to` is `None` (the default), this is exactly the original +/// `--from`-vs-worktree pipeline: `root` is the current directory, and +/// `changed_files`/`classify` run against its on-disk (possibly dirty) +/// state. +/// +/// When `to` is `Some(rev)`, that revision is materialized into a temporary +/// git worktree (`gitio::TempWorktree`, `git worktree add --detach`) and the +/// *entire* pipeline — indexing, `testless.toml` loading, the cache — runs +/// with that worktree's checkout as `root` instead: an ephemeral analysis of +/// the `--to` snapshot that never touches the caller's actual worktree or +/// its `.testless` cache. `changed_files`/`show_file`, which need repo +/// history rather than a checkout, still run against the current directory +/// (it has the same object store as the worktree, being a worktree of it). +/// The temporary worktree is removed (via `TempWorktree`'s `Drop`) when this +/// function returns, by any path, including an early `?`-propagated error. /// -/// `changed_files`/`classify` must run against the on-disk worktree before -/// the cache is (re)written: saving first would leave a freshly-created (or -/// freshly-modified) `.testless/graph.bin` sitting in the worktree, which -/// `git ls-files --others` would then report as an untracked "changed" file -/// in any repo that hasn't gitignored `.testless/` yet, polluting both the +/// `changed_files`/`classify` must run against `root`'s on-disk state before +/// its cache is (re)written: saving first would leave a freshly-created (or +/// freshly-modified) `.testless/graph.bin` sitting there, which `git +/// ls-files --others` would then report as an untracked "changed" file in +/// any repo that hasn't gitignored `.testless/` yet, polluting both the /// `changed_files` stat and (harmlessly, but wastefully) the importer scan. /// /// Failure to list changed files (bad rev, `git` missing, an unrecognized /// git status token) degrades to a run-all fallback rather than a hard -/// error. See Item 2 on `cmd_changes`'s original doc comment; callers -/// still map that to a distinct exit code, not a `main`-reported `Err`. +/// error, as does a failure to materialize `--to` itself (bad/unfetched +/// rev): both are transient/environmental, same posture as a bad `--from`. +/// See Item 2 on `cmd_changes`'s original doc comment; callers still map +/// that to a distinct exit code, not a `main`-reported `Err`. /// /// Returns the graph, its cached per-file extractions, the classification, /// the count of files `changed_files` reported (0 on the degrade-to- /// run-all path, used only for stats reporting by callers), and the loaded -/// `testless.toml` config (empty defaults if none exists), so callers that -/// need `always_run` (namely `select`/`why`) don't have to reload it. -fn analyze(from: &str) -> Result<(Graph, Vec, ChangeMode, usize, Config)> { +/// `testless.toml` config (empty defaults if none exists, or if `--to` +/// failed to materialize), so callers that need `always_run` (namely +/// `select`/`why`) don't have to reload it. +fn analyze( + from: &str, + to: Option<&str>, +) -> Result<(Graph, Vec, ChangeMode, usize, Config)> { let cwd = std::env::current_dir().context("getting current directory")?; let reg = registry(); - let cache = cache_for(&cwd); + + let worktree = match to { + None => None, + Some(to_rev) => match gitio::TempWorktree::create(&cwd, to_rev) { + Ok(wt) => Some(wt), + Err(err) => { + let reason = format!("materializing --to rev: {err:#}"); + return Ok(( + Graph::default(), + Vec::new(), + ChangeMode::RunAll { reason }, + 0, + Config::default(), + )); + } + }, + }; + let root: &std::path::Path = worktree.as_ref().map_or(&cwd, |wt| wt.path()); + + let cache = cache_for(root); let prev = cache.load(); - let config = load_config(&cwd)?; + let config = load_config(root)?; let ignore = config .ignore_globset() .context("parsing testless.toml ignore globs")?; let (graph, extractions, _stats) = - index_repo_incremental(&cwd, ®, Some(&ignore), prev).context("indexing repo")?; + index_repo_incremental(root, ®, Some(&ignore), prev).context("indexing repo")?; - let (mode, changed_count) = match gitio::changed_files(&cwd, from, None) { + let (mode, changed_count) = match gitio::changed_files(&cwd, from, to) { Ok(changed) => { - let mode = classify(&cwd, &graph, ®, &changed, &extractions, &|p| { + let mode = classify(root, &graph, ®, &changed, &extractions, &|p| { gitio::show_file(&cwd, from, p) }); (mode, changed.len()) @@ -336,11 +389,7 @@ fn analyze(from: &str) -> Result<(Graph, Vec, ChangeMode, usiz /// saving the cache), which are still surfaced as `Err` so `main` reports /// them. fn cmd_changes(from: String, to: Option) -> Result { - if to.is_some() { - anyhow::bail!("--to is not yet supported (v1 only diffs --from against the worktree)"); - } - - let (graph, _extractions, mode, changed_count, _config) = analyze(&from)?; + let (graph, _extractions, mode, changed_count, _config) = analyze(&from, to.as_deref())?; let exit_code = match &mode { ChangeMode::Selection(_) => 0, @@ -460,11 +509,7 @@ struct SelectedTest { /// selection (including an empty one), 2 for run-all, mirroring /// `cmd_changes`'s exit-code contract exactly. fn cmd_select(from: String, to: Option, format: Option) -> Result { - if to.is_some() { - anyhow::bail!("--to is not yet supported (v1 only diffs --from against the worktree)"); - } - - let (graph, _extractions, mode, changed_count, config) = analyze(&from)?; + let (graph, _extractions, mode, changed_count, config) = analyze(&from, to.as_deref())?; // `--format` always wins; omitted, it sniffs the TTY like `changes` // does. `Args` is never the sniffed default; it must be requested. let resolved_format = format.unwrap_or_else(|| { @@ -735,8 +780,8 @@ fn why_json(graph: &Graph, candidate: &WhyCandidate) -> serde_json::Value { /// prints its path (exit 0). A run-all classification short-circuits with /// the same reason/exit-code (2) contract as `select`/`changes`: there's no /// specific walk to explain when everything runs. -fn cmd_why(test_id: String, from: String) -> Result { - let (graph, _extractions, mode, _changed_count, config) = analyze(&from)?; +fn cmd_why(test_id: String, from: String, to: Option) -> Result { + let (graph, _extractions, mode, _changed_count, config) = analyze(&from, to.as_deref())?; let is_tty = std::io::stdout().is_terminal(); let seeds = match mode { @@ -865,7 +910,7 @@ fn main() { Cmd::Stats => cmd_stats().map(|()| 0), Cmd::Changes { from, to } => cmd_changes(from, to), Cmd::Select { from, to, format } => cmd_select(from, to, format), - Cmd::Why { test_id, from } => cmd_why(test_id, from), + Cmd::Why { test_id, from, to } => cmd_why(test_id, from, to), Cmd::Completion { shell } => cmd_completion(shell).map(|()| 0), }; diff --git a/crates/cli/tests/changes.rs b/crates/cli/tests/changes.rs index afe1133..bdea364 100644 --- a/crates/cli/tests/changes.rs +++ b/crates/cli/tests/changes.rs @@ -570,7 +570,6 @@ fn extensionless_import_matches_changed_file_by_stem() { mod cli_changes { use assert_cmd::Command; - use predicates::prelude::*; fn git(dir: &std::path::Path, args: &[&str]) { let status = std::process::Command::new("git") @@ -700,17 +699,70 @@ mod cli_changes { assert!(json["reason"].as_str().unwrap().contains("--from")); } - /// `--to` isn't supported yet in v1: documented punt, hard error exit 1. + fn rev_parse(dir: &std::path::Path, rev: &str) -> String { + let output = std::process::Command::new("git") + .arg("-C") + .arg(dir) + .args(["rev-parse", rev]) + .output() + .expect("failed to spawn git rev-parse"); + assert!(output.status.success(), "git rev-parse {rev} failed"); + String::from_utf8(output.stdout).unwrap().trim().to_string() + } + + /// `--to ` (issue #17): an explicit `--from`..`--to` rev range + /// (rather than `--from` vs. the worktree) reports exactly the seed for + /// a body edit committed between the two revisions. + #[test] + fn to_flag_reports_seed_for_rev_range() { + let tmp = init_repo(); + let root = tmp.path(); + let c1 = rev_parse(root, "HEAD"); + + std::fs::write( + root.join("src/math.ts"), + "export function add(a: number, b: number): number { return a + b + 1; }\n", + ) + .unwrap(); + git(root, &["commit", "-am", "edit add's body"]); + let c2 = rev_parse(root, "HEAD"); + + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["changes", "--from", &c1, "--to", &c2]) + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + assert_eq!(json["mode"], "selection"); + let seeds = json["seeds"].as_array().expect("seeds array"); + assert!( + seeds + .iter() + .any(|s| s["def"] == "add" && s["kind"] == "body"), + "expected an add/body seed, got {seeds:?}" + ); + } + + /// A `--to` rev that doesn't resolve locally degrades to the same + /// `run_all`/exit-2 fallback as a bad `--from` rev, rather than a hard + /// error. #[test] - fn to_flag_is_rejected() { + fn bad_to_rev_degrades_to_run_all_exit_2() { let tmp = init_repo(); - Command::cargo_bin("testless") + let assert = Command::cargo_bin("testless") .unwrap() - .args(["changes", "--to", "HEAD"]) + .args(["changes", "--to", "not-a-real-rev"]) .current_dir(tmp.path()) .assert() - .code(1) - .stderr(predicate::str::contains("not yet supported")); + .code(2); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap(); + assert_eq!(json["mode"], "run_all"); + assert!(json["reason"].as_str().unwrap().contains("--to")); } // --- multi-language e2e coverage (Plan 3, Task 6) ------------------ diff --git a/crates/cli/tests/select.rs b/crates/cli/tests/select.rs index 652466f..3dff2ac 100644 --- a/crates/cli/tests/select.rs +++ b/crates/cli/tests/select.rs @@ -380,18 +380,240 @@ fn config_file_edit_forces_run_all_exit_2() { assert!(json["reason"].as_str().unwrap().contains("package.json")); } -/// `--to` isn't supported yet in v1: documented punt, hard error exit 1; -/// mirrors `changes`'s identical rejection. +// --------------------------------------------------------------------- +// `--to ` (issue #17): analyze an arbitrary `--from`..`--to` rev +// range instead of `--from` vs. the live worktree, by materializing `--to` +// in a temporary git worktree and running the whole pipeline rooted there. +// +// Fixture: three commits. +// - C1: baseline, `src/math.ts` (`add` + `unrelatedHelper`) and +// `src/greet.ts` (`greet`), each with its own test file. +// - C2: edits `add`'s body only. +// - C3: (on top of C2) edits `greet`'s body only. +// +// `--from C1 --to C2` must select only `add`'s two tests (the range +// contains no `greet` change at all, even though `greet.ts` in the C2 +// worktree is untouched relative to C1); `--from C1 --to C3` must select +// both `add`'s and `greet`'s tests, since both changes fall in that range. +// `unrelatedHelper`'s test must never be selected in either case. +// --------------------------------------------------------------------- + +const GREET_TS: &str = "\ +export function greet(name: string): string { return `Hello, ${name}!`; } +"; + +const GREET_TS_BODY_EDITED: &str = "\ +export function greet(name: string): string { return `Hi, ${name}!`; } +"; + +const GREET_TEST_TS: &str = "\ +import { it, expect } from \"vitest\"; +import { greet } from \"./greet\"; +it(\"greets by name\", () => { expect(greet(\"Ada\")).toBe(\"Hello, Ada!\"); }); +"; + +fn rev_parse(dir: &std::path::Path, rev: &str) -> String { + let output = std::process::Command::new("git") + .arg("-C") + .arg(dir) + .args(["rev-parse", rev]) + .output() + .expect("failed to spawn git rev-parse"); + assert!(output.status.success(), "git rev-parse {rev} failed"); + String::from_utf8(output.stdout).unwrap().trim().to_string() +} + +/// Builds the three-commit `--to` fixture, returning the tempdir alongside +/// each commit's full sha (C1, C2, C3, in that order). +fn init_to_range_repo() -> (tempfile::TempDir, String, String, String) { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + std::fs::write(root.join("package.json"), "{ \"name\": \"to-fixture\" }\n").unwrap(); + // Distinct tests calling this helper (potentially concurrently, since + // `cargo test` runs tests in parallel by default) must not produce + // byte-identical commits: an otherwise-identical tree + the same-second + // author/committer timestamp would hash to the *same* commit sha, and + // since the `--to` temp worktree's path is derived solely from that sha + // (by design: collision-safe across runs against the same rev), two + // unrelated repos racing to the same sha would collide on the same + // filesystem path. This nonce (the tempdir's own, guaranteed-unique + // path) makes every call's tree distinct, hence every commit's sha + // distinct, regardless of timing. + std::fs::write(root.join(".nonce"), root.display().to_string()).unwrap(); + std::fs::write(root.join("src/math.ts"), MATH_TS).unwrap(); + std::fs::write(root.join("src/greet.ts"), GREET_TS).unwrap(); + std::fs::write(root.join("src/math.test.ts"), MATH_TEST_TS).unwrap(); + std::fs::write(root.join("src/greet.test.ts"), GREET_TEST_TS).unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "C1: baseline"]); + let c1 = rev_parse(root, "HEAD"); + + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + git(root, &["commit", "-am", "C2: edit add's body"]); + let c2 = rev_parse(root, "HEAD"); + + std::fs::write(root.join("src/greet.ts"), GREET_TS_BODY_EDITED).unwrap(); + git(root, &["commit", "-am", "C3: edit greet's body"]); + let c3 = rev_parse(root, "HEAD"); + + (tmp, c1, c2, c3) +} + +/// `--from C1 --to C2`: only `add`'s two tests are selected. `greet`'s test +/// (unchanged in this range) and `unrelatedHelper`'s test (never changed) +/// are both excluded. #[test] -fn to_flag_is_rejected() { - let tmp = init_repo(); +fn to_flag_selects_only_changes_within_from_to_to_range() { + let (tmp, c1, c2, _c3) = init_to_range_repo(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["select", "--from", &c1, "--to", &c2]) + .current_dir(tmp.path()) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + + assert_eq!(json["mode"], "selection"); + let tests = json["tests"].as_array().expect("tests array"); + let names: Vec> = tests + .iter() + .map(|t| { + t["name"] + .as_array() + .unwrap() + .iter() + .map(|s| s.as_str().unwrap().to_string()) + .collect() + }) + .collect(); + + assert!( + names.contains(&vec!["add".to_string(), "handles negatives".to_string()]), + "expected add/handles negatives in {names:?}" + ); + assert!( + names.contains(&vec!["add".to_string(), "handles zero".to_string()]), + "expected add/handles zero in {names:?}" + ); + assert!( + !names + .iter() + .any(|n| n.first().map(|s| s.as_str()) == Some("greets by name")), + "greet's test must NOT be selected for a C1..C2 range, got {names:?}" + ); + assert!( + !names + .iter() + .any(|n| n.first().map(|s| s.as_str()) == Some("unrelatedHelper")), + "unrelatedHelper's test must NOT be selected, got {names:?}" + ); + assert_eq!( + tests.len(), + 2, + "expected exactly 2 selected tests, got {tests:?}" + ); +} + +/// `--from C1 --to C3`: both `add`'s and `greet`'s tests are selected +/// (both changes fall inside the range), `unrelatedHelper`'s stays excluded. +#[test] +fn to_flag_selects_both_changes_across_wider_range() { + let (tmp, c1, _c2, c3) = init_to_range_repo(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["select", "--from", &c1, "--to", &c3]) + .current_dir(tmp.path()) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + + assert_eq!(json["mode"], "selection"); + let tests = json["tests"].as_array().expect("tests array"); + let names: Vec> = tests + .iter() + .map(|t| { + t["name"] + .as_array() + .unwrap() + .iter() + .map(|s| s.as_str().unwrap().to_string()) + .collect() + }) + .collect(); + + assert!( + names.contains(&vec!["add".to_string(), "handles negatives".to_string()]), + "expected add/handles negatives in {names:?}" + ); + assert!( + names.contains(&vec!["add".to_string(), "handles zero".to_string()]), + "expected add/handles zero in {names:?}" + ); + assert!( + names.contains(&vec!["greets by name".to_string()]), + "expected greet's test in {names:?}" + ); + assert!( + !names + .iter() + .any(|n| n.first().map(|s| s.as_str()) == Some("unrelatedHelper")), + "unrelatedHelper's test must NOT be selected, got {names:?}" + ); + assert_eq!( + tests.len(), + 3, + "expected exactly 3 selected tests, got {tests:?}" + ); +} + +/// The temp worktree materializing `--to` is cleaned up after the run: its +/// directory (`$TMPDIR/testless-to-`) must not exist once +/// the process has exited. +#[test] +fn to_flag_cleans_up_temp_worktree_after_run() { + let (tmp, c1, c2, _c3) = init_to_range_repo(); + let worktree_dir = std::env::temp_dir().join(format!("testless-to-{c2}")); + Command::cargo_bin("testless") .unwrap() - .args(["select", "--to", "HEAD"]) + .args(["select", "--from", &c1, "--to", &c2]) + .current_dir(tmp.path()) + .assert() + .success(); + + assert!( + !worktree_dir.exists(), + "expected temp worktree {} to be cleaned up after the run", + worktree_dir.display() + ); +} + +/// A `--to` rev that doesn't resolve locally degrades to the same +/// `run_all`/exit-2 fallback as a bad `--from` rev, rather than a hard +/// error. +#[test] +fn bad_to_rev_degrades_to_run_all_exit_2() { + let tmp = init_repo(); + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["select", "--to", "not-a-real-rev"]) .current_dir(tmp.path()) .assert() - .code(1) - .stderr(predicates::str::contains("not yet supported")); + .code(2); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap(); + assert_eq!(json["mode"], "run_all"); + assert!(json["reason"].as_str().unwrap().contains("--to")); } /// `--format text` prints `file :: seg1 > seg2` lines on stdout and a diff --git a/crates/cli/tests/why.rs b/crates/cli/tests/why.rs index e305963..8f9e20e 100644 --- a/crates/cli/tests/why.rs +++ b/crates/cli/tests/why.rs @@ -182,3 +182,60 @@ fn why_config_file_edit_forces_run_all_exit_2() { .assert() .code(2); } + +fn rev_parse(dir: &std::path::Path, rev: &str) -> String { + let output = std::process::Command::new("git") + .arg("-C") + .arg(dir) + .args(["rev-parse", rev]) + .output() + .expect("failed to spawn git rev-parse"); + assert!(output.status.success(), "git rev-parse {rev} failed"); + String::from_utf8(output.stdout).unwrap().trim().to_string() +} + +/// `--to ` (issue #17): `why` explains the same `add -> fmt -> +/// "formats a sum"` path when given an explicit `--from C1 --to C2` rev +/// range instead of `--from` vs. the live worktree. +#[test] +fn why_explains_path_with_to_flag_rev_range() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + // See `select.rs`'s `init_to_range_repo` for why this nonce matters: + // without it, two parallel test processes committing byte-identical + // content within the same wall-clock second would hash to the same + // commit sha and race on the same `--to` temp worktree path. + std::fs::write(root.join(".nonce"), root.display().to_string()).unwrap(); + std::fs::write( + root.join("package.json"), + "{ \"name\": \"why-to-fixture\" }\n", + ) + .unwrap(); + std::fs::write(root.join("src/math.ts"), MATH_TS).unwrap(); + std::fs::write(root.join("src/format.ts"), FORMAT_TS).unwrap(); + std::fs::write(root.join("src/math.test.ts"), MATH_TEST_TS).unwrap(); + std::fs::write(root.join("src/format.test.ts"), FORMAT_TEST_TS).unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "C1: baseline"]); + let c1 = rev_parse(root, "HEAD"); + + std::fs::write(root.join("src/math.ts"), MATH_TS_BODY_EDITED).unwrap(); + git(root, &["commit", "-am", "C2: edit add's body"]); + let c2 = rev_parse(root, "HEAD"); + + let assert = Command::cargo_bin("testless") + .unwrap() + .args(["why", "formats", "--from", &c1, "--to", &c2]) + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + + assert!(out.contains("add"), "expected the changed def in {out:?}"); + assert!( + out.contains("formats a sum"), + "expected the matched test's name in {out:?}" + ); +} diff --git a/crates/core/src/gitio.rs b/crates/core/src/gitio.rs index 2b5f567..220b2c3 100644 --- a/crates/core/src/gitio.rs +++ b/crates/core/src/gitio.rs @@ -163,6 +163,116 @@ pub fn show_file(repo: &Path, rev: &str, path: &Path) -> Result> bail!("git show {spec} failed: {}", stderr.trim()); } +/// Resolves `rev` to its full object id (`git rev-parse --verify `), +/// used to build a collision-safe [`TempWorktree`] directory name. `Err` for +/// a rev that doesn't resolve locally (bad rev, or one that exists remotely +/// but hasn't been fetched) — the same failure shape `changed_files` already +/// has for a bad `--from`. +fn resolve_rev(repo: &Path, rev: &str) -> Result { + let output = run_git(repo, &["rev-parse", "--verify", rev])?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("git rev-parse --verify {rev} failed: {}", stderr.trim()); + } + let sha = + String::from_utf8(output.stdout).context("git rev-parse output is not valid UTF-8")?; + Ok(sha.trim().to_string()) +} + +/// A temporary, detached-HEAD git worktree checked out at an arbitrary +/// revision (`git worktree add --detach `), backing `--to ` +/// support: the analysis pipeline runs against this checkout as its repo +/// root instead of the caller's actual worktree, so `--from`/`--to` can name +/// any two revisions rather than only `--from` vs. the live worktree. +/// +/// Lives at `$TMPDIR/testless-to-`: the sha suffix (not the +/// raw `rev` string, which might not be filesystem-safe, e.g. `origin/main`) +/// makes the location deterministic and collision-safe across concurrent +/// runs against the same rev. +/// +/// Removed on drop (`git worktree remove --force`, then `git worktree +/// prune`) — including on an early return via `?` anywhere between creation +/// and drop — via a hand-rolled `Drop` guard rather than an explicit cleanup +/// call at every exit path (no `scopeguard` dependency in this crate). +#[derive(Debug)] +pub struct TempWorktree { + path: PathBuf, + repo: PathBuf, +} + +impl TempWorktree { + /// Requires `rev` to already exist in `repo`'s local object store: a + /// rev that hasn't been fetched surfaces as an `Err` here (see + /// `resolve_rev`), not a silent no-op. + pub fn create(repo: &Path, rev: &str) -> Result { + let sha = resolve_rev(repo, rev).with_context(|| format!("resolving --to rev {rev:?}"))?; + let path = std::env::temp_dir().join(format!("testless-to-{sha}")); + + // A leftover directory/registration from a previous crashed or + // force-killed run would otherwise make `git worktree add` fail + // outright and permanently wedge every future `--to ` + // run; best-effort clear both before adding. Failures here are + // expected (nothing to clear) and intentionally ignored. + let _ = Command::new("git") + .arg("-C") + .arg(repo) + .args(["worktree", "remove", "--force"]) + .arg(&path) + .output(); + let _ = std::fs::remove_dir_all(&path); + + let output = Command::new("git") + .arg("-C") + .arg(repo) + .args(["worktree", "add", "--detach"]) + .arg(&path) + .arg(rev) + .output() + .with_context(|| format!("failed to run `git worktree add` for rev {rev:?}"))?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!( + "git worktree add --detach {} {rev} failed: {}", + path.display(), + stderr.trim() + ); + } + + Ok(Self { + path, + repo: repo.to_path_buf(), + }) + } + + /// The checkout directory: the analysis pipeline's repo root for the + /// `--to` rev, valid for exactly as long as `self` is alive. + pub fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for TempWorktree { + fn drop(&mut self) { + // Best-effort: a `Drop` impl can't propagate an error, and a + // cleanup failure (e.g. the directory already gone) shouldn't panic + // mid-unwind. `worktree remove --force` deletes the checkout + // directory itself; `worktree prune` clears the (now-dangling) + // registration in `repo`'s `.git/worktrees/` in the rare case + // `remove` didn't run (e.g. the directory was already gone). + let _ = Command::new("git") + .arg("-C") + .arg(&self.repo) + .args(["worktree", "remove", "--force"]) + .arg(&self.path) + .output(); + let _ = Command::new("git") + .arg("-C") + .arg(&self.repo) + .args(["worktree", "prune"]) + .output(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -286,4 +396,40 @@ mod tests { other => panic!("expected Renamed, got {other:?}"), } } + + #[test] + fn temp_worktree_checks_out_rev_content_and_cleans_up_on_drop() { + let dir = init_repo(); + fs::write(dir.path().join("a.txt"), "second\n").unwrap(); + git(dir.path(), &["commit", "-am", "second commit"]); + + let path = { + let wt = TempWorktree::create(dir.path(), "HEAD~1").expect("create worktree"); + let checked_out = fs::read_to_string(wt.path().join("a.txt")).expect("read a.txt"); + assert_eq!( + checked_out, "original\n", + "worktree should have HEAD~1's content, not HEAD's" + ); + assert!(wt.path().exists()); + wt.path().to_path_buf() + }; + // Dropped at end of the block above; the checkout directory must be + // gone, and the registration pruned (a second `create` against the + // same rev must succeed cleanly, not collide with a stale entry). + assert!( + !path.exists(), + "worktree directory should be removed after drop, still at {}", + path.display() + ); + + let wt2 = TempWorktree::create(dir.path(), "HEAD~1").expect("recreate worktree"); + assert_eq!(wt2.path(), path, "same rev resolves to the same temp path"); + } + + #[test] + fn temp_worktree_bad_rev_is_err() { + let dir = init_repo(); + let result = TempWorktree::create(dir.path(), "not-a-real-rev"); + assert!(result.is_err(), "expected Err for bad rev, got {result:?}"); + } } From a2ff7da47aed7ecd650f3ba2d531bfb91c2644b2 Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 01:33:17 +0300 Subject: [PATCH 8/9] docs: homebrew tap install option (#16) --- README.md | 2 +- site/src/pages/index.astro | 3 ++- site/src/styles/global.css | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 5434c0f..5046ec1 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ widens instead of guessing, and never silently skips a test that could fail. | Ecosystem | Command | |---|---| | JS / TS | `npm i -D testless-cli` (bin: `testless`) | -| Go | `curl -fsSL https://testless.itaywol.tools/install.sh \| sh` | +| Go | `curl -fsSL https://testless.itaywol.tools/install.sh \| sh` or `brew install itaywol/testless/testless` | | Rust | `cargo binstall testless` or `cargo install testless` | Prebuilt binaries on [Releases](https://github.com/itaywol/testless/releases). Nix flake repo: `nix develop`. diff --git a/site/src/pages/index.astro b/site/src/pages/index.astro index 6e22335..fdc73b1 100644 --- a/site/src/pages/index.astro +++ b/site/src/pages/index.astro @@ -93,7 +93,8 @@ const jsonLd = {

Go

- 
curl -fsSL https://testless.itaywol.tools/install.sh | sh
+ 
curl -fsSL https://testless.itaywol.tools/install.sh | sh
+brew install itaywol/testless/testless

Rust

diff --git a/site/src/styles/global.css b/site/src/styles/global.css index fe9a9dc..5b478d3 100644 --- a/site/src/styles/global.css +++ b/site/src/styles/global.css @@ -247,7 +247,7 @@ h2 { border: 1px solid var(--border); border-radius: var(--radius-sm); overflow-x: auto; - white-space: nowrap; + white-space: pre; font-size: 0.8rem; -webkit-overflow-scrolling: touch; mask-image: linear-gradient(to right, black calc(100% - 28px), transparent 100%); From 4d17aa07943d612cc825827096d309810f410e1e Mon Sep 17 00:00:00 2001 From: itay Date: Sat, 25 Jul 2026 01:51:34 +0300 Subject: [PATCH 9/9] fix: sound Go deletion seeding, ignored-change zero-seeds, review nits - classify: a deleted Go file now seeds its surviving package siblings' ModuleInit directly and folds the package directory into the importer stem scan, since Go imports name directories, never file stems, and same-package files never reference each other by name (was under- selecting to empty). - analyze: filter `changed` by testless.toml's ignore globset before classify, so an ignored file's change contributes zero seeds instead of hitting classify's "missing from graph" path and forcing run_all. - gitio: TempWorktree dir gets a per-process pid suffix (fixes a real collision between concurrent runs against the same --to rev) and its doc comment's incorrect "collision-safe" claim is corrected. - classify: scan_importers requires a >=3 char stem for its substring match; a deleted+indexed file left with no usable stem escalates to RunAll (under-select forbidden) rather than a non-indexed file's short stem, which just yields no extra seeds (accepted, lower-stakes case). --- crates/cli/src/main.rs | 16 +++- crates/cli/tests/changes.rs | 106 ++++++++++++++++++++++++ crates/core/src/classify.rs | 160 +++++++++++++++++++++++++++++++----- crates/core/src/gitio.rs | 14 ++-- 4 files changed, 268 insertions(+), 28 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 8f308f0..9b9958f 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -366,10 +366,22 @@ fn analyze( let (mode, changed_count) = match gitio::changed_files(&cwd, from, to) { Ok(changed) => { - let mode = classify(root, &graph, ®, &changed, &extractions, &|p| { + let changed_count = changed.len(); + // Files matching testless.toml's `ignore` globs were dropped at + // discovery time (see `discover`/`index_repo_incremental` + // above), so they're absent from `graph`/`extractions`; routing + // one into `classify` would hit its "indexed file missing from + // new graph" error path and force a spurious `RunAll`. `ignore` + // is discovery-level (see `config` module docs): a matching + // change must contribute zero seeds, not run everything. + let filtered: Vec<_> = changed + .into_iter() + .filter(|c| !ignore.is_match(&c.path)) + .collect(); + let mode = classify(root, &graph, ®, &filtered, &extractions, &|p| { gitio::show_file(&cwd, from, p) }); - (mode, changed.len()) + (mode, changed_count) } Err(err) => { let reason = format!("listing files changed since --from: {err:#}"); diff --git a/crates/cli/tests/changes.rs b/crates/cli/tests/changes.rs index bdea364..0bfe347 100644 --- a/crates/cli/tests/changes.rs +++ b/crates/cli/tests/changes.rs @@ -566,6 +566,75 @@ fn extensionless_import_matches_changed_file_by_stem() { ); } +/// (i) Go's imports name package *directories*, never file stems, and +/// same-package sibling files reference each other via nothing at all (no +/// import statement). Deleting `pkg/helper.go` (whose `init()` a sibling +/// test depends on structurally, since `init` folds into the file's +/// ``) must still seed the surviving same-package siblings' +/// `ModuleInit` — the stem-based importer scan alone finds nothing here +/// (no file anywhere imports the literal text `helper`/`helper.go`), so +/// before this fix the selection was empty (issue: Go file deletion +/// under-selects). +#[test] +fn deleted_go_file_seeds_package_siblings_module_init() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write(root, "go.mod", "module example.com/m\n\ngo 1.22\n"); + write( + root, + "pkg/widget.go", + "package pkg\n\nfunc Widget() int { return 1 }\n", + ); + write( + root, + "pkg/widget_test.go", + "package pkg\n\nimport \"testing\"\n\nfunc TestWidget(t *testing.T) {\n\tif Widget() != 1 {\n\t\tt.Fail()\n\t}\n}\n", + ); + + let registry = Registry::new(vec![Box::new(testless_lang_go::GoLanguage)]); + let (graph, extractions, _) = index_repo_incremental(root, ®istry, None, None).unwrap(); + + let test_file_id = testless_core::FileId( + graph + .files + .iter() + .position(|f| f.path.ends_with("widget_test.go")) + .unwrap() as u32, + ); + let test_module_init = graph + .module_init(test_file_id) + .expect("module_init present"); + + // helper.go (a third file in the same package, never written to disk + // here: it's the file being deleted) held the sibling this test's + // `` depended on. Its own `` hash included an `init()` + // whose body a sibling test structurally depended on; deleting it must + // still seed the surviving siblings' `ModuleInit`. + let changed = vec![ChangedFile { + path: PathBuf::from("pkg/helper.go"), + status: FileStatus::Deleted, + }]; + + let mode = classify( + root, + &graph, + ®istry, + &changed, + &extractions, + &no_old_content, + ); + match mode { + ChangeMode::Selection(seeds) => { + assert!(!seeds.is_empty(), "expected a non-empty selection"); + assert!( + seeds.iter().any(|s| s.def == test_module_init), + "expected the package test file's ModuleInit seeded, got {seeds:?}" + ); + } + other => panic!("expected Selection (not RunAll), got {other:?}"), + } +} + // --- `testless changes` CLI e2e coverage (Plan 3, Task 5) -------------- mod cli_changes { @@ -747,6 +816,43 @@ mod cli_changes { ); } + /// A modified file matching `testless.toml`'s `ignore` globs is + /// discovery-level dropped (never indexed, see `discover`), so before + /// this fix it hit `classify`'s "indexed file missing from new graph" + /// error path and forced `run_all`. `ignore`'s contract is that a + /// matching change contributes zero seeds, same as any other change + /// `testless` can prove has no impact — not a blanket run-everything. + #[test] + fn ignored_file_change_yields_empty_selection_not_run_all() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("src/generated")).unwrap(); + std::fs::write(root.join("src/generated/api.ts"), "export const x = 1;\n").unwrap(); + std::fs::write( + root.join("testless.toml"), + "ignore = [\"**/generated/**\"]\n", + ) + .unwrap(); + git(root, &["init", "-b", "main"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-m", "initial"]); + + std::fs::write(root.join("src/generated/api.ts"), "export const x = 2;\n").unwrap(); + + let assert = Command::cargo_bin("testless") + .unwrap() + .arg("changes") + .current_dir(root) + .assert() + .success(); + let out = String::from_utf8(assert.get_output().stdout.clone()).unwrap(); + let json: serde_json::Value = serde_json::from_str(out.trim()).unwrap_or_else(|e| { + panic!("expected JSON stdout, got {out:?} ({e})"); + }); + assert_eq!(json["mode"], "selection"); + assert_eq!(json["seeds"].as_array().unwrap().len(), 0); + } + /// A `--to` rev that doesn't resolve locally degrades to the same /// `run_all`/exit-2 fallback as a bad `--from` rev, rather than a hard /// error. diff --git a/crates/core/src/classify.rs b/crates/core/src/classify.rs index ef1b7fc..ba2d4a9 100644 --- a/crates/core/src/classify.rs +++ b/crates/core/src/classify.rs @@ -15,6 +15,15 @@ //! its own tests died with it. (A `Renamed` file is handled by rule 4 //! instead: its old content is diffed directly against its new-path //! content, so rename semantics are unaffected.) +//! +//! A deleted **Go** file is a special case (Go imports name package +//! *directories*, never file stems, and same-package sibling files +//! reference each other via nothing at all): see [`seed_go_deletion`]. +//! +//! A deleted file that *is* indexed but whose name yields no usable scan +//! needle (see [`stem_needles`]) can't be soundly narrowed by the stem +//! scan at all; rather than silently under-selecting (contributing zero +//! seeds when a real importer might exist), this escalates to `RunAll`. //! 3. Added indexed file -> seed its `TestCase` defs and its `ModuleInit`, //! both `SeedKind::Added` (new exports; nothing referenced them before). //! 4. Modified/Renamed indexed file -> re-parse old vs. new content with @@ -23,7 +32,11 @@ //! *indexed* file's raw import text references it (substring match on //! basename, e.g. an import of `"./config.json"` matches changed path //! `config.json`), seed that importer's `ModuleInit`; otherwise the file -//! contributes zero seeds (e.g. a README edit). +//! contributes zero seeds (e.g. a README edit). Unlike rule 2, a missing +//! scan needle here just yields no *extra* seeds rather than escalating: +//! an unindexed file was never a selection candidate itself, so the +//! stakes of under-selecting its (rare, short-named) importers are +//! accepted. //! 6. Any I/O/parse error anywhere -> `RunAll` with a reason naming the //! file. //! @@ -129,6 +142,10 @@ pub fn classify( match classify_one(repo, new_graph, registry, c, old_src_of) { Ok(PerFile::Seeds(mut s)) => seeds.append(&mut s), Ok(PerFile::ScanImporters(path)) => needs_import_scan.push(path), + Ok(PerFile::SeedsAndScan(mut s, mut paths)) => { + seeds.append(&mut s); + needs_import_scan.append(&mut paths); + } Err(reason) => return ChangeMode::RunAll { reason }, } } @@ -153,6 +170,11 @@ enum PerFile { /// or not). Batched up for a single pass over every indexed file's raw /// imports. ScanImporters(PathBuf), + /// A deleted Go file: seeds found directly (its surviving package + /// siblings' `ModuleInit`, see [`seed_go_deletion`]) plus one or more + /// extra paths for the raw-import stem scan (the deleted file's own + /// path, and its package directory). + SeedsAndScan(Vec, Vec), } fn classify_one( @@ -167,6 +189,22 @@ fn classify_one( // to parse: fall back to the raw-import stem scan so any surviving // importer's now-dangling reference still seeds that importer's // `ModuleInit` (see rule 2's doc comment above). + if let Some(lang) = registry.for_path(&c.path) { + if lang.id() == "go" { + return seed_go_deletion(new_graph, &c.path); + } + if stem_needles(&c.path).is_empty() { + // An indexed file whose name is too short to yield any + // usable scan needle (see `stem_needles`): the stem scan + // below would silently find nothing, which for an + // *indexed* deleted file is an under-select, not a benign + // zero-seed result. Escalate rather than guess. + return Err(format!( + "deleted indexed file {} has no usable stem (<{MIN_STEM_LEN} chars) to find importers", + c.path.display() + )); + } + } return Ok(PerFile::ScanImporters(c.path.clone())); } @@ -263,32 +301,49 @@ fn seed_added_file(new_graph: &Graph, file_id: FileId) -> Vec { seeds } +/// Minimum length (in chars) a [`stem_needles`] candidate must have to be +/// used as a substring match in [`scan_importers`]. Below this, a "match" +/// is more likely a false positive (e.g. stem `"io"` inside unrelated +/// `"studio"`) than a real reference, so short candidates are dropped +/// rather than trusted (Item 4). +const MIN_STEM_LEN: usize = 3; + +/// The substring needles [`scan_importers`] uses for `path`: its full +/// basename (e.g. `config.json`) and its extension-stripped stem (e.g. +/// `config`, covering an extensionless import specifier like `import cfg +/// from "./config"`), each kept only when at least [`MIN_STEM_LEN`] chars +/// long. Empty when both candidates are too short (or `path` has no +/// usable file name at all). +fn stem_needles(path: &Path) -> Vec { + let mut out = Vec::new(); + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + if name.chars().count() >= MIN_STEM_LEN { + out.push(name.to_string()); + } + } + if let Some(stem) = path.file_stem().and_then(|n| n.to_str()) { + if stem.chars().count() >= MIN_STEM_LEN && out.iter().all(|s| s != stem) { + out.push(stem.to_string()); + } + } + out +} + /// Scan every already-indexed file's raw import text (from `extractions`, /// which lines up index-for-index with `new_graph.files`; no re-reading or -/// re-parsing) for a reference to any of `changed_paths` (basename substring -/// match: both the full basename, e.g. `config.json`, and the basename with -/// its extension stripped, e.g. `config`, so an extensionless import -/// specifier like `import cfg from "./config"` still matches a changed -/// `config.json`). Each match seeds that importing file's `ModuleInit`. -/// `changed_paths` may be files with no registered `Language` *or* deleted -/// files (indexed or not) — either way there's no def-level diff to run, so -/// this stem scan is the only way to find who's affected. +/// re-parsing) for a reference to any of `changed_paths` (substring match +/// against each path's [`stem_needles`]). Each match seeds that importing +/// file's `ModuleInit`. `changed_paths` may be files with no registered +/// `Language`, deleted files (indexed or not), or (for a deleted Go file, +/// see [`seed_go_deletion`]) a package directory path — either way there's +/// no def-level diff to run, so this stem scan is the only way to find +/// who's affected. fn scan_importers( new_graph: &Graph, extractions: &[CachedExtraction], changed_paths: &[PathBuf], ) -> Vec { - let mut stems: Vec<&str> = Vec::new(); - for p in changed_paths { - if let Some(name) = p.file_name().and_then(|n| n.to_str()) { - stems.push(name); - } - if let Some(stem) = p.file_stem().and_then(|n| n.to_str()) { - if !stem.is_empty() { - stems.push(stem); - } - } - } + let stems: Vec = changed_paths.iter().flat_map(|p| stem_needles(p)).collect(); if stems.is_empty() { return Vec::new(); } @@ -298,7 +353,7 @@ fn scan_importers( let matched = extraction .imports .iter() - .any(|imp| stems.iter().any(|stem| imp.raw.contains(stem))); + .any(|imp| stems.iter().any(|stem| imp.raw.contains(stem.as_str()))); if matched { let file_id = FileId(i as u32); if let Some(m) = new_graph.module_init(file_id) { @@ -312,6 +367,69 @@ fn scan_importers( seeds } +/// A deleted **Go** source file: Go imports name package *directories* +/// (e.g. `example.com/m/pkg`), never a file's stem, and files within the +/// same package don't import each other at all (no statement references a +/// same-package sibling by name). So neither half of the ordinary deleted- +/// file handling (rule 2) can find the right seeds on its own: +/// +/// - The basename/stem scan (`scan_importers` via `stem_needles`) never +/// matches a same-package sibling, since nothing imports it by name. +/// - It also can't find *other* packages' importers unless it's given the +/// package directory's own name as a needle (an import of +/// `example.com/m/pkg` contains `pkg`, the directory's basename). +/// +/// Fix: (a) directly seed the `ModuleInit` of every surviving `new_graph` +/// file in the same directory as `deleted_path` (its package siblings, +/// including any test file — [`sibling_module_inits`]), and (b) still run +/// the ordinary stem scan, but with the package directory's path folded in +/// as an extra needle source alongside the deleted file's own path. +/// +/// If neither the deleted file's own name nor its package directory's name +/// yields a usable [`stem_needles`] candidate, this can't soundly rule out +/// an importer in another package — same as the non-Go short-stem case, +/// escalates to `RunAll` rather than silently under-selecting. +fn seed_go_deletion(new_graph: &Graph, deleted_path: &Path) -> Result { + let seeds = sibling_module_inits(new_graph, deleted_path); + + let mut scan_paths = vec![deleted_path.to_path_buf()]; + if let Some(dir) = deleted_path.parent() { + if !dir.as_os_str().is_empty() { + scan_paths.push(dir.to_path_buf()); + } + } + + let has_needle = scan_paths.iter().any(|p| !stem_needles(p).is_empty()); + if !has_needle { + return Err(format!( + "deleted go file {} (and its package directory) has no usable stem (<{MIN_STEM_LEN} chars) to find importers", + deleted_path.display() + )); + } + + Ok(PerFile::SeedsAndScan(seeds, scan_paths)) +} + +/// Every `new_graph` file sharing `deleted_path`'s parent directory (i.e. +/// its Go package siblings, including test files), paired with its +/// `ModuleInit` def where present, as `SeedKind::ModuleInit` seeds. +fn sibling_module_inits(new_graph: &Graph, deleted_path: &Path) -> Vec { + let dir = deleted_path.parent(); + let mut seeds = Vec::new(); + for (i, f) in new_graph.files.iter().enumerate() { + if f.path.parent() == dir { + let file_id = FileId(i as u32); + if let Some(m) = new_graph.module_init(file_id) { + seeds.push(Seed { + def: m, + kind: SeedKind::ModuleInit, + }); + } + } + } + seeds +} + fn find_file_id(graph: &Graph, path: &Path) -> Option { graph .files diff --git a/crates/core/src/gitio.rs b/crates/core/src/gitio.rs index 220b2c3..2f7c084 100644 --- a/crates/core/src/gitio.rs +++ b/crates/core/src/gitio.rs @@ -185,10 +185,13 @@ fn resolve_rev(repo: &Path, rev: &str) -> Result { /// root instead of the caller's actual worktree, so `--from`/`--to` can name /// any two revisions rather than only `--from` vs. the live worktree. /// -/// Lives at `$TMPDIR/testless-to-`: the sha suffix (not the -/// raw `rev` string, which might not be filesystem-safe, e.g. `origin/main`) -/// makes the location deterministic and collision-safe across concurrent -/// runs against the same rev. +/// Lives at `$TMPDIR/testless-to--`: the sha suffix +/// (not the raw `rev` string, which might not be filesystem-safe, e.g. +/// `origin/main`) makes the location deterministic per revision, and the +/// trailing pid suffix keeps two concurrent `testless` processes analyzing +/// the *same* rev (e.g. two CI jobs racing the same PR) from colliding on +/// one worktree — each process gets its own, since `git worktree add` +/// refuses to reuse a path/registration another checkout is still using. /// /// Removed on drop (`git worktree remove --force`, then `git worktree /// prune`) — including on an early return via `?` anywhere between creation @@ -206,7 +209,8 @@ impl TempWorktree { /// `resolve_rev`), not a silent no-op. pub fn create(repo: &Path, rev: &str) -> Result { let sha = resolve_rev(repo, rev).with_context(|| format!("resolving --to rev {rev:?}"))?; - let path = std::env::temp_dir().join(format!("testless-to-{sha}")); + let pid = std::process::id(); + let path = std::env::temp_dir().join(format!("testless-to-{sha}-{pid}")); // A leftover directory/registration from a previous crashed or // force-killed run would otherwise make `git worktree add` fail