From 8325805e22d6ce16765f8142963eb9527175c186 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 04:42:22 +0000 Subject: [PATCH 1/2] test(spreadsheet): measure every test binary, and cover what that exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage report was reading one object file. Cargo builds each integration test into its own executable, so measuring only the lib-test binary discarded everything `tests/` exercised. That is not a rounding error. `persistence.rs` reported 41.77% with 14 of its 17 functions apparently never called, while `tests/sync_flow.rs` was calling `save_spreadsheet_state` and `load_saved_spreadsheet_state` on every run and passing. The functions were covered; the report was reading the wrong object. Fixing it alone moved persistence.rs to 72.15% and the engine total 90.09% -> 90.57% without a single new test. This is the third defect of its kind in this script — the ignore regex that excluded the sources being measured, the awk matcher that never fired, and now the single-object report. All three had the same signature: a confident number that was measuring less than it claimed. `--all-targets` for the UI crate too, so a future `tests/` file is measured the day it is added rather than silently skipped. Doing that immediately surfaced `spreadsheet-ui/tests/ui.rs`, which had **never compiled**: the crate did not enable `makepad-widgets`' `test` feature, so `makepad_widgets::makepad_test` did not resolve. `cargo test --lib` never built it and nothing reported the breakage. The manifest now enables the feature, matching `pdf-makepad`, and the two tests are `#[ignore]`d with the same documented reason as `pdf-makepad`'s — the fork has no headless Linux backend. Compiled on every run, so they cannot rot further while appearing to be coverage. Then the branches the corrected report named: - `undo.rs` 86.11% -> 98.34%. Resize undo/redo, both directions. The `None` arms are the substance: a column with no width override must have its key *removed* on undo, not have a default written into it. Writing a default looks identical until the default changes, at which point every previously-resized-then-undone column stops following it. - `persistence.rs` -> 93.70%. The legacy `current.sheet.csv` fallback, including that a whitespace-only current file must not shadow a real legacy one; `save_spreadsheet_state_as` writing where it says it does; and a rejected filename writing nothing at all. - `style.rs` 92.19% -> 100%. Format and alignment codes round-trip, and the codes are distinct — a shared code passes a round-trip test while making two formats indistinguishable on disk. - `model.rs` 84.87% -> 90.99%. Undo/redo intents, from_parts/into_parts, the active-sheet accessors agreeing with each other, and the disk round trip (`#[ignore]`d: it writes the shared generated/ file). Verified by mutation, seven injected defects, each confirmed red: undo None-arm writes a default 6 fail two number formats share a code 1 fail save_as ignores its validation 1 fail legacy fallback removed 2 fail Undo intent wired to redo() 1 fail from_parts drops the active index 1 fail active_sheet_data_mut hits sheet 0 1 fail Two of those changed the tests rather than merely passing: - `save_as ignores its validation` really does write `../escape.tsv` into the crate root, and the file survives the failing run — so every later run failed on the previous run's debris rather than on the current code. The test now removes any leftover before asserting. - `undo_and_redo_intents_reach_the_workbook` failed on first run because `apply()` does not call `begin_recording` and `apply_batch()` does, so there was nothing to undo. That asymmetry is the trap pinned by the engine's `only_set_cell_records_its_own_undo_step`; it now has a test on the model side too, since a caller reaching for `apply` and then offering an undo button gets a button that does nothing. Also fixed a pre-existing clippy **error** in `util.rs` — `approx_constant` on a literal `3.14` in a test that has nothing to do with PI. Confirmed pre-existing by reproducing on a stashed tree. It denies the whole crate, so no clippy gate could be added while it stood. Engine 269 -> 280 tests, 90.09% -> 91.58%; floor 90 -> 91. UI 17 -> 23 tests, 94.55% -> 95.70%; floor 94 -> 95. --- .../spreadsheet-engine/src/persistence.rs | 102 +++++++++++ .../spreadsheet-engine/src/style.rs | 74 ++++++++ .../spreadsheet-engine/src/undo.rs | 134 +++++++++++++++ .../spreadsheet-engine/src/util.rs | 7 +- .../spreadsheet/spreadsheet-ui/Cargo.toml | 7 +- .../spreadsheet/spreadsheet-ui/src/model.rs | 158 ++++++++++++++++++ .../spreadsheet/spreadsheet-ui/tests/ui.rs | 27 +++ tools/test-spreadsheet-coverage.sh | 85 +++++++--- 8 files changed, 568 insertions(+), 26 deletions(-) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs index 6b18dae..a2ed84e 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs @@ -141,4 +141,106 @@ mod tests { assert!(validate_filename("backup-2024-01.tsv").is_ok()); assert!(validate_filename("Book 1.tsv").is_ok()); } + + /// `save_spreadsheet_state_as` writes where it says it does. + /// + /// The validation above is tested in isolation, but nothing proved the + /// validated name is the name actually used — a function that + /// validates and then writes somewhere else would pass every test in + /// this module. So this reads the file back from the path it names. + /// + /// The filename is unique per test to stay safe under `cargo test`'s + /// default parallelism; these tests share one generated directory. + #[test] + fn save_as_writes_the_file_it_names() { + let name = "coverage-save-as.tsv"; + save_spreadsheet_state_as(name, "SHEET\tone\n").expect("save_as failed"); + + let path = sheet_generated_dir_path().join(name); + let written = fs::read_to_string(&path).expect("the named file must exist"); + assert_eq!(written, "SHEET\tone\n"); + + fs::remove_file(&path).ok(); + } + + /// A rejected filename must write **nothing**. Validating and then + /// writing anyway would be worse than not validating at all, because + /// the error return would say the write did not happen. + #[test] + fn a_rejected_filename_writes_nothing() { + let escape = "../coverage-escape.tsv"; + let outside = sheet_generated_dir_path().join(escape); + + // Remove any leftover before asserting. Without this the test is + // not idempotent: verifying it by mutation — making the function + // ignore its own validation — really does write the escape file, + // and every later run then fails on the *previous* run's debris + // rather than on the current code. Verified by doing exactly that. + fs::remove_file(&outside).ok(); + + assert!(save_spreadsheet_state_as(escape, "should never land").is_err()); + + let landed = outside.exists(); + fs::remove_file(&outside).ok(); + assert!( + !landed, + "a rejected filename still wrote a file at {}", + outside.display() + ); + } + + /// The legacy `current.sheet.csv` is read when the current-format file + /// is absent — the whole point of keeping the fallback. + /// + /// This runs single-threaded against the shared default filenames, so + /// it saves and restores whatever was already there rather than + /// clobbering another test's fixture. + #[test] + fn the_legacy_file_is_read_when_the_current_one_is_missing() { + let current = sheet_generated_file_path(); + let legacy = sheet_generated_legacy_file_path(); + let saved_current = fs::read_to_string(¤t).ok(); + let saved_legacy = fs::read_to_string(&legacy).ok(); + + fs::create_dir_all(sheet_generated_dir_path()).expect("generated dir"); + fs::remove_file(¤t).ok(); + fs::write(&legacy, "LEGACY CONTENT\n").expect("write legacy"); + + assert_eq!( + load_saved_spreadsheet_state().as_deref(), + Some("LEGACY CONTENT\n"), + "with no current file the legacy one must be read" + ); + + // And the current format wins when both exist. + fs::write(¤t, "CURRENT CONTENT\n").expect("write current"); + assert_eq!( + load_saved_spreadsheet_state().as_deref(), + Some("CURRENT CONTENT\n"), + "the current format must take precedence over the legacy one" + ); + + // An empty current file must fall through to the legacy one rather + // than being returned as an empty workbook. + fs::write(¤t, " \n").expect("write blank current"); + assert_eq!( + load_saved_spreadsheet_state().as_deref(), + Some("LEGACY CONTENT\n"), + "a whitespace-only current file must not shadow a real legacy one" + ); + + fs::remove_file(¤t).ok(); + fs::remove_file(&legacy).ok(); + match saved_current { + Some(text) => { + fs::write(¤t, text).ok(); + } + None => { + fs::remove_file(¤t).ok(); + } + } + if let Some(text) = saved_legacy { + fs::write(&legacy, text).ok(); + } + } } diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/style.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/style.rs index e2efb5c..5fa2b42 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/style.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/style.rs @@ -166,3 +166,77 @@ pub fn color_from_str(s: &str) -> Option { let a: f32 = it.next()?.parse().ok()?; Some(Color::new(r, g, b, a)) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Every format's code must round-trip through `from_code`. + /// + /// A table-driven test rather than six assertions, because the failure + /// mode being guarded against is a *copy-paste* one: two variants + /// sharing a code string, or `from_code` mapping one to another. Both + /// survive a spot check of the one variant someone happened to test, + /// and both silently rewrite a user's cell formatting on the next save + /// and load. + #[test] + fn every_number_format_code_round_trips() { + let all = [ + NumberFormat::General, + NumberFormat::Currency, + NumberFormat::Percent, + NumberFormat::Decimal1, + NumberFormat::Decimal2, + NumberFormat::Integer, + ]; + for format in all { + assert_eq!( + NumberFormat::from_code(format.code()), + format, + "{format:?} did not survive a code round trip" + ); + } + + // And the codes must all be distinct, or the round trip above + // passes while two formats are indistinguishable on disk. + let mut codes: Vec<&str> = all.iter().map(|f| f.code()).collect(); + codes.sort_unstable(); + let count = codes.len(); + codes.dedup(); + assert_eq!(codes.len(), count, "two number formats share a code"); + } + + #[test] + fn every_alignment_code_round_trips() { + let all = [CellAlign::Left, CellAlign::Center, CellAlign::Right]; + for align in all { + assert_eq!( + CellAlign::from_code(align.code()), + align, + "{align:?} did not survive a code round trip" + ); + } + let mut codes: Vec<&str> = all.iter().map(|a| a.code()).collect(); + codes.sort_unstable(); + let count = codes.len(); + codes.dedup(); + assert_eq!(codes.len(), count, "two alignments share a code"); + } + + /// An unknown code must fall back to the default rather than panic. + /// These strings come off disk, so they are attacker-influenced in + /// exactly the way a corrupt or hand-edited workbook is. + #[test] + fn unknown_codes_fall_back_to_the_default() { + for junk in ["", "ZZZ", "gen", " CUR", "🙂", "L "] { + assert_eq!(NumberFormat::from_code(junk), NumberFormat::General); + assert_eq!(CellAlign::from_code(junk), CellAlign::Left); + } + } + + #[test] + fn the_defaults_are_the_ones_the_codes_fall_back_to() { + assert_eq!(NumberFormat::default(), NumberFormat::General); + assert_eq!(CellAlign::default(), CellAlign::Left); + } +} diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/undo.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/undo.rs index f761f1b..44bf869 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/undo.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/undo.rs @@ -193,6 +193,140 @@ mod tests { // `SpreadsheetData` is already in scope via `use crate::*` // (undo.rs imports `use crate::data::{CellData, CellId, SpreadsheetData};`). + /// Undo and redo of a resize must restore *and* re-apply, in both the + /// "there was an override" and "there was no override" directions. + /// + /// The `None` arms are the interesting half: a column that was using + /// the default width has no entry in `col_widths` at all, so undoing a + /// resize has to **remove** the key rather than write a default into + /// it. Writing a default looks identical until someone changes what the + /// default is, at which point every previously-resized-then-undone + /// column silently stops following it. + #[test] + fn undoing_a_column_resize_restores_the_previous_state() { + let mut data = SpreadsheetData::default(); + + // No override before: undo must leave no override. + let fresh = Change::ResizeColumn { + col: 3, + old: None, + new: Some(140.0), + }; + fresh.apply_redo(&mut data); + assert_eq!(data.col_widths.get(&3), Some(&140.0)); + fresh.apply_undo(&mut data); + assert!( + !data.col_widths.contains_key(&3), + "undo must remove the override, not store a default in it" + ); + + // An override before: undo must put the old value back. + data.col_widths.insert(3, 80.0); + let changed = Change::ResizeColumn { + col: 3, + old: Some(80.0), + new: Some(200.0), + }; + changed.apply_redo(&mut data); + assert_eq!(data.col_widths.get(&3), Some(&200.0)); + changed.apply_undo(&mut data); + assert_eq!(data.col_widths.get(&3), Some(&80.0)); + } + + #[test] + fn undoing_a_row_resize_restores_the_previous_state() { + let mut data = SpreadsheetData::default(); + + let fresh = Change::ResizeRow { + row: 7, + old: None, + new: Some(44.0), + }; + fresh.apply_redo(&mut data); + assert_eq!(data.row_heights.get(&7), Some(&44.0)); + fresh.apply_undo(&mut data); + assert!( + !data.row_heights.contains_key(&7), + "undo must remove the override, not store a default in it" + ); + + data.row_heights.insert(7, 20.0); + let changed = Change::ResizeRow { + row: 7, + old: Some(20.0), + new: Some(60.0), + }; + changed.apply_redo(&mut data); + assert_eq!(data.row_heights.get(&7), Some(&60.0)); + changed.apply_undo(&mut data); + assert_eq!(data.row_heights.get(&7), Some(&20.0)); + } + + /// Redoing a resize whose `new` is `None` — the shape produced by + /// resetting a column to its default — must clear the override. + #[test] + fn redoing_a_reset_to_default_clears_the_override() { + let mut data = SpreadsheetData::default(); + data.col_widths.insert(1, 300.0); + data.row_heights.insert(2, 90.0); + + Change::ResizeColumn { + col: 1, + old: Some(300.0), + new: None, + } + .apply_redo(&mut data); + assert!(!data.col_widths.contains_key(&1)); + + Change::ResizeRow { + row: 2, + old: Some(90.0), + new: None, + } + .apply_redo(&mut data); + assert!(!data.row_heights.contains_key(&2)); + } + + /// A whole `ChangeSet` round-trips: undo then redo lands back exactly + /// where it started, across all three change kinds at once. + #[test] + fn a_mixed_changeset_round_trips_through_undo_and_redo() { + let mut data = SpreadsheetData::default(); + data.set_cell(0, 0, "before"); + + let old = data.cells.get(&CellId::new(0, 0)).cloned(); + data.set_cell(0, 0, "after"); + let new = data.cells.get(&CellId::new(0, 0)).cloned(); + + let mut set = ChangeSet::default(); + set.push(Change::SetCell { + row: 0, + col: 0, + old, + new, + }); + set.push(Change::ResizeColumn { + col: 5, + old: None, + new: Some(111.0), + }); + set.push(Change::ResizeRow { + row: 6, + old: None, + new: Some(33.0), + }); + + set.apply_redo(&mut data); + assert_eq!(data.get_raw(0, 0), "after"); + assert_eq!(data.col_widths.get(&5), Some(&111.0)); + assert_eq!(data.row_heights.get(&6), Some(&33.0)); + + set.apply_undo(&mut data); + assert_eq!(data.get_raw(0, 0), "before"); + assert!(!data.col_widths.contains_key(&5)); + assert!(!data.row_heights.contains_key(&6)); + } + #[test] fn empty_changeset_is_empty() { assert!(ChangeSet::default().is_empty()); diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs index 710e75b..044cf5b 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs @@ -268,7 +268,12 @@ mod tests { #[test] fn format_number_floats() { - assert_eq!(format_number(3.14), "3.14"); + // 3.15 rather than 3.14: clippy denies `approx_constant` because + // 3.14 is an approximation of PI, and the lint fires even in a test + // where the value is just a two-decimal number. Nothing about this + // assertion needs PI, so the number changes rather than the lint + // being silenced. + assert_eq!(format_number(3.15), "3.15"); assert_eq!(format_number(0.5), "0.5"); assert_eq!(format_number(-2.5), "-2.5"); assert_eq!(format_number(1.0 / 3.0), "0.333333"); // 6 decimals diff --git a/crates/apps/spreadsheet/spreadsheet-ui/Cargo.toml b/crates/apps/spreadsheet/spreadsheet-ui/Cargo.toml index eae46c4..d13c1d4 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/Cargo.toml +++ b/crates/apps/spreadsheet/spreadsheet-ui/Cargo.toml @@ -16,7 +16,12 @@ description = "Makepad widget wrappers for the spreadsheet engine." # # For now, using a relative path from this crate's location: # crates/apps/spreadsheet/spreadsheet-ui/ → ../../../../makepad/widgets -makepad-widgets = { git = "https://gitdab.com/andodeki/makepad", rev = "ecf5a572ab62a1c1598909971f602f99083671cc"} +# The `test` feature gates makepad-widgets' re-export of makepad-test, which +# `tests/ui.rs` imports as `makepad_widgets::makepad_test`. Without it that +# file does not compile at all — it was checked in against a manifest that +# never enabled the feature, so `cargo test --all-targets` failed outright +# and `cargo test --lib` skipped it silently. Matches `pdf-makepad`. +makepad-widgets = { git = "https://gitdab.com/andodeki/makepad", rev = "ecf5a572ab62a1c1598909971f602f99083671cc", features = ["test"] } spreadsheet-engine = { path = "../spreadsheet-engine" } [dev-dependencies] diff --git a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs index ec341a3..d141c3a 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs @@ -221,4 +221,162 @@ mod tests { }) ); } + + /// `dispatch_input` must actually undo and redo, not merely return + /// `true`. The model is a thin adapter, and a thin adapter wired to the + /// wrong method still type-checks: `Undo => self.workbook.redo()` is a + /// single-word mistake that no signature catches. So the cell value is + /// asserted at each step rather than the boolean. + #[test] + fn undo_and_redo_intents_reach_the_workbook() { + let mut model = WorkspaceModel::new(Workbook::new()); + // `apply_batch`, not `apply`: only the batch entry point calls + // `begin_recording`, so a bare `apply` leaves nothing on the undo + // stack and `dispatch_input(Undo)` correctly returns false. That + // asymmetry is the trap pinned by the engine's + // `only_set_cell_records_its_own_undo_step`, and this test walked + // straight into it on first run. + model.apply_batch([spreadsheet_engine::WorkbookCommand::SetCell { + row: 0, + col: 0, + value: "typed".into(), + }]); + assert_eq!(model.workbook().get_raw(0, 0), "typed"); + + assert!(model.dispatch_input(crate::input::InputIntent::Undo)); + assert_eq!( + model.workbook().get_raw(0, 0), + "", + "the Undo intent must undo the edit" + ); + + assert!(model.dispatch_input(crate::input::InputIntent::Redo)); + assert_eq!( + model.workbook().get_raw(0, 0), + "typed", + "the Redo intent must put it back" + ); + } + + /// With nothing left to undo, the intent must report `false` rather + /// than claiming it did something. + #[test] + fn undo_and_redo_report_false_when_there_is_nothing_to_do() { + let mut model = WorkspaceModel::new(Workbook::new()); + assert!(!model.dispatch_input(crate::input::InputIntent::Undo)); + assert!(!model.dispatch_input(crate::input::InputIntent::Redo)); + } + + /// The recording asymmetry, stated from the model's side. + /// + /// A bare `apply` does not open an undo step, so the Undo intent has + /// nothing to pop. This is surprising enough that it is worth an + /// explicit test at this layer too: a caller reaching for + /// `model.apply(..)` and then offering the user an undo button gets a + /// button that does nothing, with no error anywhere. + #[test] + fn a_bare_apply_leaves_nothing_for_the_undo_intent() { + let mut model = WorkspaceModel::new(Workbook::new()); + model.apply(spreadsheet_engine::WorkbookCommand::SetCell { + row: 0, + col: 0, + value: "unrecorded".into(), + }); + assert_eq!(model.workbook().get_raw(0, 0), "unrecorded"); + assert!( + !model.dispatch_input(crate::input::InputIntent::Undo), + "apply() does not call begin_recording, so there is no step to \ + undo; use apply_batch() when the edit must be undoable" + ); + } + + /// `from_parts` and `into_parts` must be inverses. They are the seam + /// the widget uses to hand its sheets to the model and take them back, + /// so a mismatch in the active index silently switches the user's + /// visible sheet on every round trip. + #[test] + fn from_parts_and_into_parts_round_trip() { + let model = WorkspaceModel::from_parts( + vec![ + Sheet::empty("Alpha"), + Sheet::empty("Beta"), + Sheet::empty("Gamma"), + ], + 2, + ); + assert_eq!(model.active_sheet(), 2); + assert_eq!(model.active_sheet_name(), "Gamma"); + + let (sheets, active) = model.into_parts(); + assert_eq!(active, 2, "the active index must survive the round trip"); + assert_eq!( + sheets.iter().map(|s| s.name.as_str()).collect::>(), + vec!["Alpha", "Beta", "Gamma"] + ); + } + + #[test] + fn into_workbook_hands_back_the_same_content() { + let mut model = WorkspaceModel::new(Workbook::new()); + model.apply(spreadsheet_engine::WorkbookCommand::SetCell { + row: 1, + col: 1, + value: "kept".into(), + }); + let workbook = model.into_workbook(); + assert_eq!(workbook.get_raw(1, 1), "kept"); + } + + /// The read-only and mutable accessors must address the *same* sheet. + /// Two accessors that disagree about which sheet is active is the + /// desynchronisation the whole model exists to prevent. + #[test] + fn the_active_sheet_accessors_agree_with_each_other() { + let mut model = WorkspaceModel::new(Workbook::with_sheets_active( + vec![Sheet::empty("One"), Sheet::empty("Two")], + 1, + )); + model.active_sheet_data_mut().set_cell(3, 4, "written"); + assert_eq!( + model.active_sheet_data().get_raw(3, 4), + "written", + "the immutable accessor must see the mutable one's write" + ); + // And it must be sheet 1, not sheet 0. + assert_eq!(model.sheet_data_mut(1).unwrap().get_raw(3, 4), "written"); + assert_eq!(model.sheet_data_mut(0).unwrap().get_raw(3, 4), ""); + assert!(model.sheet_data_mut(99).is_none()); + } + + /// `save` writes and `load_saved` reads it back. + /// + /// These are the two functions that touch the user's actual file, and + /// they were the only wholly uncovered ones left in this module. The + /// test is `#[ignore]`d by default: it writes to the engine crate's + /// shared `generated/` directory, so running it concurrently with the + /// engine's own persistence tests makes both flaky for reasons that + /// have nothing to do with the code. Run it deliberately: + /// + /// ```bash + /// cargo test -p spreadsheet-ui --lib -- --ignored --test-threads=1 + /// ``` + #[test] + #[ignore = "writes the shared generated/ workbook file; run single-threaded"] + fn save_and_load_round_trip_through_disk() { + let mut model = WorkspaceModel::new(Workbook::new()); + model.add_sheet("Persisted"); + model.apply(spreadsheet_engine::WorkbookCommand::SetCell { + row: 0, + col: 0, + value: "on disk".into(), + }); + + model.save().expect("save must succeed"); + let loaded = WorkspaceModel::load_saved().expect("load must find the file"); + assert_eq!(loaded.workbook().get_raw(0, 0), "on disk"); + assert!( + loaded.sheet_names().any(|n| n == "Persisted"), + "the sheet added before saving must come back" + ); + } } diff --git a/crates/apps/spreadsheet/spreadsheet-ui/tests/ui.rs b/crates/apps/spreadsheet/spreadsheet-ui/tests/ui.rs index 466fd0a..637946e 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/tests/ui.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/tests/ui.rs @@ -1,3 +1,28 @@ +//! UI tests driving the spreadsheet workspace through Makepad event +//! delivery. +//! +//! **Currently `#[ignore]`d, for the same reason as `pdf-makepad`'s.** The +//! fork's `makepad_test` sets `MAKEPAD=headless` for the app it spawns, but +//! `platform/src/os/linux/windowing_backend.rs` only knows `X11` and +//! `Wayland` — there is no headless backend, so the app selects X11, finds +//! no display and dies. The full diagnosis is in +//! `REVIEWS/PDF_PARITY_PHASE1_STATUS.md`. +//! +//! Until this file was reached by `--all-targets` it had **never compiled**: +//! `spreadsheet-ui` did not enable `makepad-widgets`' `test` feature, so the +//! `makepad_widgets::makepad_test` import failed. `cargo test --lib` never +//! built it and nothing reported the breakage. The manifest now enables the +//! feature, matching `pdf-makepad`, so the file is compiled on every run and +//! cannot rot further while appearing to be coverage. +//! +//! Ignored rather than deleted or left failing on purpose: a red suite +//! everyone knows to disregard stops reporting the next real regression. +//! Remove the markers when the fork grows a headless backend. +//! +//! ```bash +//! cargo test -p spreadsheet-ui --test ui -- --ignored --test-threads=1 +//! ``` + use makepad_widgets::makepad_test::{run_with_config, Selector, TestApp, TestConfig}; fn run_ui_test(test_name: &str, body: impl FnOnce(TestApp)) { @@ -36,6 +61,7 @@ fn run_ui_test(test_name: &str, body: impl FnOnce(TestApp)) { } #[test] +#[ignore = "needs a Makepad headless backend; see REVIEWS/PDF_PARITY_PHASE1_STATUS.md"] fn spreadsheet_workspace_smoke() { run_ui_test("spreadsheet_workspace_smoke", |app: TestApp| { app.locator(Selector::id("formula_input")).wait_visible(); @@ -45,6 +71,7 @@ fn spreadsheet_workspace_smoke() { } #[test] +#[ignore = "needs a Makepad headless backend; see REVIEWS/PDF_PARITY_PHASE1_STATUS.md"] fn spreadsheet_workspace_can_add_sheet() { run_ui_test("spreadsheet_workspace_can_add_sheet", |app: TestApp| { app.locator(Selector::id("add_sheet_btn")) diff --git a/tools/test-spreadsheet-coverage.sh b/tools/test-spreadsheet-coverage.sh index d5068c6..bfcd282 100755 --- a/tools/test-spreadsheet-coverage.sh +++ b/tools/test-spreadsheet-coverage.sh @@ -22,8 +22,8 @@ IFS=$'\n\t' ROOT="$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" COVERAGE_TARGET="${COVERAGE_TARGET:-all}" KEEP_COVERAGE="${KEEP_COVERAGE:-0}" -ENGINE_FLOOR="${ENGINE_FLOOR:-90}" -UI_FLOOR="${UI_FLOOR:-94}" +ENGINE_FLOOR="${ENGINE_FLOOR:-91}" +UI_FLOOR="${UI_FLOOR:-95}" case "$COVERAGE_TARGET" in all | engine | ui) ;; @@ -116,20 +116,49 @@ IGNORE_COMMON='(/cargo/registry|/cargo/git|/rustc/)' # grid.rs, ui.rs and workspace.rs are the three files carrying `script_mod!`. IGNORE_UI="$IGNORE_COMMON"'|spreadsheet-ui/src/(grid|ui|workspace)\.rs|spreadsheet-ui/src/bin/' +# Collect every instrumented test binary as `-object` arguments. +# +# One binary is not enough, and the shortfall is silent. Cargo builds each +# integration test into its *own* executable, so measuring only the lib-test +# binary discards everything `tests/` exercised. That is not a rounding +# error: `persistence.rs` reported 41.77% with 14 of its 17 functions +# apparently never called, while `tests/sync_flow.rs` was calling +# `save_spreadsheet_state` and `load_saved_spreadsheet_state` on every run +# and passing. The functions were covered; the report was reading the wrong +# object. Same class of defect as the two already fixed in this script. +collect_objects() { + local pattern="$1" + OBJECTS=() + local bin first=1 + while IFS= read -r bin; do + # llvm-cov takes the first binary positionally and the rest through + # `-object`. Passing every one as `-object` makes it read the first + # *source* path as the positional binary and fail with "not a valid + # object file", so the split matters. + if (( first )); then + OBJECTS+=("$bin") + first=0 + else + OBJECTS+=(-object "$bin") + fi + done < <(find "$CARGO_TARGET_DIR/debug/deps" -maxdepth 1 -type f \ + -executable ! -name '*.d' ! -name '*.so' -name "$pattern" | sort) +} + report_for() { - local label="$1" test_bin="$2" profdata="$3" ignore="$4" floor="$5" - shift 5 + local label="$1" profdata="$2" ignore="$3" floor="$4" + shift 4 local sources=("$@") echo >&2 echo "── $label ─────────────────────────────────────────────" >&2 - "$LLVM_BIN/llvm-cov" report "$test_bin" \ + "$LLVM_BIN/llvm-cov" report "${OBJECTS[@]}" \ -instr-profile="$profdata" \ -ignore-filename-regex="$ignore" \ "${sources[@]}" local covered - covered="$("$LLVM_BIN/llvm-cov" export "$test_bin" \ + covered="$("$LLVM_BIN/llvm-cov" export "${OBJECTS[@]}" \ -instr-profile="$profdata" \ -ignore-filename-regex="$ignore" \ -summary-only "${sources[@]}" \ @@ -148,9 +177,9 @@ report_for() { } uncovered_listing() { - local label="$1" test_bin="$2" profdata="$3" ignore="$4" - shift 4 - "$LLVM_BIN/llvm-cov" show "$test_bin" \ + local label="$1" profdata="$2" ignore="$3" + shift 3 + "$LLVM_BIN/llvm-cov" show "${OBJECTS[@]}" \ -instr-profile="$profdata" \ -ignore-filename-regex="$ignore" \ "$@" \ @@ -175,18 +204,25 @@ if [[ "$COVERAGE_TARGET" == "all" || "$COVERAGE_TARGET" == "engine" ]]; then "$LLVM_BIN/llvm-profdata" merge -sparse "$WORK/profiles"/*.profraw \ -o "$WORK/engine.profdata" - ENGINE_BIN="$(find "$CARGO_TARGET_DIR/debug/deps" -maxdepth 1 -type f \ - -executable -name 'spreadsheet_engine-*' | head -1)" - if [[ -z "$ENGINE_BIN" ]]; then - echo "error: could not find the engine test binary" >&2 + # Every engine test binary: the lib tests plus each file in tests/. + collect_objects '*' + if [[ "${#OBJECTS[@]}" -eq 0 ]]; then + echo "error: no instrumented engine test binaries were produced" >&2 exit 1 fi - report_for "engine" "$ENGINE_BIN" "$WORK/engine.profdata" \ - "$IGNORE_COMMON" "$ENGINE_FLOOR" "$ENGINE/src" + # A directory is accepted as a source filter only with a single + # `-object`; with several, llvm-cov rejects it. Expand to the files. + ENGINE_SOURCES=() + while IFS= read -r f; do + ENGINE_SOURCES+=("$f") + done < <(find "$ENGINE/src" -name '*.rs' | sort) + + report_for "engine" "$WORK/engine.profdata" \ + "$IGNORE_COMMON" "$ENGINE_FLOOR" "${ENGINE_SOURCES[@]}" if [[ "$KEEP_COVERAGE" == "1" ]]; then - uncovered_listing "engine" "$ENGINE_BIN" "$WORK/engine.profdata" \ - "$IGNORE_COMMON" "$ENGINE/src" + uncovered_listing "engine" "$WORK/engine.profdata" \ + "$IGNORE_COMMON" "${ENGINE_SOURCES[@]}" fi fi @@ -203,25 +239,26 @@ if [[ "$COVERAGE_TARGET" == "all" || "$COVERAGE_TARGET" == "ui" ]]; then "$WORK/spreadsheet/spreadsheet-ui" UI="$WORK/spreadsheet/spreadsheet-ui" - cargo test --manifest-path "$UI/Cargo.toml" --lib + # `--all-targets` rather than `--lib`, so a future tests/ file is + # measured the day it is added instead of being silently ignored. + cargo test --manifest-path "$UI/Cargo.toml" --all-targets "$LLVM_BIN/llvm-profdata" merge -sparse "$WORK/profiles"/*.profraw \ -o "$WORK/ui.profdata" - UI_BIN="$(find "$CARGO_TARGET_DIR/debug/deps" -maxdepth 1 -type f \ - -executable -name 'spreadsheet_ui-*' | head -1)" - if [[ -z "$UI_BIN" ]]; then - echo "error: could not find the UI test binary" >&2 + collect_objects '*' + if [[ "${#OBJECTS[@]}" -eq 0 ]]; then + echo "error: no instrumented UI test binaries were produced" >&2 exit 1 fi # Only the controller modules are measured; see the exclusion note above. - report_for "ui-controllers" "$UI_BIN" "$WORK/ui.profdata" \ + report_for "ui-controllers" "$WORK/ui.profdata" \ "$IGNORE_UI" "$UI_FLOOR" \ "$UI/src/clipboard.rs" "$UI/src/edit.rs" "$UI/src/event_router.rs" \ "$UI/src/input.rs" "$UI/src/model.rs" "$UI/src/render_cache.rs" \ "$UI/src/selection.rs" "$UI/src/text_measure.rs" if [[ "$KEEP_COVERAGE" == "1" ]]; then - uncovered_listing "ui" "$UI_BIN" "$WORK/ui.profdata" "$IGNORE_UI" \ + uncovered_listing "ui" "$WORK/ui.profdata" "$IGNORE_UI" \ "$UI/src/clipboard.rs" "$UI/src/edit.rs" "$UI/src/event_router.rs" \ "$UI/src/input.rs" "$UI/src/model.rs" "$UI/src/render_cache.rs" \ "$UI/src/selection.rs" "$UI/src/text_measure.rs" From ac062ec3c0bd7cfde563bf465647cdf46106b77e Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 04:45:17 +0000 Subject: [PATCH 2/2] test(spreadsheet): raise the engine floor to 94, where the merge landed it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebasing onto a859053 and a88fb68 put the engine at 94.55%: their new data.rs dependency-graph tests, measured through the every-binary fix in the previous commit. A floor of 91 under a 94.55% measurement protects nothing — three and a half points of regression would pass silently. --- tools/test-spreadsheet-coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/test-spreadsheet-coverage.sh b/tools/test-spreadsheet-coverage.sh index bfcd282..1c0e086 100755 --- a/tools/test-spreadsheet-coverage.sh +++ b/tools/test-spreadsheet-coverage.sh @@ -22,7 +22,7 @@ IFS=$'\n\t' ROOT="$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" COVERAGE_TARGET="${COVERAGE_TARGET:-all}" KEEP_COVERAGE="${KEEP_COVERAGE:-0}" -ENGINE_FLOOR="${ENGINE_FLOOR:-91}" +ENGINE_FLOOR="${ENGINE_FLOOR:-94}" UI_FLOOR="${UI_FLOOR:-95}" case "$COVERAGE_TARGET" in