Open
Conversation
Introduces a factories package under forms_pro/tests/factories/ with: - UserFactory: generates random User docs; with_forms_pro_role trait includes the role in the initial doc dict so on_update fires once with the role already set - FPTeamFactory: generates FP Team docs with a random team name default Both factories implement __del_override__ as a safety-net cleanup guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces manual frappe.get_doc + Faker boilerplate with UserFactory.create(). Role assignment is kept as a post-insert step since the test specifically exercises the on_update hook that fires when a role is added after creation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces _create_user() and _create_team() helpers and the Faker/string tracking lists with UserFactory and FPTeamFactory. Key changes: - _user(*traits) and _team(owner) thin wrappers call the factories and track Document objects instead of email/name strings - Helper signatures updated to accept Document args throughout - UserFactory.build().email used where only a random email is needed (no DB insert required) - All 11 tests pass unchanged in behaviour Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds test factories ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Introduces a new required_apps list in hooks.py to include the frappe_factory_bot repository, enabling its integration into the application. This change enhances the app's capabilities by allowing the use of factory methods for testing and development.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
forms_pro/tests/factories/user_factory.py (1)
29-35: Consider logging exceptions in cleanup instead of silently swallowing them.The bare
except Exception: passmakes it difficult to diagnose cleanup failures during test runs. While cleanup should be resilient, silently swallowing exceptions can mask underlying issues (e.g., permission problems, database connection issues) and leave orphaned test data.♻️ Proposed fix to add logging
+import logging + +_logger = logging.getLogger(__name__) + `@staticmethod` def __del_override__(_self: Any) -> None: try: if frappe.db.exists("User", _self.name): frappe.delete_doc("User", _self.name, force=True, ignore_permissions=True) - except Exception: - pass + except Exception as e: + _logger.debug("Failed to delete test user %s: %s", _self.name, e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/tests/factories/user_factory.py` around lines 29 - 35, The __del_override__ cleanup currently swallows all exceptions; update it to catch Exception as e and log the error before continuing (e.g., call frappe.log_error(e, "user_factory.__del_override__ cleanup failed") or use frappe.logger().error with a clear message and the exception), keeping the existing deletion logic and still suppressing the exception to avoid breaking tests; reference the __del_override__ staticmethod and the frappe.delete_doc/frappe.db.exists calls when applying the change.forms_pro/tests/factories/fp_team_factory.py (1)
17-23: Same suggestion: log cleanup exceptions for debuggability.Consistent with the
UserFactoryfeedback, consider logging cleanup failures rather than silently passing.♻️ Proposed fix
+import logging + +_logger = logging.getLogger(__name__) + `@staticmethod` def __del_override__(_self: Any) -> None: try: if frappe.db.exists("FP Team", _self.name): frappe.delete_doc("FP Team", _self.name, force=True, ignore_permissions=True) - except Exception: - pass + except Exception as e: + _logger.debug("Failed to delete test team %s: %s", _self.name, e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/tests/factories/fp_team_factory.py` around lines 17 - 23, The __del_override__ cleanup currently swallows all exceptions, which hides failures; update the handler in the __del_override__ method so that instead of a bare except: pass it catches Exception as e and logs the error (including exception details and context like the FP Team name) using the project's logging mechanism (e.g., frappe.log_error or the module logger) when frappe.db.exists("FP Team", _self.name) and frappe.delete_doc("FP Team", _self.name, ...) fails, so failures during delete are recorded for debugging.forms_pro/tests/test_roles.py (1)
12-31: Consider adding explicit tearDown cleanup for test isolation.Unlike
TestTeamInvitationswhich tracks created users in_created_usersand explicitly deletes them intearDown, this test relies solely on the factory's__del_override__for cleanup. Python's__del__timing is non-deterministic, which could cause test pollution if users persist across test runs.♻️ Proposed fix to add explicit cleanup
class TestRoles(IntegrationTestCase): def setUp(self): super().setUp() + self._created_users = [] + + def tearDown(self): + frappe.set_user("Administrator") + for user in self._created_users: + if frappe.db.exists("User", user.name): + frappe.delete_doc("User", user.name, force=True, ignore_permissions=True) + frappe.db.commit() + super().tearDown() def test_roles(self): user = UserFactory.create() + self._created_users.append(user) roles = frappe.get_roles(user.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/tests/test_roles.py` around lines 12 - 31, Add explicit teardown cleanup for TestRoles by tracking created users and deleting them in a tearDown method: when creating a user via UserFactory.create() in test_roles (or in setUp), append the user's name to a test-level list (e.g., self._created_users) and implement tearDown to iterate over that list and remove each user (using the same deletion mechanism used elsewhere, e.g., delete_doc or user.delete()) to ensure deterministic cleanup; update setUp to initialize self._created_users if needed and ensure tearDown calls super().tearDown() at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@forms_pro/tests/factories/fp_team_factory.py`:
- Around line 17-23: The __del_override__ cleanup currently swallows all
exceptions, which hides failures; update the handler in the __del_override__
method so that instead of a bare except: pass it catches Exception as e and logs
the error (including exception details and context like the FP Team name) using
the project's logging mechanism (e.g., frappe.log_error or the module logger)
when frappe.db.exists("FP Team", _self.name) and frappe.delete_doc("FP Team",
_self.name, ...) fails, so failures during delete are recorded for debugging.
In `@forms_pro/tests/factories/user_factory.py`:
- Around line 29-35: The __del_override__ cleanup currently swallows all
exceptions; update it to catch Exception as e and log the error before
continuing (e.g., call frappe.log_error(e, "user_factory.__del_override__
cleanup failed") or use frappe.logger().error with a clear message and the
exception), keeping the existing deletion logic and still suppressing the
exception to avoid breaking tests; reference the __del_override__ staticmethod
and the frappe.delete_doc/frappe.db.exists calls when applying the change.
In `@forms_pro/tests/test_roles.py`:
- Around line 12-31: Add explicit teardown cleanup for TestRoles by tracking
created users and deleting them in a tearDown method: when creating a user via
UserFactory.create() in test_roles (or in setUp), append the user's name to a
test-level list (e.g., self._created_users) and implement tearDown to iterate
over that list and remove each user (using the same deletion mechanism used
elsewhere, e.g., delete_doc or user.delete()) to ensure deterministic cleanup;
update setUp to initialize self._created_users if needed and ensure tearDown
calls super().tearDown() at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4e21514-85f7-484c-950f-102c37e30e33
📒 Files selected for processing (5)
forms_pro/tests/factories/__init__.pyforms_pro/tests/factories/fp_team_factory.pyforms_pro/tests/factories/user_factory.pyforms_pro/tests/test_invitations.pyforms_pro/tests/test_roles.py
bench get-app with a local path skips required_apps resolution, so frappe_factory_bot was never fetched. bench install-app then failed with ModuleNotFoundError when Frappe tried to satisfy the required_apps entry. Explicitly get the app before installing forms_pro. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@forms_pro/hooks.py`:
- Line 15: The required_apps list currently contains a Git URL which Frappe
doesn't accept; update the required_apps variable to use the app slug or
org/repo form instead of the full Git URL. Locate the required_apps declaration
and replace "https://github.com/harshtandiya/frappe_factory_bot.git:main" with
the package name or repo slug (for example "frappe_factory_bot" or
"harshtandiya/frappe_factory_bot") so dependency validation during
install/migrate succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
forms_pro/hooks.py
Outdated
| # ------------------ | ||
|
|
||
| # required_apps = [] | ||
| required_apps = ["https://github.com/harshtandiya/frappe_factory_bot.git:main"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Frappe Framework v15 hooks.py, does required_appsaccept Git URLs or only app names? Please provide the official docs example forrequired_apps.
💡 Result:
required_apps is not a Git URL field. In v15 it’s meant to list dependent app identifiers (the app name like erpnext, and in some contexts an org/repo slug like frappe/erpnext), rather than full https://... Git URLs. This is consistent with how Frappe describes required apps being referenced as erpnext or frappe/erpnext (org + app). [2]
Official docs example (hooks.py):
required_apps = ["erpnext"][1]
Sources: Frappe Framework Hooks docs (Required Apps section) [1], Frappe Cloud “Required app not found” (explains frappe/erpnext org/app form vs just erpnext) [2].
Use app name in required_apps, not a Git URL.
Line 15 sets required_apps with a repository URL, but Frappe expects installed app names (e.g., ["erpnext"] or org/repo slugs like ["frappe/erpnext"]). Using a Git URL here breaks dependency validation during install/migrate. Use the app slug instead.
Proposed fix
-required_apps = ["https://github.com/harshtandiya/frappe_factory_bot.git:main"]
+required_apps = ["frappe_factory_bot"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| required_apps = ["https://github.com/harshtandiya/frappe_factory_bot.git:main"] | |
| required_apps = ["frappe_factory_bot"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/hooks.py` at line 15, The required_apps list currently contains a
Git URL which Frappe doesn't accept; update the required_apps variable to use
the app slug or org/repo form instead of the full Git URL. Locate the
required_apps declaration and replace
"https://github.com/harshtandiya/frappe_factory_bot.git:main" with the package
name or repo slug (for example "frappe_factory_bot" or
"harshtandiya/frappe_factory_bot") so dependency validation during
install/migrate succeeds.
Collaborator
|
I don't think this should be a dependency. |
…ps for frappe_factory_bot Added a new section to the README detailing how to run tests locally using frappe_factory_bot. Included installation instructions and commands for running all tests as well as specific test modules.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 93: The workflow currently checks out a mutable personal fork branch via
the bench command ("bench get-app frappe_factory_bot ... --branch main"); change
this to pin to an immutable, trusted ref by replacing the repository URL with
the upstream/trusted repository and using a specific tag or commit hash instead
of "main" (e.g., use --branch vX.Y.Z or --branch <commit-sha>) so builds are
reproducible and supply-chain safe; update the bench invocation that references
frappe_factory_bot accordingly and ensure the chosen tag/sha exists in the
trusted upstream before committing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21553432-514f-447a-b5be-efc4ac71651e
📒 Files selected for processing (3)
.github/workflows/ci.ymlREADME.mdforms_pro/hooks.py
💤 Files with no reviewable changes (1)
- forms_pro/hooks.py
✅ Files skipped from review due to trivial changes (1)
- README.md
| - name: Install | ||
| working-directory: /home/runner/frappe-bench | ||
| run: | | ||
| bench get-app frappe_factory_bot https://github.com/harshtandiya/frappe_factory_bot.git --branch main |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CI workflow does not use mutable branch refs for get-app.
rg -n --iglob '*.yml' --iglob '*.yaml' 'bench get-app .*--branch (main|master)\b' .github/workflows
# Verify source is not a personal fork namespace.
rg -n --iglob '*.yml' --iglob '*.yaml' 'bench get-app .*https://github.com/[a-zA-Z0-9_-]+/frappe_factory_bot\.git' .github/workflowsRepository: BuildWithHussain/forms_pro
Length of output: 349
Pin frappe_factory_bot to an immutable ref from a trusted source.
Line 93 uses a mutable main branch from a personal fork, which makes builds non-reproducible and weakens supply-chain safety. Pin to a version tag and use a trusted upstream repository.
Suggested change
- bench get-app frappe_factory_bot https://github.com/harshtandiya/frappe_factory_bot.git --branch main
+ bench get-app frappe_factory_bot https://github.com/<trusted-org>/frappe_factory_bot.git --branch <pinned-tag>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 93, The workflow currently checks out a
mutable personal fork branch via the bench command ("bench get-app
frappe_factory_bot ... --branch main"); change this to pin to an immutable,
trusted ref by replacing the repository URL with the upstream/trusted repository
and using a specific tag or commit hash instead of "main" (e.g., use --branch
vX.Y.Z or --branch <commit-sha>) so builds are reproducible and supply-chain
safe; update the bench invocation that references frappe_factory_bot accordingly
and ensure the chosen tag/sha exists in the trusted upstream before committing.
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
Integrates
frappe_factory_botinto the Forms Pro test suite, replacing ad-hoc test data helpers with a proper factory layer.New:
forms_pro/tests/factories/UserFactory— generatesUserdocs with randomised email/name via Faker; exposes awith_forms_pro_roletrait that includes the role in the initial doc dict soon_updatefires once with the role already setFPTeamFactory— generatesFP Teamdocs with a random team name; owner is determined by the active Frappe user at creation time (matching the real app flow)__del_override__as a safety-net cleanup guardRefactored:
test_roles.pyFaker+frappe.get_docboilerplate with a singleUserFactory.create()callRefactored:
test_invitations.py_create_user()and_create_team()helper methods and theFakerimportlist[str](email/name strings) tolist[Document]— teardown now deletes bydoc.nameUserFactory.build().emailis used where only a random email address is needed without inserting a DB recordDocumentargs throughout, eliminating.namelookups scattered across the test bodyhooks.pyfrappe_factory_bottorequired_appsso bench/CI pulls it in before running testsTesting
pre-commit run --all-filesbench --site forms.localhost run-tests --app forms_pro— all 33 tests pass (2 pre-existing skips)Checklist
Summary by CodeRabbit
Chores
Tests
Documentation