4371374bbb
`SceneCache` decided whether its cached `CadScene` was stale by comparing `scene.node_count()` against `parts.len()`. That is a guess, and its own doc comment called it "a safety net for missing `mark_dirty()` calls" — the real contract was "call `mark_dirty()` after every mutation", enforced only by discipline across ~15 write sites and already violated by the drawing tools, which push nodes directly. The guess has a blind spot: delete one node and add another in the same frame and the length is unchanged, so the cache reports clean and hands back a scene describing the previous contents. Exports and picking then run on stale geometry. `CadViewport.parts` is now a `PartsStore` — the `Vec<CadNode>` plus a monotonic `generation` that every mutating method bumps. You cannot get `&mut` to the nodes without going through a method that bumps it, so `SceneCache::scene_for(&store)` can compare generations exactly. Both the length heuristic and the manual-`mark_dirty` requirement are gone as a class, not fixed case by case. `scene(&[CadNode])` is kept for callers that hold no store (tests, the export path's detached copies) and now always rebuilds: without a generation there is nothing to compare, and guessing is what caused the bug. Migration is incremental by design. `as_mut_vec()` is an escape hatch that bumps unconditionally; three call sites still use it rather than turning this into one 118-site commit. Sharing one document across the three viewports (5.2) and retiring the remaining `mark_dirty` calls are separate changes. Also converted 10 duplicated `vp.parts.iter_mut().find(..)` blocks in workspace.rs to the checked accessor. Tests: 741 -> 750 passing, 0 failing. 9 new, including the delete-plus-add case the old heuristic missed and an in-place edit with no `mark_dirty()`; both were verified to fail against the restored length check.
95 lines
6.7 KiB
Markdown
95 lines
6.7 KiB
Markdown
# `nigig-build` test baseline
|
|
|
|
```
|
|
cargo test --locked -p nigig-build --lib
|
|
test result: ok. 750 passed; 0 failed; 10 ignored
|
|
```
|
|
|
|
**The suite is green.** CI asserts a plain pass — any failing test fails the
|
|
build. Do not reintroduce a "no worse than N failures" threshold.
|
|
|
|
The 10 ignored are the CAD `profile_benchmarks`, deliberately `#[ignore]`-d
|
|
because they are timing measurements rather than assertions. Run them with
|
|
`cargo test -- --ignored --nocapture`.
|
|
|
|
---
|
|
|
|
## History
|
|
|
|
| Stage | Result |
|
|
|---|---|
|
|
| Before any work | Crate did not compile (44 errors); no test had ever run |
|
|
| After Phase 0 | 634 passed / 17 failed |
|
|
| After Phase 1 | 679 passed / 17 failed |
|
|
| After baseline cleanup | 702 passed / 0 failed |
|
|
| After Phase 2 (security) | 720 passed / 0 failed |
|
|
| After Phase 3 (performance) | 728 passed / 0 failed |
|
|
| After Phase 4 (deletion) | 741 passed / 0 failed |
|
|
| After Phase 5.1 (generation-tracked store) | **750 passed / 0 failed** |
|
|
|
|
Phase 4 removed 29 tests along with the dead `Legacy*` command system they
|
|
exercised. That is a *reduction in duplicated coverage*, not a regression:
|
|
every one of them tested code that no longer exists, and the equivalent
|
|
`Command`/`UndoRedoStack` paths keep their own tests. Verified by diffing
|
|
the full test-name list before and after.
|
|
|
|
The 17 failures were **not** regressions — they were pre-existing logic
|
|
defects that had been invisible because the test binary never built. Each
|
|
is described below, because the diagnoses are the useful part: roughly half
|
|
turned out to be wrong *tests* rather than wrong code, and telling those
|
|
apart required reading the production callers in every case.
|
|
|
|
---
|
|
|
|
## What the 17 were
|
|
|
|
### Genuine product bugs (10)
|
|
|
|
| Area | Defect |
|
|
|---|---|
|
|
| `cad::construction_geometry` | `next_polar_increment` used `position(..).unwrap_or(0)` then advanced, so an unrecognised increment returned 10 instead of restarting the cycle at 5. |
|
|
| `cad::construction_geometry` | `snap_to_polar_angle` returned its result unnormalised, so 370° snapped to 360° instead of 0°. Now wraps into `(-π, π]`, matching the `atan2` output the viewport feeds it. |
|
|
| `cad::construction_geometry` | `parse_coord_input` accepted `"3."` as 3.0. Rust parses it, but the user is mid-way through typing `3.5`, so the live preview committed a wrong-length segment on every keystroke. Trailing `.`/`e`/`+`/`-` are now treated as incomplete, and `"nan"`/`"inf"` are rejected. |
|
|
| `cad::viewport` | The viewport carried **inline copies** of both polar helpers, so neither fix above would have reached users. Both now call the shared functions. |
|
|
| `project_management::logic` | `would_create_cycle` walked the graph from the wrong end and tested the seed node before expanding, so it missed both direct (`1→2`, then link `2→1`) and indirect (`1→2→3`, then link `3→1`) cycles. Dependency cycles could be created through the UI. |
|
|
| `project_management::logic` | `get_over_allocated_assignees` keyed unassigned tasks on the empty string, so two overlapping *unassigned* tasks were reported as one over-booked person. |
|
|
| `project_management::persistence` | Serialization escaped `"` but not `\`, and deserialization used `trim_matches(.. '"')`, which stripped escaped quotes and left the escaping backslash behind. `Phase "Alpha"` round-tripped as `Phase "Alpha\`. Replaced with a proper escape/unescape pair covering `\ " \n \r \t`. |
|
|
| `cost_estimator::data_raw` | The default-rooms index used `HashMap::remove`, which consumes the entry. The same property type appears once per region, so the first region got its default rooms and the other two got none. |
|
|
| `cost_estimator::estimate_store` | Selections were normalised only in response to a user command, so a freshly constructed store kept `Estimate::default()`'s empty region — which matches nothing in the lookup index, leaving the property dropdown blank until the user touched a control. |
|
|
| `cost_estimator::screen_state` | `is_blocked` omitted `Initializing`, so the screen reported itself interactive while cost data was still loading. Now the exact complement of `is_ready`. |
|
|
| `doc-engine::crdt` | **Text atoms sorted ascending by id.** RGA requires siblings ordered *counter descending, actor ascending*: newest-nearest-the-anchor for correct sequential typing, actor tiebreak for deterministic convergence. Typing `!` after the `h` of `hi` produced `hi!` instead of `h!i`. Block ordering legitimately stays ascending and was left alone. |
|
|
|
|
Also fixed here: `CrdtDocument::to_json`, added in Phase 0, had never been
|
|
executed. `operations` is a `BTreeMap<OpId, _>` and JSON object keys must be
|
|
strings, so it failed at runtime with *"key must be a string"*. It now
|
|
serialises the log as an array of `[key, value]` pairs.
|
|
|
|
### Wrong tests (7)
|
|
|
|
Each was verified against the production caller before being changed.
|
|
|
|
| Test | Why it was wrong |
|
|
|---|---|
|
|
| `dde_buffer_incremental_parse` | Asserted `"5<4"` is "partial". It is a valid polar entry (5 units at 4°) and indistinguishable from `"5<0"`, which a neighbouring test asserts is **valid**. Rewritten; a separate test now covers genuinely incomplete input. |
|
|
| `logic::no_cycle` | Asserted the opposite of `cycle_direct` on a structurally identical graph — relabel the ids and they are the same call with contradictory expectations. Duplicate-link suppression is the caller's job (`dependencies.contains`), not this function's. |
|
|
| `history::undo_redo_roundtrip`, `history::multiple_undo_redo_cycle` | Modelled snapshots taken *after* each mutation; every production caller snapshots *before* (`self.snapshot(); self.tasks.push(..)`). Under the real protocol the old sequences pushed the current state, so undo popped the state you were already in. |
|
|
| `solar::calculate_lead_acid_battery` | Asserted `3.125` while the engine rounds every output to 2 dp. The test's own comment computed the unrounded intermediate. |
|
|
| `services::export_json_empty_rooms` | Wanted compact `"rooms": []`; the writer emitted a valid but awkward multi-line empty array. Fixed the writer — the test's preference was reasonable. |
|
|
| `types::*_inequal_to_*` (2, fixed in Phase 0) | Asserted `assert_ne!` between distinct newtypes — a comparison the type system now rejects at compile time, which is strictly stronger. |
|
|
|
|
---
|
|
|
|
## Reproducing
|
|
|
|
```bash
|
|
sudo apt-get install -y pkg-config libwayland-dev libxcursor-dev \
|
|
libxrandr-dev libxi-dev libx11-dev libgl1-mesa-dev libasound2-dev \
|
|
libpolkit-gobject-1-dev libpolkit-agent-1-dev libglib2.0-dev \
|
|
libssl-dev libsqlite3-dev libudev-dev libpulse-dev libxkbcommon-dev
|
|
|
|
cargo test --locked -p nigig-build --lib
|
|
cargo test --locked -p doc-engine
|
|
```
|
|
|
|
`libpulse-dev` and `libxkbcommon-dev` are link-time-only requirements —
|
|
`cargo check` passes without them, `cargo test` does not.
|