test: Split Loan_test.cpp into topical suites - #7864
Open
Tapanito wants to merge 15 commits into
Open
Conversation
Gate Vault AssetsTotal and LoanBroker DebtTotal to principal-only tracking under featureLendingProtocolV1_1, replacing whole-life (accrual) interest recognition. Adds Accrual/CashBasis namespaces and three public dispatchers in LendingHelpers, consumed by LoanSet, LoanPay, and LoanManage without any amendment-awareness in the transactors themselves. Pre-amendment behavior is unchanged.
Reference VaultVersion::Legacy instead of raw 0 in getVaultVersion's docstring, correct a stale test label, and rename LoanPay's precision log fields from ValueChange to AssetsTotalDelta to match what they actually print
Cash-basis origination never recognizes interest into Vault.AssetsTotal, so checking interestDue against remaining AssetsMaximum headroom rejects loans for no reason. Split the check into Accrual/CashBasis namespaces, mirroring the existing loanOriginationDeltas/loanPaymentDeltas dispatch.
Remove unused vaultMaximum local left over from the AssetsMaximum guard refactor, and stop the DebtMaximum guard from unconditionally adding interestDue to the projected DebtTotal. Reuse the existing loanOriginationDeltas dispatcher, which already excludes interestDue under cash-basis accounting.
Loan_test.cpp had grown to ~9700 lines covering lifecycle, invariant regressions, RIPD/bug repros, and cash-basis accounting in one class. Split into Loan (core transactor tests), LoanInvariant (fuzzer/invariant repros), LoanRegression (RIPD-*/bug:/edge: repros), and LoanCashBasis (cash-basis accounting), sharing fixtures and helpers via a new LoanTestBase. LoanBatch_test/LoanArbitrary_test are rebased onto LoanTestBase instead of Loan_test for the same reason. Pure reorganization: combined case/assertion counts across the four new suites match the pre-split totals exactly (1070 cases, 116951 assertions, 0 failures).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Supersedes the previous 4-file split (Loan/LoanInvariant/ LoanRegression/LoanCashBasis) with a more granular, 10-suite layout: - Loan: core LoanSet lifecycle - LoanLifecycle: full lifecycle, self-loan, issuer/broker-as-borrower - LoanValidation: disabled-amendment and invalid-transaction checks - LoanPay: payment-mechanics regressions (fees, near-zero rate, overflow) - LoanInvariants: fuzzer-derived invariant regressions - LoanRounding: precision/scale/dust rounding regressions - LoanCoverFreezeAuth: first-loss-cover, freeze, and MPT auth permissions - LoanSecurity: PoC and RIPD-numbered regressions - LoanCashBasis: unchanged, LendingProtocolV1_1 cash-basis coverage - LoanMisc: RPC signing flow, plus the LoanBatch/LoanArbitrary manual fuzz suites relocated here Shared fixtures/helpers remain in LoanTestBase.h; testCaseWrapper moves there too since it's used by both LoanSet (Loan) and testLifecycle (LoanLifecycle), not just the former. Pure reorganization: every function body is byte-identical to the prior split, and independent/sensitive call-site wiring matches exactly (verified via diff). The combined suite total is 1071 cases (vs. 1070 previously) with an identical 116951 assertions and 0 failures. The extra case is a benign case-boundary bookkeeping artifact: testLoanSet's (unchanged) setup code performs real transaction-application work before its own first testcase() call, and now that it's the sole/first function in its own suite, that pre-existing activity surfaces as a new blank case instead of attaching to a predecessor's case as it did when other functions ran before it in the old grouping. No assertions were gained, lost, or duplicated.
Run clang-tidy (misc-include-cleaner) scoped to the files touched by the Loan test split. Trims LoanTestBase.h down to only the includes its shared helpers actually need now that most test bodies moved out, and adds direct includes to each split file for symbols they use. Restores two includes the automated fixer incorrectly dropped (test/jtx/jtx_json.h for Json(...), test/jtx/sponsor.h for sponsor::As) which broke the build; clang-tidy's usage tracking doesn't reliably see through these. Verified with a full clean rebuild and test run: 1071 cases, 116951 assertions, 0 failures, unchanged from before this cleanup.
gregtatcam
previously approved these changes
Jul 24, 2026
gregtatcam
left a comment
gregtatcam
left a comment
Contributor
There was a problem hiding this comment.
LGTM
One thought. Does it make sense adding a brief comment to each module describing what tests it covers so it's easier to figure out where the future tests should go to? Some file names are obvious but some are too general.
a1q123456
previously approved these changes
Jul 27, 2026
bthomee
dismissed stale reviews from a1q123456 and gregtatcam
July 30, 2026 13:13
The base branch was changed.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…-split # Conflicts: # include/xrpl/ledger/helpers/LendingHelpers.h # src/libxrpl/ledger/helpers/LendingHelpers.cpp # src/libxrpl/ledger/helpers/VaultHelpers.cpp # src/libxrpl/tx/transactors/lending/LoanManage.cpp # src/libxrpl/tx/transactors/lending/LoanPay.cpp # src/libxrpl/tx/transactors/lending/LoanSet.cpp # src/test/app/LendingHelpers_test.cpp # src/test/app/Loan_test.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| testBatchBypassCounterparty(FeatureBitset features) | ||
| { | ||
| // From FIND-001 | ||
| testcase << "Batch Bypass Counterparty"; |
Contributor
There was a problem hiding this comment.
Regression test checks ttLOAN_BROKER_SET instead of ttLOAN_SET (actual inner tx). If the types have different Batch-disabled status, this silences the FIND-001 test. Verify which type should be checked and use ttLOAN_SET.
gregtatcam
approved these changes
Jul 30, 2026
| // From FIND-001 | ||
| testcase << "Batch Bypass Counterparty"; | ||
|
|
||
| bool const lendingBatchEnabled = !std::ranges::any_of( |
Contributor
There was a problem hiding this comment.
Checks wrong transaction type in kDisabledTxTypes. Should verify ttLOAN_SET, not ttLOAN_BROKER_SET, for FIND-001 test:
Suggested change
| bool const lendingBatchEnabled = !std::ranges::any_of( | |
| [](auto const& disabled) { return disabled == ttLOAN_SET; }); |
| env(pay(borrower, loanKeylet.key, iouAsset(1'500'000'000LL)), Ter(tesSUCCESS)); | ||
| env.close(); | ||
|
|
||
| auto const vaultAfter = env.le(broker.vaultKeylet()); |
Contributor
There was a problem hiding this comment.
vaultAfter dereferenced without null-check. vaultBefore has BEAST_EXPECT guard but vaultAfter doesn't:
Suggested change
| auto const vaultAfter = env.le(broker.vaultKeylet()); | |
| auto const vaultAfter = env.le(broker.vaultKeylet()); | |
| BEAST_EXPECT(vaultAfter); | |
| Number const vaultAvailableAfter = vaultAfter->at(sfAssetsAvailable); |
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
3 participants
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.
Summary
Loan_test.cpphad grown to ~9700 lines covering core transactor tests, fuzzer/invariant regressions, RIPD/bug repros, and cash-basis accounting all in oneSuite. Split into four topical suites —Loan,LoanInvariant,LoanRegression,LoanCashBasis— following the existing precedent for large domains in this codebase (e.g.AMM_test/AMMCalc_test/AMMClawback_test).BrokerInfo,createVaultAndBroker,createFundedIouAsset,nextLoanKeylet,makeLoanPayments, etc.) move into a newLoanTestBase, which every split suite — including the existingLoanBatch_test/LoanArbitrary_testmanual fuzz suites — inherits from instead ofLoan_test.createFundedIouAsset,nextLoanKeylet,createFundedRippleIouAsset,brokerPseudoAccount), and four near-identical fuzzer-derived invariant tests were merged into one (testLoanPayComputePeriodicPaymentInvariants), each finding kept as its owntestcase().Based on
tapanito/lending-cash-basis(#7817) rather thandevelop, since this only reorganizes tests already on that branch.Test plan
rw make) compiles all 5 changed/new files with no new warnings../xrpld -u Loan,LoanInvariant,LoanRegression,LoanCashBasisreports 1070 cases, 116951 assertions, 0 failures — an exact match to the pre-split monolithic suite's totals.LoanBatch,LoanArbitrary) still compile and run; confirmed their underlying logic (runLoan/createLoan/makeLoanPayments/testRandomLoan, deterministic RNG seed) is byte-identical to the pre-split code, so their behavior (includingLoanBatch's pre-existing, seed-dependent failures on some random draws) is unchanged by this move.