From 9ab3a707961b85fd9bcab39a22285f8afafb442a Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 18:48:45 -0400 Subject: [PATCH] NEO-129: address review test and Bruno coverage gaps. --- ...t progress after gather intro complete.bru | 29 ++++- docs/plans/NEO-129-implementation-plan.md | 2 +- docs/reviews/2026-06-07-NEO-129.md | 13 +- .../Game/Quests/QuestAcceptApiTests.cs | 6 + .../Game/Quests/QuestProgressApiTests.cs | 112 +++++++++++++++--- 5 files changed, 138 insertions(+), 24 deletions(-) diff --git a/bruno/neon-sprawl-server/quest-progress/Get quest progress after gather intro complete.bru b/bruno/neon-sprawl-server/quest-progress/Get quest progress after gather intro complete.bru index de5f823..d7c5cf6 100644 --- a/bruno/neon-sprawl-server/quest-progress/Get quest progress after gather intro complete.bru +++ b/bruno/neon-sprawl-server/quest-progress/Get quest progress after gather intro complete.bru @@ -5,7 +5,7 @@ meta { } docs { - NEO-129: accept gather intro, complete via three alpha gathers, GET quest-progress asserts completionRewardSummary. + NEO-129: accept gather intro, complete via three alpha gathers, GET quest-progress asserts completionRewardSummary; second GET unchanged. } script:pre-request { @@ -42,6 +42,26 @@ script:pre-request { throw new Error(`expected gather interact to succeed, got ${JSON.stringify(interact.data)}`); } } + + const firstGet = await axios.get( + `${baseUrl}/game/players/${playerId}/quest-progress`, + { validateStatus: () => true }, + ); + if (firstGet.status !== 200) { + throw new Error(`expected first GET quest-progress to return 200, got ${firstGet.status}`); + } + const firstRow = firstGet.data.quests.find( + (x) => x.questId === "prototype_quest_gather_intro", + ); + if (!firstRow || firstRow.status !== "completed" || !firstRow.completionRewardSummary) { + throw new Error( + `expected first GET gather intro completed with summary, got ${JSON.stringify(firstRow)}`, + ); + } + bru.setVar( + "firstGatherCompletionRewardSummary", + JSON.stringify(firstRow.completionRewardSummary), + ); } get { @@ -64,6 +84,13 @@ tests { ]); }); + test("second GET completionRewardSummary matches first GET", function () { + const body = res.getBody(); + const row = body.quests.find((x) => x.questId === "prototype_quest_gather_intro"); + const firstSummary = JSON.parse(bru.getVar("firstGatherCompletionRewardSummary")); + expect(row.completionRewardSummary).to.eql(firstSummary); + }); + test("active and not_started rows omit completionRewardSummary", function () { const body = res.getBody(); for (const row of body.quests) { diff --git a/docs/plans/NEO-129-implementation-plan.md b/docs/plans/NEO-129-implementation-plan.md index c4fc5c1..4ef3663 100644 --- a/docs/plans/NEO-129-implementation-plan.md +++ b/docs/plans/NEO-129-implementation-plan.md @@ -57,7 +57,7 @@ - **DTOs:** **`QuestCompletionRewardSummaryJson`** with nested **`itemGrants`** + **`skillXpGrants`**; optional **`completionRewardSummary`** on **`QuestProgressRowJson`**. - **Projection:** **`QuestProgressApi.BuildSnapshot`** / **`MapQuestProgressRow`** read **`IRewardDeliveryStore.TryGet`**; omit summary when no delivery event (kickoff). - **Accept:** **`QuestAcceptApi.MapResponse`** threads **`playerId`** + **`deliveryStore`** into row mapper. -- **Tests:** eight AAA tests in **`QuestProgressApiTests`** — omit on default/active; gather complete salvage **25**; operator chain item + XP; idempotent GET replay. +- **Tests:** eleven AAA tests in **`QuestProgressApiTests`** — omit on default/active; gather complete salvage **25**; operator chain item + XP; GET-refresh chain terminal summary; omit when completed without delivery record; full-summary idempotent GET replay (gather + operator chain). **`QuestAcceptApiTests`** — null summary on active accept; summary on already-completed deny row. - **Bruno:** **`Get quest progress after gather intro complete.bru`**; default GET asserts summary omitted on **`not_started`**. - **Docs:** **`server/README.md`**; E7.M2 module anchor; alignment register; E7M2-06 backlog AC. diff --git a/docs/reviews/2026-06-07-NEO-129.md b/docs/reviews/2026-06-07-NEO-129.md index d86d7cb..e9c9754 100644 --- a/docs/reviews/2026-06-07-NEO-129.md +++ b/docs/reviews/2026-06-07-NEO-129.md @@ -2,7 +2,8 @@ **Date:** 2026-06-07 **Scope:** Branch `NEO-129-quest-progress-completion-reward-summary` vs `main` — commits `2b06eb2` … `78e805e` -**Base:** `main` +**Base:** `main` +**Follow-up:** Review suggestions and nits addressed in a subsequent commit on the same branch. ## Verdict @@ -32,17 +33,17 @@ None. ## Suggestions -1. **GET-refresh chain completion summary** — `GetQuestProgress_ShouldCompleteChainTerminal_WhenTokenHeldAndInventoryRefreshRunsOnGet` asserts **`completed`** via inventory refresh wiring but does not assert **`CompletionRewardSummary`** (unlike gather and operator-chain complete tests). That path exercises deliver-then-mark; adding item + skill XP asserts would lock summary projection on the GET side-effect completion path. +1. ~~**GET-refresh chain completion summary** — `GetQuestProgress_ShouldCompleteChainTerminal_WhenTokenHeldAndInventoryRefreshRunsOnGet` asserts **`completed`** via inventory refresh wiring but does not assert **`CompletionRewardSummary`** (unlike gather and operator-chain complete tests). That path exercises deliver-then-mark; adding item + skill XP asserts would lock summary projection on the GET side-effect completion path.~~ **Done.** `AssertOperatorChainCompletionRewardSummary` on GET-refresh chain terminal test. -2. **Bruno idempotent second GET** — The implementation plan Bruno AC calls for a second GET unchanged; **`Get quest progress after gather intro complete.bru`** performs only one GET. Optional follow-up: duplicate GET in pre-request or a second request file to match plan and integration test coverage. +2. ~~**Bruno idempotent second GET** — The implementation plan Bruno AC calls for a second GET unchanged; **`Get quest progress after gather intro complete.bru`** performs only one GET. Optional follow-up: duplicate GET in pre-request or a second request file to match plan and integration test coverage.~~ **Done.** Pre-request first GET + `bru.setVar`; main GET test asserts `eql` with stored summary. -3. **Completed without delivery record** — Kickoff adopts omit-on-miss (softer than NEO-108 fail-fast). No test forces **`status: completed`** with a store miss (e.g. direct **`IPlayerQuestStateStore`** seed without **`IRewardDeliveryStore`** record). Low priority given NEO-128 deliver-then-mark invariant in normal paths; a single defensive test would document the contract. +3. ~~**Completed without delivery record** — Kickoff adopts omit-on-miss (softer than NEO-108 fail-fast). No test forces **`status: completed`** with a store miss (e.g. direct **`IPlayerQuestStateStore`** seed without **`IRewardDeliveryStore`** record). Low priority given NEO-128 deliver-then-mark invariant in normal paths; a single defensive test would document the contract.~~ **Done.** `GetQuestProgress_ShouldOmitCompletionRewardSummary_WhenCompletedWithoutDeliveryRecord`. ## Nits -- Nit: Idempotent replay test (`GetQuestProgress_ShouldReturnSameCompletionRewardSummary_WhenGatherIntroCompletedTwice`) compares only the first **`skillXpGrants`** line, not full **`completionRewardSummary`** object equality (Bruno uses **`eql`** on arrays). Fine for single-line gather intro; operator-chain idempotent replay is untested. +- ~~Nit: Idempotent replay test (`GetQuestProgress_ShouldReturnSameCompletionRewardSummary_WhenGatherIntroCompletedTwice`) compares only the first **`skillXpGrants`** line, not full **`completionRewardSummary`** object equality (Bruno uses **`eql`** on arrays). Fine for single-line gather intro; operator-chain idempotent replay is untested.~~ **Done.** `AssertCompletionRewardSummariesEqual` helper; added operator-chain idempotent replay test. -- Nit: **`QuestAcceptApiTests`** do not assert **`CompletionRewardSummary`** null on active accept embedded rows. Mapper consistency is covered indirectly via shared **`MapQuestProgressRow`**; a one-line assert on existing accept-success tests would mirror GET omit coverage. +- ~~Nit: **`QuestAcceptApiTests`** do not assert **`CompletionRewardSummary`** null on active accept embedded rows. Mapper consistency is covered indirectly via shared **`MapQuestProgressRow`**; a one-line assert on existing accept-success tests would mirror GET omit coverage.~~ **Done.** Null on active accept; summary on already-completed deny row. - Nit: Grant-order XML is present on **`MapItemGrants`** / **`MapSkillXpGrants`**; C# operator-chain test asserts fields via **`Single`** / index **0** (order-agnostic for single grants). Acceptable for prototype freeze table; Bruno gather test asserts ordered **`skillXpGrants`** array. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs index d31af38..5df5ff9 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs @@ -76,6 +76,7 @@ public sealed class QuestAcceptApiTests Assert.Equal(0, body.Quest.CurrentStepIndex); Assert.Empty(body.Quest.ObjectiveCounters); Assert.Null(body.Quest.CompletedAt); + Assert.Null(body.Quest.CompletionRewardSummary); var getResponse = await client.GetAsync($"/game/players/{PlayerId}/quest-progress"); Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); @@ -261,6 +262,11 @@ public sealed class QuestAcceptApiTests Assert.Equal(QuestProgressApi.StatusCompleted, body.Quest!.Status); Assert.Equal(0, body.Quest.CurrentStepIndex); Assert.NotNull(body.Quest.CompletedAt); + Assert.NotNull(body.Quest.CompletionRewardSummary); + Assert.Empty(body.Quest.CompletionRewardSummary!.ItemGrants); + Assert.Single(body.Quest.CompletionRewardSummary.SkillXpGrants); + Assert.Equal("salvage", body.Quest.CompletionRewardSummary.SkillXpGrants[0].SkillId); + Assert.Equal(25, body.Quest.CompletionRewardSummary.SkillXpGrants[0].Amount); } [Fact] diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestProgressApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestProgressApiTests.cs index bd9cebd..eceb0fd 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestProgressApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestProgressApiTests.cs @@ -137,11 +137,7 @@ public sealed class QuestProgressApiTests Assert.Equal(QuestProgressApi.StatusCompleted, row.Status); Assert.Equal(0, row.CurrentStepIndex); Assert.NotNull(row.CompletedAt); - Assert.NotNull(row.CompletionRewardSummary); - Assert.Empty(row.CompletionRewardSummary!.ItemGrants); - Assert.Single(row.CompletionRewardSummary.SkillXpGrants); - Assert.Equal("salvage", row.CompletionRewardSummary.SkillXpGrants[0].SkillId); - Assert.Equal(25, row.CompletionRewardSummary.SkillXpGrants[0].Amount); + AssertGatherIntroCompletionRewardSummary(row.CompletionRewardSummary); } [Fact] @@ -170,10 +166,58 @@ public sealed class QuestProgressApiTests Assert.NotNull(secondBody); var firstRow = Assert.Single(firstBody!.Quests, static r => r.QuestId == GatherQuestId); var secondRow = Assert.Single(secondBody!.Quests, static r => r.QuestId == GatherQuestId); - Assert.NotNull(firstRow.CompletionRewardSummary); - Assert.NotNull(secondRow.CompletionRewardSummary); - Assert.Equal(firstRow.CompletionRewardSummary!.SkillXpGrants[0].SkillId, secondRow.CompletionRewardSummary!.SkillXpGrants[0].SkillId); - Assert.Equal(firstRow.CompletionRewardSummary.SkillXpGrants[0].Amount, secondRow.CompletionRewardSummary.SkillXpGrants[0].Amount); + AssertGatherIntroCompletionRewardSummary(firstRow.CompletionRewardSummary); + AssertCompletionRewardSummariesEqual(firstRow.CompletionRewardSummary, secondRow.CompletionRewardSummary); + } + + [Fact] + public async Task GetQuestProgress_ShouldReturnSameCompletionRewardSummary_WhenOperatorChainCompletedTwice() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var client = factory.CreateClient(); + CompleteOperatorChain(deps); + + // Act + var firstResponse = await client.GetAsync($"/game/players/{PlayerId}/quest-progress"); + var secondResponse = await client.GetAsync($"/game/players/{PlayerId}/quest-progress"); + + // Assert + Assert.Equal(HttpStatusCode.OK, firstResponse.StatusCode); + Assert.Equal(HttpStatusCode.OK, secondResponse.StatusCode); + var firstBody = await firstResponse.Content.ReadFromJsonAsync(); + var secondBody = await secondResponse.Content.ReadFromJsonAsync(); + Assert.NotNull(firstBody); + Assert.NotNull(secondBody); + var firstRow = Assert.Single(firstBody!.Quests, static r => r.QuestId == ChainQuestId); + var secondRow = Assert.Single(secondBody!.Quests, static r => r.QuestId == ChainQuestId); + AssertOperatorChainCompletionRewardSummary(firstRow.CompletionRewardSummary); + AssertCompletionRewardSummariesEqual(firstRow.CompletionRewardSummary, secondRow.CompletionRewardSummary); + } + + [Fact] + public async Task GetQuestProgress_ShouldOmitCompletionRewardSummary_WhenCompletedWithoutDeliveryRecord() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var client = factory.CreateClient(); + Assert.True(TryAccept(deps, GatherQuestId).Success); + Assert.True(deps.ProgressStore.TryMarkComplete(PlayerId, GatherQuestId, DateTimeOffset.UtcNow, out _)); + Assert.False(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out _)); + + // Act + var response = await client.GetAsync($"/game/players/{PlayerId}/quest-progress"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + var row = Assert.Single(body!.Quests, static r => r.QuestId == GatherQuestId); + Assert.Equal(QuestProgressApi.StatusCompleted, row.Status); + Assert.NotNull(row.CompletedAt); + Assert.Null(row.CompletionRewardSummary); } [Fact] @@ -196,13 +240,7 @@ public sealed class QuestProgressApiTests Assert.Equal(QuestProgressApi.StatusCompleted, row.Status); Assert.Equal(3, row.CurrentStepIndex); Assert.NotNull(row.CompletedAt); - Assert.NotNull(row.CompletionRewardSummary); - Assert.Single(row.CompletionRewardSummary!.ItemGrants); - Assert.Equal("survey_drone_kit", row.CompletionRewardSummary.ItemGrants[0].ItemId); - Assert.Equal(1, row.CompletionRewardSummary.ItemGrants[0].Quantity); - Assert.Single(row.CompletionRewardSummary.SkillXpGrants); - Assert.Equal("salvage", row.CompletionRewardSummary.SkillXpGrants[0].SkillId); - Assert.Equal(50, row.CompletionRewardSummary.SkillXpGrants[0].Amount); + AssertOperatorChainCompletionRewardSummary(row.CompletionRewardSummary); } [Fact] @@ -238,12 +276,54 @@ public sealed class QuestProgressApiTests Assert.Equal(QuestProgressApi.StatusCompleted, row.Status); Assert.Equal(3, row.CurrentStepIndex); Assert.NotNull(row.CompletedAt); + AssertOperatorChainCompletionRewardSummary(row.CompletionRewardSummary); Assert.True(deps.ProgressStore.TryGetProgress(PlayerId, ChainQuestId, out var afterGet)); Assert.Equal(QuestProgressStatus.Completed, afterGet.Status); Assert.Equal(3, afterGet.CurrentStepIndex); Assert.NotNull(afterGet.CompletedAt); } + private static void AssertGatherIntroCompletionRewardSummary(QuestCompletionRewardSummaryJson? summary) + { + Assert.NotNull(summary); + Assert.Empty(summary!.ItemGrants); + Assert.Single(summary.SkillXpGrants); + Assert.Equal("salvage", summary.SkillXpGrants[0].SkillId); + Assert.Equal(25, summary.SkillXpGrants[0].Amount); + } + + private static void AssertOperatorChainCompletionRewardSummary(QuestCompletionRewardSummaryJson? summary) + { + Assert.NotNull(summary); + Assert.Single(summary!.ItemGrants); + Assert.Equal("survey_drone_kit", summary.ItemGrants[0].ItemId); + Assert.Equal(1, summary.ItemGrants[0].Quantity); + Assert.Single(summary.SkillXpGrants); + Assert.Equal("salvage", summary.SkillXpGrants[0].SkillId); + Assert.Equal(50, summary.SkillXpGrants[0].Amount); + } + + private static void AssertCompletionRewardSummariesEqual( + QuestCompletionRewardSummaryJson? first, + QuestCompletionRewardSummaryJson? second) + { + Assert.NotNull(first); + Assert.NotNull(second); + Assert.Equal(first!.ItemGrants.Count, second!.ItemGrants.Count); + for (var i = 0; i < first.ItemGrants.Count; i++) + { + Assert.Equal(first.ItemGrants[i].ItemId, second.ItemGrants[i].ItemId); + Assert.Equal(first.ItemGrants[i].Quantity, second.ItemGrants[i].Quantity); + } + + Assert.Equal(first.SkillXpGrants.Count, second.SkillXpGrants.Count); + for (var i = 0; i < first.SkillXpGrants.Count; i++) + { + Assert.Equal(first.SkillXpGrants[i].SkillId, second.SkillXpGrants[i].SkillId); + Assert.Equal(first.SkillXpGrants[i].Amount, second.SkillXpGrants[i].Amount); + } + } + private static void CompleteOperatorChain(QuestWiringTestDependencies deps) { AcceptAndMarkComplete(deps, GatherQuestId);