[Tests] Add unit tests for PlayerManager, ChatResponseManager, ChatResponse, LocalizationManager#37
[Tests] Add unit tests for PlayerManager, ChatResponseManager, ChatResponse, LocalizationManager#37DiamondDagger590 wants to merge 1 commit into
Conversation
…LocalizationManager - PlayerManager: 0% → 100% (16/16 lines) — add/remove/get/lock/getAllPlayers/getAllBukkitPlayers - ChatResponseManager: 0% → 100% (20/20 lines) — pending response lifecycle, replacement expiry - ChatResponse: 0% → 100% (4/4 lines) — UUID accessor and abstract contract - ChatResponseExpireTask: 0% → 33.3% (4/12 lines, covered transitively) - LocalizationManager: 0% → 90.3% (168/186 lines) — locale chain resolution, fallback, string/list/section/component variants, placeholders, postProcessResolvedString hook, audience resolution via PlayerManager - Added Mockito 5.14.2 as testImplementation dependency Overall project coverage: 31.9% → 38.5% (+6.6pp, 268 new lines covered) https://claude.ai/code/session_01VdgvpNjKTBC7y2EPrn3bi5
WalkthroughAdds Mockito as a test dependency in ChangesUnit Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Checklist EvaluationCoverage CompletenessThe diff adds three test files covering ChatResponseManager — The expiry timer logic (the LocalizationManager — PlayerManager —
TimeProvider Usage
No test modifies global MockBukkit Usage
However: Bukkit-Dependent vs. Pure-Java Separation
Framework Test Quality
All test methods have Naming convention ( Shared Fixtures
FindingsCONCERN: Timer expiry logic in CONCERN: Two test methods have no assertions and no CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: Test method names in SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. The diff contains exclusively test infrastructure: new JUnit/Mockito test classes for Checklist sweep:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@build.gradle.kts`:
- Around line 68-69: Update the mockitoVersion variable in build.gradle.kts from
"5.14.2" to "5.23.0" to address known issues with the current version including
initialization failures with inline Byte Buddy mock maker on Java 17+ and
superclass constructor mocking problems. The mockitoVersion variable on line 68
should be changed to the new version string, which will automatically apply to
the testImplementation dependency on line 69 that uses this variable.
In `@src/test/java/com/diamonddagger590/mccore/chat/ChatResponseManagerTest.java`:
- Around line 65-85: Replace the Mockito mocking approach in the setUp() method
with MockBukkit. Remove the mockStatic(Bukkit.class) call and the manual Mockito
mocks for BukkitScheduler and Server. Instead, call MockBukkit.mock() at the
beginning of setUp() to initialize the MockBukkit environment, which will
provide real implementations like ServerMock that integrate properly with Bukkit
singletons. Update the lines that were creating BukkitScheduler and Server mocks
to instead retrieve them from the MockBukkit environment (e.g., using
MockBukkit.getSchedulerMock() or similar accessor methods from the MockBukkit
API). Additionally, add a new `@AfterEach` method that calls MockBukkit.unmock()
to properly clean up the MockBukkit state after each test.
In `@src/test/java/com/diamonddagger590/mccore/chat/ChatResponseTest.java`:
- Around line 73-79: In the onExpire_tracksExpiration() test method, replace the
assertEquals(true, response.isExpired()) assertion with the more idiomatic
assertTrue(response.isExpired()). This provides clearer intent and is the
preferred pattern for testing boolean conditions in JUnit tests.
In
`@src/test/java/com/diamonddagger590/mccore/localization/LocalizationManagerTest.java`:
- Around line 135-140: Replace all instances of `mock(Player.class)` with
MockBukkit's `PlayerMock` implementation throughout the test file, including in
the createPlayerWithLocale method and at all other locations mentioned (lines
318, 341, 359, 428, etc.). When creating PlayerMock instances, use MockBukkit's
API to set the locale instead of Mockito's when() syntax, following the coding
guideline to use real MockBukkit implementations rather than Mockito mocks for
Bukkit classes where MockBukkit provides an implementation.
In `@src/test/java/com/diamonddagger590/mccore/player/PlayerManagerTest.java`:
- Around line 161-173: The test method
`getAllBukkitPlayers_returnsEmpty_whenNoBukkitPlayers` uses Mockito's `mock()`
to create test instances, but per coding guidelines, MockBukkit's real
implementations should be used instead of Mockito mocks for Bukkit classes.
Replace the Mockito mock approach with MockBukkit's PlayerMock when creating any
Bukkit Player instances in this test to ensure consistency with the codebase
standards.
- Around line 145-159: In the
getAllBukkitPlayers_returnsBukkitPlayers_whenPresent method, replace the Mockito
mock(Player.class) with MockBukkit's PlayerMock implementation. Instead of
creating a mocked Player object, instantiate a new PlayerMock object to ensure
consistency with the framework's test patterns and coding guidelines that
require using MockBukkit's real implementations for Bukkit classes.
🪄 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: 6bab201b-765d-47b6-9826-ada7759e4995
📒 Files selected for processing (5)
build.gradle.ktssrc/test/java/com/diamonddagger590/mccore/chat/ChatResponseManagerTest.javasrc/test/java/com/diamonddagger590/mccore/chat/ChatResponseTest.javasrc/test/java/com/diamonddagger590/mccore/localization/LocalizationManagerTest.javasrc/test/java/com/diamonddagger590/mccore/player/PlayerManagerTest.java
| val mockitoVersion = "5.14.2" | ||
| testImplementation("org.mockito:mockito-core:$mockitoVersion") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of Mockito and are there any known issues with version 5.14.2?
💡 Result:
As of June 14, 2026, the latest stable version of Mockito is 5.23.0 [1][2]. Regarding version 5.14.2, there are several known issues and reported behaviors: * Initialization Failures: Users have reported issues initializing the inline Byte Buddy mock maker in certain environments (e.g., Java 17), often resulting in a MockitoInitializationException [3]. This is frequently related to outdated Byte Buddy versions or agent attachment mechanisms in newer JDKs [3]. * Superclass Constructor Mocking: There have been reports that mocking the construction of a superclass does not behave as expected in version 5.14.2 compared to earlier versions [4]. * Self-Attaching Warnings: Users may encounter warnings regarding Mockito self-attaching to enable the inline mock maker [5]. While this is a known behavior of the inline mock maker, it has prompted discussions about better configuration practices to avoid build failures in future JDK releases [5]. * Initial Release Delay: Shortly after its initial tagging, there was a temporary issue where version 5.14.2 was not correctly published to the Maven Central repository, which was subsequently resolved [6][7]. Given that 5.14.2 was released in October 2024 [1][6], it is highly recommended to upgrade to the latest stable release (5.23.0) to benefit from numerous bug fixes, improvements, and compatibility updates [8][9].
Citations:
- 1: https://mvnrepository.com/artifact/org.mockito/mockito-core
- 2: https://github.com/mockito/mockito/
- 3: Could not initialize inline Byte Buddy mock maker. Java 17. Mockito 5.14.2 mockito/mockito#3564
- 4: Superclass Constructor Mocking not working in 5.14.2 mockito/mockito#3521
- 5: Mockito:5.14.2 Warning: Mockito is currently self-attaching... mockito/mockito#3512
- 6: https://github.com/mockito/mockito/releases/tag/v5.14.2
- 7: Missing 5.14.2 release mockito/mockito#3466
- 8: https://github.com/mockito/mockito/releases
- 9: https://github.com/mockito/mockito/releases/tag/v5.23.0
Update Mockito to version 5.23.0 from 5.14.2.
Version 5.14.2 is outdated and has known issues including initialization failures with the inline Byte Buddy mock maker (particularly on Java 17+), superclass constructor mocking problems, and self-attaching warnings. The latest stable release 5.23.0 includes bug fixes and compatibility improvements.
🤖 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 `@build.gradle.kts` around lines 68 - 69, Update the mockitoVersion variable in
build.gradle.kts from "5.14.2" to "5.23.0" to address known issues with the
current version including initialization failures with inline Byte Buddy mock
maker on Java 17+ and superclass constructor mocking problems. The
mockitoVersion variable on line 68 should be changed to the new version string,
which will automatically apply to the testImplementation dependency on line 69
that uses this variable.
| @BeforeEach | ||
| void setUp() { | ||
| mockPlugin = mock(CorePlugin.class); | ||
| TimeProvider timeProvider = mock(TimeProvider.class); | ||
| when(timeProvider.now()).thenReturn(Instant.ofEpochMilli(1000)); | ||
| when(mockPlugin.getTimeProvider()).thenReturn(timeProvider); | ||
|
|
||
| BukkitScheduler scheduler = mock(BukkitScheduler.class); | ||
| BukkitTask task = mock(BukkitTask.class); | ||
| when(task.getTaskId()).thenReturn(1); | ||
| when(scheduler.runTaskTimer(any(), any(Runnable.class), anyLong(), anyLong())).thenReturn(task); | ||
| when(scheduler.runTask(any(), any(Runnable.class))).thenReturn(task); | ||
|
|
||
| bukkitMock = mockStatic(Bukkit.class); | ||
| bukkitMock.when(Bukkit::getScheduler).thenReturn(scheduler); | ||
|
|
||
| Server server = mock(Server.class); | ||
| bukkitMock.when(Bukkit::getServer).thenReturn(server); | ||
|
|
||
| manager = new ChatResponseManager(mockPlugin); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider using MockBukkit instead of Mockito for Bukkit classes.
The test mocks BukkitScheduler (line 72) and Server (line 81) using Mockito, and uses mockStatic for the Bukkit singleton. As per coding guidelines, MockBukkit provides real implementations (e.g., ServerMock) that should be preferred over Mockito mocks. Consider using MockBukkit.mock() in @BeforeEach and MockBukkit.unmock() in @AfterEach instead of static mocking.
As per coding guidelines: "Do not use Mockito to mock a Bukkit class where MockBukkit already provides a real implementation (e.g., PlayerMock, ServerMock) — use the real MockBukkit implementation instead".
🤖 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/chat/ChatResponseManagerTest.java`
around lines 65 - 85, Replace the Mockito mocking approach in the setUp() method
with MockBukkit. Remove the mockStatic(Bukkit.class) call and the manual Mockito
mocks for BukkitScheduler and Server. Instead, call MockBukkit.mock() at the
beginning of setUp() to initialize the MockBukkit environment, which will
provide real implementations like ServerMock that integrate properly with Bukkit
singletons. Update the lines that were creating BukkitScheduler and Server mocks
to instead retrieve them from the MockBukkit environment (e.g., using
MockBukkit.getSchedulerMock() or similar accessor methods from the MockBukkit
API). Additionally, add a new `@AfterEach` method that calls MockBukkit.unmock()
to properly clean up the MockBukkit state after each test.
Source: Coding guidelines
| @Test | ||
| @DisplayName("Given a response, when onExpire called, then expiration is tracked") | ||
| void onExpire_tracksExpiration() { | ||
| TestChatResponse response = new TestChatResponse(UUID.randomUUID(), 30); | ||
| response.onExpire(); | ||
| assertEquals(true, response.isExpired()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer assertTrue over assertEquals(true, ...).
Line 78 uses assertEquals(true, response.isExpired()) which is less idiomatic than assertTrue(response.isExpired()).
♻️ Suggested improvement
- assertEquals(true, response.isExpired());
+ assertTrue(response.isExpired());🤖 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/chat/ChatResponseTest.java` around
lines 73 - 79, In the onExpire_tracksExpiration() test method, replace the
assertEquals(true, response.isExpired()) assertion with the more idiomatic
assertTrue(response.isExpired()). This provides clearer intent and is the
preferred pattern for testing boolean conditions in JUnit tests.
| private TestCorePlayer createPlayerWithLocale(Locale locale) { | ||
| UUID uuid = UUID.randomUUID(); | ||
| Player bukkitPlayer = mock(Player.class); | ||
| when(bukkitPlayer.locale()).thenReturn(locale); | ||
| return new TestCorePlayer(uuid, mockPlugin, bukkitPlayer); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use MockBukkit's PlayerMock instead of Mockito mock.
Line 137 mocks Player using Mockito. This pattern is repeated throughout the test file (lines 318, 341, 359, 428, etc.). According to coding guidelines, use MockBukkit's PlayerMock implementation instead for all Player instances.
♻️ Suggested improvement
Replace all mock(Player.class) calls with MockBukkit's PlayerMock:
- Player bukkitPlayer = mock(Player.class);
- when(bukkitPlayer.locale()).thenReturn(locale);
+ Player bukkitPlayer = new be.seeseemelk.mockbukkit.entity.PlayerMock(/* server, name, uuid */);
+ // PlayerMock has setLocale() or similar method to configure localeApply this pattern to all Player mock creation sites in the test file.
As per coding guidelines: "Do not use Mockito to mock a Bukkit class where MockBukkit already provides a real implementation (e.g., PlayerMock, ServerMock) — use the real MockBukkit implementation instead".
🤖 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/localization/LocalizationManagerTest.java`
around lines 135 - 140, Replace all instances of `mock(Player.class)` with
MockBukkit's `PlayerMock` implementation throughout the test file, including in
the createPlayerWithLocale method and at all other locations mentioned (lines
318, 341, 359, 428, etc.). When creating PlayerMock instances, use MockBukkit's
API to set the locale instead of Mockito's when() syntax, following the coding
guideline to use real MockBukkit implementations rather than Mockito mocks for
Bukkit classes where MockBukkit provides an implementation.
Source: Coding guidelines
| @DisplayName("Given players with Bukkit player available, when getAllBukkitPlayers, then returns bukkit players") | ||
| void getAllBukkitPlayers_returnsBukkitPlayers_whenPresent() { | ||
| UUID uuid = UUID.randomUUID(); | ||
| Player bukkitPlayer = mock(Player.class); | ||
|
|
||
| TestCorePlayer corePlayer = mock(TestCorePlayer.class); | ||
| when(corePlayer.getAsBukkitPlayer()).thenReturn(Optional.of(bukkitPlayer)); | ||
| when(corePlayer.getUUID()).thenReturn(uuid); | ||
|
|
||
| manager.addPlayer(corePlayer); | ||
|
|
||
| Set<Player> result = manager.getAllBukkitPlayers(); | ||
| assertEquals(1, result.size()); | ||
| assertTrue(result.contains(bukkitPlayer)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use MockBukkit's PlayerMock instead of Mockito mock.
Line 148 mocks Player using Mockito, but the coding guidelines require using MockBukkit's PlayerMock implementation instead. This ensures test fidelity and consistency with the framework's test patterns.
♻️ Suggested improvement
- Player bukkitPlayer = mock(Player.class);
+ Player bukkitPlayer = new be.seeseemelk.mockbukkit.entity.PlayerMock(/* appropriate constructor args */);As per coding guidelines: "Do not use Mockito to mock a Bukkit class where MockBukkit already provides a real implementation (e.g., PlayerMock, ServerMock) — use the real MockBukkit implementation instead".
🤖 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/player/PlayerManagerTest.java`
around lines 145 - 159, In the
getAllBukkitPlayers_returnsBukkitPlayers_whenPresent method, replace the Mockito
mock(Player.class) with MockBukkit's PlayerMock implementation. Instead of
creating a mocked Player object, instantiate a new PlayerMock object to ensure
consistency with the framework's test patterns and coding guidelines that
require using MockBukkit's real implementations for Bukkit classes.
Source: Coding guidelines
| @Test | ||
| @DisplayName("Given players without Bukkit player, when getAllBukkitPlayers, then returns empty set") | ||
| void getAllBukkitPlayers_returnsEmpty_whenNoBukkitPlayers() { | ||
| UUID uuid = UUID.randomUUID(); | ||
|
|
||
| TestCorePlayer corePlayer = mock(TestCorePlayer.class); | ||
| when(corePlayer.getAsBukkitPlayer()).thenReturn(Optional.empty()); | ||
| when(corePlayer.getUUID()).thenReturn(uuid); | ||
|
|
||
| manager.addPlayer(corePlayer); | ||
|
|
||
| assertTrue(manager.getAllBukkitPlayers().isEmpty()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use MockBukkit's PlayerMock instead of Mockito mock.
Line 166 also mocks Player using Mockito. Use MockBukkit's PlayerMock for consistency with the coding guidelines.
As per coding guidelines: "Do not use Mockito to mock a Bukkit class where MockBukkit already provides a real implementation (e.g., PlayerMock, ServerMock) — use the real MockBukkit implementation instead".
🤖 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/player/PlayerManagerTest.java`
around lines 161 - 173, The test method
`getAllBukkitPlayers_returnsEmpty_whenNoBukkitPlayers` uses Mockito's `mock()`
to create test instances, but per coding guidelines, MockBukkit's real
implementations should be used instead of Mockito mocks for Bukkit classes.
Replace the Mockito mock approach with MockBukkit's PlayerMock when creating any
Bukkit Player instances in this test to ensure consistency with the codebase
standards.
Source: Coding guidelines
2 participants
Summary
addPlayer(),removePlayer(),hasCorePlayer(),getPlayer(),isPlayerLocked()(locked/unlocked/missing),getAllPlayers()(defensive copy),getAllBukkitPlayers()(present/absent Bukkit player), and UUID replacement behavior (0% → 100% line coverage, 16/16 lines)doesChatterHavePendingResponse(),getPendingResponse(),addPendingResponse()with existing response expiry,removePendingResponse()by UUID and by ChatResponse instance (identity-based matching), multi-UUID isolation, and no-op removal safety (0% → 100% line coverage, 20/20 lines)getChatterUUID()accessor,getResponseWaitTime()delegation,onExpire()callback, and UUID distinctness across instances (0% → 100% line coverage, 4/4 lines)cancelTask()paths exercised (0% → 33.3% line coverage, 4/12 lines)getLocalizedMessage()overloads (Route, CorePlayer+Route, Audience+Route, with placeholders), allgetLocalizedMessages()list overloads, allgetLocalizedMessageAsComponent()andgetLocalizedMessageAsComponents()overloads,doesAnyLocaleContainRoute(),getLocalizedSection()(player and route-only),registerLanguageFile()(single and multiple docs per locale),getLocaleChain()(client locale priority, no Bukkit player fallback),postProcessResolvedString()hook, locale chain fallback behavior, duplicate locale skipping, andNoLocalizationContainsMessageExceptionfor missing routes (0% → 90.3% line coverage, 168/186 lines; remaining 10% arebroadcastMessage()which requiresBukkit.getOnlinePlayers()andBukkit.getConsoleSender()static calls)Infrastructure change
Added Mockito 5.14.2 as a
testImplementationdependency to enable mocking CorePlugin, Bukkit scheduler, YamlDocument, Player, and other framework types.Classes covered (avoiding PR #27–#36 overlap)
PR #27 covers GetItemRequest, StatisticEntry, ReloadableContent, ReloadableContentManager, StartupProfile. PR #28 covers Methods, PlayerSettingRegistry, ReloadableContent subtypes. PR #29 covers Mutexable, ParseError, EvaluationException, exception classes. PR #30 covers RegistryAccess, StatisticModifyEvent, PostStatisticModifyEvent, Transaction. PR #31 covers database events, GUI events, SQLiteDatabaseDriver, DatabaseDriver, ManagerKey/PluginHookKey constants. PR #32 covers player events, CorePlayer, Credentials, ConnectionDetails, CorePlayerOfflineException. PR #33 covers PlayerStatisticData, ReloadableParser, ItemPluginType, ChainPlayerContextFilter. PR #34 covers BatchTransaction, FailSafeTransaction, ItemBuilderConfigurationKeys, RegistryKeyImpl. PR #35 covers BootstrapContext, IllegalSlotAssignmentException, TableVersionHistoryDAO, MutexDAO. PR #36 covers PlayerSettingDAO, GuiManager, GetItemResponseState, exception classes. This PR targets entirely different classes with no overlap.
Coverage impact
4 classes brought from 0% to 100%, 1 brought from 0% to 90.3%, 1 improved from 0% to 33.3%. Overall project coverage: 31.9% → 38.5% (+6.6pp, 268 new lines covered).
Test plan
./gradlew testhttps://claude.ai/code/session_01VdgvpNjKTBC7y2EPrn3bi5
Generated by Claude Code
Summary by CodeRabbit