From 25f3714554fa5183d1cf2224fc5933a9ac413b23 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 18:39:52 -0400 Subject: [PATCH] NEO-108: Address code review findings for encounter-progress GET. Add BuildSnapshot invariant tests, whitespace 404, idempotent second GET, grant-order assertions; dedupe MapRow progress lookup; strike review items. --- docs/reviews/2026-05-31-NEO-108.md | 10 +- .../Encounters/EncounterProgressApiTests.cs | 114 +++++++++++++++++- .../Game/Encounters/EncounterProgressApi.cs | 14 +-- 3 files changed, 123 insertions(+), 15 deletions(-) diff --git a/docs/reviews/2026-05-31-NEO-108.md b/docs/reviews/2026-05-31-NEO-108.md index 8e26cfa..ee3c6f2 100644 --- a/docs/reviews/2026-05-31-NEO-108.md +++ b/docs/reviews/2026-05-31-NEO-108.md @@ -30,19 +30,19 @@ None. ## Suggestions -1. **Invariant violation tests for `BuildSnapshot`** — Plan adopts fail-fast **`InvalidOperationException`** when **`completed`** but **`completedAt`** or **`EncounterCompleteEvent`** is missing. No direct unit test exercises these paths (would require a small fake/mocked store pair). A focused **`BuildSnapshot`** test would lock the contract without a full defeat spine. +1. ~~**Invariant violation tests for `BuildSnapshot`** — Plan adopts fail-fast **`InvalidOperationException`** when **`completed`** but **`completedAt`** or **`EncounterCompleteEvent`** is missing. No direct unit test exercises these paths (would require a small fake/mocked store pair). A focused **`BuildSnapshot`** test would lock the contract without a full defeat spine.~~ **Done.** `BuildSnapshot_ShouldThrow_WhenCompletedButCompleteEventMissing` + `BuildSnapshot_ShouldThrow_WhenCompletedButCompletedAtMissing` (`CompletedWithoutTimestampStore` fake). -2. **Idempotent second GET in C# tests** — Bruno **`Get encounter progress after defeat spine.bru`** asserts a second GET stays **`completed`** with unchanged grant summary; the C# suite stops after one GET on the completed case. A second **`GetAsync`** in the completed test (or a dedicated test) would mirror Bruno at the integration layer. +2. ~~**Idempotent second GET in C# tests** — Bruno **`Get encounter progress after defeat spine.bru`** asserts a second GET stays **`completed`** with unchanged grant summary; the C# suite stops after one GET on the completed case. A second **`GetAsync`** in the completed test (or a dedicated test) would mirror Bruno at the integration layer.~~ **Done.** Second **`GetAsync`** in completed test asserts stable state, **`completedAt`**, and grant summary. -3. **Whitespace / empty player id 404** — Route trims **`id`** and returns 404 when empty; only **`missing-player`** is tested. A **`/game/players/%20/encounter-progress`** (or `""` after trim) case would match the documented gate explicitly (same gap exists on gig progression — low priority). +3. ~~**Whitespace / empty player id 404** — Route trims **`id`** and returns 404 when empty; only **`missing-player`** is tested. A **`/game/players/%20/encounter-progress`** (or `""` after trim) case would match the documented gate explicitly (same gap exists on gig progression — low priority).~~ **Done.** `GetEncounterProgress_ShouldReturnNotFound_WhenPlayerIdIsWhitespaceOnly`. ## Nits -- Nit: **`MapRow`** calls **`progressStore.TryGetProgress`** twice on the **`active`** path (once for **`defeatedTargetIds`**, again for **`Started`**). A single **`TryGet`** with reused **`progress`** variable would avoid the duplicate lookup (prototype catalog size = 1; cosmetic only). +- ~~Nit: **`MapRow`** calls **`progressStore.TryGetProgress`** twice on the **`active`** path (once for **`defeatedTargetIds`**, again for **`Started`**). A single **`TryGet`** with reused **`progress`** variable would avoid the duplicate lookup (prototype catalog size = 1; cosmetic only).~~ **Done.** - Nit: **`EncounterProgressApiTests`** defeat helper binds hotbar **slot 0**; Bruno spine uses **slot 3** per plan (cooldown isolation). Both are fine in isolation; no change required unless slot 0 conflicts emerge in combined Bruno runs. -- Nit: **`MapGrantSummary`** preserves reward-table grant order (scrap then token). C# completed test asserts quantities via **`Single(g => g.ItemId == …)`** (order-agnostic); Bruno asserts array order. Document or test order explicitly if clients will diff arrays. +- ~~Nit: **`MapGrantSummary`** preserves reward-table grant order (scrap then token). C# completed test asserts quantities via **`Single(g => g.ItemId == …)`** (order-agnostic); Bruno asserts array order. Document or test order explicitly if clients will diff arrays.~~ **Done.** XML on **`MapGrantSummary`**; completed test asserts full grant array order (matches Bruno). ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressApiTests.cs index f68851c..d7dd7a4 100644 --- a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressApiTests.cs @@ -29,6 +29,20 @@ public sealed class EncounterProgressApiTests Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } + [Fact] + public async Task GetEncounterProgress_ShouldReturnNotFound_WhenPlayerIdIsWhitespaceOnly() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.GetAsync("/game/players/%20/encounter-progress"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + [Fact] public async Task GetEncounterProgress_ShouldReturnInactive_WhenNoEngagement() { @@ -138,13 +152,19 @@ public sealed class EncounterProgressApiTests factory, PrototypeNpcRegistry.PrototypeNpcEliteId); var response = await client.GetAsync("/game/players/dev-local-1/encounter-progress"); + var secondResponse = await client.GetAsync("/game/players/dev-local-1/encounter-progress"); // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(HttpStatusCode.OK, secondResponse.StatusCode); var body = await response.Content.ReadFromJsonAsync(); + var secondBody = await secondResponse.Content.ReadFromJsonAsync(); Assert.NotNull(body); + Assert.NotNull(secondBody); var row = Assert.Single(body!.Encounters); + var secondRow = Assert.Single(secondBody!.Encounters); Assert.Equal(EncounterProgressApi.StateCompleted, row.State); + Assert.Equal(EncounterProgressApi.StateCompleted, secondRow.State); Assert.Equal( [ PrototypeNpcRegistry.PrototypeNpcEliteId, @@ -152,16 +172,106 @@ public sealed class EncounterProgressApiTests PrototypeNpcRegistry.PrototypeNpcRangedId, ], row.DefeatedTargetIds); + Assert.Equal(row.DefeatedTargetIds, secondRow.DefeatedTargetIds); Assert.NotNull(row.CompletedAt); Assert.NotNull(row.RewardGrantSummary); - Assert.Equal(10, row.RewardGrantSummary!.Single(g => g.ItemId == "scrap_metal_bulk").Quantity); - Assert.Equal(1, row.RewardGrantSummary.Single(g => g.ItemId == "contract_handoff_token").Quantity); + Assert.Equal(row.CompletedAt, secondRow.CompletedAt); + Assert.Equal(2, row.RewardGrantSummary!.Count); + Assert.Equal("scrap_metal_bulk", row.RewardGrantSummary[0].ItemId); + Assert.Equal(10, row.RewardGrantSummary[0].Quantity); + Assert.Equal("contract_handoff_token", row.RewardGrantSummary[1].ItemId); + Assert.Equal(1, row.RewardGrantSummary[1].Quantity); + Assert.Equal(row.RewardGrantSummary[0].ItemId, secondRow.RewardGrantSummary![0].ItemId); + Assert.Equal(row.RewardGrantSummary[1].ItemId, secondRow.RewardGrantSummary[1].ItemId); var completionStore = factory.Services.GetRequiredService(); var eventStore = factory.Services.GetRequiredService(); Assert.True(completionStore.IsCompleted("dev-local-1", PrototypeEncounterId)); Assert.True(eventStore.TryGet("dev-local-1", PrototypeEncounterId, out _)); } + [Fact] + public async Task BuildSnapshot_ShouldThrow_WhenCompletedButCompleteEventMissing() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var progressStore = factory.Services.GetRequiredService(); + var completionStore = factory.Services.GetRequiredService(); + var eventStore = factory.Services.GetRequiredService(); + Assert.True( + completionStore.TryMarkCompleted( + "dev-local-1", + PrototypeEncounterId, + DateTimeOffset.UtcNow)); + + // Act + var ex = Record.Exception( + () => EncounterProgressApi.BuildSnapshot( + "dev-local-1", + registry, + progressStore, + completionStore, + eventStore)); + + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("EncounterCompleteEvent is missing", ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void BuildSnapshot_ShouldThrow_WhenCompletedButCompletedAtMissing() + { + // Arrange + var registry = CreatePrototypeEncounterRegistry(); + var progressStore = new InMemoryEncounterProgressStore(); + var completionStore = new CompletedWithoutTimestampStore(); + var eventStore = new InMemoryEncounterCompleteEventStore(); + + // Act + var ex = Record.Exception( + () => EncounterProgressApi.BuildSnapshot( + "dev-local-1", + registry, + progressStore, + completionStore, + eventStore)); + + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("completedAt is missing", ioe.Message, StringComparison.Ordinal); + } + + private static EncounterDefinitionRegistry CreatePrototypeEncounterRegistry() + { + var row = new EncounterDefRow( + PrototypeEncounterId, + "Prototype Combat Pocket", + "defeat_all_targets", + ["prototype_npc_melee", "prototype_npc_ranged", "prototype_npc_elite"], + "prototype_combat_pocket_clear"); + var catalog = new EncounterDefinitionCatalog( + "/tmp/encounters", + new Dictionary(StringComparer.Ordinal) { [PrototypeEncounterId] = row }, + catalogJsonFileCount: 1); + return new EncounterDefinitionRegistry(catalog); + } + + private sealed class CompletedWithoutTimestampStore : IEncounterCompletionStore + { + public bool IsCompleted(string playerId, string encounterId) => + string.Equals(playerId, "dev-local-1", StringComparison.Ordinal) + && string.Equals(encounterId, PrototypeEncounterId, StringComparison.Ordinal); + + public bool TryMarkCompleted(string playerId, string encounterId, DateTimeOffset completedAt) => + throw new NotSupportedException(); + + public bool TryGetCompletedAt(string playerId, string encounterId, out DateTimeOffset completedAt) + { + completedAt = default; + return false; + } + } + private static async Task BindSlot0PulseAsync(HttpClient client) { var post = await client.PostAsJsonAsync( diff --git a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressApi.cs b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressApi.cs index 47d62a6..099a05c 100644 --- a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressApi.cs +++ b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressApi.cs @@ -70,13 +70,10 @@ public static class EncounterProgressApi IEncounterCompletionStore completionStore, IEncounterCompleteEventStore completeEventStore) { - var defeatedTargetIds = Array.Empty(); - if (progressStore.TryGetProgress(playerId, encounterId, out var progress)) - { - defeatedTargetIds = progress.DefeatedNpcInstanceIds - .OrderBy(static id => id, StringComparer.Ordinal) - .ToArray(); - } + var hasProgress = progressStore.TryGetProgress(playerId, encounterId, out var progress); + var defeatedTargetIds = hasProgress + ? progress.DefeatedNpcInstanceIds.OrderBy(static id => id, StringComparer.Ordinal).ToArray() + : Array.Empty(); if (completionStore.IsCompleted(playerId, encounterId)) { @@ -102,7 +99,7 @@ public static class EncounterProgressApi }; } - if (progressStore.TryGetProgress(playerId, encounterId, out progress) && progress.Started) + if (hasProgress && progress.Started) { return new EncounterProgressRowJson { @@ -120,6 +117,7 @@ public static class EncounterProgressApi }; } + /// Preserves commit-time grant order from (reward-table apply order). private static List MapGrantSummary(IReadOnlyList grants) { var summary = new List(grants.Count);