diff --git a/Cargo.toml b/Cargo.toml index 86f54e65..bda00df3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,9 @@ improvise-command = { path = "crates/improvise-command" } ratatui = "0.30" crossterm = "0.29" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } anyhow = "1" thiserror = "2" indexmap = { version = "2", features = ["serde"] } diff --git a/context/code/command.md b/context/code/command.md index a943e8bf..d6d2756f 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -935,6 +935,52 @@ There is deliberately **no `dirty = true`** in it: it declares of `MarkDirty` — harmless, and a second home for the one thing that commit set out to give exactly one. +### `RecordRewrite` — a row's edits composed, not replayed (improvise-uo2p) + +A staged drill edit is a statement about the record **as the snapshot showed +it**. It is not a statement about the model as the previous edit left it. The +edits in one row do not commute when they are replayed one at a time. A rename +moves every cell in the row (gotcha 1). A measure edit replayed after it writes +the old key, which leaves a ghost record there and a stale value in the moved +record. A second rename replayed after the first finds nothing at the old keys, +so it is dropped. `ApplyAndClearDrill` replayed them in `HashMap` order, so the +outcome was different from one App to the next. That is gotcha 18's class of +bug, found in an effect rather than an index. + +**Sorting is not the fix.** "Measures before renames" repairs the ticket's own +case and still drops a second rename every time (rival 1 in the evidence). Any +fixed order makes the corruption reproducible instead of removing it. So the +rule is **build each row's final record once, then write it**: + +- `RecordRewrite::of(model, row, edits)` collects the row's renames and measure + edits into maps keyed by column, and reads each unedited value off the model + **before anything is written**. It then states the rewrite: clear every old + key, and write every cell at the key with *all* renames applied. A measure + edit that is staged replaces the carried value. The result does not depend on + the order of the edits. A drill stages at most one edit per `(row, column)`, + so no two edits in a row name the same column. A blank rename is refused, and + the row's other edits still land at the unrenamed coordinates, as before. +- `RecordRewrite::merged` concatenates the rows' rewrites **by phase**: every + refusal, then every item added, then every clear, then every write. The clears + come first so that a swap is a swap. Rows are taken in snapshot row order + (`BTreeMap`), so where two final records land on one cell, **the later row + wins**. That is the answer plain records mode gives for the same renames made + top to bottom. + +The collision rule is a **choice**, and it still loses the earlier row's values +silently. Plain records mode loses them the same way when a rename lands on an +existing record. Refusing the colliding rows instead would be a change to both +paths. + +**One copy.** The plain records path (`commit.rs::commit_plain_records_edit`) +calls the same `RecordRewrite::of` with its one edit, so `rename_record_coordinate` +and the drill's `apply_coordinate_rename`/`apply_measure_edit` are gone. This is +the consolidation improvise-fdk1 asked for, "regardless" of its wire half. For a +single edit the output is the effects the plain path used to emit, reordered from +`clear, set` per cell to all clears then all sets, which touches distinct keys. +One behaviour moved on the drill side: it used to discard `add_item`'s error, and +now goes through `effect::AddItem`, which refuses when the category has gone. + ### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae) A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains @@ -2629,8 +2675,10 @@ At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortCh stages in drill pending edits while in drill mode, or applies directly in plain records mode; for real keys it writes to the model. -`rename_record_coordinate` renames one coordinate of a records row. In wide format a -row is a group of cells — one per measure — so the rename has to move **every** one +`commit_plain_records_edit` hands a coordinate or measure edit to +`effect::RecordRewrite::of`, the one home it shares with the drill (improvise-uo2p; +see `RecordRewrite` above). It replaced `rename_record_coordinate`. In wide format a +row is a group of cells — one per measure — so a rename has to move **every** one of them, not just the cell under the cursor (improvise-c5v, gotcha 1). Every cell in the row moves, and a row seeded by `add-record-row` has no measure cells yet: its coordinates live on a lone measure-less cell, which must move too or the row loses @@ -3187,8 +3235,8 @@ the cursor**. The result was silent corruption of an unrelated record: appending a row changes `view_frame`'s hash, the frame observation inside `apply_effects` invalidates the cursor to the origin *mid-batch* (gotcha 24), and `EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate. -Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every -measure cell of that row. +Enter commits through `rename_record_coordinate` (since improvise-uo2p, +`RecordRewrite::of`), which by gotcha 1 moves every measure cell of that row. Two things are worth carrying past this fix. @@ -3262,3 +3310,84 @@ key the two questions agree, because a cursor key never carries a None-axis category. The **collapsed-group fold** half of improvise-l4mp is not covered: its key carries the group's name as an ordinary coordinate, so no question about the key alone can see it — it needs `GridLayout::folds_a_group`. + +## A command is a function of its context — the determinism law (improvise-hzjo) + +`cmd/determinism.rs` holds the law the old `law_toggle_group_involution` claimed +and could not state. For **every** registered command and every argument list +that builds it, the effects `execute` returns must be equal, by `Debug` text, +across contexts built **independently from equal data**: four `App`s, each +parsed from `web/demo.improv` and given the same buffers, expanded categories, +yank, panels and cursor. + +**Why the instances must be distinct.** The old test called `execute` twice +against one `CmdContext`. Every `HashMap` it reached was the same instance with +the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`, +which nothing can fail. Separately built `App`s give every map its own seed. So a +command whose effects follow hash iteration order disagrees with itself. Hash +order is the one kind of nondeterminism this repo has actually shipped (gotcha +18). This law can see it where a command's `execute` builds it, as the drill +snapshot's record order was. It cannot see the other shipped instance, the +analyzer's `HashSet`: `analyze_records` is reached only from inside +`Effect::apply`, which is outside this law (see below). The companion test +`only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that +property checked: a test-only command that emits `ctx.buffers` in iteration +order must agree with itself on one context and disagree across two. + +**Four instances, not two.** A rival that takes *the first* entry of a map, not +all of them, agrees across two seeds whenever both happen to put the same entry +first. With 21 buffers that is roughly one chance in 21, and three comparisons +make it negligible. The fixture's values are also chosen to stay distinct under +the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses +to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass. + +**The store needs several records to order, so the drills come from the +document (improvise-e1sm).** The registry's `candidate_args` are extended with +names the demo really has, because coordinates that match nothing never reach +the records a drill or a fit would order. A hand-picked name is not enough. The +first version's only real drill was `Product/Latte, Quarter/Q2`: one record, two +cells. With the store's sort removed, two contexts put those two cells in the +same order about half the time, and with three comparisons the law passed about +one run in eight (measured: 46 of 400). So the arguments now include **every +item of every regular category as a one-coordinate drill**, read off the parsed +document. A one-coordinate drill spans every other category whole, so it matches +the whole slice of records under that item, not one record picked by name. In +the demo that is four records and eight cells for each of the eight drills. The +test +also asserts that the widest candidate it actually passes matches at least four +records. That guard catches a demo with too little data, and it catches the +derived drills being dropped from the argument list. It does **not** catch the +comparison losing its wide drills some other way, because it measures what the +argument lists match, not what the built commands ordered (improvise-b9f2). The +review showed this with a rival: a `drill-into-cell` parser that refuses fewer +than two arguments sends every one-coordinate drill to the one-record cursor +fallback. The guard still reports four records, and with the store's sort +removed the law passed 38 of 300 runs. With the store's sort removed and the +parser intact, the law fails 400 of 400 runs. + +**What it cannot see.** Only commands and arguments the fixture reaches. A +command whose nondeterminism needs some other state, such as records mode, a +wizard or a drill in progress, is outside this fixture. And `Debug` equality is +text equality: two effects that differ only in something their `Debug` omits are +equal here. + +**Its scope is what `execute` returns, not what `Effect::apply` does.** The law +compares the effects a command hands back. Many effects are unit structs that +decide all their work inside `apply`: `ApplyAndClearDrill`, +`SortData`, `CleanEmptyRecords`, `Save`, `RebuildLayout`, `AcceptWizardModel`. +Hash-order iteration inside an `apply` is therefore invisible here, however +often the law runs. improvise-uo2p is the worked example. +`ApplyAndClearDrill::apply` walks `drill.pending_edits`, a `HashMap`, in hash +order. A measure edit and a category rename staged on one row can then write a +ghost record. The command that emits it passes this law, because the effect's +`Debug` is just its name. So "the one kind of nondeterminism this repo has +shipped" is guarded here only on the `execute` side. The `apply` side needs its +own test per effect, or a law over `App` state after `apply_effects`. + +**The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately +depend on something outside the context, such as a clock or an entropy source, +each with its reason. It is empty: `fit-measure`'s seed is an argument with a +fixed default, not a clock read. The list is checked in both directions. An +unlisted command that diverges fails the test, and so does a listed one that +does not diverge. A stale entry therefore cannot linger, and a new registration +is covered the day it lands. diff --git a/context/code/protocol.md b/context/code/protocol.md index c79f0e56..032feab5 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -756,28 +756,54 @@ coming back one ULP off, both in `CellDisplay::value` (host to client, so `yy` copied a neighbour) and in a `SetCell` (client to host, so a typed or pasted number landed as a neighbour). -The feature is on for **every crate that parses a frame**: `improvise-ws-server` -(`transport.rs`), `improvise-worker-server` (`worker.rs`) and -`improvise-wasm-client` (`socket.rs`). Each crate's own manifest names it, -because each browser bundle is built from one crate alone. The `wasm` CI job -and `wasm-pack` both invoke per crate, and feature unification cannot reach -across separate invocations (`context/repo-map.md`, `## Wasm`). Enabling it in -one crate and relying on the others to inherit it works in `cargo test` and -nowhere else. +The feature is on for **every workspace crate with a normal dependency on +serde_json** (improvise-dz8c). Each crate's own manifest names it, because each +build is made from one crate alone: the `wasm` CI job and `wasm-pack` invoke +per crate, and feature unification cannot reach across separate invocations +(`context/repo-map.md`, `## Wasm`). Enabling it in one crate and relying on the +others to inherit it works in `cargo test` and nowhere else. -That is also why the end-to-end tests cannot guard this alone. They run in one -unified build with `improvise-ws-server` as a dev-dependency, so they stay -green with the feature removed from the browser bundle. -`every_build_that_parses_a_frame_enables_float_roundtrip` asks `cargo tree` -per crate over normal edges instead. A new crate that parses frames belongs in -its list. +**Why "can parse" and not "parses a frame".** The first version of the rule +covered only the three crates that parse frames and listed them by hand. +Frames are not the only place a user's number crosses a JSON parser. +`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import +(`StartImportWizard`, `ImportJsonHeadless`) and the root binary's +`improvise import` all parsed with the lossy default, and in the TUI binary +nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285` +stored as `…283`). The worker bundle happened to be exact through unification, +so one import gave different numbers in the two front ends. "Parses JSON input" +cannot be read off a manifest, but "has a normal serde_json dependency" can, and +it over-approximates in the cheap direction: the feature changes only parsing, +so a crate that only serializes pays nothing for it. Dev-dependencies are out, +because a test build ships nowhere. That also leaves out `gp-tree`, whose only +serde_json is a dev-dependency (its example CLI reads a saved model). Every +number it persists is an `f32`, and 3M random `f32`s all read back exactly +through the lossy parser, against about 30% of random `f64`s. -Not covered, and filed separately rather than widened into this change -(improvise-6yvg): `improvise-io`'s JSON import and `improvise-command`'s JSON -reads also parse numbers with the default parser. They are file formats, not -frames. In the TUI binary nothing enables the feature. In the worker bundle -unification happens to reach them, so one import can give different numbers -in the two front ends. +**Why a declaration and not the build graph.** The first check asked +`cargo tree` whether each listed crate's *build* had the feature. That answer +can be yes by accident. `improvise-command` depends on `improvise-io`, so with +the feature removed from command's manifest, command's own test build still +has it, and its import test stays green. The rule requires the crate's own +manifest to say it, so a later change to a dependency cannot take it away. + +**Why not enable it once, in `improvise-protocol`.** That removes the list, but +it would give the protocol a normal serde_json dependency it has no use for +(its repo-map entry says core and serde only). It would also reach only builds +that include the protocol, and `improvise-io` does not depend on it. Under this +rule the protocol's one serde_json dependency, the optional `test-support` one, +carries the feature too. That is off in every shipped build, so it costs +nothing. + +`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`) +reads `cargo metadata --no-deps` and names every package, and every +target-specific entry, whose normal serde_json dependency lacks the feature. A +new crate is covered the day it declares the dependency. It does not reach +`harness/opfs-contract`, which is its own workspace and only serializes. + +The end-to-end tests cannot guard this alone. They run in one unified build +with `improvise-ws-server` as a dev-dependency, so they stay green with the +feature removed from the browser bundle. ### `AxisFrame` — the grid's frame, and why it is *one* type diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 57eb87f6..15ff6714 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4626,24 +4626,12 @@ before #178 too, as the review noted. **Fault injected:** the same pre-fix tree. Fails: the host stored `4598108823619341467` in South/Cost. -### `every_build_that_parses_a_frame_enables_float_roundtrip` - -The two tests above run in **one** feature-unified test build, which includes -`improvise-ws-server` and `improvise-command` as dev-dependencies. So they pass -whenever **any** crate in that build enables the feature. The shipped browser -bundle is built from one crate alone, so it would still be lossy. That was -demonstrated: with the feature removed from `improvise-wasm-client` only, both -tests stayed green while `cargo tree` showed no `float_roundtrip` in the wasm -bundle. This test asks cargo instead, once per crate that parses a frame: -`cargo tree --locked --edges normal,features --invert serde_json --package -`, with `--target wasm32-unknown-unknown` for the two bundles. It uses -normal edges only, because a dev edge would unify the feature back in -(`context/repo-map.md`, `## Wasm`). - -**Faults injected:** the feature removed from `improvise-wasm-client` alone, -which fails naming that crate while the two tests above pass. Then removed from -`improvise-ws-server` alone, which fails naming it. The native path runs -without `--target`, so the two arms are different invocations. +**Retired: `every_build_that_parses_a_frame_enables_float_roundtrip`.** It asked +`cargo tree` about three crates listed by hand. improvise-dz8c replaced it with +`every_crate_that_can_parse_json_enables_float_roundtrip`, whose entry is in +`tooling.md`. The injections recorded for it still hold: with the feature +removed from `improvise-wasm-client` alone, the two tests above stay green, so +they cannot guard the bundle alone. ### resolve::tests::every_grid_move_lands_where_its_binding_says @@ -4677,6 +4665,43 @@ laundering invisible (the edge) is also the one that makes a wrong-direction jump visible, so moving off the edge fixed one blind spot and opened the other, and each needs its own question. +**improvise-w7qd, and the floor was the hole.** As first written the test +covered `move-selection` and a four-entry jump table, behind `pressed >= 14`. +Two things passed the whole workspace, both found in re-review of #180 at +544744c: negating the `scroll-rows` and `page-scroll` arms, so `Ctrl-d`/`Ctrl-u` +and `PgDn`/`PgUp` all scroll the wrong way (advertised, and still "acting" from +the centre); and dropping the jump table *with* `$` sent to the first column, +because the sixteen `move-selection` sequences met the floor on their own. The +test now walks **every** command Normal binds. `scroll-rows` and `page-scroll` +state their landing from their own argument exactly as `move-selection` does — +from the centre of a 3x3 grid any step of at least one lands at `1 + d` +clamped, so a sign is visible whatever the page size — and a command that moves +the cursor (in the same view) without a stated landing **fails** rather than +being skipped. So a dropped table entry is caught by derivation, not by a count: +under the second rival it fails naming every jump as moving with nothing stating +where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on +`Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at +least once guards the other direction — a binding vanishing — and is a named +list on purpose, since that is the one thing no walk over bindings can see. + +**improvise-5kdr: the 3x3 fixture saw only a sign.** From the centre of a 3x3 +grid every step of one or more clamps to an edge, so the w7qd version stated +`page-scroll` as `1 + d` — one row, which is not what it does — and passed +because the true landing clamped to the same edge. Measured on #198 at 8772db2: +`page-scroll` ignoring the page size together with `scroll-rows` moving one row, +or `move-selection` moving `2d`, left the test green. The walk now stands at +(10,10) of a 21x21 grid with an 8x8 window, far from every edge, so a landing +shows magnitude: `10+d` for `move-selection` and `scroll-rows`, `10 + page·d` +for `page-scroll`, 0/20 for the jumps. The page size is not written into the +test: `grid_move`'s page rule moved into `page_rows(window_rows)` and the test +asks it, so the rule has one home. Rivals, each run: `move-selection` doubled +fails on `Up` (`move-selection ["-1","0"]`); the page-size-and-one-row pair +fails on `PgUp` (`page-scroll ["-1"]`); the one-row `scroll-rows` alone fails on +`Ctrl-u` (`scroll-rows ["-5"]`); the jump-table rival still fails naming every +jump. The residual, stated: because the test reads `page_rows`, a change to the +page rule itself moves the test with it — whether that rule is the TUI's is +`tests/lockstep.rs`'s question, not this one's. + ### an_edit_is_refused_when_the_cell_was_written_while_the_editor_was_open improvise-0j0f, found by the review of PR #188 at 48f52e7 and confirmed diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index f085ebb4..eb785201 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5314,6 +5314,63 @@ before `refuse_and_stop` was applied to its arms. For `load`, the injected fault is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test then fails with "`load absent.improv . q` quit having loaded nothing". +### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename + +improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and +applied each edit alone against the model as the previous one left it. A rename +moves the row's cells. A measure edit then wrote `row.cell_key(measure)`, the +snapshot's old key. So when the rename came first, a ghost record appeared at the +old coordinates carrying the new value, and the moved record kept the stale one. +The reviewer saw this in 16 of 40 fresh Apps. The fixture stages two measure edits +and a rename on drill row 1, so a wrong answer needs only one of the three orders +that put the rename last. Each assertion runs over 32 fresh Apps and compares the +**whole** store, so the ghost and the stale value are both visible. Before the +fix it failed on five of five runs. Rival 3 (the rewrite addresses measure edits +at the unrenamed key) fails it. + +### ui::effect::tests::a_drill_row_renamed_twice_and_edited_lands_as_one_record + +improvise-uo2p. This is the case that fails **every** time, not with some +probability. Each rename read the snapshot's old keys. The second rename found +nothing there, because the first had moved it, so one rename was always dropped +and the row split into a `Clothing/Feb` record and a `Drink/Jan` record. Rival 1 +is the ticket's proposed fix direction: measures before renames, each edit +applied alone. It passes the ticket's own case and fails here. So "a fixed +order" is not a fix; the renames have to be composed. + +### ui::effect::tests::a_drill_that_swaps_two_records_coordinates_swaps_them + +improvise-uo2p. Row 0 is renamed onto row 1's coordinates and row 1 onto row 0's. +If the rows are applied in sequence, the first move overwrites the other record +and the second move carries the overwritten values back, so one record's data +is lost in either order. Rival 2 composes each row correctly but writes it +before reading the next row, and it fails here and nowhere else. That is why +`RecordRewrite::merged` reads every row off the model first, then clears every +old key, then writes. + +### ui::effect::tests::a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates + +improvise-uo2p. Row 1's staged `Cost` belongs to row 1's record, even though row +0 is being renamed away from coordinates and row 1 onto them. Before the fix, +some hash orders left `Clothing/Jan/Cost = 999` as a ghost. Rival 3 fails it. + +### ui::effect::tests::two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row + +improvise-uo2p. Two rows renamed onto the same coordinates collide per cell. The +rule chosen is that the later snapshot row wins, which is what plain records +mode gives when the same renames are made top to bottom. Before the fix the +winner was the hash order's choice. Rival 4 reverses the row order in `merged` +and fails here. The rule is a choice, and it still loses data silently. See +`context/code/command.md`, `RecordRewrite`. + +### ui::effect::tests::a_record_rewrite_is_the_same_under_every_order_of_its_edits + +improvise-uo2p. The composition is order-independent **by construction**, so it +is asserted over all 24 orders of four edits on one row: two renames, a measure +write and a measure clear. The test pins one value as well as the equality, so +a rewrite that is the same wrong answer in every order still fails. Rival 3 +fails it. + ## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw) ### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included @@ -5337,3 +5394,83 @@ Rivals, injected, full workspace run: *else* fails: `renders_each_tile_with_its_axis_label` (TUI) and `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one is improvise-bd9o's own test of the route this decision keeps. + +## `command/cmd/determinism.rs` — the determinism law over every command (improvise-hzjo) + +The reasoning is in `context/code/command.md`, "A command is a function of its +context". This entry records what each test can see. + +### every_command_gives_equal_effects_in_independently_built_equal_contexts + +It runs all 89 registrations, 1500+ (command, args) pairs, in four contexts built +from four separately parsed copies of `web/demo.improv`, and requires equal +effect `Debug` text. It passed at `ae96b8c` with no exemptions. It is not a +regression test for a live bug. It restores a law, and the evidence is which +rivals it catches: + +- **`command-mode-backspace` reading `ctx.buffers.values().next()`** instead of + the `command` buffer: a real command following hash order. Injected, full + workspace run: this test is the **only** failure, because every + `text_buffer.rs` test builds a one-entry buffer map. With the comparison + switched to one reused context (`compare(&a, from_ref(&a))`) the same rival + **passes**. That is the half the ticket required: the distinct construction + is what does the work. +- **`DataStore::matching_positions` without its sort**: gotcha 18 re-shipped. + This entry first said the test catches it through `drill-into-cell` and + `fit-measure`'s record order. That was wrong (improvise-e1sm). `fit-measure` + never diverged, in 60 failing runs in review and in 354 here. And the catch + was a matter of luck. The only real drill was `Product/Latte, Quarter/Q2`, + which is one record with two cells, so each other context agrees with + probability 1/2. With the sort removed and the test binary run in a loop, the + law **passed 46 of 400 runs** at `fc0b13c`, and `drill-into-cell + ["Product/Latte", "Quarter/Q2"]` was the only pair that ever diverged. The + arguments now include every item of every regular category as a + one-coordinate drill, read off the parsed document (four records, eight cells + each in the demo). With the same injection the law **passed 0 of 400**, and + all eight new drills diverged in about 1195 of 1200 pairwise comparisons each. + Without the injection it passed 400 of 400. The three `cell.rs` order tests + still pin the store directly. +- **The derived drills dropped from the argument list** (the + `all.extend(one_coordinate_drills(model))` line commented out): the test + fails before comparing anything, with "the widest drill orders 1 records". + That is the only way of losing the wide drills the guard can see. It measures + what the argument lists match, not what the built commands ordered, so it can + pass while the comparison has lost its wide drills (improvise-b9f2). Review + rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, + which sends every one-coordinate drill to the one-record cursor fallback. The + guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 + runs. +- **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. + It fails as "listed … but did not". + +**A vacuity this test itself had on its first version.** The fixture's filler +buffers held `v0`…`v15`. The `command-mode-backspace` rival pops a character, +so every value collapsed to `v` (or `v1`), and the rival passed across two +distinct contexts. This is the "two derivations the fixture makes agree" shape. +The values are now `value {i} of 16`. The comparison was also widened from one +other instance to three, because a take-the-first rival agrees across two seeds +about one time in 21. + +### only_distinct_instances_can_see_a_command_that_follows_hash_order + +The check-the-checker behind the law. A test-only `EmitsInHashOrder` emits one +`SetBuffer` per `ctx.buffers` entry in iteration order. It must equal itself on +one context, which is why the old single-context law was vacuous, and differ +across two independently built ones. If a future refactor made the fixture +share one map, or share one `RandomState`, the second assertion fails. Without +this test the law would then pass everything and look healthy. + +## JSON import numbers (improvise-6yvg) + +### `a_json_import_stores_its_numbers_bit_for_bit` + +`ImportJsonHeadless` imports `[{"Region":"East","Revenue":0.24815714285714285}]` +and the test reads Region=East, _Measure=Revenue back by `f64::to_bits`. +`StartImportWizard` parses through the same `serde_json::from_slice` call, so +one test covers the crate's feature. + +**Red at `1158c23`** in `cargo test -p improvise-command`: stored +`0.24815714285714283`. That reproduces improvise-6yvg. **Limitation:** this test +cannot see the feature removed from `improvise-command` alone, because +`improvise-io`, a normal dependency, enables it in the same build. That case is +held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`). diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index d27feb93..f51186c9 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1361,3 +1361,26 @@ 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. + +### 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. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index daf7a527..48e8bd5a 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1434,3 +1434,17 @@ rather than defaulted: the alternative — expanding the group in the export — reintroduce the iot6 divergence between what the user sees and what they export. On the pre-6qet tree the body is empty (the group had no row, and pruning left nothing), so the test fails there. + +## Legacy JSON numbers (improvise-6yvg) + +### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit` + +A workbook with Food/Jan holding `0.24815714285714285` is serialized with +`serde_json` and loaded through `load_from`. A leading `{` sends it down the +legacy JSON path. The test first asserts that the text holds the exact decimal, +so what it measures is the parse. It compares `f64::to_bits`. Found by the +review of PR #190, not by the ticket. + +**Red at `1158c23`** in `cargo test -p improvise-io`: loaded +`0.24815714285714283`. Green once `improvise-io`'s own manifest enables +`float_roundtrip`, and red again with only that line reverted. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 9fea26b7..aa7d04d9 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1159,3 +1159,35 @@ failed: ex_usage_is_placeholders("q or press Q")`. The rule cost one row: `:add-items a b c ...` had three bare words and is now `:add-items ...`. This test pins the rule the macro asks, in both directions, including that shipped usage and the rival. + +### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for + +improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed +hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder +could call it directly instead of through its `keys` parameter — and +`help_bindings()` records only what goes through the parameter. Injected into +`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current +cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test +exists for — and the whole workspace stayed green, because both guards that +would object read `help_bindings()`. + +Two changes. `looked_up` is now private to the `key_row` module, whose one +caller is the module's own `keys` — the lookup `pages()` hands every builder — +so the reviewer's injection is `E0624: associated function looked_up is +private`. That narrows the door without closing it: a builder can still reach +the module-level `keys` by path (`self::keys(..)`) rather than through its +shadowing parameter. So this test states the premise directly — the number of +keys rows drawn equals the number of rows the builders asked for (every asked +row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) — +and the `self::keys` bypass fails it `51 != 50`. + +**improvise-aszt: a count is not the premise.** The test above compared +*counts*, so a bypass that also drops one real row keeps them equal. Injected in +`editing()`: call the parameter `keys(e, "clear-buffer", …)` and discard it with +`let _ =`, then return `self::keys(n, "clear-cell", None, "Edit the current +cell")` in its place. The asked row is recorded and never drawn, the drawn row +is never recorded, 50 = 50, and the whole workspace stayed green. The test now +compares **content in order**: the drawn keys rows must equal +`help_bindings()` looked up through the same `keys`, row for row. The same +injection now fails. What still passes is a bypass that draws exactly what the +builders asked for, in the order they asked — which is harmless by definition. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 0bdaca76..cd8ff234 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -681,3 +681,41 @@ reproduction. alone fails, exiting 0. All 44 others pass. That is the review's point: the older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it cannot see this rival. + +## Every JSON parser reads numbers exactly (improvise-dz8c) + +### `every_crate_that_can_parse_json_enables_float_roundtrip` + +It replaces a check over three crates listed by hand. It reads +`cargo metadata --no-deps` and fails naming every workspace package, and +every target-specific entry, whose **normal** serde_json dependency lacks +`float_roundtrip`. The rule and why it was chosen over the alternatives are in +`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`, +`improvise-command`, `improvise-io` and `improvise-protocol`. + +**Faults injected, each run:** + +- **The feature removed from `improvise-io` alone.** The check fails naming + `improvise-io (all targets)`, and so does + `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`. + `a_json_import_stores_its_numbers_bit_for_bit` stays green, because + `improvise-command` enables the feature and unification reaches io in its + build. +- **The feature removed from `improvise-command` alone.** The check fails naming + `improvise-command (all targets)`. The command crate's own import test + **stays green**: `cargo tree -p improvise-command` shows io's declaration + switching the feature on. So the manifest check is the only guard that + sees this case. It is also the case the retired build-graph check would + have passed. +- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on + `improvise-protocol` and `serde_json` and no feature (the lockfile updated, + as a real new crate would be). The check fails naming + `zz-throwaway (all targets)`. With the unconditional dependency carrying the + feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails + naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than + the build needs, since a wasm build unifies the two entries, and cheap to + satisfy. The crate was then removed. + +The `checked.len() >= 2` assertion is the positive signal. If a cargo change +renamed the metadata fields, the check would find no serde_json dependency and +pass vacuously without it. diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index 5f1bd1d9..acc61a1f 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -14,7 +14,9 @@ improvise-io = { path = "../improvise-io" } improvise-gp = { path = "../improvise-gp" } anyhow = "1" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } unicode-width = "^0.2" # Instant::now() panics on wasm32-unknown-unknown; web_time re-exports # std::time off-wasm and uses performance.now() on it (improvise-ywd). diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 6846f198..86e4e55d 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1,8 +1,8 @@ use crate::model::category::MEASURE_CATEGORY; -use crate::model::cell::{CellKey, CellValue}; +use crate::model::cell::CellKey; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; -use crate::view::{RecordCol, RecordRow}; +use crate::view::RecordCol; use super::core::{Cmd, CmdContext, read_buffer}; use super::grid::AddRecordRow; @@ -14,6 +14,7 @@ mod tests { use super::*; use crate::command::cmd::test_helpers::*; + use crate::model::cell::CellValue; use crate::workbook::Workbook; fn commit(kind: CommitKind) -> CommitBuffer { @@ -622,47 +623,8 @@ fn stage_drill_edit(record_idx: usize, col: RecordCol, value: &str) -> Box>, -) { - if value.is_empty() { - effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY)); - return; - } - effects.push(Box::new(effect::AddItem { - category: category.to_string(), - item: value.to_string(), - })); - - // Every cell in the row moves. A row seeded by `add-record-row` carries its - // coordinates on a lone measure-less cell, which must move too. - let cells: Vec<(CellKey, CellValue)> = if row.values.is_empty() { - model - .get_cell(&row.coords) - .map(|v| vec![(row.coords.clone(), v.clone())]) - .unwrap_or_default() - } else { - row.values - .iter() - .map(|(measure, v)| (row.cell_key(measure), v.clone())) - .collect() - }; - for (old_key, cell_value) in cells { - effects.push(Box::new(effect::ClearCell(old_key.clone()))); - effects.push(Box::new(effect::SetCell( - old_key.with(category, value), - cell_value, - ))); - } -} - -/// Apply a records-mode edit directly to the model (no drill state). +/// Apply a records-mode edit directly to the model (no drill state). A drill's staged +/// edits go through the same `RecordRewrite`, all of a row's at once (gotcha 19). fn commit_plain_records_edit( ctx: &CmdContext, record_idx: usize, @@ -674,11 +636,8 @@ fn commit_plain_records_edit( return; }; match col { - RecordCol::Category(category) => { - rename_record_coordinate(ctx.model, row, category, value, effects); - } - RecordCol::Measure(measure) => { - commit_regular_cell_value(&row.cell_key(measure), value, effects); + RecordCol::Category(_) | RecordCol::Measure(_) => { + effects.extend(effect::RecordRewrite::of(ctx.model, row, [(col, value)]).effects()); } RecordCol::NewMeasure => add_measure(value, effects), RecordCol::NewCategory => add_category(value, effects), diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs new file mode 100644 index 00000000..8f0a71e5 --- /dev/null +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -0,0 +1,233 @@ +//! A command is a function of its context: two contexts built independently from +//! equal data give equal effects — `context/code/command.md`, improvise-hzjo. + +use std::collections::{BTreeSet, HashSet}; + +use super::core::{Cmd, CmdContext}; +use super::registry::tests::candidate_args; +use crate::command::key::{KeyCode, KeyModifiers}; +use crate::model::Model; +use crate::model::cell::{CellKey, CellValue}; +use crate::model::record::record_of; +use crate::ui::app::App; +use crate::ui::effect::{self, Effect}; + +/// Commands whose effects depend on something outside the context, with why. +const OUTSIDE_THE_CONTEXT: &[(&str, &str)] = &[]; + +const DOCUMENT: &str = include_str!("../../../../../web/demo.improv"); + +/// One `App` over the shipped demo. Every context map holds several entries that +/// stay distinct under an edit like `pop()` — see the evidence. +fn fixture() -> App { + let workbook = crate::persistence::parse_md(DOCUMENT).expect("the demo parses"); + let mut app = App::new(workbook, None); + let vs = &mut app.view_state; + for i in 0..16 { + vs.buffers + .insert(format!("b{i}"), format!("value {i} of 16")); + } + for (name, text) in [ + ("edit", "42"), + ("formula", "Margin = Profit / Revenue"), + ("category", "Region"), + ("command", "add-item Product Mocha"), + ("export", "/tmp/improvise-hzjo.csv"), + ] { + vs.buffers.insert(name.into(), text.into()); + } + for cat in ["Product", "Quarter", "_Measure", "_Index", "_Dim"] { + vs.expanded_cats.insert(cat.into()); + } + vs.yanked = Some(CellValue::Number(7.0)); + vs.formula_panel_open = true; + vs.category_panel_open = true; + vs.view_panel_open = true; + vs.cat_panel_cursor = 2; + vs.view_panel_cursor = 1; + app.cursor_mut().selected = (1, 1); + app +} + +/// Every item as a one-coordinate drill: each spans the other categories whole, +/// so the store has several records to order — see the evidence. +fn one_coordinate_drills(model: &Model) -> Vec> { + let mut drills = Vec::new(); + for cat in model.regular_category_names() { + for item in model.category(cat).expect("listed").ordered_item_names() { + drills.push(vec![format!("{cat}/{item}")]); + } + } + drills +} + +/// How many distinct records the widest all-`Cat/Item` candidate matches. +fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { + let coordinates = |args: &Vec| -> Option> { + args.iter() + .map(|a| { + a.split_once('/') + .map(|(c, i)| (c.to_string(), i.to_string())) + }) + .collect() + }; + candidates + .iter() + .filter(|args| !args.is_empty()) + .filter_map(coordinates) + .map(|partial| { + let cells = model.data.matching_cells(&partial); + let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); + records.len() + }) + .max() + .unwrap_or(0) +} + +/// The registry's candidates, plus names the demo really has: a drill or a fit +/// over coordinates that match nothing never reaches the records it would order. +fn args_the_document_names(model: &Model) -> Vec> { + let mut all = candidate_args(); + all.extend(one_coordinate_drills(model)); + all.extend( + [ + vec!["Revenue"], + vec!["Product/Latte", "Quarter/Q2"], + vec!["Product/Latte", "Quarter/Q2", "_Measure/Cost"], + vec![ + "editing", + "5", + "Product/Latte", + "Quarter/Q2", + "_Measure/Cost", + ], + ] + .into_iter() + .map(|v| v.into_iter().map(String::from).collect()), + ); + all +} + +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { + let cmd = ctx.registry.interactive(name, args, ctx).ok()?; + Some(format!("{:?}", cmd.execute(ctx))) +} + +/// What running every registered command under `a` and under each of `others` found. +#[derive(Default)] +struct Comparison { + diverged: BTreeSet<&'static str>, + shown: Vec, + never_built: Vec<&'static str>, + compared: usize, +} + +fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { + let mut found = Comparison::default(); + let candidates = args_the_document_names(a.model); + for name in a.registry.names().collect::>() { + let before = found.compared; + for args in &candidates { + let Some(left) = effects_of(name, args, a) else { + continue; + }; + found.compared += 1; + for b in others { + let right = effects_of(name, args, b).unwrap_or_default(); + if left != right { + found.diverged.insert(name); + found + .shown + .push(format!("{name} {args:?}:\n {left}\n {right}")); + } + } + } + if found.compared == before { + found.never_built.push(name); + } + } + found +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { + let apps: Vec = (0..4).map(|_| fixture()).collect(); + let contexts: Vec = apps + .iter() + .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) + .collect(); + let model = contexts[0].model; + let widest = widest_drill(model, &args_the_document_names(model)); + assert!( + widest >= 4, + "the widest drill orders {widest} records, so the store's order is nearly a coin toss" + ); + let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.never_built.is_empty(), + "no candidate args build these in the fixture, so the law never ran them: {:?}", + found.never_built + ); + let listed: BTreeSet<&str> = OUTSIDE_THE_CONTEXT.iter().map(|(n, _)| *n).collect(); + let unlisted: Vec<&str> = found + .shown + .iter() + .filter(|s| !listed.iter().any(|n| s.starts_with(&format!("{n} ")))) + .map(String::as_str) + .collect(); + assert!( + unlisted.is_empty(), + "effects depend on more than the context ({} pairs compared):\n{}", + found.compared, + unlisted.join("\n") + ); + let stale: Vec<&&str> = listed + .iter() + .filter(|n| !found.diverged.contains(**n)) + .collect(); + assert!( + stale.is_empty(), + "listed as depending on something outside the context, but did not: {stale:?}" + ); +} + +/// Iterates a context `HashMap` and emits in that order: the one nondeterminism +/// this repo has shipped (gotcha 18). The law must see it; a reused context cannot. +#[derive(Debug)] +struct EmitsInHashOrder; +impl Cmd for EmitsInHashOrder { + fn name(&self) -> &'static str { + "emits-in-hash-order" + } + fn execute(&self, ctx: &CmdContext) -> Vec> { + ctx.buffers + .iter() + .map(|(name, value)| { + Box::new(effect::SetBuffer { + name: name.clone(), + value: value.clone(), + }) as Box + }) + .collect() + } +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn only_distinct_instances_can_see_a_command_that_follows_hash_order() { + let (first, second) = (fixture(), fixture()); + let a = first.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let b = second.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let run = |ctx: &CmdContext| format!("{:?}", EmitsInHashOrder.execute(ctx)); + assert_eq!( + run(&a), + run(&a), + "one reused context iterates its maps the same way twice" + ); + assert_ne!( + run(&a), + run(&b), + "two independently built contexts must iterate differently, or the law is vacuous" + ); +} diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index c7f86714..07edab36 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -13,6 +13,9 @@ pub mod text_buffer; pub mod tile; pub mod wizard; +#[cfg(test)] +mod determinism; + // Re-export items used by external code pub use self::core::{Cmd, CmdContext, CmdRegistry}; pub use registry::default_registry; diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 0534709e..dbd86d68 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -498,13 +498,13 @@ pub fn default_registry() -> CmdRegistry { } #[cfg(test)] -mod tests { +pub(super) mod tests { use super::*; use crate::command::cmd::test_helpers::*; /// Argument vectors to try when constructing a command, most specific /// first. A command is exercised by the first one its parser accepts. - fn candidate_args() -> Vec> { + pub(in crate::command::cmd) fn candidate_args() -> Vec> { [ vec![], vec!["1"], diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 18528e69..f07adea5 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::fmt::Debug; use std::path::PathBuf; @@ -663,24 +664,19 @@ impl Effect for ApplyAndClearDrill { let Some(drill) = app.drill_state.take() else { return; }; - if drill.pending_edits.is_empty() { - return; - } + let mut rows: BTreeMap> = BTreeMap::new(); for ((record_idx, col), new_value) in &drill.pending_edits { - let Some(row) = drill.records.get(*record_idx) else { - continue; - }; - match col { - RecordCol::Measure(measure) => { - apply_measure_edit(app, &row.cell_key(measure), new_value); - } - RecordCol::Category(category) => { - apply_coordinate_rename(app, row, category, new_value); - } - // Never staged: `commit_cell_value` applies a new measure or - // category immediately — a schema change is not a cell edit. - RecordCol::NewMeasure | RecordCol::NewCategory => {} - } + rows.entry(*record_idx) + .or_default() + .push((col, new_value.as_str())); + } + let model = &app.model_state.workbook.model; + let rewrite = RecordRewrite::merged(rows.iter().filter_map(|(record_idx, edits)| { + let row = drill.records.get(*record_idx)?; + Some(RecordRewrite::of(model, row, edits.iter().copied())) + })); + for effect in rewrite.effects() { + effect.apply(app); } // No `dirty = true` here: this declares `Model(CellData)` and // `App::apply_effects` raises the flag off that (improvise-gv9r). @@ -690,51 +686,114 @@ impl Effect for ApplyAndClearDrill { } } -/// Write one measure cell, or clear it when the edit is blank. -fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) { - let model = &mut app.model_state.workbook.model; - match new_value { - "" => { - model.clear_cell(key); - } - v => { - let value = v - .parse::() - .map(CellValue::Number) - .unwrap_or_else(|_| CellValue::Text(v.to_string())); - model.set_cell(key.clone(), value); - } - } +/// A records row's edits composed into the one record they mean, read off the model +/// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`. +#[derive(Debug, Default, PartialEq)] +pub(crate) struct RecordRewrite { + refused: bool, + items: Vec<(String, String)>, + clears: Vec, + sets: Vec<(CellKey, CellValue)>, } -/// Rename one coordinate of a records row. The row is a group of cells — -/// one per measure — so every one of them moves (improvise-c5v). -fn apply_coordinate_rename(app: &mut App, row: &RecordRow, category: &str, new_value: &str) { - if new_value.is_empty() { - app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY); - return; - } - let keys: Vec = if row.values.is_empty() { - vec![row.coords.clone()] - } else { - row.values - .iter() - .map(|(measure, _)| row.cell_key(measure)) - .collect() - }; - let _ = app - .model_state - .workbook - .add_item(category, new_value.to_string()); - for old_key in keys { - let Some(value) = app.model_state.workbook.model.get_cell(&old_key).cloned() else { - continue; +impl RecordRewrite { + /// `row`'s edits, each naming a distinct column. The result does not depend on + /// their order: every coordinate is renamed before any cell is placed. + pub(crate) fn of<'a>( + model: &crate::model::Model, + row: &RecordRow, + edits: impl IntoIterator, + ) -> Self { + let mut refused = false; + let mut renames: BTreeMap<&str, &str> = BTreeMap::new(); + let mut measures: BTreeMap<&str, Option> = BTreeMap::new(); + for (col, value) in edits { + match col { + RecordCol::Category(_) if value.is_empty() => refused = true, + RecordCol::Category(category) => { + renames.insert(category.as_str(), value); + } + RecordCol::Measure(measure) => { + measures.insert(measure.as_str(), improvise_protocol::typed(value)); + } + // Never staged: a new measure or category is a schema change, + // applied at once by `commit_cell_value`. + RecordCol::NewMeasure | RecordCol::NewCategory => {} + } + } + let renamed = |key: &CellKey| { + renames.iter().fold(key.clone(), |key, (category, item)| { + key.with(*category, *item) + }) }; - app.model_state.workbook.model.clear_cell(&old_key); - app.model_state - .workbook - .model - .set_cell(old_key.with(category, new_value), value); + let moved: Vec<(Option<&str>, CellKey)> = match (renames.is_empty(), row.values.is_empty()) + { + (true, _) => Vec::new(), + (false, true) => vec![(None, row.coords.clone())], + (false, false) => row + .values + .iter() + .map(|(measure, _)| (Some(measure.as_str()), row.cell_key(measure))) + .collect(), + }; + let mut rewrite = RecordRewrite { + refused, + items: renames + .iter() + .map(|(category, item)| (category.to_string(), item.to_string())) + .collect(), + ..Self::default() + }; + for (measure, old) in &moved { + let value = match measure.and_then(|m| measures.remove(m)) { + Some(edit) => edit, + None => model.get_cell(old).cloned(), + }; + rewrite.clears.push(old.clone()); + rewrite.sets.extend(value.map(|v| (renamed(old), v))); + } + for (measure, edit) in measures { + let key = renamed(&row.cell_key(measure)); + match edit { + Some(value) => rewrite.sets.push((key, value)), + None => rewrite.clears.push(key), + } + } + rewrite + } + + /// Several rows' rewrites as one: every clear lands before any write, so a + /// swap is a swap, and where two rows land on one cell the later row wins. + pub(crate) fn merged(rewrites: impl IntoIterator) -> Self { + rewrites.into_iter().fold(Self::default(), |mut all, one| { + all.refused |= one.refused; + all.items.extend(one.items); + all.clears.extend(one.clears); + all.sets.extend(one.sets); + all + }) + } + + pub(crate) fn effects(self) -> Vec> { + let refusal = self.refused.then(|| refuse(RECORD_COORDS_CANNOT_BE_EMPTY)); + let items = self + .items + .into_iter() + .map(|(category, item)| Box::new(AddItem { category, item }) as Box); + let clears = self + .clears + .into_iter() + .map(|key| Box::new(ClearCell(key)) as Box); + let sets = self + .sets + .into_iter() + .map(|(key, value)| Box::new(SetCell(key, value)) as Box); + refusal + .into_iter() + .chain(items) + .chain(clears) + .chain(sets) + .collect() } } @@ -3071,6 +3130,209 @@ mod tests { assert_eq!(app.model_state.workbook.model.get_cell(&key), None); } + // ── Composing a drill's staged edits (improvise-uo2p) ─────────────── + + fn drill_cell(ty: &str, month: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Type".into(), ty.into()), + ("Month".into(), month.into()), + ("_Measure".into(), measure.into()), + ]) + } + + /// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2), + /// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least. + fn two_record_drill() -> App { + let mut app = test_app(); + let cells = [ + (drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)), + (drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)), + ( + drill_cell("Clothing", "Jan", "Revenue"), + CellValue::Number(42.0), + ), + ( + drill_cell("Clothing", "Jan", "Cost"), + CellValue::Number(17.0), + ), + ]; + for (key, value) in cells.clone() { + app.model_state.workbook.model.set_cell(key, value); + } + let records = RecordRow::group(cells); + assert_eq!(records.len(), 2, "one row per coordinate tuple"); + assert_eq!(records[1].coords.get("Type"), Some("Clothing")); + StartDrill(records).apply(&mut app); + app + } + + fn stage(app: &mut App, record_idx: usize, col: RecordCol, value: &str) { + SetDrillPendingEdit { + record_idx, + col, + new_value: value.to_string(), + } + .apply(app); + } + + /// The whole store, as `(Type, Month, measure) = n`, so a ghost cannot hide. + fn drill_store(app: &App) -> std::collections::BTreeSet { + app.model_state + .workbook + .model + .data + .iter_cells() + .map(|(key, value)| { + let coord = |c: &str| key.get(c).unwrap_or("-").to_string(); + let n = value.as_f64().expect("every drill cell here is a number"); + format!( + "{}/{}/{} = {n}", + coord("Type"), + coord("Month"), + coord("_Measure") + ) + }) + .collect() + } + + fn category(name: &str) -> RecordCol { + RecordCol::Category(name.into()) + } + + fn measure(name: &str) -> RecordCol { + RecordCol::Measure(name.into()) + } + + /// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its + /// own `RandomState`, and one map iterates the same way twice. + fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) { + let expected: std::collections::BTreeSet = + expected.iter().map(|s| s.to_string()).collect(); + for attempt in 0..32 { + let mut app = two_record_drill(); + for (row, col, value) in edits { + stage(&mut app, *row, col.clone(), value); + } + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + assert_eq!(drill_store(&app), expected, "fresh App #{attempt}"); + } + } + + #[test] + fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() { + assert_drill_commits_to( + &[ + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + (1, category("Month"), "Feb"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Feb/Revenue = 42", + "Drink/Feb/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_rows_measure_edits_follow_its_rename() { + assert_drill_commits_to( + &[ + (1, measure("Revenue"), "100"), + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Jan/Revenue = 100", + "Drink/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_that_swaps_two_records_coordinates_swaps_them() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Clothing"), + (1, category("Type"), "Food"), + ], + &[ + "Clothing/Jan/Revenue = 1", + "Clothing/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 17", + ], + ); + } + + #[test] + fn a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Food"), + (1, measure("Cost"), "999"), + ], + &[ + "Drink/Jan/Revenue = 1", + "Drink/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_record_rewrite_is_the_same_under_every_order_of_its_edits() { + let app = two_record_drill(); + let row = app.drill_state.as_ref().unwrap().records[1].clone(); + let edits = [ + (measure("Cost"), "999"), + (category("Type"), "Drink"), + (measure("Revenue"), ""), + (category("Month"), "Feb"), + ]; + let model = &app.model_state.workbook.model; + let mut seen = Vec::new(); + for a in 0..4 { + for b in (0..4).filter(|&b| b != a) { + for c in (0..4).filter(|&c| c != a && c != b) { + let d = 6 - a - b - c; + let order = [a, b, c, d].map(|i| (&edits[i].0, edits[i].1)); + seen.push(RecordRewrite::of(model, &row, order)); + } + } + } + assert_eq!(seen.len(), 24); + let first = &seen[0]; + assert_eq!( + first.clears, + vec![ + drill_cell("Clothing", "Jan", "Revenue"), + drill_cell("Clothing", "Jan", "Cost") + ] + ); + assert_eq!( + first.sets, + vec![(drill_cell("Drink", "Feb", "Cost"), CellValue::Number(999.0))] + ); + assert!(seen.iter().all(|rewrite| rewrite == first)); + } + + #[test] + fn two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Drink"), + ], + &["Drink/Jan/Revenue = 42", "Drink/Jan/Cost = 17"], + ); + } + // ── Toggle effects ────────────────────────────────────────────────── #[test] @@ -3283,6 +3545,38 @@ mod tests { assert_eq!(app.view_state.status_msg, "Imported successfully"); } + #[test] + fn a_json_import_stores_its_numbers_bit_for_bit() { + const LOSSY: f64 = 0.248_157_142_857_142_85; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("data.json"); + std::fs::write( + &path, + r#"[{"Region":"East","Revenue":0.24815714285714285}]"#, + ) + .unwrap(); + + let mut app = test_app(); + ImportJsonHeadless { + path, + model_name: None, + array_path: None, + } + .apply(&mut app); + + let key = CellKey::new(vec![ + ("Region".to_string(), "East".to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]); + let Some(CellValue::Number(n)) = app.model_state.workbook.model.get_cell(&key) else { + panic!( + "the import must store the number; status {:?}", + app.view_state.status_msg + ); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "stored {n:?}, not {LOSSY:?}"); + } + // ── json_import_records (improvise-oaq helper) ────────────────────── #[test] diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 1105655f..fbb3e312 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -1741,4 +1741,66 @@ mod tests { "Sep is in no group — Q2's GroupEnd closed Q1's re-opened run" ); } + + /// improvise-t0ma: the steps themselves, not their translation into effects — + /// see context/test-evidence/core.md. + #[test] + fn records_steps_are_the_spine_then_every_non_index_dim_category_on_none() { + use super::{ViewStep, records_spine, records_steps}; + let mut m = crate::model::Model::new("T"); + m.add_category("Region").unwrap(); + m.add_category("Product").unwrap(); + let none = |c: &str| ViewStep::Axis { + category: c.to_string(), + axis: Axis::None, + }; + let mut expected = records_spine(); + expected.extend( + m.categories + .keys() + .filter(|c| !matches!(c.as_str(), "_Index" | "_Dim")) + .map(|c| none(c)), + ); + assert!( + expected.contains(&none("_Measure")) && expected.contains(&none("Region")), + "setup: the model carries _Measure and a regular category" + ); + assert_eq!(records_steps(&m), expected); + } + + /// improvise-t0ma: a drill FIXES its coordinates on Filter and frees only the + /// unfixed, non-virtual aggregated categories. + #[test] + fn drill_steps_filter_each_coordinate_and_free_the_rest() { + use super::{ViewStep, drill_steps, records_spine}; + let key = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("Month".into(), "Feb".into()), + ("_Measure".into(), "Revenue".into()), + ]); + let none_cats: Vec = ["Product", "Region", "_Index", "Customer"] + .map(String::from) + .to_vec(); + let fixed = |c: &str, i: &str| { + [ + ViewStep::Axis { + category: c.into(), + axis: Axis::Filter, + }, + ViewStep::PageSelection { + category: c.into(), + item: i.into(), + }, + ] + }; + let mut expected = records_spine(); + expected.extend(fixed("Month", "Feb")); + expected.extend(fixed("Region", "North")); + expected.extend(fixed("_Measure", "Revenue")); + expected.extend(["Product", "Customer"].map(|c| ViewStep::Axis { + category: c.into(), + axis: Axis::None, + })); + assert_eq!(drill_steps(&key, &none_cats), expected); + } } diff --git a/crates/improvise-io/Cargo.toml b/crates/improvise-io/Cargo.toml index 1fdfcaf1..1ec96762 100644 --- a/crates/improvise-io/Cargo.toml +++ b/crates/improvise-io/Cargo.toml @@ -17,7 +17,9 @@ indexmap = { version = "2", features = ["serde"] } pest = "2.9.2" pest_derive = "2.9.2" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } # The browser backend. Only the wasm build pulls these in; the native # binary never sees them (improvise-i34). diff --git a/crates/improvise-io/src/persistence/mod.rs b/crates/improvise-io/src/persistence/mod.rs index ab355b1e..505dfa2c 100644 --- a/crates/improvise-io/src/persistence/mod.rs +++ b/crates/improvise-io/src/persistence/mod.rs @@ -205,6 +205,28 @@ mod tests { assert_eq!(m2.model.name, "Budget"); } + #[test] + fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() { + use crate::storage::{MemoryFs, Storage}; + const LOSSY: f64 = 0.248_157_142_857_142_85; + let key = coord(&[("Type", "Food"), ("Month", "Jan")]); + let mut wb = two_cat_model(); + wb.model.set_cell(key.clone(), CellValue::Number(LOSSY)); + let json = serde_json::to_string(&wb).unwrap(); + assert!( + json.contains("0.24815714285714285"), + "the file holds the exact text" + ); + let fs = MemoryFs::new(); + let path = std::path::Path::new("legacy.improv"); + fs.write(path, json.as_bytes()).unwrap(); + let loaded = super::load_from(&fs, path).unwrap(); + let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else { + panic!("the cell must load as a number"); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}"); + } + // ── save/load roundtrip via file ──────────────────────────────────── #[test] diff --git a/crates/improvise-protocol/Cargo.toml b/crates/improvise-protocol/Cargo.toml index c114f10c..62c4a498 100644 --- a/crates/improvise-protocol/Cargo.toml +++ b/crates/improvise-protocol/Cargo.toml @@ -14,7 +14,8 @@ serde = { version = "1", features = ["derive"] } # `char`, and the TUI has always measured it this way. unicode-width = "^0.2" # Only for `variants`, which is off unless a test build asks for it. -serde_json = { version = "1", optional = true } +# float_roundtrip because every normal serde_json dep carries it (improvise-dz8c). +serde_json = { version = "1", optional = true, features = ["float_roundtrip"] } [features] # `variants` reaches OUT of this crate: improvise-command bounds its own diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 017bff2a..117f4fde 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -324,11 +324,12 @@ pub struct HelpPage { } pub use key_row::KeyRow; +use key_row::keys; /// The one door to a keys row: its fields are private to this module, and its /// only constructor looks the keys up. mod key_row { - use super::{ModeKey, key_label, keys_for, prefixed_labels}; + use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels}; #[derive(Debug, Clone, PartialEq, Eq)] pub struct KeyRow { @@ -347,7 +348,7 @@ mod key_row { /// An unbound command yields no keys and no row, so the page never /// advertises a key that does not exist. - pub(super) fn looked_up( + fn looked_up( mode: &ModeKey, command: &str, args: Option<&[String]>, @@ -361,17 +362,18 @@ mod key_row { (!keys.is_empty()).then_some(Self { keys, description }) } } -} -/// A keys row, looked up in `mode`. -fn keys( - mode: &ModeKey, - command: &'static str, - args: Option<&[&str]>, - description: &'static str, -) -> Option { - let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); - KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) + /// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`], + /// and the lookup `pages` hands every builder (improvise-ssoe). + pub(super) fn keys( + mode: &ModeKey, + command: &'static str, + args: Option<&[&str]>, + description: &'static str, + ) -> Option { + let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); + KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) + } } /// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap. @@ -1021,6 +1023,29 @@ mod tests { } } + /// improvise-ssoe: a keys row drawn but never asked for through the builders' + /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`. + #[test] + fn every_keys_row_drawn_is_one_the_builders_asked_for() { + let drawn: Vec = pages() + .into_iter() + .flat_map(|page| page.lines) + .filter(|line| matches!(line, HelpLine::Keys(_))) + .collect(); + // Content, in order, not a count: a bypass that also drops a real row + // keeps the count and changes this (improvise-aszt). + let asked: Vec = help_bindings() + .iter() + .filter_map(|(mode, command, args, description)| { + let args: Option> = args + .as_ref() + .map(|a| a.iter().map(String::as_str).collect()); + keys(mode, command, args.as_deref(), description) + }) + .collect(); + assert_eq!(drawn, asked); + } + /// A row whose lookup finds no key is dropped silently, so a broken lookup /// looks like a shorter page. See `context/test-evidence/protocol.md`. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index b44921aa..d88b4e23 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -619,6 +619,11 @@ enum GridMove { Page(i32), } +/// Rows one `page-scroll` step moves: three quarters of the window, at least one. +fn page_rows(window_rows: usize) -> i32 { + (window_rows as i32 * 3 / 4).max(1) +} + fn grid_move(state: &ClientState, kind: GridMove) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let (window_rows, window_cols) = state.window; @@ -641,7 +646,7 @@ fn grid_move(state: &ClientState, kind: GridMove) -> Option> { GridMove::ToEnd(true) => (row_max as usize, cursor.selected.1), GridMove::ToEnd(false) => (cursor.selected.0, col_max as usize), GridMove::Page(dir) => { - let delta = (window_rows as i32 * 3 / 4).max(1) * dir; + let delta = page_rows(window_rows) * dir; ((row + delta).clamp(0, row_max) as usize, cursor.selected.1) } }; @@ -2362,47 +2367,90 @@ mod tests { #[test] fn every_grid_move_lands_where_its_binding_says() { let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal"); - let mut moves = Vec::new(); - bound_commands(keymap, &mut moves); - // A move's destination is its own args; a jump's is its name. - let mut expected: Vec<(String, Vec, (usize, usize))> = moves - .into_iter() - .filter(|(name, _)| name == "move-selection") - .map(|(name, args)| { - let d = |i: usize| args[i].parse::().expect("a delta"); - let to = ((1 + d(0)) as usize, (1 + d(1)) as usize); - (name, args, to) - }) - .collect(); - for (name, to) in [ - ("jump-first-row", (0, 1)), - ("jump-last-row", (2, 1)), - ("jump-first-col", (1, 0)), - ("jump-last-col", (1, 2)), - ] { - expected.push((name.to_string(), Vec::new(), to)); - } - let mut pressed = 0; - for (name, args, to) in expected { - let wanted = (!args.is_empty()).then_some(args.as_slice()); - for sequence in sequences_running(keymap, &name, wanted) { + let mut bound = Vec::new(); + bound_commands(keymap, &mut bound); + bound.sort(); + bound.dedup(); + // Far from every edge of a grid far larger than any step, so a landing + // shows magnitude as well as sign (improvise-5kdr); a jump's is its name. + const START: (usize, usize) = (10, 10); + const WINDOW: (usize, usize) = (8, 8); + let d = |a: &String| a.parse::().expect("a delta"); + let at = |base: usize, by: isize| base.checked_add_signed(by).expect("inside the grid"); + let page = page_rows(WINDOW.0) as isize; + let destination = |name: &str, args: &[String]| match name { + "move-selection" => Some((at(START.0, d(&args[0])), at(START.1, d(&args[1])))), + "scroll-rows" => Some((at(START.0, d(&args[0])), START.1)), + "page-scroll" => Some((at(START.0, page * d(&args[0])), START.1)), + "jump-first-row" => Some((0, START.1)), + "jump-last-row" => Some((20, START.1)), + "jump-first-col" => Some((START.0, 0)), + "jump-last-col" => Some((START.0, 20)), + _ => None, + }; + let mut unstated = Vec::new(); + let mut unpressed = Vec::new(); + let mut pressed = std::collections::BTreeSet::new(); + for (name, args) in &bound { + let to = destination(name, args); + let sequences = sequences_running(keymap, name, Some(args.as_slice())); + if to.is_some() && sequences.is_empty() { + unpressed.push(format!("{name} {args:?}")); + } + for sequence in sequences { let mut state = furnished(AppMode::Normal); + state.grid.extent = GridExtent { rows: 21, cols: 21 }; + state.window = WINDOW; + *state.view.cursor_mut() = crate::ViewCursor { + selected: START, + row_offset: 5, + col_offset: 5, + }; + let view = state.view.active_view.clone(); for key in &sequence { if let KeyOutcome::Local(commands) = resolve(&state, *key) { commands.iter().for_each(|c| state.fold(c)); } } - assert_eq!( - state.view.cursor().selected, - to, - "{sequence:?} runs `{name} {args:?}` from (1,1)" - ); - pressed += 1; + let landed = state.view.cursor().selected; + if to.is_some() { + pressed.insert(name.as_str()); + } + match to { + Some(to) => { + assert_eq!( + landed, to, + "{sequence:?} runs `{name} {args:?}` from {START:?}" + ) + } + None if state.view.active_view == view && landed != START => { + unstated.push(format!("{name} {args:?} -> {landed:?}")) + } + None => {} + } } } assert!( - pressed >= 14, - "only {pressed} keys pressed; the walk went blind" + unpressed.is_empty(), + "stated and never pressed: {unpressed:?}" + ); + assert!( + unstated.is_empty(), + "these move the cursor and nothing states where to: {unstated:?}" + ); + let every = [ + "jump-first-col", + "jump-first-row", + "jump-last-col", + "jump-last-row", + "move-selection", + "page-scroll", + "scroll-rows", + ]; + assert_eq!( + pressed.into_iter().collect::>(), + every, + "each kind is pressed" ); } diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 83af13a8..f92e9deb 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -889,44 +889,6 @@ fn a_yanked_number_crosses_the_serialized_wire_exactly() { ); } -/// The two tests around this one share a feature-unified build, so neither can -/// see ONE frame-parsing crate lacking `float_roundtrip`. Evidence file. -#[test] -fn every_build_that_parses_a_frame_enables_float_roundtrip() { - for (krate, target) in [ - ("improvise-wasm-client", Some("wasm32-unknown-unknown")), - ("improvise-worker-server", Some("wasm32-unknown-unknown")), - ("improvise-ws-server", None), - ] { - let mut cargo = std::process::Command::new(env!("CARGO")); - cargo.current_dir(env!("CARGO_MANIFEST_DIR")).args([ - "tree", - "--locked", - "--edges", - "normal,features", - "--invert", - "serde_json", - "--package", - krate, - ]); - if let Some(target) = target { - cargo.args(["--target", target]); - } - let out = cargo.output().expect("cargo tree must run"); - assert!( - out.status.success(), - "{krate}: cargo tree failed: {}", - String::from_utf8_lossy(&out.stderr) - ); - let tree = String::from_utf8(out.stdout).expect("cargo tree prints UTF-8"); - assert!( - tree.contains(r#"serde_json feature "float_roundtrip""#), - "{krate} parses frames without float_roundtrip, so its numbers land \ - one ULP off:\n{tree}" - ); - } -} - /// A typed number is parsed exactly in the client and re-parsed from JSON by /// the host. Evidence file. #[test] diff --git a/tests/json_numbers.rs b/tests/json_numbers.rs new file mode 100644 index 00000000..506a59ec --- /dev/null +++ b/tests/json_numbers.rs @@ -0,0 +1,61 @@ +//! A number read through serde_json must be the number written (improvise-6yvg, +//! improvise-dz8c). The rule and its derivation: `context/code/protocol.md`. + +use serde_json::Value; + +/// Every NORMAL serde_json dependency in the workspace, and those lacking +/// `float_roundtrip`, each spelled `package (target)`. +fn lossy_json_dependencies(metadata: &Value) -> (Vec, Vec) { + let mut checked = vec![]; + let mut lossy = vec![]; + let packages = metadata["packages"] + .as_array() + .expect("metadata lists packages"); + for package in packages { + let name = package["name"].as_str().expect("a package has a name"); + let deps = package["dependencies"] + .as_array() + .expect("a package lists deps"); + for dep in deps { + if dep["name"] != "serde_json" || !dep["kind"].is_null() { + continue; + } + let target = dep["target"].as_str().unwrap_or("all targets"); + let spelled = format!("{name} ({target})"); + let features = dep["features"].as_array().expect("a dep lists features"); + if !features.iter().any(|f| f == "float_roundtrip") { + lossy.push(spelled.clone()); + } + checked.push(spelled); + } + } + (checked, lossy) +} + +/// Derived from `cargo metadata`, not listed: a new crate that can parse JSON +/// is covered the day it declares the dependency. Evidence file. +#[test] +fn every_crate_that_can_parse_json_enables_float_roundtrip() { + let out = std::process::Command::new(env!("CARGO")) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .args(["metadata", "--no-deps", "--format-version", "1"]) + .output() + .expect("cargo metadata must run"); + assert!( + out.status.success(), + "cargo metadata failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + let metadata: Value = serde_json::from_slice(&out.stdout).expect("metadata is JSON"); + let (checked, lossy) = lossy_json_dependencies(&metadata); + assert!( + checked.len() >= 2, + "the derivation found {checked:?}: too few to be reading the metadata right" + ); + assert!( + lossy.is_empty(), + "these parse JSON numbers with serde_json's lossy default, one ULP off \ + for roughly one decimal in seven; add features = [\"float_roundtrip\"] \ + to each crate's own manifest: {lossy:?}" + ); +}