[Unit Tests] Add McRPGStatisticCacheManager tests and expand scope DAO coverage - #325
[Unit Tests] Add McRPGStatisticCacheManager tests and expand scope DAO coverage#325DiamondDagger590 wants to merge 1 commit into
Conversation
- Add new McRPGStatisticCacheManagerTest with 10 tests covering getCache, getOfflineStatistic (cache hit, DB hit, DB miss, per-key isolation, cache population on repeat calls, per-player separation), and populateAsync (DB hit, DB miss, SQLException handling) - Expand SinglePlayerQuestScopeDAOTest from 3 to 15 tests covering all public methods: attemptCreateTable, updateTable, findPlayerUuidForQuest, getPlayerInScope, findActiveQuestsForPlayer, saveScope - Expand LandQuestScopeDAOTest from 2 to 14 tests covering all public methods: attemptCreateTable, updateTable, getLandName, findAllActiveLandQuests, saveScope - Expand PermissionQuestScopeDAOTest from 2 to 13 tests covering all public methods: attemptCreateTable, updateTable, getPermissionNode, findAllActivePermissionQuests, saveScope Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012C7Go2ra1RsMPX76BwnAdk
📝 WalkthroughWalkthroughExpanded three quest-scope DAO test suites with lifecycle, lookup, active-query, persistence, and SQL-error coverage. Added statistic cache tests for synchronous retrieval, cache population, player isolation, and asynchronous database loading. ChangesQuest scope DAO tests
Statistic cache tests
Estimated code review effort: 3 (Moderate) | ~25 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 6m 26s —— View job Claude PR Review — No blocking issues foundVerdict: No blocking issues found (0 important, 0 nits) Summary: This is a test-only PR — no
|
![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 (2)
src/test/java/us/eunoians/mcrpg/statistic/McRPGStatisticCacheManagerTest.java-214-214 (1)
214-214: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winImport
ArgumentCaptorrather than qualifying it inline.Proposed fix
+import org.mockito.ArgumentCaptor; ... - var runnableCaptor = org.mockito.ArgumentCaptor.forClass(Runnable.class); + var runnableCaptor = ArgumentCaptor.forClass(Runnable.class);As per coding guidelines, “Do not write fully-qualified type references inline in method bodies …; all types must be imported at the top of the file.”
Also applies to: 241-241, 270-270
🤖 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/statistic/McRPGStatisticCacheManagerTest.java` at line 214, Import Mockito’s ArgumentCaptor at the top of McRPGStatisticCacheManagerTest and replace the fully qualified references in each affected test with the imported type, including the usages near lines 214, 241, and 270.Source: Coding guidelines
src/test/java/us/eunoians/mcrpg/statistic/McRPGStatisticCacheManagerTest.java-107-116 (1)
107-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify the DAO lookup in miss and error-path tests.
These tests only assert an empty cache, so a regression that skips
PlayerStatisticDAO.getPlayerStatistic(...)can still pass. The close-error case can also pass if no connection is acquired or closed.Proposed assertions
+ daoMock.verify(() -> PlayerStatisticDAO.getPlayerStatistic( + mockConnection, playerUUID, statKey)); assertTrue(result.isEmpty());+ daoMock.verify(() -> PlayerStatisticDAO.getPlayerStatistic( + mockConnection, playerUUID, statKey)); assertTrue(cached.isEmpty());+ daoMock.verify(() -> PlayerStatisticDAO.getPlayerStatistic( + closingConnection, playerUUID, statKey)); + verify(closingConnection).close(); assertTrue(cached.isEmpty());Also applies to: 235-246, 264-275
🤖 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/statistic/McRPGStatisticCacheManagerTest.java` around lines 107 - 116, Strengthen the miss and error-path tests around cacheManager.getOfflineStatistic by verifying PlayerStatisticDAO.getPlayerStatistic is invoked with mockConnection, playerUUID, and statKey. In the close-error case, also verify the expected connection acquisition and close behavior so the test fails when the connection path is skipped; apply the same assertions to the additional marked test blocks.
🧹 Nitpick comments (3)
src/test/java/us/eunoians/mcrpg/database/table/quest/scope/PermissionQuestScopeDAOTest.java (1)
35-48: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated Mockito test-harness setup.
Same
@BeforeEach setUp()boilerplate asLandQuestScopeDAOTestandSinglePlayerQuestScopeDAOTest. See consolidated comment.🤖 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/database/table/quest/scope/PermissionQuestScopeDAOTest.java` around lines 35 - 48, Consolidate the duplicated Mockito test-harness initialization from PermissionQuestScopeDAOTest.setUp() with the shared setup used by LandQuestScopeDAOTest and SinglePlayerQuestScopeDAOTest. Reuse the existing common fixture or setup helper for the mock fields and stubbing, while preserving the current mockConnection, mockStatement, mockResultSet, and mockDatabase behavior.src/test/java/us/eunoians/mcrpg/database/table/quest/scope/LandQuestScopeDAOTest.java (1)
35-48: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated Mockito test-harness setup.
This
@BeforeEach setUp()and mock field block is duplicated verbatim acrossLandQuestScopeDAOTest,PermissionQuestScopeDAOTest, andSinglePlayerQuestScopeDAOTest. See consolidated comment.🤖 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/database/table/quest/scope/LandQuestScopeDAOTest.java` around lines 35 - 48, Remove the duplicated mock fields and `@BeforeEach` setUp() logic from LandQuestScopeDAOTest, PermissionQuestScopeDAOTest, and SinglePlayerQuestScopeDAOTest, and reuse a shared test-harness base or utility containing the Mockito initialization and stubbing for mockConnection, mockStatement, mockResultSet, and mockDatabase. Preserve each test’s existing access to these shared mocks.src/test/java/us/eunoians/mcrpg/database/table/quest/scope/SinglePlayerQuestScopeDAOTest.java (1)
35-48: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated Mockito test-harness setup.
Same
@BeforeEach setUp()boilerplate asLandQuestScopeDAOTestandPermissionQuestScopeDAOTest. See consolidated comment.🤖 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/database/table/quest/scope/SinglePlayerQuestScopeDAOTest.java` around lines 35 - 48, Consolidate the duplicated Mockito setup from SinglePlayerQuestScopeDAOTest.setUp() with the shared test-harness setup used by LandQuestScopeDAOTest and PermissionQuestScopeDAOTest. Reuse the existing common setup mechanism and remove redundant mock initialization and stubbing from this class, preserving the same mock fields and behavior.
🤖 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/statistic/McRPGStatisticCacheManagerTest.java`:
- Line 214: Import Mockito’s ArgumentCaptor at the top of
McRPGStatisticCacheManagerTest and replace the fully qualified references in
each affected test with the imported type, including the usages near lines 214,
241, and 270.
- Around line 107-116: Strengthen the miss and error-path tests around
cacheManager.getOfflineStatistic by verifying
PlayerStatisticDAO.getPlayerStatistic is invoked with mockConnection,
playerUUID, and statKey. In the close-error case, also verify the expected
connection acquisition and close behavior so the test fails when the connection
path is skipped; apply the same assertions to the additional marked test blocks.
---
Nitpick comments:
In
`@src/test/java/us/eunoians/mcrpg/database/table/quest/scope/LandQuestScopeDAOTest.java`:
- Around line 35-48: Remove the duplicated mock fields and `@BeforeEach` setUp()
logic from LandQuestScopeDAOTest, PermissionQuestScopeDAOTest, and
SinglePlayerQuestScopeDAOTest, and reuse a shared test-harness base or utility
containing the Mockito initialization and stubbing for mockConnection,
mockStatement, mockResultSet, and mockDatabase. Preserve each test’s existing
access to these shared mocks.
In
`@src/test/java/us/eunoians/mcrpg/database/table/quest/scope/PermissionQuestScopeDAOTest.java`:
- Around line 35-48: Consolidate the duplicated Mockito test-harness
initialization from PermissionQuestScopeDAOTest.setUp() with the shared setup
used by LandQuestScopeDAOTest and SinglePlayerQuestScopeDAOTest. Reuse the
existing common fixture or setup helper for the mock fields and stubbing, while
preserving the current mockConnection, mockStatement, mockResultSet, and
mockDatabase behavior.
In
`@src/test/java/us/eunoians/mcrpg/database/table/quest/scope/SinglePlayerQuestScopeDAOTest.java`:
- Around line 35-48: Consolidate the duplicated Mockito setup from
SinglePlayerQuestScopeDAOTest.setUp() with the shared test-harness setup used by
LandQuestScopeDAOTest and PermissionQuestScopeDAOTest. Reuse the existing common
setup mechanism and remove redundant mock initialization and stubbing from this
class, preserving the same mock fields and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 4f5ac981-a7ce-492b-8856-6c159fcafd1f
📒 Files selected for processing (4)
src/test/java/us/eunoians/mcrpg/database/table/quest/scope/LandQuestScopeDAOTest.javasrc/test/java/us/eunoians/mcrpg/database/table/quest/scope/PermissionQuestScopeDAOTest.javasrc/test/java/us/eunoians/mcrpg/database/table/quest/scope/SinglePlayerQuestScopeDAOTest.javasrc/test/java/us/eunoians/mcrpg/statistic/McRPGStatisticCacheManagerTest.java
2 participants
Summary
McRPGStatisticCacheManagerTest— 10 tests coveringgetCache(),getOfflineStatistic()(cache hit, DB fallback, DB miss, per-key isolation, cache population on repeat calls, per-player separation), andpopulateAsync()(DB hit populates cache, DB miss leaves cache empty, SQLException handled gracefully). Coverage goes from 0% to near-complete.SinglePlayerQuestScopeDAOTestfrom 3 → 15 tests covering all 6 public methodsLandQuestScopeDAOTestfrom 2 → 14 tests covering all 5 public methodsPermissionQuestScopeDAOTestfrom 2 → 13 tests covering all 5 public methodsAll scope DAO tests now cover
attemptCreateTable(exists/create/exception),updateTable(current version/v0), query methods (found/not-found/exception), andsaveScope(valid/exception).Test plan
./gradlew test)Generated by Claude Code
Summary by CodeRabbit