[Unit Tests] Add McRPGLocalizationManager locale chain, date formatting, and utility tests - #332
[Unit Tests] Add McRPGLocalizationManager locale chain, date formatting, and utility tests#332DiamondDagger590 wants to merge 2 commits into
Conversation
…ationManager Expand McRPGLocalizationManagerTest from 20 to 35 tests covering: - getLocaleChain: all 3 LocaleSetting paths (CLIENT_LOCALE, SERVER_LOCALE, SpecificLocaleSetting) with and without Bukkit player available (7 tests) - getServerDefaultLocale: locale chain head retrieval (1 test) - getRegisteredLocales: empty, multiple locales, and defensive copy (3 tests) - formatDisplayDate: English and German locale-aware date formatting (2 tests) - getPaletteReplacements: map content and size verification (2 tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015UdHRdCMsutDyGRaqUcTKu
The comment claimed the test map mirrored config.yml defaults, but it is a representative subset used to exercise the replacement mechanism. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015UdHRdCMsutDyGRaqUcTKu
|
Claude finished @DiamondDagger590's task in 4m 37s —— View job Review complete — 0 important, 2 nitsSummary: This PR is a test-only change (no Nits (2)
Both align with the NIT-level findings the PR description already self-reported (reflection fragility, untested fallback branch) — no new IMPORTANT issues found. |
📝 WalkthroughWalkthroughThe localization manager test suite now covers locale-chain construction, server defaults, registered locales, locale-specific date formatting, and palette replacement mappings. ChangesLocalization manager test coverage
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 |
![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 (3)
src/test/java/us/eunoians/mcrpg/localization/McRPGLocalizationManagerTest.java-414-426 (1)
414-426: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest defensive behavior, not only object identity.
assertNotSamepasses if the method returns a new mutable set. It does not verify that later changes tolocalizationsMapcannot affectresult, or that callers cannot mutateresult.Modify
localizationsMapafter the call and assert thatresultremains unchanged. Also assert thatresult.add(...)throwsUnsupportedOperationException.🤖 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/localization/McRPGLocalizationManagerTest.java` around lines 414 - 426, Strengthen getRegisteredLocales_returnedSetIsDefensiveCopy by verifying behavior rather than only identity: mutate localizationsMap after obtaining result and assert result is unchanged, then assert that attempting result.add(...) throws UnsupportedOperationException.src/test/java/us/eunoians/mcrpg/localization/McRPGLocalizationManagerTest.java-496-501 (1)
496-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert both mappings for every named palette role.
A size of 20 does not prove that all ten required roles have opening and closing mappings. A missing role plus an unrelated mapping still passes this test.
Assert the presence of
<role>and</role>for each expected palette role.🤖 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/localization/McRPGLocalizationManagerTest.java` around lines 496 - 501, Update getPaletteReplacements_hasOpenAndCloseTags to verify both opening and closing tag mappings for each of the ten expected palette roles, rather than relying only on the total size assertion. Retain the existing configured-palette setup and assert presence of every corresponding <role> and </role> key.src/test/java/us/eunoians/mcrpg/localization/McRPGLocalizationManagerTest.java-249-254 (1)
249-254: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that each expected locale chain terminates at English.
These tests do not assert that the final
Locale.ENGLISHnode has no successor. A chain with an unintended extra fallback locale passes these assertions.Add
assertFalse(...hasNext())for the final English node in each complete-chain test.Also applies to: 262-266, 279-283, 296-300, 327-331, 346-352
🤖 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/localization/McRPGLocalizationManagerTest.java` around lines 249 - 254, Update each complete locale-chain test in McRPGLocalizationManagerTest, including the cases around the German-to-English assertions and the listed additional chains, to assert that the final Locale.ENGLISH node’s hasNext() returns false. Keep the existing locale-order assertions unchanged and apply the termination assertion to every expected English endpoint.
🤖 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/localization/McRPGLocalizationManagerTest.java`:
- Around line 414-426: Strengthen
getRegisteredLocales_returnedSetIsDefensiveCopy by verifying behavior rather
than only identity: mutate localizationsMap after obtaining result and assert
result is unchanged, then assert that attempting result.add(...) throws
UnsupportedOperationException.
- Around line 496-501: Update getPaletteReplacements_hasOpenAndCloseTags to
verify both opening and closing tag mappings for each of the ten expected
palette roles, rather than relying only on the total size assertion. Retain the
existing configured-palette setup and assert presence of every corresponding
<role> and </role> key.
- Around line 249-254: Update each complete locale-chain test in
McRPGLocalizationManagerTest, including the cases around the German-to-English
assertions and the listed additional chains, to assert that the final
Locale.ENGLISH node’s hasNext() returns false. Keep the existing locale-order
assertions unchanged and apply the termination assertion to every expected
English endpoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8f2c2dec-277b-4415-8b92-fd23deb83f7e
📒 Files selected for processing (1)
src/test/java/us/eunoians/mcrpg/localization/McRPGLocalizationManagerTest.java
2 participants
Summary
McRPGLocalizationManagerTestfrom 20 to 35 tests (15 new tests across 5 nested classes)getLocaleChain()for all 3LocaleSettingpaths:CLIENT_LOCALE,SERVER_LOCALE, andSpecificLocaleSetting, each with and without a Bukkit player available (7 tests)getServerDefaultLocale(),getRegisteredLocales()(empty/multiple/defensive-copy),formatDisplayDate()(English + German locale-aware formatting), andgetPaletteReplacements()(content + size)config.ymldefaultsTest plan
./gradlew verifiedShadowJar(clean + test + build)McRPGLocalizationManagerTestpassaddPaletteEntryfallback branch that is unreachable via the current test setup pattern)https://claude.ai/code/session_015UdHRdCMsutDyGRaqUcTKu
Generated by Claude Code
Summary by CodeRabbit