don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
pull/147/head
VinPropane 2026-05-31 18:39:52 -04:00
parent ebb27aed2d
commit 25f3714554
3 changed files with 123 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
docs/reviews/2026-05-31-NEO-108.md
View File

@ -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

114
server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressApiTests.cs
View File

@ -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<EncounterProgressListResponse>();
var secondBody = await secondResponse.Content.ReadFromJsonAsync<EncounterProgressListResponse>();
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<IEncounterCompletionStore>();
var eventStore = factory.Services.GetRequiredService<IEncounterCompleteEventStore>();
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<IEncounterDefinitionRegistry>();
var progressStore = factory.Services.GetRequiredService<IEncounterProgressStore>();
var completionStore = factory.Services.GetRequiredService<IEncounterCompletionStore>();
var eventStore = factory.Services.GetRequiredService<IEncounterCompleteEventStore>();
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<InvalidOperationException>(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<InvalidOperationException>(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<string, EncounterDefRow>(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(

14
server/NeonSprawl.Server/Game/Encounters/EncounterProgressApi.cs
View File

@ -70,13 +70,10 @@ public static class EncounterProgressApi
IEncounterCompletionStore completionStore,
IEncounterCompleteEventStore completeEventStore)
{
var defeatedTargetIds = Array.Empty<string>();
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<string>();
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
};
}
/// <summary>Preserves commit-time grant order from <see cref="EncounterCompleteEvent.GrantedItems"/> (reward-table apply order).</summary>
private static List<EncounterRewardGrantJson> MapGrantSummary(IReadOnlyList<EncounterGrantApplied> grants)
{
var summary = new List<EncounterRewardGrantJson>(grants.Count);