[Unit Tests] Add coverage for event classes: ability cooldown, quest cancel/reward, chain lifecycle - #328
[Unit Tests] Add coverage for event classes: ability cooldown, quest cancel/reward, chain lifecycle#328DiamondDagger590 wants to merge 1 commit into
Conversation
…ard grant, and chain lifecycle Cover 7 event classes that previously had 0% test coverage: - AbilityPutOnCooldownEvent: cooldown clamping (negative→0), setter, getters - QuestCancelEvent: 3 constructors (incl. 2 deprecated), expiration flag, definition key fallback - QuestRewardGrantEvent: cancellation, mutable reward list, RewardGrantContext - QuestRewardGrantedEvent: immutable reward list (defensive copy, UnsupportedOperationException) - QuestChainExpireEvent: cancellation, Optional-wrapped nullable player - QuestChainRestartEvent: RestartReason enum, nullable player, handler list - QuestChainStepRetryEvent: retry number, max retries (-1 unlimited), step getter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnEnfiJSL4odu54xz2KCzG
|
Claude finished @DiamondDagger590's task in 3m 59s —— View job Review complete — 0 important, 2 nitsThis PR adds 7 new JUnit test files covering previously-untested event classes ( I verified each new test against its production class and confirmed the assertions genuinely exercise the constructor clamping ( Nits (2)
Neither nit blocks merge. |
📝 WalkthroughWalkthroughAdded comprehensive tests for ability, quest, and quest-chain events. The tests cover constructors, accessors, state changes, reward collections, player UUID handling, cancellation, retry limits, and handler-list consistency. ChangesEvent contract tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.java-25-32 (1)
25-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCentralize and document the fixture helpers.
The repeated fixture helpers lack required Javadoc and
@NotNulldeclarations. Extract the commonbuildDefinitionhelper into a shared test fixture. Document and annotate all remaining helper parameters and return values.
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.java#L25-L32: replace the localbuildDefinitionhelper with the shared fixture.src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainRestartEventTest.java#L26-L33: replace the localbuildDefinitionhelper with the shared fixture.src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java#L24-L35: replacebuildDefinitionwith the shared fixture and add Javadoc plus@NotNulldeclarations tobuildStep.As per coding guidelines, “Do not duplicate logic across two or more classes” and “Annotate all non-null return types and parameters with IntelliJ v12
@NotNull.”🤖 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/event/quest/chain/QuestChainExpireEventTest.java` around lines 25 - 32, Centralize the duplicated buildDefinition fixture and document all remaining test helpers with IntelliJ v12 `@NotNull` annotations. In src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.java lines 25-32 and src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainRestartEventTest.java lines 26-33, remove the local buildDefinition methods and use the shared fixture; in src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java lines 24-35, use the shared fixture and add Javadoc plus `@NotNull` annotations to buildStep’s non-null parameters and return value.Source: Coding guidelines
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java-120-142 (1)
120-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the
QuestChainStepRetryEventretry-number contract.
QuestChainStepRetryEventdocumentsretryNumberas 1-based, and there is no dispatch or source-side range check, so invalid cases likeretryNumber <= 0,maxRetries < -1, andmaxRetries == 0withretryNumber == 1can still be created and used. Add range validation in the event/API boundary and reject these states withIllegalArgumentException.🤖 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/event/quest/chain/QuestChainStepRetryEventTest.java` around lines 120 - 142, Update QuestChainStepRetryEvent construction or its API boundary to validate the documented retry-number contract: reject retryNumber values less than 1, maxRetries values less than -1, and the combination of maxRetries == 0 with retryNumber == 1 by throwing IllegalArgumentException. Preserve valid unlimited (-1) and positive-max-retry behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
src/test/java/us/eunoians/mcrpg/event/ability/AbilityPutOnCooldownEventTest.java (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a plain JUnit test for this event contract.
This test does not use MockBukkit server interaction or
McRPGPlayertracking. Remove theMcRPGBaseTestimport and superclass.Proposed change
-import us.eunoians.mcrpg.McRPGBaseTest; - -class AbilityPutOnCooldownEventTest extends McRPGBaseTest { +class AbilityPutOnCooldownEventTest {As per coding guidelines: tests that require neither MockBukkit server interaction nor
McRPGPlayertracking must use plain JUnit instead of extendingMcRPGBaseTest.Also applies to: 17-17
🤖 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/event/ability/AbilityPutOnCooldownEventTest.java` at line 6, Update AbilityPutOnCooldownEventTest to use plain JUnit by removing the McRPGBaseTest import and changing the test class so it no longer extends McRPGBaseTest; leave the event assertions and other test behavior unchanged.Source: Coding guidelines
🤖 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.
Other comments:
In
`@src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.java`:
- Around line 25-32: Centralize the duplicated buildDefinition fixture and
document all remaining test helpers with IntelliJ v12 `@NotNull` annotations. In
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.java
lines 25-32 and
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainRestartEventTest.java
lines 26-33, remove the local buildDefinition methods and use the shared
fixture; in
src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java
lines 24-35, use the shared fixture and add Javadoc plus `@NotNull` annotations to
buildStep’s non-null parameters and return value.
In
`@src/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java`:
- Around line 120-142: Update QuestChainStepRetryEvent construction or its API
boundary to validate the documented retry-number contract: reject retryNumber
values less than 1, maxRetries values less than -1, and the combination of
maxRetries == 0 with retryNumber == 1 by throwing IllegalArgumentException.
Preserve valid unlimited (-1) and positive-max-retry behavior.
---
Nitpick comments:
In
`@src/test/java/us/eunoians/mcrpg/event/ability/AbilityPutOnCooldownEventTest.java`:
- Line 6: Update AbilityPutOnCooldownEventTest to use plain JUnit by removing
the McRPGBaseTest import and changing the test class so it no longer extends
McRPGBaseTest; leave the event assertions and other test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: e7b6ab2f-0251-4629-98ad-9cef09e0b9d4
📒 Files selected for processing (7)
src/test/java/us/eunoians/mcrpg/event/ability/AbilityPutOnCooldownEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/QuestCancelEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantedEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainExpireEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainRestartEventTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/chain/QuestChainStepRetryEventTest.java
2 participants
Summary
./gradlew test— zero failures across the full suite)persona-testing.mdc) run against changes with no blocking concernsClasses Covered
AbilityPutOnCooldownEventMath.max(0, cooldown)) in constructor andsetCooldown(), large values (Long.MAX_VALUE), getters for holder/abilityQuestCancelEventgetQuestDefinitionKey()fallback to instance key when definition is nullQuestRewardGrantEventRewardGrantContextvalues, nullableQuestInstanceQuestRewardGrantedEventList.copyOf()(throwsUnsupportedOperationExceptionon add/remove), defensive copy independence from original list,RewardGrantContextvaluesQuestChainExpireEventPlayerwrapped inOptional.ofNullable(), UUID always available even with null playerQuestChainRestartEventRestartReasonenum (parameterized test over all values), nullable player (raw null), handler listQuestChainStepRetryEventQuestChainStepgetter, nullable playerTest plan
./gradlew test— zero failures)action_outcome_whenConditionnaming convention with@DisplayNameannotationsMcRPGBaseTestand useserver.addPlayer()forPlayerMockwhere neededGenerated by Claude Code
Summary by CodeRabbit