FINERACT-2649: Migrate Tier 3 loan integration tests to Feign client by DeathGun44 · Pull Request #6084 · apache/fineract

DeathGun44 · 2026-07-03T12:06:31Z

Description

This PR transitions 56 test classes (encompassing approximately 277 test methods) to utilize the Feign client, entirely removing their dependency on REST-assured.

Note on Behavior: The core test behavior and assertions remain completely unchanged. This is strictly a refactor of the underlying HTTP client layer.

Targeted commits

I've split this work into 14 commits, grouped logically by test area (reschedule, chargebacks, APA, down-payments, etc.) plus a dedicated review-feedback refinement commit to keep things manageable for review.

This will ensure that if CI fails or issues arise later, it is much easier to isolate and track down the specific batch that introduced the regression.

Scope

Included in this PR:

Comprehensive Loan Rescheduling and Re-aging tests (including Center/Group reschedules).
Chargeback integration scenarios (non-APA workflows, overpaid loans, payment type constraints).
Down-payment workflows (product configuration, transaction typing, undo disbursal, and repayment schedules).
Advanced Payment Allocation (APA) coverage for write-offs, transaction reprocessing, and external-id support.
Client loan lifecycle tests (credit balance refund, multiple disbursements, floating rates, and date validations).
Reverse-replay transaction relations and loan summary tests.
Standardization of BigDecimal precision assertions across migrated Feign tests.

Review Feedback & OpenAPI Refinements

Based on maintainer review feedback, this PR also includes cleanups to improve codebase hygiene and client generation:

OpenAPI Schema Fix (retrieveTransactionTemplate): Corrected @ApiResponse on retrieveTransactionTemplate to declare LoanTransactionData.class instead of GetLoansLoanIdTransactionsTemplateResponse.class. This aligns the OpenAPI definition with actual wire runtime behavior and enables standard generated Feign clients to deserialize transaction templates without custom workarounds.
Strongly Typed Response Models: Replaced untyped HashMap<String, Object> test builder responses with strongly-typed generated models (PostLoansResponse, PostLoansLoanIdResponse).
Simplified Business Date Context: Streamlined FeignBusinessDateHelper.runAt(...) by removing unnecessary nested ThreadLocal depth tracking.
Date Readability: Standardized LocalDate.of(...) calls to use concise numeric month parameters.

Bug Fix: Incorrect journal entry assertions in `UndoLoanDisbursalWithDownPaymentIntegrationTest`

During the migration, the stricter Feign verifyJournalEntries() (which performs exact 1-to-1 matching with count validation) exposed a pre-existing bug on develop in two test methods:

testUndoDisbursalForLoanWithSingleDisbursalAutoDownPaymentEnabledAndHasManualTransactions
testUndoDisbursalForLoanWithMultiDisbursalAutoDownPaymentEnabledAndHasManualTransactions

Both tests make a manual repayment of 100.0 after disbursement, then undo the disbursal. The "repayment entries compensated" journal entry assertions incorrectly expected 250.0 (the down-payment amount) instead of 100.0 (the actual repayment amount). The old REST-assured based verifyJournalEntries() had more lenient matching semantics that masked this error. Fixed as part of the migration commit.

Infrastructure Enhancements

This PR builds upon the shared Feign test infrastructure from earlier batches, adding and extending several utilities to streamline testing:

FeignGroupCenterHelper: Provides utilities for managing group and center associations during loan testing.
FeignRawHttpHelper: Safely handles edge cases that require raw HTTP requests to bypass strict Swagger model limitations (e.g., floating rate properties), utilizing fineract.it.url securely.
Business Date Context (runAt): Cleanly executes operations under simulated business dates with guaranteed cleanup.
Event Verification: Integrated Awaitility-based assertions for robust verification of asynchronous business events.
Shared Builders & Validators: Expanded Feign loan helpers for external-ids, re-amortizations, write-offs, and credit allocations.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per our guidelines
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow our coding conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

DeathGun44 · 2026-07-03T15:06:41Z

the test is failing and the issue is in develop itself, because a reversal of a 100.0 repayment should produce 100.0 journal entries, not 250.0. i am adding a fix for it in the pr and describing it in the description aswell

Aman-Mittal

I am still reviewing this one.....

adamsaghy · 2026-07-06T09:27:55Z

@DeathGun44 Please rebase.

DeathGun44 · 2026-07-06T09:45:45Z

@adamsaghy Done!

budaidev

The PR has a massive scope and a lot of changes, so the lot of comments doesn't mean it's bad, but I would advise to fix those to keep the refactor clean. The direction is really good.

Also please avoid any behavior changes in the test, by modifying or removing assertations, because it could lead silent errors.

DeathGun44 · 2026-07-07T11:08:14Z

@budaidev Thank you for the thorough review! I enjoy all these reviews as they help me learn a lot. I know there is a lot of code to go through. I will keep trying my best to make it as clean as possible and easier to review, and I'll make sure the quality of the code doesn't deteriorate with these bulk prs. Thanks again, everyone, for keeping up!

budaidev

looks good, great progress

budaidev · 2026-07-07T13:04:39Z

please rebase and handle the conflicts

…elpers Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

… Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

…sts to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

…to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

… dates Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

… Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

… and down-payment schedule tests to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

…ests to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

…d Feign tests Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

DeathGun44 · 2026-07-07T18:54:38Z

@adamsaghy @budaidev Done!

adamsaghy · 2026-07-08T09:42:30Z

    public void testTransactionComparator() {
        final Loan loan = new Loan();
-        LocalDate today = LocalDate.of(2023, 10, 12);
+        LocalDate today = LocalDate.of(2023, Month.OCTOBER, 12);


this does not really help readability...

Fair point! reverted all Month.ENUM back to integer months across the entire PR.

adamsaghy · 2026-07-08T09:42:32Z

        loanCharge.setChargeTime(ChargeTimeType.TRANCHE_DISBURSEMENT.getValue());
        loanCharge.setChargeCalculation(ChargeCalculationType.FLAT.getValue());
-        loanCharge.setDueDate(LocalDate.of(2022, 6, 27));
+        loanCharge.setDueDate(LocalDate.of(2022, Month.JUNE, 27));


this does not really help readability...

adamsaghy · 2026-07-08T09:43:02Z

@@ -61,12 +62,19 @@ public void runAt(String date, Runnable action) {
    }

    public void runAt(String date, String dateFormat, Runnable action) {
+        boolean outerScope = RUN_AT_NESTING_DEPTH.get() == 0;


What is this for? We should not have nested business date changes...

Agreed and removed the ThreadLocal nesting depth tracking entirely. No test actually nests runAt calls, so the complexity was unjustified.

adamsaghy · 2026-07-08T09:47:26Z

+
+/**
+ * Feign interface for the reAge transaction template endpoint. The OpenAPI-generated client deserializes this response
+ * as {@code GetLoansLoanIdTransactionsTemplateResponse}, but the server returns {@link LoanTransactionData}.


The swagger definition should be fixed then instead please

Fixed the @apiresponse on retrieveTransactionTemplate to use LoanTransactionData instead of GetLoansLoanIdTransactionsTemplateResponse. Deleted the InternalLoanReAgeApi workaround and switched to the standard generated client.

adamsaghy · 2026-07-08T09:50:32Z

+    protected HashMap<String, Object> getLoanIdFromApplication(String loanApplicationJson) {
+        Long loanId = applyForLoanFromJson(loanApplicationJson);
+        HashMap<String, Object> result = new HashMap<>();
+        result.put("resourceId", loanId.intValue());
+        result.put("resourceExternalId", getLoanDetails(loanId).getExternalId());
+        return result;
+    }
+
+    protected HashMap<String, Object> disburseLoanAsMap(String date, Integer loanId, String transactionAmount, String externalId) {
+        PostLoansLoanIdResponse response = loanHelper.disburseLoanWithExternalId(date, loanId.longValue(), transactionAmount, externalId);
+        HashMap<String, Object> result = new HashMap<>();
+        result.put("subResourceExternalId", response.getSubResourceExternalId());
+        return result;
+    }
+
+    protected HashMap<String, Object> disburseLoanAsMap(String date, Integer loanId, String transactionAmount) {
+        PostLoansLoanIdResponse response = loanHelper.disburseLoan(date, loanId.longValue(), transactionAmount);
+        HashMap<String, Object> result = new HashMap<>();
+        result.put("subResourceExternalId", response.getSubResourceExternalId());
+        return result;
+    }
+
+    protected HashMap<String, Object> disburseLoan(String date, Integer loanId, String transactionAmount, String externalId) {
+        return disburseLoanAsMap(date, loanId, transactionAmount, externalId);
+    }
+
+    protected HashMap<String, Object> disburseLoan(String date, Integer loanId, String transactionAmount) {
+        return disburseLoanAsMap(date, loanId, transactionAmount);
+    }


Build and returning hashmap seems incorrect. why dont we use directly the response object?

Replaced all HashMap<String, Object> returns with PostLoansResponse / PostLoansLoanIdResponse. Removed disburseLoanAsMap entirely.

adamsaghy · 2026-07-08T09:51:19Z

@@ -275,7 +270,7 @@ public void test_LoanReAgeTransaction_Works() {
                    installment(316.67, 0, 0, 0, 316.67, false, "13 May 2023"), //
                    installment(316.66, 0, 0, 0, 316.66, false, "12 June 2023") //
            );
-            checkMaturityDates(loanId, LocalDate.of(2023, 6, 12), LocalDate.of(2023, 6, 12));
+            checkMaturityDates(loanId, LocalDate.of(2023, Month.JUNE, 12), LocalDate.of(2023, Month.JUNE, 12));


It does not help the readability...

adamsaghy

Please review my concerns about:

readability issues: Using Month.JUNE, etc looks weird... what was the problem with "6"?
Creating HashMap objects from the response object: use the response object, dont map into HashMap...
Minimalize the code logic which build manually HashMap / Json object and use that as request (i am not saying it cannot be used, but we should ensure, we are doing only when there are no other possibilities).
Nested runAt: We should not have nested runAt. Is there a particular reason to allow it?

…s, simplify runAt

DeathGun44 · 2026-07-08T13:06:51Z

Regarding the OpenAPI backward-compatibility check failure: this is expected due to updating the @apiresponse schema on retrieveTransactionTemplate from GetLoansLoanIdTransactionsTemplateResponse to LoanTransactionData.

Inspecting LoanTransactionsApiResource.java shows the endpoint uses DefaultToApiJsonSerializer and has always returned LoanTransactionData on the wire at runtime. The previous OpenAPI annotation was incorrect. This change aligns the Swagger spec with actual runtime behavior, so there is no real-world breaking change for API consumers.

DeathGun44 · 2026-07-08T13:24:14Z

the other test failure is just flaky ,should pass on rerun.
I have also updated the pr description with regard to the current state

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from 9276e89 to 1d32a0e Compare July 3, 2026 15:11

adamsaghy reviewed Jul 3, 2026

View reviewed changes

Comment thread fineract-provider/src/test/java/org/apache/fineract/batch/command/CommandStrategyUtilsTest.java Outdated

Aman-Mittal reviewed Jul 3, 2026

View reviewed changes

Aman-Mittal requested a review from budaidev July 3, 2026 15:56

Aman-Mittal reviewed Jul 3, 2026

View reviewed changes

Comment thread ...ion-tests/src/test/java/org/apache/fineract/integrationtests/SmsCampaignIntegrationTest.java

Aman-Mittal added the java Pull requests that update Java code label Jul 3, 2026

Aman-Mittal reviewed Jul 4, 2026

View reviewed changes

Comment thread ...tests/src/test/java/org/apache/fineract/integrationtests/client/feign/FeignLoanTestBase.java Outdated

Aman-Mittal reviewed Jul 4, 2026

View reviewed changes

Comment thread ...tests/src/test/java/org/apache/fineract/integrationtests/client/feign/FeignLoanTestBase.java Outdated

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from 1d32a0e to ba3e948 Compare July 5, 2026 19:29

budaidev requested changes Jul 5, 2026

View reviewed changes

Comment thread ...src/test/java/org/apache/fineract/integrationtests/client/feign/helpers/FeignLoanHelper.java

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from ba3e948 to 07c5b62 Compare July 5, 2026 22:17

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from 07c5b62 to 94261bf Compare July 6, 2026 09:45

adamsaghy requested a review from budaidev July 6, 2026 13:43

budaidev requested changes Jul 6, 2026

View reviewed changes

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch 2 times, most recently from 6f8e2c3 to e9acca1 Compare July 7, 2026 10:56

adamsaghy requested a review from budaidev July 7, 2026 12:54

budaidev approved these changes Jul 7, 2026

View reviewed changes

DeathGun44 added 6 commits July 7, 2026 18:56

FINERACT-2649: Add reAmortize, writeOff and credit allocation Feign h…

600fd21

…elpers Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate reschedule core integration tests to Feign

568d5c9

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate reAmortize and center reschedule tests to Feign

ada6ceb

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate non-APA chargeback integration tests to Feign

42d9b74

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate down-payment product and loan summary tests to…

4be9d63

… Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate down-payment transaction and undo disbursal te…

c475bf5

…sts to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

DeathGun44 added 7 commits July 7, 2026 18:57

FINERACT-2649: Migrate APA write-off and transaction reprocess tests …

1d60e2f

…to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Extend Feign loan helpers for external-id and business…

2059749

… dates Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate APA, external-id and transaction loan tests to…

0a3a938

… Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate client loan credit balance, APA charge payment…

71dceb5

… and down-payment schedule tests to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate charge, disbursement and reverse-replay loan t…

95e272d

…ests to Feign Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Standardize BigDecimal precision assertions in migrate…

57db333

…d Feign tests Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

FINERACT-2649: Migrate core LoanAccount tests to Feign

13dc9db

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from e9acca1 to 13dc9db Compare July 7, 2026 18:51

budaidev requested a review from adamsaghy July 7, 2026 19:00

adamsaghy reviewed Jul 8, 2026

View reviewed changes

adamsaghy requested changes Jul 8, 2026

View reviewed changes

FINERACT-2649: revert Month enum, fix swagger, remove HashMap wrapper…

593eded

…s, simplify runAt

DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s3 branch from 95843e5 to 593eded Compare July 8, 2026 12:36

DeathGun44 requested a review from adamsaghy July 8, 2026 13:24

Uh oh!

Conversation

DeathGun44 commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Targeted commits

Scope

Review Feedback & OpenAPI Refinements

Bug Fix: Incorrect journal entry assertions in UndoLoanDisbursalWithDownPaymentIntegrationTest

Infrastructure Enhancements

Checklist

Uh oh!

DeathGun44 commented Jul 3, 2026

Uh oh!

Uh oh!

Aman-Mittal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsaghy commented Jul 6, 2026

Uh oh!

DeathGun44 commented Jul 6, 2026

Uh oh!

budaidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeathGun44 commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

budaidev left a comment

Choose a reason for hiding this comment

Uh oh!

budaidev commented Jul 7, 2026

Uh oh!

DeathGun44 commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeathGun44 commented Jul 3, 2026 •

edited

Loading

Bug Fix: Incorrect journal entry assertions in `UndoLoanDisbursalWithDownPaymentIntegrationTest`

DeathGun44 commented Jul 7, 2026 •

edited

Loading

adamsaghy Jul 8, 2026 •

edited

Loading

adamsaghy left a comment •

edited

Loading