[Tests] Improve coverage for Database non-blocking path and LocalizationManager broadcast/PAPI

[DiamondDagger590]

Summary

• DatabaseAdditionalTest: Added 3 tests covering the non-blocking (blockMainThreadOnStart=false) async createTables/updateTables paths, including single and multiple custom create/update functions invoked asynchronously
• LocalizationManagerBroadcastTest (new file): Added 11 tests covering:
  • broadcastMessage(Route) with loaded players, unloaded players, mixed, and console-only scenarios
  • PAPI integration in getLocalizedMessage(player, route) — hook registered with player present, hook registered without Bukkit player (skipped), and no hook registered
  • PAPI integration in getLocalizedMessages(player, route) — hook translates each line, hook skipped without Bukkit player
  • Default postProcessResolvedString identity behavior across both getLocalizedMessage(Route) and getLocalizedMessage(player, route) overloads

Coverage Impact

Class	Before (line)	After (line)	Branch
Database	75.4%	93.9%	100%
LocalizationManager	90.3%	100%	95.5%

Test plan

• All new tests pass locally (gradle test)
• Full test suite passes with no regressions
• Testing audit persona reviewed changes — all findings addressed
• JaCoCo coverage report confirms improvements

---

Generated by Claude Code

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c4f8db5f-4f88-41d3-b595-6efc834cd5b7

📥 Commits

Reviewing files that changed from the base of the PR and between ca6b94b and 7ccbfc3.

📒 Files selected for processing (2)

• src/test/java/com/diamonddagger590/mccore/database/DatabaseAdditionalTest.java
• src/test/java/com/diamonddagger590/mccore/localization/LocalizationManagerBroadcastTest.java

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/eager-thompson-8nn5n5

---

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 adds only test code with no changes to production source files.

The entire diff consists of two test classes (DatabaseAdditionalTest additions and LocalizationManagerBroadcastTest), both in src/test/java. No production API surface is modified, added, or removed.

---

Reviewing against each checklist item:

• Extension Opportunity: No new production extension points introduced; nothing to evaluate.
• Framework Contract Stability: No public classes, methods, fields, or constants are changed in production code.
• GUI Framework: No changes to BaseGui, PaginatedGui, Slot, or GuiManager.
• Database Framework: No schema changes, no UpdateTableFunction modifications, no raw DDL added to production paths.
• @NotNull / @nullable Contracts: No new public method signatures in production code.
• Registry and Extension Points: No new RegistryKey / ManagerKey constants; no string values changed.
• McCore-Specific Rule: Test code references TestCorePlayer and BroadcastTestLocalizationManager as inner test classes — these are confined to test scope and do not ship in the framework artifact.

One minor test-quality observation (not a framework contract concern): awaitDatabaseExecutor uses a hard Thread.sleep(500) to allow async thenAccept callbacks to propagate, which is a timing-dependent synchronization pattern that could produce flaky results in slow CI environments. This has no bearing on downstream plugin compatibility but is worth noting for test reliability.

---

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 file changes — no production source files are modified. Both files are under src/test/java/.

---

Findings

---

CONCERN: awaitDatabaseExecutor uses a hard Thread.sleep(500) to allow async thenAccept callbacks to propagate — this is a race condition, not a reliable synchronization mechanism.
WHY: On a slow CI machine, 500ms may not be enough for all thenAccept continuations to complete, causing intermittent false-negative test failures. On a fast machine, it wastes time unnecessarily. The correct approach is to collect all CompletableFuture handles returned by initializeDatabase and join them, or use a CountDownLatch/additional sentinel submitted after the continuations are known to have been enqueued. Since the futures returned by the lambdas complete inside the executor but the thenAccept chain may run on a different thread context, the sleep is an unreliable substitute for proper future composition.
WHERE: DatabaseAdditionalTest.awaitDatabaseExecutor (lines added in the diff)

---

CONCERN: The sentinel-based drain in awaitDatabaseExecutor only drains the executor queue once, but the thenAccept callbacks in initializeDatabase may be scheduled after that sentinel completes — meaning the sentinel proves nothing about the callbacks having run.
WHY: initializeDatabase submits task A → task A's thenAccept schedules task B. The sentinel is submitted before task B is enqueued, so the sentinel can complete before B executes. The Thread.sleep(500) is the only guard against this, making the test non-deterministic. This is a structural correctness problem, not a style preference: a test that can pass with the callbacks not yet having run cannot reliably detect regressions.
WHERE: DatabaseAdditionalTest.awaitDatabaseExecutor / initializeDatabase_createsTablesAsync_withNonBlockingStartup, initializeDatabase_invokesCustomFunctionsAsync_withNonBlockingStartup, initializeDatabase_invokesAllCustomFunctionsAsync_withNonBlockingStartup

---

CONCERN: initializeDatabase_createsTablesAsync_withNonBlockingStartup asserts that table_history exists but does not assert that only the expected tables are created, nor does it test behavior on an already-initialized schema (idempotency / already-migrated path).
WHY: The checklist explicitly requires: "For any database migration change (UpdateTableFunction), is there a test verifying it runs on both a fresh schema and an already-migrated schema?" Only the fresh-schema path is covered. There is no test asserting that running initializeDatabase a second time (already-migrated schema) does not throw, does not duplicate data, and produces the same final state.
WHERE: DatabaseAdditionalTest — missing already-migrated scenario for the non-blocking path

---

CONCERN: LocalizationManagerBroadcastTest uses MockBukkit.mock() / MockBukkit.load() but also mocks Localization, YamlDocument, and CorePapiHook with Mockito. The Localization and YamlDocument objects are not Bukkit types, so Mockito is fine there. However, CorePapiHook is mocked with Mockito and then injected via reflection into PluginHookRegistry. This bypasses the registry's public API entirely, which means if the registry's internal field name changes, the test silently stops injecting (the field lookup will throw NoSuchFieldException, but only at runtime, and the failure message will be cryptic). More critically, this is a structural problem: the registry should expose a test-seam (a setter, a factory, or a constructor parameter) rather than requiring reflective field manipulation in tests.
WHY: Reflective injection of mocks is fragile — it is not a structural test concern about coverage but about the test being able to silently pass without actually exercising the PAPI hook lookup if the field name changes or the mock is not injected correctly. If NoSuchFieldException is swallowed or the cast fails at a different line, the test may assert on a wrong value.
WHERE: LocalizationManagerBroadcastTest.registerPapiHook / PluginHookRegistry

---

CONCERN: broadcastMessage_sendsToConsole_whenNoPlayersOnline contains only an assertDoesNotThrow — it does not assert that the console actually received a message.
WHY: The checklist states: "Does every test method have at least one assertion?" — assertDoesNotThrow is technically an assertion, but it only asserts the absence of an exception, not that the console received the broadcast. The display name promises "then console still receives the message," but there is no assertion verifying that claim. This is a coverage gap: the stated behaviour (console receives message) is untested.
WHERE: LocalizationManagerBroadcastTest.broadcastMessage_sendsToConsole_whenNoPlayersOnline

---

CONCERN: The three broadcastMessage tests that assert player message receipt (assertSaid) do not assert anything about the console receiving a copy of the broadcast, even though broadcastMessage presumably sends to both players and console.
WHY: If broadcastMessage is supposed to always log to console (a common McCore pattern), that behavior is entirely untested. If ServerMock provides a console output capture mechanism, this is a coverage gap.
WHERE: LocalizationManagerBroadcastTest — all broadcastMessage_* test methods

---

CONCERN: There is no test covering broadcastMessage when englishDoc.contains(testRoute) returns false (i.e., the message key does not exist in any registered locale).
WHY: This is a missing edge-case: what happens when the localization key is absent? Does it throw? Return an empty string? Return a fallback? The checklist requires edge cases to be covered. Currently, every test pre-configures englishDoc.contains(testRoute) to return true.
WHERE: LocalizationManagerBroadcastTest — no test for missing/absent localization key

---

CONCERN: There is no test covering broadcastMessage or getLocalizedMessage when no language file is registered at all (i.e., registerEnglishDoc() is never called).
WHY: Edge case: null/empty collection state. The checklist requires coverage of empty collections and null inputs. The code path where no Localization is registered and a message is requested is entirely untested.
WHERE: LocalizationManagerBroadcastTest — no test for unregistered locale state

---

CONCERN: getLocalizedMessages_player_skipsPapi_whenNoBukkitPlayer and getLocalizedMessage_player_skipsPapi_whenNoBukkitPlayer register a CorePapiHook mock but never configure when(mockPapiHook.translateMessage(...)). If the production code calls translateMessage on a null-player path (i.e., the guard fails), Mockito will return null by default, and the test would incorrectly pass (asserting the raw string when null was returned would fail, but if the production code uses a null-safe fallback, this could mask a bug). The intent is to verify PAPI is skipped, but the test does not verify translateMessage was never called (no verify(mockPapiHook, never()).translateMessage(...)).
WHY: Without a verify(never()), the test does not confirm that PAPI was skipped — it only confirms the return value matches the raw string, which could be coincidental if the PAPI mock returns null and the code falls back. This is a structural assertion gap.
WHERE: LocalizationManagerBroadcastTest.getLocalizedMessage_player_skipsPapi_whenNoBukkitPlayer, getLocalizedMessages_player_skipsPapi_whenNoBukkitPlayer

---

CONCERN: RegistryResetExtension.setupRegistry() and RegistryResetExtension.resetRegistry() are called manually in @BeforeEach/@AfterEach. If resetRegistry() is not called due to a test failure that throws before @AfterEach (which JUnit 5 does call even on failure), this is fine — but the MockBukkit.unmock() and RegistryResetExtension.resetRegistry() are in the same @AfterEach, meaning if resetRegistry() throws, MockBukkit.unmock() is never called, leaking the mock server into subsequent tests.
WHY: MockBukkit must be torn down correctly and must not leak across tests. The checklist item: "Is MockBukkit set up and torn down correctly — not leaked across tests?" The fix is to place MockBukkit.unmock() first (or use a try/finally block in @AfterEach).
WHERE: LocalizationManagerBroadcastTest.tearDown

---

Summary

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

Test files present:

• src/test/java/com/diamonddagger590/mccore/database/DatabaseAdditionalTest.java (modified)
• src/test/java/com/diamonddagger590/mccore/localization/LocalizationManagerBroadcastTest.java (new)

Coverage gaps:

1. Already-migrated schema path not tested for non-blocking initializeDatabase
2. broadcastMessage with missing localization key (absent route) not tested
3. broadcastMessage with no registered language file not tested
4. Console message receipt not asserted in any broadcastMessage test
5. PAPI translateMessage is never verified as never() called in the skip-PAPI tests
6. Async synchronization in awaitDatabaseExecutor is race-prone (sentinel submitted before thenAccept callbacks are enqueued; Thread.sleep(500) is the only guard)
7. MockBukkit.unmock() can be skipped if resetRegistry() throws in tearDown

[github-actions]

Security Review

No security concerns found.

The diff contains only test code (DatabaseAdditionalTest.java additions and the new LocalizationManagerBroadcastTest.java). Reviewing against each checklist item:

• SQL Injection in DAO Methods: No DAO methods are introduced or modified. No query construction of any kind appears in either file.
• DDL Injection in UpdateTableFunction: No UpdateTableFunction implementations are present. The lambda callbacks in the async tests submit work to the executor but contain no DDL.
• MiniMessage Injection: No call to deserialize() or any MiniMessage API appears anywhere in the diff. The <primary>Special text string in postProcessResolvedString_defaultIdentity_returnsUnmodifiedMessage_playerOverload is passed only to assertEquals() as an expected value — it is never deserialized.
• ChatResponse / ChatResponseManager Safety: No ChatResponse implementations or chat content handling is present.
• onClick() Return Value Safety: No Slot.onClick() implementations are present.
• Framework Permission Gating: No framework operations affecting player state or data are introduced; all code is test scaffolding with mock objects and assertion calls.