[Tests] Improve coverage for CreateCoreTablesFunction and UpdateCoreTablesFunction - #93
Open
DiamondDagger590 wants to merge 1 commit into
Open
[Tests] Improve coverage for CreateCoreTablesFunction and UpdateCoreTablesFunction#93DiamondDagger590 wants to merge 1 commit into
DiamondDagger590 wants to merge 1 commit into
Conversation
…esFunction Expand CreateCoreTablesFunctionTest from 3 to 7 tests and UpdateCoreTablesFunctionTest from 3 to 6 tests, achieving 100% coverage for both classes across all JaCoCo metrics. New tests cover table creation paths (tables exist, tables don't exist, mixed), connection close exception handling, DAO method invocation verification, and direct constructor instantiation. Extract shared CallerRunsExecutor to testFixtures for reuse by downstream plugins that need mockStatic-safe executor testing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEwPGvLVRQQN5vzAWgukUm
|
Claude finished @DiamondDagger590's task in 2m 11s —— View job Reviewing PR #93
|
📝 WalkthroughWalkthroughThe PR adds a synchronous test executor and expands tests for core table creation and update functions. The tests cover table states, DAO calls, connection-close failures, asynchronous completion, and direct construction. ChangesCore table function tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
src/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java-23-26 (1)
23-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject tasks after shutdown.
executerunscommandaftershutdown()orshutdownNow(). This bypasses theExecutorServicelifecycle contract and can make post-teardown tests execute unexpectedly.Proposed fix
+import java.util.concurrent.RejectedExecutionException; + `@Override` public void execute(Runnable command) { + if (isShutdown()) { + throw new RejectedExecutionException("Executor has been shut down"); + } command.run(); }🤖 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/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java` around lines 23 - 26, Update CallerRunsExecutor.execute to reject submissions after shutdown or shutdownNow by checking the executor’s lifecycle state before running the command and throwing the standard rejection exception when it is no longer active; preserve the current caller-runs behavior for accepted tasks.src/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAnnotate the non-null
commandparameter with IntelliJ@NotNull.🤖 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/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java` at line 24, Annotate the non-null command parameter in CallerRunsExecutor.execute with IntelliJ’s `@NotNull` annotation, adding the necessary import if absent.Source: Coding guidelines
🤖 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/database/table/function/CreateCoreTablesFunctionTest.java`:
- Around line 164-192: Move normal CompletableFuture completion until after the
try-with-resources block exits so Connection.close() failures remain
exceptional. Update
createTables_completesExceptionally_whenConnectionCloseThrowsSQLException in
src/test/java/com/diamonddagger590/mccore/database/table/function/CreateCoreTablesFunctionTest.java
lines 164-192 and the corresponding close-failure test in
src/test/java/com/diamonddagger590/mccore/database/table/function/UpdateCoreTablesFunctionTest.java
lines 99-122 to assert join() fails with SQLException as its cause, replacing
the insufficient isDone() assertion and stale comments.
---
Other comments:
In
`@src/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java`:
- Around line 23-26: Update CallerRunsExecutor.execute to reject submissions
after shutdown or shutdownNow by checking the executor’s lifecycle state before
running the command and throwing the standard rejection exception when it is no
longer active; preserve the current caller-runs behavior for accepted tasks.
- Line 24: Annotate the non-null command parameter in CallerRunsExecutor.execute
with IntelliJ’s `@NotNull` annotation, adding the necessary import if absent.
🪄 Autofix
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: QUIET
Plan: Pro Plus
Run ID: 66cec97e-5fcf-4cf9-94ef-f3509af0ade7
📒 Files selected for processing (3)
src/test/java/com/diamonddagger590/mccore/database/table/function/CreateCoreTablesFunctionTest.javasrc/test/java/com/diamonddagger590/mccore/database/table/function/UpdateCoreTablesFunctionTest.javasrc/testFixtures/java/com/diamonddagger590/mccore/testing/CallerRunsExecutor.java
Comment on lines
+164
to
+192
| @Test | ||
| @DisplayName("Given a connection that throws on close, when createTables is called, then the catch block executes and future completes exceptionally") | ||
| void createTables_completesExceptionally_whenConnectionCloseThrowsSQLException() throws Exception { | ||
| Database mockDatabase = mock(Database.class); | ||
| Connection mockConnection = mock(Connection.class); | ||
|
|
||
| when(mockDatabase.getDatabaseExecutorService()).thenReturn(executor); | ||
| when(mockDatabase.getConnection()).thenReturn(mockConnection); | ||
| doThrow(new SQLException("Close failed")).when(mockConnection).close(); | ||
|
|
||
| try (MockedStatic<TableVersionHistoryDAO> tvhMock = mockStatic(TableVersionHistoryDAO.class); | ||
| MockedStatic<MutexDAO> mutexMock = mockStatic(MutexDAO.class); | ||
| MockedStatic<PlayerSettingDAO> psMock = mockStatic(PlayerSettingDAO.class); | ||
| MockedStatic<PlayerStatisticDAO> pstMock = mockStatic(PlayerStatisticDAO.class)) { | ||
|
|
||
| tvhMock.when(() -> TableVersionHistoryDAO.attemptCreateTable(any(Connection.class), any(Database.class))).thenReturn(true); | ||
| mutexMock.when(() -> MutexDAO.attemptCreateTable(any(Connection.class), any(Database.class))).thenReturn(true); | ||
| psMock.when(() -> PlayerSettingDAO.attemptCreateTable(any(Connection.class), any(Database.class))).thenReturn(true); | ||
| pstMock.when(() -> PlayerStatisticDAO.attemptCreateTable(any(Connection.class), any(Database.class))).thenReturn(true); | ||
|
|
||
| CreateTableFunction function = CreateCoreTablesFunction.getCreateCoreTablesFunction(); | ||
|
|
||
| CompletableFuture<Void> result = function.createTables(mockDatabase); | ||
| assertNotNull(result); | ||
|
|
||
| // The future was already completed normally before close() threw, | ||
| // so completeExceptionally is a no-op — the future is done, not exceptionally | ||
| assertTrue(result.isDone()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not report a connection-close failure as successful completion.
Both tests configure Connection.close() to throw SQLException, but only assert isDone(). That assertion passes for both normal and exceptional completion. The inline comments also confirm that the current implementation completes normally before resource closure, so completeExceptionally cannot change the result.
Move normal future completion until after the try-with-resources block exits. Then assert that join() fails with SQLException as its cause.
src/test/java/com/diamonddagger590/mccore/database/table/function/CreateCoreTablesFunctionTest.java#L164-L192: Assert exceptional completion for the close failure.src/test/java/com/diamonddagger590/mccore/database/table/function/UpdateCoreTablesFunctionTest.java#L99-L122: Assert exceptional completion for the close failure.
📍 Affects 2 files
src/test/java/com/diamonddagger590/mccore/database/table/function/CreateCoreTablesFunctionTest.java#L164-L192(this comment)src/test/java/com/diamonddagger590/mccore/database/table/function/UpdateCoreTablesFunctionTest.java#L99-L122
🤖 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/database/table/function/CreateCoreTablesFunctionTest.java`
around lines 164 - 192, Move normal CompletableFuture completion until after the
try-with-resources block exits so Connection.close() failures remain
exceptional. Update
createTables_completesExceptionally_whenConnectionCloseThrowsSQLException in
src/test/java/com/diamonddagger590/mccore/database/table/function/CreateCoreTablesFunctionTest.java
lines 164-192 and the corresponding close-failure test in
src/test/java/com/diamonddagger590/mccore/database/table/function/UpdateCoreTablesFunctionTest.java
lines 99-122 to assert join() fails with SQLException as its cause, replacing
the insufficient isDone() assertion and stale comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateCoreTablesFunctionTestfrom 3 to 7 tests, achieving 100% JaCoCo coverage across all metrics (instruction, line, branch, complexity, method, class)UpdateCoreTablesFunctionTestfrom 3 to 6 tests, achieving 100% JaCoCo coverage across all metricsCallerRunsExecutortest fixture tosrc/testFixtures/java/for reuse by downstream pluginsDetails
New tests added
CreateCoreTablesFunctionTest:
attemptCreateTablereturnsfalse)attemptCreateTablereturnstrue)UpdateCoreTablesFunctionTest:
updateTablemethods called)CallerRunsExecutor test fixture
A
ThreadPoolExecutorthat runs tasks on the calling thread, ensuringmockStaticscopes remain valid during task execution. Mockito'smockStaticis thread-local, so production code that submits work to an executor won't see static mocks on a separate thread. This fixture avoids that by running submitted tasks synchronously. Extracted from both test files to eliminate duplication and make it available to downstream plugins.Test plan
./gradlew clean test)./gradlew shadowJar)Generated by Claude Code
Summary by CodeRabbit