[Tests] Add unit tests for external plugin hooks - #83
Conversation
Add comprehensive test suites for CoreItemsAdderHook, CoreNexoHook, CoreModelEngineHook, and CoreMythicMobsHook. Tests cover item/block/entity detection, model resolution, drops, block placement/removal, naming fallbacks, and playBlockDropEffects. Uses Unsafe.allocateInstance to bypass PluginHook constructors and reflection to work around unmockable Kotlin classes in Nexo and MythicMobs. Adds testImplementation dependencies for all four hook libraries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L9jjQTGj22zgVWetvZMtNn
📝 WalkthroughWalkthroughThe pull request adds test-scoped external API dependencies and introduces JUnit 5 coverage for ItemsAdder, Nexo, MythicMobs, and ModelEngine hook behavior. ChangesExternal hook test coverage
Estimated code review effort: 4 (Complex) | ~45 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 |
|
Claude finished @DiamondDagger590's task in 4m 30s —— View job Claude PR Review — 0 important, 2 nitsSummary: This PR is test-only — four new JUnit test classes (~1557 lines) covering No blocking issues found. Nits (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/test/java/com/diamonddagger590/mccore/external/itemsadder/CoreItemsAdderHookTest.java (2)
341-353: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVanilla-drops tests assert only collection size, not delegation. Both tests stub
block.getDrops(tool, entity)and then assertresult.size() == 1, which passes for any single-element list and does not prove the hook forwarded the vanilla drops.
src/test/java/com/diamonddagger590/mccore/external/itemsadder/CoreItemsAdderHookTest.java#L341-L353: replaceassertEquals(1, result.size())withassertEquals(List.copyOf(blockDrops), result).src/test/java/com/diamonddagger590/mccore/external/nexo/CoreNexoHookTest.java#L238-L250: replaceassertEquals(1, result.size())withassertEquals(List.copyOf(blockDrops), result).🤖 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/external/itemsadder/CoreItemsAdderHookTest.java` around lines 341 - 353, The vanilla-drops tests verify only list size instead of delegation. In CoreItemsAdderHookTest.java lines 341-353 and CoreNexoHookTest.java lines 238-250, update the assertions in the corresponding vanilla block drop test methods to compare the returned result with List.copyOf(blockDrops), preserving the existing stubbing and setup.
50-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the static mocks defensively in teardown.
customStackMock.close()is still anAutoCloseable.close()path, so if it throws beforecustomBlockMock.close()runs, the block mock remains registered and can poison later tests with “static mocking already registered”. Use a null-safe/clean fallback close sequence or switch to try-with-resources per static mock.🤖 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/external/itemsadder/CoreItemsAdderHookTest.java` around lines 50 - 65, Update CoreItemsAdderHookTest.tearDown to close customStackMock and customBlockMock defensively so one close failure cannot prevent the other mock from being released; use a null-safe cleanup sequence that preserves cleanup of both static mocks.src/test/java/com/diamonddagger590/mccore/external/nexo/CoreNexoHookTest.java (1)
69-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile fixed-depth superclass hop for
itemID.
getSuperclass().getSuperclass()hard-codes the Nexo class-hierarchy depth; any change inNoteBlockMechanic's ancestry (or inMechanic's field placement) turns this into aNoSuchFieldExceptionthat is unrelated to the behavior under test. Walk the hierarchy instead.♻️ Proposed hierarchy walk
- Field itemIdField = mechanic.getClass().getSuperclass().getSuperclass().getDeclaredField("itemID"); - itemIdField.setAccessible(true); - itemIdField.set(mechanic, itemId); + Field itemIdField = null; + for (Class<?> current = mechanic.getClass(); current != null && itemIdField == null; current = current.getSuperclass()) { + try { + itemIdField = current.getDeclaredField("itemID"); + } catch (NoSuchFieldException ignored) { + // keep walking up the hierarchy + } + } + if (itemIdField == null) { + throw new NoSuchFieldException("itemID not found on " + mechanic.getClass()); + } + itemIdField.setAccessible(true); + itemIdField.set(mechanic, itemId);🤖 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/external/nexo/CoreNexoHookTest.java` around lines 69 - 80, Update createMechanicWithItemId to locate the itemID field by walking the superclass hierarchy from mechanic.getClass(), rather than using the fixed getSuperclass().getSuperclass() chain. Continue searching until the field is found, then make it accessible and assign itemId while preserving the existing mechanic allocation and return behavior.src/test/java/com/diamonddagger590/mccore/external/mythicmobs/CoreMythicMobsHookTest.java (1)
175-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the null-
getDisplayName()case.
MythicMob.getDisplayName()can returnnullfor mobs registered without a display name, which is a distinct branch from "mob type not registered". Only the present and absent-mob cases are covered here. 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/external/mythicmobs/CoreMythicMobsHookTest.java` around lines 175 - 199, Add a test alongside returnsDisplayName_whenMobTypeHasDisplayName and returnsRawId_whenMobTypeNotRegistered that mocks a registered MythicMob whose getDisplayName() returns null, then verifies hook.entityName returns the raw entity ID. Keep this distinct from the Optional.empty() unregistered-mob case.Source: Coding guidelines
src/test/java/com/diamonddagger590/mccore/external/modelengine/CoreModelEngineHookTest.java (2)
151-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing empty-models edge case.
A
ModeledEntitypresent with an emptygetModels()map is a distinct branch from anullmodeled entity, and it decides whether the hook returnsOptional.of(Set.of())orOptional.empty(). 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/external/modelengine/CoreModelEngineHookTest.java` around lines 151 - 167, Add a test in CoreModelEngineHookTest covering a non-null ModeledEntity whose getModels() returns an empty map, and assert the expected Optional result for hook.entityModels. Keep the existing modeled-entity test unchanged and use the same KNOWN_UUID and ModelEngineAPI mocking setup.Source: Coding guidelines
100-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMap key and blueprint name are identical, so the assertion cannot pin down which one the hook matches on.
Both tests stub
getModels()with a key equal toblueprint.getName(). Use a distinct key (e.g. key"main", blueprint name"dragon") so the test actually documents the matching contract.🤖 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/external/modelengine/CoreModelEngineHookTest.java` around lines 100 - 136, Update the modeled-entity fixtures in returnsTrue_whenEntityHasMatchingModel and returnsFalse_whenEntityHasNonMatchingModel so the getModels() map key differs from blueprint.getName(), using a distinct key such as “main” while preserving the dragon/golem blueprint names and assertions. This ensures the tests verify matching against the blueprint name rather than accidentally validating identical map keys.
🤖 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.
Nitpick comments:
In
`@src/test/java/com/diamonddagger590/mccore/external/itemsadder/CoreItemsAdderHookTest.java`:
- Around line 341-353: The vanilla-drops tests verify only list size instead of
delegation. In CoreItemsAdderHookTest.java lines 341-353 and
CoreNexoHookTest.java lines 238-250, update the assertions in the corresponding
vanilla block drop test methods to compare the returned result with
List.copyOf(blockDrops), preserving the existing stubbing and setup.
- Around line 50-65: Update CoreItemsAdderHookTest.tearDown to close
customStackMock and customBlockMock defensively so one close failure cannot
prevent the other mock from being released; use a null-safe cleanup sequence
that preserves cleanup of both static mocks.
In
`@src/test/java/com/diamonddagger590/mccore/external/modelengine/CoreModelEngineHookTest.java`:
- Around line 151-167: Add a test in CoreModelEngineHookTest covering a non-null
ModeledEntity whose getModels() returns an empty map, and assert the expected
Optional result for hook.entityModels. Keep the existing modeled-entity test
unchanged and use the same KNOWN_UUID and ModelEngineAPI mocking setup.
- Around line 100-136: Update the modeled-entity fixtures in
returnsTrue_whenEntityHasMatchingModel and
returnsFalse_whenEntityHasNonMatchingModel so the getModels() map key differs
from blueprint.getName(), using a distinct key such as “main” while preserving
the dragon/golem blueprint names and assertions. This ensures the tests verify
matching against the blueprint name rather than accidentally validating
identical map keys.
In
`@src/test/java/com/diamonddagger590/mccore/external/mythicmobs/CoreMythicMobsHookTest.java`:
- Around line 175-199: Add a test alongside
returnsDisplayName_whenMobTypeHasDisplayName and
returnsRawId_whenMobTypeNotRegistered that mocks a registered MythicMob whose
getDisplayName() returns null, then verifies hook.entityName returns the raw
entity ID. Keep this distinct from the Optional.empty() unregistered-mob case.
In
`@src/test/java/com/diamonddagger590/mccore/external/nexo/CoreNexoHookTest.java`:
- Around line 69-80: Update createMechanicWithItemId to locate the itemID field
by walking the superclass hierarchy from mechanic.getClass(), rather than using
the fixed getSuperclass().getSuperclass() chain. Continue searching until the
field is found, then make it accessible and assign itemId while preserving the
existing mechanic allocation and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 92b8f099-0fda-4a85-94d1-ce437aa6f353
📒 Files selected for processing (5)
build.gradle.ktssrc/test/java/com/diamonddagger590/mccore/external/itemsadder/CoreItemsAdderHookTest.javasrc/test/java/com/diamonddagger590/mccore/external/modelengine/CoreModelEngineHookTest.javasrc/test/java/com/diamonddagger590/mccore/external/mythicmobs/CoreMythicMobsHookTest.javasrc/test/java/com/diamonddagger590/mccore/external/nexo/CoreNexoHookTest.java
2 participants
Summary
CoreItemsAdderHook,CoreNexoHook,CoreModelEngineHook, andCoreMythicMobsHooktestImplementationdependencies for ItemsAdder, Nexo, MythicMobs, and ModelEngine APIs, plustestRuntimeOnlyfor Kotlin stdlib (required by Nexo's Kotlin classes)Details
Test approach
All four hook classes extend
PluginHook<CorePlugin>, whose constructor requires a runningCorePlugin. Tests useUnsafe.allocateInstance()to bypass the constructor and create hook instances without a server.ItemsAdder (37 tests, ~83% line coverage): Uses
MockedStatic<CustomStack>andMockedStatic<CustomBlock>to mock the ItemsAdder static API. Covers items, blocks, drops, removal, naming (Adventure Component → legacy → formatted ID fallback chain), andplayBlockDropEffects.Nexo (23 tests, ~50% line coverage): Uses
MockedStatic<NexoBlocks>only.NexoItemscannot be statically mocked because its method signatures referenceItemBuilder, whose static initializer requiresNexoPlugin. Similarly,CustomBlockMechanic(Kotlin) cannot be Mockito-mocked, so instances are created viaUnsafe.allocateInstanceonNoteBlockMechanicwith fields set via reflection.BlockSounds(Kotlin final class) is also Unsafe-allocated for theplayBlockDropEffectssound path test.MythicMobs (12 tests, 100% line coverage): Uses
MockedStatic<MythicBukkit>with mockMobExecutor. All entity detection, model resolution, and display name resolution paths are covered.ModelEngine (11 tests, 100% line coverage): Uses
MockedStatic<ModelEngineAPI>. All entity detection, blueprint lookup, model key resolution, and entity naming paths are covered.Known limitations
item,isItem,isItemOfType,itemModels,itemName) are untested due to theNexoItems→ItemBuilder→NexoPluginstatic initialization chain being fundamentally unmockabledropshappy path (Player entity breaking a custom block with a Breakable mechanic) is untested becauseBreakablelikely has the same ItemBuilder dependency chainTest plan
./gradlew test→ BUILD SUCCESSFUL)Generated by Claude Code
Summary by CodeRabbit