From 9590f4e5b3e12f488b00b6808401bd92549d01f0 Mon Sep 17 00:00:00 2001 From: Sephyi Date: Sun, 19 Apr 2026 19:07:23 +0200 Subject: [PATCH] test: add fixtures exercising ChangeStatus::Deleted and ::Renamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per audit D-037, the test suite never constructed `ChangeStatus::Deleted` or meaningfully exercised `ChangeStatus::Renamed` through the splitter or context builder. This closes that gap: tests/helpers.rs - Extract `make_renamed_file_with_diff` so rename fixtures can carry a non-empty diff body plus explicit add/delete counts. The original `make_renamed_file` becomes a thin wrapper for existing callers. tests/context.rs - Extend `format_files_shows_renamed_marker` to assert the old path, the `->` arrow, and the new path all appear in `file_breakdown` — proving `old_path` is respected, not silently dropped. - Add `format_files_mixed_statuses_show_all_markers`: one FileChange per variant in a single StagedChanges, asserting every `[+]`/`[M]`/ `[-]`/`[R]` prefix and every `change_summary` count fires. tests/splitter.rs - `deleted_file_is_represented_in_splitter_output`: a deleted file colocated with a sibling modification must land in exactly one group (or the SingleCommit branch) — never dropped. - `deleted_file_is_placed_into_a_splitter_group`: with a symbol-bearing addition in an unrelated module, the deletion still lands in exactly one split group. - `renamed_file_grouped_by_new_path_module`: the splitter must key module detection off the new path, not `old_path`. - `renamed_file_is_placed_into_a_splitter_group_with_old_path_preserved`: splitter output references the rename by its new path, and both `old_path` and `rename_similarity` round-trip unchanged on the input. Closes audit entry D-037 from #3. --- tests/context.rs | 83 +++++++++++++++++ tests/helpers.rs | 21 ++++- tests/splitter.rs | 227 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 327 insertions(+), 4 deletions(-) diff --git a/tests/context.rs b/tests/context.rs index 79958a6..358664c 100644 --- a/tests/context.rs +++ b/tests/context.rs @@ -1407,6 +1407,89 @@ fn format_files_shows_renamed_marker() { "renamed file should show similarity: {}", ctx.file_breakdown ); + // old_path must be respected in the formatted output — the old path appears + // on the left of the `->` arrow so reviewers can see what was renamed from. + assert!( + ctx.file_breakdown.contains("src/old_name.rs"), + "renamed file should include the old path in output: {}", + ctx.file_breakdown + ); + assert!( + ctx.file_breakdown.contains("->"), + "renamed file should show an arrow between old and new path: {}", + ctx.file_breakdown + ); + assert!( + ctx.file_breakdown.contains("src/new_name.rs"), + "renamed file should include the new path in output: {}", + ctx.file_breakdown + ); +} + +#[test] +fn format_files_mixed_statuses_show_all_markers() { + // One of each ChangeStatus variant in the same StagedChanges — verify every + // code path in `format_files` fires and produces the expected status prefix. + let changes = make_staged_changes(vec![ + make_file_change("src/added.rs", ChangeStatus::Added, "+pub fn n() {}", 1, 0), + make_file_change( + "src/modified.rs", + ChangeStatus::Modified, + "-old\n+new", + 1, + 1, + ), + make_file_change( + "src/deleted.rs", + ChangeStatus::Deleted, + "-pub fn g() {}", + 0, + 1, + ), + make_renamed_file("src/old.rs", "src/new.rs", 90), + ]); + let ctx = ContextBuilder::build(&changes, &[], &[], &default_config()); + assert!( + ctx.file_breakdown.contains("[+] src/added.rs"), + "added file should use [+] marker: {}", + ctx.file_breakdown + ); + assert!( + ctx.file_breakdown.contains("[M] src/modified.rs"), + "modified file should use [M] marker: {}", + ctx.file_breakdown + ); + assert!( + ctx.file_breakdown.contains("[-] src/deleted.rs"), + "deleted file should use [-] marker: {}", + ctx.file_breakdown + ); + assert!( + ctx.file_breakdown.contains("[R] src/old.rs -> src/new.rs"), + "renamed file should use [R] with old -> new formatting: {}", + ctx.file_breakdown + ); + // The change-summary count aggregates per status; verify it observed each. + assert!( + ctx.change_summary.contains("1 added"), + "change_summary should count added file: {}", + ctx.change_summary + ); + assert!( + ctx.change_summary.contains("1 modified"), + "change_summary should count modified file: {}", + ctx.change_summary + ); + assert!( + ctx.change_summary.contains("1 deleted"), + "change_summary should count deleted file: {}", + ctx.change_summary + ); + assert!( + ctx.change_summary.contains("1 renamed"), + "change_summary should count renamed file: {}", + ctx.change_summary + ); } // ─── Test Coverage: classify_span_change None path (#39) ────────────────────── diff --git a/tests/helpers.rs b/tests/helpers.rs index c825c5a..5663b75 100644 --- a/tests/helpers.rs +++ b/tests/helpers.rs @@ -32,12 +32,27 @@ pub fn make_file_change( /// Create a renamed FileChange for testing #[allow(dead_code)] pub fn make_renamed_file(old_path: &str, new_path: &str, similarity: u8) -> FileChange { + make_renamed_file_with_diff(old_path, new_path, similarity, "", 0, 0) +} + +/// Create a renamed FileChange with a diff body and explicit add/delete counts. +/// +/// Useful for splitter tests that exercise diff-shape grouping on renames. +#[allow(dead_code)] +pub fn make_renamed_file_with_diff( + old_path: &str, + new_path: &str, + similarity: u8, + diff: &str, + additions: usize, + deletions: usize, +) -> FileChange { FileChange { path: PathBuf::from(new_path), status: ChangeStatus::Renamed, - diff: Arc::from(""), - additions: 0, - deletions: 0, + diff: Arc::from(diff), + additions, + deletions, category: FileCategory::from_path(&PathBuf::from(new_path)), is_binary: false, old_path: Some(PathBuf::from(old_path)), diff --git a/tests/splitter.rs b/tests/splitter.rs index 2adffdb..6a7700d 100644 --- a/tests/splitter.rs +++ b/tests/splitter.rs @@ -8,7 +8,9 @@ use std::path::PathBuf; use commitbee::domain::{ChangeStatus, CodeSymbol, SymbolKind}; use commitbee::services::splitter::{CommitSplitter, SplitSuggestion}; -use helpers::{make_file_change, make_staged_changes}; +use helpers::{ + make_file_change, make_renamed_file, make_renamed_file_with_diff, make_staged_changes, +}; // ─── Helpers ───────────────────────────────────────────────────────────────── @@ -550,3 +552,226 @@ fn subset_empty_paths_returns_empty() { assert!(subset.files.is_empty()); assert_eq!(subset.stats.files_changed, 0); } + +// ─── ChangeStatus::Deleted fixtures (audit D-037) ─────────────────────────── + +/// Baseline: a deleted file must not be silently dropped. Either the splitter +/// returns SingleCommit (where all files are implicitly in the one group) or +/// the deletion lands in exactly one split group. +#[test] +fn deleted_file_is_represented_in_splitter_output() { + let changes = make_staged_changes(vec![ + make_file_change( + "src/services/llm/ollama.rs", + ChangeStatus::Modified, + "@@ -10,3 +10,3 @@\n-let old = 1;\n+let new = 2;\n", + 1, + 1, + ), + make_file_change( + "src/services/llm/legacy.rs", + ChangeStatus::Deleted, + "@@ -1,10 +0,0 @@\n-pub fn retired() {}\n-pub fn also_retired() {}\n", + 0, + 10, + ), + ]); + + let result = CommitSplitter::analyze(&changes, &[]); + let deleted_path = PathBuf::from("src/services/llm/legacy.rs"); + + match &result { + SplitSuggestion::SuggestSplit(groups) => { + let occurrences = groups + .iter() + .filter(|g| g.files.contains(&deleted_path)) + .count(); + assert_eq!( + occurrences, 1, + "deleted file must appear in exactly one split group: {:?}", + groups + ); + } + SplitSuggestion::SingleCommit => { + // Single-commit means all files fall into the one implicit group + // — the deleted file is still present in the input, so no further + // assertion is needed beyond verifying the input shape. + assert!(changes.files.iter().any(|f| f.path == deleted_path)); + } + } +} + +/// With a symbol-bearing addition in an unrelated module, the splitter is +/// likely to propose a split — the deletion must still land in exactly one +/// group (never duplicated, never dropped). +#[test] +fn deleted_file_is_placed_into_a_splitter_group() { + let changes = make_staged_changes(vec![ + make_file_change( + "src/services/llm/anthropic.rs", + ChangeStatus::Modified, + "@@ -0,0 +1,20 @@\n+pub fn brand_new_api() {}\n", + 20, + 0, + ), + make_file_change( + "src/services/sanitizer.rs", + ChangeStatus::Deleted, + "@@ -1,5 +0,0 @@\n-pub fn removed_sanitiser() {}\n", + 0, + 5, + ), + ]); + + let symbols = vec![make_symbol( + "brand_new_api", + SymbolKind::Function, + "src/services/llm/anthropic.rs", + true, + true, + )]; + + let result = CommitSplitter::analyze(&changes, &symbols); + let deleted_path = PathBuf::from("src/services/sanitizer.rs"); + + match result { + SplitSuggestion::SuggestSplit(groups) => { + let occurrences = groups + .iter() + .filter(|g| g.files.contains(&deleted_path)) + .count(); + assert_eq!( + occurrences, 1, + "deleted file should appear in exactly one split group: groups={:?}", + groups + ); + } + SplitSuggestion::SingleCommit => { + // A single commit is also acceptable — but the deleted file must + // have been surfaced via `changes.files` iteration (the splitter + // does not drop files from the input), so we assert the input + // invariant for clarity. + assert!(changes.files.iter().any(|f| f.path == deleted_path)); + } + } +} + +// ─── ChangeStatus::Renamed fixtures (audit D-037) ─────────────────────────── + +/// The splitter must key off the *new* path (`path`) for module detection, +/// not the stale `old_path`. A rename + sibling modification under the same +/// new module should collapse into a single commit. +#[test] +fn renamed_file_grouped_by_new_path_module() { + let changes = make_staged_changes(vec![ + make_renamed_file_with_diff( + "src/services/old_module/helper.rs", + "src/services/llm/helper.rs", + 95, + "@@ -1,3 +1,3 @@\n-use old_name;\n+use new_name;\n", + 1, + 1, + ), + make_file_change( + "src/services/llm/ollama.rs", + ChangeStatus::Modified, + "@@ -1,3 +1,3 @@\n-use old_name;\n+use new_name;\n", + 1, + 1, + ), + ]); + + let result = CommitSplitter::analyze(&changes, &[]); + + // Rename lives in the same module (`llm`) as the modification under its + // new path — splitter should produce a single commit. This asserts the + // splitter consults the new path (`path`) for module detection, not the + // stale `old_path`. + assert!( + matches!(result, SplitSuggestion::SingleCommit), + "renamed file under the same new-module as a sibling modification should group into a single commit, got {:?}", + result + ); + + // Confirm old_path survived the pipeline unchanged (the splitter uses + // `changes.subset` internally which clones FileChange records — so the + // original `old_path` must still be present on the input). + let renamed = changes + .files + .iter() + .find(|f| f.status == ChangeStatus::Renamed) + .expect("renamed file should be present"); + assert_eq!( + renamed.old_path.as_deref(), + Some(std::path::Path::new("src/services/old_module/helper.rs")), + "old_path must be preserved verbatim", + ); +} + +/// With a rename beside an unrelated addition, the renamed file must be +/// referenced by its *new* path in whichever group holds it, and `old_path` +/// / `rename_similarity` must round-trip unchanged through the splitter input. +#[test] +fn renamed_file_is_placed_into_a_splitter_group_with_old_path_preserved() { + let rename = make_renamed_file("src/parser/old.rs", "src/parser/new.rs", 88); + let changes = make_staged_changes(vec![ + rename, + make_file_change( + "src/services/llm/anthropic.rs", + ChangeStatus::Modified, + "@@ -0,0 +1,20 @@\n+pub fn brand_new_api() {}\n", + 20, + 0, + ), + ]); + + let symbols = vec![make_symbol( + "brand_new_api", + SymbolKind::Function, + "src/services/llm/anthropic.rs", + true, + true, + )]; + + let result = CommitSplitter::analyze(&changes, &symbols); + let renamed_path = PathBuf::from("src/parser/new.rs"); + + // Whether the result is SingleCommit or SuggestSplit, the renamed file + // must be represented by its *new* path in the splitter output, and the + // original `old_path` must still be reachable via the input `changes`. + match &result { + SplitSuggestion::SuggestSplit(groups) => { + let occurrences = groups + .iter() + .filter(|g| g.files.contains(&renamed_path)) + .count(); + assert_eq!( + occurrences, 1, + "renamed file should appear under its new path in exactly one group: {:?}", + groups + ); + } + SplitSuggestion::SingleCommit => { + assert!(changes.files.iter().any(|f| f.path == renamed_path)); + } + } + + // old_path must be retrievable from the original StagedChanges the caller + // passed in — the splitter borrows, it does not mutate, so this is a + // regression guard rather than a behaviour assertion. + let renamed = changes + .files + .iter() + .find(|f| f.path == renamed_path) + .expect("renamed file should be reachable via new path"); + assert_eq!( + renamed.old_path.as_deref(), + Some(std::path::Path::new("src/parser/old.rs")), + "old_path for the rename must still be the original path", + ); + assert_eq!( + renamed.rename_similarity, + Some(88), + "rename_similarity must round-trip through the splitter input", + ); +}