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.
6.7 KiB
6.7 KiB
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
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.