lanspread/REVIEW_STEP_3.md

# Review Step 3 — Test Quality

## Headline

38 tests pass across the workspace (`just test`). The new tests cover most of the load-bearing primitives. **They are real tests — they exercise actual filesystem and atomic-rename behavior, not mocked surfaces — and the assertions are tight.** Coverage of the install state machine, the version-sentinel commit path, and aggregation gating is strong.

The weakness is **coverage breadth**: PLAN.md's test plan listed roughly 25 specific scenarios, of which 13 are actually verified. The plan's "recovery: each row of the install matrix" item is only one row deep. Scanner gating and serve gating are only covered by unit tests of helper functions; the dispatch paths themselves are untested.

## Detailed test-by-test assessment

### Genuinely useful, well-designed

These tests exercise real behavior with clear assertions and would catch regressions reliably.

| Test | File | What makes it good |
|---|---|---|
| `install_success_promotes_staging_and_clears_intent` | `install/transaction.rs:537` | Runs a full install with a fake unpacker, asserts the commit-rename happened, intent flipped back to None, and no staging dir leaked. |
| `update_failure_restores_previous_local` | `install/transaction.rs:554` | Uses a deliberately failing unpacker, then asserts the old `local/old.txt` is still readable and both `.local.installing` and `.local.backup` are gone. Tests the full rollback path including the backup→local restore. |
| `uninstall_removes_only_local_install` | `install/transaction.rs:574` | Confirms uninstall removes `local/` but preserves `game.eti` and `version.ini`. Directly maps to PLAN.md's Assumption: "Uninstall removes only `local/`; archives and `version.ini` stay." |
| `recovery_restores_backup_for_interrupted_update` | `install/transaction.rs:591` | Stages a realistic crash scenario (intent=Updating, both `.local.installing` and `.local.backup` present with markers), runs recovery, asserts backup is restored to `local/`. Good integration of intent + FS state. |
| `none_recovery_leaves_markerless_reserved_dirs_untouched` | `install/transaction.rs:624` | The sideload / user-data safety test. Critical for the "never delete user files" invariant. |
| `download_recovery_sweeps_reserved_version_files` | `install/transaction.rs:637` | Both `.version.ini.tmp` and `.version.ini.discarded` get cleaned up. |
| `tmp_write_without_rename_leaves_previous_intent_intact` | `install/intent.rs:154` | Simulates a crash between tmp write and rename by writing the tmp file directly and never renaming. Asserts the old intent survives. Real test of atomic write semantics. |
| `schema_mismatch_is_treated_as_missing` | `install/intent.rs:183` | Writes schema_version=2 to disk, reads back, asserts state=None. |
| `scan_uses_version_ini_and_local_dir_as_independent_state` | `local_games.rs:650` | Parameterizes all three meaningful states (ready, local-only, eti-only) plus a non-catalog game in one test. Asserts Availability and the independence of `downloaded`/`installed`. Good shape. |
| `local_download_available_gates_on_catalog_operation_and_sentinel` | `local_games.rs:703` | Exercises every input dimension: catalog hit/miss, operation active/inactive, sentinel present/missing. Tight. |
| `aggregation_counts_only_ready_peers_as_download_sources` | `peer_db.rs:776` | Constructs a Ready peer and a LocalOnly peer, asserts `peer_count`, `eti_version`, `peers_with_game`, `get_latest_version_for_game`, and `peers_with_latest_version` all filter on Ready. Single test, four assertions, broad coverage. |
| `local_only_peer_does_not_make_game_downloadable` | `peer_db.rs:806` | The aggregate-of-one case: only a LocalOnly peer exists; nothing downloadable. |
| `version_ini_predicate_matches_only_game_root_sentinel` | `lanspread-db/src/db.rs:240` | Three cases for `is_version_ini`: root match, nested `local/version.ini` rejected, other game's path rejected. Maps directly to PLAN's `is_version_ini` tightening. |
| `commit_version_ini_writes_sentinel_last_and_sweeps_discarded` | `download.rs:1079` | Pre-seeds `.version.ini.discarded`, commits the buffer, asserts the sentinel content is correct, tmp is gone, discarded is gone. End-to-end on the download transaction commit. |
| `version_ini_buffer_accepts_out_of_order_chunks` | `download.rs:1057` | Writes to offset 4 first, then 0, asserts the buffer reconstructs `20250101`. Exact match to a PLAN test item. |
| `prepare_game_storage_skips_version_ini_sentinel` | `download.rs:1040` | Direct test that prep storage doesn't create the sentinel file. |
| `partition_requires_exactly_one_root_version_ini` | `download.rs:1114` | Tests three cases: single sentinel + nested decoy (passes), zero sentinels (errors), duplicate root sentinels (errors). |
| `local_relative_paths_are_never_transferable` | `services/stream.rs:394` | Pure unit test of the path predicate. Covers `game/local/...`, `local/...`, Windows backslashes, and the negative cases (`game/version.ini`, `game/archive.eti`). |
| `event_paths_map_to_top_level_game_id` | `services/local_monitor.rs:336` | Tests that watcher event paths map to game IDs correctly and that `.lanspread` and `local/` children are excluded. |
| `event_ignore_list_covers_reserved_names` | `services/local_monitor.rs:354` | Enumerates every reserved name and asserts each is ignored; asserts non-reserved names are not. |
| `operation_guard_clears_tracking_*` (3 tests) | `context.rs:240,254,269` | Tests the OperationGuard Drop impl across success, cancellation, and abort. The abort test (`*_when_task_is_dropped`) is particularly good — it verifies the runtime-handle fallback path inside Drop. |
| `required_service_failure_cancels_runtime_and_emits_event` | `startup.rs:360` | Real test of supervised-service semantics. |
| `restart_service_restarts_until_shutdown` | `startup.rs:395` | Verifies the restart policy actually re-invokes the factory. |

### Marginal but reasonable

| Test | Note |
|---|---|
| `build_peer_plans_handles_partial_final_chunk` (download.rs:954) | Older test, still valid. |
| `build_peer_plans_respects_file_peer_map` (download.rs:985) | Older test, still valid. |
| `runtime_handle_can_shutdown_and_await_stopped` (startup.rs:440) | Slightly tautological (builds the handle by hand, signals stopped manually). Asserts the watch-channel plumbing. Not deeply useful but cheap. |

### What's missing relative to PLAN.md's test plan

PLAN.md was explicit about test scenarios. Here's the delta. ✗ = no test; ◐ = partially tested via helper but not at the dispatch level; ✓ = covered.

**Install transactions** (PLAN.md §"Unit — install transactions"):
- ✓ Install success
- ✗ Update success — there is no test for a *successful* update path. Only `update_failure_restores_previous_local`.
- ✓ Update rollback on extract failure
- ✗ Update rollback on commit-rename failure (e.g., destination on a read-only mount, or staging fails to rename). Not exercised.
- ✓ Uninstall success
- ✗ Uninstall delete-failure restore. The `restore_backup` rollback path in `uninstall` (`transaction.rs:107-114`) is not directly tested.

**Download transaction** (PLAN.md §"Unit — download transaction"):
- ✓ Fresh download writes sentinel last via atomic rename (via `commit_version_ini_writes_sentinel_last_and_sweeps_discarded`).
- ◐ Re-download renames old version.ini to discarded and sweeps it — the commit/sweep is tested, but the *initial rename* in `begin_version_ini_transaction` (`download.rs:193-206`) has no unit test.
- ✗ Interrupted download leaves no version.ini — the `rollback_version_ini_transaction` function (`download.rs:208-221`) is reachable from many code paths in `download_game_files` and is never directly tested.
- ✓ `prepare_game_storage` skips sentinel.
- ✓ Out-of-order chunk arrivals (`version_ini_buffer_accepts_out_of_order_chunks`).
- ✗ Multi-`.eti` games. The `partition_requires_exactly_one_root_version_ini` test only covers the version.ini partition rules, not the multi-archive download path.

**Recovery** (PLAN.md §"Unit — recovery"):
- ◐ Each row of the install matrix. The matrix has roughly 11 rows. Tests cover *one* (`Updating | no | yes | yes`) plus the None default and the download-transient sweep. The other 9 rows (Installing with three FS combinations; Updating with three more FS combinations; Uninstalling with three FS combinations) are not exercised. This is the largest coverage hole.
- ✓ Download recovery sweep.
- ◐ JSON missing / corrupted / wrong schema_version. Only `schema_mismatch_is_treated_as_missing` exists; no test for parse-error/corrupt-JSON or for a mismatched `id` field (handled in `intent.rs:69` but untested).
- ✓ Markerless `.local.*` dir ignored.

**Atomic intent write** (PLAN.md §):
- ✓ Both cases listed.

**Scanner gating** (PLAN.md §"Unit — scanner gating"):
- ✗ Events for an ID under operation lock are dropped. The `handle_watch_event` function (`local_monitor.rs:208-236`) is not tested — only the helper `game_id_from_event_path` and `should_ignore_game_child`.
- ✗ Rescan-pending flag collapses bursts to ≤2 scans per ID. `RescanGate` / `run_gated_rescan` (`local_monitor.rs:261-287`) has no test.
- ✗ Sideload picked up by fallback scan.
- ◐ Non-catalog game produces no GameSummary — only the negative assertion `!scan.summaries.contains_key("non-catalog")` exists; no test verifies that the *library index* also has no entry, and no test verifies that no `LibraryDelta` is emitted.

**Serve gating** (PLAN.md §"Unit — serve gating"):
- ◐ `GetGame` refused mid-operation — `local_download_available_gates_on_catalog_operation_and_sentinel` tests the predicate, but `handle_get_game_command` / `handle_get_game` dispatch is untested.
- ◐ `GetGameFileData` / `GetGameFileChunk` refused mid-operation. Helpers untested; only `path_points_inside_local` is.
- ◐ All three handlers refuse non-catalog IDs. Same — predicate tested, dispatch untested.
- ◐ All three handlers refuse when sentinel is absent. Same.
- ✓ File-chunk handler rejects `local/`-relative paths (via the path-predicate unit test).

**Aggregation** (PLAN.md §):
- ✓ All three rules covered in two tests.

**Installed-only state** (PLAN.md §):
- ✓ Initial state covered in `scan_uses_version_ini_and_local_dir_as_independent_state`.
- ✗ Transition: user copies in `version.ini`, next rescan flips to Ready. Not tested.

### Tests that could be improved

#### `recovery_restores_backup_for_interrupted_update` (transaction.rs:591)

Right shape, but covers only one row of the matrix. To make this more useful, parameterize: iterate over `(intent, fs_state)` tuples, run recovery, assert resulting `local/` content and intent. A table-driven approach would convert this single test into 11.

#### `partition_requires_exactly_one_root_version_ini` (download.rs:1114)

The "duplicate" case in the test actually has `game/version.ini` and `game/local/version.ini`. Under the *new* tight `is_version_ini` predicate, only the first matches, so this is *not* actually a duplicate-sentinel scenario — it tests "nested decoy is ignored." That's fine, but the test name promises something else. The "multiple" case (two `game/version.ini` entries) is a true duplicate and asserts error. Worth renaming the test or splitting into two clearer ones.

#### The `TempDir` pattern is duplicated 5 times

`install/intent.rs:128`, `install/transaction.rs:498`, `local_games.rs:615`, `download.rs:926`, plus subtle variants. Each is a tiny manual reimplementation of the same primitive. Consolidating into a single `test_support` module (or using `tempfile`, which is already a stable crate) would shrink each test file by ~25 lines. Not a correctness issue.

#### No negative test for `is_version_ini` Windows-path case

The predicate normalizes `\\` to `/` (`db.rs:182-183`). Worth a single Windows-style path test to lock the contract.

### Tests that don't really test the runtime contract

None — there are no obviously fake or tautological tests. Even `runtime_handle_can_shutdown_and_await_stopped` (which I noted as "slightly tautological" above) is genuinely testing the watch-channel signal path, which is harness-grade infrastructure worth pinning.

## Test plumbing assessment

- All install/transaction tests use a `FakeUnpacker` (`transaction.rs:467-496`) that writes a deterministic `payload.txt` on success. This is the right abstraction — it isolates the FS transaction logic from the actual unrar sidecar.
- The "failing unpacker" path uses a `fail: bool` field; clean and obvious.
- Temp directory names include `process::id()` and nanosecond timestamps, so parallel test runs won't collide. Good.

## Summary

The tests that exist are good tests — real, deterministic, decent assertions. **No fake/mocked-database tests; no tests that just exercise the test scaffolding.** The author clearly understood that filesystem state has to be set up explicitly to exercise the transactions.

The gap is breadth. PLAN.md was specific about scenarios; roughly half of them are not yet covered. The largest single gap is the install-recovery matrix (11 rows, 1 row tested). Second largest is "scanner gating" — `RescanGate` and `handle_watch_event` have zero tests. Third is "serve gating" at the dispatch level — the predicate `local_download_available` is well-tested but the `handle_*` dispatch path isn't.

Recommendation: a follow-up PR that parameterizes the recovery test over the full matrix, adds a few `handle_*`-level tests against a small in-memory `Ctx`, and adds the "successful update" and "uninstall delete-failure restore" cases would bring coverage in line with what PLAN.md asked for. None of this is blocking — what's there is solid.