[Unit Tests] Expand DAO test coverage for PlayerBoardState, LoadoutDisplay, PlayerLoadoutSelection, and PlayerStat - #323
Conversation
…LoadoutSelection, and PlayerStat Add comprehensive mock-based unit tests for four DAO classes that previously had low coverage (34-56%). Tests cover attemptCreateTable, updateTable migrations, CRUD operations, SQL exception handling, parameter binding, and edge cases like invalid NamespacedKey parsing and minimum-value floors. - PlayerBoardStateDAO: 4 → ~28 tests (all 8 public methods + AcceptedBoardEntry) - LoadoutDisplayDAO: 9 → ~22 tests (updateTable migrations, saveAll, columnExists) - PlayerLoadoutSelectionDAO: 8 → ~14 tests (attemptCreateTable, updateTable) - PlayerStatDAO: 7 → ~16 tests (attemptCreateTable, saveStats batch, exceptions) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEUot1ASeqchbi29i1ofUS
- Fix getActiveLoadout no-row test to exercise catch-block path by throwing SQLException from getInt after empty ResultSet - Add multi-row iteration test for loadAcceptedForPlayer - Assert exact statement count (4) in saveAllLoadoutDisplays test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEUot1ASeqchbi29i1ofUS
📝 WalkthroughWalkthroughThe DAO test suites were reorganized into nested operation groups and expanded with coverage for schema migration, JDBC parameter binding, nullable values, empty results, SQL exceptions, and CRUD behavior across loadout, stat, and board-state persistence. ChangesDAO test coverage
Estimated code review effort: 4 (Complex) | ~45 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 |
|
Claude finished @DiamondDagger590's task in 4m 59s —— View job Review: 2 important, 1 nitThis PR only touches test files ( Important findings (also left as inline comments):
1 nit
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/us/eunoians/mcrpg/database/table/PlayerLoadoutSelectionDAOTest.java (1)
209-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test conflates two mutually exclusive paths.
If
getActiveLoadoutguards onnext(), thegetIntstub is never used (dead stubbing, and a failure under strict stubs); if it does not guard, the assertion only proves the exception fallback. Split into two tests: one withnext()returningfalseand nogetIntstub, and one asserting theSQLExceptionfallback explicitly.♻️ Suggested split
- `@Test` - `@DisplayName`("returns 1 when no row exists and getInt throws after empty result set") - void getActiveLoadout_returnsOne_whenNoRowExists() throws SQLException { + `@Test` + `@DisplayName`("returns 1 when no row exists") + void getActiveLoadout_returnsOne_whenNoRowExists() 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); - when(mockResultSet.getInt("active_loadout_id")) - .thenThrow(new SQLException("ResultSet is empty")); int result = PlayerLoadoutSelectionDAO.getActiveLoadout(mockConnection, PLAYER_UUID); assertEquals(1, result); }🤖 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/PlayerLoadoutSelectionDAOTest.java` around lines 209 - 224, Split getActiveLoadout_returnsOne_whenNoRowExists into two focused tests: keep the empty-result case with next() returning false and remove the unused getInt stubbing, then add a separate test that makes next() permit getInt execution, stubs getInt to throw SQLException, and asserts the fallback value of 1.src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java (1)
120-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace inline fully-qualified Mockito references with static imports. All four suites reference
org.mockito.ArgumentMatchers.contains,org.mockito.Mockito.never, andorg.mockito.Mockito.timesby fully-qualified name inside method bodies, while the same classes are already statically imported formock/when/verify.
src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java#L120-L126: addimport static org.mockito.ArgumentMatchers.contains;and use barecontains(...)here and at lines 160, 179, 183, 214, 364, 366.src/test/java/us/eunoians/mcrpg/database/table/PlayerLoadoutSelectionDAOTest.java#L90-L110: use the already-importednever()and a statically importedcontains(...)here and at line 241.src/test/java/us/eunoians/mcrpg/database/table/PlayerStatDAOTest.java#L96-L96: add thecontainsstatic import and use it here and at line 113.src/test/java/us/eunoians/mcrpg/database/table/board/PlayerBoardStateDAOTest.java#L98-L116: statically importneverandtimesand drop theorg.mockito.Mockito.prefixes.As per coding guidelines, "Do not write fully-qualified type references inline in method bodies (e.g.,
org.bukkit.Location loc = ...); all types must be imported at the top of the file".🤖 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/LoadoutDisplayDAOTest.java` around lines 120 - 126, Replace inline fully qualified Mockito calls with static imports and bare calls. In src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java:120-126 and its other listed usages (160, 179, 183, 214, 364, 366), statically import contains and remove the ArgumentMatchers prefix; in PlayerLoadoutSelectionDAOTest.java:90-110 and 241, use static never and contains; in PlayerStatDAOTest.java:96 and 113, add and use static contains; in PlayerBoardStateDAOTest.java:98-116, statically import never and times and remove their Mockito prefixes.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/database/table/LoadoutDisplayDAOTest.java`:
- Around line 29-33: Upgrade the Mockito test dependency from the legacy
mockito-inline 3.12.2 artifact to a Java 21–supported Mockito release in the
Gradle configuration, remove the mockito-inline dependency, and retain static
mocking through Mockito core’s mockStatic API used by LoadoutDisplayDAOTest.
In `@src/test/java/us/eunoians/mcrpg/database/table/PlayerStatDAOTest.java`:
- Line 50: Replace the hard-coded "mcrpg_player_stat" argument in all four
mockDatabase.tableExists stubs within PlayerStatDAOTest with
PlayerStatDAO.TABLE_NAME, preserving the existing stubbing behavior.
---
Nitpick comments:
In `@src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java`:
- Around line 120-126: Replace inline fully qualified Mockito calls with static
imports and bare calls. In
src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java:120-126
and its other listed usages (160, 179, 183, 214, 364, 366), statically import
contains and remove the ArgumentMatchers prefix; in
PlayerLoadoutSelectionDAOTest.java:90-110 and 241, use static never and
contains; in PlayerStatDAOTest.java:96 and 113, add and use static contains; in
PlayerBoardStateDAOTest.java:98-116, statically import never and times and
remove their Mockito prefixes.
In
`@src/test/java/us/eunoians/mcrpg/database/table/PlayerLoadoutSelectionDAOTest.java`:
- Around line 209-224: Split getActiveLoadout_returnsOne_whenNoRowExists into
two focused tests: keep the empty-result case with next() returning false and
remove the unused getInt stubbing, then add a separate test that makes next()
permit getInt execution, stubs getInt to throw SQLException, and asserts the
fallback value of 1.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 014b852a-70b3-4cef-9a23-4f336402aecd
📒 Files selected for processing (4)
src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.javasrc/test/java/us/eunoians/mcrpg/database/table/PlayerLoadoutSelectionDAOTest.javasrc/test/java/us/eunoians/mcrpg/database/table/PlayerStatDAOTest.javasrc/test/java/us/eunoians/mcrpg/database/table/board/PlayerBoardStateDAOTest.java
| import static org.mockito.ArgumentMatchers.eq; | ||
| import static org.mockito.ArgumentMatchers.isNull; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.mockStatic; | ||
| import static org.mockito.Mockito.never; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Mockito 3.12.2 mockito-inline Java 21 support minimum version
💡 Result:
Mockito 3.12.2 does not support Java 21 [1][2][3]. Official support for Java 21 was introduced in Mockito 5.x [2][4]. Mockito 3.x is a legacy version, and the project only supports the latest major version (currently 5.x) [4]. Attempts to use older versions like 3.12.2 on Java 21 will typically fail due to incompatibility with the underlying Byte Buddy library, which requires updates to recognize Java 21's bytecode version (65) [1][3]. If you are currently using Mockito 3.12.2 or any 4.x version and need to run tests on Java 21, you must upgrade to Mockito 5.x [2][4]. Starting with version 5.0.0, the "inline" mock maker is the default, so the separate mockito-inline artifact is no longer required and is effectively deprecated [5][6][4]. Additionally, please note that running Mockito on Java 21 may require explicit JVM configuration (such as adding -javaagent) due to increased restrictions on dynamic instrumentation in newer JDKs [7][6].
Citations:
- 1: Mockito does not work on Java 21 mockito/mockito#3321
- 2: Weird interaction with JDK21 method Reflection and mocks mockito/mockito#3345
- 3: Tests failures after adding mockito-inline on Java 21 mockito/mockito#3328
- 4: https://github.com/mockito/mockito
- 5: https://groups.google.com/g/mockito/c/psRyztIa5mY
- 6: https://javadoc.io/static/org.mockito/mockito-core/5.21.0/org.mockito/org/mockito/Mockito.html
- 7: https://github.com/mockito/mockito/blob/main/mockito-core/src/main/java/org/mockito/Mockito.java
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== java target and mockito dependency declarations =="
git ls-files | rg '(^|/)(build\.gradle|build\.gradle\.kts|pom\.xml|gradle\.properties|.*\.gradle)$' | while read -r f; do
echo "--- $f"
rg -n "java|sourceCompatibility|targetCompatibility|mockito|JUnit|org.mockito|bytebuddy|byte-buddy|source.*21|target.*21|java\s*[""']?21|target-version|jvm-target|properties.java.version|mocktools|inline" "$f" || true
done
echo
echo "== candidate test file relevant imports/usages =="
if [ -f src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java ]; then
wc -l src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java
sed -n '1,220p' src/test/java/us/eunoians/mcrpg/database/table/LoadoutDisplayDAOTest.java
fiRepository: DiamondDagger590/McRPG
Length of output: 11446
Upgrade Mockito test dependencies before using mockStatic.
The project targets Java 21 while build.gradle.kts pins org.mockito:mockito-inline to 3.12.2; that Mockito release is legacy and does not support Java 21 static mocking. Move the static mocks to a supported Mockito Java 21 release and use the core mockStatic API instead of the deprecated mockito-inline artifact.
🤖 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/LoadoutDisplayDAOTest.java`
around lines 29 - 33, Upgrade the Mockito test dependency from the legacy
mockito-inline 3.12.2 artifact to a Java 21–supported Mockito release in the
Gradle configuration, remove the mockito-inline dependency, and retain static
mocking through Mockito core’s mockStatic API used by LoadoutDisplayDAOTest.
| void attemptCreateTable_returnsFalse_whenTableExists() { | ||
| Connection mockConnection = mock(Connection.class); | ||
| Database mockDatabase = mock(Database.class); | ||
| when(mockDatabase.tableExists(mockConnection, "mcrpg_player_stat")).thenReturn(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use PlayerStatDAO.TABLE_NAME instead of the literal "mcrpg_player_stat".
The sibling suites in this PR stub tableExists with the DAO's constant. With a literal, a rename of the constant leaves these stubs unmatched, so tableExists returns false by default and the tests keep "passing" while asserting nothing meaningful.
♻️ Proposed change (apply to all four stubs)
- when(mockDatabase.tableExists(mockConnection, "mcrpg_player_stat")).thenReturn(true);
+ when(mockDatabase.tableExists(mockConnection, PlayerStatDAO.TABLE_NAME)).thenReturn(true);Also applies to: 63-63, 77-77, 91-91
🤖 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/PlayerStatDAOTest.java` at
line 50, Replace the hard-coded "mcrpg_player_stat" argument in all four
mockDatabase.tableExists stubs within PlayerStatDAOTest with
PlayerStatDAO.TABLE_NAME, preserving the existing stubbing behavior.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @DisplayName("returns display when row exists") | ||
| void getLoadoutDisplay_returnsDisplay_whenRowExists() 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); | ||
| when(mockResultSet.getString("display_item")).thenReturn("DIAMOND_SWORD"); | ||
| when(mockResultSet.getString("display_name")).thenReturn("Battle Loadout"); | ||
|
|
||
| Optional<LoadoutDisplay> result = LoadoutDisplayDAO.getLoadoutDisplay(mockConnection, PLAYER_UUID, 1); | ||
|
|
||
| assertTrue(result.isPresent()); |
There was a problem hiding this comment.
Important (testing): This test (and getLoadoutDisplay_returnsDisplay_whenDisplayNameIsNull below) stubs display_item/display_name on the result set but only asserts result.isPresent() — it never checks the returned LoadoutDisplay's actual item/name. A bug that swaps the two column reads or mis-wraps the item would still leave the Optional non-empty, so this test can't catch it.
LoadoutDisplay already has a working equals() (item + Optional<String> name), so assert on the value instead:
assertEquals(Optional.of(new LoadoutDisplay(new CustomItemWrapper("DIAMOND_SWORD"), "Battle Loadout")), result);(and the analogous null-name assertion in the other test).
| @Test | ||
| @DisplayName("CREATE TABLE SQL references the loadout info table") | ||
| void attemptCreateTable_referencesLoadoutInfoTable() throws SQLException { | ||
| Connection mockConnection = mock(Connection.class); | ||
| Database mockDatabase = mock(Database.class); | ||
| PreparedStatement mockStatement = mock(PreparedStatement.class); | ||
| when(mockDatabase.tableExists(mockConnection, PlayerLoadoutSelectionDAO.TABLE_NAME)).thenReturn(false); | ||
| when(mockConnection.prepareStatement(anyString())).thenReturn(mockStatement); | ||
|
|
||
| PlayerLoadoutSelectionDAO.attemptCreateTable(mockConnection, mockDatabase); | ||
|
|
||
| verify(mockConnection).prepareStatement(org.mockito.ArgumentMatchers.contains("CREATE TABLE")); | ||
| } |
There was a problem hiding this comment.
Important (testing): This test's @DisplayName claims "CREATE TABLE SQL references the loadout info table," but the assertion only checks contains("CREATE TABLE") — already covered by attemptCreateTable_returnsTrue_whenTableDoesNotExist above. It would still pass even if the FOREIGN KEY ... REFERENCES clause to the loadout info table were deleted entirely.
Assert on the FK content it claims to verify, e.g. contains("REFERENCES") or the actual loadout-info table name.
2 participants
Summary
attemptCreateTable,updateTable,saveState,deleteForPlayer,loadAcceptedForPlayer,updateStateByQuestInstanceUUID,bulkCancelExpiredBoardStates,countActiveQuestsFromBoard) plusAcceptedBoardEntryrecord, including multi-row iteration and SQL exception pathsupdateTablev1/v2 migration tests,saveAllLoadoutDisplayswith exact statement count assertion,columnExistsmetadata/PRAGMA paths, material fallback when custom item is absent, and SQL exception pathsattemptCreateTable(exists/create/exception),updateTableversion tracking, and fixes the no-row test to exercise the catch-block path matching real JDBC behaviorattemptCreateTable(exists/create/exception/PK verification),saveStatsbatch (multi-entry, empty map, exception propagation, per-entry binding), and SQL exception paths for load methodsAll tests use the established mock-based JDBC pattern (Mockito mocks for
Connection,PreparedStatement,ResultSet) and follow the@Nested/@DisplayName/action_outcome_whenConditionnaming conventions.Test plan
./gradlew test— BUILD SUCCESSFUL)persona-testing.mdc) reviewed — all findings addressedGenerated by Claude Code
Summary by CodeRabbit