Claude (dbsp-sources)
2456 lines
130 KiB
Markdown
2456 lines
130 KiB
Markdown
# Test evidence — `improvise-core`
|
||
|
||
What broke, what fault was injected, and why each test can fail. Keyed by exact
|
||
test name (design-principles §7). A renamed test orphans its entry;
|
||
`scripts/check-comment-runs.py --check-evidence` catches a heading naming a test
|
||
that does not exist.
|
||
|
||
Only tests whose narrative is longer than the code appear here. Most tests need
|
||
none.
|
||
|
||
---
|
||
|
||
## `model/types.rs`
|
||
|
||
### model::types::formula_tests::removing_a_measure_row_that_is_only_a_formula_target_removes_the_formula
|
||
|
||
improvise-g3no, demonstrated twice in a real browser and once in the TUI.
|
||
`_Measure`'s items are "explicit items UNION formula targets" (gotcha 7), so a
|
||
`Profit` that exists only because a `Formula` does is not in the category's own
|
||
item list. `Model::remove_item` called `cat.remove_item`, which no-opped, then
|
||
deleted cells naming it, of which there are none because the value is derived;
|
||
`effective_item_names` then re-synthesised the row from the surviving formula
|
||
and it came straight back. Both front ends meanwhile composed an unconditional
|
||
past-tense status beside the mutation, so the user was told it had worked.
|
||
|
||
**The diagnosis the bug report and the ticket both reached was one level too
|
||
high.** They read it as a truthfulness defect — the status is emitted before
|
||
the fallible effect, the opposite of gotcha 20's `CommitBuffer` ordering — and
|
||
asked for a `Removed` answer on `Model::remove_item` that the front ends could
|
||
let win. That is unimplementable on the browser side and would have been
|
||
discovered late: `SetStatus` is `ViewOnly` and never crosses the wire, so a
|
||
host-side refusal cannot reach a client at all; and a client cannot decide for
|
||
itself, because nothing in the projection stream says which `_Measure` rows are
|
||
derived — `FormulaCache::formulas` is `Vec<String>` of `Formula::raw`, and
|
||
recovering a target from it means a second home for the unquoting gotcha 34
|
||
says has exactly one.
|
||
|
||
The actual defect is an **asymmetry in the model**: `remove_category` has
|
||
always ended by dropping every formula targeting the category, and the item
|
||
level did not. Restoring it makes the row genuinely go away, which makes the
|
||
status both front ends already compose *true*, with no second classification
|
||
and nothing new on the wire. Inject the fault by deleting the `retain`.
|
||
|
||
The fixture builds its own model rather than using `revenue_cost_model()`,
|
||
which carries an explicit `Profit` item — against that fixture the test passes
|
||
before the fix, because there really is an item to remove. That is the whole
|
||
distinction the bug turns on, so the precondition is asserted rather than
|
||
assumed.
|
||
|
||
### model::types::formula_tests::removing_a_regular_item_removes_the_formula_targeting_it
|
||
|
||
The same asymmetry one level up, and the reason the sweep is not restricted to
|
||
`_Measure`. A non-measure target **is** a real item (`add_formula` creates it),
|
||
so removing it left a formula aimed at nothing. A `_Measure`-only fix would
|
||
have satisfied the ticket and left this live.
|
||
|
||
### model::types::formula_tests::removing_an_item_keeps_a_formula_that_only_references_it
|
||
|
||
The direction the fix must not overreach in, and it passes before the fix as
|
||
well as after — it is there to stop the next change, not to catch this one.
|
||
`retain` on `f.target == item` alone, or on `target` without `target_category`,
|
||
both look plausible and both would take formulas the user still wants:
|
||
deleting `Cost` must not delete `Profit = Revenue - Cost`.
|
||
|
||
### model::types::model_tests::a_required_virtual_category_cannot_be_removed
|
||
|
||
improvise-uej8's second half, which is improvise-3b2b's subject. The panel
|
||
deliberately lists `_Index`/`_Dim`/`_Measure` (improvise-vjms — a decision
|
||
about *visibility*), and `d` at the panel's **default cursor 0** deleted
|
||
`_Index` outright. The invariant that the virtual categories always exist
|
||
(gotcha 7) was reachable by one keypress from a fresh session, and
|
||
`GridLayout`'s records-mode detection keys off `_Index` on Row, so the symptom
|
||
is that records mode silently stops working.
|
||
|
||
The guard is in `Model::remove_category` rather than in the two `Cmd`s,
|
||
because hiding the delete key leaves the wire mutation
|
||
(`ModelCommand::RemoveCategory`) unguarded — 3b2b asks for exactly this
|
||
boundary. It reads `CategoryKind::of_name`, the one home of the reserved-name
|
||
mapping (improvise-2lh), so a `Label` category stays removable.
|
||
|
||
The second assertion is the one that earns its place: putting the category
|
||
back is not enough, since `remove_category` sweeps cells and formulas *after*
|
||
removing the key from the map. It asserts a stored `Revenue` still evaluates,
|
||
so a guard that returned too late fails. The opposite direction is already
|
||
held by `remove_category_deletes_category_and_cells` immediately below.
|
||
|
||
### a_chained_formula_still_converges_after_the_circular_marker
|
||
|
||
The anti-clamp half of improvise-7iww. `recompute_formulas` now publishes
|
||
`Error("circular")` for any cell still moving after `MAX_EVAL_DEPTH` passes, and
|
||
a guard that refuses everything would satisfy the reproduction just as well as
|
||
the fix does. This test runs a three-deep chain — `Cost`, `Profit`, `Margin` —
|
||
that converges in two passes and asserts the *numbers*, so a marker that fired
|
||
on a converging chain fails here. Inject the fault by marking every cell
|
||
evaluated in the last pass rather than only the ones that changed.
|
||
|
||
A second, free cross-check lives in
|
||
`an_unknown_name_inside_an_aggregate_errors_for_every_function`: it asserts the
|
||
error text `ref:Typo` *after* `recompute_formulas`, so a formula that errors
|
||
identically on every pass must be seen to **converge** rather than be
|
||
overwritten with `circular`. The two fixes check each other.
|
||
|
||
One arm of the loop is worth naming because it looks reachable and is not. A
|
||
pass that computes `None` where the cache holds `Some` counts as moving and
|
||
stores nothing, so it would spin to the bound and now be marked circular. The
|
||
only `None` `evaluate_formula_cell` can produce is a `WHERE` filter that does
|
||
not match falling through to `aggregate_raw` over no data — and `aggregate_raw`
|
||
reads `self.data`, never the cache, while the formulas do not change during a
|
||
recompute. So for a given key that branch answers the same thing on every pass,
|
||
and a `Some`-to-`None` transition cannot occur. The pre-existing code spun the
|
||
same sixteen passes here and left a stale value; of the two, publishing
|
||
`circular` is the safer, so the arm is documented rather than changed.
|
||
|
||
### a_derived_measure_with_stored_cells_does_not_leak_the_raw_value
|
||
|
||
improvise-s2w3's stated edge case. The fix routes a bare `SUM(Double)` through
|
||
the per-record fold when `Double` is a formula target, and the interesting case
|
||
is a target that *also* has stored cells: `formula_for` is the one home of "is
|
||
this cell derived", so the fold reads `20` (the formula) and not `999` (the raw
|
||
cell). Fails if the fast path is reinstated for a derived name, or if the fold
|
||
were built to prefer raw data over `formula_for`.
|
||
|
||
### an_aggregate_over_a_derived_measure_sees_the_formula_in_both_evaluators
|
||
|
||
improvise-s2w3, widened past the reproduction. `eval_agg` specialised
|
||
`Expr::Ref` into a raw `DataStore` lookup; a formula target has no raw cell
|
||
behind it (improvise-nrs), so `SUM(Double)` summed nothing and answered `0`
|
||
while `SUM(Double + 0)` folded per record and answered `20`. core.md §13
|
||
defends that fast path and the defence still holds — it is the fold specialised
|
||
to a ref naming a *stored* cell. What failed is the premise, not the
|
||
specialisation, so the fix asks `formula_for` rather than deleting the path.
|
||
|
||
The test runs `SUM`/`AVG`/`MIN`/`MAX` and both evaluators (direct `evaluate`,
|
||
and `evaluate_aggregated(OverHidden)` after `recompute_formulas`), because §3
|
||
says a rule stated in one evaluator and not the other is a live bug. `COUNT` is
|
||
excluded deliberately: `COUNT(Double)` and `COUNT(Double + 0)` both count
|
||
members and agreed even before the fix, so including it would have added a
|
||
passing case that discriminates nothing. Inject the fault by hard-coding
|
||
`derived = false` in `eval_agg`.
|
||
|
||
### an_unknown_name_inside_an_aggregate_errors_for_every_function
|
||
|
||
improvise-s08t. `eval_agg` narrowed its partial key only `if let Some(cat) =
|
||
find_item_category(name)`; when the lookup failed the key was left
|
||
**unconstrained**, so `SUM(Typo)` aggregated every numeric measure matching the
|
||
context and returned a plausible wrong total. A typo is silent and looks like a
|
||
number. The test asserts the error *text* (`ref:Typo`) across all five aggregate
|
||
functions and both evaluators, so a fix that errors with some other message — or
|
||
that errors only where the raw scan happened to be empty — still fails. Inject
|
||
the fault by restoring `unwrap_or("_Measure")` in place of the `ok_or_else`.
|
||
|
||
### both_evaluators_answer_every_comparison_the_same_way
|
||
|
||
improvise-vyrx's *behavioural* half, and it is deliberately the smaller half.
|
||
The ticket's defect is that `eval_bool_cached` wildcarded `BinOp` where
|
||
`eval_bool` listed its arms, so a new variant would error at runtime in one
|
||
evaluator and fail to compile in the other — an asymmetry no runtime test can
|
||
see today, because both paths answer `Err("type")` for every arithmetic
|
||
operator. **The guard is the compiler**: with the wildcard removed, adding a
|
||
`BinOp::Mod` fails the build at four sites (`eval_expr`, `eval_bool`,
|
||
`eval_expr_cached`, `eval_bool_cached`) where it previously failed at three, and
|
||
that was verified by adding the variant and reading the errors.
|
||
|
||
**A sweep for the shape found a second instance one level up**, in the same two
|
||
functions: both `eval_bool` and `eval_bool_cached` also wildcarded the outer
|
||
`Expr` match as `_ => Err("type")`, so a new `Expr` variant that ought to be
|
||
usable as a condition — an `And`, say — would have become a silent type error in
|
||
*both*. That is not vyrx's asymmetry (the two agreed), but it is vyrx's defect:
|
||
a new variant answered wrongly with nothing to notice. Both now list the six
|
||
non-comparison variants, and adding an `Expr::And` fails the build at four sites
|
||
where it previously failed at two. Verified the same way.
|
||
|
||
**What the sweep also settled is which wildcards to leave**, which is the more
|
||
useful half. `eval_agg`'s own `match inner { Expr::Ref(..) => …, _ => false }`
|
||
is deliberately *not* exhaustive: it falls through to the **general** path — the
|
||
per-record fold — which is the right answer for any expression that is not a
|
||
bare ref, including one that does not exist yet. The dangerous wildcard is the
|
||
one that falls through to a **refusal** or to a **specialisation**; a wildcard
|
||
that falls through to the general case is safe by construction. The same reading
|
||
clears `CellValue::as_f64`'s `_ => None` and `CategoryKind::of_name`'s
|
||
`_ => Regular`, whose inputs are a value that is not a number and a name that is
|
||
not reserved.
|
||
|
||
What this test adds is the neighbouring property the exhaustive arms do not
|
||
give: that the six comparison arms *agree in value* between the two evaluators.
|
||
It runs each operator against a true and a false right-hand side, so an arm
|
||
mistyped in one path (`Lt => lv <= rv`) fails. Both directions matter — a single
|
||
right-hand side is satisfied by an evaluator that answers a constant.
|
||
|
||
### re_adding_an_existing_category_at_the_limit_creates_nothing_and_succeeds
|
||
|
||
improvise-f86q. `Model::add_category` checked `MAX_CATEGORIES` *before*
|
||
`contains_key`, so at twelve regular categories re-adding an existing name was
|
||
refused with "Maximum of 12 categories reached" — a false statement, since the
|
||
category exists and nothing would be created. Per gotcha 21 `effect::AddCategory`
|
||
sets `abort_effects` on refusal, so typing an existing name into the records-mode
|
||
`+cat` column at the limit aborted the rest of the batch: the improvise-2zq
|
||
failure mode by another route.
|
||
|
||
The fix is a hoist. The test asserts three things, and the third is what stops it
|
||
from being satisfied by deleting the limit: the returned id is the existing
|
||
category's, the category count did not move, and `add_category("C12")` — a name
|
||
that really would create something — is still refused.
|
||
|
||
### a_hidden_dimension_list_reduces_to_one_of_two_states
|
||
|
||
improvise-5jm5. `evaluate_aggregated` took `none_cats: &[String]` and consulted
|
||
it once, for emptiness — so two lists with different contents and the same
|
||
emptiness gave the same answer, while the name promised a filter. Nothing
|
||
misbehaved; the signature described something the body never did.
|
||
|
||
This test is the *smaller* half of the guard and says so. The real guard is the
|
||
signature: the defect the ticket describes is no longer constructible, exactly
|
||
as `Checked`'s constructor is the guard for the basis check rather than a test
|
||
that can fail. What this test does add is that `Aggregation::of` is the only
|
||
place the list is read, and that a list of two hidden dimensions is not somehow
|
||
a third state — the reduction is emptiness, and the enum is not a count.
|
||
|
||
### apply_agg_over_nothing_is_absent_for_every_function
|
||
|
||
An empty cell must read as *absent*, not as zero — otherwise every blank
|
||
intersection in a pivot renders `0` and a sparse model looks fully populated.
|
||
Fails if `apply_agg` ever gains a `Sum => 0.0` identity arm for the empty case.
|
||
`eval_agg` deliberately has the opposite rule; see the next entry.
|
||
|
||
### eval_agg_over_no_rows_is_zero_only_where_an_identity_exists
|
||
|
||
A *formula* aggregate over no rows is `0` for `SUM`/`COUNT`, which have an
|
||
identity, and an error for `AVG`/`MIN`/`MAX`, which do not. This is deliberately
|
||
**not** `apply_agg`'s rule, and the two are separate functions precisely so the
|
||
difference is visible. improvise-kdp named both after they had been duplicated
|
||
inline in two evaluators; the test pins the contract directly rather than only
|
||
through formula eval.
|
||
|
||
Its **vehicle changed** with improvise-s08t. It reached "no rows" through
|
||
`Expr::Ref("Nonexistent")` — a name that resolves to nothing — which was
|
||
precisely the bug s08t names: an unresolvable ref left the selection
|
||
unconstrained instead of erroring. So the test was standing on the defect to
|
||
demonstrate a neighbouring property. It now uses a name that **resolves** and
|
||
has no cells (`Revenue`, declared and never written), which is the emptiness
|
||
the entry is about; an unknown name is now an error and is pinned separately by
|
||
`an_unknown_name_inside_an_aggregate_errors_for_every_function`.
|
||
|
||
### find_item_category_falls_back_to_formula_targets
|
||
|
||
`add_formula` never inserts its target into `_Measure` (the item is *implied* by
|
||
the formula definition). So `find_item_category` must fall back to the formula
|
||
list, or `Expr::Ref("Profit")` fails to resolve and every cell of a chained
|
||
formula errors `ref:Profit` (improvise-nrs). The test asserts the setup — that
|
||
the target really is not a stored item — before asserting the lookup, so it
|
||
cannot pass by the item having been added after all.
|
||
|
||
### stored_virtual_categories_match_their_names
|
||
|
||
`CategoryKind::of_name` is used all over the codebase in place of looking the
|
||
`Category` up. That substitution is only sound because (a) a fresh `Model`
|
||
really does store each reserved name under its virtual kind and (b)
|
||
`add_category` hands back the existing virtual category rather than shadowing a
|
||
reserved name with a `Regular` one. The second half is the arm that would break
|
||
silently: a shadowed `_Measure` would still answer `of_name` correctly while the
|
||
stored category had the wrong kind (improvise-2lh).
|
||
|
||
### aggregating_one_text_value_yields_that_text
|
||
|
||
The head of the improvise-l4qx family. `aggregate_cell_values` used to be
|
||
`filter_map(as_f64)`, which **dropped** text. `_Index`/`_Dim` sit on `Axis::None`
|
||
in every default view, so `none_cats` is non-empty almost always and the
|
||
aggregating path was almost always taken — making **every text cell in every
|
||
document render blank**. `.improv` carries pipe-quoted text as a first-class
|
||
value, so a user could type a text value, save, load, and never see it again.
|
||
|
||
### aggregating_differing_text_values_is_an_explicit_error
|
||
|
||
Two different words have no reduction. The user is told `mixed` rather than
|
||
shown one of them arbitrarily, or a blank.
|
||
|
||
### aggregating_text_among_numbers_is_an_explicit_error
|
||
|
||
The case that produced a *plausible* number before: the text was dropped and the
|
||
rest summed, so the cell read as authoritative while under-reporting. A blank
|
||
would have been noticed; a wrong total is not.
|
||
|
||
### a_lone_number_still_counts_as_one_rather_than_passing_through
|
||
|
||
The regression guard for the hot path, and the test that pins the **branch
|
||
order** in `aggregate_cell_values`. The all-numeric branch is checked *first*,
|
||
which is the entire difference between this rule and "a single value passes
|
||
through". `Count` is the only function that can tell them apart — every other
|
||
function is the identity on a one-element set — so this test sets
|
||
`measure_agg["Status"] = Count` and requires `1`, not `7`. Reordering the two
|
||
branches passes every other test in the family.
|
||
|
||
### aggregating_nothing_is_still_absent
|
||
|
||
The text rule must not turn `apply_agg`'s emptiness rule into an error or a
|
||
zero. Fails if `aggregate_cell_values` answers on an empty slice at all.
|
||
|
||
### where_filter_absent_category_does_not_apply_formula
|
||
|
||
`Profit = 42 WHERE Region = "East"` evaluated against a key with **no** Region
|
||
coordinate must not apply the formula: the condition cannot be satisfied, so the
|
||
raw cell value (absent) is the answer. The bug was an `if let Some(item_val)`
|
||
that failed to bind when the category was absent and fell through, applying the
|
||
formula unconditionally — so a grand-total cell showed the filtered formula's
|
||
value.
|
||
|
||
### sum_inner_expression_constrains_which_cells_are_summed
|
||
|
||
`SUM(Revenue)` ignored its inner expression and summed every numeric cell
|
||
matching the partial key. With `Revenue=100` and `Cost=50` both stored for
|
||
`Region=East`, `Total = SUM(Revenue)` returned `150`. The fixture needs **two**
|
||
measures on one coordinate or the bug is invisible.
|
||
|
||
### add_formula_same_target_name_different_category_both_coexist
|
||
|
||
`add_formula` deduplicated by `target` name alone, ignoring `target_category`,
|
||
so `Profit` in `_Measure` and `Profit` in `KPI` could not coexist — the second
|
||
silently replaced the first.
|
||
|
||
### add_formula_same_target_name_different_category_evaluates_independently
|
||
|
||
The consequence of the same bug seen through evaluation rather than through
|
||
`formulas.len()`: the dropped formula's cell evaluated to absent. Worth keeping
|
||
beside the count assertion because a fix that deduplicates correctly but
|
||
dispatches on name alone would pass the first and fail this one.
|
||
|
||
### remove_formula_only_removes_specified_target_category
|
||
|
||
The mirror image: `remove_formula` matched by target name alone, so removing
|
||
`Profit` from `_Measure` also destroyed the `Profit` formula in `KPI`.
|
||
|
||
### near_zero_divisor_consistent_with_equality_epsilon
|
||
|
||
improvise-0bf. `=`/`!=` compare with `FLOAT_EQ_EPSILON = 1e-10`, but the
|
||
division-by-zero guard checked `rv == 0.0` exactly. With `X = 1e-11` the
|
||
comparison `X = 0` was true *and* `100 / X` happily returned `1e13` — incoherent
|
||
semantics for `IF(X = 0, a, y / X)`, whose else branch divides by a value its own
|
||
condition called zero. Any value the equality operator treats as zero must also
|
||
trigger `div/0`. This exercises the **direct** eval path (`eval_expr` inside
|
||
`eval_formula_depth`).
|
||
|
||
### near_zero_divisor_consistent_in_cached_eval_path
|
||
|
||
The same bug in the **fixed-point/cached** path (`eval_expr_cached` inside
|
||
`eval_formula_with_cache`), whose division guard also used exact `== 0.0`. Two
|
||
tests because there are two evaluators and a rule stated in one and not the
|
||
other is a live bug — which is exactly how this one shipped.
|
||
|
||
### recompute_formulas_multi_stem_values_stable
|
||
|
||
improvise-6os replaced an O(n²) `Vec::contains` dedupe of coordinate stems with
|
||
an `IndexSet` — a pure performance change. The test pins the observable contract
|
||
instead of the mechanism: a multi-stem model (chained formulas, several measures
|
||
per stem) produces the stated values, and a second `recompute_formulas` call
|
||
reproduces the **exact same cache**, so a dedupe that lost or duplicated stems
|
||
would show up as a changed value or a non-idempotent cache.
|
||
|
||
### formula_chain_preserves_full_precision
|
||
|
||
Display rounds; eval must not. `Tax = Price * 0.075` on `Price = 10` is exactly
|
||
`0.75`, which `format_f64(_, true, 0)` displays as `"1"`. The test asserts both:
|
||
that the display really does round (so the fixture is discriminating) and that
|
||
`Total = Price + Tax` is `10.75` and not `11`. Without the first assertion the
|
||
test would pass on a fixture where rounding changes nothing.
|
||
|
||
## `format.rs`
|
||
|
||
### a_finite_number_never_displays_as_infinity
|
||
|
||
improvise-pikk. `round_half_away` scales by `10^decimals` before truncating, so
|
||
`n * factor` overflows to infinity for a large finite `n` and any nonzero
|
||
precision: `format_f64(1e307, false, 2)` rendered `"inf"`. The model held the
|
||
number correctly and the display invented an infinity. Gotcha 8 is why nothing
|
||
else caught it — `format_f64` is the whole display path and never feeds back
|
||
into eval, so this is a wrong pixel with no wrong number behind it to trip a
|
||
formula test.
|
||
|
||
Reachability is low and that is not a reason to leave it: the shipped default
|
||
format is `",.0"`, whose factor is `1`, so it needs both an absurd magnitude and
|
||
an explicit `.N` format.
|
||
|
||
The test carries its own anti-clamp assertion, because "return `n` unchanged"
|
||
satisfies the overflow case *and* every other case. `1e307 * 100` overflows and
|
||
`1e306 * 100` does not, which pins the threshold; and `format_f64(2.5, false, 0)
|
||
== "3"` pins that the rounding path is still reached at all — half-away-from-
|
||
zero, where a passthrough would hand the value to `{:.0}` and get Rust's
|
||
half-to-even `"2"`. Inject the fault by dropping the `is_finite` arm.
|
||
|
||
## `model/cell.rs`
|
||
|
||
### matching_cells_yields_the_primary_insertion_order
|
||
|
||
improvise-shyt. `matching_cells` walked the secondary index's `HashSet`, so its
|
||
order was hash order rather than entry order. `DataStore::cells` is an `IndexMap`
|
||
*precisely so* records mode can list rows in entry order. The check is against
|
||
`iter_cells` — the primary map's own order — and **not** against a literal,
|
||
which would only pin whatever the hasher happened to do on this machine today.
|
||
|
||
### the_same_cells_entered_in_opposite_orders_list_in_opposite_orders
|
||
|
||
The half a single-process test cannot reach by calling twice: two stores holding
|
||
the *same* logical cells, built by opposite insertion histories. Each must list
|
||
its own history, so one result is the reverse of the other. Under the index walk
|
||
the two orders were unrelated to both histories **and to each other**, because
|
||
`RandomState` keys each fresh `HashMap` differently.
|
||
`doctrine(an_order_test_needs_two_histories)`.
|
||
|
||
### matching_values_yields_the_primary_insertion_order
|
||
|
||
The same guarantee for the value-only hot path, which aggregation folds `f64`
|
||
over. f64 addition is not associative, so an unstable order made a `SUM`
|
||
reproducible only within one process — drift in the last bits, which is the kind
|
||
of wrongness nobody reports and nobody can reproduce. `Min`/`Max`/`Count` and
|
||
the text-unanimity rule are order-free and were never affected.
|
||
|
||
### an_empty_partial_matches_every_cell_from_either_door
|
||
|
||
improvise-hkt6. `matching_positions` indexed `sets[0]` and panicked on an empty
|
||
partial. Both public callers guard, so it was unreachable — but a guard one level
|
||
away from the indexing is how this class of bug returns, so the input the guards
|
||
prevent is fed in directly here.
|
||
|
||
It asserts the **answer**, not merely the absence of a panic: an empty partial
|
||
constrains nothing, so every cell matches, and that must be the same answer the
|
||
callers' fast paths give. Returning `vec![]` would also have been total and would
|
||
have made deleting a fast path silently flip "all" to "none".
|
||
|
||
### live_records_come_back_in_the_stores_own_insertion_order
|
||
|
||
improvise-2yc2. `live_records` walked the `records: HashMap`, so it yielded a
|
||
different order in every process and in every instance — the improvise-shyt
|
||
defect, caught here before a caller could inherit it. The order decided on is the
|
||
store's own: a record sits where its **first** cell sits in the primary
|
||
`IndexMap`. The fixture interleaves the measures deliberately — East's second
|
||
measure arrives after West, and must not move East behind it.
|
||
|
||
### the_same_records_entered_in_opposite_orders_list_in_opposite_orders
|
||
|
||
`doctrine(an_order_test_needs_two_histories)` applied to records: one history
|
||
cannot tell "ordered" from "hashed that way today, on this machine".
|
||
|
||
### a_recreated_record_takes_its_new_place_not_its_old_one
|
||
|
||
The claim `live_records` makes about **why** it derives the order from `cells`
|
||
rather than memoising a position on `RecordEntry` — and which nothing else could
|
||
see (Copilot, PR 20). A memoised position passes all three order tests above and
|
||
still puts a re-created record back at its **original** place while its cells sit
|
||
at the end of the store. The entry survives the deletion on purpose (it is a
|
||
tombstone, below), which is exactly what makes the memoised position available to
|
||
be wrong.
|
||
|
||
### deleting_and_recreating_a_record_does_not_reset_its_version
|
||
|
||
The tombstone earns its place here. If `RecordEntry` were dropped when the last
|
||
cell went, a delete-and-recreate would reset the counter to zero — and a client
|
||
holding version 0 against the *old* record would be told its basis is current and
|
||
its write applied. That is precisely the silent accept ADR 001's basis check
|
||
exists to prevent.
|
||
|
||
### a_surrogate_for_a_removed_record_fails_to_resolve
|
||
|
||
The first arm of staleness: a surrogate for a record whose cells are all gone
|
||
must fail to resolve rather than resolve to *some other* record. The fixture
|
||
holds **two** records, so a bug returning "any record" is visible; with one, the
|
||
assertion would pass by naming the only record there is.
|
||
|
||
### a_second_measure_on_a_record_resolves_to_the_same_record
|
||
|
||
The records-mode unit seen from the index side: two measures on one coordinate
|
||
tuple are one record, so they share one entry. A cell-level unit would be
|
||
violated by one legal edit (renaming a category coordinate moves every measure
|
||
cell in the row).
|
||
|
||
### a_live_records_surrogate_is_the_one_that_resolves_it
|
||
|
||
The pair `live_records` yields is not two facts that could drift apart: the
|
||
surrogate is the record's own, so it is the one that resolves it back.
|
||
|
||
## `model/record.rs`
|
||
|
||
### a_records_surrogate_is_pinned_to_a_literal
|
||
|
||
The surrogate must mean the same thing in the process that minted it and in the
|
||
one that restarts under it, so it is pinned to a literal rather than to
|
||
"whatever this build computes". **A failure here is not a bug in the test**: it
|
||
means the hash, the key or the byte serialization moved, and every surrogate any
|
||
client holds is now a name for something else. That is a wire-format break.
|
||
|
||
### the_two_kinds_do_not_share_a_hash
|
||
|
||
A record and an aggregate may be built from the same coordinates — an aggregate
|
||
over a single record is the degenerate case. A client holding an aggregate id
|
||
must not be able to spend it as a record id by accident, which is what the domain
|
||
tag byte is for.
|
||
|
||
### coordinates_that_concatenate_alike_do_not_hash_alike
|
||
|
||
The length prefixes in `hash_coords` earn their place: without them `("Ab","c")`
|
||
and `("A","bc")` serialize to the same bytes, and a collision here is a silent
|
||
write to the wrong record.
|
||
|
||
### renaming_either_half_of_a_coordinate_changes_the_hash
|
||
|
||
Two categories, not one: a one-coordinate fixture cannot tell "the item moved"
|
||
from "the category moved" — the same blind spot as the one-category fixture that
|
||
could not distinguish `TransposeAxes` from `CycleAxis`.
|
||
|
||
### a_surrogate_round_trips_through_json_as_a_string
|
||
|
||
`serde_json` writes a `u128` as a bare number and a JavaScript host parses every
|
||
JSON number as `f64`, so anything above 2^53 comes back **changed** — resolving
|
||
to a different record or to none. The test asserts the wire shape (a quoted
|
||
32-hex string, 34 bytes with quotes), not merely that a round trip succeeds,
|
||
because a numeric encoding round-trips fine inside Rust.
|
||
|
||
### construction_order_does_not_reach_the_hash
|
||
|
||
`CellKey` is canonically sorted and `without` preserves that order, so "the
|
||
normalized record" is a property the type guarantees rather than a convention to
|
||
maintain.
|
||
|
||
## `view/types.rs`
|
||
|
||
### view::types::tests::moving_a_category_off_the_constraining_axes_forgets_its_page_selection
|
||
|
||
improvise-z336, measured under Chromium by cycling a category all the way
|
||
round: `Pag(Espresso) -> None` (unfiltered, 63,700) `-> Row -> Col -> Page`
|
||
and **Espresso is back**, unasked, at 18,400. `set_axis` never touched
|
||
`page_selections`; only `on_category_removed` and `on_item_removed` ever
|
||
cleared one. So every route out of a filter merely **hid** it and the
|
||
selection was silently reinstated the moment the category returned to Page.
|
||
|
||
What the report and the ticket both concluded — that the browser had no way to
|
||
clear a filter — was true, and the mechanism was not the one either named.
|
||
There is **no clear-a-page-selection operation anywhere**: not in
|
||
`ModelCommand`, not in the `View` API, and **not in the TUI** either, so this
|
||
was a product gap at parity rather than a browser regression; `[` and `]`
|
||
cycle among the category's items with no "All" entry to land on. Making
|
||
`set_axis` forget the selection closes it with no new command, no new wire
|
||
variant and no new key: cycling the axis round is now genuinely a way out.
|
||
|
||
Inject the fault by deleting the `page_selections.remove`. The test asserts
|
||
**both** halves — that the selection is gone while off Page, and that it is
|
||
still gone on the way back — because the first alone is satisfied by a `View`
|
||
that merely stops *reporting* the selection while the category sits elsewhere,
|
||
which is what the code already did.
|
||
|
||
### view::types::tests::moving_a_category_from_page_to_filter_keeps_its_page_selection
|
||
|
||
The trap gotcha 27 exists to name, and the rival fix this separates out. The
|
||
obvious spelling is "clear it unless the new axis is `Page`" — and `Filter`
|
||
constrains too: a drill fixes its coordinates on `Filter`, not `Page`, and
|
||
`.improv` round-trips a selection under it. That version passes the test above
|
||
and silently breaks drill. The predicate is therefore `Axis::filters_data()`,
|
||
which is `filtering_cats()`'s own membership — "what data is visible", never
|
||
"what the user can act on".
|
||
|
||
It passes before the fix as well as after: it is there to stop the next
|
||
change. `roundtrip_tests.rs` would also have caught it, but only as a
|
||
persistence failure a long way from the line that caused it.
|
||
|
||
### a_pivot_view_still_seats_categories_row_then_column_then_page
|
||
|
||
improvise-2zq, the **negative arm**, and the load-bearing one. The fix makes a
|
||
records view seat a new category on `Axis::None`; without this test, "always seat
|
||
on `None`" would pass every assertion about records mode while quietly deleting
|
||
the auto-assignment `on_category_added` exists for.
|
||
|
||
Probing confirmed the asymmetry: seating every category on `None` fails 12 tests,
|
||
while removing the virtual bump failed **none at all** — which is how the bump
|
||
came to be wrong in two directions with nothing noticing.
|
||
|
||
### a_records_view_seats_a_new_category_on_none_and_stays_a_records_view
|
||
|
||
`on_category_added` used to bump a virtual off Row or Column to make room. With
|
||
`_Index` on Row and `_Dim` on Column that *is* records mode, so naming a category
|
||
with `N` while in records mode silently ejected the user from the grid they were
|
||
standing in and returned them to a pivot.
|
||
|
||
### a_measure_on_column_is_not_bumped_off_it
|
||
|
||
The second thing the bump broke, and the reason the fix is about virtuals in
|
||
general rather than about records mode: `_Measure` on Column is an ordinary
|
||
configuration — it is what `web/demo.improv` ships — and adding a second regular
|
||
category kicked it off the axis, making every measure vanish from the grid. The
|
||
fixture is deliberately **not** a records layout, so it exercises the pivot path
|
||
rather than the early return.
|
||
|
||
### cycle_axis_unknown_category_is_noop
|
||
|
||
improvise-t8s: `cycle_axis` panicked through `axis_of`'s `expect` when given a
|
||
category not registered with the view, while `set_axis` silently no-ops on the
|
||
same input. It now goes through `try_axis_of`.
|
||
|
||
### tileable_cats_omits_filter_but_keeps_every_other_axis
|
||
|
||
improvise-dz8: a coordinate the drill fixed must not appear in tile select — the
|
||
user cannot move it, so offering it offers an action that cannot be taken. The
|
||
test flips the category back to `Page` afterwards, so it distinguishes "omits
|
||
`Filter`" from "omits this particular category".
|
||
|
||
### filtering_cats_covers_page_and_filter_but_not_the_rest
|
||
|
||
`Filter` constrains the data exactly as `Page` does; only `Page` is navigable.
|
||
The fixture puts a category on each of `Page`, `Filter`, `Row` and `None` so the
|
||
result separates the two questions rather than merely counting.
|
||
|
||
### on_category_removed_forgets_a_collapsed_group
|
||
|
||
improvise-qx1z arm 2. `View::on_category_removed` cleared `category_axes`,
|
||
`page_selections` and `hidden_items` — but not `collapsed_groups`. The other
|
||
three lines being right there is what makes this an omission rather than a
|
||
decision: collapse `Month/Q1`, delete the category, re-add it with `Jan[Q1]`,
|
||
and the group comes back **pre-collapsed** with nothing saying why. Reachable
|
||
from shipped keys, through the category panel.
|
||
|
||
Fault injected: delete the added line; the test fails at
|
||
`is_group_collapsed("Month", "Q1")`. The test re-adds the category after
|
||
removing it, because a view that merely *forgot the axis* would also report
|
||
`false` for a category it no longer knows — asking the question after the
|
||
re-add is what separates "cleared" from "not currently registered".
|
||
|
||
### on_item_removed_forgets_a_hidden_item
|
||
|
||
improvise-qx1z arm 1, at the `View` level. `Model::remove_item` removes the item
|
||
and its cells and **touches no view at all**, so `hidden_items["Region"]` kept
|
||
`"East"`: re-adding the item put it in the model and in the category panel while
|
||
leaving it absent from the grid, with no indication why.
|
||
|
||
The fixture hides **two** items and removes one. Without the second, a fix that
|
||
dropped the whole category entry — `hidden_items.remove(cat_name)` — would pass
|
||
while destroying unrelated state; the surviving `West` assertion is the input
|
||
that separates "forget this item" from "forget this category".
|
||
|
||
### on_item_removed_forgets_a_page_selection_naming_it
|
||
|
||
The third question improvise-qx1z asked to check while there, and the answer was
|
||
the same: a page selection naming a removed item survived it. Asserted in both
|
||
directions in one test — removing a *different* item must leave the selection
|
||
alone — because a hook that cleared the selection unconditionally would satisfy
|
||
the removal half and silently reset the user's page on every unrelated delete.
|
||
|
||
## `view/layout.rs`
|
||
|
||
### prune_axis_items_indexes_data_positions_not_visual_ones
|
||
|
||
`prune_axis_items` asks `keep` about **data** indices — the positions a
|
||
`row`/`col` argument means — so a group header between two data items must not
|
||
shift the numbering the predicate sees. The fixture places the header at visual
|
||
index 1 and prunes data index 1: if the predicate saw visual indices, `"A"` would
|
||
be the one dropped. improvise-n3wt also removed the `&[bool]` + `debug_assert`
|
||
pair, whose length could disagree with the axis in release builds only.
|
||
|
||
### a_text_cell_renders_in_a_default_view
|
||
|
||
The user-visible face of improvise-l4qx, asserted at the layout rather than at
|
||
the aggregation helper. The test asserts its own premise first —
|
||
`!layout.none_cats.is_empty()` — because if a default view ever stopped putting
|
||
`_Index`/`_Dim` on `Axis::None` the test would pass without exercising the bug at
|
||
all.
|
||
|
||
### records_mode_gives_each_measure_its_own_value_column
|
||
|
||
Pins the whole column vocabulary in order:
|
||
`[Region, +cat, Revenue, Cost, +]` — the regular categories, the new-category
|
||
column (improvise-2zq, sitting where the column it creates will appear), one
|
||
column per raw measure, then the new-measure column (improvise-c5v). It checks
|
||
both the labels and the `RecordCol` variants, so a column drawn with the right
|
||
label but resolved to the wrong kind fails.
|
||
|
||
### records_mode_initial_layout_is_sorted
|
||
|
||
On entry into records mode the store is sorted by `CellKey`, so rows appear in a
|
||
deterministic order regardless of insertion order. The fixture inserts `East`
|
||
after `North`, the opposite of alphabetical, so "did nothing" fails.
|
||
|
||
### records_mode_new_record_appends_at_bottom
|
||
|
||
The complement: once the layout exists, a new record appends rather than
|
||
re-sorting into the middle. The new record's coordinate (`AAA`) sorts *before*
|
||
every existing one, so a re-sort would move it to the top and move the row the
|
||
user was looking at.
|
||
|
||
### records_display_out_of_bounds_col_returns_none
|
||
|
||
improvise-byz: `records_display` for an out-of-bounds column called
|
||
`col_label(col)`, which returns `""`; `""` was then looked up as a category name
|
||
in the record, never found, and the function returned `Some("")` — masking
|
||
view/data mismatches as blank cells. It must return `None`, symmetric with
|
||
`cell_key`.
|
||
|
||
## `view/axis.rs`
|
||
|
||
### the_label_vocabularies_are_unchanged
|
||
|
||
improvise-rml, characterization: these strings were scattered across four
|
||
render/command sites before being pulled onto `Axis`. Pinned verbatim so the move
|
||
could not quietly restyle the UI.
|
||
|
||
### filter_is_outside_the_rotation
|
||
|
||
improvise-dz8: `Filter` names a coordinate the drill fixed, so it is outside the
|
||
Row→Column→Page→None rotation entirely. Both directions are asserted — cycling a
|
||
`Filter` must not hand the user a navigable axis, **and** no navigable axis may
|
||
rotate into `Filter`, or tile select could hand one out.
|
||
|
||
### next_is_a_four_cycle_over_the_navigable_axes
|
||
|
||
Applying `next` four times is the identity from every starting point and no
|
||
navigable axis is a fixed point, so no variant is stranded or skipped. Together
|
||
with the entry above this pins the rotation without enumerating it twice.
|
||
|
||
### a_routing_formula_over_a_group_sums_the_members_answers
|
||
|
||
improvise-4zl7, and the hand-computed half of its evidence. Every number in
|
||
`route_then_aggregate_tests` is written out rather than obtained from a second
|
||
call into the evaluator, because the bug being guarded is *the evaluator
|
||
answering consistently and wrongly*.
|
||
|
||
The fixture is four records — `Cost` of 10 and 100 in North, 20 and 200 in
|
||
South — and the formula is `Pred = SUM(IF(Cost < 50, 1, 10))`. `Cost` straddles
|
||
the threshold **inside each region**, which is the whole design of the fixture:
|
||
|
||
| cell | route-then-aggregate | aggregate-then-route |
|
||
|---|---|---|
|
||
| one record (`Cost` 10) | 1 | 1 |
|
||
| North (`Tag` away) | 1 + 10 = **11** | `110 < 50` → **10** |
|
||
| everything | 1+10+1+10 = **22** | `330 < 50` → **10** |
|
||
|
||
A fixture whose records fall on the same side of the split would give the same
|
||
answer either way at the record level and differ only by a factor of the record
|
||
count, which is the kind of agreement that reads as correctness.
|
||
|
||
Two faults were injected and both were watched to fail. Restoring the old
|
||
`eval_agg` — sum the matching raw values and ignore the inner expression —
|
||
fails **7 of the 8** tests with 330 for 22, 110 for 11 and 10 for 1; the one
|
||
survivor is `is_is_one_when_the_cells_own_coordinate_matches_and_zero_otherwise`,
|
||
which has no `SUM` and so never reaches the fold. Making `eval_is` return `0.0`
|
||
unconditionally fails exactly the **3** coordinate tests, with 400 for 214 and
|
||
200 for 14. The two faults failing disjoint-but-overlapping sets is itself the
|
||
evidence that the tests separate the fold from the predicate.
|
||
|
||
### a_coordinate_split_survives_aggregating_over_that_very_category
|
||
|
||
The case the ruling exists for, and the one the ticket's own text called the
|
||
product question. With `Region` aggregated away the cell has no `Region`
|
||
coordinate of its own, so a formula that read the *cell's* coordinate would see
|
||
nothing and take `Expr::Is`'s `unwrap_or(false)` branch — answering 100 once.
|
||
The fold hands the expression a **record** key, so each of the four records
|
||
answers for its own coordinate: 7 + 7 + 100 + 100 = 214.
|
||
|
||
### a_member_whose_input_is_missing_errors_rather_than_being_dropped
|
||
|
||
The asymmetry between the `Expr::Ref` fast path and the fold, asserted rather
|
||
than left to be discovered. `SUM(Revenue)` skips a record with no `Revenue`
|
||
cell; `SUM(<expr>)` cannot, because the expression is what is being asked and an
|
||
expression that will not evaluate is an error. The fixture clears one record's
|
||
`Cost` and writes a different measure on the same record, so the record still
|
||
exists and still has a member cell to be found by — otherwise the record would
|
||
simply vanish from `matching_cells` and the test would prove nothing.
|
||
|
||
### feature_roles_line_up_with_the_names_preprocess_emitted
|
||
|
||
Lives in `gp-tree`, recorded here because the rule it guards is improvise-4zl7's.
|
||
`feature_roles` is the one statement of `preprocess`'s expansion order and
|
||
`encode_row` drives off it, so a drift is silent in both directions: the emitter
|
||
would spell the wrong feature and a re-encoded row would land in the wrong
|
||
column. The assertion rebuilds each feature's *name* from its role and compares
|
||
it with the name `preprocess` emitted, which is the only thing that ties the two
|
||
walks together.
|
||
|
||
The last four assertions exist because the loop is vacuous over a fixture that
|
||
produces only one kind of feature. The fixture has a numeric column with a
|
||
missing cell, a categorical column and a date column, so all four roles appear.
|
||
Fault injected: swapping the `Numeric` and `Missing` pushes in `feature_roles`
|
||
fails this test **and** `encode_row_replays_the_training_expansion`, which is
|
||
the pair that would have disagreed.
|
||
|
||
### a_formula_targeting_another_category_still_folds_over_its_records
|
||
|
||
Found by self-review, not by the ticket. The fold evaluates the inner expression
|
||
at each member record's key, and the first version built that key by folding the
|
||
record's coordinates onto **`context`** — which still carries the formula's own
|
||
target coordinate. For the ordinary `_Measure` target that is harmless, because
|
||
`Expr::Ref` overwrites `_Measure` on the way in. For a formula targeting any
|
||
other category it is not: the record key keeps `Levy=Tax`, so `evaluate_depth`
|
||
matches the formula again at that key and the whole cell converges to
|
||
`Error("circular")` — a formula quietly self-referencing through its own
|
||
aggregate.
|
||
|
||
The key is now folded onto `partial` (`context.without(target_category)`, plus
|
||
any inline `WHERE`), which drops the target coordinate by construction rather
|
||
than by every caller remembering to. Fault injected by restoring the `context`
|
||
fold: this test fails with `Error("circular")` against `11.0` and the other
|
||
eight pass, which is why it is a separate test and not an extra assertion on one
|
||
of them.
|
||
|
||
### the_two_evaluators_agree_when_the_fold_reads_a_derived_measure
|
||
|
||
Adversarial review of PR #54, and the sharpest finding of the round: the first
|
||
version of the fold reintroduced **exactly the divergence the ruling exists to
|
||
remove**, through the cached path.
|
||
|
||
`eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at
|
||
`context.with(cat, name)`. Inside the fold, `context` is a **member-record key**
|
||
carrying the `none_cats` coordinates, while `formula_cache` is keyed by stems
|
||
with `none_cats` **stripped**. The lookup could never hit, fell through to
|
||
`aggregate_raw`, found no raw cell behind a formula target, and answered
|
||
`Err("ref:…")`. So a routing formula reading a *derived* measure gave
|
||
`Number(11.0)` uncached and `Error("ref:Margin")` cached — and the cached path
|
||
is the one a pivot grid runs whenever any category sits on `Axis::None`.
|
||
|
||
The fixture is the four-record one plus `Margin = Cost * 2`, so the fold's input
|
||
is a formula rather than raw data. The test asserts the uncached value **and**
|
||
that the two agree; asserting only agreement would pass if both broke.
|
||
|
||
The fix makes them agree by construction rather than by this test: `eval_expr`
|
||
is hoisted to module level and **both** evaluators hand it to `eval_agg` as
|
||
`eval_at`. Note the repair that was *not* taken — teaching the fold to hit the
|
||
cache by stripping `none_cats` off the member key returns the **group's** value
|
||
to every member, which is aggregate-then-route through a cache lookup, silent.
|
||
Fault injected by restoring `eval_expr_cached` as the fold's evaluator: this
|
||
test fails with `Error("ref:Margin")` and the other eleven pass.
|
||
|
||
### the_cached_evaluator_has_its_own_working_coordinate_predicate
|
||
|
||
The same review's second finding, and a lesson about where to inject a fault.
|
||
`Expr::Is` has an arm in **each** evaluator. The original evidence injected the
|
||
fault into the shared `eval_is`, which both arms call — proving the tests see
|
||
the predicate, not that they see the cached *copy*. Replacing only the cached
|
||
arm with `Ok(0.0)` left the suite at 1330 passing.
|
||
|
||
The first attempt to close it — a coordinate split run through
|
||
`recompute_formulas` + `evaluate_aggregated` — **also** stayed green, and that
|
||
is the interesting part: once the fold routes through `eval_expr`, an `Is`
|
||
inside a `SUM` never reaches the cached arm at all. The cached arm is now
|
||
reachable only by a **bare** `Is` outside an aggregate, which is what this test
|
||
uses. Fault injected into that arm alone: it fails, `Number(0.0)` against
|
||
`Number(1.0)`.
|
||
|
||
The general shape: an injected fault proves coverage only of the code path it is
|
||
injected into. Two arms of the same rule need two injections, and if one of them
|
||
turns out to be unreachable, that is a fact about the design worth writing down.
|
||
|
||
|
||
### a_user_category_whose_name_leads_with_an_underscore_is_seated_as_regular
|
||
|
||
improvise-bidq. `View::on_category_added`'s early return read
|
||
`cat_name.starts_with('_') || self.is_records_layout()` — the **same
|
||
expression** mixing two derivations one operator apart, the left a name prefix
|
||
and the right `CategoryKind`. Four lines up, `is_records_layout` asks the enum.
|
||
|
||
`Model::add_category` does not validate names, so a user category literally
|
||
named `_Foo` was `CategoryKind::Regular` to `records_columns`, `cell_id` and
|
||
`is_empty_model`, and "virtual" to these two lines — seated on `Axis::None`,
|
||
so it never appeared in a pivot while every other consumer treated it as
|
||
ordinary. The second site (`holds_a_regular`) had the same split, which is why
|
||
the test asserts a *second* category's seat too: fixing only the first leaves
|
||
`_Foo` on Row while `Region` also takes Row.
|
||
|
||
Decided explicitly, per the ticket: `add_category` is **not** made to refuse a
|
||
leading underscore. The three reserved names are the ones `CategoryKind::of_name`
|
||
knows, `_Foo` is a legal document, and a user-visible restriction is a larger
|
||
change than making one answer out of two. The fix is that both sites now ask
|
||
`CategoryKind::of_name(..).is_virtual()`.
|
||
|
||
Fault injected by restoring either `starts_with('_')`: the test fails, `None`
|
||
against `Row` on the first site and `Row` against `Column` on the second.
|
||
|
||
### an_ungrouped_item_after_a_grouped_run_belongs_to_no_group
|
||
|
||
### a_grouped_run_does_not_leak_into_the_next_prefix
|
||
|
||
improvise-gd83, the two arms of one defect. `expand_category` emitted a
|
||
`GroupHeader` on **entering** a group and no boundary on leaving one, so
|
||
`row_group_for`/`col_group_for`'s nearest-preceding walk answered with a group
|
||
that had already ended.
|
||
|
||
The arms are separate because the two escapes are different and a fix can close
|
||
one and miss the other. The first is *within* one expansion — `Jan[Q1]`,
|
||
`Feb[Q1]`, `Sep` — closed by emitting `AxisEntry::GroupEnd` on a
|
||
`Some -> None` transition. The second is *across* expansions: `cross_product`
|
||
expands the inner category once per prefix, each call starting with
|
||
`last_group = None`, so `Food/Bob[Family]` is immediately followed by
|
||
`Clothing/Carol`, which is ungrouped and inherited Family with no transition
|
||
anywhere to hook onto. That one needs a `GroupEnd` at the **end** of an
|
||
expansion whose last item was grouped.
|
||
|
||
`GroupEnd` is deliberately NOT emitted on `Some(A) -> Some(B)`: B's own header
|
||
already ends A's run, and emitting one there moved
|
||
`data_row_to_visual_skips_headers` and `data_col_to_visual_skips_headers` for
|
||
no gain. Those two are untouched, which is the evidence the boundary is as
|
||
narrow as it claims.
|
||
|
||
Fault injected by deleting either emission: the corresponding test fails with
|
||
`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and
|
||
`the_two_front_ends_draw_the_same_column_header` fails with it.
|
||
|
||
### a_collapsed_groups_header_survives_pruning
|
||
|
||
### a_header_whose_items_all_prune_away_is_dropped
|
||
|
||
### a_partly_pruned_group_keeps_the_header_its_survivors_sit_under
|
||
|
||
### an_ungrouped_item_after_a_pruned_group_belongs_to_no_group
|
||
|
||
improvise-lhi4 and improvise-u6gc: two defects in one `match` block, and they
|
||
pull in opposite directions, which is why they were fixed together. The naive
|
||
repair of the first — stop pruning headers — deepens the second.
|
||
|
||
`prune_axis_items` kept a header only if a data item under it survived the
|
||
predicate, and dropped a `GroupEnd` whenever the header it closed was still
|
||
pending. Both readings are wrong about a case the entry stream can distinguish.
|
||
|
||
**lhi4**: a collapsed group contributes **zero** `DataItem`s — `expand_category`
|
||
emits the `GroupHeader` and then `continue`s past every item — so its header
|
||
was always pruned and the group vanished from the grid entirely. `z`
|
||
(toggle-group-under-cursor) resolves the group from `row_group_for(cursor)`,
|
||
so with no header and no items no row named it: collapse became one-way from
|
||
the grid, recoverable only through `:toggle-group <cat> <group>`.
|
||
|
||
**u6gc**: dropping a pruned header's `GroupEnd` **re-opens an earlier run**.
|
||
With `Jan[Q1], Feb[Q1], Mar[Q2], Sep` and `Mar` empty, the kept list became
|
||
`[H(Q1), Jan, Feb, Sep]` and the nearest-preceding walk answered `Q1` for
|
||
`Sep` — exactly the inheritance improvise-gd83 added `GroupEnd` to prevent, so
|
||
the regression was live whenever `prune_empty` was on.
|
||
|
||
The fix distinguishes the two emptinesses **without consulting the `View`**,
|
||
which `GridLayout` has already dropped: a run that contained no `DataItem` at
|
||
all was never expanded, i.e. collapsed, and its header survives; a run that
|
||
contained items the predicate rejected is genuinely empty and goes. **Superseded
|
||
twice** (see `an_outer_group_whose_rows_all_prune_loses_its_header`). Since
|
||
improvise-6qet a collapsed group emits its fold as a `DataItem`, so no flat run
|
||
is item-less. The one run that still was, an outer group's header followed
|
||
directly by an inner group's header, was kept by that arm when it should have
|
||
gone (improvise-b5zh). The arm is deleted. A
|
||
`GroupEnd` is kept whenever a run is open in the output, so a re-opened earlier
|
||
header is closed even though the header between them was pruned. That keeps
|
||
gotcha 1's shape — still a predicate over the data index, no `&View`
|
||
parameter, no flag vector.
|
||
|
||
The tests are separate because each is satisfied by a fix that misses another.
|
||
`a_header_whose_items_all_prune_away_is_dropped` **passes before the change**
|
||
and is the discriminator: it is what stops "keep every header" from counting as
|
||
a fix for lhi4, and it is the acceptance criterion lhi4 names.
|
||
|
||
`a_partly_pruned_group_keeps_the_header_its_survivors_sit_under` was added by
|
||
self-review and covers the **commonest** case, which the other three do not
|
||
reach: a group where some items prune and some do not. All three of the others
|
||
put a group in an all-or-nothing state, so between them they said nothing about
|
||
the mixed one — and it is the case a user meets first.
|
||
|
||
Reviewing it turned up a fact worth keeping about the shape of the code rather
|
||
than a defect. Rewriting `run_had_item` to mean "some item was **rejected**"
|
||
instead of "there was an item" is a **no-op**, and the whole workspace stays
|
||
green: the header is pushed eagerly by the *first surviving item*, so
|
||
`pending_header` is already `None` by the time any rejection can be recorded,
|
||
and the flag is therefore only ever read in runs where **nothing** survived —
|
||
where "no item at all" and "no item accepted" coincide. The eager push is what
|
||
makes the collapsed/pruned distinction robust rather than delicate, and a
|
||
reviewer trying to break it by attacking that flag is attacking the wrong half.
|
||
|
||
`a_collapsed_groups_header_survives_pruning` asserts the **whole** `row_items`
|
||
sequence rather than `any(|e| is Q1 header)`. An `any()` cannot see a header
|
||
emitted in the wrong position or an extra entry beside it; the sequence can.
|
||
Note what it still cannot claim: two consecutive `GroupEnd`s are not reachable,
|
||
because `expand_category` never emits them, so "no stray `GroupEnd`" is a
|
||
structural property of the writer and not something any fixture falsifies.
|
||
|
||
Its fixture carries an **unfunded `Mar`** for a reason integration review
|
||
found: the first version funded every item, so nothing was ever pruned and the
|
||
test **passed with `prune_empty` disabled entirely** — the name claimed more
|
||
than the test could see, which is the same vacuity class as the `any()` above.
|
||
With `Mar` present, turning pruning off leaves `DataItem(["Mar"])` in the
|
||
sequence and the assertion fails, so the fixture now demonstrates that pruning
|
||
ran.
|
||
|
||
Three faults injected against the fixed code, measured over all four tests
|
||
(`c` = collapsed, `p` = prune-away, `g` = GroupEnd, `m` = partly-pruned;
|
||
`✗` = fails, which is the point):
|
||
|
||
| injected fault | c | p | g | m |
|
||
|---|---|---|---|---|
|
||
| stop keeping the `GroupEnd` (drop the `run_open` push) | ✗ | ok | ✗ | ✗ |
|
||
| drop `!had_item` — keep every pending header | ok | ✗ | ok | ok |
|
||
| invert to `had_item` — keep only fully-pruned headers | ✗ | ✗ | ok | ok |
|
||
|
||
Every column is distinct, so no test here stands in for another. In
|
||
particular the **third row separates the collapsed test from the `GroupEnd`
|
||
test** — integration review reported that no fault did, which does not
|
||
reproduce against this tree; inverting the condition fails `c` while `g`
|
||
passes, because a re-opened run is closed by the `GroupEnd` arm whatever the
|
||
header condition says. The two membership failures read `Some(("Month",
|
||
"Q1"))` against `None`.
|
||
|
||
Funding is **per fixture and deliberate in both directions**. The three
|
||
all-or-nothing fixtures fund every item they expect to survive, because on a
|
||
wholly unfunded fixture every row prunes and the tests would assert over an
|
||
empty grid — `row_count()` is asserted first in each for that reason. The
|
||
collapsed fixture instead carries one *unfunded* `Mar`, so that pruning is
|
||
demonstrably running: see its own note above.
|
||
|
||
### a_trailing_conversion_letter_does_not_erase_the_precision
|
||
|
||
improvise-711n. `parse_number_format` read the precision as
|
||
`fmt[i+1..].parse::<u8>().ok().unwrap_or(0)` over everything after the last `.`,
|
||
so any trailing conversion letter made the whole suffix unparseable and the
|
||
format fell back to **zero decimals, silently**. `format: ,.2f` — the spelling
|
||
`context/repo-map.md`'s own `.improv` example carries, and the printf/Python
|
||
convention a user reaches for first — displayed integers everywhere.
|
||
|
||
Fails before the fix with `left: (true, 0)`, `right: (true, 2)`.
|
||
|
||
The test also pins the reading against its nearest *wrong* neighbour. Two
|
||
implementations both fix `,.2f`: take the leading digit run, or trim the
|
||
trailing non-digits off the tail. They differ only on a tail with digits on both
|
||
sides of a letter, so `".2f3"` is in the assertion. Injecting
|
||
`s.trim_end_matches(|c: char| !c.is_ascii_digit())` in place of `leading_digits`
|
||
fails the test at `left: (false, 0)`, `right: (false, 2)` — so the test sees the
|
||
rule, not merely the bug.
|
||
|
||
### a_zero_padded_picture_string_is_read_as_printf_not_as_excel
|
||
|
||
The companion to the above, and the reason the fix is not "be generous". A
|
||
forgiving reader would have to answer `.00`, where printf says precision zero
|
||
and an Excel picture says two places. This pins the printf reading — `"0.00"` is
|
||
`(false, 0)` — so that "accept the trailing `f`" cannot quietly grow into
|
||
"accept anything and guess". It passes before the fix as well as after: it is
|
||
there to stop the *next* change, not to catch this one.
|
||
|
||
### a_precision_too_large_to_name_reads_as_none
|
||
|
||
`".300"` does not fit `u8`, so it reads as no precision rather than as 300
|
||
decimal places. Unchanged by improvise-711n and pinned so the fallback is a
|
||
stated rule rather than an artefact of the integer type.
|
||
|
||
### a_format_naming_no_precision_at_all_reads_as_zero
|
||
|
||
`",f"` has no `.`, and `".f"` has a `.` with no digits after it. Both are zero
|
||
decimals. Pinned because `leading_digits` returns `""` for the second, and an
|
||
empty string reaching `parse::<u8>()` is the one path in the function with no
|
||
digits to carry the answer.
|
||
|
||
---
|
||
|
||
## `workbook.rs`
|
||
|
||
### workbook::tests::removing_a_measure_row_that_is_only_a_formula_target_removes_the_formula
|
||
|
||
improvise-g3no through the facade both front ends actually reach —
|
||
`effect::RemoveItem` calls `Workbook::remove_item`, never `Model::remove_item`
|
||
— so this is the one that says the fix arrives where the keypress lands. Its
|
||
leaf name is shared with the `model/types.rs` test below, hence the
|
||
qualification.
|
||
|
||
It asserts `effective_item_names("_Measure")` **by value** rather than
|
||
`!contains("Profit")`, so a fix that removed the formula and also lost
|
||
`Revenue` or `Cost` on the way fails here rather than passing a negative.
|
||
|
||
---
|
||
|
||
## `format.rs` — the decimal default and a formula's own format (improvise-e0ex)
|
||
|
||
### a_ratio_keeps_its_decimals_under_a_fresh_views_format
|
||
|
||
The bug the ticket reports, at the smallest place it is visible.
|
||
`View::new`'s `number_format` was `,.0` — *zero* decimals — so `2/3` reached
|
||
`format_f64` and came back as `"1"`, half-away-from-zero. Written before the
|
||
fix and observed failing with `left: "1" right: "0.67"`.
|
||
|
||
It asks the **view's default** rather than a literal `",.2"`, so the assertion
|
||
is about the shipped default and not about `parse_number_format`, which already
|
||
had its own tests. Reverting `View::new` to `,.0` fails it.
|
||
|
||
What it deliberately does not assert: that the default is exactly two places.
|
||
`0.67` would also fail under `.1` or `.3`, which is the point — the test names
|
||
"a ratio keeps its decimals", and the precise number is a product decision
|
||
recorded in `context/code/core.md`, not a property.
|
||
|
||
### a_formulas_own_format_wins_for_its_target_and_nowhere_else
|
||
|
||
The override, and both halves of "nowhere else" — a raw measure beside the
|
||
formula target, and a key with no `_Measure` coordinate at all (the aggregating
|
||
cell). The fixture uses `.4` against a `,.2` view so the two answers differ in
|
||
*digits*, not merely in grouping.
|
||
|
||
Two faults injected, in opposite directions, because one alone is not enough:
|
||
|
||
- `FormatPlan::at` returning `self.view` unconditionally (the override never
|
||
applies) fails with `left: "0.67" right: "0.6667"`.
|
||
- `FormatPlan::at` returning the first entry in the plan for any key (the
|
||
override leaks to every cell) fails with `left: "0.6667" right: "0.67"`.
|
||
|
||
A test asserting only the target's own column passes the second fault, which is
|
||
why the two neighbours are in the same test rather than in a sibling that could
|
||
be deleted on its own.
|
||
|
||
### a_formula_naming_no_format_leaves_its_target_on_the_views
|
||
|
||
The fallback, asserted where it is easiest to lose: a formula target whose
|
||
formula carries no format must be indistinguishable from a raw measure. It
|
||
keeps `FormatPlan::of` from indexing every target with some default and then
|
||
"overriding" it back, which would be a second copy of the view's answer.
|
||
|
||
### the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none
|
||
|
||
Found by review (ChatGPT Codex, PR #125), not by the tests above, which is the
|
||
entry's point: they used one formula and so could not see an ownership question
|
||
at all.
|
||
|
||
A target category need not be `_Measure` — `- Tax = Revenue * 0.1 [CustomCat]`
|
||
is in the format's own documentation — so a key may carry coordinates two
|
||
formulas claim. The first `FormatPlan` indexed **only the formatted** formulas,
|
||
so a later formula's `FORMAT` reached a cell whose value an earlier, unformatted
|
||
formula produces: the number and its spelling came from different formulas.
|
||
|
||
The fixture puts the same two formulas in both orders and asserts the owner's
|
||
answer each time, so a plan that prefers the formatted one fails the first half
|
||
and a plan that ignores formats entirely fails the second.
|
||
|
||
Fault injected: `.filter(|t| t.format.is_some())` before the `find` — precisely
|
||
the behaviour of the version review rejected. Fails with `left: "0.6667"
|
||
right: "0.67"`.
|
||
|
||
### the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does
|
||
|
||
The other half of the same finding, and the one a single run cannot see.
|
||
`by_target` was a `HashMap<category, HashMap<item, _>>` and `at` iterated it, so
|
||
with two claiming categories the answer came from whichever hashed first —
|
||
**a different answer per process**, gotcha 18's shape exactly (an index whose
|
||
values are hashed is for lookup, never for choosing between candidates).
|
||
|
||
It rebuilds the plan five times rather than asserting once, because each
|
||
`HashMap` gets its own `RandomState` and a single construction can be right by
|
||
luck. It is the same shape as `analyze_records`' five-iteration guard.
|
||
|
||
This one cannot be made to fail by re-introducing a `HashMap` *reliably*, which
|
||
is why the test that carries the load is its sibling above; kept because the
|
||
repetition is what documents that hash order was the defect.
|
||
|
||
### a_view_created_after_a_label_still_keeps_it_off_the_axes
|
||
|
||
`View::on_category_added` took a name and asked `CategoryKind::of_name`, which
|
||
reads `Label` as `Regular` by design. `Workbook::add_label_category` papered
|
||
over that by calling `set_axis(name, Axis::None)` straight afterwards, so the
|
||
views existing *at that moment* were right; `Workbook::create_view` had only
|
||
names to hand and had no such correction, so every view created after the label
|
||
pivoted it. Reproduced at `d46ec59`: `Description` on `Axis::Row` in a view
|
||
created one line after `add_label_category`.
|
||
|
||
The fix gives seating the kind, and the two `set_axis` corrections are deleted —
|
||
one answer rather than two, of which only one caller knew to write the second.
|
||
|
||
### a_view_created_after_a_regular_category_still_seats_it_on_row
|
||
|
||
The control, and the direction the bug's own shape cannot supply. Seating
|
||
everything on `Axis::None` satisfies the label assertion above completely, so
|
||
without this a blanket `unseated = true` reads as a fix.
|
||
|
||
Fault injected: `let unseated = true;`. This test fails (`left: None right:
|
||
Row`) along with `a_label_does_not_displace_a_regular_category_in_a_new_view`
|
||
and `first_category_goes_to_row_second_to_column_rest_to_page`, while every
|
||
label assertion in the repository still passes.
|
||
|
||
### a_label_does_not_displace_a_regular_category_in_a_new_view
|
||
|
||
The consequence the per-category assertions cannot state, because seating is
|
||
**order-dependent**: the first regular category takes Row, the second Column,
|
||
the rest Page. A label wrongly counted as regular therefore does not merely sit
|
||
in the wrong place — it *consumes the slot* the next real category wanted, and
|
||
on an already-pivoted document it pushes that category to `Page`, which
|
||
filters the whole document down to one value.
|
||
|
||
The fixture adds the label first and the regular category second, so the
|
||
displacement is what is measured rather than the label's own axis.
|
||
|
||
Fault injected: restoring `let unseated = CategoryKind::of_name(cat_name)
|
||
.is_virtual() || self.is_records_layout();`. Five tests fail, including the two
|
||
pre-existing ones (`add_label_category_sets_none_axis_on_all_views`,
|
||
`label_category_defaults_to_none_axis`) that used to be carried by the deleted
|
||
`set_axis` correction — which is the evidence that the consolidation did not
|
||
cost coverage.
|
||
|
||
---
|
||
|
||
## `view/types.rs`
|
||
|
||
### view::types::tests::an_unused_view_name_steps_past_every_name_already_held
|
||
|
||
improvise-vw47. `unused_view_name` is the one derivation both user-facing
|
||
"create a view" resolvers call, and the whole of what it must get right is that
|
||
the *count* of views is not a free *name*. Deleting `View 2` from `Default`,
|
||
`View 2`, `View 3` leaves two views and the name `View 3` taken, so the old
|
||
`format!("View {}", len + 1)` named a view that already existed and
|
||
`Workbook::create_view` — which replaces, deliberately, for the `_Records` and
|
||
`_Drill` rebuilders (`context/code/core.md` §11) — reset it to defaults.
|
||
|
||
Four cases, and only the last three discriminate. `["Default"] -> "View 2"` and
|
||
`["Default", "View 2"] -> "View 3"` pin the no-gap behaviour so the fix cannot
|
||
be paid for by renaming the ordinary case; `["Default", "View 3"] -> "View 4"`
|
||
is the bug; the run `["Default", "View 3", "View 4", "View 5"] -> "View 6"`
|
||
separates "searches" from "bumps once", which a single-gap fixture cannot. The
|
||
last case, `["Default", "Sales"] -> "View 3"`, pins that a name outside the
|
||
`View <n>` family still counts toward the starting guess — the helper counts
|
||
views, it does not parse names.
|
||
|
||
Two rivals were injected and both fail here or one level up. Starting the
|
||
search at `1` rather than at the count fails this test's first case **and** the
|
||
pre-existing `create_and_switch_view_names_incrementally`: it renames the first
|
||
view a fresh workbook creates. Making `Workbook::create_view` refuse or rename
|
||
a name it already holds passes this test and breaks records mode, because both
|
||
rebuilders depend on the replacement — which is why the fix is a helper beside
|
||
`create_view` and not a change to it.
|
||
|
||
### model::cell::nonfinite_cell_values::a_nonfinite_number_survives_the_json_wire
|
||
|
||
improvise-55v2. `serde_json` writes a non-finite `f64` as `null`, and `null`
|
||
cannot be read back as one — so `CellValue::Number(f64::INFINITY)` serialized to
|
||
`{"Number":null}` and the frame carrying it was unreadable at the far end. The
|
||
websocket reader **skips** an unparseable frame and the worker reports only a
|
||
console error, so one poisoned cell silently dropped a whole batch. `to_string`
|
||
returned `Ok`, so nothing at the send site could notice either.
|
||
|
||
**The design question the ticket left open is already answered by the format**,
|
||
and measuring that was the useful half of this work. `.improv` carries
|
||
non-finite numbers as first-class values: `format_number` writes `inf`, `-inf`
|
||
and `nan`, the grammar's `bare_value` accepts them, and the walker maps them
|
||
back — all three round-trip losslessly, verified before anything was changed.
|
||
So "refuse them before sending" would make a legal saved document unprojectable
|
||
to a browser, and the remaining option is the one taken: give the wire an
|
||
explicit representation. It also means `resolve.rs`'s `typed()` accepting `inf`
|
||
is correct rather than the second half of a defect.
|
||
|
||
The tokens are therefore **one home**, `nonfinite_token`/`parse_nonfinite` in
|
||
`improvise-core`, called by the serde adaptor, by `persistence/format.rs` and by
|
||
`persistence/parse.rs` — where three separate copies stood before and a fourth
|
||
was about to be written. `format_f64` is deliberately not folded in: it is the
|
||
display layer (gotcha 8) and may legitimately want a different glyph.
|
||
|
||
Three rivals:
|
||
|
||
- **A plain `derive` on the `f64`** — the state at HEAD — fails this test at
|
||
`left: "{\"Number\":null}"` and also fails the io-side agreement test.
|
||
- **Drifting the shared token** (`-inf` → `-infinity`) fails this test, the
|
||
agreement test, and `generated_file_roundtrips`, because the writer and the
|
||
reader stop agreeing.
|
||
- **Re-copying the tokens into `persistence/format.rs`** so there are two homes
|
||
again, drifted, leaves *this* test passing and fails the agreement test and
|
||
the grammar round-trip. That is the rival the agreement test exists for: a
|
||
second home that happens to agree is harmless, and one that does not is what
|
||
must fail.
|
||
|
||
The companion `an_ordinary_number_is_still_a_json_number` is the direction the
|
||
fix could quietly break: it pins `{"Number":1.5}` exactly, and that a bare JSON
|
||
integer still reads as a number rather than being mistaken for a token.
|
||
|
||
### persistence::nonfinite_agreement::a_nonfinite_cell_is_spelled_the_same_on_disk_and_on_the_wire
|
||
|
||
improvise-55v2's other half. It writes each of the three values through
|
||
`format_md`, reads the token off the `## Data` line, reads the token off
|
||
`serde_json`, and requires them equal — then reparses and requires the value
|
||
back. It is the guard that makes "one home" checkable rather than asserted: the
|
||
two carriers are compared against each other, so a future second copy fails here
|
||
exactly when it drifts, which is the only time it matters.
|
||
### model::types::formula_tests::count_aggregation
|
||
|
||
improvise-a36c. The test asserted `val >= 1.0` for `COUNT(Sales)` at
|
||
`Region=East`, where the fixture's answer is exactly `1.0` — so every inflated
|
||
count cleared the bar. The ticket's own fault injection (`values.len() + 999.0`
|
||
in `eval_agg`) is not the worst case: it fails one *other* test,
|
||
`model::types::agg_helper_tests::eval_agg_over_no_rows_is_zero_only_where_an_identity_exists`,
|
||
which sees the empty case move from `0` to `999`. Preserve the empty case
|
||
(`if values.is_empty() { 0.0 } else { len + 999.0 }`) and the **entire
|
||
workspace of 1699 tests passes** while `COUNT` over any non-empty set is wrong
|
||
by 999. That measurement, not the ticket's, is what this entry exists to record:
|
||
the only thing standing between a wrong `COUNT` and a green suite was a check on
|
||
the identity element, which is the one input `COUNT` gets right by accident.
|
||
|
||
Two assertions, because one does not separate the neighbouring question.
|
||
`assert_eq!(at East, 1.0)` answers *is the count inflated* — it fails the
|
||
`+999` rival at `left: Some(1000.0)`. It does **not** answer *does `COUNT` count
|
||
anything*: a `Count` arm returning a constant `1.0` satisfies it, and at a
|
||
fixture whose expected value is `1` that rival is invisible by construction.
|
||
The second assertion, `COUNT` unscoped over all three regions being `3.0`,
|
||
is what fails it, at `left: Some(1.0)`. Both rivals were injected and both fail,
|
||
at different assertions — the first at the East count, the second at the total.
|
||
|
||
The fixture is deliberately asymmetric: one matching cell under the scoped key
|
||
and three under the unscoped one, so no single constant can satisfy both.
|
||
|
||
## `view/layout.rs`, `model/types.rs` — a collapsed group is one aggregate row (improvise-6qet)
|
||
|
||
Reasoning in `context/code/core.md` §14. The bug is one statement: `expand_category`
|
||
`continue`d past every item of a collapsed group, so the group had no data row and
|
||
the cursor-addressed `z` could never name it. Restoring that `continue` against the
|
||
fixed tree fails fifteen tests across `improvise-core`, `improvise-io` and
|
||
`improvise-command` — every test below that touches the layout.
|
||
|
||
### a_collapsed_group_contributes_one_aggregate_row
|
||
|
||
The inversion of `collapsed_group_has_header_but_no_data_items`, which asserted the
|
||
premise 6qet changed and was rewritten rather than deleted. **Two** groups, one
|
||
collapsed and one not, so `folds_a_group` is asserted `true` for the fold and
|
||
`false` for `Apr` under the open group — a predicate that answered `true` for
|
||
everything under a header would pass a one-group fixture. Also asserts the fold
|
||
sits under its own header (`row_group_for(0) == Q1`), which is what lets `z` name
|
||
it.
|
||
|
||
### a_non_contiguous_collapsed_group_folds_once
|
||
|
||
`A[G1], B[G2], C[G1]`: a group's items need not be contiguous. Rival: dedup only
|
||
against the *previous* group (right for the contiguous case, which every other
|
||
fixture is) — `G1` then folds twice, under two headers, and only this test fails.
|
||
|
||
### a_category_nested_under_a_collapsed_group_is_folded_too
|
||
|
||
`Month × Region` on Row, `Q1` collapsed: `Q1/North` is a fold too, because no
|
||
record carries `Month=Q1` whatever sits beside it. Rival: the `collapsed` flag not
|
||
inherited down `cross_product` — only this test fails, and without it the TUI would
|
||
offer `Q1/North` for editing.
|
||
|
||
### model::types::model_tests::a_collapsed_groups_cell_aggregates_its_items
|
||
|
||
`Jan=3, Feb=4` under `Q1`, `Apr=100` under `Q2`: the fold is `7`. That value
|
||
separates the real fold from absent (`None`), from pass-through of the first member
|
||
(`3`), from a count (`2`) and from a fold that leaks another group (`107`).
|
||
|
||
Also the witness for the trap the first implementation of this ticket fell into:
|
||
reading membership off `Category::groups` rather than each item's `group` field.
|
||
`add_item_in_group` registers no `Group`, so that rival answers "not a group" for
|
||
this fixture and the value is absent — this test, the next, and
|
||
`a_collapsed_groups_header_survives_pruning` fail (the fold's row prunes as empty).
|
||
|
||
### model::types::model_tests::a_groups_stored_values_fold_once_not_per_member
|
||
|
||
Under `Axis::None` (`Store`), `Avg` and `Count` over `Jan/A=2, Jan/B=4, Feb/A=9`.
|
||
The group's average is `5` and its count `3`; folding the members' aggregates gives
|
||
`avg(3, 9) = 6` and `count(2, 1) = 2`. Rival: always fold members' results (what the
|
||
formula-target branch did before improvise-eccl) — this test fails. `Sum` cannot see
|
||
the difference, which is why the fixture sets `measure_agg`.
|
||
|
||
### model::types::model_tests::an_item_named_like_a_group_is_read_as_the_item
|
||
|
||
An item literally named `Q1` beside the group `Q1`: the item wins and reads `50`, so
|
||
an existing document with such an item is unchanged. Rival: drop the
|
||
`items.contains_key` short-circuit — the key expands to `Jan + Feb = 7` and this
|
||
test fails.
|
||
|
||
#### `a_collapsed_groups_header_survives_pruning`, re-aimed
|
||
|
||
Re-aimed by improvise-6qet (the entry above, under improvise-lhi4, still holds for
|
||
why the fixture carries an unfunded `Mar`). The expected sequence now includes the
|
||
fold, `DataItem { ["Q1"], collapsed: true }`, and `row_group_for(0)` is `Q1` — the
|
||
fold is the row `z` names the group from. Note the narrowing this implies: an
|
||
**empty** collapsed group now has an empty fold row, which `prune_empty` drops, and
|
||
its header goes with it. lhi4 kept that header because it was the only way back to
|
||
the rows; the fold is now that way back.
|
||
|
||
### an_outer_group_whose_rows_all_prune_loses_its_header
|
||
|
||
improvise-b5zh asked whether `close_pending_run`'s "a header whose run saw no item
|
||
is collapsed, and survives" arm was dead since improvise-6qet, and proposed
|
||
deleting it. It was **not dead**, and where it ran it was wrong. With two grouped
|
||
categories on one axis (`Region` with `North[Upper]`, `South[Lower]`, outside
|
||
`Month` with `Jan`, `Feb` in `Q1`), `cross_product` puts each outer header
|
||
directly before an inner one: `[Upper, Q1, North/Jan, …]`. The inner header
|
||
closed the outer header's run as "saw no item", so `Upper` was kept even with
|
||
every North row pruned. At `1a6b877` the pruned axis was
|
||
`[Upper, GroupEnd, Lower, Q1, South/Jan, South/Feb]`, an orphan header drawn
|
||
over nothing. The fix keeps the pending headers as a stack: a header closes the
|
||
pending header of its **own** category and everything nested inside it, a kept
|
||
item flushes the whole stack, and a `GroupEnd` drops the innermost. Rival: a
|
||
header clears every pending header, which is the flat reading of "a header ends
|
||
the previous run". Only this test fails, because `Lower` is lost when `Q1`
|
||
arrives. The flat-case tests (lhi4, u6gc, the partly-pruned group) pass
|
||
unchanged under both versions, which is why none of them could see this.
|
||
|
||
One pre-existing shape is left alone and stated: two consecutive `GroupEnd`s
|
||
(an inner and an outer run ending together) keep only the first, because
|
||
`run_open` is a single flag. That was true before this change, pruned or not.
|
||
|
||
### records_steps_are_the_spine_then_every_non_index_dim_category_on_none
|
||
|
||
improvise-t0ma. Before this, `records_steps` was guarded by one incidental test: the
|
||
`is_index_or_dim() → is_measure()` rival failed only
|
||
`save_with_unfinished_record_is_readable`, a persistence test that happens to enter
|
||
records mode. This asserts the **steps**, not the effects `cmd/grid.rs` translates
|
||
them into, because the steps are the model-side answer a browser will also consume.
|
||
The expected list filters `_Index`/`_Dim` by **name**, not by `CategoryKind`, so it
|
||
is not a second copy of the predicate under test, and the setup asserts `_Measure`
|
||
and a regular category are both present, since a model with neither could not tell
|
||
the two predicates apart. Rival: `is_measure()` in place of `is_index_or_dim()`.
|
||
This test fails, because `_Measure` is dropped and `_Index`/`_Dim` are put on `None`
|
||
after the spine that just placed them.
|
||
|
||
### drill_steps_filter_each_coordinate_and_free_the_rest
|
||
|
||
improvise-t0ma's companion for `drill_steps`. The fixture includes a `none_cat` that
|
||
the key also fixes (`Region`) and one that is virtual (`_Index`), so both exclusions
|
||
are exercised. It also lists the key's coordinates out of sorted order, which checks
|
||
that the steps follow `CellKey`'s canonical order. Rival: `Axis::Page` in place of
|
||
`Axis::Filter` for a fixed coordinate, which reopens improvise-dz8 (a drilled
|
||
coordinate the user could page away from). This test fails.
|
||
|
||
### a_folds_ratio_formula_is_the_ratio_of_its_folded_inputs
|
||
|
||
improvise-eccl. `Margin = (Revenue - Cost) / Revenue` with Jan `10/5` and Feb `30/3`
|
||
under `Q1`, and Apr in `Q2` with no data, so the Q1 fold is row 1. The member ratios
|
||
are 0.5 and 0.9, so the three candidate rules give three different answers: summed
|
||
1.4, averaged 0.7, pooled `(40 - 8) / 40` 0.8. The expected value is the **pooled**
|
||
one, and it is also asserted equal to the same document with Month on `Axis::None`,
|
||
which is the ticket's criterion. The None-axis document works because Apr carries
|
||
no data, so the None axis pools exactly Q1's months. At `a164726f` the fold read
|
||
**1.4**. The ticket's own fixture (0.5 and 0.5) would have separated sum from
|
||
pool, but not mean from pool. The second formula set chains
|
||
(`Profit = Revenue - Cost`, `Margin = Profit / Revenue`), so a reference to
|
||
another formula target is read over the fold as well. A rule that folds a formula
|
||
ref's members while pooling raw refs fails there.
|
||
|
||
### a_folds_aggregate_formula_pools_its_members_records
|
||
|
||
The guard on the other side of improvise-eccl. Once a formula at a fold is
|
||
evaluated at the group key, an aggregate inside it (`SUM`, `MAX`) must pool every
|
||
covered member's records, because `Month=Q1` matches no record. `MAX(Revenue)` is
|
||
the discriminating case. It **failed before the change too**: the old rule folded
|
||
the members' `MAX`es with the measure's `Sum`, giving 40 where the answer is 30.
|
||
`SUM` alone agrees under the old rule and the new one. Rival: `contexts_of` stops
|
||
pooling at a fold (`Reach::Fold { .. } => Vec::new()`). This test alone fails.
|
||
|
||
### collapsing_a_group_with_a_hidden_member_leaves_the_total_where_it_was
|
||
|
||
improvise-f2oo. `Q1 = {Jan 3, Feb 4}`, `Q2 = {Apr 100}`, Feb hidden. Expanded, the
|
||
drawn rows total 103. At `a164726f` collapsing gave a Q1 fold of 7 and a total of
|
||
**107**: `z` moved the total. The ruling: a fold covers the members the grid
|
||
draws. The test asserts 103 both ways and a fold of 3. It then hides Jan too and
|
||
asserts that there is no Q1 row and a total of 100. That half already held, and it
|
||
is the inconsistency the ticket named: before, a hidden value appeared only while
|
||
a sibling was visible. Rival: `GridLayout::evaluate` covering every member
|
||
(`&|_, _| true`). This test alone fails, with 107.
|
||
|
||
### a_folds_aggregate_filtered_on_the_folded_category_counts_each_record_once
|
||
|
||
Found by the ChatGPT Codex review of PR #203, at head `14acf46d`. Pooling an
|
||
aggregate over a fold's member contexts ran `agg_values` once per member, and an
|
||
inline `WHERE` on the **folded** category overwrites that coordinate, as it does
|
||
at a record. Every member therefore became the same record set.
|
||
`SUM(Revenue WHERE Month = "Jan")` on the Q1 fold read **20**, Jan twice. The
|
||
test asserts 10, and equality with the Month-on-None document. The fix dedupes
|
||
the member contexts after the filter is applied, so each record set is pooled
|
||
once. The rival is the pre-fix code, which read 20. `a_folds_aggregate_formula_pools_its_members_records`
|
||
could not see this, because none of its aggregates carried a filter.
|
||
|
||
### a_fold_evaluates_each_formula_it_reaches_once
|
||
|
||
improvise-o1y0, from PR #203's review. A reference from a fold's formula to
|
||
another formula target is evaluated over the fold in turn, with nothing
|
||
remembered, so a doubling chain `F{i} = F{i-1} + F{i-1}` re-evaluated
|
||
exponentially. The test **counts** formula evaluations rather than timing them,
|
||
through a test-only thread-local counter. At `2a4f35d7` evaluating `F8` on one
|
||
fold took **511** evaluations (2⁹ − 1); with the memo it takes 9. It also
|
||
asserts the chain's value (7 × 256), so a memo that returned the wrong entry
|
||
cannot pass. Rival: keep the in-progress marker but drop finished values from
|
||
the memo (`.filter(Option::is_none)`). This test fails with 511.
|
||
|
||
### a_cycle_at_a_fold_is_still_circular
|
||
|
||
improvise-o1y0's other half. `P = Q + 1`, `Q = P + 1` at a fold must read
|
||
`Error("circular")`. The memo's in-progress marker is what answers it now,
|
||
because the depth bound the fold path used before is gone. Without the marker
|
||
the evaluation recurses until the stack runs out, which is why the marker is
|
||
there rather than being an optimisation.
|
||
|
||
### one_formula_at_two_fold_keys_is_evaluated_at_each
|
||
|
||
improvise-d7aj, found by the adversarial re-review of PR #203 at `b3379d1a`. The
|
||
fold memo in `fold_formula_once` is keyed by the whole `CellKey`, and nothing
|
||
pinned that: both tests above keep one key context throughout, so a memo keyed
|
||
by the formula alone passed the whole workspace. A reference to an item of
|
||
**another** category moves the key while `_Measure` names the same formula.
|
||
Stores A (Jan 3, Feb 4) and B (Jan 100, Feb 200), `G = Revenue * 2`,
|
||
`F = IF(Revenue > 50, G, G + B)`. At `(Q1, A, F)` Revenue is 7, so F is
|
||
`G + B`: G at A is 14, and `B` reads F at `(Q1, B)`, where Revenue is 300, so
|
||
that F is G at B, 600. The answer is **614**. Rival: the memo keyed by
|
||
`formula.target` alone. F is still in progress at `(Q1, A)` when F at `(Q1, B)`
|
||
is asked, so that key reads `circular`. Injected into a full workspace run, it
|
||
fails this test and nothing else. The reviewer's probe, now committed.
|
||
|
||
### a_diamond_at_a_fold_is_not_circular
|
||
|
||
improvise-d7aj's second case, which passed at `b3379d1a` and was pinned by
|
||
nothing. `A1 = B2 + C3`, `B2 = D * 2`, `C3 = D * 3`, `D = Revenue` reaches D by
|
||
two paths in one fold evaluation, so the second reach must find D *finished*
|
||
and not *in progress*. It asserts **35** over D = 7 and exactly four
|
||
evaluations, so a memo that re-evaluates D passes neither. Rival: never store a
|
||
finished value, so only the in-progress marker stays. This test fails with
|
||
`circular`, and so does `a_fold_evaluates_each_formula_it_reaches_once`, whose
|
||
doubling chain is also a diamond at every step. So this test is a named pin of
|
||
that shape rather than the only guard against it.
|
||
|
||
### a_fold_answers_is_and_where_on_its_category_as_the_none_axis_does
|
||
|
||
improvise-lnkb, ruled by the orchestrator: on a fold, the folded category reads
|
||
as absent. `Flag = Month IS "Q1"` and `Marked = Revenue * 2 WHERE Month = "Q1"`
|
||
are each compared with the same document with Month on `Axis::None`. At
|
||
`689c7665` `Flag` read **1** on the fold and 0 on None. The final assertion pins
|
||
`Flag` at `Some(0.0)`, false rather than absent, so a fix that dropped the cell
|
||
cannot pass. Rival: `read_coordinate` ignoring the reach (`if false && …`).
|
||
This test fails.
|
||
|
||
### `is_item_agrees_with_effective_item_names_everywhere`
|
||
|
||
**improvise-4daa**, found by the #265 review at `adb6b814`. `Model::first_non_item`
|
||
used to ask `effective_item_names(cat).contains(name)` for every coordinate. That
|
||
built and cloned the whole item list per coordinate per projected cell.
|
||
`Model::is_item` asks the same question with no allocation. This test holds the
|
||
two answers equal across every pairing of three categories (`Region`, `_Measure`,
|
||
and one that does not exist) and six names:
|
||
- an item;
|
||
- a measure item;
|
||
- a measure formula target;
|
||
- a formula target on a regular category, which `add_formula` makes a real item;
|
||
- a name that is nothing.
|
||
|
||
A setup assertion checks that `Profit` is not in `_Measure`'s explicit items, so
|
||
only the formula branch can answer it.
|
||
|
||
**Rival:** `is_item` without the formula-target branch fails this test, and
|
||
`ui::host::tests::a_write_under_a_formula_is_refused_for_the_formula_not_as_a_non_item`
|
||
in `improvise-command`.
|
||
|
||
**Bench**, not committed, on an Apple M4 Max, 16 cores. It is the reviewer's
|
||
`bench265`, and each build ran interleaved 5 times; figures are the minimum per
|
||
build.
|
||
|
||
| build | pivot 150×120 (18k cells) | records 2000×5 (2000-item label) |
|
||
|---|---|---|
|
||
| `47b166ee` (#252, before) | 131 ms | 1.93 ms |
|
||
| `adb6b814` (#265 head) | 193 ms | 145 ms |
|
||
| with `is_item` | 136 ms | 2.82 ms |
|
||
|
||
Single, non-interleaved runs on this machine varied by more than 2×. That is why
|
||
each build was run 5 times, interleaved, and the minimum taken.
|
||
|
||
### records_mode_records_display_returns_values
|
||
|
||
**What was vacuous (improvise-sfwy).** The category-column half asserted only
|
||
`!display.is_empty()`, where the fixture's value is `North`. The ticket named
|
||
the test under its old name, `records_mode_resolve_display_returns_values`.
|
||
improvise-umi4 renamed it when `resolve_display` was removed, and kept the
|
||
weak assertion.
|
||
|
||
**Fault first, then the fix** (the improvise-jkxr procedure). Two faults were
|
||
injected into `records_value`'s `RecordCol::Category` arm. The first returns
|
||
`"GARBAGE"`. The second returns the category's *name*, `Region`, a plausible
|
||
wrong answer. Under the old assertion the test **passed** with each fault.
|
||
Now it asserts `Some("North")` exactly. With the faults it fails:
|
||
`left: Some("GARBAGE")` and `left: Some("Region")`. Without them it passes.
|
||
The production code is unchanged.
|
||
|
||
### pruning_keeps_every_surviving_rows_runs_at_every_level
|
||
|
||
improvise-e265, strengthened by improvise-6u6s. On a two-level grouped axis,
|
||
`prune_empty` used to change a surviving row's group. Pruning the inner header
|
||
dropped its `GroupEnd` too, and under the flat reading the ungrouped inner item
|
||
then joined the outer group.
|
||
|
||
**What it asserts.** For every surviving row and at every level:
|
||
- the run the row is in names the same `(category, group)` pruned as unpruned
|
||
(the whole open-run stack, from `open_runs`, not only the innermost);
|
||
- whether it is in the **same run** as the previous survivor is the same
|
||
answer pruned as unpruned, where "the same run" means the same header entry.
|
||
|
||
When every row survives, the pruned entries must equal the unpruned ones,
|
||
marker for marker.
|
||
|
||
**The fixtures** are five, each under every collapse subset of its groups:
|
||
- `Region` over `Month`;
|
||
- three levels, `Region` over `Month` over `Kind`;
|
||
- `Month = [Jan in Q1, Mar, Feb in Q1]`, a non-contiguous group, so one parent
|
||
holds two `Q1` runs (improvise-h0iv);
|
||
- a middle level with no groups at all (`Kind` between `Region` and `Month`);
|
||
- two outer groups side by side (`North` in `Upper`, `South` in `Lower`).
|
||
|
||
**How it is built** (improvise-6u6s). Each fixture's layout is built once per
|
||
collapse subset, with no cells, so every row is present. `prune_axis_items` is
|
||
then called directly with a keep predicate for every non-empty set of rows.
|
||
That makes 4,992 cases. The first version built two `Workbook`s per funded-cell
|
||
mask, about 4,000 cases, and put `improvise-core`'s lib at 0.86–0.92s on an
|
||
Apple M4 Max (16 cores) under load average 39–77, against about 0.16s without
|
||
it. The reviewer measured 0.94–2.46s under heavier load, once past the 2s
|
||
tripwire. Now the lib runs in 0.17–0.23s on the same machine, against 0.16–0.18s
|
||
with the module skipped. `prune_empty` is still covered, because it calls the
|
||
same pruner, and `prune_empty_keeps_a_row_past_an_inner_run_in_the_outer_group`
|
||
pins that wiring.
|
||
|
||
**Red and rivals.** The e265 form failed at base 202e7eac, on `mask 0b100` with
|
||
only `North/Mar` funded: pruned said `Upper`, unpruned said `None`. Rivals, each
|
||
injected and restored:
|
||
- a flat reader (every marker read at level 0) fails it;
|
||
- a pruner that owes no `GroupEnd` fails it (u6gc's shape: a run whose closing
|
||
header was pruned is left open);
|
||
- a pruner that keeps headers as they arrive fails it;
|
||
- **runs keyed by `(level, category, group name)` instead of header position**
|
||
fails it, at `fixture 2, mask 0b101, collapsed []: run identity at level 1,
|
||
row 1`. Pruning `Mar` must leave North's two `Q1` runs as two, and the rival
|
||
merges them.
|
||
|
||
That last rival passed every test in #250 (improvise-6u6s). Those fixtures had
|
||
no non-contiguous group and compared only the innermost group, so they could not
|
||
see a merged run.
|
||
|
||
The equality half was added after the first version of `remark` emitted `End0`
|
||
where the input had `End1, End0`. That output was equivalent but not equal, and
|
||
the assertion caught it.
|
||
|
||
### a_pruned_axis_draws_no_header_over_nothing_and_ends_on_no_marker
|
||
|
||
improvise-e265's other property, over the same 4,992 cases. Every kept header
|
||
must be followed, past any deeper headers, by a data item, and the axis must
|
||
not end on a `GroupEnd`. At base 202e7eac, `mask 0b1` left a trailing
|
||
`GroupEnd`. The ticket counted the header-over-nothing shape on main before #200.
|
||
|
||
Rivals:
|
||
- a pruner that keeps headers as they arrive fails the first half;
|
||
- a pruner that passes `GroupEnd`s through as they arrive (the pre-e265 shape)
|
||
fails the second.
|
||
|
||
In the shipped pruner both properties hold by construction, because markers are
|
||
emitted only before an item. So the obvious rival, deleting a trailing-marker
|
||
strip, has nothing to delete. An earlier draft had that strip, and it was dead
|
||
code.
|
||
|
||
### prune_empty_keeps_a_row_past_an_inner_run_in_the_outer_group
|
||
|
||
The enumeration drives `prune_axis_items` directly, so this test pins that
|
||
`prune_empty` reaches it. With only `North/Mar` funded, the pruned grid has one
|
||
row, and it is in `Upper`. It is a wiring test, not a discriminating one: #200's
|
||
pruner also answered `Upper` here. What it catches is a `prune_empty` that
|
||
stops pruning (the row count) or a reader that loses the outer run.
|
||
|
||
### a_row_after_an_inner_groups_end_is_still_in_the_outer_group
|
||
|
||
The user's ruling on improvise-e265, at the layout: membership is per level,
|
||
and `row_group_for` answers the innermost open group. With nothing pruned or
|
||
folded:
|
||
- `North/Jan` is in `Month`/`Q1`, the innermost;
|
||
- `North/Mar` is in `Region`/`Upper`, because `Q1` ended and `Upper` did not;
|
||
- `South/Mar` is in nothing.
|
||
|
||
Failed at base 202e7eac on the second assertion: the flat reading said `None`.
|
||
The flat-membership rival fails it again.
|
||
|
||
### a_lookup_does_not_read_past_its_row
|
||
|
||
improvise-0r01, which is ChatGPT Codex's P2 on #263. `innermost_group` built
|
||
`open_runs` for the whole axis, cloning a level stack per row, to answer for one
|
||
row on every `z` keypress. `open_runs` was made lazy and the lookup took
|
||
`.nth(idx)`. That is superseded: the walk is now `walk_open_runs`, which lends
|
||
one stack to a visitor rather than yielding a copy per row (see
|
||
`a_late_lookup_answers_for_its_own_row`).
|
||
|
||
The walk's extent cannot be seen through a well-formed axis, so the test builds
|
||
an axis whose second header is unreadable on purpose: `level: usize::MAX`, which
|
||
`Vec::resize` cannot allocate. A lookup for row 0 must answer `Upper` without
|
||
reaching it.
|
||
|
||
Rival: collecting `open_runs` before `.nth(idx)`, which is the eager shape. It
|
||
fails with `capacity overflow`, and the other 335 core tests pass. That rival
|
||
was run against the iterator form. Under `walk_open_runs` the equivalent is a
|
||
visitor that never breaks, and the witness holds the same way.
|
||
|
||
### a_fold_row_evaluates_each_formula_once_across_the_row
|
||
|
||
improvise-h6z1. `F0 = Revenue`, `F{i} = F{i-1} + F{i-1}` up to F8, as columns,
|
||
with Q1 collapsed so the Q1 row is a fold. The test evaluates every cell of that
|
||
row twice and compares the two runs: once with a fresh memo per cell (`evaluate`),
|
||
where each column re-derives the chain below it (1 + 2 + … + 9 = **45**), and
|
||
once with one memo shared across the row (`evaluate_in`), which gives **9**. It
|
||
also asserts the two runs produce identical values, so a memo that returned a
|
||
wrong entry cannot pass. The 45 is asserted as a setup, so a fixture whose chain
|
||
had collapsed into one column could not pass the 9 by accident. Rival:
|
||
`evaluate_covering_in` ignoring the memo it is given and making a fresh one.
|
||
Injected into a full workspace run, it fails this test (45 against 9) and
|
||
nothing else.
|
||
|
||
### a_spanned_scan_finds_what_a_lookup_per_member_finds
|
||
|
||
improvise-yb4b. `fold_stored` used to enumerate one `CellKey` per member of a
|
||
fold and look each one up. Since #239 a column total spans every row
|
||
category, which is 3,000 lookups for one 18k column, paid twice per keypress.
|
||
`DataStore::spanned_values` now answers with one index-narrowed scan. This test
|
||
compares the scan with the per-member definition it replaces, in both modes, on
|
||
a store built to separate them. It has a record with an **extra coordinate**:
|
||
`get` misses it and `matching_values` finds it. It has an **uncovered item**, a
|
||
record **lacking** the spanned category, **another measure**, and another
|
||
value of an unspanned coordinate. It also has a covered item that was never
|
||
interned. The values are distinct powers of two, so a sum identifies the
|
||
admitted set. The order differs on purpose: the store's order, not member
|
||
order, as gotcha 18 names it.
|
||
|
||
Three faults were injected, each run against the full `improvise-core` and
|
||
`improvise-command` suites:
|
||
|
||
- Exact mode loses its length check (`==` → `>=`). **Only this test fails.**
|
||
Nothing else in either suite reaches an `Exact` fold over a record with an
|
||
extra coordinate.
|
||
- The span ignores coverage (`|| true`). This test fails, and so do five others
|
||
(collapsed-group, hidden-row and CSV-agreement tests).
|
||
- A category the store never interned is skipped instead of matching nothing.
|
||
**Only this test fails.**
|
||
|
||
### a_span_finds_what_its_members_find_in_store_order
|
||
|
||
improvise-jq0m. `spanned_values` now chooses between two narrowings, and a
|
||
fixed example can only ever take one of them. This property builds random
|
||
stores over `R`, `P` and `M`. Some records lack `R`, some carry an extra `X`,
|
||
and `r6` is a covered item the store never interned. It spans `R` and sometimes
|
||
`P`, in both modes, and compares the result with the per-member definition,
|
||
**in store order**: the values the members' own lookups find, read back in
|
||
`iter_cells` order. Store and span sizes vary across cases, so both narrowings
|
||
are taken.
|
||
|
||
Three faults were injected, each run against the full `improvise-core` and
|
||
`improvise-command` suites:
|
||
|
||
- The per-item narrowing drops the base, so it keeps each item's pair alone.
|
||
This test and `a_spanned_scan_finds_what_a_lookup_per_member_finds` fail.
|
||
**Nothing in either crate's grid or command tests does.** Those fixtures never
|
||
hold a second value of the base's coordinate within a small span.
|
||
- The base narrowing drops the base. This test fails, along with eight
|
||
fold and totals tests.
|
||
- The per-item positions are not sorted after they are merged. **Only this
|
||
test fails.** Values stay correct and only their order moves, which changes
|
||
a Sum or Avg in the last bits.
|
||
|
||
The regression itself is a cost, which no test here can see. The evidence for
|
||
it is the `grid_projection` bench's grouped cases, added for this ticket.
|
||
|
||
### a_memo_answers_for_the_covers_it_was_built_with
|
||
|
||
**What broke (improvise-pntu).** Found reviewing PR #247, and it was also
|
||
ChatGPT Codex's P2 there. `FoldMemo` was keyed by (fold key, span categories),
|
||
while `evaluate_covering_in` took the aggregation and the covers as separate
|
||
arguments. With `margin_book(["R = Revenue / Cost"])`, key
|
||
`{Month=Q1, _Measure=R}` and `Exact`, one memo was passed first with covers=all
|
||
and then with Feb hidden. The second call returned **5.0** (40 / 8, the first
|
||
fold's answer) where a fresh memo gives **2.0** (10 / 5, Jan alone).
|
||
|
||
**Red first.** The test was first written against the old API, exactly as the
|
||
reviewer gave the repro, and it failed `left: Some(Number(5.0)), right:
|
||
Some(Number(2.0))`. The fix moves agg and covers into the memo
|
||
(`FoldMemo::new(agg, covers)`), so that shape **no longer compiles**. The
|
||
signature is the guard. The test now builds one memo per covers and asserts
|
||
three things. Each memo gives its own fold's value. The Feb-hidden memo agrees
|
||
with a fresh `evaluate_covering`. The covers=all memo still says 5.0 afterwards.
|
||
|
||
**Rival:** `FoldMemo::new` storing `|_, _| true` in place of the covers it was
|
||
given. This test fails, because Jan alone reads 5.0.
|
||
|
||
### a_memo_answers_for_the_aggregation_it_was_built_with
|
||
|
||
pntu's other half. `{_Measure=Revenue}` names no month. So under an `Exact`
|
||
memo it is not a stored cell (`None`), and under an `OverHidden` memo it sums
|
||
Jan and Feb (`40.0`).
|
||
|
||
**Rivals:**
|
||
- `evaluate_covering_in` passing `Aggregation::Exact` instead of the memo's
|
||
agg: this test fails.
|
||
- The same fault in `evaluate_spanning_in`: caught by two existing tests,
|
||
`a_max_measures_total_does_not_move_when_a_row_group_collapses` and
|
||
`an_average_measures_total_is_the_mean_over_its_records`. So it needed no
|
||
new test.
|
||
|
||
### pruning_evaluates_each_fold_formula_once_and_keeps_the_same_rows
|
||
|
||
**What was slow (improvise-metf).** `GridLayout::prune_empty` evaluated every
|
||
cell with its own fresh fold memo. That is h6z1's shape, and it sits on the same
|
||
keypress path, since `with_frozen_records` prunes whenever the view asks. Each
|
||
column of a folded chain re-derived the chain below it.
|
||
|
||
**Measured first**, on an Apple M4 Max (64 GB) running other agents' builds.
|
||
The bench is `cargo bench --bench grid_projection`, whose new "pruned" lines time
|
||
a `with_frozen_records` build with `prune_empty` on. There were two back-to-back
|
||
pairs, and the second put the old line back for the "before" half:
|
||
|
||
| Case | Before (fresh memo per cell) | After (one memo per build) |
|
||
|---|---|---|
|
||
| dense 18k | 90 ms, 88 ms | 82 ms, 100 ms |
|
||
| folded chain ~18k | 900 ms, 705 ms | 355 ms, 351 ms |
|
||
| folded chain ~90k | 6.40 s, 6.62 s | 3.00 s, 4.96 s |
|
||
|
||
It pays on the folded chain: about 2.0× to 2.5× at 18k, and 1.3× to 2.1× at 90k.
|
||
The dense grid has no folds and moves only within noise. So `prune_empty` now
|
||
shares one memo per build, taken from `GridLayout::fold_memo` and dropped before
|
||
the axes are pruned.
|
||
|
||
**The test.** It uses `margin_book`'s F0..F8 chain with Q1 collapsed. It counts
|
||
fold-formula evaluations across one pruned `with_frozen_records` build and
|
||
asserts **9**. It also asserts that the pruned rows are exactly the rows a fresh
|
||
`evaluate` per cell would keep, so sharing the memo cannot change what is
|
||
pruned. And it asserts that the Q1 fold row survives, so the count is not
|
||
measured over an empty grid.
|
||
|
||
**Rival:** the old line, `self.evaluate(model, &k)` per cell. This test fails:
|
||
`left: 45, right: 9`.
|
||
|
||
### a_grid_evaluates_under_its_own_covers_not_another_grids
|
||
|
||
**What was open (improvise-opbo, P3).** It confirms ChatGPT Codex's P2 on #258.
|
||
`GridLayout::evaluate_in` and `column_total` took any `&FoldMemo`. With
|
||
`margin_book(["R = Revenue / Cost"])` and Q1 collapsed, there are two layouts
|
||
over one model: one draws Jan and Feb, the other hides Feb. The first layout's
|
||
memo, handed to the second layout's `evaluate_in`, gave **5.0** for
|
||
`{Month=Q1, _Measure=R}` where the second grid reads **2.0**. Nothing in
|
||
production reached it, because each build made its own memo.
|
||
|
||
**Red first.** The test was first written against `evaluate_in` with a foreign
|
||
memo. It failed `left: Some(Number(5.0)), right: Some(Number(2.0))`.
|
||
|
||
**Fix: the review's proposal.** `GridLayout::evaluator(model)` returns a
|
||
`GridEval` that borrows the layout and owns a memo the layout built itself. It
|
||
has `cell(key)` and `column_total(col)`. `evaluate_in`, the memo-taking
|
||
`column_total`, and `fold_memo` are gone. So the shape the test first used no
|
||
longer compiles, and the signature is the guard. The test now asserts two
|
||
things. Each layout's evaluator answers for its own covers: 5.0 and 2.0. And
|
||
asking one does not move the other.
|
||
|
||
**Rival:** the evaluator's covers ignoring the layout's hidden items. This test
|
||
fails `left: Some(Number(5.0)), right: Some(Number(2.0))`.
|
||
|
||
**The earlier rivals, restated for the new API, still fail:**
|
||
- `pruning_evaluates_each_fold_formula_once_and_keeps_the_same_rows` with a
|
||
fresh evaluator per cell: `left: 45, right: 9`.
|
||
- `one_grid_build_evaluates_each_fold_formula_once_for_cells_and_totals` with
|
||
a fresh evaluator per cell: `left: 54, right: 18`.
|
||
- The same test with a fresh evaluator per total: `left: 54, right: 18`.
|
||
|
||
The entries above for h6z1, 0jw5 and metf name `evaluate_in` and `fold_memo`
|
||
because those were the API when they were written. This entry supersedes the
|
||
names, not the arguments.
|
||
|
||
### a_late_lookup_answers_for_its_own_row
|
||
|
||
improvise-0r01, the second round: ChatGPT Codex's P3 on #273. The lazy
|
||
`open_runs` iterator still yielded `Some(open.clone())` for every data item, so
|
||
`innermost_group(..).nth(idx)` allocated and dropped a stack for every row
|
||
before the target. A `z` near the end of a large axis paid nearly the whole
|
||
original cost. `a_lookup_does_not_read_past_its_row` could not see this: it asks
|
||
for row 0, and it proves only that the suffix is not read. The walk is now
|
||
`walk_open_runs`, which lends one stack to a visitor and copies nothing.
|
||
|
||
This test asks a late index. It covers rows 0 to 5 of a
|
||
`Q1{Jan, Feb}, Mar, Q2{Apr, May}` axis, and index 5 is past the end. So it pins
|
||
the row counting that the visitor form moved into `innermost_group`. Both rivals
|
||
were run against the whole `improvise-core` suite:
|
||
- A counter that never advances, so every lookup answers row 0: this test fails,
|
||
along with four others.
|
||
- A walker that ignores `GroupEnd`: this test fails, along with seven others.
|
||
|
||
**What it cannot see is the allocation itself.** A visitor that clones the
|
||
stack at every row returns the same answers, so no assertion here can tell the
|
||
two apart without a counting allocator. The copy-free property is held by the
|
||
shape: the visitor receives a borrowed slice of open runs (`&[OpenRun]` since
|
||
improvise-felo.6), and no owned stack
|
||
leaves the walk. It is not held by a test.
|
||
|
||
### a_parent_group_covers_every_leaf_beneath_its_child_groups
|
||
|
||
improvise-felo.6. `items_in_group` read only the items whose own `group` named
|
||
the group, so a year covered nothing: its months sit in quarters. Rival run
|
||
against `improvise-core`: the direct-only filter restored. Four tests fail —
|
||
this one, `a_hierarchy_opens_closed_as_one_fold_per_root` (the 2024 fold reads
|
||
empty), `collapsing_a_parent_folds_its_children_whatever_their_state` and
|
||
`hidden_items_leave_every_fold_above_them`. The fixture adds `2024-03` last, so
|
||
the year and its first quarter are non-contiguous in insertion order.
|
||
|
||
### a_year_quarter_month_hierarchy_nests_its_runs
|
||
|
||
improvise-felo.6: the whole marker sequence of a Year > Quarter > Month axis
|
||
opened to depth 2, compared exactly. Rival: every header inside a category
|
||
emitted at depth 0 (a flat hierarchy). This test fails, and so do
|
||
`pruning_keeps_every_surviving_rows_runs_at_every_level` (the `NESTED` fixture),
|
||
`the_nested_fixture_draws_runs_inside_one_category` and
|
||
`pruning_drops_an_empty_inner_group_and_keeps_its_parent`.
|
||
|
||
### a_hierarchy_opens_closed_as_one_fold_per_root
|
||
|
||
The user's ruling on improvise-felo.6: groups start closed. Two rivals fail it.
|
||
The old open default (`inherits_collapsed` always false) fails twelve tests
|
||
across `improvise-core` and `improvise-io`, this one among them. The INNERMOST
|
||
collapsed group folding instead of the outermost (`.rev()` on the fold search)
|
||
fails this one, `hidden_items_leave_every_fold_above_them` and
|
||
`the_group_under_the_cursor_is_the_innermost_at_every_depth`: under it a closed
|
||
year shows its quarters as rows.
|
||
|
||
### a_groups_depth_opens_it_and_n_minus_one_levels_beneath
|
||
|
||
`expanded(2): Time/2024` opens 2024 and its quarters, not its months. Rival: an
|
||
ancestor reaching `k <= n` levels instead of `k < n`. Only this test fails, which
|
||
is why it exists: the boundary is the whole semantics of N.
|
||
|
||
### a_toggle_overrides_what_a_group_inherits_either_way_and_toggling_back_clears_it
|
||
|
||
`z` writes `Collapsed` or `Expanded(1)`, or removes the fold where the flip
|
||
lands on what the group inherits. Rival: never remove. This test and
|
||
`command::cmd::grid::tests::z_opens_a_closed_hierarchy_one_level_at_a_time_and_closes_it_again`
|
||
fail; every layout test passes, since a redundant fold draws the same grid. What
|
||
the rival breaks is the file (a line per toggle, forever) and `z z` being the
|
||
identity on the view.
|
||
|
||
### pruning_drops_an_empty_inner_group_and_keeps_its_parent
|
||
|
||
`prune_empty` at depth: an empty quarter goes, header and all, and its year's
|
||
header stays for the sibling quarter that has data. Rival: `prune_axis_items`
|
||
keying open runs by level alone. This test and the `NESTED` enumeration fail.
|
||
|
||
## `model/id.rs` (ADR 007 clause 4 and amendment 1, improvise-cq1q.2, improvise-cq1q.14)
|
||
|
||
Each rival below was injected into the finished module, run against `cargo test -p improvise-core --lib model::id`, and
|
||
reverted. The reasoning is `context/code/core.md` §20.
|
||
|
||
### model::id::tests::slugify_case_folds_rather_than_lowercases
|
||
|
||
Rival: `display.to_lowercase()` in place of `unicase`'s full folding. Only
|
||
this test fails: `ß` lowercases to itself and becomes `_`, so `stra_e` against
|
||
`strasse`. `slugify_follows_the_amendments_table` cannot see this rival,
|
||
because each of its displays lowercases and folds alike.
|
||
|
||
### model::id::tests::slugify_follows_the_amendments_table
|
||
|
||
Every row of the amendment's clause 4 table, plus `-`, `%` and the empty
|
||
display. Rivals, each of which fails this test: trimming only leading `_`
|
||
(`East_` stays), trimming `_` and not `-` (`-East-`), not collapsing runs of
|
||
`_` (`a__b`), not prepending `n` (`2024`), and stripping instead of replacing
|
||
(`Region(code)` is `regioncode`). The last also fails
|
||
`slugify_keeps_a_hyphen_between_underscores` and
|
||
`a_slug_taken_by_another_kind_takes_a_suffix`.
|
||
|
||
### model::id::tests::slugify_trims_hyphens_and_underscores_until_neither_is_at_an_end
|
||
|
||
Rival: trim leading `_` only, or `_` only at both ends. `_-_East-_-` keeps a
|
||
hyphen, and `-_-` is not empty. A single `trim_matches` over both characters
|
||
passes; two sequential single-character trims would not.
|
||
|
||
### model::id::tests::slugify_trims_before_it_prepends_the_n
|
||
|
||
Rival: no `n` prefix, which fails this test, the table and the property below.
|
||
`-_7up` is `n7up` only if the trim runs first; prepending first would leave the
|
||
`-_` in front of the `n`.
|
||
|
||
### model::id::tests::slugify_output_has_the_id_syntax_and_is_a_fixed_point
|
||
|
||
The property over arbitrary displays: a non-empty slug satisfies
|
||
`is_id_syntax` and slugifying it returns it. Rivals that fail it (and
|
||
`generated_ids_are_distinct_well_formed_and_never_reserved`): no `_`
|
||
collapse, no `n` prefix, keeping a trailing `_` or `-`.
|
||
|
||
### model::id::tests::id_syntax_agrees_with_an_independent_checker
|
||
|
||
`is_id_syntax` against a hand-written scanner of `^[a-z](_?[a-z0-9-])*$` that
|
||
does not end in `-`, over `[a-cA-C0-9 _%é-]{0,8}`, so uppercase, space, symbols
|
||
and non-ASCII are reached. Dropping the alphabet check also fails it. Rivals:
|
||
allowing `__`, allowing a leading digit and allowing a trailing `-` each fail
|
||
here and in `claim_refuses_what_a_generated_id_could_not_be`.
|
||
|
||
### model::id::tests::natural_order_decides_the_zeros_tie_at_its_own_run_not_after_the_rest
|
||
|
||
Rival: compare leading zeros only after every run compares equal in value.
|
||
`a-7c` then sorts after `a-007b`, and this test fails alone.
|
||
|
||
### model::id::tests::suffixed_displays_are_distinct_when_literals_already_hold_suffixes
|
||
|
||
Displays are drawn from `A`, `A #2`, `A #3` and `B`, so literal suffixes meet
|
||
generated ones. Rival: numbering without the skip fails it.
|
||
|
||
### model::id::tests::claim_refuses_what_a_generated_id_could_not_be
|
||
|
||
Rival: `claim` keeps the old alphabet-only check. `-x`, `x-`, `2024`, `a__b`,
|
||
`x_` and `-` are admitted, and the test fails. It also pins the accepted ids
|
||
`east-1`, `item-4`, `a-7` and `q1_-_2024`.
|
||
|
||
### model::id::tests::claim_admits_each_virtual_id_once
|
||
|
||
Rival: `VIRTUAL_IDS` without `_records` and `_drill`. The test spells the five
|
||
ids out instead of iterating the constant, because an iteration over the
|
||
constant passes with the constant shortened (the first version did).
|
||
|
||
### model::id::tests::natural_order_puts_a_counter_suffix_in_numeric_order
|
||
|
||
Rivals: comparing digit runs as strings (`profit-10` before `profit-2`) fails
|
||
this test, `natural_order_compares_digit_runs_of_any_length_by_value` and
|
||
`a_suffix_numbers_equal_displays_in_natural_id_order`; plain `str::cmp` fails
|
||
those and `natural_order_ties_on_value_go_to_fewer_leading_zeros`.
|
||
|
||
### model::id::tests::natural_order_ties_on_value_go_to_fewer_leading_zeros
|
||
|
||
Rivals: more zeros first, or no tie-break (`a-7` equal to `a-007`). Both fail this test and the any-length test (`x000` after `x0`).
|
||
|
||
### model::id::tests::natural_order_puts_the_id_that_runs_out_of_runs_first
|
||
|
||
Rival: the longer id first. Only this test fails.
|
||
|
||
### model::id::tests::natural_order_is_a_total_order
|
||
|
||
Antisymmetry, equality only for equal strings and transitivity over
|
||
`[a-c0-3_-]{0,7}`, which includes ids starting with a digit.
|
||
|
||
### model::id::tests::a_suffix_numbers_equal_displays_in_natural_id_order
|
||
|
||
Rival: ordering the group by `str::cmp` on the ids. `profit-10` is numbered
|
||
before `profit-2` and this test fails alone.
|
||
|
||
### model::id::tests::a_suffix_skips_a_number_a_literal_display_already_holds
|
||
|
||
Rival: numbering without the skip. A literal `Profit #2` and a generated
|
||
`Profit #2` are then both shown, and this test fails alone. A rival that starts
|
||
numbering at ` #1` also fails it and
|
||
`a_suffix_is_gapless_when_a_thing_goes_away`.
|
||
|
||
### model::id::tests::the_suffix_is_the_next_counter_value_not_the_lowest_free_number
|
||
|
||
Rival: the lowest free `<base>-<n>` for each base, which an early draft of
|
||
ADR 007 proposed and clause 4's shared `gensym` counter replaced. Three tests fail: this one (`"west-1"` against
|
||
`"west-2"`), `an_empty_slug_takes_its_kind_and_the_shared_counter` (`"item-1"`)
|
||
and `a_slug_taken_by_another_kind_takes_a_suffix` (`"time_period-1"`). Only
|
||
interleaving two bases separates one shared counter from per-base numbering,
|
||
and this test is the one that does it on purpose.
|
||
|
||
### model::id::tests::a_claimed_id_keeps_its_spelling_and_generation_steps_past_it
|
||
|
||
Rival: `claim` validates the id but does not record it. This test and
|
||
`claim_refuses_a_duplicate_a_malformed_and_a_reserved_id` fail. The second
|
||
`East` is issued `east-1`, the file's own id, which is the collision clause 4
|
||
forbids: hand-written ids are canonical, and renaming one would corrupt the
|
||
data section.
|
||
|
||
### model::id::tests::generated_ids_are_distinct_well_formed_and_never_reserved
|
||
|
||
Rival: `generate` does not record the id it issues, so nothing is retired. Five
|
||
tests fail, this proptest among them (`reissued`), along with every
|
||
suffix-order test. The proptest is the one that states the property itself: no
|
||
id is issued twice in a session, whatever the displays.
|
||
|
||
### model::id::tests::two_allocators_fed_alike_generate_alike
|
||
|
||
This test builds two allocators independently, so they share no hash state,
|
||
rather than calling one twice. `seen` is a `HashSet`, but it is only ever
|
||
queried and never iterated, so its order cannot reach an id (gotcha 18). No
|
||
rival of the current shape breaks this test. It guards against a future
|
||
change that picks a candidate by walking `seen`.
|
||
|
||
### model::id::tests::a_long_claimed_run_of_suffixes_is_walked_without_recursion
|
||
|
||
This is the Codex P2 on PR #318 (review of `a2ddb06e`), reproduced by the adversarial review of
|
||
`689480bf`. `IdAllocator::suffixed` recursed once per taken candidate, and the
|
||
call was not in tail position. A new-version file saved from a non-ASCII import
|
||
claims `item-1` … `item-N`, and the counter restarts at 1 on load, so the
|
||
next empty-slug generation walks all N. Against the recursive version this test
|
||
(N = 20,000) aborted the whole test binary: `has overflowed its stack` /
|
||
`SIGABRT`. It is an abort, not a failed assertion, so no test could catch it.
|
||
The loop passes, and still advances the counter on every candidate it tries.
|
||
The second assertion, `category-20002`, pins that: a loop that restarted from
|
||
a local counter on each call would answer `category-1`.
|
||
|
||
### model::id::tests::claim_refuses_an_id_that_only_begins_like_a_virtual_one
|
||
|
||
Found by the adversarial review of `689480bf`. With `claim` rewritten to
|
||
exempt any id that begins with a virtual id (`VIRTUAL_IDS.iter().any(|v|
|
||
id.starts_with(v))`), all 11 earlier tests passed, because
|
||
`claim_refuses_a_duplicate_a_malformed_and_a_reserved_id` tries only `_draft`.
|
||
This test fails on that rival (`left: Ok(())` for `_indexer`). The match is
|
||
exact by the ADR's own wording: clause 1 says no other id may begin with `_`.
|
||
|
||
### model::id::tests::a_display_with_leading_spaces_never_slugs_into_the_reserved_space
|
||
|
||
Rival, from the same review: strip the leading `_` before the space step, so
|
||
that ` Draft` becomes `_draft`. This test fails (`left: "_draft"`), and so does
|
||
`generated_ids_are_distinct_well_formed_and_never_reserved`. Before this test,
|
||
the proptest was the only thing that saw the rival.
|
||
|
||
## Displays beside keys (ADR 007 clause 2, improvise-cq1q.3, first slice)
|
||
|
||
Each renderer test below builds a projection whose display differs from its key.
|
||
The test-only way to get one is `set_display`, since no production path makes
|
||
them differ yet. Every test failed before the change, with the key drawn where
|
||
the display should be: `left: "Region"` against `right: "Sales Area"` in the
|
||
projection and wasm tests, and a missing "Sales Area" or "Overview" in the TUI
|
||
buffers.
|
||
|
||
### ui::project::tests::the_category_panel_shows_a_display_and_keeps_the_key_commands_name
|
||
|
||
It finds the row by `name == "Region"` before asserting on the display. A rival
|
||
that writes the display into `name` fails the lookup, which covers the half that
|
||
keeps commands working. `_Measure` is checked to fall back to its own key.
|
||
|
||
### ui::view_panel::tests::a_view_row_draws_its_display_and_marks_the_active_view_by_its_key
|
||
|
||
Rival: the active marker compares `summary.display == active_view`. This test
|
||
fails on that rival (no `▶ Overview`), because the active view is a key
|
||
(`ViewState::active_view`) and its display differs from it.
|
||
|
||
### render::tests::the_panels_paint_displays_and_keep_keys_for_what_a_command_names
|
||
|
||
The browser half of the same three panels. It also asserts that the view row is
|
||
still `active` when found by key.
|
||
|
||
## Displays in the grid frame, page bar, item rows and CSV (ADR 007 clause 2, improvise-cq1q.12)
|
||
|
||
The same test-only way to make a display differ from its key (`set_display`,
|
||
`set_item_display`, `set_group_display`). Each rival below was injected, the
|
||
workspace run, and the named tests failed; the revert was re-run green.
|
||
|
||
### view::layout::tests::a_layout_shows_displays_and_still_addresses_by_keys
|
||
|
||
Rivals: an item's display pushed as its key (`display.push(item_name)`); a page
|
||
axis's `selected_display` set to `selected`. Both fail on the display asserts,
|
||
while `cell_key`, `row_group_for` and `row_run_start` stay on keys.
|
||
|
||
### view::layout::tests::a_folded_group_is_shown_under_the_groups_display_and_keyed_by_its_name
|
||
|
||
A folded row's tuple holds the group name where an item name would be, so the
|
||
display must come from the group. Not a distinct injection; it covers the arm
|
||
that the item test cannot reach.
|
||
|
||
### view::layout::tests::a_records_grid_shows_displays_in_its_headers_and_coordinates
|
||
|
||
Rival: `coordinate_display` returns the item key. Fails here and in
|
||
`ui::project::tests::a_records_coordinate_shows_its_display_and_edits_its_key`.
|
||
|
||
### ui::project::tests::the_frame_shows_displays_and_its_key_tuples_still_address_the_cells
|
||
|
||
The rival the ticket names: `axis_frame` writes `tuple: display.clone()`. The
|
||
test rebuilds every cell's `CellKey` from the frame's key tuples, its `cats` and
|
||
the page bar, and compares it to `layout.cell_key`, so it fails (and so does the
|
||
records test, which looks the Region column up by its key tuple). Also fails when
|
||
a group header's display is its key.
|
||
|
||
### ui::project::tests::each_view_summary_shows_its_own_display_and_its_categories_displays
|
||
|
||
Rival: every summary takes the ACTIVE view's display. The second view "Plan" has
|
||
a display unlike both the active view's ("Overview") and its own key, so the
|
||
fault fails here by name.
|
||
|
||
### ui::project::tests::an_item_row_shows_its_display_and_keeps_its_key
|
||
|
||
Rival: the item row's display is its key. The unrenamed item "West" must show its
|
||
key, and `_Measure`'s "Revenue" shows "Sales".
|
||
|
||
### ui::project::tests::the_page_bar_carries_displays_beside_the_keys_a_page_command_names
|
||
|
||
Rival: `selected_display` is the key. Fails here, in the layout test, in the TUI
|
||
grid test and in the CSV test.
|
||
|
||
### render::tests::a_header_draws_displays_and_blanks_a_repeat_by_key
|
||
|
||
Rival: suppression compares display tuples. West shares East's DISPLAY ("Same")
|
||
and must still draw, so comparing displays blanks it.
|
||
|
||
### render::tests::a_header_is_as_wide_as_what_it_shows
|
||
|
||
Rival: `header_widths` measures the key tuples. Fails (key "E" against display
|
||
"Eastern Seaboard"), as does the TUI grid test.
|
||
|
||
### render::tests::a_frame_with_ragged_displays_is_refused
|
||
|
||
Asserts the control parses, then that a `cat_displays` shorter than `cats` and a
|
||
display tuple shorter than its key tuple are each refused with their own message.
|
||
|
||
### resolve::tests::hiding_a_displayed_row_item_names_its_key
|
||
|
||
Rival: the hide resolver reads the first DISPLAY of the row frame. It composes
|
||
`HideItem { item: "Eastern Seaboard" }` and the test fails on `"East"`.
|
||
|
||
### resolve::tests::picking_a_displayed_page_item_still_names_its_key
|
||
|
||
Picks index 1 of a page axis whose displays are not its keys and requires the
|
||
command to name `Q2`. The index is the only thing that travels from the menu.
|
||
|
||
### render::tests::the_grid_page_bar_and_item_rows_paint_displays
|
||
|
||
The browser half. Rival: the cycle's `items` relays the keys, which fails here.
|
||
|
||
### ui::grid::tests::the_frame_and_page_bar_draw_displays_not_keys
|
||
|
||
The TUI half. Rival: the group header draws `group.name`; fails on the missing
|
||
"▼ Bay". The key assertions use strings that appear only as keys.
|
||
|
||
### ui::category_panel::tests::an_item_row_draws_its_display_not_its_key
|
||
|
||
The TUI item row. Not separately injected: its draw path is the one line the
|
||
`CatTreeEntry::Item` pattern changed.
|
||
|
||
### persistence::export::tests::export_csv_writes_displays_in_every_header_and_label
|
||
|
||
Rival: the column header uses `col_label` (the key). The exact output pins the
|
||
corner cell `Kind (Who=Alicia)`, the column header and the row label.
|
||
|
||
### view::layout::tests::a_page_axis_with_ragged_displays_is_refused
|
||
|
||
From Copilot's finding on #336: `item_displays` is a parallel list on a wire type
|
||
that derives `Deserialize`. The control parses a well-formed axis first; then a
|
||
shorter list and an absent one beside `items: Some(..)` are each refused with the
|
||
"item displays" message. Without the `try_from` the same JSON parses.
|
||
|
||
### ui::viewport::tests::a_column_is_as_wide_as_the_display_its_header_draws
|
||
|
||
From the Opus review of #336: `header_level_seed` could measure keys and the
|
||
suite stayed green. The fixture's column display ("Januaryyyyy", 11) is far longer
|
||
than its key ("Jan") and the cell, so the 5-column minimum cannot hide it. Rival:
|
||
the seed reads `tuple` instead of `display`; the width is 5, not 12, and this test
|
||
fails.
|
||
|
||
### ui::viewport::tests::the_row_header_is_as_wide_as_the_display_its_rows_draw
|
||
|
||
Rival: `compute_row_header_width` reads `tuple`. That width drives `visible_cols`
|
||
on the keypress path (the improvise-gtn7.1 shape: a scroll measured elsewhere than
|
||
it is drawn), so a key measurement lets the cursor scroll to an undrawn column.
|
||
The display "Eastern Seaboard" against the key "E" gives 17, not the 4 minimum.
|
||
|
||
### render::tests::a_column_group_header_is_painted_by_its_display
|
||
|
||
Rival: `col_groups` labels a run with `g.name`. The browser test for group displays
|
||
covered row groups only; this one folds a column frame whose group "Q1" is shown as
|
||
"First quarter" and fails on the key.
|
||
|
||
### draw::content_tests::the_grid_title_names_the_views_display_not_its_key
|
||
|
||
improvise-cq1q.13, TUI half. The view is displayed "Quarterly overview of regional
|
||
sales" under the key "Default", and the whole frame is drawn through `draw`. Rival:
|
||
`draw_content` passes `app.view_state.active_view` to the widget; the title reads
|
||
`View: Default` and this test fails on both assertions.
|
||
|
||
### render::tests::the_title_names_the_active_views_display_not_its_key
|
||
|
||
The browser half. Rival: `render_model` takes `view_name` from
|
||
`state.view.active_view`; the test fails. The second half is the control: a view
|
||
the list does not hold falls back to its key rather than to an empty title.
|
||
|
||
### resolve::display_tests::a_panel_status_names_what_a_person_sees_not_the_keys
|
||
|
||
The browser resolver's panel statuses: axis cycle, filter, delete category, delete
|
||
item. Displays are 36 to 38 characters against keys of 4 to 6. Rivals, each run:
|
||
the cycle status passes the key; the filter composes `SetPageSelection` with the
|
||
item's display; the item removal names the display. Each fails here, the last two
|
||
on the `Region/North` and `RemoveItem` assertions that pin the lookup to keys.
|
||
|
||
### resolve::display_tests::a_tile_status_names_the_display
|
||
|
||
Rival: the tile route passes `tile.category`; fails on the key.
|
||
|
||
### resolve::display_tests::the_item_prompt_names_the_display_and_the_mode_keeps_the_key
|
||
|
||
Rival: the mode is built with the display as its category
|
||
(`item_add_shown(shown, ..)`); the prompt reads right and the commit would add to
|
||
a category that does not exist. The test fails on `category == "Region"`.
|
||
|
||
### resolve::display_tests::a_refused_cell_names_the_category_it_shows
|
||
|
||
Rival: the non-item refusal gets the category key. The stray coordinate stays a key
|
||
because it names no item and has no display.
|
||
|
||
### command::cmd::panel::tests::a_panel_status_names_what_a_person_sees_not_the_keys
|
||
|
||
The host half of the panel statuses and the item-add prompt. Rivals, each run: the
|
||
axis status passes `cat_name` (fails); the filter status passes the item key
|
||
(fails); the item-add mode is built from the display (fails on the
|
||
`category: "Type"` assertion).
|
||
|
||
Review follow-up: the same test now also pins the operands. Rivals: the host
|
||
`RemoveCategory` and `SetPageSelection` given the display fail on the
|
||
`RemoveCategory("Type")` and `item: "Food"` assertions. The client twin fails the
|
||
same way on `RemoveCategory`.
|
||
|
||
### command::cmd::tile::tests::a_tile_status_names_the_display_not_the_key
|
||
|
||
Rival: the status formats `name`. It fails on both the present display and the
|
||
absent key.
|
||
|
||
### ui::app::non_item_key_tests::a_non_item_refusal_names_the_categorys_display
|
||
|
||
Rival, run once per site: the refusal in `EditOrDrill` (the `i` key) and the one in
|
||
`CmdContext::value_cell_key` (the `p` key) each pass the key. Each fails here.
|
||
|
||
### ui::app::non_item_key_tests::a_commit_on_a_non_item_cell_names_the_categorys_display
|
||
|
||
The third site, `uneditable` in `cmd/core.rs`, which only a commit reaches once an
|
||
editor is open (`:enter-mode editing`). Rival: it passes the key; the two tests
|
||
above stay green and this one fails.
|
||
|
||
### ui::app::non_item_key_tests::a_refused_wire_write_names_the_category_it_shows
|
||
|
||
The wire's own refusal in `Grids::unwritable`. Rival: the category key; fails. The
|
||
formula arm (once a third copy of the derived wording, now `derived_refusal`) is
|
||
pinned to its wording so the consolidation cannot change what it says.
|
||
|
||
### resolve::display_tests::the_declines_an_advance_and_a_commit_reach_name_the_category_it_shows
|
||
|
||
From the Opus review of #339: the browser's `declined_at` non-item arm could pass the
|
||
key with the whole suite green. It is reached by `reopened_at` (an advance),
|
||
`refused_here` (a commit) and `settle_declined_editor`. Rival: the arm passes
|
||
`category`; the test fails on all three assertions, and the display is 36 characters
|
||
against the key "Region".
|
||
|
||
### model::source::tests::a_summary_reduces_as_the_values_did
|
||
|
||
improvise-qddo. The reduction moved from `aggregate_cell_values` to `Summary::reduce`; the test keeps
|
||
the old function as `reference` and compares all 585 value sequences of up to three values from an
|
||
eight-value pool (NaN, both zeros, infinity, two texts, an error) on all five functions. Rivals, each
|
||
failing it by name: unanimity ignored (`Others::One` kept for any second value), a number beside a
|
||
unanimous text read as unanimous, and `<` for the order of the zeros (the sign of a zero extreme is
|
||
lost). The first two also fail `aggregating_differing_text_values_is_an_explicit_error` and
|
||
`aggregating_text_among_numbers_is_an_explicit_error`; the zero rival also fails
|
||
`eager_math_tests::min_and_max_of_both_zeros_do_not_depend_on_their_order`.
|
||
|
||
### model::source::tests::a_merge_of_summaries_is_the_summary_of_the_union
|
||
|
||
Review of #356 (Opus): the pool gained `1e16`, `1.0`, `-1e16`, `0.1` and `0.2`. A merge rounding each part's
|
||
sum before pooling, keeping the zero sign, changed 514 production answers while the workspace passed;
|
||
that rival now fails this test, `numbers_pool_what_the_formula_aggregate_pools` (a NaN-free split now,
|
||
compared as bits), `summaries_built_from_parts_merge_as_the_union_does` and the fixture test below, by
|
||
name: four tests, for each of the three round-then-pool variants.
|
||
|
||
A read of several parts is a merge. The test merges every ordered pair of sequences of up to two
|
||
values and compares with the reference of the concatenation. Rivals: the least not merged, the
|
||
greatest merged with the other side's least, the zeros unordered. Each fails it.
|
||
|
||
### model::source::tests::numbers_pool_what_the_formula_aggregate_pools
|
||
|
||
`eval_agg` pools `Numbers` now, not a concatenated `Vec<f64>`. Every split of a five-number list
|
||
(a NaN and both zeros among them) pooled by merge must equal the whole, by bits for all five
|
||
functions and for the total. Rivals: the least not merged, the greatest seeded from the other
|
||
side's least. Both fail it.
|
||
|
||
### model::source::tests::the_store_and_a_scanned_table_answer_every_select_alike
|
||
|
||
`DataStore` and `TableSource` (a scan, sharing only `Summary`) are asked every select over the
|
||
fixture, at both widths. Rivals, each failing it: the store ignores `exact` for a select with no
|
||
span, and the store ignores `exact` for a spanned one. The first also fails
|
||
`an_exact_select_with_no_span_names_one_cell`.
|
||
|
||
### model::source::tests::the_store_and_a_scanned_table_list_the_same_cells
|
||
|
||
`keys` and `cells` under every base of the fixture, and `value_at` for every row and an absent key.
|
||
Rival: `DataStore::keys` ignores its partial. It fails here and in seven evaluator tests that read
|
||
the member records of an aggregate over an expression
|
||
(`route_then_aggregate_tests::a_routing_formula_at_a_record_is_the_rule_itself` among them).
|
||
|
||
### model::source::tests::an_exact_select_with_no_span_names_one_cell
|
||
|
||
A select with `exact` and no span is the cell, not the cells containing the pairs. The store's
|
||
`spanned_values` is never called for it (the evaluator reads an exact key through `value_at`), so
|
||
this is the contract a second source implements. Rival: `exact` ignored when there is no span.
|
||
|
||
### model::over_tests::a_model_over_a_source_evaluates_as_the_model_that_owns_the_cells
|
||
|
||
improvise-rnak. A model with its store emptied and `over` a `TableSource` of the same rows is asked
|
||
every read the evaluator makes (exact and over-hidden values, covered by a view that hides East,
|
||
spanned over none, one and two categories) at every sub-key of every stored key, a group named in
|
||
place of a member, and every formula target; the answers must agree to the bit. The fixture has a
|
||
ref, an aggregate of a ref, an aggregate of an expression, a filtered formula, a cycle and a
|
||
formula over formulas, and 2,000+ answers are compared. Each rival replaces one read with the owned
|
||
store, and fails it by name: the member records of an aggregate over an expression, the pooled
|
||
formula aggregate's raw read, the fold of a group, the exact read, and the stored cell a filtered
|
||
formula falls back to (a `Bonus` cell stored where its `WHERE` does not hold; before that cell
|
||
was added to the fixture this rival passed).
|
||
|
||
### model::over_tests::the_formula_cells_of_a_model_over_a_source_are_the_owned_models
|
||
|
||
`recompute_formulas` over a source under three None sets: the formula cache must equal the owned
|
||
model's at every key, and at least 40 cells are present across the three (a cache that is empty on
|
||
both sides would pass the comparison). Rival: the stems read from the owned store, which finds no
|
||
stem and fills no cell; it fails here and nowhere else.
|
||
|
||
### model::over_tests::a_model_over_a_source_does_not_read_its_own_store
|
||
|
||
A cell is set in the over-model's own store (the only cell it holds) with a value no fold could
|
||
mistake. The exact read and the fold of its measure must be the source's. Rivals: a fold or an exact
|
||
read of the owned store fail it.
|
||
|
||
### model::over_tests::the_records_a_drill_freezes_are_read_from_the_source
|
||
|
||
`drill_snapshot` (records mode's rows) over the empty store and a source. Rival: the unconstrained
|
||
branch reads the owned store; it fails here.
|
||
|
||
### model::over_tests::a_model_over_a_source_refuses_a_write
|
||
|
||
`set_cell` on a model over a source panics in a debug build, naming the reason. Without the assert the
|
||
write goes to the owned store and nothing reads it.
|
||
|
||
### a_summary_built_from_an_engines_parts_reduces_as_one_built_from_the_values
|
||
|
||
Review of #356 (Codex): an engine outside core could not build a `Summary` without replaying rows. The
|
||
parts are computed in the test without `Summary::of`, for every sequence of up to three values of the
|
||
pool. No rival of the first version failed this test alone (its filter was untested); see below.
|
||
|
||
### summaries_built_from_parts_merge_as_the_union_does
|
||
|
||
A summary built from parts, merged with one built from values and with one built from parts, equals the
|
||
reference of the union, for every pair of sequences of up to two values. This is where a bad extreme
|
||
would matter, since merge reads the extremes.
|
||
|
||
### parts_no_sequence_of_values_could_produce_are_refused
|
||
|
||
Review of #356 (Opus, Codex): `from_parts` repaired some impossible states and accepted others
|
||
(`One(Number)`, least above greatest). It refuses all of them with a `BadParts` naming the reason.
|
||
The earlier entry's claim that a NaN-extreme rival failed `parts_that_contradict_themselves_...` was
|
||
false: that test passed with the filter removed. Rivals now, each failing this test alone: NaN extreme
|
||
allowed, least above greatest allowed, extremes with no numbers allowed, a number accepted among the
|
||
others, a sum with no numbers allowed, and no extremes beside a sum that is not NaN allowed.
|
||
|
||
### the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts
|
||
|
||
An exactness test needs inputs whose float sum is inexact and no NaN. `1e16 + 1.0 - 1e16` rounded part
|
||
by part is `0.0`; pooled exactly it is `1.0`. The merge runs in both orders, so whichever part a rival
|
||
rounds is the inexact one: rounding only the other part, only self, or both each fail it (the first
|
||
version merged one way and a rival rounding only the exact part passed it; Opus caught that).
|
||
|
||
### keys_of_a_store_are_its_cells_keys_without_the_values
|
||
|
||
Review of #356 (Codex): `DataStore::keys` went through `cells` and cloned every value. It reads keys
|
||
only now; the test pins the keys and their order against `cells`. No clone-count test: a value clone is
|
||
not observable from outside, so the equality is all that can be asserted.
|
||
|
||
### a_model_over_a_source_refuses_a_clear
|
||
|
||
Review of #357: the `clear_cell` guard had no test. The refusal is an `assert!`, so the test holds in a
|
||
release build too (`cargo test --release -p improvise-core --lib over_tests` was run: the earlier
|
||
`debug_assert!` failed all four refusal tests there, `a_model_over_a_source_refuses_a_write` among
|
||
them, and was the bug). Rival: the guard removed from `clear_cell` fails this test only.
|
||
|
||
### a_model_over_a_source_refuses_to_remove_a_category
|
||
|
||
`remove_category` swept the owned store with no guard. Rival: the guard removed.
|
||
|
||
### a_model_over_a_source_refuses_to_remove_an_item
|
||
|
||
The same for `remove_item`.
|
||
|
||
### putting_a_populated_model_over_a_source_drops_its_own_cells
|
||
|
||
`Model::over` empties the owned store, so `get_cell` on an over-model is `None` for a cell the
|
||
source holds, and the evaluator still reads the source's value.
|
||
|
||
### records_mode_reads_the_records_from_a_source
|
||
|
||
Review of #357: a records-mode `GridLayout` over a source. `group_records` reading the owned store
|
||
left all 563 tests green; with this test it fails (the grid has no rows).
|
||
|
||
### putting_a_model_with_formula_cells_over_a_source_drops_what_those_cells_computed
|
||
|
||
Review of #357 (Codex): `Model::over` emptied the store and kept `formula_cache`, which was computed from
|
||
those cells; reads consult the cache first, so old formula values survived until a recompute. The model
|
||
has a computed Profit, the source holds a different revenue, and the cell must be gone after `over`.
|
||
Rival: the `formula_cache.clear()` removed fails this test alone (before the fix it failed here first).
|
||
`measure_agg` and the categories are the definition and stay; nothing else is derived from the cells.
|