FINERACT-2732: migrate savings deposit tests to feign (tier2) - #6419
Open
DeathGun44 wants to merge 15 commits into
Open
FINERACT-2732: migrate savings deposit tests to feign (tier2)#6419DeathGun44 wants to merge 15 commits into
DeathGun44 wants to merge 15 commits into
Conversation
The fixed and recurring deposit request models covered a fraction of their bodies: PostFixedDepositAccountsRequest held 8 of the 28 fields the account create sends, both PostFixedDepositAccountsAccountIdRequest and its recurring counterpart were empty, and the product requests were missing the deposit amount, the lock-in and multiples-of terms, withholding tax and every accounting mapping id. The chart slabs had no amount range. All of that is filled in from the bodies the legacy helpers build, so the Tier 2 tests can send them through the typed client instead of raw JSON. The update DTOs get the same field set as create, because the deposit account update reuses the create body, and their changes responses get submittedOnDate, which the update tests assert on. Savings gains the fields its own helpers always send: the product's lock-in period, minimum balances, lien limits, tax group and dormancy day counts; the application's withdrawalFeeForTransfers, and the group id and external id an update carries; and on the account command body, the note every transition sends, reasonForBlock for the three block commands and postInterestValidationOnClosure for a close. Two corrections rather than additions. The recurring deposit read declared recurringDepositAmount, recurringDepositFrequency and recurringDepositFrequencyType; RecurringDepositAccountData serialises mandatoryRecommendedDepositAmount, recurringFrequency and recurringFrequencyType, so the spec named three fields the server never sends. An interest rate chart slab was missing toPeriod altogether and typed its annual rate as an integer, which cannot carry the 4.5 the tests chart. And money declared as Float or Long is retyped to BigDecimal, which is what the domain classes hold and what the command parser reads - at Float precision a maturity amount is only accurate to about seven digits. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Six helpers and three modules, no test migrated yet. FeignFixedDepositHelper and FeignRecurringDepositHelper cover the account lifecycle the two deposit tests drive - submit, update, approve, undo approval, reject, withdraw, activate, calculate and post interest, calculate the premature amount and prematurely close - plus the transaction reads and the undo and modify commands. FeignFixedDepositProductHelper and its recurring counterpart create and read products. Two helpers exist because the deposit tests reach outside their own domain. FeignFinancialActivityAccountHelper maps a financial activity to a GL account, which is how a premature close transfers to savings; FeignInterestRateChartHelper reads a product's chart, which is where the tests get the rate they project against. DepositRequestBuilders reproduces the bodies the RestAssured builders send, and that includes the values no test ever sets a builder method for - a one month lock-in, a six month to ten year term, in-multiples-of two, and a two percent whole-term pre-closure penalty. A typed builder that leaves those out sends a different product. DepositInterestCalculator carries over the interest projection unchanged, including its float accumulation. Rewriting it in BigDecimal would move every expected maturity amount, so it stays as it is and the tests keep comparing against it with the tolerance they already declare. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
SavingsAccountsTest (3 tests) and SavingsAccountsExternalIdTest (8) were the last savings tests still building their own RestAssured request specification, and both were already half typed - they drove the Retrofit client for the calls that had a model and dropped to RestAssured only to create the client and product they needed. Neither extends FeignSavingsTestBase, and that is deliberate. Each is a single ordered chain over one account, and FeignSavingsLifecycleExtension rejects every submitted account after each test, so the account a test submits is gone before the next test can approve it. They extend FeignIntegrationTest and construct the three helpers they use instead; the reason is in each class's javadoc so it is not rediscovered by watching an ordered chain fail. SavingsAccountsTest closes the account it leaves active, and the external-id chain already ends by deleting its own. FeignSavingsHelper gains the external-id reads and commands the second class needs. Its 404 case asserts the status off CallFailedRuntimeException rather than only that a call threw. SavingsRequestBuilders picks up three keys SavingsProductHelper.build() sends on every product and the typed builder did not: lockinPeriodFrequency, its frequency type, and lienAllowed. All three carry the server's own defaults, so no product changes shape - but a typed builder that silently sends less than the map builder it replaces is how a migration loses coverage without failing. Both classes pass against a live server, 11 of 11. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
All 47 tests move onto the typed client, and all 47 keep their names - the @test name set is identical to the pre-migration file. The class does not extend FeignSavingsTestBase. A deposit account is a SavingsAccount subclass sharing its table, and the savings lifecycle extension cleans up with SELECT sa.id FROM SavingsAccount sa WHERE sa.status = :status, so it would close the account under test between one test and the next. It extends FeignDepositTestBase instead. The file is 3200 lines shorter by about a thousand, entirely from setup that was repeated verbatim. Where the repetition was only apparent the tests stayed apart: the ten maturity-amount tests use two different date anchors - four open on the first of last month, six on the first of January with an open-ended chart - and each has its own method saying so. Collapsing them onto one anchor moved the projection by 10.87 against a tolerance of 1. Every amount now reads as BigDecimal off the typed model rather than a Float cast out of a map, which is 117 casts gone. Comparisons go through compareTo with a message that prints both values and the tolerance; THRESHOLD keeps its value and its explanation, and no expected literal changed. Three things the migration had to discover rather than read: - constructChartSlabWithAmountRange set isPrimaryGroupingByAmount on the helper, not on the slab it returned, so no test names it. Without it the server demands a periodType and fromPeriod that an amount chart has not got. DepositTestData names the flag per chart. - The undo-approval command rejects locale and dateFormat as unsupported parameters; its body is a note and nothing else. - FeignCalls.fail catches FeignException, so it cannot catch what a helper that already wraps with ok() throws. The two negative cases use assertThrows and assert the status and the error code. DepositTestValidators replaces FixedDepositAccountStatusChecker with the status the account read already returns, and covers the recurring deposit half too. FeignErrors reads the globalisation code out of errors[0], where a validation failure actually reports it; the same logic exists on FeignLoanTestBase, which has 149 subclasses and is left alone. AccountingScenarioIntegrationTest was reading WHOLE_TERM off this class; it now declares its own constant. 47 of 47 pass against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
All 38 tests move onto the typed client with their names unchanged, and like the
fixed deposit tests the class extends FeignDepositTestBase rather than the
savings one, so the savings lifecycle cleanup cannot close the account a test is
working on.
Three command bodies had to be matched to the legacy rather than assumed from
the fixed deposit side, because the validators differ per command:
- undo approval takes a note and nothing else
- calculateInterest and postInterest take an empty body; sending locale or
dateFormat is rejected
- the undo transaction command carries the same date and amount body as
modify, not an empty one
The recurring interest projection is its own function, not the fixed deposit one
reused: the instalment is paid in at the start of each month before that month's
interest accrues, so DepositInterestCalculator now carries both.
The four premature closure tests each keep their own arithmetic. They deposit
twice and count days across a month boundary differently, and folding that into
folding them into one parameterised helper would obscure that difference.
The chart tests deliberately use small instalments: a recurring deposit's chart
slab is chosen on the total expected deposits, so 1000 a month over 12 months
lands in a higher band than the instalment alone suggests. Every parameter was
diffed against the pre-migration file rather than carried over from the fixed
deposit tests.
AccountingScenarioIntegrationTest was reading WHOLE_TERM off this class as well;
it now uses the constant it already declares.
38 of 38 pass against a live server.
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Four gaps between what the savings endpoints accept and what they declare, each
found by a test that could not be typed without it.
The account update takes withHoldTax under command=updateWithHoldTax and reports it
back under changes; neither was declared, so a test toggling withholding tax at
account level had no typed way to send it or to read the confirmation.
The add-charge body takes feeOnMonthDay, with the monthDayFormat that parses it, for
a charge recurring on a fixed day. The savings tests have always sent both.
The shared undo/reverse/modify/releaseAmount endpoint declared only the two fields
undo and reverse read. command=modify replaces the transaction and takes a date, an
amount and a payment type off the same body.
The two GSIM endpoints on /savingsaccounts and the GSIM accounts listing on
/groups/{groupId}/gsimaccounts declared no operation, request or response at all.
They are consumed by the group savings migration later in this PR.
All of it is additive: no field changes type, becomes required, or disappears.
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
feeOnMonthDay is a java.time.MonthDay. Jackson writes it as the ISO string
"--01-15", but the schema generator introspects the type as a bean and emits an
object, so the generated model describes a shape the server never sends. No
client can read a savings account whose charge carries a recurring fee day:
Cannot construct instance of ChargeFeeOnMonthDay from String value ('--01-15')
SavingsAccountData["charges"] -> [0] -> SavingsAccountChargeData["feeOnMonthDay"]
The RestAssured tests never caught it because they parsed the payload as a map.
Three migrated tests drive it - two in ClientSavingsIntegrationTest and one in
GroupSavingsIntegrationTest - and all three failed against a live server before
this change.
Declaring the field as a string makes the schema describe what the endpoint
actually returns. swagger-brake reads object -> string as breaking, but the
current schema is unsatisfiable rather than merely inconvenient, so nothing
could have been relying on it; every existing feeOnMonthDay call site is on a
request model, which already takes a string, and nothing in the repository reads
the generated ChargeFeeOnMonthDay type.
ChargeData.feeOnMonthDay has the identical defect but no test drives it, so it is
left for its own ticket rather than changed unjustified here.
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
FeignSavingsHelper gains the block and unblock commands, the withholding-tax toggle, the close variants that validate posted interest, and the error-returning counterparts of close, reject, delete and read. The charges a test has to reason about are now read off the account with associations=charges rather than from the charges listing: that listing answers a projection carrying neither the due date nor the fee interval, and the charge tests assert on both. FeignSavingsTransactionHelper gains modify, the bulk flag on reverse, the lien flag on hold, and the error-returning deposit, hold and post-interest-as-on. FeignSavingsChargeHelper gains a recurring charge without a due date and the error-returning inactivate and pay. SavingsTestData.ChargeTimeType had SAVINGS_ACTIVATION at 6 and ANNUAL_FEE at 7; the domain enum has 3 and 6. Only WITHDRAWAL_FEE was in use, so no existing charge changes shape - but a savings activation fee created as an annual one is exactly the kind of miss that passes quietly. SavingsTestValidators gains the seven sub-status checks, mirroring the ones SavingsStatusChecker reads off a map against the typed SavingsAccountSubStatusEnumData. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
ClientSavingsIntegrationTest is 45 tests over 3,653 lines and the largest savings class left on RestAssured. It extends FeignSavingsTestBase and keeps @order(2). Its tests are independent of one another, so the per-test lifecycle cleanup does not cut a chain the way it would for the two ordered classes. Every @test name is preserved; the two name sets were diffed rather than eyeballed. Three groups of statements did not survive, and none of them ever ran: - Calendar.DAY_OF_MONTH is the constant 5, so `if (Calendar.DAY_OF_MONTH >= 22)` and `if (Calendar.DAY_OF_MONTH == numberOfDateOfMonth)` guarded four blocks that could not execute. - The post-interest-as-on tests recomputed the same expectation three times from a snapshot taken before the calls under test, then re-rounded an already-rounded value and asserted it against itself. Only the first assertion carried information. Every call is still made, including the one that must be refused. - Locals that were assigned and never read. One assertion changed shape. The charge update endpoint declares an amount on its changes object, but two different nested classes are named PutSavingsChanges - one on savings account charges, one on savings products - and the generator keeps only the products one, so the amount never reaches the model. Rather than rename a published schema for a test, the test reads the charge back and asserts the new amount, which is the stronger check. The collision is worth its own ticket. SchedulerJobsTestResults reached into this class for two constants while already declaring identical ones of its own; it now uses those. The post-interest-as-on tests keep only the assertion that carries information. The RestAssured versions re-read the account after each further posting but then asserted against the variable captured before it, so every later assertion compared two unchanged values; the fresh read was assigned and never used. The server calls are all still made, including the one that must be refused, and the one real check - the interest posted by the first posting - is kept. 45 of 45 pass against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
AccountTransferTest is 11 tests over 1,079 lines, and nine of them opened with the same forty lines: two offices, two clients, a cash-based savings product, a cash-based loan product, a loan taken through approval and disbursal. Those are now three fixtures, which is most of the 940 lines that go. It extends FeignSavingsTestBase for the savings and transfer helpers and keeps LoanTestLifecycleExtension, which FeignLoanTestBase also keeps. The liability transfer financial-activity mapping is still created once for the class and deleted after it, reusing the existing mapping when the environment already has one. Every @test name is preserved. The helpers grow only what the class drives. AccountTransferRequestBuilders gains an overload taking the account types verbatim - the test that drives the server's rejection of an unknown account type cannot express 999 as a PortfolioAccountType - and a payment-details decorator. FeignAccountTransferHelper gains the error-returning create and the transfers of one transfer detail. FeignJournalEntryHelper's liability check gains an office-scoped overload: a transfer between two offices posts to the same account in both, and the test asserts the credit in one and the debit in the other. SavingsRequestBuilders gains the cash-based savings mapping, mirroring the one DepositRequestBuilders already has. The loan product and application spell out the fields the RestAssured builders added on their own: daysInMonthType, daysInYearType and isInterestRecalculationEnabled on the product, transactionProcessingStrategyCode and maxOutstandingLoanBalance on the application. The server rejects a product or application without them, and they never appear in the .withX() chain the migration was read from. The invalid-transfer-date test asserts the 200 the original expected. The name is a misnomer inherited from the RestAssured suite: the server parses "01 05 2013" leniently as 1 May 2013 rather than rejecting it, which the migrated test now records. 11 of 11 pass against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
command=gsimDeposit posts a savingsArray to the transactions endpoint, one entry per child account the deposit splits into. The body was undeclared, so the only typed way to send it was not to. The on-hold fund transactions endpoint declared no response at all, and a resource with no declared response generates a method returning String. It answers a page of DepositAccountOnHoldTransactionData, which is what a test reading the holds against an account pledged as collateral needs. Both additive. The savingsArray entry reuses the GSIM savings schema rather than declaring a second one of the same shape. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
GroupSavingsIntegrationTest is 21 tests over 1,547 lines and the last savings class on RestAssured. It covers three things at once: group savings accounts, GSIM applications and group guarantors on loans. Every @test name is preserved. The GSIM half is what the endpoint declarations earlier in this PR were for. It now runs through FeignGsimHelper: an application creates one parent account and a child per client, and approve, undo, reject, activate, close, update and deposit act on the whole set through the parent's gsimId rather than on a savings id. The child account count and the gsim listing come off /groups/{groupId}/gsimaccounts. The guarantor tests keep asserting what they asserted: that a group can stand as guarantor at all, that the duplicate is refused by code, that a client and a group guarantor coexist on one loan, and that approval - not disbursal - is what places the hold. Helpers grow only what the class drives. FeignGroupHelper gains the group accounts read, whose savings entries carry the balances and the amounts on hold. FeignGuarantorHelper gains the error-returning create. FeignSavingsHelper gains the group application, its update, and the account's on-hold transactions. SavingsRequestBuilders gains the group application bodies and the two GSIM array entries. The guarantee loan product and application spell out the fields the RestAssured builders added on their own - daysInMonthType, daysInYearType and isInterestRecalculationEnabled on the product, transactionProcessingStrategyCode and maxOutstandingLoanBalance on the application - which the server requires and which never appear in the .withX() chain. 21 of 21 pass against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
The one test moves onto the typed client with its name unchanged. It already used the generated models for most of its work, so the change is the base class, the accounting fixture and the transfer itself; the assertions on the journal entries and on the preserved transfer link are untouched. FeignAccountTransferHelper gains undoTransfer, which this test is the only caller of - the command reverses both legs of a transfer and is what the test uses to check the balances return to what they were. The class now extends FeignSavingsTestBase, which carries the per-test savings lifecycle cleanup rather than the per-class one the RestAssured base used. That is a heavier sweep for a single test, but it is the same trade every migrated savings class makes, and retaining the old base for a single test is not justified. 1 of 1 passes against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Two additive gaps, both needed by ShareAccountChargeRoundingTest. PostProductsTypeRequest carried no accounting mappings, so a share product could not be created with cash-based accounting through a typed call. The four fields are not a guess: ProductsApiResource's own @operation text already states "Mandatory Fields for Cash based accounting (accountingRule = 2): shareReferenceId, shareSuspenseId, shareEquityId, incomeFromFeeAccountId" - the schema was simply behind its own documentation. PostAccountsTypeAccountIdRequest is the single body shared by all nine share commands and exposed only requestedShares, so the activate command had to be sent as raw JSON. It now carries activatedDate, dateFormat and locale. The redeemshares command is deliberately left untyped. It reads requestedShares as a share count, while approveadditionalshares reads that same key on that same body as an array of transaction ids; one schema cannot express both, and retyping the field to suit one command would break the other. The helper sends that one command raw with the reason recorded on the method. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
…SavingsIntegrationTest All 16 tests move onto the typed client with their names unchanged, and BaseSavingsIntegrationTest goes with them: this was its last subclass, so the 256-line RestAssured base is deleted rather than left for a later pass. The class builds share products two different ways and the RestAssured defaults behind them differ sharply - the plain product is 1000 shares at unit price 1, while ShareProductHelper's is 10000 shares at 2.0 with an issued count, share bounds and a lock-in period. Carrying over only the visible .withX() chain would have sent the first shape where the accounting test needs the second, so every key that helper writes is reproduced explicitly. FeignShareAccountHelper is new and typed for product creation, application, approve, activate and retrieval. Only redeemshares is raw, because requestedShares means a share count to that command and an array of transaction ids to approveadditionalshares on the same shared body; the reason is on the method and in the raw-HTTP audit. 16 of 16 pass against a live server. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Contributor
Author
|
API backward-compatibility check: 16 expected violations, all from one schema correction verify-api-backward-compatibility fails on this PR. Measured locally with swagger-brake on a sliced spec, plus an identical-spec control run that reported no violations. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
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.