[Unit Tests] Expand board DAO and ExpiredQuestScanTask test coverage - #347
[Unit Tests] Expand board DAO and ExpiredQuestScanTask test coverage#347DiamondDagger590 wants to merge 2 commits into
Conversation
- BoardCooldownDAOTest: add tests for listCooldowns, deleteCooldowns, isOnCooldown with key filters, CooldownRecord accessors/equality - PlayerBoardStateDAOTest: add tests for deleteForPlayer, loadAcceptedForPlayer (with/without quest UUID, multiple entries), updateStateByQuestInstanceUUID, bulkCancelExpiredBoardStates, AcceptedBoardEntry accessors/equality - ExpiredQuestScanTaskTest: restructure with @nested grouping, consolidate cases into @ParameterizedTest with @CsvSource, add minute/hour boundary and 24+ hour edge cases
|
Claude finished @DiamondDagger590's task in 5m 16s —— View job Review: 1 important, 2 nitsTest-only PR (703 additions / 195 deletions, no Important
Nits
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request reorganizes three test suites with JUnit nested groups. It expands board cooldown DAO coverage and adds parameterized and boundary cases for expired quest time formatting. ChangesBoard cooldown DAO tests
Player board state DAO tests
Expired quest time formatting tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR expands and reorganizes unit tests without changing product behavior or runtime code, so no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
| @Nested | ||
| @DisplayName("isOnCooldown") | ||
| class IsOnCooldownTests { | ||
|
|
||
| @DisplayName("Returns false when no matching rows exist") | ||
| @Test | ||
| void isOnCooldown_returnsFalse_whenNoResults() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| ResultSet mockResultSet = mock(ResultSet.class); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(false); | ||
|
|
||
| boolean result = BoardCooldownDAO.isOnCooldown( | ||
| mockConnection, | ||
| "rotation", | ||
| "player", | ||
| UUID.randomUUID().toString(), | ||
| null, | ||
| null | ||
| ); | ||
|
|
||
| assertFalse(result); | ||
| } | ||
|
|
||
| @DisplayName("Returns true when a matching row exists") | ||
| @Test | ||
| void isOnCooldown_returnsTrue_whenPresent() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| ResultSet mockResultSet = mock(ResultSet.class); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(true); | ||
|
|
||
| boolean result = BoardCooldownDAO.isOnCooldown( | ||
| mockConnection, | ||
| "rotation", | ||
| "player", | ||
| UUID.randomUUID().toString(), | ||
| null, | ||
| null | ||
| ); | ||
|
|
||
| assertTrue(result); | ||
| } | ||
|
|
||
| @DisplayName("Binds questDefinitionKey parameter when provided") | ||
| @Test | ||
| void isOnCooldown_bindsQuestDefinitionKey() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| ResultSet mockResultSet = mock(ResultSet.class); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(false); | ||
|
|
||
| NamespacedKey questKey = new NamespacedKey("mcrpg", "mine_stone"); | ||
| BoardCooldownDAO.isOnCooldown( | ||
| mockConnection, | ||
| "quest_repeat", | ||
| "player", | ||
| UUID.randomUUID().toString(), | ||
| questKey, | ||
| null | ||
| ); | ||
|
|
||
| verify(mockStatement).setString(eq(5), eq(questKey.toString())); | ||
| } | ||
|
|
||
| @DisplayName("Binds categoryKey parameter when provided") | ||
| @Test | ||
| void isOnCooldown_bindsCategoryKey() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| ResultSet mockResultSet = mock(ResultSet.class); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(false); | ||
|
|
||
| NamespacedKey categoryKey = new NamespacedKey("mcrpg", "daily_personal"); | ||
| BoardCooldownDAO.isOnCooldown( | ||
| mockConnection, | ||
| "category_rotation", | ||
| "player", | ||
| UUID.randomUUID().toString(), | ||
| null, | ||
| categoryKey | ||
| ); | ||
|
|
||
| verify(mockStatement).setString(eq(5), eq(categoryKey.toString())); | ||
| } | ||
|
|
||
| @DisplayName("Binds both keys at correct indices when both provided") | ||
| @Test | ||
| void isOnCooldown_bindsBothKeys() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| ResultSet mockResultSet = mock(ResultSet.class); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
| when(mockStatement.executeQuery()).thenReturn(mockResultSet); | ||
| when(mockResultSet.next()).thenReturn(false); | ||
|
|
||
| NamespacedKey questKey = new NamespacedKey("mcrpg", "mine_stone"); | ||
| NamespacedKey categoryKey = new NamespacedKey("mcrpg", "daily_personal"); | ||
| BoardCooldownDAO.isOnCooldown( | ||
| mockConnection, | ||
| "quest_repeat", | ||
| "player", | ||
| UUID.randomUUID().toString(), | ||
| questKey, | ||
| categoryKey | ||
| ); | ||
|
|
||
| verify(mockStatement).setString(eq(5), eq(questKey.toString())); | ||
| verify(mockStatement).setString(eq(6), eq(categoryKey.toString())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Important (testing): None of the isOnCooldown tests (nor listCooldowns/pruneExpiredCooldowns below) verify that McRPG.getInstance().getTimeProvider().now() is actually bound as the expires_at comparison parameter (setLong at the relevant index) — only the return value / other string params are asserted. A regression that drops the time bind, shifts its index, or swaps in wall-clock time instead of the injected TimeProvider would pass every existing test unnoticed.
The codebase already has the fix pattern in PlayerLoginTimeDAOTest.saveLoggedOutInSafeZone_bindsAllParameters_whenTrue: stub when(mcRPG.getTimeProvider().now()).thenReturn(fixedInstant) (via the inherited mcRPG field from McRPGBaseTest) and add verify(mockStatement).setLong(eq(N), eq(fixedInstant.toEpochMilli())) to at least one test per method.
2 participants
Summary
listCooldowns(empty/populated results),deleteCooldowns(with/without category key filter, zero rows),isOnCooldownwithquestDefinitionKeyfilter,categoryKeyfilter, and both filters simultaneously, plusCooldownRecordrecord accessors and equality. Restructured existing tests into@Nestedgroups.deleteForPlayer(with/without rows),loadAcceptedForPlayer(empty, with quest UUID, null quest UUID, multiple entries),updateStateByQuestInstanceUUID(match/no match),bulkCancelExpiredBoardStates(with/without expired states), plusAcceptedBoardEntryrecord accessors and equality. Restructured existing tests into@Nestedgroups.@Nestedgrouping, consolidated standard cases into@ParameterizedTestwith@CsvSource, added edge cases for sub-minute truncation, minute/hour boundaries, and 24+ hour non-wrapping behavior.Test plan
./gradlew test— BUILD SUCCESSFUL)action_outcome_whenCondition, descriptive@DisplayName)Generated by Claude Code
Summary by CodeRabbit