NEO-127: address code review test and backlog doc findings
Add null-bundle and invalid-id deny tests, fix replay XP assertion, align E7M2-04 backlog wording with TryApplyGrant, strike review items done.pull/166/head
parent
581c90ab34
commit
65ca11171b
|
|
@ -160,7 +160,7 @@ Combat intro bundle is **skill XP only** — encounter loot already granted **`c
|
|||
**In scope**
|
||||
|
||||
- `RewardRouterOperations.TryDeliverQuestCompletion` in `server/NeonSprawl.Server/Game/Quests/` (or `Game/Rewards/`).
|
||||
- Item grants via **`PlayerInventoryOperations`**; skill XP via **`MissionRewardSkillXpGrant`**.
|
||||
- Item grants via **`PlayerInventoryOperations`**; skill XP via **`SkillProgressionGrantOperations.TryApplyGrant`** with fixed **`mission_reward`** source kind (deny envelopes; not the fire-and-forget **`MissionRewardSkillXpGrant`** wrapper).
|
||||
- Deny reason codes (inventory full, unknown item/skill, xp deny).
|
||||
- Unit + integration tests (AAA): happy path, inventory deny, idempotent replay via store.
|
||||
|
||||
|
|
|
|||
|
|
@ -54,7 +54,7 @@
|
|||
|
||||
- **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item rollback, `IRewardDeliveryStore.TryRecord`.
|
||||
- **Types:** `RewardDeliveryResult`, `RewardDeliveryReasonCodes`.
|
||||
- **Tests:** 7 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny).
|
||||
- **Tests:** 10 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny, null bundle empty record, invalid id deny).
|
||||
- **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated.
|
||||
|
||||
## Technical approach
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ NEO-127 adds **`RewardRouterOperations.TryDeliverQuestCompletion`** under `Game/
|
|||
| Path | Result |
|
||||
|------|--------|
|
||||
| `docs/plans/NEO-127-implementation-plan.md` | **Matches** — kickoff decisions adopted; AC checked; reconciliation accurate. |
|
||||
| `docs/plans/E7M2-prototype-backlog.md` (E7M2-04) | **Partially matches** — AC checkboxes complete; **In scope** still names **`MissionRewardSkillXpGrant`** while implementation correctly uses **`TryApplyGrant`** per plan kickoff (stale backlog wording only). |
|
||||
| `docs/plans/E7M2-prototype-backlog.md` (E7M2-04) | **Matches** — AC checkboxes complete; in-scope skill XP path aligned with **`TryApplyGrant`**. |
|
||||
| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-127 implementation anchor documents router apply path. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-04 router apply; row correctly stays **Planned** until NEO-132 capstone. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required for this slice. |
|
||||
|
|
@ -32,11 +32,11 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Empty / null bundle test** — Kickoff decision and plan both require success with empty grant snapshots and **`TryRecord`** for null or both-empty bundles. Code handles this (skips inventory pre-flight, records empty arrays), but no test locks the behavior. A single AAA test with `bundle: null` or `new QuestRewardBundleRow([], [])` would close the gap.
|
||||
1. ~~**Empty / null bundle test** — Kickoff decision and plan both require success with empty grant snapshots and **`TryRecord`** for null or both-empty bundles. Code handles this (skips inventory pre-flight, records empty arrays), but no test locks the behavior. A single AAA test with `bundle: null` or `new QuestRewardBundleRow([], [])` would close the gap.~~ **Done.** `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull`.
|
||||
|
||||
2. **Invalid id deny tests** — `invalid_player_id` / `invalid_quest_id` are implemented and documented in `server/README.md`, but untested. Two small deny-path tests (empty/whitespace ids) would match the fail-closed store precedent from NEO-126.
|
||||
2. ~~**Invalid id deny tests** — `invalid_player_id` / `invalid_quest_id` are implemented and documented in `server/README.md`, but untested. Two small deny-path tests (empty/whitespace ids) would match the fail-closed store precedent from NEO-126.~~ **Done.** `TryDeliverQuestCompletion_ShouldDenyInvalidPlayerId_WhenPlayerIdIsEmpty` and `…InvalidQuestId…`.
|
||||
|
||||
3. **Replay XP assertion** — In `TryDeliverQuestCompletion_ShouldApplyGatherIntroSkillXp_WhenBundleIsSkillOnly`, the second-call assertion reuses `salvageXp` captured before the replay `Deliver` call. Re-fetch **`GetXpTotals`** after the second call to prove no double XP grant on idempotent replay.
|
||||
3. ~~**Replay XP assertion** — In `TryDeliverQuestCompletion_ShouldApplyGatherIntroSkillXp_WhenBundleIsSkillOnly`, the second-call assertion reuses `salvageXp` captured before the replay `Deliver` call. Re-fetch **`GetXpTotals`** after the second call to prove no double XP grant on idempotent replay.~~ **Done.** Gather intro test re-fetches XP after replay.
|
||||
|
||||
## Nits
|
||||
|
||||
|
|
@ -54,7 +54,7 @@ None.
|
|||
dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests
|
||||
```
|
||||
|
||||
All 7 tests pass (verified 2026-06-07).
|
||||
All 10 tests pass (verified 2026-06-07; was 7 before review follow-up).
|
||||
|
||||
Optional before merge:
|
||||
|
||||
|
|
|
|||
|
|
@ -47,7 +47,9 @@ public sealed class RewardRouterOperationsTests
|
|||
Assert.Equal(salvageBefore + 25, salvageXp);
|
||||
Assert.True(second.Success);
|
||||
Assert.Equal(RewardDeliveryReasonCodes.AlreadyDelivered, second.ReasonCode);
|
||||
Assert.Equal(salvageBefore + 25, salvageXp);
|
||||
var xpAfterReplay = deps.SkillProgressionStore.GetXpTotals(PlayerId);
|
||||
Assert.True(xpAfterReplay.TryGetValue("salvage", out var salvageAfterReplay));
|
||||
Assert.Equal(salvageBefore + 25, salvageAfterReplay);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
|
@ -200,10 +202,92 @@ public sealed class RewardRouterOperationsTests
|
|||
Assert.False(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out _));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var deps = ResolveDependencies(factory);
|
||||
var xpBefore = deps.SkillProgressionStore.GetXpTotals(PlayerId);
|
||||
deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryBefore);
|
||||
|
||||
// Act
|
||||
var result = Deliver(deps, GatherQuestId, bundle: null, TimeProvider.System);
|
||||
|
||||
// Assert
|
||||
Assert.True(result.Success);
|
||||
Assert.Null(result.ReasonCode);
|
||||
Assert.Empty(result.GrantedItems);
|
||||
Assert.Empty(result.GrantedSkillXp);
|
||||
Assert.NotNull(result.DeliveryEvent);
|
||||
Assert.True(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out var stored));
|
||||
Assert.Empty(stored.GrantedItems);
|
||||
Assert.Empty(stored.GrantedSkillXp);
|
||||
Assert.Equal(xpBefore, deps.SkillProgressionStore.GetXpTotals(PlayerId));
|
||||
deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryAfter);
|
||||
Assert.Equal(TotalBagQuantity(inventoryBefore!), TotalBagQuantity(inventoryAfter!));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryDeliverQuestCompletion_ShouldDenyInvalidPlayerId_WhenPlayerIdIsEmpty()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var deps = ResolveDependencies(factory);
|
||||
var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]);
|
||||
|
||||
// Act
|
||||
var result = RewardRouterOperations.TryDeliverQuestCompletion(
|
||||
" ",
|
||||
GatherQuestId,
|
||||
bundle,
|
||||
deps.ItemRegistry,
|
||||
deps.InventoryStore,
|
||||
deps.SkillRegistry,
|
||||
deps.SkillProgressionStore,
|
||||
deps.LevelCurve,
|
||||
deps.PerkUnlockEngine,
|
||||
deps.DeliveryStore,
|
||||
TimeProvider.System);
|
||||
|
||||
// Assert
|
||||
Assert.False(result.Success);
|
||||
Assert.Equal(RewardDeliveryReasonCodes.InvalidPlayerId, result.ReasonCode);
|
||||
Assert.False(deps.DeliveryStore.TryGet(" ", GatherQuestId, out _));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryDeliverQuestCompletion_ShouldDenyInvalidQuestId_WhenQuestIdIsEmpty()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var deps = ResolveDependencies(factory);
|
||||
var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]);
|
||||
|
||||
// Act
|
||||
var result = RewardRouterOperations.TryDeliverQuestCompletion(
|
||||
PlayerId,
|
||||
" ",
|
||||
bundle,
|
||||
deps.ItemRegistry,
|
||||
deps.InventoryStore,
|
||||
deps.SkillRegistry,
|
||||
deps.SkillProgressionStore,
|
||||
deps.LevelCurve,
|
||||
deps.PerkUnlockEngine,
|
||||
deps.DeliveryStore,
|
||||
TimeProvider.System);
|
||||
|
||||
// Assert
|
||||
Assert.False(result.Success);
|
||||
Assert.Equal(RewardDeliveryReasonCodes.InvalidQuestId, result.ReasonCode);
|
||||
Assert.False(deps.DeliveryStore.TryGet(PlayerId, " ", out _));
|
||||
}
|
||||
|
||||
private static RewardDeliveryResult Deliver(
|
||||
RewardRouterTestDependencies deps,
|
||||
string questId,
|
||||
QuestRewardBundleRow bundle,
|
||||
QuestRewardBundleRow? bundle,
|
||||
TimeProvider timeProvider) =>
|
||||
RewardRouterOperations.TryDeliverQuestCompletion(
|
||||
PlayerId,
|
||||
|
|
@ -259,6 +343,20 @@ public sealed class RewardRouterOperationsTests
|
|||
return total;
|
||||
}
|
||||
|
||||
private static int TotalBagQuantity(PlayerInventorySnapshot snapshot)
|
||||
{
|
||||
var total = 0;
|
||||
foreach (var slot in snapshot.BagSlots)
|
||||
{
|
||||
if (!slot.IsEmpty)
|
||||
{
|
||||
total += slot.Quantity;
|
||||
}
|
||||
}
|
||||
|
||||
return total;
|
||||
}
|
||||
|
||||
private sealed record RewardRouterTestDependencies(
|
||||
IItemDefinitionRegistry ItemRegistry,
|
||||
IPlayerInventoryStore InventoryStore,
|
||||
|
|
|
|||
Loading…
Reference in New Issue