[Tests] Add unit tests for HooksRegistrar and improve Sfun, ManagerRegistry coverage#49
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughMockito test dependencies were added to the Gradle build. New tests cover hook registration selection, ChangesHooks registrar tests
Sfun edge cases
Manager registry behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Extensibility ReviewBreaking change risk: NONE — this diff adds only test code and a test dependency; no production sources are modified. The entire diff consists of:
None of these changes touch production source files, public APIs, abstract classes, interfaces, registry constants, GUI framework classes, or database migration logic. CONCERN: Aside from the extensibility observation above (which is a design note on the production class revealed by the test, not caused by the test itself), no contract-breaking changes are present in this diff. No renamed symbols, no new abstract methods, no signature changes, no |
Testing ReviewI'll systematically apply every checklist item to the diff. AnalysisProduction files changed (inferred from test targets)
Test files present
FindingsCONCERN: CONCERN: CONCERN: CONCERN: There is no test for the scenario where a plugin was previously registered and CONCERN: CONCERN: The three new CONCERN: CONCERN: CONCERN: No test anywhere in this diff (or referenced existing tests) covers the behavior of SummaryProduction files changed:
|
Security ReviewNo security concerns found. The diff contains exclusively test infrastructure changes: Mockito dependency declarations in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/diamonddagger590/mccore/bootstrap/registrar/HooksRegistrarTest.java`:
- Around line 61-66: Add IntelliJ `@NotNull` annotations to the non-null helper
signatures in `HooksRegistrarTest`: annotate the `context()` return type and the
`enablePlugin(String)` parameter, and add the missing
`org.jetbrains.annotations.NotNull` import. Keep the fix scoped to these helper
methods and preserve their existing behavior.
In `@src/test/java/com/diamonddagger590/mccore/parser/SfunTest.java`:
- Around line 160-171: The cosh infinity tests only assert Double.isInfinite(),
so they allow the wrong sign to pass; update the Sfun.cosh test cases to
explicitly assert Double.POSITIVE_INFINITY for both Double.POSITIVE_INFINITY and
Double.NEGATIVE_INFINITY inputs. Use the existing test methods
cosh_returnsInfinity_whenInputIsPositiveInfinity and
cosh_returnsInfinity_whenInputIsNegativeInfinity in SfunTest to pin the contract
and catch the upstream sign bug.
In
`@src/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java`:
- Line 145: Rename the test method in ManagerRegistryTest so it follows the
repository convention methodUnderTest_expectedOutcome_whenCondition and matches
the actual scenario exercised by the assertion. The current name suggests a
parent-instance check, but the test uses a subclass instance; update the method
name (and keep it aligned with the existing `@DisplayName`) to reflect that
registered() returns true when a subclass instance is registered and checked by
the subclass 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: 22c102af-1c0f-4d7d-9779-0887af131d2c
📒 Files selected for processing (4)
build.gradle.ktssrc/test/java/com/diamonddagger590/mccore/bootstrap/registrar/HooksRegistrarTest.javasrc/test/java/com/diamonddagger590/mccore/parser/SfunTest.javasrc/test/java/com/diamonddagger590/mccore/registry/manager/ManagerRegistryTest.java
| @Test | ||
| @DisplayName("Given positive infinity, when computing cosh, then returns infinity") | ||
| void cosh_returnsInfinity_whenInputIsPositiveInfinity() { | ||
| double result = Sfun.cosh(Double.POSITIVE_INFINITY); | ||
| assertTrue(Double.isInfinite(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given negative infinity, when computing cosh, then returns infinity") | ||
| void cosh_returnsInfinity_whenInputIsNegativeInfinity() { | ||
| double result = Sfun.cosh(Double.NEGATIVE_INFINITY); | ||
| assertTrue(Double.isInfinite(result)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert Double.POSITIVE_INFINITY explicitly.
Line 164 and Line 171 only check isInfinite(), so the negative-infinity case still passes even though Sfun.cosh(Double.NEGATIVE_INFINITY) currently returns the wrong sign upstream. These tests should pin the contract to positive infinity or they miss the bug they are meant to catch.
Proposed fix
`@Test`
`@DisplayName`("Given positive infinity, when computing cosh, then returns infinity")
void cosh_returnsInfinity_whenInputIsPositiveInfinity() {
double result = Sfun.cosh(Double.POSITIVE_INFINITY);
- assertTrue(Double.isInfinite(result));
+ assertEquals(Double.POSITIVE_INFINITY, result);
}
`@Test`
`@DisplayName`("Given negative infinity, when computing cosh, then returns infinity")
void cosh_returnsInfinity_whenInputIsNegativeInfinity() {
double result = Sfun.cosh(Double.NEGATIVE_INFINITY);
- assertTrue(Double.isInfinite(result));
+ assertEquals(Double.POSITIVE_INFINITY, result);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| @DisplayName("Given positive infinity, when computing cosh, then returns infinity") | |
| void cosh_returnsInfinity_whenInputIsPositiveInfinity() { | |
| double result = Sfun.cosh(Double.POSITIVE_INFINITY); | |
| assertTrue(Double.isInfinite(result)); | |
| } | |
| @Test | |
| @DisplayName("Given negative infinity, when computing cosh, then returns infinity") | |
| void cosh_returnsInfinity_whenInputIsNegativeInfinity() { | |
| double result = Sfun.cosh(Double.NEGATIVE_INFINITY); | |
| assertTrue(Double.isInfinite(result)); | |
| `@Test` | |
| `@DisplayName`("Given positive infinity, when computing cosh, then returns infinity") | |
| void cosh_returnsInfinity_whenInputIsPositiveInfinity() { | |
| double result = Sfun.cosh(Double.POSITIVE_INFINITY); | |
| assertEquals(Double.POSITIVE_INFINITY, result); | |
| } | |
| `@Test` | |
| `@DisplayName`("Given negative infinity, when computing cosh, then returns infinity") | |
| void cosh_returnsInfinity_whenInputIsNegativeInfinity() { | |
| double result = Sfun.cosh(Double.NEGATIVE_INFINITY); | |
| assertEquals(Double.POSITIVE_INFINITY, result); | |
| } |
🤖 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/parser/SfunTest.java` around lines
160 - 171, The cosh infinity tests only assert Double.isInfinite(), so they
allow the wrong sign to pass; update the Sfun.cosh test cases to explicitly
assert Double.POSITIVE_INFINITY for both Double.POSITIVE_INFINITY and
Double.NEGATIVE_INFINITY inputs. Use the existing test methods
cosh_returnsInfinity_whenInputIsPositiveInfinity and
cosh_returnsInfinity_whenInputIsNegativeInfinity in SfunTest to pin the contract
and catch the upstream sign bug.
…overage Add HooksRegistrarTest with 10 tests covering all testable plugin hook registrations (Nexo, ItemsAdder, PlaceholderAPI, ModelEngine, MythicMobs, CMI, Citizens). HeadDatabase is excluded because its API is compileOnly. Expand SfunTest with 8 new tests covering cosh infinity/large-value branches, erfc negative-x and small-x branches, and tanh negative range. Add 2 ManagerRegistryTest cases for key-miss fallback and subclass instance registration. Add Mockito test dependencies (mockito-core, mockito-junit-jupiter). Coverage: 33.0% -> 34.5% (+58 lines) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvWAiwNFbF4qdRwU1aU9rv
- Add @NotNull annotations to HooksRegistrarTest helper methods - Assert exact infinity values in SfunTest cosh tests instead of isInfinite() - Rename ManagerRegistryTest method to match actual subclass-instance scenario Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvWAiwNFbF4qdRwU1aU9rv
19df49c to
269f256
Compare
Extensibility ReviewBreaking change risk: NONE — this diff contains only test code additions with no changes to production source files. All three modified files reside under No extensibility concerns found. |
Testing ReviewI'll systematically apply each checklist item to every file in the diff. File-by-File Analysis
|
Security ReviewNo security concerns found. The diff contains exclusively test code additions across three test files ( |
2 participants
Summary
compileOnlyand causesNoClassDefFoundErrorat test time.coshinfinity/large-value branches,erfcnegative-x and small-x branches, andtanhnegative range.mockito-core,mockito-junit-jupiter5.14.2) required for static mocking ofBukkit.getPluginManager().Coverage
Test plan
./gradlew test)🤖 Generated with Claude Code
https://claude.ai/code/session_01GvWAiwNFbF4qdRwU1aU9rv
Generated by Claude Code
Summary by CodeRabbit
Tests
Chores