Merge pull request #96 from fiddlerwoaroof/wave/gate-and-coverage
Three checks that could not fail: duplicate evidence headings, the records-mode settle, the LayoutGeometry case list
This commit is contained in:
Ed Langley
GitHub
@@ -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::<ModelCommand>()`, 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.
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -21,4 +21,5 @@ unicode-width = "^0.2"
|
||||
web-time = "1"
|
||||
|
||||
[dev-dependencies]
|
||||
improvise-protocol = { path = "../improvise-protocol", features = ["test-support"] }
|
||||
tempfile = "3"
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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<Box<dyn Effect>> = vec![
|
||||
fn the_layout_geometry_cases_are_every_wire_effect_of_that_kind() {
|
||||
let app = grouped_app();
|
||||
let mut reachable: Vec<String> = 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<String> = 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<Box<dyn Effect>> {
|
||||
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(),
|
||||
|
||||
@@ -273,7 +273,7 @@ pub fn reduce_key(app: &mut App, key: KeyEvent) -> Vec<Command> {
|
||||
}
|
||||
|
||||
#[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<ModelCommand> {
|
||||
/// 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<ModelCommand> {
|
||||
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<String> = every_model_command()
|
||||
.iter()
|
||||
.map(|cmd| variant_name(cmd).to_string())
|
||||
.collect();
|
||||
let missed: Vec<String> = improvise_protocol::variants::declared::<ModelCommand>()
|
||||
.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]
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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};
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user