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.