[Tests] Improve item builder test coverage (BaseItemBuilder, SkullBuilder, SpawnerBuilder)#67
[Tests] Improve item builder test coverage (BaseItemBuilder, SkullBuilder, SpawnerBuilder)#67DiamondDagger590 wants to merge 1 commit into
Conversation
…wnerBuilder Improve test coverage for the item builder subsystem: - BaseItemBuilder: new BaseItemBuilderMockBukkitTest covering enchantment addition/removal, getPlainName, getPlainLore, setColor, setItemDamage, isEdible, builder conversions, withType, setTrim, copy constructor, and fluent API chaining (coverage 65% -> 85.8%) - SkullBuilder: add tests for withAudience(Audience) with/without UUID, hideSkullDynamicToolTip merge branch, build with UUID and name+url (coverage 67% -> 95.3%) - SpawnerBuilder: add tests exercising positive count/delay/range and zero/negative count guard paths (coverage unchanged at 72% due to MockBukkit BlockStateMeta limitations) Overall line coverage: 81.0% -> 82.9% Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TmAF3ehyVWsALSN2Ua7yY
WalkthroughAdds MockBukkit coverage for ChangesBuilder Test Coverage
Estimated code review effort: 4 (Complex) | ~45 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 |
Testing ReviewI'll systematically apply every checklist item to the diff. AnalysisProduction files changed (inferred from test targets)
Test files present
FindingsCONCERN: CONCERN: CONCERN: Mockito is used to mock CONCERN: All five CONCERN: CONCERN: No test verifies that CONCERN: No test verifies that CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. The diff is entirely test code ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/com/diamonddagger590/mccore/builder/item/BaseItemBuilderMockBukkitTest.java`:
- Around line 301-333: Extend SetItemDamageTests with a test covering a negative
value passed to ItemBuilder.setItemDamage, asserting the documented behavior and
that the call completes without error. Follow the existing zero-damage test
structure and naming conventions, using a damageable DIAMOND_SWORD builder.
- Around line 80-95: Strengthen the affected tests so their assertions verify
the behavior promised by each `@DisplayName`, not merely absence of exceptions or
object identity. In addEnchantment_appliesStoredEnchantment_whenEnchantedBook
and addEnchantment_preservesExistingStoredEnchantments_whenAddingNew, inspect
EnchantmentStorageMeta.hasStoredEnchant for the expected stored enchantments.
Update the tests covering unchanged enchantments, no-change operations, and
damage clamping to assert the resulting state and clamped value. In the
copy-constructor test, verify the copied display name, placeholders, item flags,
and other configured state in addition to its type.
🪄 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: 5823f5b9-dcea-4021-b2f4-7b121a37d2e6
📒 Files selected for processing (3)
src/test/java/com/diamonddagger590/mccore/builder/item/BaseItemBuilderMockBukkitTest.javasrc/test/java/com/diamonddagger590/mccore/builder/item/impl/SkullBuilderTest.javasrc/test/java/com/diamonddagger590/mccore/builder/item/impl/SpawnerBuilderTest.java
| @Test | ||
| @DisplayName("Given an enchanted book, when addEnchantment is called, then stored enchantment is applied without error") | ||
| void addEnchantment_appliesStoredEnchantment_whenEnchantedBook() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.ENCHANTED_BOOK); | ||
|
|
||
| assertDoesNotThrow(() -> builder.addEnchantment(Enchantment.SHARPNESS, 5)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an enchanted book with existing stored enchantment, when addEnchantment is called, then both are applied without error") | ||
| void addEnchantment_preservesExistingStoredEnchantments_whenAddingNew() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.ENCHANTED_BOOK); | ||
| builder.addEnchantment(Enchantment.SHARPNESS, 3); | ||
|
|
||
| assertDoesNotThrow(() -> builder.addEnchantment(Enchantment.UNBREAKING, 2)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Several @DisplayNames promise verified behavior that the assertions never check.
These tests only assert "no exception thrown" or reference-sameness, but their names/descriptions claim to verify the actual resulting state:
- Lines 80-86 / 88-95:
@DisplayNamesays the (stored) enchantment "is applied", but onlyassertDoesNotThrowis checked — a brokenaddEnchantmentonENCHANTED_BOOKwould still pass. Verify viaEnchantmentStorageMeta.hasStoredEnchant(...). - Lines 127-135:
@DisplayNamesays "returns builder unchanged", but onlyassertSame(builder, result)(chaining) is checked, not that enchantments are actually unaffected. - Lines 285-290:
@DisplayNamesays "completes without error and no change", but no state is compared before/after. - Lines 312-317:
@DisplayNamesays "clamps to max without error", but the clamped damage value is never asserted. - Lines 585-598:
@DisplayNamesays "original state is preserved on the copy", but onlycopy.getType()is checked —setDisplayName,addPlaceholder, andaddItemFlageffects are set up but never verified oncopy.
These gaps mean a real regression in enchantment application, damage clamping, or copy-state preservation would not be caught by these tests.
🧪 Example fix for the copy-constructor test
ItemStack stack = original.asItemStack();
ItemBuilder copy = ItemBuilder.from(stack);
assertNotNull(copy);
assertEquals(Material.DIAMOND, copy.getType());
+ assertEquals("Original Name", copy.getPlainName());
+ assertTrue(copy.asItemStack().getItemMeta().hasItemFlag(org.bukkit.inventory.ItemFlag.HIDE_ENCHANTS));Also applies to: 127-135, 285-290, 312-317, 585-598
🤖 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/com/diamonddagger590/mccore/builder/item/BaseItemBuilderMockBukkitTest.java`
around lines 80 - 95, Strengthen the affected tests so their assertions verify
the behavior promised by each `@DisplayName`, not merely absence of exceptions or
object identity. In addEnchantment_appliesStoredEnchantment_whenEnchantedBook
and addEnchantment_preservesExistingStoredEnchantments_whenAddingNew, inspect
EnchantmentStorageMeta.hasStoredEnchant for the expected stored enchantments.
Update the tests covering unchanged enchantments, no-change operations, and
damage clamping to assert the resulting state and clamped value. In the
copy-constructor test, verify the copied display name, placeholders, item flags,
and other configured state in addition to its type.
| @Nested | ||
| @DisplayName("setItemDamage") | ||
| class SetItemDamageTests { | ||
|
|
||
| @Test | ||
| @DisplayName("Given a damageable item, when setItemDamage is called, then completes without error") | ||
| void setItemDamage_completesWithoutError_whenDamageableItem() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.DIAMOND_SWORD); | ||
| assertDoesNotThrow(() -> builder.setItemDamage(100)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given damage exceeding max durability, when setItemDamage is called, then clamps to max without error") | ||
| void setItemDamage_clampsToMax_whenExceedingDurability() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.DIAMOND_SWORD); | ||
| assertDoesNotThrow(() -> builder.setItemDamage(Material.DIAMOND_SWORD.getMaxDurability() + 100)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given zero damage, when setItemDamage is called, then completes without error") | ||
| void setItemDamage_completesWithoutError_whenZeroDamage() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.DIAMOND_SWORD); | ||
| assertDoesNotThrow(() -> builder.setItemDamage(0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given setItemDamage, when called, then returns builder for chaining") | ||
| void setItemDamage_returnsSelf_forChaining() { | ||
| ItemBuilder builder = ItemBuilder.from(ItemType.DIAMOND_SWORD); | ||
| ItemBuilder result = builder.setItemDamage(50); | ||
| assertSame(builder, result); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing negative-value edge case for setItemDamage.
This nested class covers positive, max-exceeding, and zero damage, but not a negative value. Coding guidelines require zero/negative numeric-input coverage for new tests.
As per coding guidelines, "Cover edge cases in tests: null inputs, empty collections, zero/negative numeric inputs, and max/limit values."
🤖 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/com/diamonddagger590/mccore/builder/item/BaseItemBuilderMockBukkitTest.java`
around lines 301 - 333, Extend SetItemDamageTests with a test covering a
negative value passed to ItemBuilder.setItemDamage, asserting the documented
behavior and that the call completes without error. Follow the existing
zero-damage test structure and naming conventions, using a damageable
DIAMOND_SWORD builder.
Source: Coding guidelines
2 participants
Summary
BaseItemBuilderMockBukkitTestwith 34 tests covering enchantment addition/removal,getPlainName,getPlainLore,setColor,setItemDamage,isEdible, builder conversions (success paths),withType,setTrim, copy constructor, and fluent API chainingSkullBuilderTestwith 5 new tests coveringwithAudience(Audience)with/without UUID,hideSkullDynamicToolTipmerge branch (existing tooltip data), and build verification with UUID and name+url texturesSpawnerBuilderTestwith 5 new tests exercising positive count/delay/range code paths and zero/negative count guard branchesCoverage Impact
BaseItemBuilderSkullBuilderSpawnerBuilderSpawnerBuilder coverage is unchanged because MockBukkit does not fully implement
BlockStateMeta/CreatureSpawner, so property-value assertions are not possible — tests useassertDoesNotThrowto exercise the code paths.Notes
assertDoesNotThrowpatterns where MockBukkit'sDataComponentTypessupport is incomplete (enchantments, color, damage, trim)DataComponentTypesassertions where MockBukkit supports them (PROFILE, TOOLTIP_DISPLAY, ITEM_NAME, CUSTOM_NAME, LORE)Test plan
./gradlew testgreen🤖 Generated with Claude Code
https://claude.ai/code/session_013TmAF3ehyVWsALSN2Ua7yY
Generated by Claude Code
Summary by CodeRabbit