[Tests] Add unit tests for CoreTask, MultiExecutionCoreTask, DelayableCoreTask, RepeatableCoreTask, CancelableCoreTask, ExpireableCoreTask#38
Conversation
…sk, DelayableCoreTask, RepeatableCoreTask, CancelableCoreTask, ExpireableCoreTask) https://claude.ai/code/session_01FVP5Wg4o96EX7GfVUwDdQf
WalkthroughAdds ChangesTask hierarchy unit test suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Testing ReviewAudit ReportProduction files changed: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: No test covers CONCERN: No test exercises CONCERN: CONCERN: CONCERN: |
Security ReviewNo security concerns found. The diff adds Mockito dependencies to the test build configuration and introduces five new unit test classes ( Reviewing against each checklist item:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/task/core/DelayableCoreTaskTest.java`:
- Around line 55-56: The test setup uses `any(Long.class)` wrapper matcher in
the stubs for `mockScheduler.runTaskLater` and
`mockScheduler.runTaskLaterAsynchronously` methods, but these methods expect a
primitive `long` parameter. Since `any(Long.class)` can return null and unboxing
causes NullPointerException, replace `any(Long.class)` with `anyLong()` in both
lines to properly match primitive long values without null-boxing issues.
In
`@src/test/java/com/diamonddagger590/mccore/task/core/MultiExecutionCoreTaskTest.java`:
- Around line 129-140: The test runTask_switchingAsyncSync_reflectsLatest claims
to validate that the async flag reflects the latest run mode, but it is missing
a critical assertion. After calling task.runTask(false) on the sync execution,
the test immediately checks getExecutions() but never verifies that
isTaskRunningAsync() has been set to false. Add an assertion after the sync
runTask call to explicitly verify that isTaskRunningAsync() returns false,
completing the validation of the test's stated purpose.
🪄 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: 65f89b65-0db2-4b31-a647-e6af31fa3aa4
📒 Files selected for processing (7)
build.gradle.ktssrc/test/java/com/diamonddagger590/mccore/task/core/CancelableCoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/CoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/ExpireableCoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/MultiExecutionCoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.java
| when(mockScheduler.runTaskLater(any(), any(Runnable.class), any(Long.class))).thenReturn(mockBukkitTask); | ||
| when(mockScheduler.runTaskLaterAsynchronously(any(), any(Runnable.class), any(Long.class))).thenReturn(mockBukkitTask); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the problematic matcher usage exists in this file.
rg -n "runTaskLater\\(any\\(\\), any\\(Runnable\\.class\\), any\\(Long\\.class\\)\\)" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java
rg -n "runTaskLaterAsynchronously\\(any\\(\\), any\\(Runnable\\.class\\), any\\(Long\\.class\\)\\)" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.javaRepository: DiamondDagger590/McCore
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Check the BukkitScheduler runTaskLater method signature
rg -n "runTaskLater" --type java -A 2 -B 2 src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java | head -30
# Check imports in the test file
rg -n "^import" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java | grep -i "mockito\|argumentmatcher"
# Check if anyLong is already imported
rg "anyLong" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.javaRepository: DiamondDagger590/McCore
Length of output: 1731
🏁 Script executed:
#!/bin/bash
# Get full context of the test file around lines 55-56
head -60 src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java | tail -30
# Check if anyLong is used elsewhere in test files
rg "anyLong" src/test/java/
# Look at the BukkitScheduler method signatures in Paper API
# Check what the actual parameter types are expected to be
fd "*.java" src/ | xargs rg "runTaskLater\(" --type java -A 1 | head -20Repository: DiamondDagger590/McCore
Length of output: 2745
🏁 Script executed:
#!/bin/bash
# Check the BukkitScheduler interface/class to see the actual method signatures
# Look for the method declarations in the codebase or check Paper API docs
rg "BukkitScheduler" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java -A 5 -B 5
# Check if there are any actual test method that runs - to see if the matcher issue causes failures
rg "void.*runTask_sync_usesRunTaskLater" src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java -A 10
# Check Mockito documentation - see if there are any compiler warnings or test execution issues
# Let's check the actual full test file
wc -l src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.javaRepository: DiamondDagger590/McCore
Length of output: 1287
🏁 Script executed:
#!/bin/bash
# Check how the production code calls runTaskLater to understand the parameter types
cat src/main/java/com/diamonddagger590/mccore/task/core/DelayableCoreTask.java | grep -n "runTaskLater" -A 2 -B 2
# Look at the complete test file to understand the context better
head -80 src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java
# Check if anyLong is imported anywhere in the test suite
rg "import.*anyLong" src/test/java/Repository: DiamondDagger590/McCore
Length of output: 3185
🏁 Script executed:
#!/bin/bash
# Let's verify the BukkitScheduler interface method signatures
# Check if Paper/Bukkit is available as a dependency to see the actual signature
fd "BukkitScheduler" --type f 2>/dev/null | head -5
# Check gradle files to understand Paper API version
rg "bukkit|paper" build.gradle -i | grep -i "version\|api\|dependency"
# Let's look for how other tests in the codebase handle similar scheduler mocking
find src/test/java -name "*.java" -type f | xargs rg "runTaskLater\|anyLong" | head -20
# Check if the current test file actually compiles and runs successfully
# by looking for CI/test runner logs or errors
git log --oneline -1Repository: DiamondDagger590/McCore
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Let's examine if the tests actually run and pass with the current code
# Check the git log for test-related commits
git log --oneline src/test/java/com/diamonddagger590/mccore/task/core/DelayableCoreTaskTest.java | head -10
# Let's look at the build.gradle or gradle files to understand the Paper/Bukkit API version
find . -name "*.gradle*" -type f | head -5
cat build.gradle.kts 2>/dev/null || cat settings.gradle 2>/dev/null || true
# Check if there are any similar patterns in other test files (checking how they mock schedulers)
find src/test/java -name "*.java" -type f -exec grep -l "runTaskLater\|BukkitScheduler" {} \;
# Let's verify what the correct matcher should be according to Mockito
# Check if anyLong is mentioned anywhere in the test directory
rg "ArgumentMatchers" src/test/java/ | grep -i "long\|any" | head -5Repository: DiamondDagger590/McCore
Length of output: 6747
🏁 Script executed:
#!/bin/bash
# Check if other task test files have similar scheduler mocking patterns
for file in src/test/java/com/diamonddagger590/mccore/task/core/*.java; do
echo "=== $(basename $file) ==="
grep -n "runTaskLater\|anyLong\|any(Long" "$file" 2>/dev/null | head -5
done
# Check the Paper API BukkitScheduler interface documentation
# Look for the actual method signature
rg "runTaskLater.*long" . --type java 2>/dev/null | head -10
# Let's check the exact type signature from Paper by searching for method definitions
rg "BukkitTask runTaskLater" . --type java -A 1 2>/dev/nullRepository: DiamondDagger590/McCore
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Check if there are CI/test execution logs or recent test results
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "test\|junit" 2>/dev/null | head -5
# Look for any GitHub Actions or CI configuration
ls -la .github/ 2>/dev/null || echo "No .github directory"
# Check if the recent commit that added tests indicates any issues
git log --all --oneline --grep="DelayableCoreTask\|CoreTask" | head -10
# Let's verify the exact Paper API BukkitScheduler signature by looking at what Mockito expects
# Create a simple test to understand the actual types
python3 - <<'PY'
# According to Paper/Bukkit API, BukkitScheduler.runTaskLater has signature:
# BukkitTask runTaskLater(Plugin plugin, Runnable task, long delay)
# The third parameter is a PRIMITIVE long, not Long (wrapper)
# In Mockito:
# - any(Long.class) is for wrapper type Long - can return null
# - anyLong() is for primitive long - cannot be null
# Using any(Long.class) on a primitive long parameter will cause:
# When Mockito tries to unbox the null return from any(Long.class), it will throw NPE
print("Analysis of the matcher issue:")
print("1. BukkitScheduler.runTaskLater(Plugin, Runnable, long) - third param is PRIMITIVE long")
print("2. any(Long.class) creates a matcher for wrapper Long type - can return null")
print("3. Unboxing null to primitive long causes NullPointerException")
print("4. Correct matcher: anyLong() - for primitive long parameters")
print("")
print("This is a real bug that should cause test setup to fail.")
PYRepository: DiamondDagger590/McCore
Length of output: 999
🏁 Script executed:
#!/bin/bash
# Check the GitHub Actions workflow to see if tests were run
cat .github/workflows/gradle.yml | grep -A 10 "test"
# Check recent workflow runs/status
git log --all --oneline -15
# Try to understand if the tests actually ran successfully
# by checking the commit message for any indication of test results
git show --format=full a79faee | head -30
# Let's also verify: does anyLong need to be imported?
# Check Mockito ArgumentMatchers to confirm anyLong exists
python3 - <<'PY'
# Mockito 5.14.2 (the version in build.gradle.kts) definitely has anyLong()
# This is a standard matcher in org.mockito.ArgumentMatchers
print("Mockito 5.14.2 includes ArgumentMatchers.anyLong()")
print("The fix requires:")
print("1. Import: import static org.mockito.ArgumentMatchers.anyLong;")
print("2. Replace any(Long.class) with anyLong() on lines 55-56")
PYRepository: DiamondDagger590/McCore
Length of output: 2948
🏁 Script executed:
#!/bin/bash
# Final verification: Check if the tests can even run with the current code
# by checking if there's any indication they were supposed to run
cat .github/workflows/gradle.yml
# Confirm that anyLong() is the correct matcher and needs to be imported
python3 - <<'PY'
# Verify Mockito ArgumentMatchers API
# In Mockito 5.14.2+:
# - any(Long.class) -> ArgumentMatcher<Long> - returns null for wrapper type
# - anyLong() -> ArgumentMatcher for primitive long - cannot return null
# BukkitScheduler signature (from Paper API):
# BukkitTask runTaskLater(Plugin plugin, Runnable task, long delay)
# ^^^^ primitive
# When mocking:
# - any(Long.class) will cause null to be unboxed to primitive long -> NPE
# - anyLong() is the correct matcher for primitive long
print("✓ anyLong() is the correct matcher for primitive long parameters")
print("✓ Import needed: import static org.mockito.ArgumentMatchers.anyLong;")
print("✓ The diff in the review is correct")
print("")
print("Root cause: Lines 55-56 use any(Long.class) for primitive long params")
print("Impact: Test setup fails with NullPointerException during `@BeforeEach`")
print("Severity: CRITICAL - blocks all tests in this class")
PYRepository: DiamondDagger590/McCore
Length of output: 1006
Replace wrapper matcher with primitive matcher in scheduler stubs.
Lines 55 and 56 use any(Long.class) for methods that expect a primitive long parameter. Since any(Long.class) can return null and unboxing null to primitive long causes NullPointerException, the test setup fails in @BeforeEach. Use anyLong() instead.
Proposed fix
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
@@
- when(mockScheduler.runTaskLater(any(), any(Runnable.class), any(Long.class))).thenReturn(mockBukkitTask);
- when(mockScheduler.runTaskLaterAsynchronously(any(), any(Runnable.class), any(Long.class))).thenReturn(mockBukkitTask);
+ when(mockScheduler.runTaskLater(any(), any(Runnable.class), anyLong())).thenReturn(mockBukkitTask);
+ when(mockScheduler.runTaskLaterAsynchronously(any(), any(Runnable.class), anyLong())).thenReturn(mockBukkitTask);🤖 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/task/core/DelayableCoreTaskTest.java`
around lines 55 - 56, The test setup uses `any(Long.class)` wrapper matcher in
the stubs for `mockScheduler.runTaskLater` and
`mockScheduler.runTaskLaterAsynchronously` methods, but these methods expect a
primitive `long` parameter. Since `any(Long.class)` can return null and unboxing
causes NullPointerException, replace `any(Long.class)` with `anyLong()` in both
lines to properly match primitive long values without null-boxing issues.
| @DisplayName("Given a multi-execution task, when run async then sync, then async flag reflects last run") | ||
| void runTask_switchingAsyncSync_reflectsLatest() { | ||
| MultiExecutionCoreTask task = createTask(); | ||
|
|
||
| task.runTask(true); | ||
| assertTrue(task.isTaskRunningAsync()); | ||
|
|
||
| task.runTask(false); | ||
| // After sync run, the parent sets taskRunningAsync = false | ||
| // But MultiExecutionCoreTask resets taskExecuted then calls super | ||
| assertEquals(2, task.getExecutions()); | ||
| } |
There was a problem hiding this comment.
Assert the final async flag state explicitly.
The test name/display text says it validates latest run mode, but after Line 136 it never checks that the sync run sets isTaskRunningAsync() to false.
Proposed fix
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@
void runTask_switchingAsyncSync_reflectsLatest() {
MultiExecutionCoreTask task = createTask();
@@
task.runTask(false);
- // After sync run, the parent sets taskRunningAsync = false
- // But MultiExecutionCoreTask resets taskExecuted then calls super
+ assertFalse(task.isTaskRunningAsync());
assertEquals(2, task.getExecutions());
}🤖 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/task/core/MultiExecutionCoreTaskTest.java`
around lines 129 - 140, The test runTask_switchingAsyncSync_reflectsLatest
claims to validate that the async flag reflects the latest run mode, but it is
missing a critical assertion. After calling task.runTask(false) on the sync
execution, the test immediately checks getExecutions() but never verifies that
isTaskRunningAsync() has been set to false. Add an assertion after the sync
runTask call to explicitly verify that isTaskRunningAsync() returns false,
completing the validation of the test's stated purpose.
2 participants
Summary
runTask()sync scheduling,runTask(false)sync path,runTask(true)async path,TaskCompletedExceptionon double execution for both sync and async paths (0% → 100% line coverage, 22/22 lines)getTaskStartTime()returning correct epoch millis (0% → 100% line coverage, 9/9 lines)runTaskLaterwith correct tick conversion (seconds × 20), asyncrunTaskLaterAsynchronously, and zero-delay zero-tick edge case (0% → 100% line coverage, 12/12 lines)runTaskTimer, asyncrunTaskTimerAsynchronously, delay expiration timing, interval completion after frequency, pause preventing execution, resume restoring execution, idempotent pause/resume, andtogglePause()in both directions (0% → 100% line coverage, 47/47 lines)cancelTask()flag setting, cancelledrun()callingBukkit.cancelTask()andonCancel(), cancelledrun()skipping parent logic, and non-cancelledrun()delegating normally (0% → 100% line coverage, 11/11 lines)onCancel, interval expiry with maxIntervals=1 and maxIntervals=3, both limits with duration first vs intervals first, and no-duration-limit time immunity (0% → 100% line coverage, 34/34 lines)Infrastructure change
Added Mockito 5.14.2 (
mockito-core+mockito-junit-jupiter) astestImplementationdependencies to enable mockingCorePlugin,BukkitScheduler,BukkitTask, andBukkitstatic methods for task scheduling tests.Classes covered (avoiding PR #27–#37 overlap)
PR #27 covers GetItemRequest, StatisticEntry, ReloadableContent, ReloadableContentManager, StartupProfile. PR #28 covers Methods, PlayerSettingRegistry, ReloadableContent subtypes. PR #29 covers Mutexable, ParseError, EvaluationException, exception classes. PR #30 covers RegistryAccess, StatisticModifyEvent, PostStatisticModifyEvent, Transaction. PR #31 covers database events, GUI events, SQLiteDatabaseDriver, DatabaseDriver, ManagerKey/PluginHookKey constants. PR #32 covers player events, CorePlayer, Credentials, ConnectionDetails, CorePlayerOfflineException. PR #33 covers PlayerStatisticData, ReloadableParser, ItemPluginType, ChainPlayerContextFilter. PR #34 covers BatchTransaction, FailSafeTransaction, ItemBuilderConfigurationKeys, RegistryKeyImpl. PR #35 covers BootstrapContext, IllegalSlotAssignmentException, TableVersionHistoryDAO, MutexDAO. PR #36 covers PlayerSettingDAO, GuiManager, GetItemResponseState, exception classes. PR #37 covers PlayerManager, ChatResponseManager, ChatResponse, LocalizationManager. This PR targets entirely different classes with no overlap.
Coverage impact
6 classes brought from 0% to 100% line coverage. 135 new lines covered. Overall project coverage: 31.9% → 36.4% (+4.5pp).
Test plan
./gradlew testhttps://claude.ai/code/session_01FVP5Wg4o96EX7GfVUwDdQf
Generated by Claude Code
Summary by CodeRabbit