fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #209 from fiddlerwoaroof/fix/phg1-record-rewrite-tests

Browse Source 
Give RecordRewrite's three untested branches a test that can fail (improvise-phg1)
This commit is contained in:
Ed Langley
2026-09-22 23:38:51 -07:00
committed by GitHub
parent 7767cee9b6 7e1e8afba2
commit ea119bd9d1
2 changed files with 176 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/test-evidence/command.md
+56
View File
@@ -5518,3 +5518,59 @@ one test covers the crate's feature.
cannot see the feature removed from `improvise-command` alone, because
`improvise-io`, a normal dependency, enables it in the same build. That case is
held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`).
## `ui/effect.rs` — `RecordRewrite`'s untested branches (improvise-phg1)
### renaming_a_drill_row_held_by_a_lone_measureless_cell_moves_that_cell
improvise-phg1(a). A row seeded by `o` (`AddRecordRow`) carries its coordinates
on one cell with no `_Measure`, and `RecordRewrite::of` moves that cell in its
`(false, true)` arm. Replacing the arm with `Vec::new()` left the whole workspace
green, and a renamed `o` row then left `CellKey([])` behind with no coordinates.
The gap predates improvise-uo2p, because `rename_record_coordinate` had the same
arm and nothing tested it. The fixture is that literal case: an empty-coordinate
cell on drill row 1, renamed. Under the rival it fails with `-/-/- = 0` still in
the store, and nothing else in the workspace fails.
### a_measure_a_sparse_drill_row_lacks_is_written_at_the_renamed_record
improvise-phg1(b). An edit to a measure the row has no cell for goes through
`of`'s second loop, which must write at the **renamed** key. Every uo2p fixture
row carried every measure, so that loop only ever ran without a rename, and
writing `row.cell_key(measure)` unrenamed brought uo2p's ghost record back
unseen. Here drill row 1 has `Revenue` but not `Cost`, and stages `Cost` and a
rename. Under the rival it fails with a `Clothing/Jan/Cost = 999` ghost, and
nothing else in the workspace fails.
### every_drill_row_reads_the_model_before_any_row_clears_it
improvise-phg1(c). `RecordRewrite::merged` puts every clear after every read.
The rival lets each row's clears land before the next row reads the model, but
defers every write to the end. The uo2p swap test cannot see **that** rival: a
row's clears aimed at its own old keys touch nothing another row reads. A wider
rival, where each row's clears *and* writes land before the next row reads, is
a different fault: the swap test does catch it (uo2p's rival 2, which fails
`a_drill_that_swaps_two_records_coordinates_swaps_them` alone). So this test
exists for the narrow rival only. What separates it is a clear aimed at
**another row's** key. A blank edit on a measure the row lacks clears that
measure at the row's *renamed* coordinates. So row 0 is renamed onto row 1's
coordinates and blanks `Profit`, which only row 1 has, and row 1 is renamed
away. Row 1's `Profit` must still arrive at `Food/Jan`. Under the rival it is
cleared before row 1 reads it, `Food/Jan/Profit = 5` is lost, and nothing else
in the workspace fails. This probe is the reviewer's, from the ticket.
### a_blank_edit_on_a_measure_a_renamed_row_lacks_clears_it_at_the_renamed_record
improvise-q2mq. `RecordRewrite::of`'s second loop handles the measures a row has
no cell for. Its **clear** arm must aim at the *renamed* key, exactly as its
write arm must (improvise-phg1(b)). The phg1 sparse-row test stages a value, so
it covers only the write arm. Aiming the clear arm alone at
`row.cell_key(measure)`, unrenamed, left the whole workspace green.
The probe is the reviewer's. Row 0 (`Food/Jan`, `Revenue = 1`) is renamed onto
`Clothing`, where row 1 already holds `Profit = 5`, and blanks `Profit`, which
row 0 lacks. The blank belongs to row 0's final record at `Clothing/Jan`, so the
store is exactly `{Clothing/Jan/Revenue = 1}`. Under the rival the clear lands
on `Food/Jan/Profit`, where there is nothing to clear, and
`Clothing/Jan/Profit = 5` survives. Injected alone into a full workspace run,
the rival fails this test and nothing else.
crates/improvise-command/src/ui/effect.rs
+120 -20
View File
@@ -3143,26 +3143,9 @@ mod tests {
/// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2),
/// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least.
fn two_record_drill() -> App {
let mut app = test_app();
let cells = [
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Cost"),
CellValue::Number(17.0),
),
];
for (key, value) in cells.clone() {
app.model_state.workbook.model.set_cell(key, value);
}
let records = RecordRow::group(cells);
assert_eq!(records.len(), 2, "one row per coordinate tuple");
let app = drill_of(&two_record_drill_cells());
let records = &app.drill_state.as_ref().unwrap().records;
assert_eq!(records[1].coords.get("Type"), Some("Clothing"));
StartDrill(records).apply(&mut app);
app
}
@@ -3206,10 +3189,49 @@ mod tests {
/// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its
/// own `RandomState`, and one map iterates the same way twice.
fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) {
let cells = two_record_drill_cells();
assert_drill_over(&cells, edits, expected);
}
fn two_record_drill_cells() -> Vec<(CellKey, CellValue)> {
vec![
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Cost"),
CellValue::Number(17.0),
),
]
}
/// A drill over exactly `cells`, which must group into two records.
fn drill_of(cells: &[(CellKey, CellValue)]) -> App {
let mut app = test_app();
for (key, value) in cells {
app.model_state
.workbook
.model
.set_cell(key.clone(), value.clone());
}
let records = RecordRow::group(cells.iter().cloned());
assert_eq!(records.len(), 2, "one row per coordinate tuple");
StartDrill(records).apply(&mut app);
app
}
fn assert_drill_over(
cells: &[(CellKey, CellValue)],
edits: &[(usize, RecordCol, &str)],
expected: &[&str],
) {
let expected: std::collections::BTreeSet<String> =
expected.iter().map(|s| s.to_string()).collect();
for attempt in 0..32 {
let mut app = two_record_drill();
let mut app = drill_of(cells);
for (row, col, value) in edits {
stage(&mut app, *row, col.clone(), value);
}
@@ -3218,6 +3240,84 @@ mod tests {
}
}
#[test]
fn renaming_a_drill_row_held_by_a_lone_measureless_cell_moves_that_cell() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(CellKey::new(vec![]), CellValue::Number(0.0)),
],
&[(1, category("Type"), "Drink")],
&["Food/Jan/Revenue = 1", "Drink/-/- = 0"],
);
}
#[test]
fn a_measure_a_sparse_drill_row_lacks_is_written_at_the_renamed_record() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
],
&[(1, measure("Cost"), "999"), (1, category("Type"), "Drink")],
&[
"Food/Jan/Revenue = 1",
"Food/Jan/Cost = 2",
"Drink/Jan/Revenue = 42",
"Drink/Jan/Cost = 999",
],
);
}
#[test]
fn every_drill_row_reads_the_model_before_any_row_clears_it() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Profit"),
CellValue::Number(5.0),
),
],
&[
(0, category("Type"), "Clothing"),
(0, measure("Profit"), ""),
(1, category("Type"), "Food"),
],
&[
"Clothing/Jan/Revenue = 1",
"Food/Jan/Revenue = 42",
"Food/Jan/Profit = 5",
],
);
}
#[test]
fn a_blank_edit_on_a_measure_a_renamed_row_lacks_clears_it_at_the_renamed_record() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(
drill_cell("Clothing", "Jan", "Profit"),
CellValue::Number(5.0),
),
],
&[
(0, category("Type"), "Clothing"),
(0, measure("Profit"), ""),
],
&["Clothing/Jan/Revenue = 1"],
);
}
#[test]
fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() {
assert_drill_commits_to(