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.
pull/100/head
VinPropane 2026-05-24 16:45:36 -04:00
parent c279eed572
commit 1fb4368a2c
7 changed files with 74 additions and 5 deletions

View File

@ -103,7 +103,9 @@
| Test file | What it covers | | 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 ## Open questions / risks

View File

@ -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. | | [`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`. | | [`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). | | [`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. Register/tracking: alignment table E3.M2 **In Progress** with NEO-66 note is correct.
@ -34,11 +34,11 @@ None.
## Suggestions ## 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 plans 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 plans 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 ## Nits

View File

@ -358,6 +358,53 @@ public class RecipeDefinitionCatalogLoaderTests
Assert.Contains("is not in item catalogs", ioe.Message, StringComparison.Ordinal); 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<InvalidOperationException>(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<InvalidOperationException>(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<InvalidOperationException>(ex);
Assert.Contains("bad_recipes.json", ioe.Message, StringComparison.Ordinal);
Assert.Contains("invalid JSON", ioe.Message, StringComparison.Ordinal);
}
[Fact] [Fact]
public void Load_ShouldThrow_WhenUnknownRequiredSkillId() public void Load_ShouldThrow_WhenUnknownRequiredSkillId()
{ {

View File

@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Time.Testing; using Microsoft.Extensions.Time.Testing;
using NeonSprawl.Server.Game.AbilityInput; using NeonSprawl.Server.Game.AbilityInput;
using NeonSprawl.Server.Game.Crafting;
using NeonSprawl.Server.Game.Gathering; using NeonSprawl.Server.Game.Gathering;
using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Items;
using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.Mastery;
@ -37,10 +38,14 @@ public sealed class SalvageActivityDeniedRegistryWebApplicationFactory : WebAppl
var resourceNodesDir = ResourceNodeCatalogPathResolution.TryDiscoverResourceNodesDirectory(AppContext.BaseDirectory) var resourceNodesDir = ResourceNodeCatalogPathResolution.TryDiscoverResourceNodesDirectory(AppContext.BaseDirectory)
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
"Could not discover repo content/resource-nodes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); "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:SkillsDirectory", skillsDir);
builder.UseSetting("Content:MasteryDirectory", masteryDir); builder.UseSetting("Content:MasteryDirectory", masteryDir);
builder.UseSetting("Content:ItemsDirectory", itemsDir); builder.UseSetting("Content:ItemsDirectory", itemsDir);
builder.UseSetting("Content:ResourceNodesDirectory", resourceNodesDir); builder.UseSetting("Content:ResourceNodesDirectory", resourceNodesDir);
builder.UseSetting("Content:RecipesDirectory", recipesDir);
builder.UseSetting("Game:EnableMasteryFixtureApi", "true"); builder.UseSetting("Game:EnableMasteryFixtureApi", "true");
builder.ConfigureTestServices(services => builder.ConfigureTestServices(services =>

View File

@ -1,6 +1,7 @@
using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.Mvc.Testing;
using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Configuration;
using NeonSprawl.Server.Game.Crafting;
using NeonSprawl.Server.Game.Gathering; using NeonSprawl.Server.Game.Gathering;
using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Items;
using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.Mastery;
@ -35,10 +36,14 @@ public sealed class PostgresWebApplicationFactory : WebApplicationFactory<Progra
var resourceNodesDir = ResourceNodeCatalogPathResolution.TryDiscoverResourceNodesDirectory(AppContext.BaseDirectory) var resourceNodesDir = ResourceNodeCatalogPathResolution.TryDiscoverResourceNodesDirectory(AppContext.BaseDirectory)
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
"Could not discover repo content/resource-nodes from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); "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:SkillsDirectory", skillsDir);
builder.UseSetting("Content:MasteryDirectory", masteryDir); builder.UseSetting("Content:MasteryDirectory", masteryDir);
builder.UseSetting("Content:ItemsDirectory", itemsDir); builder.UseSetting("Content:ItemsDirectory", itemsDir);
builder.UseSetting("Content:ResourceNodesDirectory", resourceNodesDir); builder.UseSetting("Content:ResourceNodesDirectory", resourceNodesDir);
builder.UseSetting("Content:RecipesDirectory", recipesDir);
builder.ConfigureAppConfiguration((_, config) => builder.ConfigureAppConfiguration((_, config) =>
{ {

View File

@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Time.Testing; using Microsoft.Extensions.Time.Testing;
using NeonSprawl.Server.Game.AbilityInput; using NeonSprawl.Server.Game.AbilityInput;
using NeonSprawl.Server.Game.Crafting;
using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.Mastery;
using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.PositionState;
using NeonSprawl.Server.Game.Skills; using NeonSprawl.Server.Game.Skills;
@ -29,8 +30,12 @@ public sealed class MissionRewardDeniedRegistryWebApplicationFactory : WebApplic
var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory) var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory)
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
"Could not discover repo content/mastery from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); "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:SkillsDirectory", skillsDir);
builder.UseSetting("Content:MasteryDirectory", masteryDir); builder.UseSetting("Content:MasteryDirectory", masteryDir);
builder.UseSetting("Content:RecipesDirectory", recipesDir);
builder.ConfigureTestServices(services => builder.ConfigureTestServices(services =>
{ {

View File

@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Time.Testing; using Microsoft.Extensions.Time.Testing;
using NeonSprawl.Server.Game.AbilityInput; using NeonSprawl.Server.Game.AbilityInput;
using NeonSprawl.Server.Game.Crafting;
using NeonSprawl.Server.Game.Mastery; using NeonSprawl.Server.Game.Mastery;
using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.PositionState;
using NeonSprawl.Server.Game.Skills; using NeonSprawl.Server.Game.Skills;
@ -29,8 +30,12 @@ public sealed class RefineActivityDeniedRegistryWebApplicationFactory : WebAppli
var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory) var masteryDir = MasteryCatalogPathResolution.TryDiscoverMasteryDirectory(AppContext.BaseDirectory)
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
"Could not discover repo content/mastery from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); "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:SkillsDirectory", skillsDir);
builder.UseSetting("Content:MasteryDirectory", masteryDir); builder.UseSetting("Content:MasteryDirectory", masteryDir);
builder.UseSetting("Content:RecipesDirectory", recipesDir);
builder.ConfigureTestServices(services => builder.ConfigureTestServices(services =>
{ {