67 lines
5.9 KiB
Markdown
67 lines
5.9 KiB
Markdown
# Code review (re-review) — NEO-48 testing refactor + mastery fixture API
|
||
|
||
**Date:** 2026-05-17
|
||
**Scope:** Branch `NEO-48-perk-state-get-branch-selection-post-bruno` · commits `3ba8300`–`ffdbed2` (delta since [first review](2026-05-17-NEO-48.md)) · full branch vs `main` for merge readiness
|
||
**Base:** `main`
|
||
**Prior review:** [`2026-05-17-NEO-48.md`](2026-05-17-NEO-48.md) (perk-state HTTP + initial Bruno/tests)
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
Follow-up work closes the first-review Bruno/test gaps and replaces a short-lived **fixture-player seeding** experiment with a cleaner **dev-only mastery fixture API**. **`POST /game/players/{id}/__dev/mastery-fixture`** (gated to Development, Testing, or `Game:EnableMasteryFixtureApi`) resets perk state and sets absolute skill XP without going through grant/level-up hooks—giving deterministic Bruno deny smokes on shared `dev-local-1`. **`PerkStateApiTests`** adds null-body **400** and HTTP deny smoke for `unknown_track` / `unknown_tier` / `unknown_branch`; **`MasteryFixtureApiTests`** verifies Production+flag-off returns **404** and that reset + XP zero yields **`level_too_low`** on the next branch POST. Bruno deny requests use `script:pre-request` + `axios` against the fixture endpoint; `{{playerId}}` from `Local.bru` is used consistently on deny flows. **191** server tests pass. Merge risk remains low; the new surface is explicitly non-production and tested for disable behavior.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-48-implementation-plan.md`](../plans/NEO-48-implementation-plan.md) | **Partially matches** — perk-state HTTP/Bruno acceptance unchanged; kickoff table documents **`__dev/mastery-fixture`** for Bruno deny setup. **Files to add/modify** tables do not list `MasteryFixture*.cs` or store `TryReset*` / `TrySetSkillXpTotal` (implementation detail worth a one-line plan add). |
|
||
| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Matches** — NEO-48 handoff covers HTTP only; dev fixture correctly omitted from module contracts. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — production perk rules still flow through `PerkUnlockEngine`; fixture bypasses grant path by design (dev/QA only). |
|
||
| [`docs/manual-qa/NEO-48.md`](../manual-qa/NEO-48.md) | **Matches** — fixture curl examples + Bruno deny checklist updated. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — Perk state section documents fixture API, gating, and **404** when disabled. |
|
||
| Prior [`2026-05-17-NEO-48.md`](2026-05-17-NEO-48.md) | **Superseded for merge** by this re-review for the testing-refactor delta; first-review suggestions 1–3 marked done there remain valid. |
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. **Atomic fixture apply** — `MasteryFixtureOperations.TryApply` can leave mixed state if `resetPerkState` succeeds and a later `skillXp` entry fails (or an earlier XP write succeeds before a later one fails). For a dev endpoint this is unlikely with Bruno’s fixed payloads, but consider applying XP first then reset, a single transaction on Postgres, or returning **409**/**500** with a note when partial apply is detected.
|
||
2. **Fixture API test coverage** — Add small tests for **400** (null body / bad `schemaVersion`) and **404** (unknown player, negative XP in `skillXp`) on `__dev/mastery-fixture` to mirror perk-state gate tests.
|
||
3. **Document fixture vs grant semantics** — README could note that **`skillXp` on the fixture sets absolute totals without running `SkillProgressionGrantOperations` / perk level-up reevaluation** (intentional for deny setup; use normal grant API when testing unlock side effects).
|
||
|
||
## Nits
|
||
|
||
- Nit: `Get perk state.bru` still asserts `playerId === "dev-local-1"` while deny requests use `{{playerId}}` from the environment—harmless if `Local.bru` keeps the default; align assertions for copy-paste environments.
|
||
- Nit: `TryResetPerkState` / `TrySetSkillXpTotal` live on production store interfaces—acceptable for prototype; if the surface grows, consider a dev-only `IMasteryFixtureStore` facade later.
|
||
- Nit: Bruno deny flows mutate shared `dev-local-1`; folder `docs` explain fixture pre-requests—run deny requests before happy-path smokes or accept order sensitivity in manual runs.
|
||
- Nit: `MasteryFixtureApiTests.PostMasteryFixture_ShouldResetSalvageXpAndPerkState_ThenLevelTooLowDenies` performs reset and deny POST both under **Act**; valid for an integration scenario, but heavy **Arrange** (grant + pick) could move to a private helper like `PerkStateApiTests.GrantSalvageLevel2Async`.
|
||
|
||
## Delta since first review (testing refactor)
|
||
|
||
| Area | Change |
|
||
|------|--------|
|
||
| **Bruno** | `Post branch select deny branch already chosen.bru`; `level_too_low` uses fixture pre-request (no skip-on-already-leveled); `{{playerId}}` + `baseUrl` resolution fixed. |
|
||
| **Server** | `MasteryFixtureApi` / `MasteryFixtureOperations` / DTOs; `Program.cs` conditional map; `EnableMasteryFixtureApi` in appsettings; store `TryResetPerkState` + `TrySetSkillXpTotal` (in-memory + Postgres). |
|
||
| **Tests** | `MasteryFixtureApiTests` (2); `PerkStateApiTests` +4 deny/null-body; `InMemoryWebApplicationFactory` enables fixture flag for Testing. |
|
||
| **Reverted approach** | `GamePlayerSeed` / extra fixture player ids (commit `b72201b`) removed in favor of fixture API (`ffdbed2`)—simpler ops story. |
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# From repo root
|
||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkState|MasteryFixture"
|
||
|
||
# Full server suite (191 tests at re-review)
|
||
dotnet test NeonSprawl.sln
|
||
|
||
# Bruno (Development server, EnableMasteryFixtureApi on)
|
||
# bruno/neon-sprawl-server/perk-state/
|
||
```
|
||
|
||
Confirm deny Bruno requests pass on a fresh dev server; confirm `POST …/__dev/mastery-fixture` returns **404** when `ASPNETCORE_ENVIRONMENT=Production` and `Game:EnableMasteryFixtureApi=false`.
|