[Unit Tests] Add coverage for quest reward and objective types - #326
[Unit Tests] Add coverage for quest reward and objective types#326DiamondDagger590 wants to merge 1 commit into
Conversation
Add unit tests for RedeemableExperienceRewardType, RedeemableLevelsRewardType, BoostedExperienceRewardType, and AdvancementCompleteObjectiveType. Tests cover parseConfig, serializeConfig/fromSerializedConfig round-trips, withExactAmount, withAmountMultiplier, withLocalizationRoute, withInlineDisplayLabel, describeForDisplay, isScalable, getNumericAmount, and edge cases (invalid keys, empty filters, non-numeric amounts, type coercion). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aga2GRcjD1Fq53TPpAWaR3
📝 WalkthroughWalkthroughAdded JUnit coverage for ChangesQuest and reward coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @DiamondDagger590's task in 7m 48s —— View job Claude finished the reviewTodo list
Verdict: 4 important, 3 nitsThis PR adds four new test classes (1237 lines, zero production code touched) covering Important findings
Nits
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableLevelsRewardTypeCoverageTest.java (1)
311-338: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an assertion for the scaled amount in
WithAmountMultiplier.Both tests in this nested class check only that
localizationRouteanddisplayLabelsurvivewithAmountMultiplier. Neither test checks the resultingamount.withAmountMultipliercomputesMath.max(1, (int) (amount * multiplier)), which truncates and floors the result to a minimum of1. Add a test that verifies the scaled amount for a normal multiplier, and a boundary test that verifies the floor-clamp to1.As per path instructions, "Test every new public method with non-trivial logic" and "Cover relevant edge cases, including... off-by-one boundaries."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableLevelsRewardTypeCoverageTest.java` around lines 311 - 338, Extend the WithAmountMultiplier tests for RedeemableLevelsRewardType.withAmountMultiplier to assert the serialized amount after scaling, including a normal multiplier result and a low-multiplier boundary that clamps the amount to 1. Preserve the existing localization-route and display-label assertions.Source: Path instructions
src/test/java/us/eunoians/mcrpg/quest/reward/builtin/BoostedExperienceRewardTypeCoverageTest.java (1)
286-313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an assertion for the scaled amount in
WithAmountMultiplier.Both tests in this nested class check only that
localizationRouteanddisplayLabelsurvivewithAmountMultiplier. Neither test checks the resultingamount.withAmountMultipliercomputesMath.max(1, (int) (amount * multiplier)), which truncates and floors the result to a minimum of1. Add a test that verifies the scaled amount for a normal multiplier, and a boundary test that verifies the floor-clamp to1when a small multiplier would otherwise produce0or a negative value.✅ Suggested additional tests
`@Nested` `@DisplayName`("withAmountMultiplier") class WithAmountMultiplier { + `@Test` + `@DisplayName`("scales amount by multiplier") + void withAmountMultiplier_scalesAmount() { + BoostedExperienceRewardType configured = new BoostedExperienceRewardType() + .fromSerializedConfig(Map.of("amount", 400)); + BoostedExperienceRewardType scaled = configured.withAmountMultiplier(0.5); + assertEquals(200, scaled.getNumericAmount().orElse(0)); + } + + `@Test` + `@DisplayName`("clamps scaled amount to minimum of 1") + void withAmountMultiplier_clampsToMinimumOne() { + BoostedExperienceRewardType configured = new BoostedExperienceRewardType() + .fromSerializedConfig(Map.of("amount", 1)); + BoostedExperienceRewardType scaled = configured.withAmountMultiplier(0.1); + assertEquals(1, scaled.getNumericAmount().orElse(0)); + } + `@Test` `@DisplayName`("preserves localization route") void withAmountMultiplier_preservesRoute() {As per path instructions, "Test every new public method with non-trivial logic" and "Cover relevant edge cases, including... off-by-one boundaries."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/BoostedExperienceRewardTypeCoverageTest.java` around lines 286 - 313, Add amount assertions to the WithAmountMultiplier tests, verifying normal scaling through withAmountMultiplier and the Math.max floor-clamp to 1 for a multiplier producing zero or a negative result. Keep the existing localization-route and display-label preservation assertions intact, and cover both the ordinary and boundary cases.Source: Path instructions
src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableExperienceRewardTypeCoverageTest.java (1)
327-354: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an assertion for the scaled amount in
WithAmountMultiplier.Both tests in this nested class check only that
localizationRouteanddisplayLabelsurvivewithAmountMultiplier. Neither test checks the resultingamount.withAmountMultipliercomputesMath.max(1, (int) (amount * multiplier)), which truncates and floors the result to a minimum of1. Add a test that verifies the scaled amount for a normal multiplier, and a boundary test that verifies the floor-clamp to1.As per path instructions, "Test every new public method with non-trivial logic" and "Cover relevant edge cases, including... off-by-one boundaries."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableExperienceRewardTypeCoverageTest.java` around lines 327 - 354, The WithAmountMultiplier tests currently verify only metadata preservation; add assertions covering its calculated amount. Extend the existing normal-multiplier case to assert the expected scaled amount, and add a boundary test using a multiplier whose truncated result is below 1 to verify the amount is clamped to 1, using serializeConfig() and the existing configured reward setup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeCoverageTest.java`:
- Around line 62-94: Update AdvancementCompleteObjectiveType.parseConfig to
validate every advancement entry, warn with the configuration key and invalid
value, and enforce a match-nothing result or throw a descriptive configuration
exception instead of silently retaining malformed keys. Revise
checkInitialProgress_specificFilter_skipsInvalidKey and
checkInitialProgress_specificFilter_handlesMixedKeys to assert the selected
invalid-configuration behavior rather than accepting zero progress.
---
Nitpick comments:
In
`@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/BoostedExperienceRewardTypeCoverageTest.java`:
- Around line 286-313: Add amount assertions to the WithAmountMultiplier tests,
verifying normal scaling through withAmountMultiplier and the Math.max
floor-clamp to 1 for a multiplier producing zero or a negative result. Keep the
existing localization-route and display-label preservation assertions intact,
and cover both the ordinary and boundary cases.
In
`@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableExperienceRewardTypeCoverageTest.java`:
- Around line 327-354: The WithAmountMultiplier tests currently verify only
metadata preservation; add assertions covering its calculated amount. Extend the
existing normal-multiplier case to assert the expected scaled amount, and add a
boundary test using a multiplier whose truncated result is below 1 to verify the
amount is clamped to 1, using serializeConfig() and the existing configured
reward setup.
In
`@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableLevelsRewardTypeCoverageTest.java`:
- Around line 311-338: Extend the WithAmountMultiplier tests for
RedeemableLevelsRewardType.withAmountMultiplier to assert the serialized amount
after scaling, including a normal multiplier result and a low-multiplier
boundary that clamps the amount to 1. Preserve the existing localization-route
and display-label assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: fd2b710d-2b5c-401a-ab5a-816d457abeb5
📒 Files selected for processing (4)
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeCoverageTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/builtin/BoostedExperienceRewardTypeCoverageTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableExperienceRewardTypeCoverageTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/builtin/RedeemableLevelsRewardTypeCoverageTest.java
| @Test | ||
| @DisplayName("skips null NamespacedKey from invalid advancement string") | ||
| void checkInitialProgress_specificFilter_skipsInvalidKey() { | ||
| Section section = mock(Section.class); | ||
| when(section.contains("advancements")).thenReturn(true); | ||
| when(section.getStringList("advancements")).thenReturn(List.of( | ||
| ":::invalid:::key:::" | ||
| )); | ||
| AdvancementCompleteObjectiveType configured = type.parseConfig(section); | ||
|
|
||
| PlayerMock player = server.addPlayer(); | ||
| long progress = configured.checkInitialProgress(player); | ||
|
|
||
| assertEquals(0, progress); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("handles mix of valid and invalid keys in filter") | ||
| void checkInitialProgress_specificFilter_handlesMixedKeys() { | ||
| Section section = mock(Section.class); | ||
| when(section.contains("advancements")).thenReturn(true); | ||
| when(section.getStringList("advancements")).thenReturn(List.of( | ||
| ":::bad:::", | ||
| "minecraft:story/iron_tools", | ||
| "also:::bad" | ||
| )); | ||
| AdvancementCompleteObjectiveType configured = type.parseConfig(section); | ||
|
|
||
| PlayerMock player = server.addPlayer(); | ||
| long progress = configured.checkInitialProgress(player); | ||
|
|
||
| assertEquals(0, progress); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject or diagnose invalid advancement keys during configuration loading.
These tests accept malformed advancements entries as a normal zero-progress case. parseConfig retains the invalid strings, and checkInitialProgress skips them without a warning. A malformed key can therefore create an objective that cannot progress without identifying the bad configuration value.
Validate each key in parseConfig. Log a warning with the configuration key and invalid value, then use match-nothing or throw a descriptive exception. Update these tests to assert the selected behavior.
As per coding guidelines, “Config parsers must not silently convert invalid keys into a no-filter or match-all result; log a warning and use match-nothing or throw during loading.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeCoverageTest.java`
around lines 62 - 94, Update AdvancementCompleteObjectiveType.parseConfig to
validate every advancement entry, warn with the configuration key and invalid
value, and enforce a match-nothing result or throw a descriptive configuration
exception instead of silently retaining malformed keys. Revise
checkInitialProgress_specificFilter_skipsInvalidKey and
checkInitialProgress_specificFilter_handlesMixedKeys to assert the selected
invalid-configuration behavior rather than accepting zero progress.
Source: Coding guidelines
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @Test | ||
| @DisplayName("returns 0 with specific filter when advancements are not registered on server") | ||
| void checkInitialProgress_specificFilter_returnsZero_whenAdvancementsNotRegistered() { | ||
| Section section = mock(Section.class); |
There was a problem hiding this comment.
IMPORTANT: All four checkInitialProgress tests in this nested class only exercise the "not yet completed" path and assert progress == 0. The "already granted" branch in AdvancementCompleteObjectiveType.checkInitialProgress (player.getAdvancementProgress(advancement).isDone() → true, incrementing count) is never covered — the PR description says this is skipped because "MockBukkit does not support registering custom advancements," but Player, Advancement, and AdvancementProgress are all mockable interfaces (no real registration needed): mock(Player.class) + mock(Advancement.class) + mock(AdvancementProgress.class), with player.getAdvancementProgress(advancement) stubbed to return it and .isDone() stubbed true, combined with mockStatic(Bukkit.class) for the empty-filter branch's Bukkit.advancementIterator() or the specific-filter branch's Bukkit.getAdvancement(...).
This leaves the entire reason the method exists (retroactive progress grant) permanently unverified — a regression that always returns 0, or double-counts, would pass CI silently.
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| type = new AdvancementCompleteObjectiveType(); |
There was a problem hiding this comment.
IMPORTANT: This "Coverage" test file (added to raise AdvancementCompleteObjectiveType from ~34%) never exercises describeObjective(McRPGPlayer, long) or its private extractDisplayName helper (production AdvancementCompleteObjectiveType.java:130-168) — the only non-trivial public formatting method on the class, with three branches (empty filter / single advancement / multi-advancement loop) plus MiniMessage tag-escaping and underscore→space conversion. None of it is verified anywhere in the repo.
| assertEquals(300, configured.getNumericAmount().orElse(0)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
IMPORTANT: None of the three new reward-type coverage tests (this file, RedeemableExperienceRewardTypeCoverageTest, RedeemableLevelsRewardTypeCoverageTest) test grant(Player) — neither the no-op branch (amount <= 0) nor the success branch that resolves the McRPGPlayer via the registry and calls modifyBoostedExperience/modifyRedeemableExperience/modifyRedeemableLevels. This breaks with the established convention in this same package: ItemRewardTypeCoverageTest, CommandRewardTypeCoverageTest, ExperienceRewardTypeCoverageTest, and ScalableCommandRewardTypeCoverageTest all cover grant() including its no-op branches. grant() is the reward's core side-effecting behavior and is left completely unverified here.
| import static org.mockito.Mockito.when; | ||
|
|
||
| @DisplayName("RedeemableExperienceRewardType Coverage") | ||
| class RedeemableExperienceRewardTypeCoverageTest extends McRPGBaseTest { |
There was a problem hiding this comment.
IMPORTANT: Only the no-arg describeForDisplay() is tested across all three new reward-type coverage tests. The describeForDisplay(McRPGPlayer) overload (e.g. BoostedExperienceRewardType.java:159-188 and the equivalent methods here / in RedeemableLevelsRewardType) — with its localizationRoute branch (+ exception fallback), inline displayLabel branch, and default LocalizationKey branch (+ exception fallback to describeForDisplay()) — is completely untested, even though this is the player-facing overload actually used in reward GUIs/lore. AbilityUpgradeRewardTypeDisplayTest already establishes the pattern for testing exactly this in the same package.
2 participants
Summary
RedeemableExperienceRewardType,RedeemableLevelsRewardType, andBoostedExperienceRewardType— three structurally similar quest reward types that were at ~44-45% coverageAdvancementCompleteObjectiveType— a quest objective type at ~34% coverage@Nested/@DisplayNamegrouping conventions and extendMcRPGBaseTestWhat's covered
Each reward type test covers:
parseConfig,serializeConfig/fromSerializedConfiground-trips,withExactAmount,withAmountMultiplier,withLocalizationRoute,withInlineDisplayLabel,describeForDisplay,isScalable,getNumericAmount, and edge cases (missing keys, string/Long/Double type coercion, non-numeric amounts).The advancement objective test covers:
checkInitialProgress(unregistered advancements, invalid keys, mixed valid/invalid keys),processProgresswith multi-advancement filters (matching and non-matching),parseConfigedge cases (empty list, single advancement), andAdvancementCompleteQuestContextconstruction.What's not covered
grant()anddescribeForDisplay(McRPGPlayer)on the reward types require deep McRPG runtime state (registry access, player manager) and are excluded from these pure-logic coverage tests.checkInitialProgresswith actually-granted advancements is excluded because MockBukkit does not support registering custom advancements.Test plan
./gradlew test— zero failures)Generated by Claude Code
Summary by CodeRabbit