From 1fb4368a2ca950117d07d50f1ee9ea6a85f1cbe6 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 16:45:36 -0400 Subject: [PATCH] NEO-66: Address code review follow-ups for recipe catalog load. Pin Content:RecipesDirectory in all host test factories, add loader tests for empty dir/invalid JSON/unknown output itemId, document manual QA skip. --- docs/plans/NEO-66-implementation-plan.md | 4 +- docs/reviews/2026-05-24-NEO-66.md | 8 ++-- .../RecipeDefinitionCatalogLoaderTests.cs | 47 +++++++++++++++++++ ...vityDeniedRegistryWebApplicationFactory.cs | 5 ++ .../PostgresWebApplicationFactory.cs | 5 ++ ...wardDeniedRegistryWebApplicationFactory.cs | 5 ++ ...vityDeniedRegistryWebApplicationFactory.cs | 5 ++ 7 files changed, 74 insertions(+), 5 deletions(-) diff --git a/docs/plans/NEO-66-implementation-plan.md b/docs/plans/NEO-66-implementation-plan.md index f94a9b0..c0b696a 100644 --- a/docs/plans/NEO-66-implementation-plan.md +++ b/docs/plans/NEO-66-implementation-plan.md @@ -103,7 +103,9 @@ | Test file | What it covers | |-----------|----------------| -| `server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs` | **Unit:** valid eight-recipe prototype catalog loads; `recipes` not array; schema violation (incl. bad I/O row); duplicate `id` across files; wrong/missing Slice 3 ids; missing `process` or `make`; `requiredSkillId` ≠ `refine`; unknown `itemId` in inputs/outputs; unknown `requiredSkillId`; `schemaVersion` ≠ 1; missing dir/schema. **Host:** `InMemoryWebApplicationFactory` + `/health` + DI `RecipeDefinitionCatalog` count 8; `WebApplicationFactory` with empty recipes dir fails startup with actionable message. | +| `server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs` | **Unit:** valid eight-recipe prototype catalog loads; `recipes` not array; schema violation (incl. bad I/O row); duplicate `id` across files; wrong/missing Slice 3 ids; missing `process` or `make`; `requiredSkillId` ≠ `refine`; unknown `itemId` in inputs/outputs; unknown `requiredSkillId`; `schemaVersion` ≠ 1; missing dir/schema; no `*_recipes.json` files; invalid JSON. **Host:** `InMemoryWebApplicationFactory` + `/health` + DI `RecipeDefinitionCatalog` count 8; `WebApplicationFactory` with empty recipes dir fails startup with actionable message. | + +**Manual QA:** Skipped — no player-visible HTTP; acceptance is fully covered by AAA loader/host tests above plus Bruno `recipe-catalog/Health after recipe catalog load.bru` (same pattern as NEO-51). ## Open questions / risks diff --git a/docs/reviews/2026-05-24-NEO-66.md b/docs/reviews/2026-05-24-NEO-66.md index 158a44e..8164c86 100644 --- a/docs/reviews/2026-05-24-NEO-66.md +++ b/docs/reviews/2026-05-24-NEO-66.md @@ -24,7 +24,7 @@ NEO-66 delivers **E3M2-02**: fail-fast startup load of `content/recipes/*_recipe | [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server startup catalog only; no client mutation. | | [`scripts/validate_content.py`](../../scripts/validate_content.py) | **Matches** — C# gate mirrors `_validate_recipe_catalogs` + `_prototype_slice3_recipe_gate`; sync comments on `PrototypeSlice3RecipeCatalogRules`. | | [`server/README.md`](../../server/README.md) | **Matches** — Recipe catalog section (config keys, discovery, fail-fast, Slice 3 parity). | -| Manual QA | **N/A** — plan defers to automated loader/host tests + Bruno health smoke (same pattern as NEO-51); no player-visible HTTP. Consider one-line skip rationale in plan **Tests** section per [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md). | +| Manual QA | **N/A** — plan documents skip rationale: automated loader/host tests + Bruno health smoke (same pattern as NEO-51); no player-visible HTTP. | Register/tracking: alignment table E3.M2 **In Progress** with NEO-66 note is correct. @@ -34,11 +34,11 @@ None. ## Suggestions -1. **Pin `Content:RecipesDirectory` in other test factories** — `InMemoryWebApplicationFactory` pins recipes (per plan). Also pin in `PostgresWebApplicationFactory`, `MissionRewardDeniedRegistryWebApplicationFactory`, and `RefineActivityDeniedRegistryWebApplicationFactory` (and any future host factories) so CI never depends on ancestor discovery alone when layout differs. `SalvageActivityDeniedRegistryWebApplicationFactory` already pins items/resource-nodes; add recipes there too for symmetry. +1. ~~**Pin `Content:RecipesDirectory` in other test factories** — `InMemoryWebApplicationFactory` pins recipes (per plan). Also pin in `PostgresWebApplicationFactory`, `MissionRewardDeniedRegistryWebApplicationFactory`, and `RefineActivityDeniedRegistryWebApplicationFactory` (and any future host factories) so CI never depends on ancestor discovery alone when layout differs. `SalvageActivityDeniedRegistryWebApplicationFactory` already pins items/resource-nodes; add recipes there too for symmetry.~~ **Done.** All five host factories now pin `Content:RecipesDirectory`. -2. **Loader test gaps vs plan test table** — Add AAA cases for: empty recipes directory (no `*_recipes.json`), invalid JSON file, unknown `itemId` in **outputs** (inputs covered). Low risk given CI + existing coverage, but closes the plan’s enumerated failure modes. +2. ~~**Loader test gaps vs plan test table** — Add AAA cases for: empty recipes directory (no `*_recipes.json`), invalid JSON file, unknown `itemId` in **outputs** (inputs covered). Low risk given CI + existing coverage, but closes the plan’s enumerated failure modes.~~ **Done.** Added `Load_ShouldThrow_WhenNoRecipeCatalogFiles`, `Load_ShouldThrow_WhenJsonIsInvalid`, `Load_ShouldThrow_WhenUnknownItemIdInOutputs`. -3. **Plan Tests section — manual QA skip note** — Add one line stating manual QA is skipped because acceptance is fully covered by unit/host tests + Bruno health (per planning-implementation-docs). +3. ~~**Plan Tests section — manual QA skip note** — Add one line stating manual QA is skipped because acceptance is fully covered by unit/host tests + Bruno health (per planning-implementation-docs).~~ **Done.** See [NEO-66 plan](../plans/NEO-66-implementation-plan.md) **Tests** section. ## Nits diff --git a/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs b/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs index 551da97..04ee8c9 100644 --- a/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Crafting/RecipeDefinitionCatalogLoaderTests.cs @@ -358,6 +358,53 @@ public class RecipeDefinitionCatalogLoaderTests Assert.Contains("is not in item catalogs", ioe.Message, StringComparison.Ordinal); } + [Fact] + public void Load_ShouldThrow_WhenUnknownItemIdInOutputs() + { + // Arrange + var (_, recipesDir, defSchemaPath, ioSchemaPath) = CreateTempContentLayout(); + var root = JsonNode.Parse(ValidPrototypeCatalogJson) as JsonObject + ?? throw new InvalidOperationException("expected object root"); + var row = GetRecipeRow(root, "refine_scrap_standard"); + var outputs = row["outputs"] as JsonArray ?? throw new InvalidOperationException("expected outputs array"); + outputs[0] = JsonNode.Parse("""{"itemId": "unknown_item_xyz", "quantity": 1}"""); + WriteCatalog(recipesDir, root.ToJsonString()); + // Act + var ex = Record.Exception(() => LoadCatalog(recipesDir, defSchemaPath, ioSchemaPath)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("unknown_item_xyz", ioe.Message, StringComparison.Ordinal); + Assert.Contains("outputs[0]", ioe.Message, StringComparison.Ordinal); + Assert.Contains("is not in item catalogs", ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_ShouldThrow_WhenNoRecipeCatalogFiles() + { + // Arrange + var (_, recipesDir, defSchemaPath, ioSchemaPath) = CreateTempContentLayout(); + // Act + var ex = Record.Exception(() => LoadCatalog(recipesDir, defSchemaPath, ioSchemaPath)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("no *_recipes.json files", ioe.Message, StringComparison.Ordinal); + Assert.Contains(recipesDir, ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_ShouldThrow_WhenJsonIsInvalid() + { + // Arrange + var (_, recipesDir, defSchemaPath, ioSchemaPath) = CreateTempContentLayout(); + File.WriteAllText(Path.Combine(recipesDir, "bad_recipes.json"), "{ not json", Encoding.UTF8); + // Act + var ex = Record.Exception(() => LoadCatalog(recipesDir, defSchemaPath, ioSchemaPath)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("bad_recipes.json", ioe.Message, StringComparison.Ordinal); + Assert.Contains("invalid JSON", ioe.Message, StringComparison.Ordinal); + } + [Fact] public void Load_ShouldThrow_WhenUnknownRequiredSkillId() { diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs index fabbdb8..f9efdbe 100644 --- a/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Time.Testing; using NeonSprawl.Server.Game.AbilityInput; +using NeonSprawl.Server.Game.Crafting; using NeonSprawl.Server.Game.Gathering; using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Mastery; @@ -37,10 +38,14 @@ public sealed class SalvageActivityDeniedRegistryWebApplicationFactory : WebAppl var resourceNodesDir = ResourceNodeCatalogPathResolution.TryDiscoverResourceNodesDirectory(AppContext.BaseDirectory) ?? throw new InvalidOperationException( "Could not discover repo content/resource-nodes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); + var recipesDir = RecipeCatalogPathResolution.TryDiscoverRecipesDirectory(AppContext.BaseDirectory) + ?? throw new InvalidOperationException( + "Could not discover repo content/recipes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); builder.UseSetting("Content:SkillsDirectory", skillsDir); builder.UseSetting("Content:MasteryDirectory", masteryDir); builder.UseSetting("Content:ItemsDirectory", itemsDir); builder.UseSetting("Content:ResourceNodesDirectory", resourceNodesDir); + builder.UseSetting("Content:RecipesDirectory", recipesDir); builder.UseSetting("Game:EnableMasteryFixtureApi", "true"); builder.ConfigureTestServices(services => diff --git a/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs index 4c13ca3..0337336 100644 --- a/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.Extensions.Configuration; +using NeonSprawl.Server.Game.Crafting; using NeonSprawl.Server.Game.Gathering; using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Mastery; @@ -35,10 +36,14 @@ public sealed class PostgresWebApplicationFactory : WebApplicationFactory { diff --git a/server/NeonSprawl.Server.Tests/Game/Skills/MissionRewardDeniedRegistryWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/Skills/MissionRewardDeniedRegistryWebApplicationFactory.cs index 5055913..1e29349 100644 --- a/server/NeonSprawl.Server.Tests/Game/Skills/MissionRewardDeniedRegistryWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/Game/Skills/MissionRewardDeniedRegistryWebApplicationFactory.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Time.Testing; using NeonSprawl.Server.Game.AbilityInput; +using NeonSprawl.Server.Game.Crafting; using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.Skills; @@ -29,8 +30,12 @@ public sealed class MissionRewardDeniedRegistryWebApplicationFactory : WebApplic var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory) ?? throw new InvalidOperationException( "Could not discover repo content/mastery from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); + var recipesDir = RecipeCatalogPathResolution.TryDiscoverRecipesDirectory(AppContext.BaseDirectory) + ?? throw new InvalidOperationException( + "Could not discover repo content/recipes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); builder.UseSetting("Content:SkillsDirectory", skillsDir); builder.UseSetting("Content:MasteryDirectory", masteryDir); + builder.UseSetting("Content:RecipesDirectory", recipesDir); builder.ConfigureTestServices(services => { diff --git a/server/NeonSprawl.Server.Tests/Game/Skills/RefineActivityDeniedRegistryWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/Skills/RefineActivityDeniedRegistryWebApplicationFactory.cs index bd18798..ea04cc8 100644 --- a/server/NeonSprawl.Server.Tests/Game/Skills/RefineActivityDeniedRegistryWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/Game/Skills/RefineActivityDeniedRegistryWebApplicationFactory.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Time.Testing; using NeonSprawl.Server.Game.AbilityInput; +using NeonSprawl.Server.Game.Crafting; using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.Skills; @@ -29,8 +30,12 @@ public sealed class RefineActivityDeniedRegistryWebApplicationFactory : WebAppli var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory) ?? throw new InvalidOperationException( "Could not discover repo content/mastery from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); + var recipesDir = RecipeCatalogPathResolution.TryDiscoverRecipesDirectory(AppContext.BaseDirectory) + ?? throw new InvalidOperationException( + "Could not discover repo content/recipes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); builder.UseSetting("Content:SkillsDirectory", skillsDir); builder.UseSetting("Content:MasteryDirectory", masteryDir); + builder.UseSetting("Content:RecipesDirectory", recipesDir); builder.ConfigureTestServices(services => {