NEO-54: address code review suggestions for inventory tests and docs
parent
362608ed5f
commit
ac3900c194
|
|
@ -48,7 +48,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
| E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned |
|
||||
| E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned |
|
||||
|
||||
**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), and **NEO-53** (`GET /game/world/item-definitions`) moved the register row to **In Progress**; later slices update the alignment table as they land.
|
||||
**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), and **NEO-54** (inventory store + stack/slot rules engine) moved the register row to **In Progress**; later slices update the alignment table as they land.
|
||||
|
||||
### Epic 4 — World Topology
|
||||
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ NEO-54 delivers the planned split between **`IPlayerInventoryStore`** (in-memory
|
|||
| [`docs/plans/NEO-54-implementation-plan.md`](../plans/NEO-54-implementation-plan.md) | **Matches** — 24 bag + 1 equipment, split store/engine, reason codes, `V005`, Postgres/in-memory parity tests, DI via catalog extensions, docs/README updates; acceptance checklist complete. |
|
||||
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — NEO-54 bullet under **Related implementation slices**; frozen roster/stack limits align with tests. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row notes NEO-54 landed; NEO-55 HTTP still planned. |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 **In Progress**; footnote still lists NEO-50–NEO-53 only (see suggestion). |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M3 **In Progress**; footnote lists NEO-50–NEO-54. |
|
||||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative inventory mutations; no client-trusted placement in this slice. |
|
||||
| [`server/README.md`](../../server/README.md) | **Matches** — Player inventory store section: containers, reason codes, persistence note, NEO-55 deferral. |
|
||||
|
||||
|
|
@ -31,17 +31,17 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **AAA in Postgres deny test** — In `PlayerInventoryPersistenceIntegrationTests.TryAddStack_WhenDenied_ShouldNotPersistPartialState`, move `Assert.Equal(PlayerInventorySnapshot.BagSlotCount, OccupiedBagSlotCount(afterDeny!))` from the first **Act** block into **Assert** (or a dedicated assert phase after both act steps), per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).
|
||||
1. ~~**AAA in Postgres deny test** — In `PlayerInventoryPersistenceIntegrationTests.TryAddStack_WhenDenied_ShouldNotPersistPartialState`, move `Assert.Equal(PlayerInventorySnapshot.BagSlotCount, OccupiedBagSlotCount(afterDeny!))` from the first **Act** block into **Assert** (or a dedicated assert phase after both act steps), per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).~~ **Done.** — removed in-Act assertion; slot count verified only in **Assert** via fresh factory read-back.
|
||||
|
||||
2. **`StoreMissing` coverage (optional)** — `PlayerInventoryMutationKind.StoreMissing` is documented on `PlayerInventoryMutationOutcome` but untested. A small operations test for an unknown `playerId` (in-memory store without that bucket) would lock the contract before NEO-55 HTTP maps it.
|
||||
2. ~~**`StoreMissing` coverage (optional)** — `PlayerInventoryMutationKind.StoreMissing` is documented on `PlayerInventoryMutationOutcome` but untested. A small operations test for an unknown `playerId` (in-memory store without that bucket) would lock the contract before NEO-55 HTTP maps it.~~ **Done.** — `TryAddStack_ForUnknownPlayer_ShouldReturnStoreMissing` in `PlayerInventoryOperationsTests.cs`.
|
||||
|
||||
3. **Multi-slot add (optional)** — Add a test that places quantity **> `stackMax`** across **multiple new** empty slots (e.g. 41 × `field_stim_mk0` on empty bag → three occupied slots). Merge and bag-full paths are covered; this would explicitly guard the `FillEmptySlots` loop beyond single-slot adds.
|
||||
3. ~~**Multi-slot add (optional)** — Add a test that places quantity **> `stackMax`** across **multiple new** empty slots (e.g. 41 × `field_stim_mk0` on empty bag → three occupied slots). Merge and bag-full paths are covered; this would explicitly guard the `FillEmptySlots` loop beyond single-slot adds.~~ **Done.** — `TryAddStack_WhenQuantityExceedsStackMax_ShouldSpanMultipleEmptySlots` asserts 20+20+1 across three slots.
|
||||
|
||||
4. **Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to mention **NEO-54** alongside NEO-50–NEO-53 (same pattern as NEO-52/NEO-53 reviews).
|
||||
4. ~~**Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to mention **NEO-54** alongside NEO-50–NEO-53 (same pattern as NEO-52/NEO-53 reviews).~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`PlayerInventoryMutationOutcome.cs`** — enum and record parameters use **2-space** indent; neighboring outcome types (e.g. `SkillProgressionGrantApplyOutcome.cs`) use **4-space**. Format-only tidy.
|
||||
- ~~Nit: **`PlayerInventoryMutationOutcome.cs`** — enum and record parameters use **2-space** indent; neighboring outcome types (e.g. `SkillProgressionGrantApplyOutcome.cs`) use **4-space**. Format-only tidy.~~ **Done.**
|
||||
|
||||
- Nit: **`PostgresPlayerInventoryStore.TryReplaceSnapshot`** — delete-all + insert per mutation is correct for snapshot replace; concurrent writers are last-write-wins (same class of risk as other NEO-29-style stores). Fine for prototype; NEO-55 HTTP may want documenting or serializing per player if load tests show races.
|
||||
|
||||
|
|
|
|||
|
|
@ -224,6 +224,53 @@ public sealed class PlayerInventoryOperationsTests
|
|||
Assert.Equal(TotalBagQuantity(before!), TotalBagQuantity(after!));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryAddStack_WhenQuantityExceedsStackMax_ShouldSpanMultipleEmptySlots()
|
||||
{
|
||||
// Arrange
|
||||
using var factory = new InMemoryWebApplicationFactory();
|
||||
var registry = factory.Services.GetRequiredService<IItemDefinitionRegistry>();
|
||||
var store = factory.Services.GetRequiredService<IPlayerInventoryStore>();
|
||||
|
||||
// Act
|
||||
var outcome = PlayerInventoryOperations.TryAddStack(
|
||||
"dev-local-1",
|
||||
"field_stim_mk0",
|
||||
quantity: 41,
|
||||
registry,
|
||||
store);
|
||||
|
||||
// Assert
|
||||
Assert.Equal(PlayerInventoryMutationKind.Applied, outcome.Kind);
|
||||
Assert.Equal(41, CountItem(outcome.Snapshot!, "field_stim_mk0"));
|
||||
Assert.Equal(3, OccupiedBagSlotCount(outcome.Snapshot!));
|
||||
Assert.Equal(20, outcome.Snapshot!.BagSlots[0].Quantity);
|
||||
Assert.Equal(20, outcome.Snapshot.BagSlots[1].Quantity);
|
||||
Assert.Equal(1, outcome.Snapshot.BagSlots[2].Quantity);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryAddStack_ForUnknownPlayer_ShouldReturnStoreMissing()
|
||||
{
|
||||
// Arrange
|
||||
using var factory = new InMemoryWebApplicationFactory();
|
||||
var registry = factory.Services.GetRequiredService<IItemDefinitionRegistry>();
|
||||
var store = factory.Services.GetRequiredService<IPlayerInventoryStore>();
|
||||
|
||||
// Act
|
||||
var outcome = PlayerInventoryOperations.TryAddStack(
|
||||
"unknown-player-xyz",
|
||||
"scrap_metal_bulk",
|
||||
quantity: 1,
|
||||
registry,
|
||||
store);
|
||||
|
||||
// Assert
|
||||
Assert.Equal(PlayerInventoryMutationKind.StoreMissing, outcome.Kind);
|
||||
Assert.Null(outcome.ReasonCode);
|
||||
Assert.Null(outcome.Snapshot);
|
||||
}
|
||||
|
||||
private static int CountItem(PlayerInventorySnapshot snapshot, string itemId)
|
||||
{
|
||||
var total = 0;
|
||||
|
|
|
|||
|
|
@ -79,8 +79,6 @@ public sealed class PlayerInventoryPersistenceIntegrationTests(PostgresIntegrati
|
|||
quantity: 1,
|
||||
registry,
|
||||
store);
|
||||
store.TryGetSnapshot("dev-local-1", out var afterDeny);
|
||||
Assert.Equal(PlayerInventorySnapshot.BagSlotCount, OccupiedBagSlotCount(afterDeny!));
|
||||
}
|
||||
|
||||
// Act — verify on fresh host
|
||||
|
|
|
|||
|
|
@ -3,19 +3,19 @@ namespace NeonSprawl.Server.Game.Items;
|
|||
/// <summary>Result of <see cref="PlayerInventoryOperations.TryAddStack"/> or <see cref="PlayerInventoryOperations.TryRemoveStack"/> (NEO-54).</summary>
|
||||
public enum PlayerInventoryMutationKind
|
||||
{
|
||||
/// <summary>Validation or rules deny — inventory unchanged.</summary>
|
||||
Denied,
|
||||
/// <summary>Validation or rules deny — inventory unchanged.</summary>
|
||||
Denied,
|
||||
|
||||
/// <summary><see cref="IPlayerInventoryStore"/> could not read or write the player bucket.</summary>
|
||||
StoreMissing,
|
||||
/// <summary><see cref="IPlayerInventoryStore"/> could not read or write the player bucket.</summary>
|
||||
StoreMissing,
|
||||
|
||||
/// <summary>Mutation applied and persisted.</summary>
|
||||
Applied,
|
||||
/// <summary>Mutation applied and persisted.</summary>
|
||||
Applied,
|
||||
}
|
||||
|
||||
/// <param name="ReasonCode">Populated when <see cref="Kind"/> is <see cref="PlayerInventoryMutationKind.Denied"/>.</param>
|
||||
/// <param name="Snapshot">Authoritative snapshot after apply, or unchanged snapshot on deny; null when store missing.</param>
|
||||
public readonly record struct PlayerInventoryMutationOutcome(
|
||||
PlayerInventoryMutationKind Kind,
|
||||
string? ReasonCode,
|
||||
PlayerInventorySnapshot? Snapshot);
|
||||
PlayerInventoryMutationKind Kind,
|
||||
string? ReasonCode,
|
||||
PlayerInventorySnapshot? Snapshot);
|
||||
|
|
|
|||
Loading…
Reference in New Issue