[Unit Tests] Add quest objective type tests for advancement, enchant, and loadout equip - #334
[Unit Tests] Add quest objective type tests for advancement, enchant, and loadout equip#334DiamondDagger590 wants to merge 1 commit into
Conversation
…equip Expand test coverage for three quest objective types: - AdvancementCompleteObjectiveTypeTest: add describeObjective tests covering ANY, SINGLE, and MULTI locale key branches with proper localization manager stubs and placeholder verification - EnchantItemObjectiveTypeTest: add describeObjective tests for all seven filter combinations (any, single item, multi item, single enchantment, multi enchantment, single both, multi both) - LoadoutEquipObjectiveTypeTest: expand from 5 to 15 tests covering identity, canProcess, parseConfig, and processProgress with proper AbilityRegistry setup for ability type/key filter matching - LoadoutEquipQuestContextTest: new test class covering getAbilityKey for equip/swap/unequip events and getPlayerUUID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDTR3sDK7r7ffqVdqgVnSR
|
Claude finished @DiamondDagger590's task in 5m 10s —— View job Review: 2 important, 3 nitsTest-only PR (no Important findings:
Nits
Inline comments posted on the two Important findings above. |
📝 WalkthroughWalkthroughThe PR adds localized description tests for advancement and enchantment objectives. It also expands loadout objective tests for configuration, context handling, ability filters, and progress, and adds context extraction tests. ChangesQuest objective tests
Estimated code review effort: 3 (Moderate) | ~20 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| localizationManager = RegistryAccess.registryAccess() | ||
| .registry(RegistryKey.MANAGER) | ||
| .manager(McRPGManagerKey.LOCALIZATION); | ||
| lenient().when(localizationManager.getLocalizedMessage(any(McRPGPlayer.class), any(Route.class), anyMap())) |
There was a problem hiding this comment.
Important — testing: The blanket lenient().when(localizationManager.getLocalizedMessage(any(McRPGPlayer.class), any(Route.class), anyMap())).thenReturn("localized text") stub makes every describeObjective in this class indistinguishable — all 7 DescribeObjective tests below (lines 313-415) only assert assertNotNull/assertFalse(isEmpty())/contains("\n"), so a regression that swaps SINGLE_ITEM for SINGLE_ENCHANTMENT, drops a placeholder, or picks the wrong locale key would still pass every test.
EnchantItemObjectiveType.describeObjective (src/main/java/.../EnchantItemObjectiveType.java:169-270) branches into ANY / single-item / multi-item / single-enchantment / multi-enchantment / both — real logic worth locking down. The sibling AdvancementCompleteObjectiveTypeTest added in this same PR shows the stronger pattern: stub each LocalizationKey individually and verify() the exact key + placeholder map passed. Recommend applying that same pattern here instead of the blanket stub.
|
|
||
| Ability passiveAbility = mock(Ability.class); | ||
| when(passiveAbility.getAbilityType()).thenReturn(AbilityType.PASSIVE); | ||
| NamespacedKey passiveKey = new NamespacedKey("mcrpg", "test_passive"); |
There was a problem hiding this comment.
Important — testing: This PR adds thorough parseConfig/processProgress coverage but no test for LoadoutEquipObjectiveType.describeObjective (src/main/java/.../LoadoutEquipObjectiveType.java:134-161), which has real branching: specific-ability name resolution, a switch over PASSIVE/ACTIVE/INNATE each hitting a distinct LocalizationKey, a default warning-log fallthrough, and a final ANY fallback. None of it is exercised, so a regression swapping any of the four locale keys or breaking the fallthrough would go undetected. Both sibling objective types touched in this PR got dedicated describeObjective coverage — this one should too.
Also worth adding: processProgress for an ability key that isn't registered in AbilityRegistry currently throws AbilityNotRegisteredException (src/main/java/us/eunoians/mcrpg/ability/AbilityRegistry.java:234-240) — every existing test pre-registers the ability via mockAbilityRegistry(...), so this reachable failure path (e.g. a stale ability key from an event) has no test asserting the current behavior.
![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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/LoadoutEquipObjectiveTypeTest.java-77-180 (1)
77-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
parseConfigbehavior for empty and malformed filters.Line 84 only verifies that
parseConfigreturns an object. It does not verify that an empty configuration accepts a registered ability.Add a progress assertion for the empty configuration. Add cases for an invalid
"ability-type"and an invalid"ability"value. Assert that both configured objectives return zero progress for a registered ability.As per coding guidelines, "Cover relevant edge cases, including ... malformed or missing configuration 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/us/eunoians/mcrpg/quest/objective/type/builtin/LoadoutEquipObjectiveTypeTest.java` around lines 77 - 180, Expand LoadoutEquipObjectiveTypeTest.parseConfig_noFilters_createsEmptyFilter to register an ability and assert processProgress returns one for the empty configuration. Add tests covering invalid ability-type and invalid ability values; register a matching ability/context and assert each configured objective returns zero progress, using the existing mockAbilityRegistry and mockContextWithAbility helpers.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.
Inline comments:
In
`@src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.java`:
- Around line 265-287: Strengthen localization verification across the affected
tests: in
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.java
lines 265-287, verify the multi header route and capture/assert all formatted
advancement placeholder maps; in
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java
lines 51-52, remove the broad default localization response; and in lines
313-415, assert each expected localization route plus its count, item, and
enchantment placeholder map.
---
Other comments:
In
`@src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/LoadoutEquipObjectiveTypeTest.java`:
- Around line 77-180: Expand
LoadoutEquipObjectiveTypeTest.parseConfig_noFilters_createsEmptyFilter to
register an ability and assert processProgress returns one for the empty
configuration. Add tests covering invalid ability-type and invalid ability
values; register a matching ability/context and assert each configured objective
returns zero progress, using the existing mockAbilityRegistry and
mockContextWithAbility helpers.
🪄 Autofix
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: 8e08531c-8809-49e1-a60a-3a056e2ec7cd
📒 Files selected for processing (4)
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.javasrc/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.javasrc/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/LoadoutEquipObjectiveTypeTest.javasrc/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/LoadoutEquipQuestContextTest.java
| @Test | ||
| @DisplayName("uses MULTI locale keys when multiple advancements configured") | ||
| void describeObjective_usesMultiKeys_whenMultipleAdvancements(McRPGPlayer player) { | ||
| Section section = mock(Section.class); | ||
| when(section.contains("advancements")).thenReturn(true); | ||
| when(section.getStringList("advancements")).thenReturn(List.of( | ||
| "minecraft:story/iron_tools", | ||
| "minecraft:nether/find_fortress" | ||
| )); | ||
| AdvancementCompleteObjectiveType configured = type.parseConfig(section); | ||
|
|
||
| when(localizationManager.getLocalizedMessage(eq(player), | ||
| eq(LocalizationKey.QUEST_OBJECTIVE_ADVANCEMENT_COMPLETE_MULTI_HEADER), anyMap())) | ||
| .thenReturn("Complete 2 advancements"); | ||
| when(localizationManager.getLocalizedMessage(eq(player), | ||
| eq(LocalizationKey.QUEST_OBJECTIVE_ADVANCEMENT_COMPLETE_MULTI_ITEM), anyMap())) | ||
| .thenReturn(" - advancement"); | ||
|
|
||
| String description = configured.describeObjective(player, 2); | ||
| assertNotNull(description); | ||
| assertTrue(description.startsWith("Complete 2 advancements")); | ||
| assertTrue(description.contains("\n")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify localization routes and placeholder maps, not only output shape. The new tests can pass when a description uses the wrong localization route or omits filter data.
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.java#L265-L287: verify the multi header and capture all formatted advancement placeholders.src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java#L51-L52: remove the broad default response for cases that need route-specific verification.src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java#L313-L415: assert each expected route, count, item, and enchantment placeholder map.
📍 Affects 2 files
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.java#L265-L287(this comment)src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java#L51-L52src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java#L313-L415
🤖 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/AdvancementCompleteObjectiveTypeTest.java`
around lines 265 - 287, Strengthen localization verification across the affected
tests: in
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/AdvancementCompleteObjectiveTypeTest.java
lines 265-287, verify the multi header route and capture/assert all formatted
advancement placeholder maps; in
src/test/java/us/eunoians/mcrpg/quest/objective/type/builtin/EnchantItemObjectiveTypeTest.java
lines 51-52, remove the broad default localization response; and in lines
313-415, assert each expected localization route plus its count, item, and
enchantment placeholder map.
2 participants
Summary
describeObjectivetests covering the ANY, SINGLE, and MULTI locale key branches with proper localization manager stubs and placeholder verification (e.g.extractDisplayNamestrips namespace and replaces underscores)describeObjectivetests for all filter combinations — any, single item, multi item, single enchantment, multi enchantment, single both, and multi both — with blanket localization manager stubAbilityRegistryin test setup to support ability lookup inprocessProgressgetAbilityKeyfor EQUIP, SWAP, and UNEQUIP (throwsNoSuchElementException) events, plusgetPlayerUUIDTest plan
./gradlew testcompletes successfullyGenerated by Claude Code
Summary by CodeRabbit