[Tests] Add remaining branch coverage gaps for ManagerRegistry and Methods.lookAt

[DiamondDagger590]

Summary

Addresses remaining branch coverage gaps identified during PR #59 persona audit, after rebasing onto latest develop (which merged the original RegistryAccess, StatisticModifyEvent, and prior ManagerRegistry/Methods tests).

• ManagerRegistry: Add tests for registered(Manager) and registered(ManagerKey) with non-matching entries in the map — exercises the for-loop body returning false (rather than the empty-map short path already covered)
• Methods.lookAt: Add test for the dx == 0, dz > 0 branch where yaw stays at 0 — asserts both yaw and pitch for completeness (addresses review feedback about incomplete assertions)

Coverage Impact

Class	Branch Before	Branch After
ManagerRegistry	85.7% (12/14)	100% (14/14)
Methods.lookAt	already covered	explicit test added

Test plan

• All new tests pass (./gradlew test)
• Full test suite passes with no regressions
• JaCoCo confirms ManagerRegistry at 100% branch coverage

🤖 Generated with Claude Code

https://claude.ai/code/session_014PUN8sfRgFi1k1nPoyjGqm

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 39 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: 5fcc17b0-b39c-4be9-99e7-89a62a3016b1

📥 Commits

Reviewing files that changed from the base of the PR and between b994d23 and f45efa4.

📒 Files selected for processing (2)

• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java

Walkthrough

This PR adds unit tests only: new test cases for StatisticModifyEvent constructor/setter validation and cancellable behavior, RegistryAccessTest covering registration and singleton behavior, additional ManagerRegistry registered() lookup tests, and a new Methods.lookAt yaw assertion test.

Changes

Test coverage additions

Layer / File(s)	Summary
StatisticModifyEvent test suite src/test/.../event/statistic/StatisticModifyEventTest.java	New test class validating constructor type checks per statistic kind, setNewValue type validation, cancellable default/setter, and getter correctness.
RegistryAccess registration and singleton tests src/test/.../registry/RegistryAccessTest.java	New test class with reset lifecycle, verifying duplicate registration errors, registered() state, registry() retrieval, and registryAccess() singleton identity.
ManagerRegistry registered() lookup tests src/test/.../registry/manager/ManagerRegistryTest.java	Four new test cases validating registered() returns false/true based on instance or key matches among registered managers.
Methods.lookAt yaw test src/test/.../util/MethodsTest.java	New test case asserting yaw is approximately zero for a target with positive Z and same X coordinate.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

• DiamondDagger590/McCore#24: Both PRs add/extend unit tests for ManagerRegistryTest, covering registered(...)/manager lookup semantics.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check	✅ Passed	The title is concise and accurately describes the main test-driven branch coverage work, even though it omits two additional test files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

---

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 new test classes and additional test methods with no changes to production source files.

No extensibility concerns found.

The entire diff consists of:

• A new test class StatisticModifyEventTest covering constructor validation, setter validation, cancellable contract, and getter correctness
• A new test class RegistryAccessTest covering singleton behavior, registration guards, and key-based retrieval
• Additional test methods appended to the existing ManagerRegistryTest covering edge cases in registered() with multiple entries
• One additional test method in MethodsTest covering a lookAt yaw edge case

None of these changes touch any public API surface, abstract class hierarchy, method signatures, field declarations, registry key string values, @Deprecated symbols, GUI framework types, or database migration infrastructure. No production contracts are altered and no downstream plugin depending on McCore will experience any compile-time or runtime behavioral change.

[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. I must therefore evaluate whether the tests themselves are structurally sound and whether the production classes they exercise have adequate coverage.

---

Findings

---

CONCERN: StatisticModifyEventTest uses Instant.now() directly in two TIMESTAMP test methods instead of a fixed/injected value.
WHY: The TimeProvider rule requires all time-based logic in tests to use an injected fixed clock, not wall-clock time. If StatisticModifyEvent or SimpleStatistic ever performs time arithmetic internally, this test will be sensitive to wall-clock time, making assertions non-deterministic. Even if current production code does not do arithmetic, the pattern sets a precedent for fragile tests and violates the stated policy. There is also no @AfterEach resetting TimeProvider state, which would be required if TimeProvider were set.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java — methods constructor_succeeds_whenTimestampStatisticGetsInstantValue and constructor_throws_whenTimestampStatisticGetsLongValue

---

CONCERN: StatisticModifyEventTest constructs TestCorePlayer by passing null as the second argument to CorePlayer(UUID, ...) but never tests null as the player argument to StatisticModifyEvent itself.
WHY: If StatisticModifyEvent's constructor has a null-guard on the CorePlayer parameter (or if downstream consumers assume non-null), the missing null-player edge case is a coverage gap. The checklist explicitly requires null-input edge cases for every non-trivial public constructor/method.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java — no test covers new StatisticModifyEvent(null, ...)

---

CONCERN: StatisticModifyEventTest does not test a null new-value argument to the constructor or to setNewValue.
WHY: The type-validation branch in StatisticModifyEvent (inferred from the tests exercising IllegalArgumentException) must decide what to do when newValue is null. Whether it throws or silently accepts null is unspecified by any test, leaving that branch untested. This is a null-input edge case required by the checklist.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java

---

CONCERN: StatisticModifyEventTest does not test a null old-value argument to the constructor.
WHY: Same rationale as above — the constructor accepts an oldValue parameter whose null behaviour is completely untested.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java

---

CONCERN: StatisticModifyEventTest does not test a null ModificationType argument.
WHY: ModificationType is passed directly into the constructor. If the constructor does not null-check it, callers could silently store a null ModificationType. No test probes this boundary.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java

---

CONCERN: StatisticModifyEventTest does not test ModificationType.DECREMENT (or any modification type beyond SET and INCREMENT).
WHY: If StatisticModifyEvent behaves differently based on ModificationType (e.g. validation logic, sign checks), only two of the enum values are exercised. All enum branches are a required edge case under the checklist's "max/limit values" and general coverage rules.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java

---

CONCERN: RegistryAccessTest calls RegistryResetExtension.setupRegistry() / resetRegistry() manually in @BeforeEach / @AfterEach rather than using the extension via @ExtendWith(RegistryResetExtension.class).
WHY: This is a structural problem: if a test method throws an unexpected exception, JUnit 5 still guarantees @AfterEach runs, so teardown is safe in this case. However, the extension exists specifically to manage this lifecycle declaratively and consistently. Calling extension methods manually bypasses the extension contract (e.g. any beforeEach/afterEach ordering guarantees relative to other extensions), and duplicates teardown logic that could diverge from the extension's own implementation. If RegistryResetExtension later adds additional setup/teardown steps, this test silently misses them.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java

---

CONCERN: RegistryAccessTest.registryAccess_returnsSameInstance_whenCalledMultipleTimes uses reference equality (first == second) asserted via assertTrue instead of assertSame.
WHY: This is a structural problem, not a style preference. assertTrue(first == second) produces no diagnostic message on failure — it just says "expected true but was false." assertSame(first, second) produces "expected <instance1> but was <instance2>," making failures actionable. Using the wrong assertion method is a test quality defect.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java — registryAccess_returnsSameInstance_whenCalledMultipleTimes

---

CONCERN: RegistryAccessTest does not test registry(RegistryKey.PLUGIN_HOOK), registry(RegistryKey.PLAYER_SETTING), or registry(RegistryKey.STATISTIC) — only RegistryKey.MANAGER is retrieved.
WHY: setupRegistry() presumably registers all four standard registry types. The retrieval path for the other three keys is completely untested, leaving a coverage gap for the registry(RegistryKey) method across its full domain.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java

---

CONCERN: RegistryAccessTest does not test registry(RegistryKey) with an unregistered key (i.e., a key that has no corresponding entry), nor does it test what registered(Registry) returns for a null argument.
WHY: These are null/missing-key edge cases required by the checklist. The behaviour on an unknown key (returns null? throws?) is unspecified by any test.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java

---

CONCERN: RegistryAccessTest does not cover the register method's happy path independently — it relies entirely on setupRegistry() side-effects in @BeforeEach.
WHY: The test for register_throwsIllegalArgument_whenRegistryAlreadyRegistered implicitly proves registration happened, but there is no explicit test asserting that register succeeds and makes the registry retrievable when called for the first time. The positive path of register is only incidentally covered.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java

---

CONCERN: MethodsTest.lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX does not test the pitch component of the returned Location.
WHY: Methods.lookAt computes both yaw and pitch. The existing test only asserts getYaw(). If the pitch calculation is incorrect for this particular geometry (target on positive Z, same X, same Y as origin), the error is invisible. The test has an assertion, so it won't silently pass, but the assertion is incomplete for the method under test.
WHERE: src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java — lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX

---

CONCERN: MethodsTest.lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX uses org.bukkit.Location with a null world argument and does not set up MockBukkit.
WHY: If Location construction or any method called on it internally requires a non-null World or Bukkit server instance (e.g. for coordinate validation), this test will fail at runtime with a NullPointerException from inside Bukkit internals rather than a meaningful assertion failure — but only in some Bukkit versions. The existing tests in the same class that use null worlds should be examined for consistency. If the broader test class never sets up MockBukkit, this is fine; but if other tests in the file do use MockBukkit, this test is inconsistently structured.
WHERE: src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java — lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX

---

CONCERN: StatisticModifyEventTest has no test for the SET_STRING type with a null element inside the set (i.e., Set.of(null) is not tested, nor is a set containing a mix of valid strings and null).
WHY: Depending on how the SET_STRING type-check is implemented (e.g. instanceof Set check only, vs. element-type inspection), a Set<Integer> passed as the value might pass the instance check. There is no test verifying that a Set containing non-String elements is rejected.
WHERE: src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java

---

Summary

Production files changed: None — this diff is test-only.

Test files present:

• src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java (new)
• src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java (new)
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java (extended)
• src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java (extended)

Coverage gaps:

1. Null player, null newValue, null oldValue, null ModificationType in StatisticModifyEvent constructor
2. Instant.now() used directly instead of a fixed clock in TIMESTAMP tests
3. Only SET and INCREMENT modification types tested; DECREMENT and any others untested
4. SET_STRING does not test a Set of non-String elements
5. RegistryAccess.registry(RegistryKey) tested for only one of four registered keys
6. RegistryAccess.registry(RegistryKey) not tested with an unknown/unregistered key
7. RegistryAccess.register positive path not explicitly tested in isolation
8. Methods.lookAt pitch not asserted in new test case
9. RegistryAccessTest bypasses RegistryResetExtension lifecycle contract by calling static methods manually
10. assertSame should replace assertTrue(first == second) in singleton test

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code additions and modifications — unit tests for StatisticModifyEvent, RegistryAccess, ManagerRegistry, and Methods.lookAt(). No DAO methods, DDL-generating functions, MiniMessage deserialization calls, ChatResponse implementations, Slot.onClick() overrides, or permission-gated operations appear in this changeset. There is no production framework code to audit against the checklist.

[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/RegistryAccessTest.java`:
- Around line 56-62: The identity check in
RegistryAccessTest.registryAccess_returnsSameInstance_whenCalledMultipleTimes
should use the purpose-built assertion instead of a raw equality comparison.
Replace the current first == second assertion with assertSame so the test
clearly expresses singleton identity and produces a better failure message; keep
the test focused on RegistryAccess.registryAccess() returning the same instance.

🪄 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: 3df78f53-6494-4a7d-b8a6-7622cb9245e7

📥 Commits

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

📒 Files selected for processing (4)

• src/test/java/com/diamonddagger590/mccore/event/statistic/StatisticModifyEventTest.java
• src/test/java/com/diamonddagger590/mccore/registry/RegistryAccessTest.java
• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java

[github-actions]

Extensibility Review

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

No extensibility concerns found.

Both changes are confined to the src/test/ source tree and introduce additional unit test methods to existing test classes (ManagerRegistryTest and MethodsTest). No public API, abstract class, interface, registry constant, method signature, nullability contract, GUI framework class, or database migration path is touched. There is nothing here that affects downstream plugin compilation or runtime behavior.

[github-actions]

Testing Review

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

---

Analysis

Production files changed

The diff contains zero production file changes. Both changed files are in src/test/java/. There are no new public methods, database migrations, GUI changes, or bug fixes introduced on the production side.

Test files present

• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java

---

Checklist Application

Coverage Completeness

No new production logic was introduced, so "for every new public method" coverage requirements do not apply. However, the tests themselves can still be evaluated for structural correctness.

TimeProvider Usage

Neither new test touches time-based logic. No System.currentTimeMillis() or Instant.now() calls appear. ✅

MockBukkit Usage

The lookAt test in MethodsTest constructs org.bukkit.Location objects with a null world argument. This needs scrutiny.

CONCERN: org.bukkit.Location is constructed directly with null world in a test that may or may not require MockBukkit to be active, but the existing MethodsTest suite must already handle this. The structural question is whether new org.bukkit.Location(null, ...) works without MockBukkit being initialized, and whether the rest of MethodsTest already sets up MockBukkit.

Looking at the diff context: the existing MethodsTest already uses org.bukkit.Location with null worlds (visible from the surrounding tests at line ~726 using the same pattern). If MockBukkit is set up at the class level, using null world is fine. If it is not, and existing tests pass without it, then this new test is consistent with the existing pattern. No new structural problem is introduced here beyond what already exists. ✅ (consistent with existing suite pattern)

Bukkit-Dependent vs. Pure-Java Separation

The lookAt test uses Bukkit's Location class. If MethodsTest as a whole runs without MockBukkit (relying on Bukkit's Location being instantiable with null world via CraftBukkit/MockBukkit), this is already the established pattern in the file. The new test does not diverge from that pattern. ✅

Framework Test Quality

CONCERN: The lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX test name does not match the required givenContext_whenAction_thenOutcome naming convention.
WHY: The method is named lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX. The convention mandates the prefix given on the context segment. The name skips the given prefix entirely and leads with the method under test, which is a structural violation of the project's naming convention — not a style preference, since consistent naming is enforced as a framework test quality requirement in this checklist.
WHERE: src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java, method lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX

---

CONCERN: The lookAt test asserts pitch == 0.0 with a delta of 0.01, but it does not assert the scenario where origin and target have differing Y values, meaning the pitch calculation branch of Methods.lookAt is not covered by this new test. Given that the @DisplayName states "pitch is near zero" yet the assertion uses assertEquals(0.0, result.getPitch(), 0.01) as if pitch is expected to be exactly zero, the test conflates "near zero" with "zero" without justification.
WHY: This is not a style concern — it is a coverage and correctness gap. If Methods.lookAt has non-trivial pitch calculation logic (distance-based arctangent), the only scenario tested here (same X, same Y=0, different Z) may trivially produce pitch=0 by construction (no vertical displacement), meaning the pitch assertion is vacuous and adds no coverage of the pitch calculation code path. The pitch logic branch where dy != 0 remains uncovered by this addition.
WHERE: src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java, method lookAt_setsYawToZero_whenTargetIsOnPositiveZWithSameX / production Methods.lookAt

---

CONCERN: The two new ManagerRegistryTest methods do not follow the givenContext_whenAction_thenOutcome naming convention.
WHY: registered_returnsFalse_whenOnlyNonMatchingManagerRegistered and registered_returnsFalse_whenOnlyNonMatchingManagerRegisteredByKey begin with the method under test (registered_) rather than given. This violates the structural naming requirement defined in the checklist.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java, both new methods

---

CONCERN: The two new ManagerRegistryTest methods test the negative path of registered() (returns false when a non-matching manager is registered), but there is no corresponding test for the empty-registry case — i.e., registered(instance) and registered(key) when the registry contains zero entries.
WHY: The empty-collection edge case is explicitly called out in the checklist. A registry with zero entries is a distinct and important boundary from a registry with one non-matching entry. The behavior (e.g., null-safety of internal map lookup) may differ.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java / production ManagerRegistry.registered()

---

CONCERN: Neither new ManagerRegistryTest method tests registered() with a null argument.
WHY: The checklist explicitly requires null-input edge cases for non-trivial logic. If ManagerRegistry.registered(Manager) or ManagerRegistry.registered(NamespacedKey) does not guard against null, a NullPointerException is a latent bug. No test verifies the contract for null inputs.
WHERE: src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java / production ManagerRegistry.registered()

---

Summary

Production files changed: None

Test files present:

• src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
• src/test/java/com/diamonddagger590/mccore/util/MethodsTest.java

Coverage gaps:

1. Methods.lookAt pitch-nonzero branch (dy ≠ 0) not covered by the new test
2. ManagerRegistry.registered() with an empty registry not covered
3. ManagerRegistry.registered() with null arguments not covered

Structural problems:

1. Three of four new test methods violate the givenContext_whenAction_thenOutcome naming convention
2. The pitch assertion in the lookAt test is vacuous for its stated purpose (zero vertical displacement guarantees pitch=0 regardless of pitch logic correctness)

[github-actions]

Security Review

No security concerns found.

Both changed files are test classes only — no DAO methods, DDL constructors, MiniMessage deserialization, ChatResponse handling, onClick() implementations, or permission-gated operations are introduced or modified. The diff adds unit test cases for ManagerRegistry and Methods.lookAt() with no production code changes and no player-influenced data flowing through any McCore security boundary.