[Tests] Improve edge-case coverage for FailSafeTransaction, NoLocalizationContainsMessageException, ReloadableTask

[DiamondDagger590]

Summary

• FailSafeTransaction: Added 3 tests covering commit() failure triggering rollback, failureCause wrapping the original exception message, and state remaining COMMITTED when autocommit-reset fails in the finally block
• NoLocalizationContainsMessageException: Added 2 tests covering getMessage() with an empty locale set (exercises the Optional.empty branch of stream reduce) and comma-separated formatting with multiple locales
• ReloadableTask: Added 2 tests verifying that runTask is called during construction and documenting that the async flag defaults to false during construction due to super() calling reloadContent() before field assignment

Test plan

• All existing tests pass (./gradlew test — BUILD SUCCESSFUL)
• New tests verified against production source code behavior
• Testing audit persona (review-testing) run against changes — one dead variable flagged and fixed
• No production code changes

---

Generated by Claude Code

Summary by CodeRabbit

• Tests
  • Expanded coverage for task initialization, including asynchronous configurations.
  • Added transaction failure scenarios to verify error preservation and correct committed-state handling.
  • Added localization coverage for empty and multiple locale configurations, including route details and localized names.

[github-actions]

Claude finished @DiamondDagger590's task in 2m 35s —— View job

---

Reviewing PR #92

Verdict: No blocking issues found

This PR only adds new test methods to three existing test files (ReloadableTaskTest, FailSafeTransactionTest, NoLocalizationContainsMessageExceptionTest) — no production code changed. Since the diff doesn't touch src/main/**, the security/architecture/extensibility/error-handling/performance lenses don't apply; review-testing was run and verified each new assertion against the actual production source behavior (field-init-order quirk in ReloadableTask, rollback/commit/failure-cause semantics in FailSafeTransaction, and message formatting in NoLocalizationContainsMessageException). All new tests are correct, follow the @DisplayName/naming convention, and no dead code or missing critical edge cases were found.

• Gather context (PR diff, changed files)
• Route to review lenses — test-only change; only review-testing applies
• Fan out to persona subagents
• Consolidate findings
• Post final review

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Task Initialization

Layer / File(s)	Summary
Initial task execution tests src/test/java/com/diamonddagger590/mccore/configuration/task/ReloadableTaskTest.java	Tests verify synchronous initial execution and runTask(false) during construction with async=true.

Fail-Safe Transactions

Layer / File(s)	Summary
Commit failure and recovery tests src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java	Tests verify rollback, failure-cause preservation, suppressed commit failures, and COMMITTED state retention after autocommit reset failure.

Localized Exception Messages

Layer / File(s)	Summary
Locale message formatting tests src/test/java/com/diamonddagger590/mccore/exception/localization/NoLocalizationContainsMessageExceptionTest.java	Tests verify route inclusion, Optional.empty, and English and French locale names in exception messages.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• DiamondDagger590/McCore#90: Both PRs update NoLocalizationContainsMessageExceptionTest.java for multi-locale message formatting.

🚥 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 concisely identifies the test coverage added for all three components named in the changes.
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-67rxqo

---

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 (1)

src/test/java/com/diamonddagger590/mccore/exception/localization/NoLocalizationContainsMessageExceptionTest.java-78-88 (1)

78-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the comma separator explicitly.

The test name states that locale names are comma-separated, but the assertions only check that both names appear. An implementation that joins them with spaces would pass. Assert either locale order with a comma because Set.of(...) does not guarantee iteration order.

Proposed test assertion

         String message = ex.getMessage();
         assertNotNull(message);
-        assertTrue(message.contains(Locale.ENGLISH.getDisplayName()));
-        assertTrue(message.contains(Locale.FRENCH.getDisplayName()));
+        String english = Locale.ENGLISH.getDisplayName();
+        String french = Locale.FRENCH.getDisplayName();
+        assertTrue(message.contains(english + "," + french)
+                || message.contains(french + "," + english));

🤖 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/exception/localization/NoLocalizationContainsMessageExceptionTest.java`
around lines 78 - 88, Update
getMessage_containsCommaSeparatedLocaleNames_whenConstructedWithMultipleLocales
to assert that the two locale display names occur with a comma between them,
accepting either order because Set.of does not guarantee iteration order.

🧹 Nitpick comments (1)

src/test/java/com/diamonddagger590/mccore/configuration/task/ReloadableTaskTest.java (1)

127-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the new test identifiers with project conventions.

Rename constructor_callsRunTaskOnInitialTask to include its condition, for example constructor_callsRunTaskOnInitialTask_whenFirstConstructed. Rename doc and r to descriptive callback parameters; r is a single-letter name outside a loop.

As per coding guidelines, use meaningful variable names and avoid single-letter names except for loop counters. As per path instructions, test methods under src/test/**/*.java use methodUnderTest_expectedOutcome_whenCondition.

Also applies to: 140-140

🤖 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/configuration/task/ReloadableTaskTest.java`
around lines 127 - 129, Rename the test method
constructor_callsRunTaskOnInitialTask to follow the
methodUnderTest_expectedOutcome_whenCondition convention, such as
constructor_callsRunTaskOnInitialTask_whenFirstConstructed. In the BiFunction
callback, replace the abbreviated parameters doc and r with descriptive names
that identify the YAML document and route.

Sources: Coding guidelines, Path instructions

🤖 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/exception/localization/NoLocalizationContainsMessageExceptionTest.java`:
- Around line 78-88: Update
getMessage_containsCommaSeparatedLocaleNames_whenConstructedWithMultipleLocales
to assert that the two locale display names occur with a comma between them,
accepting either order because Set.of does not guarantee iteration order.

---

Nitpick comments:
In
`@src/test/java/com/diamonddagger590/mccore/configuration/task/ReloadableTaskTest.java`:
- Around line 127-129: Rename the test method
constructor_callsRunTaskOnInitialTask to follow the
methodUnderTest_expectedOutcome_whenCondition convention, such as
constructor_callsRunTaskOnInitialTask_whenFirstConstructed. In the BiFunction
callback, replace the abbreviated parameters doc and r with descriptive names
that identify the YAML document and route.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 5d98ee36-9f82-44a4-8587-a7c28f576a9c

📥 Commits

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

📒 Files selected for processing (3)

• src/test/java/com/diamonddagger590/mccore/configuration/task/ReloadableTaskTest.java
• src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java
• src/test/java/com/diamonddagger590/mccore/exception/localization/NoLocalizationContainsMessageExceptionTest.java