[Tests] Add unit tests for player events, CorePlayer, database records, CorePlayerOfflineException

[DiamondDagger590]

Summary

• PlayerEventTest: Tests PlayerLoadEvent and PlayerUnloadEvent — constructor, getCorePlayer(), instance/static handler list identity, and cross-event handler list isolation (both 0% → 100% line coverage)
• PlayerSettingChangeEventTest: Tests getOldSetting() with null and non-null values, getNewSetting(), handler list identity, and cross-event handler list isolation with PlayerLoadEvent (0% → 100% line coverage)
• CorePlayerEvent: Tested transitively through PlayerLoadEvent — getCorePlayer() getter and UUID preservation (75% → 100% line coverage)
• CorePlayerTest: Tests getUUID(), getPlugin(), getStatisticData(), getPlayerSettings() (empty), getPlayerSetting() (absent key), useMutex() (both true/false), isAfk() with no hooks registered, equals() (same UUID, different UUID, self, non-CorePlayer, null), hashCode() consistency, and toString() format (20% → 66.7% line coverage; remaining 33% are Bukkit-dependent methods: getAsBukkitPlayer(), setPlayerSetting())
• DatabaseRecordTest: Tests Credentials and ConnectionDetails records — field accessors, equals, hashCode, toString, and edge cases (zero port, zero idle timeout, disabled leak detection) (both 0% → 100% line coverage)
• CorePlayerOfflineExceptionTest: Tests single-arg constructor (auto-generated message containing UUID), two-arg constructor (custom message), getCorePlayer() accessor, and RuntimeException inheritance (0% → 100% line coverage)

Classes covered (avoiding PR #27, PR #28, PR #29, PR #30, and PR #31 overlap)

PR #27 covers GetItemRequest, StatisticEntry, ReloadableContent, ReloadableContentManager, StartupProfile. PR #28 covers Methods, PlayerSettingRegistry, ReloadableContent subtypes. PR #29 covers Mutexable, ParseError, EvaluationException, and exception classes. PR #30 covers RegistryAccess, StatisticModifyEvent, PostStatisticModifyEvent, Transaction. PR #31 covers database events, GUI events, SQLiteDatabaseDriver, DatabaseDriver, ManagerKey/PluginHookKey constants. This PR targets entirely different classes with no overlap.

Coverage impact

8 classes improved: 7 brought from 0% to 100% line coverage, 1 improved from 20% to 66.7%. Overall project coverage: 27.0% → 28.1%.

Test plan

• All new tests pass via ./gradlew test
• JaCoCo coverage report confirms expected coverage on all 8 targeted classes
• No existing tests broken by these additions
• No production code changes — test-only PR

https://claude.ai/code/session_012EzjJy84uWQ59EwcB2mjAX

---

Generated by Claude Code

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff adds only test classes with no changes to production source files.

The entire diff consists of new files added under src/test/java/. No production classes, interfaces, abstract classes, public APIs, registry constants, GUI components, or database schema are modified. Test code is not part of the compiled artifact distributed to downstream plugin developers and imposes no contract obligations on consumers of McCore.

A brief scan of each test file confirms:

• DatabaseRecordTest.java — exercises Credentials and ConnectionDetails as a consumer would; no production changes.
• PlayerEventTest.java — instantiates PlayerLoadEvent / PlayerUnloadEvent via a local TestCorePlayer stub; no production changes.
• PlayerSettingChangeEventTest.java — implements PlayerSetting in a local stub; no production changes.
• CorePlayerOfflineExceptionTest.java — constructs CorePlayerOfflineException with existing constructors; no production changes.
• CorePlayerTest.java — subclasses CorePlayer with a local stub and calls RegistryResetExtension helpers; no production changes.

No extensibility concerns found.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff, examining both the new test files and the production classes they target.

---

Analysis

Production Classes Under Test (inferred from test targets)

• DatabaseRecord (containing Credentials and ConnectionDetails records)
• PlayerLoadEvent, PlayerUnloadEvent, CorePlayerEvent
• PlayerSettingChangeEvent
• CorePlayerOfflineException
• CorePlayer

Test Files Present

• DatabaseRecordTest.java
• PlayerEventTest.java
• PlayerSettingChangeEventTest.java
• CorePlayerOfflineExceptionTest.java
• CorePlayerTest.java

---

Findings

---

CONCERN: PlayerEventTest and PlayerSettingChangeEventTest instantiate Bukkit classes (HandlerList, NamespacedKey) without MockBukkit, and CorePlayerTest uses NamespacedKey directly — these will fail at runtime unless the Bukkit API is initialized.
WHY: HandlerList and NamespacedKey are Bukkit classes. Calling new NamespacedKey("test", "nonexistent") and referencing PlayerLoadEvent.getHandlerList() (which internally owns a HandlerList) requires the Bukkit server environment. Without MockBukkit.mock() / MockBukkit.unmock(), these tests either silently pass against a stub jar or throw ExceptionInInitializerError at runtime, making them structurally unreliable regardless of what they assert.
WHERE: PlayerEventTest.java, PlayerSettingChangeEventTest.java, CorePlayerTest.java (the getPlayerSetting_returnsEmpty_whenKeyNotPresent test)

---

CONCERN: PlayerEventTest tests that need MockBukkit have no @BeforeEach/@AfterEach MockBukkit lifecycle, meaning if any test does trigger Bukkit initialization, the mock is never torn down and will leak across the test suite.
WHY: The checklist requires MockBukkit.mock() in setup and MockBukkit.unmock() in teardown. Their absence means any Bukkit state initialized by one test bleeds into subsequent tests, causing non-deterministic failures.
WHERE: PlayerEventTest.java, PlayerSettingChangeEventTest.java

---

CONCERN: CorePlayerTest references RegistryResetExtension.setupRegistry() / RegistryResetExtension.resetRegistry(), but this class does not appear in the diff and is not in src/testFixtures/java/. Its existence, contract, and correctness cannot be verified.
WHY: If RegistryResetExtension is a shared test helper, it must live in src/testFixtures/java/ per the checklist so downstream repos can depend on it. If it only exists in src/test/java/, it is inaccessible to McRPG. If it does not exist at all, every CorePlayerTest method will fail to compile.
WHERE: CorePlayerTest.java — RegistryResetExtension class (missing from diff)

---

CONCERN: CorePlayerTest calls RegistryResetExtension.setupRegistry() / resetRegistry() as static utility methods in @BeforeEach/@AfterEach rather than using the JUnit 5 @ExtendWith mechanism. If the extension also needs to run as a JUnit extension (e.g., to catch exceptions mid-test), the static-call pattern bypasses that lifecycle entirely.
WHY: A class named RegistryResetExtension implies it implements BeforeEachCallback/AfterEachCallback. Calling its methods statically instead of via @ExtendWith(RegistryResetExtension.class) defeats the extension contract and means teardown is skipped if a test throws.
WHERE: CorePlayerTest.java, lines with @BeforeEach / @AfterEach

---

CONCERN: PlayerSettingChangeEventTest imports PlayerLoadEvent solely to assert handler list inequality, but the test class resides in the package com.diamonddagger590.mccore.event.setting.setting (double setting segment), which does not match any plausible production package structure.
WHY: A malformed package name means the test file lives in the wrong directory on disk (event/setting/setting/) and will not be picked up by standard test discovery unless the directory physically mirrors the double-segment name. This is a structural problem, not a style issue.
WHERE: PlayerSettingChangeEventTest.java — package declaration line 1

---

CONCERN: CorePlayerTest has no test covering setPlayerSetting / removing a setting or verifying getPlayerSetting returns a value after one is added — the happy-path of the setting map is untested.
WHY: The checklist requires coverage of non-trivial logic paths. CorePlayer.setPlayerSetting (or equivalent mutator) and the retrieval of a successfully stored setting represent the primary use case of the player settings map. Testing only the empty/absent case leaves the mutation path entirely uncovered.
WHERE: CorePlayerTest.java — missing givenSettingAdded_whenGetPlayerSetting_thenReturnsIt test

---

CONCERN: CorePlayerTest.isAfk_returnsFalse_whenNoHooksRegistered tests only the vacuous case (no hooks registered). There is no test for the case where an AfkPluginHook is registered and returns true, meaning the branching logic of isAfk() is only half-covered.
WHY: A method that delegates to registered hooks and returns false by default has two meaningful branches: hooks absent (covered) and hook present returning true (not covered). Single-branch coverage of a conditional is a coverage gap.
WHERE: CorePlayerTest.java — isAfk tests

---

CONCERN: DatabaseRecordTest tests toString by asserting the password "secret" appears in the Credentials toString output. If Credentials is a sensitive-data holder, including credentials in toString is a security concern — but more critically for testing: if the production code intentionally masks the password in toString, this test will fail and incorrectly signal a bug. The test asserts an implementation detail of toString that may be intentionally omitted.
WHY: Asserting that toString contains "secret" tightly couples the test to a security-sensitive implementation choice. If the record masks the password (a common hardening step), the test is wrong. There is no corresponding production-code diff to confirm the toString behavior, making this assertion unverifiable from the diff alone.
WHERE: DatabaseRecordTest.java — toString_containsAllFieldValues test

---

CONCERN: DatabaseRecordTest does not test null inputs for Credentials or ConnectionDetails fields (e.g., null host, null username, null database name).
WHY: The checklist explicitly requires null-input edge cases for non-trivial constructors. Records that store these values do not null-check by default; a null host passed to downstream JDBC code will produce a confusing NPE rather than an early validation error. If the production class validates nulls, that validation is untested.
WHERE: DatabaseRecordTest.java — CredentialsTest, ConnectionDetailsTest

---

CONCERN: DatabaseRecordTest does not test negative values for ConnectionDetails numeric fields (negative timeout, negative connection counts).
WHY: The checklist requires negative/zero numeric edge cases. HikariCP rejects certain negative values at pool-creation time. If ConnectionDetails is supposed to validate these, that validation is untested. If it intentionally permits them, a test documenting that contract is still missing.
WHERE: DatabaseRecordTest.java — ConnectionDetailsTest

---

CONCERN: No test covers CorePlayer.getStatisticData() beyond asserting non-null — the content, type, and mutability of the returned object are untested.
WHY: getStatisticData() returning a non-null object is the weakest possible assertion for a method that presumably returns a meaningful data container. If the production class returns a default-initialized object, the fields of that object (zeroed stats, empty maps) represent the real contract and are untested.
WHERE: CorePlayerTest.java — getStatisticData_returnsNonNull

---

CONCERN: CorePlayerOfflineExceptionTest.constructor_messageIndicatesPlayerNotOnline hardcodes the substring "not online" as the expected message fragment, but there is no production diff showing the actual message template. If the message changes, this test gives a misleading failure.
WHY: Asserting a specific human-readable substring of an exception message couples the test to phrasing rather than semantics, and without the production source in the diff the assertion cannot be verified as correct. This is a structural correctness issue: if the message never contained "not online", the test was always wrong.
WHERE: CorePlayerOfflineExceptionTest.java — constructor_messageIndicatesPlayerNotOnline

---

Summary

Production files changed: DatabaseRecord (Credentials, ConnectionDetails), PlayerLoadEvent, PlayerUnloadEvent, CorePlayerEvent, PlayerSettingChangeEvent, CorePlayerOfflineException, CorePlayer

Test files present: DatabaseRecordTest.java, PlayerEventTest.java, PlayerSettingChangeEventTest.java, CorePlayerOfflineExceptionTest.java, CorePlayerTest.java

Coverage gaps:

• Null inputs for Credentials and ConnectionDetails constructor fields
• Negative numeric inputs for ConnectionDetails
• CorePlayer.setPlayerSetting / happy-path retrieval of a stored setting
• isAfk() with a registered hook that returns true
• getStatisticData() content beyond non-null
• CorePlayer behavior with useMutex=true under contention (mutex logic itself)
• RegistryResetExtension missing from diff — contract and shared-fixture placement unverifiable
• MockBukkit lifecycle absent from all tests that touch Bukkit types (HandlerList, NamespacedKey)
• Malformed package name in PlayerSettingChangeEventTest (event.setting.setting)
• toString password-masking assumption in DatabaseRecordTest unverifiable without production diff

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code (src/test/java/...). All files are JUnit 5 unit tests covering Credentials, ConnectionDetails, PlayerLoadEvent, PlayerUnloadEvent, PlayerSettingChangeEvent, CorePlayerOfflineException, and CorePlayer. No production DAO methods, DDL constructors, MiniMessage calls, ChatResponse implementations, Slot.onClick() overrides, or permission-gated operations are introduced or modified. None of the checklist items apply to test-only code.

[coderabbitai]

Warning

Review limit reached

@DiamondDagger590, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 53 minutes and 30 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 07f80200-eec5-4534-a661-de6b035845e7

📥 Commits

Reviewing files that changed from the base of the PR and between fcb6a13 and d13035a.

📒 Files selected for processing (5)

• src/test/java/com/diamonddagger590/mccore/database/DatabaseRecordTest.java
• src/test/java/com/diamonddagger590/mccore/event/player/PlayerEventTest.java
• src/test/java/com/diamonddagger590/mccore/event/setting/setting/PlayerSettingChangeEventTest.java
• src/test/java/com/diamonddagger590/mccore/exception/CorePlayerOfflineExceptionTest.java
• src/test/java/com/diamonddagger590/mccore/player/CorePlayerTest.java

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/stoic-cori-3bk26a

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}