nigig-org/TEST_BASELINE.md

# `nigig-build` test baseline

```
cargo test --locked -p nigig-build --lib
test result: ok. 1133 passed; 0 failed; 21 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 21 ignored are the CAD `profile_benchmarks` and release-only
merge tests, 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 |
| After the CAD engine coverage work | **1044 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. |

---

## CAD engine line coverage

The pass/fail count above says the suite is green. It does not say what
the suite touches, and until this work nothing measured that.

A correction worth recording, because it cost six sessions. The reason
given for not measuring — including by me, repeatedly, in commit
messages and in this file — was that `cargo test -p nigig-build` needs
wayland, X11, GL, alsa and polkit and therefore "does not build in this
environment". That was never tested. It builds fine: in a plain
container, `bash tools/makepad-native-libs.sh --install` followed by
`cargo check --locked -p nigig-build --lib` takes 2m52s, and
`cargo test --locked -p nigig-build --lib` takes about five minutes cold
and under a second warm for 1133 tests.
`--test cad_integration` adds 154 more. `cargo fmt -p nigig-build
--check`, the lockfile gate and the mistyped-enum-variant gate all pass.

The install script existed the whole time, written for exactly this,
with a comment explaining that libasound and libpulse are needed to
*link* a test binary rather than merely to check it. Both this file's
"Reproducing" section and the review's "Required next commands" pointed
at it. An assumption made once and never retested is indistinguishable
from a fact, which is the same failure this file criticises the CAD
review for.

`tools/test-cad-coverage.sh` measures it, and is still the right tool for
the job: it needs no apt, no root and no desktop packages, it runs in
about 20 seconds warm against five minutes for the full crate, and it is
what the CI gate uses. The full-crate build is the thing to reach for
when a change touches the widget layer, which the harness cannot see. Nineteen of the CAD module's
thirty files are pure -- geometry, the scene graph, undo/redo, the
four exporters, file I/O -- and their only Makepad imports are the math
types, the CSG library and two log macros, all dependency-free Rust. The
script copies those into a host-only crate with the same module path,
runs them plus `tests/cad_integration.rs` under `-C instrument-coverage`,
and enforces a total floor and a per-file floor. The `cad-engine-coverage`
job in `.forgejo/workflows/nigig-build.yml` runs it on every push.

| File | Lines | Floor | Note |
|---|---|---|---|
| `batching.rs` | 100.00% | 99 | new — Phase 3 instanced batching |
| `cull.rs` | 100.00% | 99 | Phase 1 viewport culling |
| `nav_pad.rs` | 100.00% | 99 | |
| `render_budget.rs` | 99.75% | 99 | Phase 4 batching + Phase 5 2-D/3-D LOD budget |
| `lod.rs` | 99.68% | 99 | new — Phase 5 2-D/3-D sub-pixel policy |
| `scene_holder.rs` | 100.00% | 99 | |
| `section_shape.rs` | 100.00% | 99 | |
| `send_sync_audit.rs` | 100.00% | 99 | |
| `math.rs` | 99.60% | 97 | was 20.40%, no tests at all |
| `arch_svg.rs` | 99.02% | 97 | |
| `arch_stl.rs` | 98.88% | 98 | |
| `arch_gltf.rs` | 98.65% | 97 | |
| `cad_scene.rs` | 98.56% | 96 | |
| `constants.rs` | 98.51% | 95 | |
| `tools.rs` | 97.07% | 95 | was 0.00% — see below |
| `commands.rs` | 96.23% | 94 | |
| `construction_geometry.rs` | 95.88% | 95 | rest is `panic!` arms inside tests |
| `persistence.rs` | 94.51% | 90 | rest is the two real-data-dir wrappers |
| `exporters.rs` | 91.17% | 88 | rest is the save-dialog branch |
| `arch_pdf.rs` | 88.99% | 86 | rest is the printpdf emitter |
| **Total** | **97.39%** | **96** | 653 engine-coverage tests (499 unit + 154 integration); 1,133 full-crate unit tests |

Measured against the state of the tree when this table was written; the
floors, not the table, are what CI enforces.

### The other half: the widget layer, measured at last

The engine table above is 97% of the *smaller* half. Here is the rest,
from `./tools/test-cad-widget-coverage.sh` — the real crate, real tests,
`-C instrument-coverage`, no desktop required:

| File | Lines | Covered |
|---|---|---|
| `mod.rs` | 42 | 80.95% |
| `script_bindings.rs` | 724 | 71.27% |
| `viewport.rs` | 3,548 | 14.37% |
| `workspace.rs` | 2,462 | 14.18% |
| `viewport_input.rs` | 672 | **0.00%** |
| `viewport_render.rs` | 2,083 | **0.00%** |
| `workspace_actions.rs` | 690 | **0.00%** |
| `cad_editor_sheet.rs` | 224 | **0.00%** |
| `viewport_2d.rs` | 138 | **0.00%** |
| `code_editor.rs` | 67 | **0.00%** |
| **Total** | **10,637** | **13.25%** |

9,228 lines have never been executed by a test. Six files have never had
a single line run. Among them is `viewport_input.rs` — every click,
drag, modifier and keystroke the CAD editor handles — and
`viewport_render.rs`, every draw call.

Worth sitting with, because it reframes the engine work above: 1133
green tests and a 97% engine coexist with an input layer that no test
has ever touched. The two facts were never in tension; they were just
never on the same page.

No floors here yet, deliberately. A floor at 13% reads as a blessing.
The first person to write a real input test should set one behind them.

### What moving the number actually costs

The first attempt is worth recording, because it calibrates the rest.
`nav_pad.rs` extracted the navigation pad's hit-testing out of
`viewport_input.rs` into a pure module: 64 lines moved, now at 100% with
7 tests, and a one-pixel misalignment fixed on the way. The widget total
moved 13.15% -> 13.25%.

`viewport_input.rs` is still at 0.00%, and will stay there under this
strategy — extraction makes logic testable by moving it *out*, so the
handler shrinks rather than getting covered. At roughly 60 lines per
extraction, the remaining 672 lines of input handling are ten more of
these. That is a reasonable way to spend the effort, and the pixel bug
suggests the yield is real, but it will not produce a big number
quickly. Genuinely covering the handlers themselves means driving them
with an event loop — `makepad-test`, which `tests/cad_ui.rs` and
spreadsheet-ui already use — and that is a different piece of work.

### Which is the right tool

| | `test-cad-coverage.sh` | `test-cad-widget-coverage.sh` |
|---|---|---|
| Measures | the 15 pure engine files | the 10 widget files |
| Needs | nothing but network | the Makepad Linux packages, so root |
| Cold / warm | ~3 min / ~20 s | ~6 min / ~4 s |
| Gated in CI | yes, with per-file floors | not yet |

```bash
./tools/test-cad-coverage.sh                  # measure and enforce
KEEP_COVERAGE=1 ./tools/test-cad-coverage.sh  # keep the uncovered-line list

# local loop: seed the caches once, then each run takes ~20s
export CAD_COV_TOOLCHAIN_HOME=~/.cache/cad-cov \
       CAD_COV_TARGET_DIR=~/.cache/cad-cov/target
```

---

## 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.