[Unit Tests] Add coverage for StateTypeWarningLog, McRPGPlayerStat, TimeGateCondition, and QuestChainStartConditionTypeRegistry - #331
Conversation
…ition, QuestChainStartConditionTypeRegistry, and PlayerLoginTimeDAO New test coverage for four previously untested classes: - StateTypeWarningLogTest: warn-once deduplication, independent keys, null cause - McRPGPlayerStatTest: enum keys, namespace, distinct values - TimeGateConditionTest: time boundary evaluation, timezone handling, edge cases - QuestChainStartConditionTypeRegistryTest: register, get, replace, getAll Also adds missing attemptCreateTable coverage to PlayerLoginTimeDAOTest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013FwUQGJ2Z9mU7otSsG2fw9
|
Claude finished @DiamondDagger590's task in 3m 2s —— View job Review complete ✅
No blocking issues found. 1 nit. SummaryThis PR is test-only (408 lines across 5 files under All 29 new/added tests are correct and align with the actual behavior of the code under test:
All follow the project's Nit1 nit
Note on verificationI was not able to execute |
📝 WalkthroughWalkthroughThis pull request adds unit tests for warning-log deduplication, database table creation, quest condition registries and time gates, and ChangesUnit 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.
Actionable comments posted: 1
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/database/table/PlayerLoginTimeDAOTest.java-36-63 (1)
36-63: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStrengthen the table-creation test assertions.
The tests do not verify that
Database.tableExists(...)controls the branch. The existing-table test also does not verify that no statement is prepared. The creation test matches onlyCREATE TABLE, so a different table name would still pass.Verify the database lookup, verify no
prepareStatement(...)call for an existing table, and matchPlayerLoginTimeDAO.TABLE_NAMEin the creation SQL.Proposed assertions
assertTrue(created); - verify(mockConnection).prepareStatement(contains("CREATE TABLE")); + verify(mockDatabase).tableExists(mockConnection, PlayerLoginTimeDAO.TABLE_NAME); + verify(mockConnection).prepareStatement( + contains("CREATE TABLE `" + PlayerLoginTimeDAO.TABLE_NAME + "`")); verify(mockStatement).executeUpdate(); boolean created = PlayerLoginTimeDAO.attemptCreateTable(mockConnection, mockDatabase); assertFalse(created); + verify(mockDatabase).tableExists(mockConnection, PlayerLoginTimeDAO.TABLE_NAME); + verify(mockConnection, never()).prepareStatement(anyString());🤖 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/PlayerLoginTimeDAOTest.java` around lines 36 - 63, Strengthen the tests for PlayerLoginTimeDAO.attemptCreateTable by verifying tableExists(mockConnection, PlayerLoginTimeDAO.TABLE_NAME) in both branches. In the existing-table test, also verify that mockConnection.prepareStatement(...) is never called; in the creation test, match both "CREATE TABLE" and PlayerLoginTimeDAO.TABLE_NAME in the SQL assertion.src/test/java/us/eunoians/mcrpg/combat/state/StateTypeWarningLogTest.java-41-64 (1)
41-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unexpected logger calls.
verify(...).log(...)only verifies matching calls. These tests still pass ifwarnOncelogs an additional warning with different arguments. AddverifyNoMoreInteractions(mockLogger)after the expected verifications.Proposed test fix
import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.verifyNoMoreInteractions; verify(mockLogger, times(1)).log(Level.WARNING, "something broke", cause); + verifyNoMoreInteractions(mockLogger); verify(mockLogger).log(Level.WARNING, "A failed", causeA); verify(mockLogger).log(Level.WARNING, "B failed", causeB); + verifyNoMoreInteractions(mockLogger);🤖 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/combat/state/StateTypeWarningLogTest.java` around lines 41 - 64, Add verifyNoMoreInteractions(mockLogger) after the expected log verifications in warnOnce_suppressesDuplicate and warnOnce_differentKeysAreIndependent, ensuring both tests reject any additional logger calls.
🤖 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/stat/McRPGPlayerStatTest.java`:
- Around line 7-14: Remove the McRPGBaseTest import and change
McRPGPlayerStatTest to a plain JUnit test class without extending McRPGBaseTest;
leave its enum and NamespacedKey assertions unchanged.
---
Other comments:
In `@src/test/java/us/eunoians/mcrpg/combat/state/StateTypeWarningLogTest.java`:
- Around line 41-64: Add verifyNoMoreInteractions(mockLogger) after the expected
log verifications in warnOnce_suppressesDuplicate and
warnOnce_differentKeysAreIndependent, ensuring both tests reject any additional
logger calls.
In `@src/test/java/us/eunoians/mcrpg/database/table/PlayerLoginTimeDAOTest.java`:
- Around line 36-63: Strengthen the tests for
PlayerLoginTimeDAO.attemptCreateTable by verifying tableExists(mockConnection,
PlayerLoginTimeDAO.TABLE_NAME) in both branches. In the existing-table test,
also verify that mockConnection.prepareStatement(...) is never called; in the
creation test, match both "CREATE TABLE" and PlayerLoginTimeDAO.TABLE_NAME in
the SQL assertion.
🪄 Autofix
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: 207e41cb-a4f0-474c-a331-177f61ffcaee
📒 Files selected for processing (5)
src/test/java/us/eunoians/mcrpg/combat/state/StateTypeWarningLogTest.javasrc/test/java/us/eunoians/mcrpg/database/table/PlayerLoginTimeDAOTest.javasrc/test/java/us/eunoians/mcrpg/quest/chain/condition/QuestChainStartConditionTypeRegistryTest.javasrc/test/java/us/eunoians/mcrpg/quest/chain/condition/builtin/TimeGateConditionTest.javasrc/test/java/us/eunoians/mcrpg/stat/McRPGPlayerStatTest.java
| import us.eunoians.mcrpg.McRPGBaseTest; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| @DisplayName("McRPGPlayerStat") | ||
| class McRPGPlayerStatTest extends McRPGBaseTest { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove unnecessary McRPGBaseTest inheritance.
These tests only read enum values and NamespacedKey fields. They do not require MockBukkit server interaction or McRPGPlayer tracking. Use plain JUnit to keep the test isolated.
Proposed fix
-import us.eunoians.mcrpg.McRPGBaseTest;
-
`@DisplayName`("McRPGPlayerStat")
-class McRPGPlayerStatTest extends McRPGBaseTest {
+class McRPGPlayerStatTest {As per coding guidelines, “Do not extend McRPGBaseTest when a test requires neither MockBukkit server interaction nor McRPGPlayer tracking; use plain JUnit instead.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import us.eunoians.mcrpg.McRPGBaseTest; | |
| import static org.junit.jupiter.api.Assertions.assertEquals; | |
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | |
| import static org.junit.jupiter.api.Assertions.assertNotNull; | |
| @DisplayName("McRPGPlayerStat") | |
| class McRPGPlayerStatTest extends McRPGBaseTest { | |
| import static org.junit.jupiter.api.Assertions.assertEquals; | |
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | |
| import static org.junit.jupiter.api.Assertions.assertNotNull; | |
| `@DisplayName`("McRPGPlayerStat") | |
| class McRPGPlayerStatTest { |
🤖 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/stat/McRPGPlayerStatTest.java` around lines 7
- 14, Remove the McRPGBaseTest import and change McRPGPlayerStatTest to a plain
JUnit test class without extending McRPGBaseTest; leave its enum and
NamespacedKey assertions unchanged.
Source: Coding guidelines
2 participants
Summary
NamespacedKey, independent keys logging independently, null cause handling, and no-interaction baseline. Pure Mockito — no MockBukkit needed.mcrpgnamespace, specific key strings for HEALTH and MANA, distinct keys, and enum cardinality. Uses@ParameterizedTestwith@EnumSource.registered()true/false, silent replacement on duplicate key,getAll()with multiple types, empty registry, and independent key registration.attemptCreateTablecoverage — creates table when absent, returns false when already exists. Follows the established mock JDBC pattern.Total: 29 new tests across 4 new test files and 1 modified test file.
Test plan
./gradlew verifiedShadowJarpasses with zero failuresMcRPGBaseTest,@DisplayName,action_outcome_whenConditionnamingsaveLoggedOutInSafeZonewhich is already handled by the existing test viamcRPG.getTimeProvider())Generated by Claude Code
Summary by CodeRabbit