Skip to content

FINERACT-2649: batch migrate Tier 4 loan tests to feign - #6158

Merged
adamsaghy merged 26 commits into
apache:developfrom
DeathGun44:FINERACT-2649/batch-migrate-loan-tests-to-feign-s4
Aug 17, 2026
Merged

FINERACT-2649: batch migrate Tier 4 loan tests to feign#6158
adamsaghy merged 26 commits into
apache:developfrom
DeathGun44:FINERACT-2649/batch-migrate-loan-tests-to-feign-s4

Conversation

@DeathGun44

@DeathGun44 DeathGun44 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What this PR does

fourth batch of the loan integration-test migration. It moves 54 test classes (655 test methods) off REST-assured onto the typed Feign client.

None of those 54 classes reference REST-assured any more. With this merged, 151 integration-test classes run on the typed client.

99 files changed:

  • 54 test classes (655 test methods)
  • 2 shared base classes (FeignLoanTestBase, BaseLoanIntegrationTest)
  • 34 shared Feign helpers and builders (16 of them new)

The tests assert the same things as before. This changes how they talk to the server, not what they check. The few assertions that did change are listed under "Bugs found" and each one is a fix.

Commits

26 commits, each a group of related tests (loan charges, COB, external asset owner, delinquency, advanced payment allocation, and so on).

What's covered

  • Client loan lifecycle - charges of every calculation type, credit balance refund, external IDs, backdated disbursement, overpaid status, modifying approved amounts, floating-rate interest recalculation, date validation.
  • Close of Business (COB) - partitioning, catch-up, inline COB, account locks.
  • External asset owner - transfer, cancel, search, initiate/undisbursed.
  • Advanced Payment Allocation - repayment schedules, interest refunds, charges, reprocessing.
  • Charges - specific due date, after maturity, instalment fees, penalties, charge-off accounting.
  • Delinquency - buckets, business events, classification.
  • Batch API, scheduler jobs, journal reversal, SQL-injection reporting.

Bugs found

The typed client is stricter than REST-assured, so a few real problems showed up:

  • Wrong charge calculation type. ChargeRequestBuilders had CHARGE_CALCULATION_TYPE_PERCENTAGE_AMOUNT_AND_INTEREST = 4, but 4 is percent-of-interest. Percent-of-amount-and-interest is 3, which is what ChargesHelper uses and what the tests meant. Every test using that builder was charging on the wrong base and still passing, because the amounts were plausible. Now uses ChargeCalculationType.PERCENT_OF_AMOUNT_AND_INTEREST.
  • A comparison that could never be true. In testLoanCharges_INSTALMENT_FEE the instalment numbers are Long (the request model requires it) but the schedule reports its period as an Integer. Long.equals(Integer) is always false, so the "this is the waived instalment" and "this is the paid instalment" branches never ran and the test compared against the wrong expected values. Now compared numerically.
  • A crash hidden in a helper. modifyLoanApplication passed command through Map.of(...), which rejects nulls. Three charge tests in ClientLoanIntegrationTest pass null here and were throwing NullPointerException before reaching the server. Now uses the typed query parameter, which drops a null.
  • A dropped product setting. LoanProductTestBuilder.withDisallowExpectedDisbursements(true) also quietly turned on a 100% over-applied disbursement allowance. The migrated builder set it to false, which broke chargeOff because it disburses two 1000 tranches against a 1000 approval. Restored.
  • A COB step missing its prerequisite. CobPartitioningTest registered the external-asset-owner COB step but had lost the ASSET_TRANSFER GL mapping that step needs. The step list is tenant-wide and never restored, so this broke an unrelated later test.
  • Weak assertions. The SQL-injection tests caught bare RuntimeException as "the server rejected this", so a client-side connection failure counted as a pass. They also looked for "404" in the exception message, which includes the request path, so a report whose ID contained 404 hid a real failure. Both now check the HTTP status.
  • Interest calculation changed. One charges test had moved from SAME_AS_REPAYMENT_PERIOD to DAILY. Reverted.
  • Locale changed. Account transfers were posted with en instead of en_GB. Restored.
  • A reversal check that never fired. verifyTransactions compared manuallyReversed with Objects.equals(item.getManuallyReversed(), tr.reversed), but the API omits that field entirely for non-reversed transactions, so the comparison was Objects.equals(null, false) - always false, making the whole predicate unreachable. The check now treats null and false as the same state, and per review feedback it exists in both validators: LoanTestValidators (Feign) and BaseLoanIntegrationTest (REST-assured), where there was previously no check at all.

Shared test infrastructure

  • LoanChargeCommandsApi and ClientChargeCommandsApi (new). Two hand-written Feign nterfaces, bound to the generated response models, for the three request shapes the generated request models cannot express: a bodyless ?command=waive POST (the generated client always serialises at least {}, which that endpoint answers with a 500), a locale-formatted decimal amount ("50,05"), and a client-charge amount with decimal places. No JSON is built by hand. Retyping the shared models instead was rejected: PostLoansLoanIdChargesRequest.amount has 118 call sites passing a number, and PostClientsClientIdChargesRequest.amount is an Integer whose correction is a breaking OpenAPI change (swagger-brake R010) that belongs in its own PR.
  • LoanRequestBuilders - added a type-safe paymentAllocationOrder(PaymentAllocationType...) overload, replacing copies of the same private helper in individual tests.
  • ChargeRequestBuilders - dropped its three local CHARGE_CALCULATION_TYPE_* constants. All call sites now use the ChargeCalculationType enum, so the name and the value can't drift apart again.

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.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

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

@DeathGun44 DeathGun44 changed the title Fineract-2649: batch migrate Tier 4 loan tests to feign FINERACT-2649: batch migrate Tier 4 loan tests to feign Jul 21, 2026
budaidev
budaidev previously approved these changes Jul 24, 2026

@budaidev budaidev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any larger issues, it looks good.

@Aman-Mittal
Aman-Mittal requested a review from adamsaghy July 24, 2026 17:19
@DeathGun44

Copy link
Copy Markdown
Contributor Author

Hi @adamsaghy can you please take a look at this one ,when you have a moment!

@DeathGun44
DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s4 branch from 434c2be to 882af35 Compare August 5, 2026 06:33
@DeathGun44

DeathGun44 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Rebased ,also removed feignhttp calls as per the reviews on #6194 and updated pr description reflecting the current state

@DeathGun44
DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s4 branch from 882af35 to c6ea6e7 Compare August 5, 2026 10:51
@DeathGun44
DeathGun44 marked this pull request as draft August 5, 2026 13:09
@DeathGun44
DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s4 branch from c6ea6e7 to 85d66cf Compare August 5, 2026 14:00
@DeathGun44
DeathGun44 marked this pull request as ready for review August 5, 2026 14:45
@DeathGun44

Copy link
Copy Markdown
Contributor Author

test seems flaky, should pass on rerun

@DeathGun44

Copy link
Copy Markdown
Contributor Author

@adamsaghy please take a look when you have a moment!

@budaidev

Copy link
Copy Markdown
Contributor

please resolve the conflict

@adamsaghy

Copy link
Copy Markdown
Contributor

@DeathGun44 Sorry for being slow. Let me review it now

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeathGun44 Ultimately it looks good. Great job (huge PR..).

Can you please review my concerns and advise on them?

@DeathGun44
DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s4 branch 2 times, most recently from 2981ae2 to 290cace Compare August 14, 2026 07:49
@DeathGun44
DeathGun44 marked this pull request as draft August 14, 2026 07:50
@DeathGun44
DeathGun44 marked this pull request as ready for review August 14, 2026 10:31
@DeathGun44
DeathGun44 requested a review from adamsaghy August 14, 2026 10:38
@DeathGun44

DeathGun44 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@adamsaghy please take a look when you have a moment!

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

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

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

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

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

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

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

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

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

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

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Move the two remaining BaseLoanIntegrationTest subclasses
(LoanChargeRoundingTest, ClientChargeRoundingTest) onto FeignLoanTestBase
and the typed Feign client, completing the loan integration-test migration.

Loan charge assertions use the typed client throughout; the rounded-to-zero
cases go through FeignLoanHelper.addLoanChargeExpectingError and assert the
HTTP status and globalisation code instead of substring-matching the message.

The client-charge amount field is an integer in the generated model, so the
decimal amounts these rounding tests post go through FeignRawHttpHelper (the
sanctioned non-RestAssured raw HTTP path) rather than retyping the API model;
reads and the 400 rounded-to-zero assertion use the typed client and the raw
HTTP status respectively. Test-only change.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
DeathGun44 force-pushed the FINERACT-2649/batch-migrate-loan-tests-to-feign-s4 branch from e450bc9 to ab25b41 Compare August 17, 2026 19:40
@DeathGun44

Copy link
Copy Markdown
Contributor Author

@adamsaghy Done!

@DeathGun44

Copy link
Copy Markdown
Contributor Author

the failed test seems flaky

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adamsaghy
adamsaghy merged commit e62a735 into apache:develop Aug 17, 2026
177 of 180 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants