diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs index 654d220..460d0ef 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs @@ -31,3 +31,16 @@ pub use sheet::Sheet; pub use style::{BorderEdge, BorderTarget, CellAlign, CellStyle, Color, NumberFormat}; pub use undo::{Change, ChangeSet}; pub use workbook_api::{Workbook, WorkbookCommand}; + +#[cfg(test)] +pub(crate) mod test_disk { + use std::sync::{Mutex, MutexGuard}; + + static LOCK: Mutex<()> = Mutex::new(()); + + /// Serialize tests that touch the shared `generated/` save files, so + /// parallel `cargo test` workers cannot clobber each other's fixtures. + pub(crate) fn lock() -> MutexGuard<'static, ()> { + LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) + } +} diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs index a2ed84e..7b9946a 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs @@ -197,6 +197,7 @@ mod tests { /// clobbering another test's fixture. #[test] fn the_legacy_file_is_read_when_the_current_one_is_missing() { + let _disk = crate::test_disk::lock(); let current = sheet_generated_file_path(); let legacy = sheet_generated_legacy_file_path(); let saved_current = fs::read_to_string(¤t).ok(); diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs index 044cf5b..10124b2 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs @@ -368,4 +368,46 @@ mod tests { // =AZ10 with 0,+1 → =BA10 assert_eq!(adjust_formula_refs("=AZ10", 0, 1), "=BA10"); } + + // --- write_col_letters (no-alloc variant used by the grid draw loop) --- + + #[test] + fn write_col_letters_matches_col_letters() { + for i in [0u32, 1, 25, 26, 27, 51, 52, 701, 702, 18277] { + let mut buf = String::from("leftover"); + write_col_letters(i, &mut buf); + assert_eq!(buf, col_letters(i), "write_col_letters({i}) diverged"); + } + // The buffer is cleared on entry, so a shorter label leaves no + // stale tail from the previous call. + let mut buf = String::new(); + write_col_letters(702, &mut buf); // "AAA" + write_col_letters(0, &mut buf); // "A" + assert_eq!(buf, "A"); + } + + // --- adjust_formula_refs: digit-bearing names and overflow guards --- + + #[test] + fn adjust_refs_function_name_with_digits() { + // LOG10 has digits in its identifier; it must not be treated as a + // cell reference. + assert_eq!(adjust_formula_refs("=LOG10(100)", 1, 1), "=LOG10(100)"); + } + + #[test] + fn adjust_refs_overflowing_column_is_left_alone() { + // An 8-letter column exceeds u32 in `col_str_to_index`, so the + // scanner hits the overflow fallback. With a zero delta the output + // is verbatim, proving the fallback copies rather than mangles. + assert_eq!(adjust_formula_refs("=AAAAAAAA1", 0, 0), "=AAAAAAAA1"); + } + + #[test] + fn adjust_refs_overflowing_row_is_left_alone() { + // A 30-digit row does not fit i64, so the scanner copies it verbatim. + let row = "9".repeat(30); + let formula = format!("=A{}", row); + assert_eq!(adjust_formula_refs(&formula, 1, 1), formula); + } } diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs index 0cb4a68..f6fd8a9 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs @@ -144,18 +144,15 @@ impl Workbook { } /// Load from the default save path (`generated/current.workbook.tsv`). - /// Falls back to legacy single-sheet format if the workbook file + /// Falls back to the legacy single-sheet format if the workbook file /// doesn't exist. pub fn load() -> Option { + // `deserialize_workbook` already migrates the legacy V1 formats + // (single-sheet `#MP_SHEET_V2` and the older CSV) into a one-sheet + // workbook, so there is no separate legacy branch to take here. let saved = load_saved_spreadsheet_state()?; - if let Some((sheets, active)) = deserialize_workbook(&saved) { - return Some(Self { sheets, active }); - } - if is_legacy_workbook_payload(&saved) { - Some(migrate_legacy_workbook(&saved)) - } else { - None - } + let (sheets, active) = deserialize_workbook(&saved)?; + Some(Self { sheets, active }) } /// Save to the default save path. @@ -495,22 +492,6 @@ impl Workbook { } } -fn is_legacy_workbook_payload(serialized: &str) -> bool { - let first = serialized.lines().next().unwrap_or_default().trim(); - first == "#MP_SHEET_V2" || first.contains(',') -} - -fn migrate_legacy_workbook(serialized: &str) -> Workbook { - let mut data = SpreadsheetData::default(); - data.deserialize(serialized); - data.undo_stack.clear(); - data.redo_stack.clear(); - Workbook { - sheets: vec![Sheet::new("Sheet 1", data)], - active: 0, - } -} - impl Default for Workbook { fn default() -> Self { Self::new() @@ -692,23 +673,6 @@ mod tests { assert_eq!(wb.get_raw(0, 0), "test"); } - #[test] - fn legacy_payload_detection_rejects_arbitrary_text() { - assert!(is_legacy_workbook_payload("#MP_SHEET_V2\n")); - assert!(is_legacy_workbook_payload("0,0,legacy\n")); - assert!(!is_legacy_workbook_payload("not a workbook")); - } - - #[test] - fn legacy_payload_migrates_to_one_sheet() { - let mut data = SpreadsheetData::default(); - data.set_cell(0, 0, "legacy"); - let migrated = migrate_legacy_workbook(&data.serialize()); - assert_eq!(migrated.sheet_count(), 1); - assert_eq!(migrated.get_raw(0, 0), "legacy"); - assert_eq!(migrated.active_sheet(), 0); - } - #[test] fn workbook_try_deserialize_reports_invalid_input() { let mut wb = Workbook::new(); @@ -973,4 +937,190 @@ mod tests { wb.set_active_sheet(1); assert_eq!(wb.get_display_value(0, 0), "20"); } + + // ── Previously-uncovered branches ─────────────────────────────────── + + /// An empty sheet list cannot build a workbook, so `with_sheets_active` + /// falls back to a fresh single-sheet workbook. + #[test] + fn with_sheets_active_empty_defaults_to_one_sheet() { + let wb = Workbook::with_sheets_active(vec![], 7); + assert_eq!(wb.sheet_count(), 1); + assert_eq!(wb.active_sheet(), 0); + } + + /// Setting an out-of-range active index is a no-op, not a panic. + #[test] + fn set_active_sheet_out_of_range_is_noop() { + let mut wb = Workbook::new(); + wb.set_active_sheet(99); + assert_eq!(wb.active_sheet(), 0); + } + + /// Removing a sheet *before* the active one shifts the active index + /// down so it keeps pointing at the same sheet. + #[test] + fn remove_sheet_before_active_shifts_active_down() { + let mut wb = Workbook::new(); + wb.add_sheet("B"); + wb.add_sheet("C"); + wb.add_sheet("D"); // ["Sheet 1", "B", "C", "D"] + wb.set_active_sheet(2); // C + let removed = wb.remove_sheet(0); // remove "Sheet 1" (before C) + assert!(removed.is_some()); + assert_eq!(wb.sheet_count(), 3); + assert_eq!(wb.active_sheet(), 1); // shifted down, still C + assert_eq!(wb.active_sheet_name(), "C"); + } + + /// Removing the active sheet (or one after it) clamps the active index + /// to the new last sheet. + #[test] + fn remove_sheet_at_or_after_active_clamps_to_end() { + let mut wb = Workbook::new(); + wb.add_sheet("B"); + wb.add_sheet("C"); // ["Sheet 1", "B", "C"] + wb.set_active_sheet(2); // C (last) + let removed = wb.remove_sheet(1); // remove B; active 2 >= new len 2 + assert!(removed.is_some()); + assert_eq!(wb.sheet_count(), 2); + assert_eq!(wb.active_sheet(), 1); // clamped to the new last sheet + assert_eq!(wb.active_sheet_name(), "C"); + } + + /// `apply` forwards the remaining style commands to the active sheet: + /// italic, underline, number format, and background. + #[test] + fn apply_covers_italic_underline_number_format_and_background() { + let mut wb = Workbook::new(); + wb.apply_batch([ + WorkbookCommand::ToggleItalic { row: 0, col: 0 }, + WorkbookCommand::ToggleUnderline { row: 0, col: 0 }, + WorkbookCommand::SetNumberFormat { + row: 0, + col: 0, + format: NumberFormat::Currency, + }, + WorkbookCommand::SetBackground { + row: 0, + col: 0, + color: Some(Color::new(1.0, 0.0, 0.0, 1.0)), + }, + ]); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert!(cell.style.italic); + assert!(cell.style.underline); + assert_eq!(cell.style.number_format, NumberFormat::Currency); + assert_eq!(cell.style.bg_color, Some(Color::new(1.0, 0.0, 0.0, 1.0))); + } + + /// `apply`'s SetBorders arm: a per-edge target writes each requested + /// edge, and `BorderTarget::None` clears all four. + #[test] + fn apply_set_borders_covers_clear_and_per_edge() { + let mut wb = Workbook::new(); + let edge = BorderEdge { + color: Color::new(0.0, 0.0, 1.0, 1.0), + width: 2.0, + }; + wb.apply(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::All, + edge, + top: true, + bottom: true, + left: true, + right: true, + }); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert_eq!(cell.style.border_top, Some(edge)); + assert_eq!(cell.style.border_bottom, Some(edge)); + assert_eq!(cell.style.border_left, Some(edge)); + assert_eq!(cell.style.border_right, Some(edge)); + + wb.apply(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::None, + edge, + top: false, + bottom: false, + left: false, + right: false, + }); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert_eq!(cell.style.border_top, None); + assert_eq!(cell.style.border_bottom, None); + assert_eq!(cell.style.border_left, None); + assert_eq!(cell.style.border_right, None); + } + + /// `Workbook::default()` is a fresh single-sheet workbook. + #[test] + fn workbook_default_is_a_fresh_single_sheet() { + let wb = Workbook::default(); + assert_eq!(wb.sheet_count(), 1); + assert_eq!(wb.active_sheet_name(), "Sheet 1"); + } + + /// `save()` / `load()` round-trip through the default path, a legacy + /// single-sheet payload still loads, and a non-workbook payload yields + /// `None`. + /// + /// Runs single-threaded against the shared default filenames via + /// `crate::test_disk::lock()`, and saves/restores whatever was there + /// before — the same convention as the persistence module's disk tests. + #[test] + fn workbook_save_load_legacy_and_reject_via_default_path() { + let _disk = crate::test_disk::lock(); + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("generated"); + let current = dir.join("current.workbook.tsv"); + let saved_current = std::fs::read_to_string(¤t).ok(); + + // Round-trip through the default path. + let mut wb = Workbook::new(); + wb.set_cell(0, 0, "disk"); + wb.set_cell(0, 1, "=A1&\"!\""); + wb.save().expect("save failed"); + let loaded = Workbook::load().expect("load failed"); + assert_eq!(loaded.sheet_count(), 1); + assert_eq!(loaded.active_sheet(), 0); + assert_eq!(loaded.get_raw(0, 0), "disk"); + assert_eq!(loaded.get_display_value(0, 1), "disk!"); + + // A legacy single-sheet payload still loads as a one-sheet workbook. + let mut legacy_data = SpreadsheetData::default(); + legacy_data.set_cell(0, 0, "legacy"); + std::fs::write(¤t, legacy_data.serialize()).expect("write legacy"); + let migrated = Workbook::load().expect("legacy load failed"); + assert_eq!(migrated.sheet_count(), 1); + assert_eq!(migrated.get_raw(0, 0), "legacy"); + + // A non-workbook payload yields None. + std::fs::write(¤t, "not a workbook\n").expect("write garbage"); + assert!(Workbook::load().is_none()); + + // Restore whatever was there before. + match saved_current { + Some(text) => { + std::fs::write(¤t, text).ok(); + } + None => { + std::fs::remove_file(¤t).ok(); + } + } + } }