From 3834b2316a7aa14937ed0330f3cdb19cbaa89839 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 15:46:20 -0400 Subject: [PATCH] NEO-63: address code review follow-ups. AAA depletion test layout, source_kind_not_allowed assertion, HTTP inventory_full deny test, trim duplicate XP test, doc comments. --- docs/reviews/2026-05-24-NEO-63.md | 12 +-- .../Game/Interaction/InteractionApiTests.cs | 2 +- ...ctionResourceNodeGatherIntegrationTests.cs | 92 ++++++++++++++----- .../InteractionResourceNodeGatherXpTests.cs | 38 +------- .../Game/Gathering/GatherOperations.cs | 2 +- .../Skills/SkillProgressionGrantOperations.cs | 2 +- 6 files changed, 77 insertions(+), 71 deletions(-) diff --git a/docs/reviews/2026-05-24-NEO-63.md b/docs/reviews/2026-05-24-NEO-63.md index 1e4ae7e..7680561 100644 --- a/docs/reviews/2026-05-24-NEO-63.md +++ b/docs/reviews/2026-05-24-NEO-63.md @@ -34,19 +34,19 @@ None. ## Suggestions -1. **AAA layout in depletion integration test** — `InteractionResourceNodeGatherIntegrationTests.PostInteract_ResourceNode_WhenDepleted_ShouldDenyWithNodeDepletedWithoutGrants` asserts **`Allowed`** inside the Arrange loop (10 pre-gathers). Move loop invariants to **Act** (fire-and-forget or collect last response) and keep all expectations in **Assert**, per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) — same pattern flagged on NEO-61. +1. ~~**AAA layout in depletion integration test** — `InteractionResourceNodeGatherIntegrationTests.PostInteract_ResourceNode_WhenDepleted_ShouldDenyWithNodeDepletedWithoutGrants` asserts **`Allowed`** inside the Arrange loop (10 pre-gathers). Move loop invariants to **Act** (fire-and-forget or collect last response) and keep all expectations in **Assert**, per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) — same pattern flagged on NEO-61.~~ **Done.** — pre-gather loop moved to **Act**; **`Assert.All`** on successful responses; depletion + totals asserted in **Assert**. -2. **Tighten skill-deny integration assertion** — `PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldDenyWithoutGrants` only checks `ReasonCode` is non-empty. Assert the expected passthrough **`source_kind_not_allowed`** (from `SkillProgressionSnapshotApi.ReasonSourceKindNotAllowed`) so regressions in XP rollback + reason mapping are caught explicitly. +2. ~~**Tighten skill-deny integration assertion** — `PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldDenyWithoutGrants` only checks `ReasonCode` is non-empty. Assert the expected passthrough **`source_kind_not_allowed`** (from `SkillProgressionSnapshotApi.ReasonSourceKindNotAllowed`) so regressions in XP rollback + reason mapping are caught explicitly.~~ **Done.** -3. **Optional HTTP `inventory_full` deny** — Engine test covers full-bag deny without depletion; no interact-level test yet. Low priority since **`GatherOperationsTests`** already locks the rule; add one HTTP test if you want parity with depletion deny at the wire boundary. +3. ~~**Optional HTTP `inventory_full` deny** — Engine test covers full-bag deny without depletion; no interact-level test yet. Low priority since **`GatherOperationsTests`** already locks the rule; add one HTTP test if you want parity with depletion deny at the wire boundary.~~ **Done.** — **`PostInteract_ResourceNode_WhenBagFull_ShouldDenyWithInventoryFullWithoutDepletingNode`**. -4. **Test duplication (optional cleanup)** — `InteractionResourceNodeGatherXpTests` overlaps with `InteractionResourceNodeGatherIntegrationTests` on terminal control and alpha XP smoke. Plan kept both for regression; consider trimming the legacy file to terminal-only once NEO-63 integration tests are trusted. +4. ~~**Test duplication (optional cleanup)** — `InteractionResourceNodeGatherXpTests` overlaps with `InteractionResourceNodeGatherIntegrationTests` on terminal control and alpha XP smoke. Plan kept both for regression; consider trimming the legacy file to terminal-only once NEO-63 integration tests are trusted.~~ **Done.** — legacy file trimmed to terminal-only; **`InteractionApiTests`** summary points at NEO-63 integration tests. ## Nits -- Nit: **`SkillProgressionGrantOperations`** summary still references “NEO-41 gather interact hook”; interact now routes through **`GatherOperations`** (NEO-63). +- ~~Nit: **`SkillProgressionGrantOperations`** summary still references “NEO-41 gather interact hook”; interact now routes through **`GatherOperations`** (NEO-63).~~ **Done.** -- Nit: **`GatherOperations`** class comment says “Interact wiring is NEO-63” — wiring is landed; update to “wired from **`InteractionApi`** (NEO-63)”. +- ~~Nit: **`GatherOperations`** class comment says “Interact wiring is NEO-63” — wiring is landed; update to “wired from **`InteractionApi`** (NEO-63)”.~~ **Done.** - Nit: Stacked branch — five commits include NEO-62 plan + engine before NEO-63 wiring. Call out both issue keys in the PR title/description so reviewers know scope. diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionApiTests.cs index c2e380c..4b09426 100644 --- a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionApiTests.cs @@ -7,7 +7,7 @@ using Xunit; namespace NeonSprawl.Server.Tests.Game.Interaction; -/// Integration tests for (NS-18). NEO-41: covers gather XP on resource_node kind. +/// Integration tests for (NS-18). NEO-63: covers gather via interact. public class InteractionApiTests { [Fact] diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherIntegrationTests.cs index a61c89f..fff00ec 100644 --- a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherIntegrationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherIntegrationTests.cs @@ -59,27 +59,67 @@ public sealed class InteractionResourceNodeGatherIntegrationTests await using var factory = new InMemoryWebApplicationFactory(); var client = factory.CreateClient(); await MoveNearAlphaAsync(client); - for (var i = 0; i < 10; i++) + var request = new InteractionRequest { - var gather = await client.PostAsJsonAsync( - "/game/players/dev-local-1/interact", - new InteractionRequest - { - SchemaVersion = InteractionRequest.CurrentSchemaVersion, - InteractableId = PrototypeInteractableRegistry.PrototypeResourceNodeAlphaId, - }); - Assert.Equal(HttpStatusCode.OK, gather.StatusCode); - var body = await gather.Content.ReadFromJsonAsync(); - Assert.NotNull(body); - Assert.True(body!.Allowed); + SchemaVersion = InteractionRequest.CurrentSchemaVersion, + InteractableId = PrototypeInteractableRegistry.PrototypeResourceNodeAlphaId, + }; + + // Act — exhaust capacity, then one more interact + var successfulGathers = new InteractionResponse[10]; + for (var i = 0; i < successfulGathers.Length; i++) + { + var gather = await client.PostAsJsonAsync("/game/players/dev-local-1/interact", request); + successfulGathers[i] = (await gather.Content.ReadFromJsonAsync())!; } + var depletedInteract = await client.PostAsJsonAsync("/game/players/dev-local-1/interact", request); + var inventoryAfter = await client.GetAsync("/game/players/dev-local-1/inventory"); + var progressionAfter = await client.GetAsync("/game/players/dev-local-1/skill-progression"); + + // Assert + Assert.All(successfulGathers, static r => Assert.True(r.Allowed)); + Assert.Equal(HttpStatusCode.OK, depletedInteract.StatusCode); + var envelope = await depletedInteract.Content.ReadFromJsonAsync(); + Assert.NotNull(envelope); + Assert.False(envelope!.Allowed); + Assert.Equal(GatherReasonCodes.NodeDepleted, envelope.ReasonCode); + Assert.Equal(10, CountItem( + (await inventoryAfter.Content.ReadFromJsonAsync())!, + "scrap_metal_bulk")); + Assert.Equal( + 10 * GatherSkillXpConstants.ActivityXpPerResourceNodeGather, + (await progressionAfter.Content.ReadFromJsonAsync())! + .Skills!.Single(static s => s.Id == "salvage").Xp); + } + + [Fact] + public async Task PostInteract_ResourceNode_WhenBagFull_ShouldDenyWithInventoryFullWithoutDepletingNode() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++) + { + await client.PostAsJsonAsync( + "/game/players/dev-local-1/inventory", + new PlayerInventoryMutationRequest + { + SchemaVersion = PlayerInventoryMutationRequest.CurrentSchemaVersion, + MutationKind = "add", + ItemId = "survey_drone_kit", + Quantity = 1, + }); + } + + await MoveNearAlphaAsync(client); var inventoryBefore = await client.GetAsync("/game/players/dev-local-1/inventory"); var progressionBefore = await client.GetAsync("/game/players/dev-local-1/skill-progression"); - var bagBefore = await inventoryBefore.Content.ReadFromJsonAsync(); - var skillsBefore = await progressionBefore.Content.ReadFromJsonAsync(); - var scrapBefore = CountItem(bagBefore!, "scrap_metal_bulk"); - var salvageBefore = skillsBefore!.Skills!.Single(static s => s.Id == "salvage").Xp; + var scrapBefore = CountItem( + (await inventoryBefore.Content.ReadFromJsonAsync())!, + "scrap_metal_bulk"); + var salvageBefore = (await progressionBefore.Content.ReadFromJsonAsync())! + .Skills!.Single(static s => s.Id == "salvage").Xp; // Act var interact = await client.PostAsJsonAsync( @@ -89,22 +129,24 @@ public sealed class InteractionResourceNodeGatherIntegrationTests SchemaVersion = InteractionRequest.CurrentSchemaVersion, InteractableId = PrototypeInteractableRegistry.PrototypeResourceNodeAlphaId, }); - var inventoryAfter = await client.GetAsync("/game/players/dev-local-1/inventory"); - var progressionAfter = await client.GetAsync("/game/players/dev-local-1/skill-progression"); // Assert Assert.Equal(HttpStatusCode.OK, interact.StatusCode); var envelope = await interact.Content.ReadFromJsonAsync(); Assert.NotNull(envelope); Assert.False(envelope!.Allowed); - Assert.Equal(GatherReasonCodes.NodeDepleted, envelope.ReasonCode); - - var bagAfter = await inventoryAfter.Content.ReadFromJsonAsync(); - var skillsAfter = await progressionAfter.Content.ReadFromJsonAsync(); - Assert.Equal(scrapBefore, CountItem(bagAfter!, "scrap_metal_bulk")); + Assert.Equal(GatherReasonCodes.InventoryFull, envelope.ReasonCode); + Assert.Equal( + scrapBefore, + CountItem( + (await (await client.GetAsync("/game/players/dev-local-1/inventory")).Content + .ReadFromJsonAsync())!, + "scrap_metal_bulk")); Assert.Equal( salvageBefore, - skillsAfter!.Skills!.Single(static s => s.Id == "salvage").Xp); + (await (await client.GetAsync("/game/players/dev-local-1/skill-progression")).Content + .ReadFromJsonAsync())! + .Skills!.Single(static s => s.Id == "salvage").Xp); } [Fact] @@ -171,7 +213,7 @@ public sealed class InteractionResourceNodeGatherIntegrationTests var envelope = await interact.Content.ReadFromJsonAsync(); Assert.NotNull(envelope); Assert.False(envelope!.Allowed); - Assert.False(string.IsNullOrEmpty(envelope.ReasonCode)); + Assert.Equal(SkillProgressionSnapshotApi.ReasonSourceKindNotAllowed, envelope.ReasonCode); var bag = await inventory.Content.ReadFromJsonAsync(); Assert.NotNull(bag); diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs index 5e7507b..ee0daad 100644 --- a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs @@ -9,7 +9,7 @@ using Xunit; namespace NeonSprawl.Server.Tests.Game.Interaction; -/// NEO-41 legacy coverage retained for terminal control; gather side effects covered by NEO-63 integration tests. +/// NEO-41 terminal-only regression; gather side effects covered by NEO-63 integration tests. public sealed class InteractionResourceNodeGatherXpTests { [Fact] @@ -43,40 +43,4 @@ public sealed class InteractionResourceNodeGatherXpTests var salvage = body!.Skills!.Single(static s => s.Id == "salvage"); Assert.Equal(0, salvage.Xp); } - - [Fact] - public async Task PostInteract_ResourceNode_WhenInRange_ShouldGrantSalvageActivityXp_ViaProgressionSnapshot() - { - // Arrange — node at (12, -6), radius 3; stand at (10, -6) → horizontal distance 2 - await using var factory = new InMemoryWebApplicationFactory(); - var client = factory.CreateClient(); - var move = new MoveCommandRequest - { - SchemaVersion = MoveCommandRequest.CurrentSchemaVersion, - Target = new PositionVector { X = 10, Y = 0.9, Z = -6 }, - }; - await client.PostAsJsonAsync("/game/players/dev-local-1/move", move); - - // Act - var interact = await client.PostAsJsonAsync( - "/game/players/dev-local-1/interact", - new InteractionRequest - { - SchemaVersion = InteractionRequest.CurrentSchemaVersion, - InteractableId = PrototypeInteractableRegistry.PrototypeResourceNodeAlphaId, - }); - var snapshot = await client.GetAsync("/game/players/dev-local-1/skill-progression"); - - // Assert - Assert.Equal(HttpStatusCode.OK, interact.StatusCode); - var envelope = await interact.Content.ReadFromJsonAsync(); - Assert.NotNull(envelope); - Assert.True(envelope!.Allowed); - - Assert.Equal(HttpStatusCode.OK, snapshot.StatusCode); - var body = await snapshot.Content.ReadFromJsonAsync(); - Assert.NotNull(body); - var salvage = body!.Skills!.Single(static s => s.Id == "salvage"); - Assert.Equal(GatherSkillXpConstants.ActivityXpPerResourceNodeGather, salvage.Xp); - } } diff --git a/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs b/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs index 3975e95..187396b 100644 --- a/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs +++ b/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs @@ -6,7 +6,7 @@ namespace NeonSprawl.Server.Game.Gathering; /// /// Orchestrates gather side effects: capacity pre-check, inventory grant, skill XP, depletion commit (NEO-62). -/// Interact wiring is NEO-63. +/// Wired from on resource_node interact (NEO-63). /// public static class GatherOperations { diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs index f9f781a..2f7651c 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs @@ -2,7 +2,7 @@ using NeonSprawl.Server.Game.Mastery; namespace NeonSprawl.Server.Game.Skills; -/// Shared NEO-38 skill XP grant apply (used by HTTP POST and NEO-41 gather interact hook). +/// Shared NEO-38 skill XP grant apply (HTTP POST, gather engine via NEO-63 interact, and other callers). public static class SkillProgressionGrantOperations { ///