NEO-58: address code review follow-ups
Document optional Bruno smoke in plan; add empty-catalog file loader tests; tighten host DI test AAA; strike through resolved review items.pull/93/head
parent
6656853473
commit
44fc8d7b62
|
|
@ -96,11 +96,12 @@
|
||||||
|
|
||||||
| Item | Coverage |
|
| Item | Coverage |
|
||||||
|------|----------|
|
|------|----------|
|
||||||
| `server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeCatalogLoaderTests.cs` | **Add** — AAA: `Load` happy path with repo-shaped JSON; failures for missing dir/schema, bad `schemaVersion`, schema violations, duplicate `nodeDefId`, incomplete Slice 2 ids, lens set mismatch, orphan yield `nodeDefId`, unknown `itemId`, duplicate yield `nodeDefId`, missing yield for node; `Host_ShouldResolveCatalogFromDi`; `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid`. |
|
| `server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeCatalogLoaderTests.cs` | **Add** — AAA: `Load` happy path with repo-shaped JSON; failures for missing dir/schema, bad `schemaVersion`, schema violations, duplicate `nodeDefId`, incomplete Slice 2 ids, lens set mismatch, orphan yield `nodeDefId`, unknown `itemId`, duplicate yield `nodeDefId`, missing yield for node, empty `*_resource_nodes.json` / `*_resource_yields.json` directories; `Host_ShouldResolveCatalogFromDi`; `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid`. |
|
||||||
| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | **Change** if needed — ensure resource-node content is discoverable in test host (same ancestor walk as items). |
|
| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | **Change** if needed — ensure resource-node content is discoverable in test host (same ancestor walk as items). |
|
||||||
| **CI / PR gate** | Existing `scripts/validate_content.py` remains content regression; server tests guard C#/Python drift for Slice 2 constants. |
|
| **CI / PR gate** | Existing `scripts/validate_content.py` remains content regression; server tests guard C#/Python drift for Slice 2 constants. |
|
||||||
|
| **Bruno smoke (optional)** | `bruno/neon-sprawl-server/resource-node-catalog/Health after resource node catalog load.bru` — `GET /health` after boot; no resource-node HTTP API this story (pre-commit hook when `Program.cs` changes). |
|
||||||
|
|
||||||
No Bruno or client tests (out of scope).
|
No client tests (out of scope).
|
||||||
|
|
||||||
## Open questions / risks
|
## Open questions / risks
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -32,15 +32,15 @@ None.
|
||||||
|
|
||||||
## Suggestions
|
## Suggestions
|
||||||
|
|
||||||
1. **Plan vs Bruno** — [`NEO-58-implementation-plan.md`](../plans/NEO-58-implementation-plan.md) lists Bruno as out of scope, but `bruno/neon-sprawl-server/resource-node-catalog/` adds a health smoke request. Either add a one-line note under **Tests** (“optional Bruno smoke”) or drop the folder to keep plan scope literal; the request itself is fine for manual boot verification.
|
1. ~~**Plan vs Bruno** — [`NEO-58-implementation-plan.md`](../plans/NEO-58-implementation-plan.md) lists Bruno as out of scope, but `bruno/neon-sprawl-server/resource-node-catalog/` adds a health smoke request. Either add a one-line note under **Tests** (“optional Bruno smoke”) or drop the folder to keep plan scope literal; the request itself is fine for manual boot verification.~~ **Done.** Plan **Tests** table now documents optional Bruno health smoke (pre-commit when `Program.cs` changes).
|
||||||
|
|
||||||
2. **Explicit empty-catalog file test (optional)** — `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` covers an empty directory; a loader unit test asserting `no *_resource_nodes.json files` (and yields) would mirror item-catalog negative coverage and make failure messages grep-friendly in CI logs.
|
2. ~~**Explicit empty-catalog file test (optional)** — `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` covers an empty directory; a loader unit test asserting `no *_resource_nodes.json files` (and yields) would mirror item-catalog negative coverage and make failure messages grep-friendly in CI logs.~~ **Done.** Added `Load_ShouldThrow_WhenNoResourceNodesJsonFiles` and `Load_ShouldThrow_WhenNoResourceYieldsJsonFiles`.
|
||||||
|
|
||||||
## Nits
|
## Nits
|
||||||
|
|
||||||
- Nit: Slice 2 lens gate checks **set** equality (same as Python/NEO-57); duplicate `gatherLens` on two nodes still fails via set mismatch — acceptable for prototype.
|
- Nit: Slice 2 lens gate checks **set** equality (same as Python/NEO-57); duplicate `gatherLens` on two nodes still fails via set mismatch — acceptable for prototype.
|
||||||
|
|
||||||
- Nit: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` uses `/health` in **Act** (NEO-51 item-catalog host test does the same); strict AAA would move the HTTP call to **Arrange** and keep **Act** as `GetRequiredService<ResourceNodeCatalog>()` only.
|
- ~~Nit: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` uses `/health` in **Act** (NEO-51 item-catalog host test does the same); strict AAA would move the HTTP call to **Arrange** and keep **Act** as `GetRequiredService<ResourceNodeCatalog>()` only.~~ **Done.** Host test is sync; **Act** is `GetRequiredService<ResourceNodeCatalog>()` only (`CreateClient()` in **Arrange** builds the host).
|
||||||
|
|
||||||
- Nit: No `docs/manual-qa/NEO-58.md` — consistent with plan (server boot-only, no user-facing HTTP for resource nodes); Bruno health folder is sufficient smoke.
|
- Nit: No `docs/manual-qa/NEO-58.md` — consistent with plan (server boot-only, no user-facing HTTP for resource nodes); Bruno health folder is sufficient smoke.
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,3 @@
|
||||||
using System.Net;
|
|
||||||
using System.Text;
|
using System.Text;
|
||||||
using System.Text.Json.Nodes;
|
using System.Text.Json.Nodes;
|
||||||
using Microsoft.AspNetCore.Mvc.Testing;
|
using Microsoft.AspNetCore.Mvc.Testing;
|
||||||
|
|
@ -486,6 +485,44 @@ public class ResourceNodeCatalogLoaderTests
|
||||||
Assert.Contains(missingDir, ioe.Message, StringComparison.Ordinal);
|
Assert.Contains(missingDir, ioe.Message, StringComparison.Ordinal);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_ShouldThrow_WhenNoResourceNodesJsonFiles()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var (_, resourceNodesDir, nodeSchemaPath, yieldSchemaPath) = CreateTempContentLayout();
|
||||||
|
WriteYieldCatalog(resourceNodesDir, ValidPrototypeYieldCatalogJson);
|
||||||
|
// Act
|
||||||
|
var ex = Record.Exception(() => ResourceNodeCatalogLoader.Load(
|
||||||
|
resourceNodesDir,
|
||||||
|
nodeSchemaPath,
|
||||||
|
yieldSchemaPath,
|
||||||
|
ValidPrototypeItemIds,
|
||||||
|
NullLogger.Instance));
|
||||||
|
// Assert
|
||||||
|
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||||
|
Assert.Contains("no *_resource_nodes.json files", ioe.Message, StringComparison.Ordinal);
|
||||||
|
Assert.Contains(resourceNodesDir, ioe.Message, StringComparison.Ordinal);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_ShouldThrow_WhenNoResourceYieldsJsonFiles()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var (_, resourceNodesDir, nodeSchemaPath, yieldSchemaPath) = CreateTempContentLayout();
|
||||||
|
WriteNodeCatalog(resourceNodesDir, ValidPrototypeNodeCatalogJson);
|
||||||
|
// Act
|
||||||
|
var ex = Record.Exception(() => ResourceNodeCatalogLoader.Load(
|
||||||
|
resourceNodesDir,
|
||||||
|
nodeSchemaPath,
|
||||||
|
yieldSchemaPath,
|
||||||
|
ValidPrototypeItemIds,
|
||||||
|
NullLogger.Instance));
|
||||||
|
// Assert
|
||||||
|
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||||
|
Assert.Contains("no *_resource_yields.json files", ioe.Message, StringComparison.Ordinal);
|
||||||
|
Assert.Contains(resourceNodesDir, ioe.Message, StringComparison.Ordinal);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Load_ShouldThrow_WhenNodeSchemaFileMissing()
|
public void Load_ShouldThrow_WhenNodeSchemaFileMissing()
|
||||||
{
|
{
|
||||||
|
|
@ -506,16 +543,14 @@ public class ResourceNodeCatalogLoaderTests
|
||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds()
|
public void Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds()
|
||||||
{
|
{
|
||||||
// Arrange
|
// Arrange
|
||||||
await using var factory = new InMemoryWebApplicationFactory();
|
using var factory = new InMemoryWebApplicationFactory();
|
||||||
using var client = factory.CreateClient();
|
_ = factory.CreateClient();
|
||||||
// Act
|
// Act
|
||||||
var response = await client.GetAsync("/health");
|
|
||||||
// Assert
|
|
||||||
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
|
|
||||||
var catalog = factory.Services.GetRequiredService<ResourceNodeCatalog>();
|
var catalog = factory.Services.GetRequiredService<ResourceNodeCatalog>();
|
||||||
|
// Assert
|
||||||
Assert.Equal(4, catalog.DistinctNodeCount);
|
Assert.Equal(4, catalog.DistinctNodeCount);
|
||||||
Assert.True(catalog.TryGetNode("prototype_bio_mat_gamma", out var node));
|
Assert.True(catalog.TryGetNode("prototype_bio_mat_gamma", out var node));
|
||||||
Assert.Equal("bio", node!.GatherLens);
|
Assert.Equal("bio", node!.GatherLens);
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue