[Tests] Add unit tests for ManagerRegistry, PluginHookRegistry, TimeProvider, DriverRegistry

[DiamondDagger590]

Summary

• ManagerRegistryTest (11 tests): Covers register, duplicate detection, registered checks by instance and ManagerKey, manager retrieval by key, subclass lookup support (the special-casing for generic managers like GuiManager/PlayerManager), and null return for unregistered keys.
• PluginHookRegistryTest (11 tests): Covers register, duplicate detection, registered checks, pluginHook by key returning Optional, pluginHooks filtering by extensible interface class (e.g. CustomBlockHook), mixed-hook queries, and empty registry behavior.
• TimeProviderTest (7 tests): Covers now() with fixed clock, repeated calls returning same instant, nowLocal() with UTC/positive/negative zone offsets, and clock() accessor.
• DatabaseDriverTypeTest (7 tests): Covers getDriverName(), fromString() with exact/case-insensitive/mixed-case matching, invalid/empty input returning empty Optional, and values() enumeration.
• DriverRegistryTest (8 tests): Covers register with tryDriver validation (success and failure), registered by instance, isDriverRegistered by DatabaseDriverType, getDriver returning Optional for registered/unregistered types.

Also fixes two pre-existing compilation/test failures:

• ChainComparatorTest: Added explicit type witnesses (Comparator.<Integer>naturalOrder() / reverseOrder()) to satisfy ChainComparator generic inference with varargs constructor
• ParserTest: Changed function-without-parenthesis test input from "sin 5" to "sin" — spaces are stripped by the parser, so "sin5" is treated as a variable name rather than a function call

Coverage improvements

Package	Before	After
registry/manager	8.6%	74.3%
database/driver	0%	73.9%
registry/plugin	11.1%	66.7%
Overall	19.3%	20.8%

All 42 new tests target pure-Java classes with zero MockBukkit or server mock required. All tests use Given/When/Then @DisplayName annotations.

Test plan

• ./gradlew test passes with all 216 tests green (174 existing + 42 new)
• ./gradlew shadowJar builds successfully
• Existing statistic, parser, pair, linked-node, chain-comparator, and chain-filter tests remain unaffected
• No production code changes beyond test fixes

https://claude.ai/code/session_01N7Qpd7kWuEaSXAvFgGJhCa

---

Generated by Claude Code

Summary by CodeRabbit

Release Notes

• Tests

  • Expanded test coverage for database drivers, registry operations, parser behavior, and utility functions.
  • Added comprehensive test suites validating driver registration, manager registry, plugin hook registry, and time provider functionality.

• Documentation

  • Updated testing guidelines emphasizing MockBukkit best practices, boundary value testing, and separation of pure-Java unit tests from framework-dependent tests.

[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 48 minutes and 44 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: 9c307fed-b759-4cf5-af81-1e67666f4f3c

📥 Commits

Reviewing files that changed from the base of the PR and between 89d252d and 04a9247.

📒 Files selected for processing (1)

• src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java

Walkthrough

This pull request expands test coverage across registry, utility, and parser components while refining testing personas and guidelines. The changes add five new test classes and enhance existing tests, with documentation updates to clarify testing patterns around boundary values, MockBukkit player setup, and test quality expectations.

Changes

Test Coverage Expansion and Testing Guidance Refinement

Layer / File(s)	Summary
Testing Personas and Guidelines .claude/commands/review-testing.md, .cursor/rules/persona-testing.mdc	Documentation checklists updated with specific guidance for testing config-driven ReloadableContent paths at boundary values, preferred MockBukkit patterns (server.addPlayer()), Bukkit-dependent vs. pure-Java test separation, and refined @DisplayName Given/When/Then format requirements.
Database Driver Type and Registry Tests src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java, src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java	DatabaseDriverTypeTest validates enum driver name retrieval and case-insensitive string parsing. DriverRegistryTest defines a parameterized TestDriver and covers registration lifecycle, presence checks, type-based driver retrieval, and driver overwriting behavior.
Plugin Hook and Manager Registry Tests src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java, src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java	PluginHookRegistryTest covers hook registration, presence checks, key-based and interface-based filtering, with empty-case handling. ManagerRegistryTest validates manager registration, retrieval by key, subclass-to-parent-key mapping, and per-test isolation using RegistryResetExtension.
Time Provider and Comparator Type Witness Tests src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java, src/test/java/com/diamonddagger590/mccore/util/comparator/ChainComparatorTest.java	TimeProviderTest validates fixed-clock behavior across now() consistency and nowLocal(ZoneId) timezone conversion. ChainComparatorTest updated with explicit generic type witnesses for comparator instances.
Parser Invalid Function Syntax Tests src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java	Expanded error coverage to assert ParseError for function names without parentheses and document whitespace-stripping behavior where space-separated function and argument are treated as a variable.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• DiamondDagger590/McCore#5: Previous PR that established the Testing Auditor Persona framework; this PR refines those same documentation guidelines with additional boundary-value and MockBukkit usage patterns.

🚥 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically describes the main change: adding unit tests for four registry/utility classes (ManagerRegistry, PluginHookRegistry, TimeProvider, DriverRegistry), which aligns with the primary objective of the PR.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/zen-hamilton-KpdSt

---

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

[github-actions]

Extensibility Review

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

All modified and added files reside exclusively under src/test/java/. No public API, abstract class, interface, registry constant, method signature, or production source file is touched.

Specific observations:

• DatabaseDriverTypeTest.java — new test class; exercises existing production DatabaseDriverType enum. No production change.
• DriverRegistryTest.java — new test class; exercises existing production DriverRegistry and DatabaseDriver interface via a local test-only implementation. No production change.
• ParserTest.java — one test input changed from "sin 5" to "sin" to better isolate the "missing parenthesis" error path. No production change.
• ManagerRegistryTest.java — new test class; exercises existing production ManagerRegistry. No production change.
• PluginHookRegistryTest.java — new test class; exercises existing production PluginHookRegistry. No production change.
• TimeProviderTest.java — new test class; exercises existing production TimeProvider. No production change.
• ChainComparatorTest.java — two lines updated to use explicit type witnesses (Comparator.<Integer>naturalOrder(), Comparator.<Integer>reverseOrder()) to satisfy stricter compiler inference. No production change.

No extensibility concerns found.

[github-actions]

Testing Review

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

---

Analysis

Production Files Changed

The diff contains only test files and one test-fixture helper (RegistryResetExtension referenced but not shown). No production source files are modified. The one semantic change to a production-adjacent area is a test fix in ParserTest ("sin 5" → "sin"), which is purely a test correction.

---

Findings

---

CONCERN: RegistryResetExtension is used in both ManagerRegistryTest and PluginHookRegistryTest but is not present in src/testFixtures/java/ in this diff — it is referenced from com.diamonddagger590.mccore.testing.RegistryResetExtension and called as static utility methods (setupRegistry() / resetRegistry()), yet no definition appears anywhere in the diff.
WHY: If RegistryResetExtension lives in src/test/java/ rather than src/testFixtures/java/, downstream repositories (McRPG) that depend on these registry patterns cannot reuse the fixture. If it doesn't exist at all, both test classes fail to compile. Either way, shared test infrastructure must be in src/testFixtures/java/.
WHERE: ManagerRegistryTest.java, PluginHookRegistryTest.java → com.diamonddagger590.mccore.testing.RegistryResetExtension

---

CONCERN: ManagerRegistryTest and PluginHookRegistryTest use RegistryResetExtension.setupRegistry() / resetRegistry() as plain static calls inside @BeforeEach / @AfterEach rather than as a registered JUnit 5 @ExtendWith extension. If RegistryResetExtension implements BeforeEachCallback / AfterEachCallback, the lifecycle contract is being broken — extension callbacks won't fire for exceptions thrown in @BeforeEach, and if a test throws before tearDown() the registry state leaks into subsequent tests.
WHY: Structural problem: JUnit 5 extensions guarantee cleanup even on failure; manual @AfterEach calls do not if the test body throws before reaching tearDown(). The correct pattern is @ExtendWith(RegistryResetExtension.class).
WHERE: ManagerRegistryTest.java (@BeforeEach setUp / @AfterEach tearDown), PluginHookRegistryTest.java (same)

---

CONCERN: ManagerRegistryTest constructs TestManagerA and TestManagerB by passing null as the plugin argument (super(null)). If Manager performs any null-check on the plugin reference during construction or during any method exercised by the test, the tests will throw NullPointerException rather than failing with a meaningful assertion.
WHY: Coverage gap / structural fragility: the test relies on undefined behavior of the Manager superclass constructor. There is no assertion or documented expectation that null is an accepted value. A dedicated NullPointerException guard test or a proper stub/mock for CorePlugin should be used.
WHERE: ManagerRegistryTest.java (TestManagerA, TestManagerB inner classes)

---

CONCERN: PluginHookRegistryTest has the same super(null) pattern for PluginHook<CorePlugin> subclasses (TestHookA, TestHookB, TestHookC).
WHY: Same structural fragility as above — if PluginHook performs a null-check or uses the plugin reference internally, all tests in this file fail with NPE rather than a meaningful assertion failure. No null acceptance contract is asserted.
WHERE: PluginHookRegistryTest.java (TestHookA, TestHookB, TestHookC inner classes)

---

CONCERN: DatabaseDriverTypeTest.values_containsSqlite asserts assertEquals(1, values.length) — this is a structural assertion about the enum's total member count. If a second driver type (e.g., MYSQL) is added to DatabaseDriverType in the future, this test fails for the wrong reason (it is not testing behavior, it is hard-coding the enum size).
WHY: Structural problem: enum-size assertions are brittle and mask the actual intent (verifying SQLITE is present). The test should use assertTrue(Arrays.asList(values).contains(DatabaseDriverType.SQLITE)) rather than asserting the total count.
WHERE: DatabaseDriverTypeTest.java (values_containsSqlite)

---

CONCERN: DriverRegistryTest.register_throwsRuntimeException_whenDriverClassMissing uses new TestDriver(false) to trigger failure, where tryDriver() returns false. The TestDriver hard-codes getDatabaseDriverClass() to return "org.sqlite.JDBC". The test name says "when driver class is missing" but the actual mechanism is controlled by a boolean flag — whether the real DriverRegistry.register() calls tryDriver() to gate registration, or whether it calls Class.forName() on the driver class string, is not verifiable from this test alone. If the production code uses Class.forName() and the SQLite JDBC driver is on the test classpath, tryDriver() returning false may be irrelevant and the exception may not be thrown.
WHY: Coverage gap: the test's assumption about how DriverRegistry validates a driver is implicit and untested at the unit level. There is no test verifying the path where tryDriver() returns true but the driver class is genuinely absent from the classpath.
WHERE: DriverRegistryTest.java (register_throwsRuntimeException_whenDriverClassMissing)

---

CONCERN: TimeProviderTest has no @AfterEach resetting any global TimeProvider state. This is acceptable only if TimeProvider instances are purely local (injected, not static singletons). If TimeProvider is ever stored as a static field anywhere in the production code and these tests mutate it, state would leak. The tests currently construct local instances only — but there is no test covering a global/static accessor pattern (if one exists on TimeProvider).
WHY: Conditional coverage gap: if TimeProvider exposes a static setInstance() or similar, that path has no reset guard and no test. Based on the diff this appears safe, but should be confirmed against the full production class.
WHERE: TimeProviderTest.java

---

CONCERN: ParserTest.getValue_throwsParseError_whenFunctionMissingParenthesis was changed from "sin 5" to "sin". The original input "sin 5" (a function name followed by a space and argument without parentheses) was a more realistic edge case for the "missing parenthesis" scenario. The replacement "sin" (bare identifier with no argument at all) may be testing a different code path (missing argument vs. missing parenthesis). There is now no test for "sin 5" — a function name with a space-separated argument that is syntactically invalid.
WHY: Coverage gap: the edge case of a function name followed by a non-parenthesized argument ("sin 5") is no longer covered after this change.
WHERE: ParserTest.java (getValue_throwsParseError_whenFunctionMissingParenthesis)

---

Summary

Production files changed: None (all changes are in test files)

Test files present:

• DatabaseDriverTypeTest.java (new)
• DriverRegistryTest.java (new)
• ParserTest.java (modified)
• ManagerRegistryTest.java (new)
• PluginHookRegistryTest.java (new)
• TimeProviderTest.java (new)
• ChainComparatorTest.java (modified — compiler-hint-only changes, no behavioral change)

Coverage gaps:

1. RegistryResetExtension location unknown — not confirmed in src/testFixtures/java/
2. Manual @BeforeEach/@AfterEach registry reset instead of @ExtendWith — leaks on exception
3. null plugin passed to Manager and PluginHook constructors — undefined contract, fragile
4. Enum-size hard-coding in values_containsSqlite — brittle structural assertion
5. DriverRegistry exception path relies on implicit tryDriver() contract without classpath-absent driver test
6. "sin 5" edge case removed from ParserTest with no replacement coverage
7. No test for static/global TimeProvider accessor pattern (if it exists)

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code additions and minor test corrections (a Parser input change and explicit type witnesses on Comparator calls). No production DAO methods, UpdateTableFunction implementations, MiniMessage deserialization call sites, ChatResponse handlers, Slot.onClick() overrides, or permission-gated operations were added or modified. There is nothing in the changed code that touches user-influenced data paths, query construction, or framework-level security controls.

[DiamondDagger590]

Addressing Testing Review Feedback

Actionable items fixed in b03301e:

Fixed:

• Add comprehensive code guide and cursor rules for McCore #4 (brittle enum-size assertion): Changed values_containsSqlite to use Arrays.asList(values).contains(SQLITE) instead of hardcoding assertEquals(1, values.length).
• Fix post-edit-compile hook execution by explicitly using bash #6 ("sin 5" edge case): Added a new test getValue_treatsAsVariable_whenFunctionNameSpaceSeparatedFromArgument that documents the actual behavior — spaces are stripped by the parser's convertInput(), so "sin 5" becomes "sin5" which is parsed as a variable (returning 0.0), not as a function call missing parentheses.

Not changed (by design):

• Update PaperMC and Cloud dependencies to latest versions #1: RegistryResetExtension is already in src/testFixtures/java/com/diamonddagger590/mccore/testing/ — it wasn't in the diff because it wasn't modified.
• Setup maven publishing #2: JUnit 5's @AfterEach runs even when tests throw, so there's no state leak risk. Additionally, RegistryResetExtension is a plain utility class with static methods (not a JUnit extension implementing BeforeEachCallback/AfterEachCallback), so @ExtendWith isn't applicable. This follows the established pattern from StatisticRegistryTest.
• Fix ArrayStoreException in BaseItemBuilder placeholder handling #3: null plugin in Manager/PluginHook test doubles is pragmatic — @NotNull is a compile-time hint only, and these test doubles never invoke plugin(). We're testing the registry, not the managers themselves. Adding Mockito as a dependency for this would be overkill.
• Add AI-powered PR review automation and developer tooling #5: DriverRegistryTest correctly tests the production path — DriverRegistry.register() calls tryDriver() and the test override controls its return value. This is standard unit testing (overriding an interface method to control test conditions).
• Add command guides and MCP configuration for developer workflow #7: TimeProvider is a record with no static singleton or mutable global state. Each test constructs its own local instance.

---

Generated by Claude Code

[github-actions]

Extensibility Review

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

All modified and added files reside exclusively under src/test/java/. No public API, abstract class, interface, registry constant, method signature, or production class is altered. The one change to a pre-existing test (ParserTest) corrects a test assertion and adds a new test case; it does not modify the Parser class itself.

No extensibility concerns found.

[github-actions]

Testing Review

Testing Audit Report — McCore

---

CONCERN: ManagerRegistryTest depends on RegistryResetExtension.setupRegistry() / resetRegistry() which lives in src/test/java/ (implied by import com.diamonddagger590.mccore.testing.RegistryResetExtension), not in src/testFixtures/java/
WHY: The checklist requires shared fixtures to be placed in src/testFixtures/java/ so downstream repos (McRPG) can depend on them. RegistryResetExtension is used by both ManagerRegistryTest and PluginHookRegistryTest, making it a shared fixture. If it lives in src/test/java/ it cannot be consumed by downstream test classpaths.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java, src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java — fixture class com.diamonddagger590.mccore.testing.RegistryResetExtension

---

CONCERN: DriverRegistryTest does not test registering null as a driver.
WHY: register() accepts a @NotNull-annotated parameter, but the checklist requires null-input edge cases to be covered for non-trivial public methods. If the null-check is enforced only by annotation (no explicit throw), passing null at runtime bypasses it silently. There is no test verifying the contract fails fast on null.
WHERE: src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java / DriverRegistry.register()

---

CONCERN: DriverRegistryTest does not test registering two different drivers that share the same DatabaseDriverType.
WHY: DriverRegistry maps by DatabaseDriverType. If a second driver with SQLITE type is registered after the first, the behavior (overwrite, throw, or silently ignore) is untested. This is a non-trivial edge case for the registry's key-collision path.
WHERE: src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java / DriverRegistry

---

CONCERN: DatabaseDriverTypeTest enumerates only SQLITE but does not test null input to fromString.
WHY: fromString(null) is a standard null-input edge case. If the implementation calls String.equalsIgnoreCase() on null it will throw NullPointerException rather than return Optional.empty(). The checklist explicitly requires null-input edge cases for non-trivial public methods.
WHERE: src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java / DatabaseDriverType.fromString()

---

CONCERN: ManagerRegistryTest passes null as the CorePlugin argument to Manager constructors (super(null)) but does not test what happens when ManagerRegistry.register() itself receives null.
WHY: Null-input coverage is required for every non-trivial public method. The register(Manager) path is untested for a null argument.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java / ManagerRegistry.register()

---

CONCERN: PluginHookRegistryTest similarly omits a null-input test for register() and for pluginHooks(null) (querying by a null interface class).
WHY: Same null-input checklist requirement. pluginHooks(Class<T>) iterating registered hooks against a null class could throw NullPointerException or silently return empty — neither path is exercised.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java / PluginHookRegistry.register(), PluginHookRegistry.pluginHooks()

---

CONCERN: TimeProviderTest does not reset any shared TimeProvider state in @AfterEach.
WHY: The checklist states: "If a test modifies TimeProvider state, is that state reset in @AfterEach?" These tests construct local TimeProvider instances, which is correct isolation. However, if TimeProvider has any static/singleton state (e.g., a default instance), tests that modify it would leak. The tests do not demonstrate awareness of this requirement — there is no @AfterEach guard at all. If a static default provider exists in production code this becomes a real leak vector.
WHERE: src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java / TimeProvider

---

CONCERN: The ParserTest change replaces a test that verified "sin 5" throws ParseError with a test that asserts it evaluates to 0.0, but no regression test documents why the original behavior was changed or what bug was fixed.
WHY: The checklist requires: "If a bug was fixed, is there a regression test?" The diff comment says "sin 5" is now treated as variable "sin5" returning 0.0. If this is a bug fix (the parser should not crash on a space), there is no test named or annotated to mark it as a regression guard. If it is a behaviour clarification, the old test being removed without a replacement that documents the original failure is a coverage gap.
WHERE: src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java / Parser

---

CONCERN: No test covers the DriverRegistry.getDataSourceProperties() interaction path — specifically, that properties returned by a registered driver are actually applied when a data source is configured.
WHY: getDataSourceProperties() is a non-trivial public method on DatabaseDriver returned as a List<Pair<String,String>>. The TestDriver stub returns List.of() in every test. The registry's behaviour when a driver returns non-empty properties (and whether those are passed through correctly) is entirely untested.
WHERE: src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java / DatabaseDriver.getDataSourceProperties()

---

Summary

Production files changed: DatabaseDriverType, DriverRegistry, DatabaseDriver (interface), Parser, ManagerRegistry, PluginHookRegistry, TimeProvider, ChainComparator

Test files present: DatabaseDriverTypeTest, DriverRegistryTest, ParserTest, ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest, ChainComparatorTest

Coverage gaps:

• null inputs to fromString, register, and pluginHooks across all new test classes
• Duplicate DatabaseDriverType key collision in DriverRegistry
• getDataSourceProperties() non-empty path in DriverRegistry
• Regression test documentation for the Parser "sin 5" behaviour change
• RegistryResetExtension fixture not in src/testFixtures/java/
• No @AfterEach guard for TimeProvider static state (if any)

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code additions and minor test corrections (a Parser test fixup, two ChainComparator explicit type-witness casts, and new unit tests for DatabaseDriverType, DriverRegistry, ManagerRegistry, PluginHookRegistry, and TimeProvider). No production DAO methods, DDL constructors, MiniMessage deserialization paths, ChatResponse implementations, Slot.onClick() overrides, or permission-gated operations were added or modified.

[github-actions]

Extensibility Review

Breaking change risk: NONE — This diff contains only test files and AI-assistant prompt/rule documentation; no production source files are modified.

Reviewing against each checklist category:

Public interfaces / abstract classes / method signatures: No production code is touched. All changes are confined to src/test/java/, .claude/commands/, and .cursor/rules/. No contracts are altered.

GUI framework (BaseGui, PaginatedGui, Slot, GuiManager): Not touched.

Database framework (UpdateTableFunction, schema migration): Not touched.

Registry and extension points (RegistryKey, ManagerKey constants): Not touched. The test classes reference existing constants (RegistryKey.MANAGER, RegistryKey.PLUGIN_HOOK) as read-only consumers; no string values are changed.

@NotNull / @Nullable contracts: No new public API methods are introduced.

@Deprecated symbols: None added or removed.

McCore plugin-agnostic rule: The test helpers (TestManagerA, TestHookA, etc.) are package-private inner classes inside test sources, not production code, so no consumer-plugin-specific logic bleeds into the framework.

Minor observation (non-breaking, informational only):

CONCERN: DriverRegistryTest.TestDriver hard-codes "org.sqlite.JDBC" and DatabaseDriverType.SQLITE as the only exercised driver type, so the registry's behavior with a second distinct driver type is untested.
WHY: Not a contract issue for downstream consumers, but the test suite gives no signal about whether a non-SQLite driver would be handled correctly by DriverRegistry. This is a coverage gap, not a breaking change.
WHERE: src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java — TestDriver

CONCERN: ManagerRegistryTest passes null as the plugin argument to Manager<CorePlugin> constructors (super(null)). If Manager's constructor or any lifecycle method enforces @NotNull on its plugin parameter, these tests will throw at construction rather than at the assertion under test, producing misleading failure messages.
WHY: Pure test-quality concern; no downstream contract is affected. However, if Manager(P plugin) is annotated @NotNull, this quietly violates the contract inside test code, which could confuse future contributors.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java — TestManagerA, TestManagerB, TestManagerASub constructors; same pattern in PluginHookRegistryTest.

No extensibility concerns found for the production framework contract. All findings above are test-quality observations only.

[github-actions]

Testing Review

Testing Audit Report

Production files changed:

• .claude/commands/review-testing.md (documentation only)
• .cursor/rules/persona-testing.mdc (documentation only)
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java (test file modified)
• src/test/java/com/diamonddagger590/mccore/util/comparator/ChainComparatorTest.java (test file modified)

New test files added:

• src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java
• src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java

---

CONCERN: DriverRegistryTest uses a hardcoded tryDriver() stub but does not test the case where the JDBC class actually exists on the classpath versus is absent. The register_throwsRuntimeException_whenDriverClassMissing test conflates "tryDriver returns false" with "class missing" — the production path for the real SQLite driver likely calls Class.forName(), meaning a false-returning stub may not exercise the actual exception branch.
WHY: If DriverRegistry.register() catches ClassNotFoundException and rethrows as RuntimeException, the test covers that. But if it calls tryDriver() and throws on false, the test is only verifying the boolean-gate path, not the class-loading path. The distinction matters if both paths exist in production. Without seeing the production class, the test's @DisplayName ("when driver class missing") is misleading — the stub just returns false, which is a different condition.
WHERE: DriverRegistryTest.register_throwsRuntimeException_whenDriverClassMissing / DriverRegistry (production)

---

CONCERN: DriverRegistryTest has no test for registering two different driver types (e.g., if a second DatabaseDriverType existed or if two drivers of different types are registered). More critically, there is no test for attempting to register the same driver type twice (duplicate type registration), which is the symmetric guard that ManagerRegistryTest and PluginHookRegistryTest both cover via their "already registered" tests.
WHY: If DriverRegistry enforces uniqueness by DatabaseDriverType, the duplicate-type path is untested. If it does not enforce uniqueness, that policy is also untested.
WHERE: DriverRegistryTest / DriverRegistry (production)

---

CONCERN: ManagerRegistryTest calls RegistryResetExtension.setupRegistry() and RegistryResetExtension.resetRegistry() — a class referenced in src/test/java rather than src/testFixtures/java/. If RegistryResetExtension is test infrastructure intended to be shared with downstream repos (McRPG), it must live in src/testFixtures/java/.
WHY: The checklist explicitly requires shared fixtures to be in src/testFixtures/java/ so downstream repos can depend on them without duplicating setup. Both ManagerRegistryTest and PluginHookRegistryTest depend on this extension, making it shared infrastructure by definition.
WHERE: ManagerRegistryTest, PluginHookRegistryTest / com.diamonddagger590.mccore.testing.RegistryResetExtension (should be in src/testFixtures/java/)

---

CONCERN: ManagerRegistryTest and PluginHookRegistryTest both use @BeforeEach/@AfterEach to manually call RegistryResetExtension.setupRegistry() and RegistryResetExtension.resetRegistry() as static utility methods rather than as a JUnit 5 @ExtendWith extension. This is a structural misuse — if RegistryResetExtension implements BeforeEachCallback/AfterEachCallback, the @ExtendWith(RegistryResetExtension.class) annotation should be used instead, which is both more idiomatic and harder to accidentally omit in future test classes.
WHY: Manual invocation of extension lifecycle methods bypasses JUnit's lifecycle contract and is fragile. If a test method throws before tearDown() runs in certain configurations, state leaks.
WHERE: ManagerRegistryTest, PluginHookRegistryTest

---

CONCERN: TimeProviderTest never tests behavior when TimeProvider is constructed with the system clock (i.e., Clock.systemUTC() or the default constructor if one exists). If TimeProvider has a no-arg constructor that wraps Clock.systemUTC(), there is no test confirming that now() returns a non-null, reasonable value from wall-clock time — leaving the default-construction path entirely uncovered.
WHY: The test suite only exercises the Clock-injected constructor path. If TimeProvider is designed to be used framework-wide without injection (e.g., new TimeProvider() exists as a convenience), that construction path and its now() behavior are untested.
WHERE: TimeProviderTest / TimeProvider (production)

---

CONCERN: TimeProviderTest does not test the @AfterEach reset requirement from the checklist. The test class itself is clean (no static state mutation), but if TimeProvider has any static or singleton default instance (e.g., TimeProvider.system() or similar), tests that modify it must reset it. The current tests do not address this, and there is no @AfterEach guard present.
WHY: This only becomes a gap if TimeProvider exposes a static default. Without seeing the production class it cannot be confirmed absent, and the checklist requires the reset pattern to be present whenever state could be modified.
WHERE: TimeProviderTest / TimeProvider (production)

---

CONCERN: DatabaseDriverTypeTest.values_containsSqlite is a trivial enum membership test with no coverage value — it cannot fail unless the enum member is deleted, which would break compilation first. It occupies a test slot without covering any logic.
WHY: Not a structural problem per se, but it displaces a more meaningful edge-case test, such as: what does fromString return when passed null? The null input edge case is absent from all fromString tests, and if DatabaseDriverType.fromString calls .toLowerCase() on its argument without a null-check, null will throw NullPointerException rather than returning Optional.empty().
WHERE: DatabaseDriverTypeTest.values_containsSqlite / DatabaseDriverType.fromString (null input not tested)

---

CONCERN: DatabaseDriverTypeTest and DriverRegistryTest have no MockBukkit setup, which is correct since they test pure-Java logic. However, ManagerRegistryTest and PluginHookRegistryTest also have no MockBukkit setup, yet they construct Manager<CorePlugin> and PluginHook<CorePlugin> with null plugin references. If Manager or PluginHook constructors or any method called during register() dereferences the plugin reference, these tests will produce misleading NullPointerExceptions rather than meaningful assertion failures.
WHY: Passing null for the plugin is a pragmatic shortcut, but it is undocumented and fragile. If the production Manager base class adds a null-check or accesses plugin during construction, all these tests break silently in unexpected ways rather than failing on a clear assertion.
WHERE: ManagerRegistryTest (inner TestManagerA, TestManagerB, TestManagerASub) / PluginHookRegistryTest (inner TestHookA, TestHookB, TestHookC)

---

CONCERN: The ParserTest fix correctly separates the "sin" (no argument → ParseError) case from the "sin 5" (space-stripped → treated as variable → 0.0) case. However, the new test's @DisplayName says "treats as variable due to space stripping" — this is implementation-detail documentation embedded in a display name, which is appropriate for a regression test. What is missing is a regression test tag (@Tag("regression")) or comment confirming this documents a discovered bug. Without it, the behavior appears to be a deliberate feature rather than a known quirk that could be changed.
WHY: If "sin 5" returning 0.0 is a bug (space-stripping causing incorrect variable lookup), it warrants a regression marker. If it is intentional behavior, the test is correct as written. The distinction is not visible from the diff alone, but the comment // "sin 5" has its space stripped → "sin5" → not a function name, treated as variable → default 0.0 suggests this is a known edge-case behavior discovered during debugging, not a designed feature.
WHERE: ParserTest.getValue_treatsAsVariable_whenFunctionNameSpaceSeparatedFromArgument

---

Summary

Production files changed: .claude/commands/review-testing.md (docs), .cursor/rules/persona-testing.mdc (docs) — no production logic changed.

Test files present: DatabaseDriverTypeTest, DriverRegistryTest, ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest, ParserTest (modified), ChainComparatorTest (modified)

Coverage gaps:

• DatabaseDriverType.fromString(null) — null input untested, potential NPE
• DriverRegistry — duplicate driver type registration untested
• DriverRegistry — JDBC class-loading failure path not distinguished from tryDriver() == false
• TimeProvider — default/no-arg constructor path (if it exists) untested
• RegistryResetExtension — located in wrong source set if intended for downstream use

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test infrastructure and test case changes: new unit tests for DatabaseDriverType, DriverRegistry, ManagerRegistry, PluginHookRegistry, TimeProvider, and ChainComparator, plus updates to testing persona documents and a corrected ParserTest assertion. None of the changed files touch DAO query construction, DDL generation, MiniMessage deserialization, ChatResponse handling, Slot.onClick() return values, or permission-gated framework operations. No production code paths from the threat model are modified.

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff contains only test code, documentation rule updates, and a test behavior fix; no production source files are modified.

The diff touches:

• .claude/commands/review-testing.md — documentation only
• .cursor/rules/persona-testing.mdc — documentation only
• New test classes (DatabaseDriverTypeTest, DriverRegistryTest, ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest)
• Modified test classes (ParserTest, ChainComparatorTest)

None of these files are part of McCore's public API surface. No production classes, interfaces, abstract classes, method signatures, registry constants, or Javadoc contracts are changed. There is nothing for a downstream plugin to compile against differently, and no extension points are added or removed.

---

That said, two findings within the test code itself are worth flagging for correctness, since test infrastructure in src/testFixtures is part of McCore's downstream contract:

---

CONCERN: ParserTest splits one test into two — the new test asserts that "sin 5" evaluates to 0.0 because spaces are stripped, making "sin5" an unknown variable. This behavior is documented only in a comment, not in production Javadoc.
WHY: If a downstream consumer reads this test as specification (tests-as-docs), they will learn that Parser silently strips spaces and treats unknown identifiers as 0.0. If that behavior ever changes, downstream code relying on it will break with no deprecation warning. The production Parser class should carry Javadoc on its space-stripping behavior so the contract is on the API, not only in a test comment.
WHERE: src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java — getValue_treatsAsVariable_whenFunctionNameSpaceSeparatedFromArgument

---

CONCERN: ManagerRegistryTest.manager_returnsNull_whenNoManagerRegisteredForKey asserts that registry().manager(KEY_A) returns null for an unregistered key. If ManagerRegistry#manager is ever tightened to @NotNull or changed to return Optional, this test will be the only place that documents the nullable contract — and it will silently pass until a caller NPEs.
WHY: The @NotNull/@Nullable contract for ManagerRegistry#manager should be declared on the production method signature, not inferred from a test assertion. If a downstream plugin sees @NotNull on the method and trusts it, a null return is a contract violation. If the method is intentionally @Nullable, that annotation must be on the production method.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java — manager_returnsNull_whenNoManagerRegisteredForKey

---

No other extensibility concerns found. The ChainComparatorTest explicit type witness change (Comparator.<Integer>naturalOrder()) is a compile-correctness fix with no contract impact. The DriverRegistryTest.TestDriver inner class correctly implements the DatabaseDriver interface and exercises the public extension point without modifying McCore internals — this is exactly the pattern downstream plugins should follow.

[github-actions]

Testing Review

Audit Report

Production files changed: .claude/commands/review-testing.md, .cursor/rules/persona-testing.mdc (documentation only — no production logic changed)

Test files present:

• src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java (new)
• src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java (modified)
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java (new)
• src/test/java/com/diamonddagger590/mccore/util/comparator/ChainComparatorTest.java (modified)

Coverage gaps: See concerns below.

---

CONCERN: RegistryResetExtension is used in both ManagerRegistryTest and PluginHookRegistryTest but is not present in the diff's src/testFixtures/java/ tree — its location is unknown from this diff.
WHY: If RegistryResetExtension lives in src/test/java/ rather than src/testFixtures/java/, downstream repos (McRPG) that also need to reset registry state between tests cannot depend on it without duplicating the implementation. The checklist explicitly requires shared fixtures to be placed in src/testFixtures/java/.
WHERE: RegistryResetExtension (unknown location) / ManagerRegistryTest.java, PluginHookRegistryTest.java

---

CONCERN: ManagerRegistryTest and PluginHookRegistryTest construct Manager and PluginHook subclasses with super(null), suppressing the plugin reference. If Manager or PluginHook calls any method on the plugin reference during the tests exercised here, the tests will throw NullPointerException rather than a meaningful assertion failure, silently masking real behavior.
WHY: This is a structural fragility: the tests pass only because the code paths exercised today never dereference the plugin. If production code ever dereferences it in register() or registered(), the tests will fail with NPE rather than a test assertion, making diagnosis harder. A null-safe stub or a minimal CorePlugin mock should be used.
WHERE: ManagerRegistryTest.java (lines ~23–33), PluginHookRegistryTest.java (lines ~25–37)

---

CONCERN: ManagerRegistryTest does not test unregistration (if ManagerRegistry exposes an unregister method) or what manager(KEY_A) returns after a registry reset — the teardown calls RegistryResetExtension.resetRegistry() but there is no test asserting the post-reset state is clean.
WHY: If resetRegistry() is broken or incomplete, every subsequent test that calls setupRegistry() will silently inherit stale state. A single test asserting the registry is empty after reset (or that registered(KEY_A) returns false on a fresh registry) would catch this class of failure.
WHERE: ManagerRegistryTest.java / RegistryResetExtension (production: ManagerRegistry)

---

CONCERN: DriverRegistryTest has no test for null input to register(), registered(), isDriverRegistered(), or getDriver().
WHY: The checklist requires null-input edge cases for every non-trivial public method. If DriverRegistry does not guard against null, callers will get NullPointerException at runtime. If it does guard, that guard is untested.
WHERE: DriverRegistryTest.java / DriverRegistry.java

---

CONCERN: DriverRegistryTest.register_throwsRuntimeException_whenDriverClassMissing only asserts that some RuntimeException is thrown when tryDriver() returns false. It does not assert the exception message or type is meaningful to a developer diagnosing a missing JDBC driver.
WHY: This is a borderline structural issue: the test will pass for any unchecked exception, including an accidental NullPointerException, masking regressions in the error-reporting path. Asserting on the specific exception type (e.g., IllegalStateException) or a substring of the message would make the test a genuine regression guard rather than a near-vacuous check.
WHERE: DriverRegistryTest.java (~line 80) / DriverRegistry.java

---

CONCERN: DatabaseDriverTypeTest.values_containsSqlite asserts only that SQLITE is present in values(). This test adds no value beyond what the compiler already guarantees — referencing DatabaseDriverType.SQLITE at the assertion site already proves its existence.
WHY: A test with a trivially-true assertion cannot catch regressions and occupies maintenance budget for no coverage benefit. This is a structural problem (the test cannot fail under any realistic regression) rather than a style preference.
WHERE: DatabaseDriverTypeTest.java (~lines 59–63)

---

CONCERN: TimeProviderTest has no @AfterEach that resets any global/static TimeProvider state. If TimeProvider is stored in a static field anywhere in McCore (e.g., on CorePlugin or a singleton), tests that modify that field and then fail mid-test will leak a fixed clock into subsequent tests.
WHY: The checklist explicitly requires: "If a test modifies TimeProvider state, is that state reset in @AfterEach?" Even if the current TimeProvider tests only construct local instances, the pattern should be established now — particularly because ManagerRegistryTest and PluginHookRegistryTest already demonstrate the reset pattern via RegistryResetExtension.
WHERE: TimeProviderTest.java / TimeProvider.java

---

CONCERN: PluginHookRegistryTest.pluginHooks_returnsAllMatching_whenFilteredByInterface does not assert insertion order or set-equality in a deterministic way — assertEquals(2, hooks.size()) plus contains checks are sufficient for correctness, but if pluginHooks() returns a List with guaranteed order, the test does not document or verify that contract. If it returns an unordered collection, using List as the return type is misleading and that mismatch is not tested.
WHY: Coverage gap: the ordering/determinism contract of pluginHooks(Class) is untested. If the implementation changes from ordered to unordered (or vice versa), no test will catch the regression.
WHERE: PluginHookRegistryTest.java (~lines 103–113) / PluginHookRegistry.java

---

CONCERN: Neither ManagerRegistryTest nor PluginHookRegistryTest starts MockBukkit, yet both depend on RegistryAccess global state. These are plain-Java registry tests with no Bukkit API involvement. The checklist asks: does any test spin up MockBukkit but use neither MockBukkit server interaction nor any Bukkit APIs? The inverse also applies here — confirm RegistryAccess.registryAccess() itself does not silently invoke Bukkit internals. If it does, MockBukkit must be started; if it doesn't, the current plain-JUnit approach is correct but should be explicitly verified.
WHY: If RegistryAccess or RegistryResetExtension.setupRegistry() touches Bukkit.getServer() or any Bukkit static, tests will fail with IllegalStateException outside of MockBukkit context, but only on certain execution orders — a latent environmental fragility.
WHERE: ManagerRegistryTest.java, PluginHookRegistryTest.java / RegistryAccess.java, RegistryResetExtension.java

---

CONCERN: The ParserTest modification that renames the existing getValue_throwsParseError_whenFunctionMissingParenthesis test to use "sin" instead of "sin 5" and adds a new test for "sin 5" is correct, but the new test's comment — "treated as variable → default 0.0" — reveals a behaviour that may itself be a bug (silently returning 0.0 for an unrecognised token). There is no regression test asserting that this behaviour is intentional with a link to an issue or specification.
WHY: If the silent-zero behaviour is a known quirk being documented, a @DisplayName note is insufficient — there should be either a tracked issue reference or a test that explicitly asserts the behaviour is expected (e.g., via a comment citing the spec). As written, a future fix that makes "sin 5" throw ParseError will break this test with no indication of whether the breakage is a regression or an improvement.
WHERE: ParserTest.java (~lines 368–374) / Parser.java

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test infrastructure changes, documentation/persona-prompt updates, and a parser test behavioral correction. No production DAO methods, UpdateTableFunction implementations, MiniMessage deserialization call sites, ChatResponse handlers, Slot.onClick() overrides, or permission-gated operations are added or modified. All new test code exercises read-only assertions against pure-Java utilities (TimeProvider, DatabaseDriverType, DriverRegistry, ManagerRegistry, PluginHookRegistry, Parser) — none of which handle player-influenced data through McCore's security-sensitive abstractions.

[coderabbitai]

Actionable comments posted: 1

🤖 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/registry/plugin/PluginHookRegistryTest.java`:
- Line 22: Rename the test interface MarkerHookInterface to follow the Hook
suffix convention (e.g., MarkerHook) and update all references in
PluginHookRegistryTest accordingly; specifically, change the interface name
MarkerHookInterface to MarkerHook and adjust any usages, imports, or type
references within the class/methods in PluginHookRegistryTest so the test
compiles and follows the project's plugin hook naming rule.

🪄 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: 8b919128-a6bf-4e89-b4bb-17630d02b589

📥 Commits

Reviewing files that changed from the base of the PR and between aa64aae and 89d252d.

📒 Files selected for processing (9)

• .claude/commands/review-testing.md
• .cursor/rules/persona-testing.mdc
• src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java
• src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java
• src/test/java/com/diamonddagger590/mccore/util/comparator/ChainComparatorTest.java

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename test interface to follow Hook suffix convention.

MarkerHookInterface should use the Hook suffix (e.g., MarkerHook) to stay consistent with McCore naming rules for plugin hook interfaces.

As per coding guidelines: "Use Hook suffix for plugin hook interfaces (e.g., AfkPluginHook, CoreCitizensHook)."

🤖 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/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java`
at line 22, Rename the test interface MarkerHookInterface to follow the Hook
suffix convention (e.g., MarkerHook) and update all references in
PluginHookRegistryTest accordingly; specifically, change the interface name
MarkerHookInterface to MarkerHook and adjust any usages, imports, or type
references within the class/methods in PluginHookRegistryTest so the test
compiles and follows the project's plugin hook naming rule.

[DiamondDagger590]

Addressing remaining testing review feedback

Fixed in 89d252d:

• DriverRegistry duplicate type registration — Added test verifying that registering a second driver for the same DatabaseDriverType silently overwrites the first (unlike ManagerRegistry which throws). This documents an asymmetry worth knowing about.

Fixed in 04a9247:

• TimeProviderTest missing reset pattern — Added RegistryResetExtension.setupRegistry() / resetRegistry() in @BeforeEach / @AfterEach. While the current tests only construct local TimeProvider instances, CorePlugin holds a static singleton (CorePlugin.getInstance()) whose getTimeProvider() is the production access path. Adding the reset pattern now guards against future tests in this class that may interact with the singleton, and keeps the test class structurally consistent with ManagerRegistryTest and PluginHookRegistryTest.

Remaining concerns — not changed (by design):

• Null input tests (register(null), fromString(null), pluginHooks(null)) — Per CLAUDE.md: "Only validate at system boundaries (user input, external APIs)." These are internal framework methods annotated with @NotNull. Testing null on them tests undefined behavior, not a coverage gap.
• DriverRegistry tryDriver() vs Class.forName() conflation — The test correctly overrides tryDriver() to control its return value. We're unit-testing DriverRegistry.register(), not the default tryDriver() implementation. Testing Class.forName() with a missing classpath entry would be an integration test concern.
• @ExtendWith vs manual static calls — RegistryResetExtension is a plain utility class with static methods, not a JUnit Extension. @ExtendWith is not applicable. @AfterEach runs even when tests throw, so there is no leak risk.

---

Generated by Claude Code

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff contains only test files, test-support tooling, and documentation updates with no changes to production source.

No extensibility concerns found.

Rationale: Every changed file falls into one of three categories that are out of scope for the extensibility checklist:

1. AI assistant prompt/rule files (.claude/commands/review-testing.md, .cursor/rules/persona-testing.mdc) — these are internal developer tooling, not part of McCore's public API or binary contract.
2. Test-only source files (src/test/java/...) — DatabaseDriverTypeTest, DriverRegistryTest, ParserTest, ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest, ChainComparatorTest — none of these are shipped as part of the framework artifact consumed by downstream plugins.
3. Test fixture infrastructure (RegistryResetExtension) — referenced from tests but not a production extension point.

No public interfaces were added or changed, no abstract methods were introduced, no method signatures were altered, no constants were renamed, no @Deprecated symbols were touched, and no GUI, database migration, or registry key contracts were modified. There is nothing for a downstream plugin author to break against.

[github-actions]

Testing Review

Testing Audit Report

Production files changed:

• .claude/commands/review-testing.md (documentation only)
• .cursor/rules/persona-testing.mdc (documentation only)

Test files present:

• src/test/java/com/diamonddagger590/mccore/database/driver/DatabaseDriverTypeTest.java (new)
• src/test/java/com/diamonddagger590/mccore/database/driver/DriverRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/parser/ParserTest.java (modified)
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/registry/plugin/PluginHookRegistryTest.java (new)
• src/test/java/com/diamonddagger590/mccore/util/TimeProviderTest.java (new)
• src/test/java/com/diamonddagger590/mccore/util/comparator/ChainComparatorTest.java (modified)

Coverage gaps:

DatabaseDriverType.fromString(null), DriverRegistry duplicate-register overwrite assertion completeness, TimeProvider without registry setup dependency, ManagerRegistry.manager() null-key guard, PluginHookRegistry interface-query ordering stability.

---

Concerns

CONCERN: DatabaseDriverType.fromString(null) is not tested.
WHY: The fromString method receives external input (e.g., a config string). A null argument is a realistic caller error. Without a test, it is unknown whether the implementation throws NullPointerException, returns Optional.empty(), or handles it gracefully. This is a required edge-case check under the "null inputs" coverage rule.
WHERE: DatabaseDriverTypeTest.java / DatabaseDriverType

---

CONCERN: DriverRegistryTest.register_overwritesExistingDriver_whenSameDriverTypeRegisteredTwice registers two drivers of the same type but does not assert that registered(first) returns false after the second registration — only that getDriver returns the second.
WHY: If the registry retains both entries internally (e.g., in a list rather than a map keyed by type), getDriver could still return second by iteration order while registered(first) would also still return true. The test does not verify the replace-not-accumulate semantic. This is a structural gap: the assertion tests the outcome but not the invariant it implies.
WHERE: DriverRegistryTest.java / DriverRegistry

---

CONCERN: DriverRegistryTest does not test that register throws (or does not throw) when the same driver instance is registered twice — only that a second different instance of the same type overwrites the first.
WHY: ManagerRegistryTest shows register throws IllegalArgumentException when the same class is registered again. DriverRegistry appears to silently overwrite. These are different contracts. If DriverRegistry.register is also supposed to throw on duplicate instance registration, that path is untested. If it is supposed to silently overwrite, a test confirming no exception is thrown for the same instance would make the contract explicit.
WHERE: DriverRegistryTest.java / DriverRegistry

---

CONCERN: TimeProviderTest calls RegistryResetExtension.setupRegistry() / resetRegistry() in @BeforeEach / @AfterEach, but TimeProvider takes a Clock as a constructor argument and has no apparent dependency on the registry.
WHY: If TimeProvider genuinely does not interact with the registry, this setup introduces a hidden coupling between the test and registry state. Should the registry setup fail (e.g., missing fixture class on the classpath in a downstream project), these otherwise pure-Java tests will break for an unrelated reason. If the registry dependency is real (e.g., TimeProvider registers itself), the test should assert that post-setup state. Either way the current code needs clarification: remove the registry setup if unused, or add an assertion that demonstrates why it is required.
WHERE: TimeProviderTest.java / TimeProvider

---

CONCERN: TimeProviderTest does not test the behavior when TimeProvider is constructed with the system clock (i.e., new TimeProvider(Clock.systemUTC())).
WHY: The system-clock constructor path is almost certainly the production default. There is no test confirming that now() returns a non-null, post-epoch instant when the real clock is used. This matters because a misconfigured default (e.g., null clock field) would only fail at runtime.
WHERE: TimeProviderTest.java / TimeProvider

---

CONCERN: ManagerRegistryTest.manager_returnsNull_whenNoManagerRegisteredForKey asserts null is returned for an unregistered key, but there is no test for what happens when a null key is passed to registry().manager(null) or registry().registered(null).
WHY: ManagerKey is annotated with @NotNull in framework code patterns, but callers can still pass null at runtime. The contract (throw vs. return null/false) must be covered. This falls under the "null inputs" edge-case requirement.
WHERE: ManagerRegistryTest.java / ManagerRegistry

---

CONCERN: PluginHookRegistryTest.pluginHooks_returnsAllMatching_whenFilteredByInterface asserts size() == 2 and membership but does not assert against a fixed order or document that ordering is undefined.
WHY: If pluginHooks returns a List, callers may depend on insertion order. If the underlying collection is a HashMap or similar, order is non-deterministic. The test passes today but could become a flaky false-negative if the implementation changes. Either assert exact ordering (if the contract guarantees it) or switch to containsExactlyInAnyOrder semantics and document the unordered contract. As written, assertTrue(hooks.contains(hookB)) masks this.
WHERE: PluginHookRegistryTest.java / PluginHookRegistry

---

CONCERN: PluginHookRegistryTest does not test pluginHook(null) or registered(null).
WHY: Same null-input coverage requirement as flagged for ManagerRegistryTest. The key types are presumably @NotNull-annotated, but the contract under null must be verified.
WHERE: PluginHookRegistryTest.java / PluginHookRegistry

---

CONCERN: The ParserTest change splits one test into two, but the new test getValue_treatsAsVariable_whenFunctionNameSpaceSeparatedFromArgument asserts 0.0 against a comment-documented assumption ("spaces stripped → treated as variable → default 0.0") rather than against verified production behaviour.
WHY: If the Parser implementation changes how it resolves unknown identifiers (e.g., throws instead of returning 0.0, or returns Double.NaN), this test becomes a change-detector for an undocumented default rather than a test of a specified contract. The comment acknowledges this is inferential. The test should either be backed by a documented contract in Parser (a @throws or Javadoc stating unknown variables resolve to 0.0), or be marked @Disabled with a TODO, or the assertion value should be derived from a named constant (Parser.DEFAULT_VARIABLE_VALUE) rather than a magic literal.
WHERE: ParserTest.java / Parser

---

CONCERN: DatabaseDriverTypeTest.values_containsSqlite is a tautological test — it asserts that an enum contains its own value.
WHY: This test can never fail unless the enum member is deleted, at which point the test would fail to compile rather than fail at runtime. It provides zero behavioral coverage and adds noise. Tests that cannot meaningfully fail should be removed rather than left to give false confidence.
WHERE: DatabaseDriverTypeTest.java / DatabaseDriverType

---

CONCERN: No test covers the DatabaseDriver interface methods on a real (non-TestDriver) implementation — specifically getConnectionUrl and populateDataSourceCredentials for the SQLite driver.
WHY: The TestDriver stub hardcodes "jdbc:sqlite:test.db" and a no-op populateDataSourceCredentials. If there is a concrete SQLiteDriver production class, its URL construction logic (which likely includes the file path from Credentials) is completely untested. This is a coverage gap for a non-trivial method.
WHERE: DriverRegistryTest.java / concrete SQLiteDriver production class (if present)

---

Production files changed: .claude/commands/review-testing.md, .cursor/rules/persona-testing.mdc (documentation only — no production logic changed)

Test files present: DatabaseDriverTypeTest, DriverRegistryTest, ParserTest (modified), ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest, ChainComparatorTest (modified)

Coverage gaps: fromString(null), system-clock TimeProvider path, null-key guards on ManagerRegistry and PluginHookRegistry, overwrite-not-accumulate invariant in DriverRegistry, concrete SQLiteDriver URL construction, Parser unknown-variable contract documentation.

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test infrastructure and documentation changes: new JUnit 5 test classes (DatabaseDriverTypeTest, DriverRegistryTest, ManagerRegistryTest, PluginHookRegistryTest, TimeProviderTest), modifications to existing test cases (ParserTest, ChainComparatorTest), and updates to AI persona prompt documents (.claude/commands/review-testing.md, .cursor/rules/persona-testing.mdc).

None of the changed files touch DAO query construction, UpdateTableFunction DDL, MiniMessage deserialization call sites, ChatResponse lifecycle code, Slot.onClick() return values, or permission-gated framework operations. No checklist item has a surface to apply to this diff.