NEO-101: Address code review — schema tests, module snapshot, host boot.
Adds row-level JsonSchema violation tests for both loaders, symmetric empty reward-tables host boot test, and E5.M3 implementation snapshot; review doc marked Done.pull/140/head
parent
5e26143fbd
commit
21749e7977
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E5.M3 |
|
||||
| **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) |
|
||||
| **Stage target** | Prototype |
|
||||
| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) decomposed |
|
||||
| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) catalog + CI landed · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) server load landed · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) |
|
||||
| **Linear** | Label **`E5.M3`** · [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md) **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) · **E5M3-06** [NEO-105](https://linear.app/neon-sprawl/issue/NEO-105) · **E5M3-07** [NEO-106](https://linear.app/neon-sprawl/issue/NEO-106) · **E5M3-08** [NEO-108](https://linear.app/neon-sprawl/issue/NEO-108) · **E5M3-09** [NEO-107](https://linear.app/neon-sprawl/issue/NEO-107) · **E5M3-10** [NEO-109](https://linear.app/neon-sprawl/issue/NEO-109) · **E5M3-11** [NEO-110](https://linear.app/neon-sprawl/issue/NEO-110) · **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) |
|
||||
|
||||
## Purpose
|
||||
|
|
@ -78,6 +78,10 @@ The **first shipped encounter + reward spine** is **frozen** for prototype tunin
|
|||
|
||||
**Payout policy:** Per-defeat **gig XP** stays on [NEO-44](../../plans/NEO-44-implementation-plan.md). Encounter complete grants **loot + quest token** **once** per player per encounter id. Full **E7.M2** quest credit routing consumes **`EncounterCompleteEvent`** (E5M3-09 hook stub).
|
||||
|
||||
## Implementation snapshot (NEO-101)
|
||||
|
||||
**Server catalog load landed ([NEO-101](https://linear.app/neon-sprawl/issue/NEO-101)):** fail-fast startup loaders under [`server/NeonSprawl.Server/Game/Encounters/`](../../../server/NeonSprawl.Server/Game/Encounters/) — `RewardTableDefinitionCatalogLoader` (after item catalog) then `EncounterDefinitionCatalogLoader` (after reward tables); CI-parity E5M3 gates in C#; eager DI resolve in `Program.cs`. Config + discovery: [server README — Reward table catalog](../../../server/README.md#reward-table-catalog-contentreward-tables-neo-101), [Encounter catalog](../../../server/README.md#encounter-catalog-contentencounters-neo-101). Plan: [NEO-101 implementation plan](../../plans/NEO-101-implementation-plan.md). Injectable registries deferred to **NEO-102**.
|
||||
|
||||
## Source anchors
|
||||
|
||||
- Master plan: [`neon_sprawl_vision.plan.md`](../../../neon_sprawl_vision.plan.md) — Epic 5.
|
||||
|
|
|
|||
|
|
@ -120,7 +120,7 @@ Working backlog for **Epic 5 — Slice 3** ([Epic 5 · Slice 3 — encounters an
|
|||
- [x] Host fails startup on invalid encounter or reward-table JSON (mirror CI rules).
|
||||
- [x] Tests cover at least one happy path and one malformed catalog rejection.
|
||||
|
||||
**Landed ([NEO-101](https://linear.app/neon-sprawl/issue/NEO-101)):** fail-fast encounter + reward-table catalog loaders, DI registration, 19 AAA tests; plan [NEO-101-implementation-plan.md](NEO-101-implementation-plan.md).
|
||||
**Landed ([NEO-101](https://linear.app/neon-sprawl/issue/NEO-101)):** fail-fast encounter + reward-table catalog loaders, DI registration, 22 AAA tests; plan [NEO-101-implementation-plan.md](NEO-101-implementation-plan.md).
|
||||
|
||||
### E5M3-03 — Injectable encounter/reward registries + DI
|
||||
|
||||
|
|
|
|||
|
|
@ -51,7 +51,7 @@
|
|||
- **Catalogs:** `RewardTableDefinitionCatalog`, `EncounterDefinitionCatalog` singletons; eager resolve in `Program.cs`.
|
||||
- **DI:** `AddEncounterAndRewardCatalogs` — reward tables load after item catalog; encounters load after reward tables.
|
||||
- **Config:** `ContentPathsOptions` encounter + reward-table path keys; `InMemoryWebApplicationFactory` pins repo paths.
|
||||
- **Tests:** 19 AAA tests in `RewardTableDefinitionCatalogLoaderTests` + `EncounterDefinitionCatalogLoaderTests` (loader + host boot).
|
||||
- **Tests:** 22 AAA tests in `RewardTableDefinitionCatalogLoaderTests` + `EncounterDefinitionCatalogLoaderTests` (loader + host boot, incl. row schema violation).
|
||||
- **Docs:** `server/README.md` catalog sections; alignment register E5.M3 row updated.
|
||||
|
||||
## Technical approach
|
||||
|
|
|
|||
|
|
@ -18,7 +18,7 @@ NEO-101 adds fail-fast startup loading for **`content/reward-tables/*_reward_tab
|
|||
|------|--------|
|
||||
| `docs/plans/NEO-101-implementation-plan.md` | **Matches** — shipped loaders, DI order, config keys, tests, README, alignment register; acceptance checklist checked; reconciliation section accurate. |
|
||||
| `docs/plans/E5M3-prototype-backlog.md` (E5M3-02) | **Matches** — landed note cites NEO-101 plan and 19 tests. |
|
||||
| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — freeze table and CI paragraph reference NEO-101 loader sync; Summary **Status** still generic “In Progress — Slice 3 backlog” without an explicit **NEO-101 server load landed** bullet (alignment register was updated; module page could mirror). |
|
||||
| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — freeze table and CI paragraph reference NEO-101 loader sync; Summary **Status** notes **NEO-101 server load landed**; **Implementation snapshot (NEO-101)** paragraph added. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-101** server load; runtime NEO-102+ still outstanding. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-101 landed** + README anchor. |
|
||||
| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **N/A** — no server-load paragraph required for this story; CI gates unchanged. |
|
||||
|
|
@ -31,20 +31,20 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Add `Load_ShouldThrow_WhenRowViolatesSchema` tests** — The implementation plan test table lists **schema violation** for both loaders; peer catalog tests (`ItemDefinitionCatalogLoaderTests`, `RecipeDefinitionCatalogLoaderTests`, etc.) use empty `displayName` or similar. Current negative coverage uses wrong top-level shape / `schemaVersion` / cross-refs but not a row-level JsonSchema failure. Add one case per loader (e.g. empty `displayName` on a otherwise-valid prototype row) to lock schema messaging and match precedent.
|
||||
1. ~~**Add `Load_ShouldThrow_WhenRowViolatesSchema` tests** — The implementation plan test table lists **schema violation** for both loaders; peer catalog tests (`ItemDefinitionCatalogLoaderTests`, `RecipeDefinitionCatalogLoaderTests`, etc.) use empty `displayName` or similar. Current negative coverage uses wrong top-level shape / `schemaVersion` / cross-refs but not a row-level JsonSchema failure. Add one case per loader (e.g. empty `displayName` on a otherwise-valid prototype row) to lock schema messaging and match precedent.~~ **Done.** `RewardTableDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema` and `EncounterDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema` (empty `displayName`).
|
||||
|
||||
2. **Optional module page snapshot** — Add a short **NEO-101 landed** bullet under `E5_M3_EncounterAndRewardTables.md` (or an **Implementation snapshot** paragraph) pointing at `server/NeonSprawl.Server/Game/Encounters/` and the README sections, so the module page stays aligned with the register without opening the alignment doc.
|
||||
2. ~~**Optional module page snapshot** — Add a short **NEO-101 landed** bullet under `E5_M3_EncounterAndRewardTables.md` (or an **Implementation snapshot** paragraph) pointing at `server/NeonSprawl.Server/Game/Encounters/` and the README sections, so the module page stays aligned with the register without opening the alignment doc.~~ **Done.** Summary **Status** + **Implementation snapshot (NEO-101)** section.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: Host boot failure test (`Host_ShouldFailStartup_WhenEncountersDirectoryInvalid`) covers empty encounters dir only; a symmetric empty **reward-tables** dir case would mirror NEO-88 completeness but is not required by the plan.
|
||||
- ~~Nit: Host boot failure test (`Host_ShouldFailStartup_WhenEncountersDirectoryInvalid`) covers empty encounters dir only; a symmetric empty **reward-tables** dir case would mirror NEO-88 completeness but is not required by the plan.~~ **Done.** `Host_ShouldFailStartup_WhenRewardTablesDirectoryInvalid` added.
|
||||
|
||||
- Nit: `AddEncounterAndRewardCatalogs` re-binds `ContentPathsOptions` (same as other catalog extensions); harmless but duplicates registration if consolidated later.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# NEO-101-focused (19 tests — all passed locally)
|
||||
# NEO-101-focused (22 tests — all passed locally)
|
||||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"
|
||||
|
||||
# CI parity
|
||||
|
|
@ -54,4 +54,4 @@ python3 scripts/validate_content.py
|
|||
dotnet test NeonSprawl.sln
|
||||
```
|
||||
|
||||
**Local run:** 19/19 encounter-catalog tests passed. Full suite: 519 passed, 11 failed — all Postgres/docker-compose harness failures unrelated to this diff.
|
||||
**Local run:** 22/22 encounter-catalog tests passed. Full suite: 519 passed, 11 failed — all Postgres/docker-compose harness failures unrelated to this diff.
|
||||
|
|
|
|||
|
|
@ -244,6 +244,38 @@ public class EncounterDefinitionCatalogLoaderTests
|
|||
Assert.Contains("rewardTableId 'missing_reward_table' is not in reward table catalogs", ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenRowViolatesSchema()
|
||||
{
|
||||
// Arrange
|
||||
var (_, encountersDir, schemaPath) = CreateTempContentLayout();
|
||||
const string bad = """
|
||||
{
|
||||
"schemaVersion": 1,
|
||||
"encounters": [
|
||||
{
|
||||
"id": "prototype_combat_pocket",
|
||||
"displayName": "",
|
||||
"completionCriteria": { "kind": "defeat_all_targets" },
|
||||
"requiredNpcInstanceIds": [
|
||||
"prototype_npc_melee",
|
||||
"prototype_npc_ranged",
|
||||
"prototype_npc_elite"
|
||||
],
|
||||
"rewardTableId": "prototype_combat_pocket_clear"
|
||||
}
|
||||
]
|
||||
}
|
||||
""";
|
||||
File.WriteAllText(Path.Combine(encountersDir, "bad_encounters.json"), bad, Encoding.UTF8);
|
||||
// Act
|
||||
var ex = Record.Exception(() => LoadCatalog(encountersDir, schemaPath));
|
||||
// Assert
|
||||
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||
Assert.Contains("bad_encounters.json", ioe.Message, StringComparison.Ordinal);
|
||||
Assert.Contains("Encounter catalog validation failed", ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenSchemaFileMissing()
|
||||
{
|
||||
|
|
@ -281,6 +313,32 @@ public class EncounterDefinitionCatalogLoaderTests
|
|||
Assert.Equal("prototype_combat_pocket_clear", encounter!.RewardTableId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Host_ShouldFailStartup_WhenRewardTablesDirectoryInvalid()
|
||||
{
|
||||
// Arrange
|
||||
var badDir = Path.Combine(Path.GetTempPath(), "neon-sprawl-empty-reward-tables-" + Guid.NewGuid().ToString("n"));
|
||||
Directory.CreateDirectory(badDir);
|
||||
try
|
||||
{
|
||||
// Act
|
||||
var ex = Record.Exception(() =>
|
||||
{
|
||||
using var factory = new WebApplicationFactory<Program>().WithWebHostBuilder(b =>
|
||||
b.UseSetting("Content:RewardTablesDirectory", badDir));
|
||||
factory.CreateClient();
|
||||
});
|
||||
// Assert
|
||||
Assert.NotNull(ex);
|
||||
Assert.Contains("Reward table catalog validation failed", ex.ToString(), StringComparison.Ordinal);
|
||||
}
|
||||
finally
|
||||
{
|
||||
if (Directory.Exists(badDir))
|
||||
Directory.Delete(badDir);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Host_ShouldFailStartup_WhenEncountersDirectoryInvalid()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -258,6 +258,35 @@ public class RewardTableDefinitionCatalogLoaderTests
|
|||
Assert.Contains("prototype_combat_pocket_clear fixedGrants must match", ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenRowViolatesSchema()
|
||||
{
|
||||
// Arrange
|
||||
var (_, rewardTablesDir, tableSchemaPath, grantRowSchemaPath) = CreateTempContentLayout();
|
||||
const string bad = """
|
||||
{
|
||||
"schemaVersion": 1,
|
||||
"rewardTables": [
|
||||
{
|
||||
"id": "prototype_combat_pocket_clear",
|
||||
"displayName": "",
|
||||
"fixedGrants": [
|
||||
{ "itemId": "scrap_metal_bulk", "quantity": 10 },
|
||||
{ "itemId": "contract_handoff_token", "quantity": 1 }
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
""";
|
||||
File.WriteAllText(Path.Combine(rewardTablesDir, "bad_reward_tables.json"), bad, Encoding.UTF8);
|
||||
// Act
|
||||
var ex = Record.Exception(() => LoadCatalog(rewardTablesDir, tableSchemaPath, grantRowSchemaPath));
|
||||
// Assert
|
||||
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||
Assert.Contains("bad_reward_tables.json", ioe.Message, StringComparison.Ordinal);
|
||||
Assert.Contains("Reward table catalog validation failed", ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenSchemaFileMissing()
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in New Issue