FINERACT-2649: Migrate Tier 3 loan integration tests to Feign client#6084
FINERACT-2649: Migrate Tier 3 loan integration tests to Feign client#6084DeathGun44 wants to merge 14 commits into
Conversation
|
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 |
9276e89 to
1d32a0e
Compare
Aman-Mittal
left a comment
There was a problem hiding this comment.
I am still reviewing this one.....
1d32a0e to
ba3e948
Compare
ba3e948 to
07c5b62
Compare
|
@DeathGun44 Please rebase. |
07c5b62 to
94261bf
Compare
|
@adamsaghy Done! |
budaidev
left a comment
There was a problem hiding this comment.
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.
6f8e2c3 to
e9acca1
Compare
|
@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
left a comment
There was a problem hiding this comment.
looks good, great progress
|
please rebase and handle the conflicts |
e9acca1 to
13dc9db
Compare
|
@adamsaghy @budaidev Done! |
95843e5 to
593eded
Compare
|
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. |
|
the other test failure is just flaky ,should pass on rerun. |
|
@adamsaghy please take a look when you have a moment! |
|
I am postponing to merge this till release of 1.15.0 is not finished |
…elpers 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>
…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>
593eded to
3514432
Compare
|
The test is just flaky, a rerun should pass it. |
nope. they seem real failures |
…s, simplify runAt Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
3514432 to
779a812
Compare
You're right my mistake, it wasn't flaky. When I moved these tests onto the new FeignLoanTestBase, I accidentally left off the ExternalEventsExtension that BaseLoanIntegrationTest uses. That extension is what resets the external-event configuration after each test. I fixed it now! |
|
This one seems flaky ,should pass on rerun |
|
updated the pr description as per the current state! |
Description
This PR transitions 47 test classes (encompassing approximately 275 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 , APA, down-payments, etc.): two commits extending the shared Feignhelpers, eleven migration batches, plus a dedicated review-feedback refinement commit.
This ensures 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:
down-payment installment allocation, repayment schedules).
installment-fee error handling.
recalculation, repayment calculation, and date validations).
BigDecimalprecision assertions across migrationReview Feedback & OpenAPI Refinements
Based on maintainer review feedback, the production-code footprint of this PR has been reduced to 11 purely additive schema-documentation lines across 2 Swagger files - no behavioral changes:
LoanTransactionsApiResourceSwagger: The earlier@ApiResponstionTemplatehas been dropped. Instead, the fields the endpointalready returns for reAge/reAmortize templates (numberOfPastInstallments,numberOfFutureInstallments,nextInstallmentDueDate,calculatedStartDate) are now documented on the existing response schema. This lets the standard glize these templates directly — the interim customInternalLoanReAgeApiworkaround interface has been removed.SavingsProductsApiResourceSwagger: Documented the existing `sponse field.HashMap<String, Object>test builder responses with strongly-typed generated models (PostLoansResponse,PostLoansLoanIdResponse).LocalDate.of(...)calls to use concise numeric month parameters, matching the pre-migration originals.Bug Fix:
pre-existing bug on
developin two test methods: - `testUndoDisbursalForLoanWithSingleDisbursalAutoDownPaymentEnabledtestUndoDisbursalForLoanWithMultiDisbursalAutoDownPaymentEnabledAndHasManualTransactionsBoth 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 (tThe old REST-assured based
verifyJournalEntries()had more lenientmatching semantics that masked this error. Fixed as part of the migration commit.Infrastructure Enhancements
This PR builds upon the shared Feign test infrastructure from earlie
FeignGroupCenterHelper(new): Utilities for managing group a loan testing.FeignRawHttpHelper(extended): Safely handles edge cases that require raw HTTP requests to bypass strict Swagger model limitations (e.g., floating rateproperties), utilizing
fineract.it.urlsecurely.Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.