FINERACT-2533: Add comprehensive unit tests for SavingsAccountDomainS…

[awaneetdecoder]

Currently, SavingsAccountDomainServiceJpa lacks comprehensive unit tests for its core domain logic. This PR adds a JUnit 5 test suite covering 15 scenarios, including critical edge cases such as:

1. Insufficient funds during withdrawal.

2. Transactions on inactive/blocked accounts.

3. Interest recalculation for backdated transactions.

4. Hold/Lien amount transaction processing.

[adamsaghy]

@awaneetdecoder Can you help me understand these mocked situations how help the test coverage?

[awaneetdecoder]

Hi @adamsaghy, thank you for the feedback.
You are absolutely right — the current tests focus more on behavioral verification (ensuring the service orchestrates calls to repositories, recalculates interest on backdated entries, and triggers journal entries) rather than state-based verification of the SavingsAccount balance logic itself.
I used mock(SavingsAccount.class) to isolate SavingsAccountDomainServiceJpa from the complex internal state of the JPA entity, which is difficult to instantiate correctly in a pure unit test without a persistence context.
My proposal to improve this:

Keep the tests that verify real orchestration — backdated interest recalculation, journal entries, reversal repository save.
Remove or refactor the weak tests that only verify mocked exception throwing.
If the project prefers integration tests for this service to exercise full JPA/entity logic, please point me to a preferred base class (like AbstractIntegrationTest) and I will rewrite accordingly.

Which approach aligns best with the project's testing standards for this module?

[adamsaghy]

Hi @adamsaghy, thank you for the feedback. You are absolutely right — the current tests focus more on behavioral verification (ensuring the service orchestrates calls to repositories, recalculates interest on backdated entries, and triggers journal entries) rather than state-based verification of the SavingsAccount balance logic itself. I used mock(SavingsAccount.class) to isolate SavingsAccountDomainServiceJpa from the complex internal state of the JPA entity, which is difficult to instantiate correctly in a pure unit test without a persistence context. My proposal to improve this:

Keep the tests that verify real orchestration — backdated interest recalculation, journal entries, reversal repository save. Remove or refactor the weak tests that only verify mocked exception throwing. If the project prefers integration tests for this service to exercise full JPA/entity logic, please point me to a preferred base class (like AbstractIntegrationTest) and I will rewrite accordingly.

Which approach aligns best with the project's testing standards for this module?

In my opinion probably integration tests or event better E2E test coverage would be the best.

E2E testing is doing no mocking at all, but rather tests scenarios like: customer opens a savings account, deposit some amount, withdraw later, etc.

Alongside it checks whether these steps were executed and the result is correct and additional scenarios can cover what happens if user tries to withdraw more than available, etc.

If you are interested in writing E2E tests for savings account related actions, you can take a look at fineract-e2e-tests-runner/src/test/resources/features/SavingsAccount.feature. The logic and test cases can be found under fineract-e2e-tests-runner and fineract-e2e-tests-core modules.

[awaneetdecoder]

Hi @adamsaghy, thank you for the clear guidance!
This makes perfect sense. I will study fineract-e2e-tests-runner/src/test/resources/features/SavingsAccount.feature and the related modules to understand the existing patterns.
I will close this PR and open a new one with proper E2E test scenarios covering:

• Insufficient funds during withdrawal
• Transactions on blocked/inactive accounts
• Backdated transaction interest recalculation

Thank you for pointing me in the right direction!

[awaneetdecoder]

Hi @adamsaghy, thank you for the clear guidance!

As suggested, I have closed this PR and opened a new one with proper
E2E test scenarios instead of unit tests.

The new PR covers:

• Insufficient funds during withdrawal (C2441)

New PR: #5646

The implementation follows the existing patterns in SavingsAccountStepDef.java:

• Uses try/catch FeignException (not mocks)
• Dynamic currency parameter (no hardcoding)
• Assertions.fail() to prevent false positives
• HTTP 403 confirmed from AbstractPlatformDomainRuleException
• Error code confirmed from InsufficientAccountBalanceException.java

Thank you for pointing me in the right direction!