1a359b4ae6
A collapsed group is written to CSV as its own total row, because an export writes what the grid draws (improvise-iot6). core.md §14 carries the argument; the evidence entries record each rival and where it failed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
1364 lines
72 KiB
Markdown
1364 lines
72 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.
|
||
|
||
### a_deserialized_store_can_resolve_its_records
|
||
|
||
The record index is derived state and is `#[serde(skip)]`-equivalent — it must
|
||
come back *with* the data, rebuilt through `set`, rather than being written
|
||
alongside it.
|
||
|
||
## `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.
|
||
|
||
### every_axis_gets_its_own_color
|
||
|
||
Distinct axes must be distinguishable at a glance; `color()` is the only reason
|
||
`AxisColor` exists, so a collision defeats it.
|
||
|
||
### 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. 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 (the
|
||
formula-target branch taken for stored values) — 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.
|