From ea3b7cceb581b9c893549b78e96f4a48714f7f98 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 12:21:19 -0400 Subject: [PATCH] NEO-120: Address code review test coverage gaps. Add HTTP tests for already_completed deny snapshot and empty {} body; tighten already_active AAA arrange. Strike through resolved review items. --- docs/plans/NEO-120-implementation-plan.md | 4 +- docs/reviews/2026-06-07-NEO-120.md | 8 +-- .../Game/Quests/QuestAcceptApiTests.cs | 72 +++++++++++++++++-- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/docs/plans/NEO-120-implementation-plan.md b/docs/plans/NEO-120-implementation-plan.md index 866ad54..248f2a6 100644 --- a/docs/plans/NEO-120-implementation-plan.md +++ b/docs/plans/NEO-120-implementation-plan.md @@ -57,7 +57,7 @@ - **Route:** `POST /game/players/{id}/quests/{questId}/accept` — `QuestAcceptApi.MapResponse`; 404 via `IPositionStateStore`; optional body v1. - **DTOs:** `QuestAcceptRequest`, `QuestAcceptResponse` (`accepted`, `reasonCode`, `quest` row). - **Shared mapper:** `QuestProgressApi.MapQuestProgressRow` overloads for GET + accept POST. -- **Tests:** eight AAA cases in `QuestAcceptApiTests`. +- **Tests:** ten AAA cases in `QuestAcceptApiTests` (incl. `already_completed` deny snapshot, empty `{}` body). - **Bruno:** accept gather intro, refine prerequisite deny, duplicate accept. - **Docs:** `server/README.md`; E7.M1 module snapshot + backlog updated. @@ -157,7 +157,7 @@ flowchart TD | Test file | What it covers | |-----------|----------------| -| `QuestAcceptApiTests.cs` | **404:** unknown player + whitespace id. **Happy accept:** POST gather intro → `accepted: true`, `quest.status` active, step 0, empty counters; GET confirms active. **Prerequisite deny:** POST refine intro without gather complete → `prerequisite_incomplete`, GET still `not_started` for refine. **Duplicate accept:** second POST → `already_active` with `quest` snapshot, no counter drift. **Unknown quest:** bogus quest id → `unknown_quest`. **Optional body:** POST with no body succeeds for happy accept. | +| `QuestAcceptApiTests.cs` | **404:** unknown player + whitespace id. **Happy accept:** POST gather intro → `accepted: true`, `quest.status` active, step 0, empty counters; GET confirms active. **Prerequisite deny:** POST refine intro without gather complete → `prerequisite_incomplete`, GET still `not_started` for refine. **Duplicate accept:** second POST → `already_active` with `quest` snapshot. **Completed deny:** accept + complete gather intro → re-accept → `already_completed` with `quest.status` completed + `completedAt`. **Unknown quest:** bogus quest id → `unknown_quest`. **Optional body:** POST with no body or `{}` succeeds for happy accept. | | Existing quest test suite | Regression smoke — operations + GET unchanged when accept not called. | ## Open questions / risks diff --git a/docs/reviews/2026-06-07-NEO-120.md b/docs/reviews/2026-06-07-NEO-120.md index d79dbfa..46a9e9e 100644 --- a/docs/reviews/2026-06-07-NEO-120.md +++ b/docs/reviews/2026-06-07-NEO-120.md @@ -32,15 +32,15 @@ NEO-120 adds **`POST /game/players/{id}/quests/{questId}/accept`** — a thin HT ## Suggestions -1. **HTTP test for `already_completed` deny with snapshot** — The plan response table and README document **`already_completed`** returning **`accepted: false`** plus a **`quest`** row. Integration tests cover **`already_active`** and operations-layer **`TryAccept_ShouldDenyAlreadyCompleted`** exists in `QuestStateOperationsTests`, but no HTTP test exercises **`MapResponse`** for a completed row (status **`completed`**, optional **`completedAt`**). A single test completing gather intro then re-accepting would lock the wire contract for NEO-122. +1. ~~**HTTP test for `already_completed` deny with snapshot** — The plan response table and README document **`already_completed`** returning **`accepted: false`** plus a **`quest`** row. Integration tests cover **`already_active`** and operations-layer **`TryAccept_ShouldDenyAlreadyCompleted`** exists in `QuestStateOperationsTests`, but no HTTP test exercises **`MapResponse`** for a completed row (status **`completed`**, optional **`completedAt`**). A single test completing gather intro then re-accepting would lock the wire contract for NEO-122.~~ **Done.** `PostQuestAccept_ShouldDenyAlreadyCompleted_WhenQuestAlreadyCompleted` in `QuestAcceptApiTests.cs`. -2. **Empty `{}` body test** — README documents `{}` as valid (default **`schemaVersion` 0**). Tests cover omitted body and explicit **`schemaVersion: 1`**; a one-liner POST with **`StringContent("{}")`** would close the loop on the documented third valid shape. +2. ~~**Empty `{}` body test** — README documents `{}` as valid (default **`schemaVersion` 0**). Tests cover omitted body and explicit **`schemaVersion: 1`**; a one-liner POST with **`StringContent("{}")`** would close the loop on the documented third valid shape.~~ **Done.** `PostQuestAccept_ShouldReturnAcceptedTrue_WhenRequestBodyIsEmptyObject` in `QuestAcceptApiTests.cs`. ## Nits - Nit: **`MapQuestProgressRow(QuestStepState)`** throws **`InvalidOperationException`** on unknown **`QuestProgressStatus`** (would surface as 500). Inherited defensive guard from NEO-119; acceptable for prototype. -- Nit: **`PostQuestAccept_ShouldDenyAlreadyActive_WhenQuestAlreadyAccepted`** asserts on the first accept response inside **Arrange** (lines 159–162). Works fine; optional move of **`firstBody`** checks into **Assert** if strict AAA phase purity is desired. +- ~~Nit: **`PostQuestAccept_ShouldDenyAlreadyActive_WhenQuestAlreadyAccepted`** asserts on the first accept response inside **Arrange** (lines 159–162). Works fine; optional move of **`firstBody`** checks into **Assert** if strict AAA phase purity is desired.~~ **Done.** Arrange seeds active row without assertions; assert uses step index **0**. - Nit: Bruno **`Accept duplicate.bru`** pre-request re-accepts gather intro (idempotent when seq 3 ran). Correct for standalone runs; collection order matters only when hitting a persistent dev server without reset. @@ -62,6 +62,6 @@ dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj **Bruno:** run the three accept requests in `bruno/neon-sprawl-server/quest-progress/` (seq 3–5) against a local server with fresh `dev-local-1`. -**Reviewer note:** All 8 `QuestAcceptApiTests` and 93 quest-namespace tests passed locally during review. +**Reviewer note:** All 8 `QuestAcceptApiTests` and 93 quest-namespace tests passed locally during review. **Follow-up:** suggestions 1–2 and nit on `already_active` AAA addressed — 10 `QuestAcceptApiTests` pass. **Manual:** None required beyond Bruno for this server-only story; Godot accept binding verification belongs to NEO-122. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs index 29c3a5f..c595ccc 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs @@ -2,6 +2,8 @@ using System.Net; using System.Net.Http.Json; using System.Text; using System.Text.Json; +using Microsoft.Extensions.DependencyInjection; +using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Quests; using NeonSprawl.Server.Tests; using Xunit; @@ -100,6 +102,27 @@ public sealed class QuestAcceptApiTests Assert.Equal(GatherQuestId, body.Quest!.QuestId); } + [Fact] + public async Task PostQuestAccept_ShouldReturnAcceptedTrue_WhenRequestBodyIsEmptyObject() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + using var content = new StringContent("{}", Encoding.UTF8, "application/json"); + + // Act + var response = await client.PostAsync( + $"/game/players/{PlayerId}/quests/{GatherQuestId}/accept", + content); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.True(body!.Accepted); + Assert.Equal(GatherQuestId, body.Quest!.QuestId); + } + [Fact] public async Task PostQuestAccept_ShouldReturnBadRequest_WhenSchemaVersionInvalid() { @@ -153,13 +176,9 @@ public sealed class QuestAcceptApiTests // Arrange await using var factory = new InMemoryWebApplicationFactory(); var client = factory.CreateClient(); - var first = await client.PostAsync( + _ = await client.PostAsync( $"/game/players/{PlayerId}/quests/{GatherQuestId}/accept", JsonContent.Create(new QuestAcceptRequest { SchemaVersion = QuestAcceptRequest.CurrentSchemaVersion })); - Assert.Equal(HttpStatusCode.OK, first.StatusCode); - var firstBody = await first.Content.ReadFromJsonAsync(); - Assert.NotNull(firstBody); - Assert.True(firstBody!.Accepted); // Act var response = await client.PostAsync( @@ -174,7 +193,48 @@ public sealed class QuestAcceptApiTests Assert.Equal(QuestStateReasonCodes.AlreadyActive, body.ReasonCode); Assert.NotNull(body.Quest); Assert.Equal(QuestProgressApi.StatusActive, body.Quest!.Status); - Assert.Equal(firstBody.Quest!.CurrentStepIndex, body.Quest.CurrentStepIndex); + Assert.Equal(0, body.Quest.CurrentStepIndex); + } + + [Fact] + public async Task PostQuestAccept_ShouldDenyAlreadyCompleted_WhenQuestAlreadyCompleted() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + var questRegistry = factory.Services.GetRequiredService(); + var progressStore = factory.Services.GetRequiredService(); + var timeProvider = TimeProvider.System; + Assert.True(QuestStateOperations.TryAccept( + PlayerId, + GatherQuestId, + questRegistry, + progressStore, + factory.Services.GetRequiredService(), + factory.Services.GetRequiredService(), + timeProvider).Success); + Assert.True(QuestStateOperations.TryMarkComplete( + PlayerId, + GatherQuestId, + questRegistry, + progressStore, + timeProvider).Success); + + // Act + var response = await client.PostAsync( + $"/game/players/{PlayerId}/quests/{GatherQuestId}/accept", + JsonContent.Create(new QuestAcceptRequest { SchemaVersion = QuestAcceptRequest.CurrentSchemaVersion })); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.False(body!.Accepted); + Assert.Equal(QuestStateReasonCodes.AlreadyCompleted, body.ReasonCode); + Assert.NotNull(body.Quest); + Assert.Equal(QuestProgressApi.StatusCompleted, body.Quest!.Status); + Assert.Equal(0, body.Quest.CurrentStepIndex); + Assert.NotNull(body.Quest.CompletedAt); } [Fact]