[Unit Tests] Add coverage for BucketEmpty/Fill objectives and Sound/Title rewards#308
Open
DiamondDagger590 wants to merge 1 commit into
Open
[Unit Tests] Add coverage for BucketEmpty/Fill objectives and Sound/Title rewards#308DiamondDagger590 wants to merge 1 commit into
DiamondDagger590 wants to merge 1 commit into
Conversation
…and Sound/Title reward types Add extended coverage test suites for four quest system classes: - BucketEmptyObjectiveTypeCoverageTest: canProcess, processProgress with/without filter, parseConfig edge cases - BucketFillObjectiveTypeCoverageTest: same coverage pattern for the fill variant - SoundRewardTypeTest: identity, serialization round-trips, parseConfig, grant smoke tests, default methods - TitleRewardTypeTest: identity, serialization round-trips, timing defaults, parseConfig, grant smoke tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Die2JoeWQdKGfBiSQkXGvb
WalkthroughAdded comprehensive JUnit 5 coverage for bucket fill/empty objectives and sound/title rewards, including context handling, filtering, identity, configuration parsing, serialization, granting, defaults, and invalid inputs. ChangesBucket objective coverage
Reward type coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/BucketFillObjectiveTypeCoverageTest.java`:
- Around line 264-267: In the test setup around the PlayerBucketFillEvent,
replace the mocked ItemStack and its getType stubbing with a real ItemStack
initialized using filledBucketMaterial. Keep the event mock and configure it to
return the real stack via getItemStack, preserving the existing test behavior.
In
`@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/SoundRewardTypeTest.java`:
- Around line 276-281: Update grant_configuredInstance_playsSound to verify the
PlayerMock heard the configured ENTITY_PLAYER_LEVELUP sound after
configured.grant(player). Retain the no-exception assertion if desired, and use
PlayerMock.assertSoundHeard with the configured sound, volume, and pitch.
- Around line 308-310: Update the withExactAmount_returnsSameInstance test to
use assertSame instead of assertEquals, matching the existing identity assertion
pattern in withAmountMultiplier_returnsSameInstance.
- Around line 290-292: Update withAmountMultiplier_returnsSameInstance in
SoundRewardTypeTest to use assertSame when verifying
baseType.withAmountMultiplier(2.0) returns the identical object, and add the
corresponding JUnit assertSame static import.
In
`@src/test/java/us/eunoians/mcrpg/quest/reward/builtin/TitleRewardTypeTest.java`:
- Around line 320-343: Replace assertEquals with assertSame in
withAmountMultiplier_returnsSameInstance and
withExactAmount_returnsSameInstance, and add the corresponding JUnit assertion
import. Preserve the existing method calls and test expectations.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b34d2946-42c1-4f39-8aad-df539c748929
📒 Files selected for processing (4)
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/BucketEmptyObjectiveTypeCoverageTest.javasrc/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/BucketFillObjectiveTypeCoverageTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/builtin/SoundRewardTypeTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/builtin/TitleRewardTypeTest.java
Comment on lines
+264
to
+267
| PlayerBucketFillEvent event = mock(PlayerBucketFillEvent.class); | ||
| ItemStack itemStack = mock(ItemStack.class); | ||
| when(itemStack.getType()).thenReturn(filledBucketMaterial); | ||
| when(event.getItemStack()).thenReturn(itemStack); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use a real ItemStack instance instead of mocking it.
Since McRPGBaseTest initializes the MockBukkit environment, you can safely use a real ItemStack instance rather than mocking this data object. This reduces mock setup boilerplate and relies on actual Bukkit behavior.
♻️ Proposed refactor
private PlayerBucketFillEvent createMockBucketFillEvent(Material filledBucketMaterial) {
PlayerBucketFillEvent event = mock(PlayerBucketFillEvent.class);
- ItemStack itemStack = mock(ItemStack.class);
- when(itemStack.getType()).thenReturn(filledBucketMaterial);
- when(event.getItemStack()).thenReturn(itemStack);
+ when(event.getItemStack()).thenReturn(new ItemStack(filledBucketMaterial));
return event;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PlayerBucketFillEvent event = mock(PlayerBucketFillEvent.class); | |
| ItemStack itemStack = mock(ItemStack.class); | |
| when(itemStack.getType()).thenReturn(filledBucketMaterial); | |
| when(event.getItemStack()).thenReturn(itemStack); | |
| PlayerBucketFillEvent event = mock(PlayerBucketFillEvent.class); | |
| when(event.getItemStack()).thenReturn(new ItemStack(filledBucketMaterial)); |
🤖 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/BucketFillObjectiveTypeCoverageTest.java`
around lines 264 - 267, In the test setup around the PlayerBucketFillEvent,
replace the mocked ItemStack and its getType stubbing with a real ItemStack
initialized using filledBucketMaterial. Keep the event mock and configure it to
return the real stack via getItemStack, preserving the existing test behavior.
Comment on lines
+276
to
+281
| public void grant_configuredInstance_playsSound() { | ||
| SoundRewardType configured = baseType.fromSerializedConfig( | ||
| Map.of("sound", "ENTITY_PLAYER_LEVELUP", "volume", 1.0f, "pitch", 1.0f)); | ||
| PlayerMock player = server.addPlayer(); | ||
| assertDoesNotThrow(() -> configured.grant(player)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Verify that the sound was actually played to the player.
While assertDoesNotThrow confirms the method doesn't crash, it doesn't verify the expected side effect. MockBukkit's PlayerMock provides assertSoundHeard to explicitly verify that the player received the sound, which makes the test much more robust.
♻️ Proposed refactor
public void grant_configuredInstance_playsSound() {
SoundRewardType configured = baseType.fromSerializedConfig(
Map.of("sound", "ENTITY_PLAYER_LEVELUP", "volume", 1.0f, "pitch", 1.0f));
PlayerMock player = server.addPlayer();
assertDoesNotThrow(() -> configured.grant(player));
+ player.assertSoundHeard(Sound.ENTITY_PLAYER_LEVELUP);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void grant_configuredInstance_playsSound() { | |
| SoundRewardType configured = baseType.fromSerializedConfig( | |
| Map.of("sound", "ENTITY_PLAYER_LEVELUP", "volume", 1.0f, "pitch", 1.0f)); | |
| PlayerMock player = server.addPlayer(); | |
| assertDoesNotThrow(() -> configured.grant(player)); | |
| } | |
| public void grant_configuredInstance_playsSound() { | |
| SoundRewardType configured = baseType.fromSerializedConfig( | |
| Map.of("sound", "ENTITY_PLAYER_LEVELUP", "volume", 1.0f, "pitch", 1.0f)); | |
| PlayerMock player = server.addPlayer(); | |
| assertDoesNotThrow(() -> configured.grant(player)); | |
| player.assertSoundHeard(Sound.ENTITY_PLAYER_LEVELUP); | |
| } |
🤖 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/SoundRewardTypeTest.java`
around lines 276 - 281, Update grant_configuredInstance_playsSound to verify the
PlayerMock heard the configured ENTITY_PLAYER_LEVELUP sound after
configured.grant(player). Retain the no-exception assertion if desired, and use
PlayerMock.assertSoundHeard with the configured sound, volume, and pitch.
Comment on lines
+290
to
+292
| public void withAmountMultiplier_returnsSameInstance() { | ||
| assertEquals(baseType, baseType.withAmountMultiplier(2.0)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use assertSame to explicitly check object identity.
Since the test name implies checking for the exact same instance (this), assertSame is semantically more precise than assertEquals.
💡 Proposed refactor
public void withAmountMultiplier_returnsSameInstance() {
- assertEquals(baseType, baseType.withAmountMultiplier(2.0));
+ assertSame(baseType, baseType.withAmountMultiplier(2.0));
}(Ensure you also add import static org.junit.jupiter.api.Assertions.assertSame; at the top of the file).
🤖 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/SoundRewardTypeTest.java`
around lines 290 - 292, Update withAmountMultiplier_returnsSameInstance in
SoundRewardTypeTest to use assertSame when verifying
baseType.withAmountMultiplier(2.0) returns the identical object, and add the
corresponding JUnit assertSame static import.
Comment on lines
+308
to
+310
| public void withExactAmount_returnsSameInstance() { | ||
| assertEquals(baseType, baseType.withExactAmount(10)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use assertSame to explicitly check object identity.
Similar to withAmountMultiplier_returnsSameInstance, use assertSame here to perfectly match the test's intent of checking instance identity.
💡 Proposed refactor
public void withExactAmount_returnsSameInstance() {
- assertEquals(baseType, baseType.withExactAmount(10));
+ assertSame(baseType, baseType.withExactAmount(10));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void withExactAmount_returnsSameInstance() { | |
| assertEquals(baseType, baseType.withExactAmount(10)); | |
| } | |
| public void withExactAmount_returnsSameInstance() { | |
| assertSame(baseType, baseType.withExactAmount(10)); | |
| } |
🤖 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/SoundRewardTypeTest.java`
around lines 308 - 310, Update the withExactAmount_returnsSameInstance test to
use assertSame instead of assertEquals, matching the existing identity assertion
pattern in withAmountMultiplier_returnsSameInstance.
Comment on lines
+320
to
+343
| @Test | ||
| @DisplayName("withAmountMultiplier returns this") | ||
| public void withAmountMultiplier_returnsSameInstance() { | ||
| assertEquals(baseType, baseType.withAmountMultiplier(2.0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("isScalable returns false") | ||
| public void isScalable_returnsFalse() { | ||
| assertFalse(baseType.isScalable()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("getNumericAmount returns empty") | ||
| public void getNumericAmount_returnsEmpty() { | ||
| assertTrue(baseType.getNumericAmount().isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("withExactAmount returns this") | ||
| public void withExactAmount_returnsSameInstance() { | ||
| assertEquals(baseType, baseType.withExactAmount(10)); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use assertSame to verify instance identity.
Since the intent of these tests is to verify that the methods return the exact same instance (this), assertSame is a more precise assertion than assertEquals. assertEquals only validates logical equivalence.
♻️ Proposed fixes
Add the import at the top of the file:
import static org.junit.jupiter.api.Assertions.assertSame;And update the assertions:
`@Test`
`@DisplayName`("withAmountMultiplier returns this")
public void withAmountMultiplier_returnsSameInstance() {
- assertEquals(baseType, baseType.withAmountMultiplier(2.0));
+ assertSame(baseType, baseType.withAmountMultiplier(2.0));
}
`@Test`
`@DisplayName`("isScalable returns false")
public void isScalable_returnsFalse() {
assertFalse(baseType.isScalable());
}
`@Test`
`@DisplayName`("getNumericAmount returns empty")
public void getNumericAmount_returnsEmpty() {
assertTrue(baseType.getNumericAmount().isEmpty());
}
`@Test`
`@DisplayName`("withExactAmount returns this")
public void withExactAmount_returnsSameInstance() {
- assertEquals(baseType, baseType.withExactAmount(10));
+ assertSame(baseType, baseType.withExactAmount(10));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| @DisplayName("withAmountMultiplier returns this") | |
| public void withAmountMultiplier_returnsSameInstance() { | |
| assertEquals(baseType, baseType.withAmountMultiplier(2.0)); | |
| } | |
| @Test | |
| @DisplayName("isScalable returns false") | |
| public void isScalable_returnsFalse() { | |
| assertFalse(baseType.isScalable()); | |
| } | |
| @Test | |
| @DisplayName("getNumericAmount returns empty") | |
| public void getNumericAmount_returnsEmpty() { | |
| assertTrue(baseType.getNumericAmount().isEmpty()); | |
| } | |
| @Test | |
| @DisplayName("withExactAmount returns this") | |
| public void withExactAmount_returnsSameInstance() { | |
| assertEquals(baseType, baseType.withExactAmount(10)); | |
| } | |
| } | |
| `@Test` | |
| `@DisplayName`("withAmountMultiplier returns this") | |
| public void withAmountMultiplier_returnsSameInstance() { | |
| assertSame(baseType, baseType.withAmountMultiplier(2.0)); | |
| } | |
| `@Test` | |
| `@DisplayName`("isScalable returns false") | |
| public void isScalable_returnsFalse() { | |
| assertFalse(baseType.isScalable()); | |
| } | |
| `@Test` | |
| `@DisplayName`("getNumericAmount returns empty") | |
| public void getNumericAmount_returnsEmpty() { | |
| assertTrue(baseType.getNumericAmount().isEmpty()); | |
| } | |
| `@Test` | |
| `@DisplayName`("withExactAmount returns this") | |
| public void withExactAmount_returnsSameInstance() { | |
| assertSame(baseType, baseType.withExactAmount(10)); | |
| } | |
| } |
🤖 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/TitleRewardTypeTest.java`
around lines 320 - 343, Replace assertEquals with assertSame in
withAmountMultiplier_returnsSameInstance and
withExactAmount_returnsSameInstance, and add the corresponding JUnit assertion
import. Preserve the existing method calls and test expectations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BucketEmptyObjectiveTypeCoverageTest(19 tests): canProcess, processProgress with/without bucket filter, wrong context type handling, identity, parseConfig edge cases (empty list, invalid materials, no buckets key)BucketFillObjectiveTypeCoverageTest(19 tests): same coverage pattern for the fill variant, correctly mockinggetItemStack().getType()instead ofgetBucket()SoundRewardTypeTest(~30 tests): identity, describeForDisplay, serializeConfig round-trips, fromSerializedConfig (missing/invalid sound, missing volume/pitch defaults, non-numeric fallbacks, case-insensitive sound names), parseConfig, grant smoke tests, default QuestRewardType methodsTitleRewardTypeTest(~28 tests): identity, describeForDisplay, serializeConfig with defaults and configured values, fromSerializedConfig (round-trips, missing fields, non-numeric timing fallbacks, empty config), parseConfig (all fields, missing fields, null getString), grant smoke tests, default QuestRewardType methodsTest plan
QuestRewardDistributionResolverPotBehaviorTestfailures (6 tests, present onrecodebase branch)assertDoesNotThrow) andassertNotSame(true, ...)→assertFalse()fixes🤖 Generated with Claude Code
https://claude.ai/code/session_01Die2JoeWQdKGfBiSQkXGvb
Generated by Claude Code
Summary by CodeRabbit