[Tests] Improve coverage for Statistic default method, SimpleStatistic, StatisticCache, ItemBuilderConfigurationKeys

[DiamondDagger590]

Summary

• Cover Statistic.getMaxSetSize() default method (0% → 100%): Added a test using a minimal Statistic implementation that does not override the default method, verifying it returns -1.
• Cover ItemBuilderConfigurationKeys constructor (97% → 100%): Added a test that instantiates the utility class to cover the implicit constructor.
• Expand SimpleStatisticTest from 4 to 16 tests: Added equals/hashCode edge cases (self-reference, null, different type, differing StatisticType, differing maxSetSize), toString verification, all StatisticType variants (INT, LONG, DOUBLE, STRING, TIMESTAMP, SET_STRING), and record accessor consistency checks. Reorganized into @Nested classes.
• Expand StatisticCacheTest from 6 to 15 tests: Added multi-player isolation (separate values per player, cross-player query returns empty), overwrite behavior, empty cache invalidation safety (no-op on empty cache for both UUID and key invalidation), cross-player size tracking, and entry-level invalidation isolation. Reorganized into @Nested classes.
• Renamed all carried-over test methods to follow methodUnderTest_expectedOutcome_whenCondition convention.

Overall coverage: 80.7% line / 70.7% branch (up from 80.6% / 70.7%). These were the only remaining non-PR-covered gaps — all other classes with < 100% coverage are addressed by existing open PRs.

Test plan

• All 16 new/expanded SimpleStatisticTest tests pass
• All 15 new/expanded StatisticCacheTest tests pass
• ItemBuilderConfigurationKeysTest constructor test passes
• Full test suite passes (./gradlew test)
• JaCoCo confirms Statistic and ItemBuilderConfigurationKeys at 100% line/branch
• Testing auditor persona reviewed changes — naming convention issues fixed

---

Generated by Claude Code

Summary by CodeRabbit

• Tests
  • Expanded coverage for statistic creation, accessors, equality, hash codes, string output, and default behavior.
  • Added comprehensive cache tests for retrieval, updates, player isolation, invalidation, size tracking, type handling, and expiration.
  • Added verification that item builder configuration keys can be instantiated successfully.

[github-actions]

Claude finished @DiamondDagger590's task in 49s —— View job

---

Claude PR Review

No blocking issues found.

All 3 changed files are under src/test/java/** (SimpleStatisticTest, StatisticCacheTest, ItemBuilderConfigurationKeysTest) — no src/main/**/*.java or extensibility-sensitive paths (event/, registry/, gui/, database/, public signatures) are touched, so per the routing rules in .github/claude-review-prompt.md none of the code-focused review lenses (security, architecture, error-handling, performance, extensibility) apply here.

I read through the diff directly: the new/reorganized tests follow the methodUnderTest_expectedOutcome_whenCondition naming convention, use @Nested/@DisplayName consistently, and cover meaningful edge cases (equals/hashCode contract, all StatisticType variants, multi-player cache isolation, empty-cache invalidation no-ops). Nothing looks incorrect or risky.

• Gather context (PR diff, changed files)
• Route to review lenses — none apply (test-only diff)
• Fan out to applicable persona subagents — skipped (no matching lens)
• Consolidate findings — none
• Post final review

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request expands JUnit coverage for SimpleStatistic and StatisticCache. It also adds a constructor-instantiation test for ItemBuilderConfigurationKeys.

Changes

Statistics and Builder Test Coverage

Layer / File(s)	Summary
SimpleStatistic behavior coverage src/test/java/com/diamonddagger590/mccore/statistic/SimpleStatisticTest.java	Nested tests cover constructors, statistic types, equality, hash codes, string output, accessors, and Statistic default methods.
StatisticCache behavior coverage src/test/java/com/diamonddagger590/mccore/statistic/cache/StatisticCacheTest.java	Nested tests cover cache retrieval, insertion, overwrites, player isolation, invalidation, size tracking, statistic types, and TTL expiration.
ItemBuilderConfigurationKeys construction coverage src/test/java/com/diamonddagger590/mccore/builder/item/ItemBuilderConfigurationKeysTest.java	The test verifies successful construction and a non-null instance.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• DiamondDagger590/McCore#33: Adds related unit-test coverage for statistic components, including PlayerStatisticData.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the pull request's test coverage improvements across the targeted classes.
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

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

---

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

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)

src/test/java/com/diamonddagger590/mccore/statistic/cache/StatisticCacheTest.java-184-197 (1)

184-197: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Make the TTL test deterministic.

Thread.sleep(1500) depends on wall-clock timing and scheduler availability. Inject a fixed TimeProvider into the cache test path and advance it explicitly before the expiry assertion.

As per coding guidelines, “In tests that assert time-dependent behavior, inject a mock or fixed TimeProvider rather than depending on wall-clock time.”

🤖 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/statistic/cache/StatisticCacheTest.java`
around lines 184 - 197, The TTL expiration test method
get_returnsEmpty_whenTtlExpires currently relies on Thread.sleep and wall-clock
timing; inject a fixed or mock TimeProvider when constructing StatisticCache,
then explicitly advance that provider past the TTL before the empty-result
assertion. Preserve the existing assertions that the entry is initially present
and absent after expiration.

Source: Coding guidelines

src/test/java/com/diamonddagger590/mccore/statistic/SimpleStatisticTest.java-122-122 (1)

122-122: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace the direct null equality call.

PMD flags stat.equals(null). Use assertNotEquals(stat, null) to retain the equality-contract check without the direct null comparison.

Proposed fix

-            assertFalse(stat.equals(null));
+            assertNotEquals(stat, null);

🤖 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/statistic/SimpleStatisticTest.java`
at line 122, In SimpleStatisticTest, replace the direct stat.equals(null)
assertion with assertNotEquals(stat, null), preserving the existing
equality-contract check without invoking equals directly with null.

Source: Linters/SAST tools

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

Other comments:
In
`@src/test/java/com/diamonddagger590/mccore/statistic/cache/StatisticCacheTest.java`:
- Around line 184-197: The TTL expiration test method
get_returnsEmpty_whenTtlExpires currently relies on Thread.sleep and wall-clock
timing; inject a fixed or mock TimeProvider when constructing StatisticCache,
then explicitly advance that provider past the TTL before the empty-result
assertion. Preserve the existing assertions that the entry is initially present
and absent after expiration.

In
`@src/test/java/com/diamonddagger590/mccore/statistic/SimpleStatisticTest.java`:
- Line 122: In SimpleStatisticTest, replace the direct stat.equals(null)
assertion with assertNotEquals(stat, null), preserving the existing
equality-contract check without invoking equals directly with null.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 57fc2d71-d0b8-48c9-8e66-0d02c7e4707c

📥 Commits

Reviewing files that changed from the base of the PR and between 0fbf220 and a81de29.

📒 Files selected for processing (3)

• src/test/java/com/diamonddagger590/mccore/builder/item/ItemBuilderConfigurationKeysTest.java
• src/test/java/com/diamonddagger590/mccore/statistic/SimpleStatisticTest.java
• src/test/java/com/diamonddagger590/mccore/statistic/cache/StatisticCacheTest.java