refactor(test): make tests deterministic#1242
Merged
Nihantra-Patel merged 15 commits intoJun 1, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1242 +/- ##
===========================================
+ Coverage 81.12% 81.57% +0.44%
===========================================
Files 152 153 +1
Lines 10531 10560 +29
===========================================
+ Hits 8543 8614 +71
+ Misses 1988 1946 -42
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR comprehensively refactors the lending app's test infrastructure by introducing a custom 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lending/loan_management/doctype/loan_disbursement/test_loan_disbursement.py (1)
16-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim redundant bootstrap calls in
TestLoanDisbursement.setUp
lending/tests/test_utils.pyinstantiatesBootStrapTestData()at import time and it already runsinit_loan_products()andinit_customers()(also sets_Test Companyloan_accrual_frequency to"Monthly"; plus creates loan accounts + demand offset order). So inlending/loan_management/doctype/loan_disbursement/test_loan_disbursement.py(setUp, lines ~17-20), the extrainit_loan_products()/init_customers()are redundant;master_init()only matters for the"Weekly"accrual frequency override—keepmaster_init()only if the test depends on weekly accrual, otherwise drop/limit it accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lending/loan_management/doctype/loan_disbursement/test_loan_disbursement.py` around lines 16 - 20, The setUp in TestLoanDisbursement currently calls master_init(), init_loan_products(), and init_customers() redundantly; remove the duplicate init_loan_products() and init_customers() calls and only keep master_init() when this test actually requires the weekly accrual override (loan_accrual_frequency = "Weekly"); if the test does not depend on weekly accrual, drop master_init() entirely so setUp relies on the BootStrapTestData pre-initialization instead. Ensure references to TestLoanDisbursement.setUp, master_init(), init_loan_products(), and init_customers() are updated accordingly.lending/loan_management/doctype/sanctioned_loan_amount/test_sanctioned_loan_amount.py (1)
26-29:⚠️ Potential issue | 🟠 MajorRemove duplicated product/customer bootstrap from
TestSanctionedLoanAmount.setUp
BootStrapTestData()runs atlending/tests/utils.pyimport time (viaLendingTestSuite), and it already callsinit_loan_products()andinit_customers(), so those two calls insetUpare redundant. Keepmaster_init()because it performs additional setup (create_loan_accounts(),setup_loan_demand_offset_order()) and sets the accrual frequency to"Weekly".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lending/loan_management/doctype/sanctioned_loan_amount/test_sanctioned_loan_amount.py` around lines 26 - 29, Remove the redundant bootstrap calls in TestSanctionedLoanAmount.setUp: delete the init_loan_products() and init_customers() calls, leaving only master_init() in setUp, because BootStrapTestData (triggered by LendingTestSuite at import time) already runs init_loan_products and init_customers; ensure master_init() remains because it performs additional necessary setup (create_loan_accounts, setup_loan_demand_offset_order and accrual frequency).
🧹 Nitpick comments (2)
.github/workflows/run-individual-tests.yml (1)
131-141: ⚡ Quick winAvoid expanding
${{ matrix.test }}directly inside therunshell.
${{ matrix.test }}is derived from repository file paths and is expanded inline into the shell on lines 133 and 141, which is the template-injection pattern flagged by static analysis. Pass it throughenvand reference the shell variable instead so its value can never be interpreted as shell code.🔒️ Proposed fix using an env var
- name: Run Tests + env: + MATRIX_TEST: ${{ matrix.test }} run: | - site_name=$(echo "${{matrix.test}}" | sed -e 's/.*\.\(test_.*$\)/\1/') + site_name=$(echo "$MATRIX_TEST" | sed -e 's/.*\.\(test_.*$\)/\1/') echo "$site_name" mkdir ~/frappe-bench/sites/$site_name cp -r "${GITHUB_WORKSPACE}/.github/helper/site_config.json" ~/frappe-bench/sites/$site_name/site_config.json cd ~/frappe-bench/ bench --site $site_name reinstall --yes bench --site $site_name install-app lending bench --site $site_name set-config allow_tests true - bench --site $site_name run-tests --module ${{ matrix.test }} --lightmode + bench --site $site_name run-tests --module "$MATRIX_TEST" --lightmode🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run-individual-tests.yml around lines 131 - 141, The workflow is currently expanding the GitHub Actions expression ${{ matrix.test }} directly inside the shell which risks template-injection; instead add an env entry (e.g., TEST_MODULE: ${{ matrix.test }}) to the job/step and use the shell variable $TEST_MODULE everywhere in the step: replace usages of the inline expression in the site_name extraction (sed expression), and in the bench run-tests invocation, with the quoted shell variable "$TEST_MODULE" so the value is treated as data not code; ensure all references (site_name derivation and bench --site ... run-tests --module) use the new shell variable.lending/loan_management/doctype/loan_adjustment/test_loan_adjustment.py (1)
21-29: 💤 Low valueRedundant bootstrap calls in
setUp.
master_init(),init_loan_products(), andinit_customers()are now also performed byBootStrapTestData(lending/tests/utils.py). They remain idempotent, so this is harmless, but notemaster_init()resets accrual frequency to"Weekly", overriding the bootstrap's"Monthly"— intentional only if these tests rely on weekly accrual. Otherwise thissetUpcan be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lending/loan_management/doctype/loan_adjustment/test_loan_adjustment.py` around lines 21 - 29, The TestLoanAdjustment.setUp currently calls master_init(), init_loan_products(), and init_customers() which duplicate BootStrapTestData in lending/tests/utils.py and unintentionally reset accrual frequency to "Weekly"; remove these redundant calls from TestLoanAdjustment.setUp so the bootstrap from BootStrapTestData is used instead, or if the test truly requires weekly accrual keep only master_init() (or better: remove master_init() and explicitly set the accrual frequency to "Weekly" in the test setup) — locate TestLoanAdjustment.setUp and update it accordingly, referencing master_init, init_loan_products, init_customers, and BootStrapTestData.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lending/tests/utils.py`:
- Around line 21-57: The module currently instantiates BootStrapTestData() at
import which triggers DB writes (create_loan_accounts,
set_loan_settings_in_company, create_loan_security_price) and a
frappe.db.commit() in make_master_data; remove the top-level BootStrapTestData()
call and instead expose a function (e.g., initialize_test_data or
BootStrapTestData.bootstrap()) that calls make_presets/make_master_data and
performs commits only when invoked from test setup; ensure frappe.db.commit() is
moved into that callable (or removed if tests handle transactions) so no DB
writes occur on import, and update tests to call the new function in their
before_tests/setup hooks.
---
Outside diff comments:
In `@lending/loan_management/doctype/loan_disbursement/test_loan_disbursement.py`:
- Around line 16-20: The setUp in TestLoanDisbursement currently calls
master_init(), init_loan_products(), and init_customers() redundantly; remove
the duplicate init_loan_products() and init_customers() calls and only keep
master_init() when this test actually requires the weekly accrual override
(loan_accrual_frequency = "Weekly"); if the test does not depend on weekly
accrual, drop master_init() entirely so setUp relies on the BootStrapTestData
pre-initialization instead. Ensure references to TestLoanDisbursement.setUp,
master_init(), init_loan_products(), and init_customers() are updated
accordingly.
In
`@lending/loan_management/doctype/sanctioned_loan_amount/test_sanctioned_loan_amount.py`:
- Around line 26-29: Remove the redundant bootstrap calls in
TestSanctionedLoanAmount.setUp: delete the init_loan_products() and
init_customers() calls, leaving only master_init() in setUp, because
BootStrapTestData (triggered by LendingTestSuite at import time) already runs
init_loan_products and init_customers; ensure master_init() remains because it
performs additional necessary setup (create_loan_accounts,
setup_loan_demand_offset_order and accrual frequency).
---
Nitpick comments:
In @.github/workflows/run-individual-tests.yml:
- Around line 131-141: The workflow is currently expanding the GitHub Actions
expression ${{ matrix.test }} directly inside the shell which risks
template-injection; instead add an env entry (e.g., TEST_MODULE: ${{ matrix.test
}}) to the job/step and use the shell variable $TEST_MODULE everywhere in the
step: replace usages of the inline expression in the site_name extraction (sed
expression), and in the bench run-tests invocation, with the quoted shell
variable "$TEST_MODULE" so the value is treated as data not code; ensure all
references (site_name derivation and bench --site ... run-tests --module) use
the new shell variable.
In `@lending/loan_management/doctype/loan_adjustment/test_loan_adjustment.py`:
- Around line 21-29: The TestLoanAdjustment.setUp currently calls master_init(),
init_loan_products(), and init_customers() which duplicate BootStrapTestData in
lending/tests/utils.py and unintentionally reset accrual frequency to "Weekly";
remove these redundant calls from TestLoanAdjustment.setUp so the bootstrap from
BootStrapTestData is used instead, or if the test truly requires weekly accrual
keep only master_init() (or better: remove master_init() and explicitly set the
accrual frequency to "Weekly" in the test setup) — locate
TestLoanAdjustment.setUp and update it accordingly, referencing master_init,
init_loan_products, init_customers, and BootStrapTestData.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16e50846-91a2-430e-bb78-588396a70080
📒 Files selected for processing (65)
.github/helper/install.sh.github/workflows/ci.yml.github/workflows/ci_faux.yml.github/workflows/run-individual-tests.yml.mergify.ymllending/loan_management/doctype/bulk_repayment_log/test_bulk_repayment_log.pylending/loan_management/doctype/days_past_due_log/test_days_past_due_log.pylending/loan_management/doctype/lending_settings/test_lending_settings.pylending/loan_management/doctype/loan/test_loan.pylending/loan_management/doctype/loan_accrual_repost/test_loan_accrual_repost.pylending/loan_management/doctype/loan_adjustment/test_loan_adjustment.pylending/loan_management/doctype/loan_application/test_loan_application.pylending/loan_management/doctype/loan_balance_adjustment/test_loan_balance_adjustment.pylending/loan_management/doctype/loan_category/test_loan_category.pylending/loan_management/doctype/loan_classification/test_loan_classification.pylending/loan_management/doctype/loan_demand/test_loan_demand.pylending/loan_management/doctype/loan_demand_offset_order/test_loan_demand_offset_order.pylending/loan_management/doctype/loan_disbursement/test_loan_disbursement.pylending/loan_management/doctype/loan_freeze_log/test_loan_freeze_log.pylending/loan_management/doctype/loan_interest_accrual/test_loan_interest_accrual.pylending/loan_management/doctype/loan_limit_change_log/test_loan_limit_change_log.pylending/loan_management/doctype/loan_npa_log/test_loan_npa_log.pylending/loan_management/doctype/loan_partner/test_loan_partner.pylending/loan_management/doctype/loan_product/test_loan_product.pylending/loan_management/doctype/loan_refund/test_loan_refund.pylending/loan_management/doctype/loan_repayment/test_loan_repayment.pylending/loan_management/doctype/loan_repayment_repost/test_loan_repayment_repost.pylending/loan_management/doctype/loan_repayment_schedule/test_loan_repayment_schedule.pylending/loan_management/doctype/loan_restructure/test_loan_restructure.pylending/loan_management/doctype/loan_restructure_limit_log/test_loan_restructure_limit_log.pylending/loan_management/doctype/loan_security/test_loan_security.pylending/loan_management/doctype/loan_security_assignment/test_loan_security_assignment.pylending/loan_management/doctype/loan_security_deposit/test_loan_security_deposit.pylending/loan_management/doctype/loan_security_price/test_loan_security_price.pylending/loan_management/doctype/loan_security_release/test_loan_security_release.pylending/loan_management/doctype/loan_security_shortfall/loan_security_shortfall.pylending/loan_management/doctype/loan_security_shortfall/test_loan_security_shortfall.pylending/loan_management/doctype/loan_security_type/test_loan_security_type.pylending/loan_management/doctype/loan_transfer/test_loan_transfer.pylending/loan_management/doctype/loan_write_off/test_loan_write_off.pylending/loan_management/doctype/pledge/test_pledge.pylending/loan_management/doctype/process_loan_classification/test_process_loan_classification.pylending/loan_management/doctype/process_loan_demand/test_process_loan_demand.pylending/loan_management/doctype/process_loan_interest_accrual/test_process_loan_interest_accrual.pylending/loan_management/doctype/process_loan_restructure_limit/test_process_loan_restructure_limit.pylending/loan_management/doctype/process_loan_security_shortfall/process_loan_security_shortfall.pylending/loan_management/doctype/process_loan_security_shortfall/test_process_loan_security_shortfall.pylending/loan_management/doctype/sanctioned_loan_amount/test_sanctioned_loan_amount.pylending/loan_management/report/alm_audit_report/alm_audit_report.pylending/loan_management/report/alm_audit_report/test_alm_audit_report.pylending/loan_management/report/applicant_wise_loan_security_exposure/test_applicant_wise_loan_security_exposure.pylending/loan_management/report/future_cashflow_report/test_future_cashflow_report.pylending/loan_management/report/loan_outstanding_report/test_loan_outstanding_report.pylending/loan_management/report/loan_repayment_and_closure/test_loan_repayment_and_closure.pylending/loan_management/report/loan_security_exposure/test_loan_security_exposure.pylending/loan_management/report/loan_security_ledger/test_loan_security_ledger.pylending/loan_management/report/loan_security_status/test_loan_security_status.pylending/loan_management/report/loan_statement_of_account/test_loan_statement_of_account.pylending/loan_management/report/past_cashflow_report/test_past_cashflow_report.pylending/loan_origination/doctype/loan_document_type/test_loan_document_type.pylending/loan_origination/doctype/loan_lead/test_loan_lead.pylending/loan_origination/doctype/loan_origination_settings/test_loan_origination_settings.pylending/loan_origination/doctype/loan_purpose/test_loan_purpose.pylending/tests/test_utils.pylending/tests/utils.py
Comment on lines
+21
to
+57
| class BootStrapTestData: | ||
| def __init__(self): | ||
| self.make_presets() | ||
| self.make_master_data() | ||
|
|
||
| def make_presets(self): | ||
| create_loan_accounts() | ||
| setup_loan_demand_offset_order() | ||
|
|
||
| def make_master_data(self): | ||
| set_loan_settings_in_company() | ||
| set_loan_accrual_frequency("Monthly") | ||
| init_loan_products() | ||
| self.make_flat_interest_rate_loan() | ||
| create_loan_security_type() | ||
| create_loan_security() | ||
| create_loan_security_price( | ||
| "Test Security 1", 500, "Nos", nowdate(), add_days(nowdate(), 1), update_if_existing=True | ||
| ) | ||
| create_loan_security_price( | ||
| "Test Security 2", 250, "Nos", nowdate(), add_days(nowdate(), 1), update_if_existing=True | ||
| ) | ||
| init_customers() | ||
| make_customer("_Test Loan Customer 2") | ||
| frappe.db.commit() # nosemgrep | ||
|
|
||
| def make_flat_interest_rate_loan(self): | ||
| create_loan_product( | ||
| "Flat Interest Rate Loan", | ||
| "Flat Interest Rate Loan", | ||
| 1000000, | ||
| 12, | ||
| repayment_schedule_type="Flat Interest Rate", | ||
| ) | ||
|
|
||
|
|
||
| BootStrapTestData() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Frappe framework test runner, does the before_tests hook run before or after test modules are imported during test discovery?
💡 Result:
before_tests runs before the test modules (test files) are imported during test discovery. Evidence: Frappe’s hook documentation describes before_tests as “Test Hooks” that run “before tests are run on a site,” and the unit-testing docs note there is a “test initialization script” that can be skipped via --skip-before-tests, implying the initialization (including before_tests) happens ahead of the actual test execution/import/collection phase. [1][2][3]
Citations:
- 1: https://docs.frappe.io/framework/user/en/python-api/hooks
- 2: https://docs.frappe.io/framework/v13/user/en/guides/automated-testing/unit-testing
- 3: https://docs.frappe.io/framework/v14/user/en/guides/automated-testing/unit-testing
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
sed -n '1,120p' lending/tests/utils.py | cat -nRepository: frappe/lending
Length of output: 3568
Avoid DB writes + frappe.db.commit() at module import time in lending/tests/utils.py
BootStrapTestData() is instantiated at import (line 57), which runs create_loan_accounts/set_loan_settings_in_company/create_loan_security_price and then frappe.db.commit() (line 45) during test discovery/import. Even if before_tests runs earlier, this still creates unwanted side effects when the module is merely imported (e.g., skipped/filtered runs).
♻️ Suggested change
-BootStrapTestData()
-
-
class LendingTestSuite(ERPNextTestSuite):
"""Class for creating Lending test records"""
- pass
+ `@classmethod`
+ def setUpClass(cls):
+ super().setUpClass()
+ BootStrapTestData()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lending/tests/utils.py` around lines 21 - 57, The module currently
instantiates BootStrapTestData() at import which triggers DB writes
(create_loan_accounts, set_loan_settings_in_company, create_loan_security_price)
and a frappe.db.commit() in make_master_data; remove the top-level
BootStrapTestData() call and instead expose a function (e.g.,
initialize_test_data or BootStrapTestData.bootstrap()) that calls
make_presets/make_master_data and performs commits only when invoked from test
setup; ensure frappe.db.commit() is moved into that callable (or removed if
tests handle transactions) so no DB writes occur on import, and update tests to
call the new function in their before_tests/setup hooks.
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
2 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 by CodeRabbit
New Features
Bug Fixes
Tests
Chores