test: Split Loan_test.cpp into topical suites - #7864
Merged
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}
mathbunnyru
reviewed
Aug 4, 2026
Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
mathbunnyru
approved these changes
Aug 4, 2026
mathbunnyru
enabled auto-merge
August 4, 2026 13:29
mathbunnyru
approved these changes
Aug 4, 2026
mathbunnyru
disabled auto-merge
August 4, 2026 13:42
mathbunnyru
enabled auto-merge
August 4, 2026 13:42
Contributor
There was a problem hiding this comment.
Most of the diff consists of file renames/deletions with no visible content changes, and the two substantive code snippets (suite.h's new protected runner() accessor, and the jtx::Balance qualification fixes in LoanBroker_test.cpp) look correct and low-risk. The one thing worth double-checking is the ubsan suppression path update, which doesn't line up with the four-suite split described in the MR.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Aug 4, 2026
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…-split # Conflicts: # src/test/app/Loan_test.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Contributor
There was a problem hiding this comment.
Most of the diff is file moves/deletions with no visible content, so the reviewable surface is small: a new protected accessor in suite.h, a renamed+trivially-edited LoanBroker_test.cpp, and a path update in ubsan.supp. The ubsan.supp change looks suspicious — it points at a file name that doesn't match any of the four topical suites described in the MR.
mathbunnyru
approved these changes
Aug 4, 2026
mathbunnyru
enabled auto-merge
August 4, 2026 15:33
Tapanito
added a commit
that referenced
this pull request
Aug 6, 2026
Loan_test.cpp was split into topical suites under src/test/app/lending/ on develop (#7864). Port testLoanPayFundsConservedPayeeBelowReserve into LoanPay_test.cpp.
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
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, invariant/fuzzer regressions, RIPD/bug repros, and cash-basis accounting all in oneSuite. Split into a newsrc/test/app/lending/directory containing 12 topical suites, grouped by concern rather than by history:LendingHelpers,LoanBroker,LoanCashBasis,LoanCoverFreezeAuth,LoanInvariants,LoanLifecycle,LoanMisc,LoanPay,LoanRounding,LoanSecurity,LoanSet,LoanValidation.LendingHelpers_test.cppandLoanBroker_test.cpp, moved intosrc/test/app/lending/alongside the new suites so all lending tests live in one place.BrokerInfo,createVaultAndBroker,createFundedIouAsset,nextLoanKeylet,makeLoanPayments, etc.) live in a newLoanTestBase.h, which every split suite — including the pre-existing manual fuzz suitesLoanBatch_test/LoanArbitrary_test(now inLoanMisc_test.cpp) — inherits from.src/test/app/lending/Loan_test.cpp, that reruns all 12 topical suites under the historicalLoanname so existing tooling/muscle memory that invokes-u Loankeeps working. This needed a small addition tobeast::unit_test::Suite(a protectedrunner()accessor) so the aggregator can dispatch into its member suites via the global suite registry.test/app/Loan_test.cpppath to its new home,test/app/lending/LoanSecurity_test.cpp(the file that still contains the intentional unsigned-underflow PoC), and picked up a handful of reviewer/AI-flagged fixes along the way (a couple of unguarded SLE dereferences, an ambiguousBalancereference from unity-build batching, stale comments).Based on
develop(the cash-basis accounting work this originally depended on, #7817, has since merged intodevelop). Kept in sync withdevelopvia several merges; the most recent one conflicted on the now-deleted monolithicLoan_test.cppagainst an unrelateddevelopcommit that renamedloanBroker/Lending/Accrual/CashBasisnamespaces to snake_case — that rename was reapplied by hand to the split files.Test plan
rw make) compiles all changed/new files with no new warnings../xrpld -u Loan(the aggregator, rerunning all 12 topical suites) reports 1188 cases, 122054 tests, 0 failures../xrpld -u <Suite>) with 0 failures.LoanBatch,LoanArbitrary) still compile and run; their underlying logic is unchanged by this move.