[Tests] Add unit tests for database events, ManagerKey, and improve Pair/Parser coverage

[DiamondDagger590]

Summary

• DatabaseTableEventsTest: New test class covering all 4 database table event classes (PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, TablesUpdatedEvent) — tests construction, getHandlers(), getHandlerList(), verifies handler list sharing across instances, and asserts pairwise distinctness across event types. Coverage: 0% → 100%.
• ManagerKeyTest: New test class covering the 3 static ManagerKey constants (COMMAND, RELOADABLE_CONTENT, CHAT_RESPONSE), verifying each resolves to the correct manager class. Coverage: 0% → 100%.
• PairTest improvements: Added direct equals() calls for non-Pair objects and null to cover the !(obj instanceof Pair) branch, plus null-handling tests for hashCode(). Coverage: 94.1% → 100%.
• ParserTest improvements: Added error-path tests for trailing tokens after valid expressions, unmatched closing parentheses, and missing function brackets. Strengthened getInputString to assert exact sanitized value and getTree caching to verify object identity. Coverage: 96.7% → 98.0%.

Overall line coverage improved from 33.0% to 33.5% (1338 → 1360 lines covered out of 4055).

Test plan

• All 698 tests pass (./gradlew test)
• JaCoCo report confirms coverage improvements for all target classes
• No changes to production code

🤖 Generated with Claude Code

https://claude.ai/code/session_01X2Mb9p5PuTL2bSW5RMWcWF

[coderabbitai]

Walkthrough

Adds new JUnit 5 test classes and methods across four modules: DatabaseTableEventsTest for Bukkit HandlerList behavior across four event types, expanded PairTest for equals/hashCode edge cases, additional ParserTest cases for malformed expression error handling and caching, and ManagerKeyTest for enum constant validation.

Changes

Test Coverage Expansion

Layer / File(s)	Summary
DatabaseTableEvents HandlerList tests src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java	New test class asserting getHandlers()/getHandlerList() non-nullity, reference equality within an event type, shared HandlerList across instances of the same event type, and distinct HandlerList objects across all four event types (PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, TablesUpdatedEvent).
Pair equals/hashCode edge cases src/test/java/com/diamonddagger590/mccore/pair/PairTest.java	Adds assertFalse import and new tests for equals with non-Pair and null arguments, hashCode with null left/right/both fields on MutablePair, and equality of two MutablePair instances with null in both positions.
Parser error handling and caching src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java	Adds ParseError tests for trailing extra tokens, unmatched closing parenthesis, and missing closing bracket; adds getInputString() non-empty assertion and getTree() caching test comparing getValue() results.
ManagerKey enum validation src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java	New test class asserting each ManagerKey constant (COMMAND, RELOADABLE_CONTENT, CHAT_RESPONSE) is non-null and managerClass() returns the expected manager class.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• DiamondDagger590/McCore#24: Modifies the same ParserTest.java file by adding parser error-handling test cases.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: added and expanded unit tests across database events, ManagerKey, Pair, and Parser.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

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

---

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.}

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff contains only test code additions with no changes to production source files.

No extensibility concerns found.

All changes in this diff are confined to the src/test/java directory: new test classes for DatabaseTableEventsTest, ManagerKeyTest, and additions to existing PairTest and ParserTest. No public API surfaces, abstract classes, interfaces, method signatures, registry keys, GUI framework types, or database framework components are modified. There is nothing for a downstream plugin developer to compile against differently, and no framework contracts are altered.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff.

---

Findings

---

CONCERN: allDatabaseEvents_handlerLists_areDistinctPerType asserts only that each HandlerList is non-null, but does not assert that the four handler lists are distinct from one another.
WHY: The test's @DisplayName explicitly states "each type has its own distinct HandlerList," but no assertion verifies distinctness (e.g., assertNotSame(preCreate, created)). The four assertNotNull calls could all pass even if every event returned the same static HandlerList instance. This is a structural correctness problem: the test cannot falsify the claim it advertises.
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java – allDatabaseEvents_handlerLists_areDistinctPerType

---

CONCERN: DatabaseTableEventsTest constructs Bukkit Event subclasses but uses no MockBukkit setup, yet HandlerList is a live Bukkit API class whose static state accumulates across test runs.
WHY: HandlerList is a real Bukkit runtime class with mutable static state (handlers list). Constructing events and calling getHandlerList() without MockBukkit.mock() / MockBukkit.unmock() can cause static handler registration to leak between test classes in the same JVM run. Because each event's static HandlerList is never cleared, subsequent test runs in the same process will see registrations from earlier runs. Either MockBukkit should be set up and torn down per class, or — if no Bukkit server is actually needed — the test must explicitly verify that the framework itself isolates these statics (which it does not currently do).
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java

---

CONCERN: hashCode_handlesNullLeft_whenLeftIsNull hard-codes the expected hash value 42 and hashCode_handlesNullRight_whenRightIsNull hard-codes "test".hashCode(). These assertions encode an assumed implementation of hashCode rather than verifying a contract.
WHY: If the Pair hashCode implementation changes (e.g., adds a prime multiplier, XORs components), the tests will fail for the wrong reason — they are testing a private implementation detail, not a publicly documented contract. The correct structural assertion is that the hash is consistent across multiple calls (stability contract) and that two equal pairs produce the same hash (equals–hashCode contract). Asserting a concrete numeric value for a composed hash is a structural fragility problem, not a style preference.
WHERE: src/test/java/com/diamonddagger590/mccore/pair/PairTest.java – hashCode_handlesNullLeft_whenLeftIsNull, hashCode_handlesNullRight_whenRightIsNull

---

CONCERN: getInputString_returnsSanitizedInput_whenCalled asserts only assertFalse(input.isEmpty()) — it does not verify what sanitization actually means (e.g., spaces stripped, case-normalised, whitespace collapsed).
WHY: The test's @DisplayName promises it tests "sanitized input," but a non-empty string satisfies the assertion regardless of whether sanitization occurred at all. This is a coverage gap: the non-trivial sanitization logic inside Parser is not exercised by this assertion and cannot be falsified by it.
WHERE: src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java – getInputString_returnsSanitizedInput_whenCalled

---

CONCERN: getTree_returnsCachedTree_whenCalledTwice asserts tree1.getValue() == tree2.getValue() but does not assert tree1 == tree2 (or assertSame). If caching is the behaviour under test, object identity must be checked.
WHY: Two independently constructed trees that happen to evaluate the same expression will also produce equal getValue() results, so the assertion passes even if no caching exists at all. The structural intent of the test (verify the tree is cached / same object returned) is not captured by value equality.
WHERE: src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java – getTree_returnsCachedTree_whenCalledTwice

---

CONCERN: ManagerKeyTest contains no MockBukkit setup, yet CoreCommandManager, ReloadableContentManager, and ChatResponseManager are Bukkit-coupled manager classes. If any of these classes or their static initializers reference Bukkit APIs, the tests will fail at runtime with no Bukkit server present.
WHY: Only ManagerKey.managerClass() (which returns a Class<?> literal) is exercised here, so no Bukkit API is actually called. This means the tests are plain Java tests that do not need MockBukkit — but the risk is that importing those manager classes could trigger static initializers that require a Bukkit context. If they do not, these tests are correctly plain JUnit tests. If they do, MockBukkit must be set up. The diff does not show those manager class bodies, so this must be flagged as an unverified structural risk.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java

---

CONCERN: No tests cover the production-side logic of PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, or TablesUpdatedEvent being fired and received by a registered listener. The tests only verify the HandlerList plumbing.
WHY: The non-trivial contract of these events is that they are correctly dispatched through Bukkit's event system and that a registered Listener receives them. Testing only getHandlers() and getHandlerList() leaves the actual event-firing path untested. This is a coverage gap for the primary purpose of these classes.
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java (gap in coverage of production classes PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, TablesUpdatedEvent)

---

Summary

Production files changed: PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, TablesUpdatedEvent (event classes), Pair/ImmutablePair/MutablePair (pair additions), Parser (parser additions), ManagerKey (registry manager enum)

Test files present:

• src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java
• src/test/java/com/diamonddagger590/mccore/pair/PairTest.java
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java

Coverage gaps:

1. Distinctness of HandlerList instances not asserted despite being advertised
2. Bukkit static HandlerList state not isolated (no MockBukkit lifecycle)
3. hashCode tests assert implementation-specific numeric values instead of contractual properties
4. getInputString sanitization behaviour not actually verified
5. getTree caching not verified by object identity
6. Bukkit-context risk in ManagerKeyTest uninvestigated
7. Event dispatch and listener receipt path entirely untested

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code (src/test/java/...): unit tests for database table events, pair utilities, expression parser edge cases, and manager key enum values. No production framework code is modified. None of the checklist items apply — there are no DAO methods, DDL construction, MiniMessage deserialization calls, ChatResponse implementations, onClick() overrides, or permission-gated operations in the changed files.

[coderabbitai]

Actionable comments posted: 3

🤖 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/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java`:
- Around line 100-111: The test in DatabaseTableEventsTest only checks that the
HandlerList values from PreTablesCreateEvent, TablesCreatedEvent,
PreTablesUpdateEvent, and TablesUpdatedEvent are non-null, but it does not
verify they are actually distinct. Update
allDatabaseEvents_handlerLists_areDistinctPerType to assert pairwise
non-identity between the handler lists returned by each getHandlerList() call so
the test fails if any event class shares the same static HandlerList instance.

In `@src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java`:
- Around line 437-443: The new tests are too weak for their stated behavior: in
ParserTest, getInputString_returnsSanitizedInput_whenCalled should assert the
actual sanitized value returned by Parser.getInputString(), not just that it is
non-empty, and the caching-related test around the tree comparison should verify
Parser’s cache-specific behavior directly instead of only comparing evaluation
results. Tighten the assertions so they fail if sanitization or caching
regresses, using the existing Parser, getInputString, and tree-evaluation
helpers already present in this test file.

In
`@src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java`:
- Line 16: Rename the affected tests in ManagerKeyTest to follow the repository
convention methodUnderTest_expectedOutcome_whenCondition, since the current
names like command_managerClass_returnsCoreCommandManagerClass omit the required
_whenCondition suffix. Update each of the test method names referenced in the
diff so they clearly encode the condition, while keeping the method-under-test
and expected-outcome parts intact.

🪄 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: ASSERTIVE

Plan: Pro

Run ID: 0962c4dd-5c5c-405d-8066-512398cbdcb6

📥 Commits

Reviewing files that changed from the base of the PR and between 17bed82 and 8196a21.

📒 Files selected for processing (4)

• src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java
• src/test/java/com/diamonddagger590/mccore/pair/PairTest.java
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff contains only new test classes and test method additions with no changes to production source code.

No extensibility concerns found.

All changes are confined to src/test/java, adding unit tests for existing event classes (PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, TablesUpdatedEvent), the Pair hierarchy (ImmutablePair, MutablePair), Parser/ExpressionNode, and ManagerKey. No public API surfaces, abstract classes, interfaces, method signatures, registry constants, GUI framework types, or database framework types are modified. There is nothing here that affects binary or behavioral compatibility for downstream plugin consumers.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff.

---

Analysis

Production files changed

The diff contains only test file additions/modifications — no production source files are changed. This means I must evaluate:

1. Whether the tests themselves are structurally correct
2. Whether the tests are testing real behavior or just scaffolding
3. Whether the new test files introduce any structural problems

---

Findings

---

CONCERN: DatabaseTableEventsTest spins up Bukkit HandlerList instances directly without MockBukkit but HandlerList is a Bukkit API class. Whether this compiles and runs cleanly depends entirely on MockBukkit or a Bukkit stub being on the test classpath. If HandlerList has static state that bleeds between test classes (which it does — Bukkit's HandlerList maintains a static registry of all handler lists), that state is never cleared between tests or test runs.
WHY: Bukkit's HandlerList.unregisterAll() / HandlerList static state persists across test methods in the same JVM. The test class has no @BeforeEach/@AfterEach to reset this. This is not a style issue — it is a structural isolation failure. Any test that registers a listener into one of these HandlerList instances in a later run will see stale state.
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java

---

CONCERN: DatabaseTableEventsTest uses zero MockBukkit setup (MockBukkit.mock() / MockBukkit.unmock()) yet directly instantiates Bukkit event classes that extend org.bukkit.event.Event. The test class has no @BeforeAll/@AfterAll or @BeforeEach/@AfterEach lifecycle hooks at all.
WHY: org.bukkit.event.Event subclasses rely on Bukkit's internals. If any of the four event classes under test register themselves with Bukkit's event system during construction (which is the standard Bukkit pattern — HandlerList is static per class), then running this test without a MockBukkit server context may either silently succeed with an improperly initialized environment or fail non-deterministically depending on test execution order. The structural requirement is: if Bukkit classes are instantiated, MockBukkit must be set up and torn down.
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java

---

CONCERN: allDatabaseEvents_handlerLists_areDistinctPerType asserts that each event type's HandlerList is a distinct instance. This assertion is only valid on the first run of the test suite in a JVM. Because HandlerList instances are held in static fields on each event class, a second test execution within the same JVM (e.g., running tests twice in the same Gradle daemon) will see the same static instances — but that is not the real problem. The problem is that this test is asserting implementation details of static state without any guarantee that the static fields were freshly initialized. If another test class in the suite has already caused these classes to load, the assertion still passes, but the test provides no real isolation guarantee.
WHY: Coverage gap: there is no test verifying that the HandlerList static state is reset between uses, which matters for Bukkit event infrastructure. The test validates identity but not lifecycle correctness.
WHERE: src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java

---

CONCERN: The hashCode tests for MutablePair assert specific computed values (assertEquals(42, hash) and assertEquals("test".hashCode(), hash)). These assertions encode assumptions about the exact hashCode implementation (e.g., that it is left.hashCode() + right.hashCode() or similar additive formula). If the production hashCode implementation changes to a standard Objects.hash(left, right) or a prime-multiplier formula, these tests will break without the implementation being wrong — they are testing an implementation detail rather than a contract.
WHY: Structural problem: asserting exact numeric hash values couples tests to implementation internals. The correct assertion for "handles null gracefully" is that hashCode() does not throw (i.e., the test should assert no exception is thrown), not that it returns a specific integer. As written, assertEquals(42, hash) will fail if the implementation uses Objects.hash(null, 42) which returns a different value.
WHERE: src/test/java/com/diamonddagger590/mccore/pair/PairTest.java — methods hashCode_handlesNullLeft_whenLeftIsNull, hashCode_handlesNullRight_whenRightIsNull, hashCode_returnsZero_whenBothSidesAreNull

---

CONCERN: hashCode_returnsZero_whenBothSidesAreNull asserts assertEquals(0, pair.hashCode()). This is an exact-value assertion against a specific implementation. Objects.hash(null, null) returns 961, not 0. If the production code uses any standard hash combinator other than a bare null-check sum, this test will fail falsely. Worse, if the production code does return 0 today, this test will silently pass while masking a potentially poor hash distribution.
WHY: Coverage gap and structural problem: the test does not assert the contract (no NPE on null inputs) — it asserts a magic number that is only correct for one specific implementation.
WHERE: src/test/java/com/diamonddagger590/mccore/pair/PairTest.java — hashCode_returnsZero_whenBothSidesAreNull

---

CONCERN: getTree_returnsSameCachedInstance_whenCalledTwice uses assertSame to verify that Parser.getTree() returns a cached ExpressionNode. However, there is no production file changed in this diff that introduces or modifies a getTree() caching behavior. If getTree() already existed and was already cached, this test adds no regression value. If getTree() was not previously cached, this test is asserting new behavior with no corresponding production change in the diff — meaning the test either always passed (caching was already there) and adds no value, or it is testing a production change that is missing from the diff.
WHY: Coverage gap: the diff shows no production change to Parser that would justify this new test. Either the production change is absent from the diff (a missing production file), or the test is redundant. Either way, the audit cannot confirm coverage is adequate without the corresponding production change.
WHERE: src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java — getTree_returnsSameCachedInstance_whenCalledTwice / production class Parser

---

CONCERN: ManagerKeyTest tests only that three specific ManagerKey enum constants exist and return the correct class. There is no test for: (a) that ManagerKey.values() is exhaustive (i.e., no constant is untested), (b) that a null or unknown key is handled, or (c) that managerClass() never returns null for any constant. If a new ManagerKey constant is added in the future, these tests will not catch a misconfigured managerClass() return.
WHY: Coverage gap: enum coverage is partial. Only 3 of N constants are tested. There is no exhaustive assertion (e.g., for (ManagerKey key : ManagerKey.values()) assertNotNull(key.managerClass())), which means future enum additions have zero test coverage by default.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java

---

CONCERN: ManagerKeyTest imports and references ChatResponseManager, CoreCommandManager, and ReloadableContentManager — all of which are McCore framework classes. If any of these classes have Bukkit-dependent constructors or static initializers, loading their Class objects via ManagerKey.COMMAND.managerClass() may trigger class loading that requires a Bukkit environment. The test has no MockBukkit setup.
WHY: Structural problem: class-literal references in an enum (managerClass() returning CoreCommandManager.class) trigger class loading. If CoreCommandManager or ChatResponseManager have static Bukkit dependencies, this test will fail in a plain JUnit context without MockBukkit. No MockBukkit lifecycle is present in this test class.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java

---

CONCERN: None of the new test files place any shared fixtures in src/testFixtures/java/. The DatabaseTableEventsTest in particular tests event infrastructure that downstream repos (McRPG) would likely need to fire and assert against in integration tests. No test fixtures for event construction helpers are provided.
WHY: Coverage gap: per the checklist, shared fixtures must be in src/testFixtures/java/ so downstream repos can depend on them. Event construction helpers for PreTablesCreateEvent, TablesCreatedEvent, PreTablesUpdateEvent, and TablesUpdatedEvent are candidates for test fixtures that are absent.
WHERE: src/testFixtures/java/ — directory not touched by this diff

---

Summary

Production files changed: None (diff contains only test additions)

Test files present:

• src/test/java/com/diamonddagger590/mccore/event/database/DatabaseTableEventsTest.java (new)
• src/test/java/com/diamonddagger590/mccore/pair/PairTest.java (modified)
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java (modified)
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerKeyTest.java (new)

Coverage gaps:

1. DatabaseTableEventsTest — no MockBukkit lifecycle; Bukkit static HandlerList state is never reset; structural isolation failure
2. DatabaseTableEventsTest — no test fixtures provided for downstream event consumers
3. PairTest — hashCode null tests assert specific magic numbers tied to one implementation; contract (no NPE) is not the assertion being made
4. ParserTest — getTree() caching test has no corresponding production change in the diff; coverage justification is unverifiable
5. ManagerKeyTest — enum constant coverage is non-exhaustive; no values() sweep; potential Bukkit class-loading issue without MockBukkit setup

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code additions and modifications (src/test/java/...). All changes are unit tests covering event handler list identity, pair equality/hashing, parser error handling, and manager key constants. No production framework code — no DAO methods, DDL functions, MiniMessage deserialization calls, ChatResponse implementations, onClick() overrides, or permission-gated operations — is added or modified. None of the checklist items apply to test-only code.