diff --git a/context/code/command.md b/context/code/command.md index ba7fd1e7..2178cad9 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -732,6 +732,24 @@ layout (gotcha 23), so a batch that **crosses** — `:toggle-records-mode . ente grid. One edit, after the rebuild, has no crossing case to get wrong, and it also removes the per-edit coverage question three separate writers created. +**The guard is two-armed and the crossing test could only see one arm** +(improvise-4dpj). `settle_normal_mode` matches `Normal | RecordsNormal`, and +replacing that with a one-way `Normal` left the whole workspace green: the `:` +crossing drives the layout both ways, but both of its branches end in +`enter-mode normal`, so the mode handed to the settle is `AppMode::Normal` +either way. It discriminates "settles at all" and "settles against the ending +layout", and not "settles from **either** mode" — three questions that look +like one until an injection separates them. The `RecordsNormal` arm is live: +`SetAxis { "_Index", Axis::None }` applied from a records grid leaves +`is_records_mode()` false with the mode still `RecordsNormal`. + +**And records-ness is auto-detected from the axes (gotcha 1), so a view switch +crosses it too.** `SwitchViewAtCursor` emits `[SwitchView(target), +ChangeMode(Normal)]`, which is a genuinely different path from the toggle the +`:` test drives — nothing in it says "records", the crossing falls out of the +target view's axis map. Moving the settle back before `rebuild_layout` is the +injection for both, and it fails each by name. + **This fixes the TUI and not the browser, and that is LIVE (improvise-e5pf).** `Command::ChangeMode(AppMode::Normal)` is on the wire and `reduce.rs` folds it as `view.mode = mode.clone()` with no resolution, so the browser front end still @@ -1323,6 +1341,54 @@ cells will bump their records when it shares a batch with a `CellData` one — a over-bump, costing a rejection that need not have happened, which is the same asymmetry `Dirty::of` is deliberately sloppy in. +**The case list is checked for completeness, not for length** (improvise-26dm). +`layout_geometry_effects_actually_change_document_config` walked a hand-built +list under an `assert_eq!(cases.len(), 13)`, and a count catches a case being +*deleted* and never one being *absent* — the two are written identically. The +adversarial review of PR #78 added a fourteenth `pub struct` declaring +`LayoutGeometry` and the suite stayed green. +`doctrine(a_list_checked_for_length_reads_like_a_list_checked_for_completeness)`. + +improvise-5ubs's answer — read serde's derive-generated variant list out of its +`unknown variant … expected one of` message — cannot reach this one: an effect +is a struct implementing a trait, so there is no derived enumeration of effects +at all. But gotcha 38 says `ModelCommand` is derived from the model effects, +with `model_effects` mapping each variant back to the effect it came from, so +the list is now diffed against the `LayoutGeometry` effects *reachable through +`ModelCommand`*, and the failure names the missing effect rather than a number. +The two lists agree exactly today, which is itself the evidence the +substitution lost nothing. + +**The right-hand side had to be bounded before it could be believed** (PR #96, +found independently by this branch's review and by ChatGPT Codex). +`every_model_command()`'s doc comment claimed the enumeration was +compiler-forced — `variant_name` matches exhaustively, so a new variant fails +the build. That inference is invalid: the build fails in *`variant_name`*, and +nothing obliged the value list to gain an entry. Deleting one from it failed +this check and nothing else in the workspace. + +So the list is now itself checked, by `improvise-5ubs`'s mechanism: +`the_enumeration_names_every_variant_the_derive_declares` diffs it against +`variants::declared::()`, serde's derive-generated variant list. +That makes the chain **derive → `every_model_command()` → `model_effects` → +case list**, every link checked, and the whole thing is what the ticket asked +for. `variants` was `#[cfg(test)] pub(crate)`, so reaching it costs a +`test-support` feature on `improvise-protocol` gating an optional `serde_json` +— **off in every shipped build**, which is why the wasm gate is unaffected. +The assertion message still names **both** directions, because a short +right-hand side reads as a spurious case and invites deleting the left, +degrading both sides together — the failure the ticket exists to close. + +**Its bound is stated rather than assumed**: this closes the effects on the +wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is +still invisible — which is precisely the probe variant PR #78 injected. That is +an honest residual and not a fix, and it is the reason the ticket asked for a +compiler-maintained enumeration of *effects*: none exists without a proc macro +or inventory-style registration, and a fourth hand-rolled list in a fourth +place is the disease. A second `[…; 13]` array survives one function away in +`every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint` and is +filed, not smuggled in. + On abort, the loop resets `abort_effects` so the next dispatch starts clean and records `batch_aborted` where a caller can still read it. diff --git a/context/code/tooling.md b/context/code/tooling.md index 1dec5bd9..02593116 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -208,6 +208,37 @@ also has no `context/test-evidence/` and no JS roots, so the evidence half is vacuously clean there and the cap is the only thing that can decide the exit code — the isolation is what makes the assertion name one question. +### Two headings, one test + +An orphan is a heading whose test nobody can find; the mirror image is a test +that **two** headings answer for, and it is invisible to every rule above — +each copy names a test that runs, so each copy passes. PR #92 shipped two +entries under one heading where the older described a fixture the test no +longer had and repeated a claim an adversarial review had measured false. A +reader meeting the older one first has no way to know a successor exists two +thousand lines down, so the stale copy reads as current. That is worse than an +orphan, which at least announces itself as broken. + +It is a **refusal and not a warning**, on the precedent `improvise-rm4s` set for +the ambiguous leaf: the two are the same defect seen from opposite ends — one +name answered by two tests, and one test answered by two names — so they are +answered the same way. The refusal names the file and every line, in file order. + +The key is the **resolved test, not the spelling**. `### f` beside +`### m::tests::f` is two headings for one test, and grouping by the string would +miss it precisely because qualification is the repair `rm4s` asks for; a heading +qualified in one file and bare in another would have been the first way this +came back. Resolution reuses the same leaf table the ambiguity rule uses, so a +leaf that is genuinely ambiguous canonicalises to itself and is reported as +ambiguous rather than as a duplicate — two *different* tests sharing a leaf are +not two entries for one test, and conflating them would file the wrong repair. +The check is over the whole directory rather than per file, because a heading +keys a test name and a test name is unique workspace-wide once qualified. + +What it still cannot see, and neither can any of the others: an entry that +exists, names a live test, is the only one that does — and describes it +backwards. + ## 3. The others `scripts/kb` is the only writer of `prolog-sessions/kb.pl`; its own reasoning is diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 445b548b..55d7a178 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1017,6 +1017,15 @@ Tier 2 moves, tier 3 does not. A shape change invalidates every in-flight `CellData` batch through `layout_version` alone; it must not also masquerade as a write to any record. +Fixture repaired under improvise-75ld: it asked for +`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where +`on_category_added` had already seated `Month` on Column — so the "layout change" +was a no-op, and the test passed only because the tier was bumped off the +declaration. It now asserts the axis before and after, and asks for `Axis::Row`. +This is the failure that turned "reclassify three effects" into "stop reading the +tier off the classification". Injected fault: the same as +`both_shape_counters_move_for_every_reshaping_command` — fails naming `SetAxis`. + ### a_loaded_document_is_dirty_because_its_path_still_names_the_old_one **A loaded document is dirty, and that is a behaviour change with a reason** @@ -3076,16 +3085,6 @@ that reshapes nothing fails loudly rather than proving nothing. Injected fault: as above. Fails naming `SetAxis`, then `AddItem` once the earlier cases are removed. -### a_layout_change_moves_the_layout_version_and_no_record - -Pre-existing test, fixture repaired under improvise-75ld. It asked for -`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where -`on_category_added` had already seated `Month` on Column — so the "layout change" -was a no-op, and the test passed only because the tier was bumped off the -declaration. It now asserts the axis before and after, and asks for `Axis::Row`. -This is the failure that turned "reclassify three effects" into "stop reading the -tier off the classification". - ### a_configuration_change_that_draws_nothing_is_still_a_document_change The third case the shape/config split creates, and the one that forced two @@ -3496,3 +3495,104 @@ The fix is not local and none is chosen: `grid_in` takes `&App` and `recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the view it is projecting. Candidates are on the ticket. + +### a_batch_that_leaves_records_settles_out_of_records_normal + +improvise-4dpj, gap 1. `settle_normal_mode` matches `Normal | RecordsNormal`, +and replacing that with a one-way `AppMode::Normal` left the **entire workspace +suite passing**. The existing crossing test, +`a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`, drives the +layout both ways, but both of its branches end in `enter-mode normal`, so the +mode handed to the settle is `AppMode::Normal` in both directions. It +discriminates "settles at all" and "settles against the ending layout", and not +"settles from **either** mode". + +This drives the production repro: from a records grid, +`apply_effects([SetAxis { "_Index", Axis::None }])` leaves +`layout.is_records_mode()` false with the mode still `RecordsNormal`, so the +`RecordsNormal` arm is live and not defensive. Both the records-ness of the +grid and the mode before the batch are asserted as preconditions, so a fixture +that stopped entering records mode fails loudly rather than proving nothing. + +Injected fault: `if matches!(self.view_state.mode, AppMode::Normal)`. Fails by +name, `left: RecordsNormal, right: Normal`. It is the only test in the +workspace that fails. + +### switching_views_across_records_ness_settles_against_the_view_switched_to + +improvise-4dpj, gap 2 — raised by ChatGPT Codex against PR #91 and verified +correct on `main` before being tested. `SwitchViewAtCursor` emits +`[SwitchView(target), ChangeMode(Normal)]`, and this is a **different path** +from the one gap 1's siblings drive: that test toggles records mode explicitly, +where a view switch crosses records-ness only because it is auto-detected from +the axes (gotcha 1). Nothing in the batch says "records". + +Driven in both directions, with the crossing asserted before and after so a +fixture that stopped crossing fails rather than passes. Injected fault: move +`settle_normal_mode()` back above `rebuild_layout()` at the end of +`apply_effects` — the exact regression PR #91's round 2 fixed. This test fails +by name; so does gap 1's, along with six pre-existing records-mode tests +(measured in review; an earlier draft of this entry said five). + +**Scope note, stated because an overclaim is worse than a gap**: this drives +the *effects* `SwitchViewAtCursor` emits, not the view-panel key path that +composes them. Reaching the panel needs a `view_panel_cursor` on a `CmdContext` +built in `command/cmd/panel.rs`, which is another wave's ground this round. The +command's own coverage lives there; what is unexercised is the two joined. + +### the_layout_geometry_cases_are_every_wire_effect_of_that_kind + +improvise-26dm. `layout_geometry_effects_actually_change_document_config` was +guarded by `assert_eq!(cases.len(), 13)`, which catches a case being *deleted* +from the list and never one being *absent* from it — and the two are written +identically as an `assert_eq` over a number. The adversarial review of PR #78 +added a fourteenth `pub struct` declaring `LayoutGeometry`, pointed at by +nothing, and the whole suite stayed green. + +improvise-5ubs's mechanism cannot reach it: an effect is a struct implementing +a trait, so serde has no derived enumeration to diff against. `model_effects` +maps each `ModelCommand` variant back to the effect it came from (gotcha 38), +so the list is diffed against the `LayoutGeometry` effects reachable through +`ModelCommand`. The two sets are equal today, which is the evidence the +substitution lost nothing. + +**The right-hand side is bounded by the derive, and that took a second round.** +Review of PR #96 injected the deletion of `ModelCommand::TogglePruneEmpty` from +`every_model_command()`: this was the **only** test in the workspace that +failed, and `every_model_command`'s doc comment claiming the list was +compiler-forced was wrong — `variant_name`'s exhaustive match forces an arm +*there*, not an entry in the list. ChatGPT Codex raised the same thing on the +PR. `the_enumeration_names_every_variant_the_derive_declares` closes it, so the +chain is derive → enumeration → `model_effects` → case list with every link +checked. The assertion here still names **both** directions, because a short +right-hand side reads as a spurious case on the left and invites deleting it — +degrading both sides together. + +Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old +count said `12 != 13`; this says +`right: [… "TogglePruneEmpty" …]` and **names the effect**. + +**Residual, stated rather than closed**: this is total over the wire and not +over the effects. An effect declaring `LayoutGeometry` that no `ModelCommand` +names is still invisible — which is precisely PR #78's probe. Closing that +needs an enumeration of effects the compiler maintains, and none exists without +a proc macro or inventory-style registration. + +### the_enumeration_names_every_variant_the_derive_declares + +The bound the entry above rests on. Injection is Codex's own, run end to end: a +`ModelCommand::ProbeGeometry` whose effect declares `LayoutGeometry`, with the +`model_effects` and `variant_name` arms the compiler forces, omitted from +**both** lists. Before this test, the whole workspace stayed green. After it, +the failure is staged and each stage names the probe — first +`every_model_command() never constructs ["ProbeGeometry"]`, then, once the +enumeration is repaired and only the case list is short, +`the_layout_geometry_cases_are_every_wire_effect_of_that_kind` with +`ProbeGeometry` on the right. That staging is the point: the two tests answer +different questions and the injection separates them. + +The name reader is shared, not copied: `reduce.rs`'s `effect_type_name` and its +own `the_effect_name_reader_reads_all_three_debug_shapes` are what this leans +on, because a second reader here would be a derivation done twice and could +drift while one copy was repaired. It reads the leading identifier off the +derived `Debug`, `Effect` being a trait object that `type_name` cannot reach. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index d1760560..223bdddd 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -192,3 +192,30 @@ reason for a file nobody can find — the same failure as a dead ticket id, whic The first arm was filed as latent and was live when it was written: `context/code/kb-tool.md` had been unpublished, unargued and unnoticed since it was added. + + +### Duplicate evidence headings — `SELF_TEST_DUPLICATES` + +improvise-hsoe. `--check-evidence` verified that a heading names a test that +**runs**, and a duplicate heading names one just as well, so both copies pass. +PR #92 shipped two entries under +`a_layout_change_moves_the_layout_version_and_no_record`, and two more pairs +sat in `tui.md` — one of them asserting that `THE_MEMBERSHIP_CORRECT_SPANS` was +"a live tripwire, do not remove it" some four hundred lines above the entry +recording that improvise-gd83 had removed it. A reader meeting the older copy +first takes it as current, which is worse than an orphan: an orphan announces +itself as broken. + +The fault is injected by the six cases in `SELF_TEST_DUPLICATES`, and three of +them are the ones that would not get written first. Two *different* tests +sharing a leaf must come back **ambiguous and not duplicate**, because the +wrong diagnosis files the wrong repair. A prose heading must be free to repeat, +because it claims no test. And the bare and module-qualified spellings of one +test must collide — grouping by the string rather than by the resolved test +would have missed exactly the case improvise-rm4s's qualification repair +creates, which is the first way this would have come back. + +The live tree was itself the first injection: before the three merges the check +exited 1 naming all three pairs with both line numbers; after them it exits 0. +The clean arms (two headings naming two tests, and prose repeated) are what +stop an always-refuse from satisfying the rest. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index a7e19714..81057857 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -87,22 +87,21 @@ Each header line is cut into columns and compared *exactly*. A `contains` would be one-directional: it cannot see a label the browser wrongly blanked (the filter would skip it) and it cannot see one painted under the wrong column. -**`THE_MEMBERSHIP_CORRECT_SPANS` is a deliberately-wrong assertion and a live -tripwire. Do not remove it.** The `assert_ne!` against it starts failing the day -improvise-gd83 is fixed, at which point the `assert_eq!` above it should become -that constant. +**improvise-gd83 is fixed and `THE_MEMBERSHIP_CORRECT_SPANS` is gone**, along +with the `assert_ne!` tripwire that named it. The value it predicted — +`[(Q1,2), (None,1), (Q1,2), (None,1)]` — is now what the test asserts, element +for element. `Sep` is ungrouped, so the run ends before it and resumes under +the second measure's own `Q1` header; a precondition asserts the fixture still +contains an ungrouped column, because without one the test cannot see a leaked +group at all. -The span assertion above it pins `[(Some("Q1"), 6)]` — Q1 covering all six -columns *including* the two under the ungrouped `Sep` — because -`expand_category` emits a header on entering a group and no boundary on leaving -one, so the nearest-preceding walk keeps answering Q1 (`AxisFrame::group_of`). -That is `GridLayout`'s rule and the TUI paints the same span; it is reproduced -rather than corrected, because correcting it changes what the TUI draws. - -The membership-correct answer is spelled out rather than guessed, because a -wrong future expectation is worse than none: whoever picks up improvise-gd83 -will read it and believe it. The frame here is **not** one Q1 run — `_Measure` -is the outer level, so the column axis is: +The superseded expectation — Q1 covering all six columns *including* the two +under the ungrouped `Sep`, because `expand_category` emitted a header on +entering a group and no boundary on leaving one, so the nearest-preceding walk +kept answering Q1 — is recorded here because it is what `AxisFrame::group_of` +answered before the fix, and the frame it was derived from still explains the +current value. The frame is **not** one Q1 run — `_Measure` is the outer level, +so the column axis is: col_cats = ["_Measure", "Month"] Group(Q1) [Revenue,Jan] [Revenue,Feb] [Revenue,Sep] @@ -114,6 +113,23 @@ rather than after both. So membership-correct spans are **four** runs, not two run between them. Hence `[(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)]`. +improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is +one-directional and passes while the TUI labels **every** column. The run rule +now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends +call it; the TUI's group line is compared cell for cell against spans derived +from the browser's own runs. + +Fault injected exactly as that ticket specifies — forcing the TUI to emit the +group label on every column (`head = col_groups.get(ci)`, dropping the run-head +lookup). Before: `cargo test -p improvise` stayed green. After: this test fails +with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against +`["▼ Q1","","","▼ Q1","",""]`. + +One thing the shared rule does **not** flatten: the TUI labels its first +*visible* column even mid-run, because a scrolled grid would otherwise draw an +unlabelled group line. That is a scrolling concession, kept explicitly, and it +is why the comparison is run at `col_offset == 0`. + ### neither_front_end_produces_a_slash_joined_label A separate assertion because the agreement tests above would also hold if @@ -165,6 +181,15 @@ so it stays as it is; the multi-level agreement is `tests/grid_frame_agreement.rs`, which drives both front ends over a two-level, grouped fixture. +Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the +leading `▼ Co…` header now draws above `East`, and everything below shifts +down one. Nothing else in the buffer changed. That the group line, the column +headers, the separator, the data rows and the totals strip are byte-identical +is the evidence the change is the boundary condition it claims to be and not a +reshuffle of the draw loop. It also fails alongside +`a_group_header_first_on_its_axis_reaches_both_front_ends` when `body_from`'s +match is forced to `false`. + ### two_row_categories_produce_cross_product_labels Names the rendering the protocol must not carry: multi-level row headers show @@ -509,44 +534,6 @@ nothing. Fault injected by forcing `body_from`'s match to `false`: this test fails and `the_whole_frame_is_unchanged` fails with it. -### the_whole_frame_is_unchanged - -Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the -leading `▼ Co…` header now draws above `East`, and everything below shifts -down one. Nothing else in the buffer changed. That the group line, the column -headers, the separator, the data rows and the totals strip are byte-identical -is the evidence the change is the boundary condition it claims to be and not a -reshuffle of the draw loop. - -### the_two_front_ends_draw_the_same_column_header - -Two changes, from two tickets. - -improvise-gd83: `THE_MEMBERSHIP_CORRECT_SPANS` and its `assert_ne!` tripwire are -gone, and the value they predicted — `[(Q1,2), (None,1), (Q1,2), (None,1)]` — -is now what the test asserts, element for element. `Sep` is ungrouped, so the -run ends before it and resumes under the second measure's own `Q1` header. A -precondition asserts the fixture still contains an ungrouped column, because -without one the test cannot see a leaked group at all. - -improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is -one-directional and passes while the TUI labels **every** column. The run rule -now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends -call it; the TUI's group line is compared cell for cell against spans derived -from the browser's own runs. - -Fault injected exactly as the ticket specifies — forcing the TUI to emit the -group label on every column (`head = col_groups.get(ci)`, dropping the run-head -lookup). Before: `cargo test -p improvise` stayed green. After: this test fails -with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against -`["▼ Q1","","","▼ Q1","",""]`. - -One thing the shared rule does **not** flatten: the TUI labels its first -*visible* column even mid-run, because a scrolled grid would otherwise draw an -unlabelled group line. That is a scrolling concession, kept explicitly, and it -is why the comparison is run at `col_offset == 0`. - - ### two_abutting_runs_of_one_group_name_are_two_spans_at_both_ends improvise-h0iv, end to end. The unit demonstration is in diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index 4fc83907..5f1bd1d9 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -21,4 +21,5 @@ unicode-width = "^0.2" web-time = "1" [dev-dependencies] +improvise-protocol = { path = "../improvise-protocol", features = ["test-support"] } tempfile = "3" diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 1cba2e88..4cc5efd6 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -2604,6 +2604,89 @@ mod tests { } } + /// The settle's `RecordsNormal` arm, driven from the mode it names. The + /// `:` crossing above enters the settle in `Normal` both ways — the evidence. + #[test] + fn a_batch_that_leaves_records_settles_out_of_records_normal() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + let mut app = App::new(wb, None); + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.layout.is_records_mode(), "setup: the grid is records"); + assert_eq!( + app.view_state.mode, + AppMode::RecordsNormal, + "setup: the mode handed to the settle must be RecordsNormal" + ); + + app.apply_effects(vec![Box::new(super::super::effect::SetAxis { + category: "_Index".into(), + axis: Axis::None, + })]); + + assert!( + !app.layout.is_records_mode(), + "setup: the batch has to leave records, or nothing crossed" + ); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "a batch that left records stayed in RecordsNormal (improvise-4dpj)" + ); + } + + /// The batch `SwitchViewAtCursor` emits, both ways across records-ness: + /// records-ness is auto-detected from the axes, so a view switch moves it. + #[test] + fn switching_views_across_records_ness_settles_against_the_view_switched_to() { + for to_records in [false, true] { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + let mut app = App::new(wb, None); + // `R` creates and enters `_Records`; the pivot view it left is the + // other end of every crossing below. + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + let records_view = app.view_state.active_view.clone(); + let (from, to) = match to_records { + true => ("Default", records_view.as_str()), + false => (records_view.as_str(), "Default"), + }; + app.apply_effects(vec![Box::new(super::super::effect::SwitchView( + from.into(), + ))]); + assert_eq!( + app.layout.is_records_mode(), + !to_records, + "setup: the switch has to CROSS, so it must start on the other side" + ); + + app.apply_effects(vec![ + Box::new(super::super::effect::SwitchView(to.into())), + super::super::effect::change_mode(AppMode::Normal), + ]); + + assert_eq!( + app.layout.is_records_mode(), + to_records, + "setup: the switch has to land, or nothing crossed" + ); + let expected = match to_records { + true => AppMode::RecordsNormal, + false => AppMode::Normal, + }; + assert_eq!( + app.view_state.mode, expected, + "the batch settled against the view it LEFT, not the one it \ + switched to (improvise-4dpj)" + ); + } + } + /// A `:` line that really runs settles through `ReturnToNormalUnlessMoved`, /// which an empty line never reaches. See the evidence. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index d79469a9..6f300035 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1410,11 +1410,38 @@ mod tests { } } - /// Effects declaring `LayoutGeometry` must move `document_config` — the - /// production hash `model_state.dirty` is decided by (improvise-1xf4). + /// The case list below is COMPLETE over the wire vocabulary, which the + /// compiler maintains — improvise-26dm, `context/code/command.md`. #[test] - fn layout_geometry_effects_actually_change_document_config() { - let cases: Vec> = vec![ + fn the_layout_geometry_cases_are_every_wire_effect_of_that_kind() { + let app = grouped_app(); + let mut reachable: Vec = crate::ui::reduce::tests::every_model_command() + .iter() + .flat_map(|cmd| crate::ui::reduce::model_effects(&app, cmd)) + .filter(|e| e.kind() == EffectKind::Model(ModelChange::LayoutGeometry)) + .map(|e| crate::ui::reduce::tests::effect_type_name(e.as_ref())) + .collect(); + let mut listed: Vec = layout_geometry_cases() + .iter() + .map(|e| crate::ui::reduce::tests::effect_type_name(e.as_ref())) + .collect(); + reachable.sort_unstable(); + reachable.dedup(); + listed.sort_unstable(); + listed.dedup(); + assert_eq!( + listed, reachable, + "the LayoutGeometry case list and the wire vocabulary disagree. A name \ + only on the LEFT means every_model_command() has a hole, not that the \ + case is spurious; a name only on the RIGHT is an effect nothing here \ + exercises. Read `context/code/command.md` before deleting either side" + ); + } + + /// The `LayoutGeometry` effects, named once so two tests can ask the same + /// list the same questions — improvise-26dm, `context/code/command.md`. + fn layout_geometry_cases() -> Vec> { + vec![ Box::new(AddCategory("Region".into())), Box::new(RemoveCategory("Month".into())), Box::new(CreateView("Second".into())), @@ -1443,10 +1470,14 @@ mod tests { Box::new(CycleAxis("Type".into())), Box::new(SetNumberFormat(",.3f".into())), Box::new(TogglePruneEmpty), - ]; - // A COUNT, not a totality check: nothing forces a fourteenth here, and a - // probe variant proved it — improvise-5ubs, `context/code/command.md`. - assert_eq!(cases.len(), 13, "the LayoutGeometry effects were thirteen"); + ] + } + + /// Effects declaring `LayoutGeometry` must move `document_config` — the + /// production hash `model_state.dirty` is decided by (improvise-1xf4). + #[test] + fn layout_geometry_effects_actually_change_document_config() { + let cases = layout_geometry_cases(); for effect in cases { assert_eq!( effect.kind(), diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index f3716cc6..416e78e6 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -273,7 +273,7 @@ pub fn reduce_key(app: &mut App, key: KeyEvent) -> Vec { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use crate::model::cell::CellValue; use crate::ui::app::AppMode; @@ -492,9 +492,9 @@ mod tests { app } - /// One value per `ModelCommand` variant. [`variant_name`] matches - /// exhaustively, so a new variant fails the build until it is listed here. - fn every_model_command() -> Vec { + /// One value per `ModelCommand` variant — by hand, and NOT compiler-forced: + /// [`variant_name`]'s exhaustive match only forces an arm there (PR #96). + pub(crate) fn every_model_command() -> Vec { vec![ ModelCommand::SetCell { cell: record_cell(&a_stored_cell()), @@ -658,7 +658,7 @@ mod tests { /// An effect's type name, off the leading identifier of its derived `Debug`. /// `Effect` is a trait object, so `std::any::type_name` cannot reach it. - fn effect_type_name(effect: &dyn Effect) -> String { + pub(crate) fn effect_type_name(effect: &dyn Effect) -> String { format!("{effect:?}") .split(|c: char| !c.is_alphanumeric() && c != '_') .next() @@ -711,6 +711,25 @@ mod tests { assert_eq!(names.len(), total, "a variant is listed twice: {names:?}"); } + /// ...and covers it WHOLLY, against serde's derive — the one enumeration the + /// compiler maintains (improvise-5ubs). `context/code/command.md`. + #[test] + fn the_enumeration_names_every_variant_the_derive_declares() { + let listed: Vec = every_model_command() + .iter() + .map(|cmd| variant_name(cmd).to_string()) + .collect(); + let missed: Vec = improvise_protocol::variants::declared::() + .into_iter() + .filter(|name| !listed.contains(name)) + .collect(); + assert!( + missed.is_empty(), + "every_model_command() never constructs {missed:?} — adding an arm to \ + variant_name is compiler-forced, adding a VALUE to the list is not" + ); + } + /// A model `Command` applied authoritatively lands where the keypress bound /// to the same mutation lands, `Dirty` included. #[test] diff --git a/crates/improvise-protocol/Cargo.toml b/crates/improvise-protocol/Cargo.toml index 7f350f5c..c114f10c 100644 --- a/crates/improvise-protocol/Cargo.toml +++ b/crates/improvise-protocol/Cargo.toml @@ -13,6 +13,13 @@ serde = { version = "1", features = ["derive"] } # (`AxisFrame::header_widths`). A terminal column is a display cell, not a # `char`, and the TUI has always measured it this way. unicode-width = "^0.2" +# Only for `variants`, which is off unless a test build asks for it. +serde_json = { version = "1", optional = true } + +[features] +# `variants` reaches OUT of this crate: improvise-command bounds its own +# ModelCommand enumeration with it. Off in every shipped build (improvise-26dm). +test-support = ["dep:serde_json"] [dev-dependencies] serde_json = "1" diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index d58bbc33..d4acf230 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -13,8 +13,8 @@ pub mod keymap; mod reduce; pub mod render; pub mod resolve; -#[cfg(test)] -pub(crate) mod variants; +#[cfg(any(test, feature = "test-support"))] +pub mod variants; pub mod viewmodel; pub use basis::{Basis, BasisCache, Rejection}; diff --git a/scripts/check-comment-runs.py b/scripts/check-comment-runs.py index 07b61e7a..6f34f27f 100755 --- a/scripts/check-comment-runs.py +++ b/scripts/check-comment-runs.py @@ -413,8 +413,33 @@ def js_files(roots): yield os.path.join(dirpath, f) +def _duplicate_claims(claims, by_leaf, defined): + """Two headings naming ONE test, keyed by the test they resolve to. + + Resolved and not spelled, so `### f` beside `### m::tests::f` is caught + too: the heading is the key a reader looks a test up by, and two entries + under one key means the first one found is taken as current. See + context/code/tooling.md. + """ + def canonical(kind, name): + if kind == "js" or name in defined["rust"]: + return name + matches = by_leaf.get(name, set()) + return next(iter(matches)) if len(matches) == 1 else name + + grouped = {} + for (kind, name), where in claims.items(): + grouped.setdefault((kind, canonical(kind, name)), []).extend(where) + def in_file_order(loc): + path, _, n = loc.rpartition(":") + return (path, int(n) if n.isdigit() else 0) + + return {key: sorted(where, key=in_file_order) + for key, where in grouped.items() if len(where) > 1} + + def evidence_verdict(claims, defined, citing): - """(orphans, unstated, ambiguous), keyed by ("rust"|"js", name). + """(orphans, unstated, ambiguous, duplicates), keyed by ("rust"|"js", name). `orphans` is evidence naming a test nobody can find. `unstated` is the reverse obligation (improvise-lyyc): a test that POINTS AT the evidence @@ -422,8 +447,11 @@ def evidence_verdict(claims, defined, citing): `ambiguous` is a bare leaf that two tests answer to -- a refusal and not a warning, because a heading satisfied by a test in another module answering another question is a pass while the gate does nothing. + `duplicates` is the mirror image: two headings answered by one test, where + every copy passes and the reader takes whichever comes first as current + (improvise-hsoe). Also a refusal, on the improvise-rm4s precedent. Only that narrower reverse obligation, because most tests have no evidence - and should not be forced to. What none of the three can see: an entry can + and should not be forced to. What none of the four can see: an entry can exist and still describe its subject backwards. """ by_leaf = {} @@ -452,7 +480,7 @@ def evidence_verdict(claims, defined, citing): for name in names: if (kind, name) not in claimed: unstated.setdefault((kind, name), []).extend(sorted(names[name])) - return orphans, unstated, ambiguous + return orphans, unstated, ambiguous, _duplicate_claims(claims, by_leaf, defined) def check_evidence(roots, rust_listing=None, rust_ignored=None): @@ -493,10 +521,10 @@ def check_evidence(roots, rust_listing=None, rust_ignored=None): for name in c: citing["js"].setdefault(name, []).append(path) - orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) - if not orphans and not unstated and not ambiguous: + orphans, unstated, ambiguous, duplicates = evidence_verdict(claims, defined, citing) + if not orphans and not unstated and not ambiguous and not duplicates: print(f"test-evidence: {len(claims)} entries, all name a test that runs " - f"(names from {source}); " + f"and no test is named twice (names from {source}); " f"{sum(len(v) for v in citing.values())} tests cite the file and all have one; " f"{prose} prose headings claim nothing") return 0 @@ -514,6 +542,11 @@ def check_evidence(roots, rust_listing=None, rust_ignored=None): print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) for path in sorted(by_leaf[name]): print(f" -> {path}", file=sys.stderr) + if duplicates: + print(f"\n{len(duplicates)} tests are named by more than one test-evidence " + "heading; merge them or give each its own test:\n", file=sys.stderr) + for (kind, name), where in sorted(duplicates.items()): + print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) if unstated: print(f"\n{len(unstated)} tests cite {EVIDENCE_DIR} and have no entry there:\n", file=sys.stderr) @@ -910,15 +943,64 @@ SELF_TEST_AMBIGUITY = [ ] -def _ambiguity_self_test(): +# (name, evidence markdown, cargo listing, js source, the tests named twice). +# +# A heading that names a test that RUNS passes -- and a second heading naming +# the same one passes just as well, so an entry superseded by its successor +# reads as current to whoever meets it first (improvise-hsoe, found on PR #92). +# Every case here is an injection; the clean arms stop an always-fail passing. +SELF_TEST_DUPLICATES = [ + ("one heading twice in one file is refused", + "### a_real_test\n### a_real_test\n", "m::tests::a_real_test: test\n", "", + ["m::tests::a_real_test"]), + + ("the qualified and bare spellings of one test are the same heading", + "### a_real_test\n### m::tests::a_real_test\n", "m::tests::a_real_test: test\n", "", + ["m::tests::a_real_test"]), + + # A BARE heading, because two qualified ones resolve by the exact-match arm + # and never reach the ambiguity guard this case is named for (review, PR #96). + ("a bare heading over an ambiguous leaf is not a duplicate of a qualified one", + "### shared\n### m::tests::shared\n### n::tests::shared\n", + "m::tests::shared: test\nn::tests::shared: test\n", "", []), + + ("a JS heading twice is refused too", + '#### "a spec"\n#### "a spec"\n', "", 'test("a spec", () => {});\n', ["a spec"]), + + ("two headings naming two tests are not duplicates", + "### one\n### two\n", "m::one: test\nm::two: test\n", "", []), + + ("prose headings repeat freely -- they claim no test", + "### Helper - x\n### Helper - x\n", "", "", []), +] + + +def _duplicate_self_test(): failures = [] + for name, md, listing, js, want in SELF_TEST_DUPLICATES: + claims = {} + for kind, cname, n in evidence_claims(md): + if kind != "prose": + claims.setdefault((kind, cname), []).append(f"fixture:{n}") + jd, _ = js_tests(js) + defined = {"rust": cargo_test_names(listing), "js": jd} + *_, duplicates = evidence_verdict(claims, defined, {"rust": {}, "js": {}}) + got = sorted(k[1] for k in duplicates) + if got != sorted(want): + failures.append(f" DUPLICATES: {name}: expected {sorted(want)}, saw {got}") + return failures + + +def _ambiguity_self_test(): + failures = _duplicate_self_test() for name, md, listing, want_o, want_a in SELF_TEST_AMBIGUITY: claims = {} for kind, cname, n in evidence_claims(md): if kind != "prose": claims.setdefault((kind, cname), []).append(f"fixture:{n}") defined = {"rust": cargo_test_names(listing), "js": set()} - orphans, _, ambiguous = evidence_verdict(claims, defined, {"rust": {}, "js": {}}) + orphans, _, ambiguous, _dupes = evidence_verdict( + claims, defined, {"rust": {}, "js": {}}) got_o = sorted(k[1] for k in orphans) got_a = sorted(k[1] for k in ambiguous) if got_o != sorted(want_o): @@ -953,7 +1035,9 @@ def _evidence_self_test(): defined = {"rust": cargo_test_names(listing), "js": jd} citing = {"rust": {k: ["fixture.rs"] for k in rc}, "js": {k: ["fixture.js"] for k in jc}} - orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) + orphans, unstated, ambiguous, duplicates = evidence_verdict(claims, defined, citing) + if duplicates: + failures.append(f" DUPLICATES: {name}: unexpected {sorted(k[1] for k in duplicates)}") got_o = sorted(k[1] for k in orphans) got_u = sorted(k[1] for k in unstated) if got_o != sorted(want_orphans): @@ -1037,6 +1121,7 @@ def self_test(): f"{len(SELF_TEST_EVIDENCE)} evidence verdicts, " f"{len(SELF_TEST_LISTING) + len(SELF_TEST_HEADINGS)} listing readings, " f"{len(SELF_TEST_AMBIGUITY)} name resolutions, " + f"{len(SELF_TEST_DUPLICATES)} duplicate-heading verdicts, " f"{len(SELF_TEST_DISPATCH)} invocations answer the cap") return 0