[change] Replace global mock decorator with local contextmanager in test_admin.TestAdminTransaction#397
Open
asmodehn wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors the test module by moving two mock target path strings from module scope into the TestAdminTransaction class as class attributes: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 658-663: The test uses the class attribute _mock_upgrade in a
decorator which is evaluated at class-definition time and prevents subclasses
from overriding it; change the decorated patch on _mock_upgrade in
test_upgrade_selected_action_perms to use a runtime context manager (with
mock.patch(self._mock_upgrade, return_value=True):) like the existing
self._mock_connect usage so subclass overrides of _mock_upgrade are respected,
and apply the same conversion to any other test methods that currently patch
_mock_upgrade via decorator (search for occurrences of
`@mock.patch`(_mock_upgrade) and replace them with with
mock.patch(self._mock_upgrade, ...): blocks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66737650-d830-4b91-9f9e-a9e49dae1714
📒 Files selected for processing (1)
openwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/tests/test_admin.py (1)
655-659: Good refactoring for inheritance support.Moving the mock paths from module-level to class attributes enables subclasses to override them, which aligns with the PR objective. The inherited test class in
tests/openwisp2/sample_firmware_upgrader/tests.pywill correctly access these attributes.
Comment on lines
+658
to
+663
| _mock_upgrade = "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade" | ||
| _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" | ||
|
|
||
| def test_upgrade_related(self, *args): | ||
| self._login() | ||
| env = self._create_upgrade_env() | ||
| self._create_firmwareless_device(organization=env["d1"].organization) | ||
| # check state is good before proceeding | ||
| fw = DeviceFirmware.objects.filter( | ||
| image__build_id=env["build2"].pk | ||
| ).select_related("image") | ||
| self.assertEqual(Device.objects.count(), 3) | ||
| self.assertEqual(UpgradeOperation.objects.count(), 0) | ||
| self.assertEqual(fw.count(), 0) | ||
|
|
||
| with self.subTest("Invalid upgrade_options"): | ||
| response = self.client.post( | ||
| self.build_list_url, | ||
| { | ||
| "action": "upgrade_selected", | ||
| "upgrade_related": "upgrade_related", | ||
| "upgrade_options": "invalid", | ||
| ACTION_CHECKBOX_NAME: (env["build2"].pk,), | ||
| }, | ||
| follow=True, | ||
| ) | ||
| id_attr = ( | ||
| ' id="id_upgrade_options_error"' if django.VERSION >= (5, 2) else "" | ||
| ) | ||
| self.assertContains( | ||
| response, | ||
| f'<ul class="errorlist"{id_attr}><li>Enter a valid JSON.</li></ul>', | ||
| @mock.patch(_mock_upgrade, return_value=True) | ||
| def test_upgrade_selected_action_perms(self, *args): | ||
| with mock.patch(self._mock_connect, return_value=True): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using self._mock_upgrade in context managers for inheritance consistency.
The refactor successfully moves mock paths to class attributes, but there's an inconsistency that may affect subclass customization:
_mock_upgradeused in decorators is evaluated at class definition time and captures the base class valueself._mock_connectused in context managers is evaluated at runtime and respects subclass overrides
If a subclass needs to override _mock_upgrade, the inherited decorated methods won't use the overridden value. For full inheritance support, consider converting _mock_upgrade to context manager style as well:
♻️ Proposed change for consistent inheritance behavior
- `@mock.patch`(_mock_upgrade, return_value=True)
- def test_upgrade_selected_action_perms(self, *args):
- with mock.patch(self._mock_connect, return_value=True):
+ def test_upgrade_selected_action_perms(self, *args):
+ with mock.patch(self._mock_upgrade, return_value=True), \
+ mock.patch(self._mock_connect, return_value=True):Apply this pattern to all test methods that currently use the decorator.
📝 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
| _mock_upgrade = "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade" | |
| _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" | |
| def test_upgrade_related(self, *args): | |
| self._login() | |
| env = self._create_upgrade_env() | |
| self._create_firmwareless_device(organization=env["d1"].organization) | |
| # check state is good before proceeding | |
| fw = DeviceFirmware.objects.filter( | |
| image__build_id=env["build2"].pk | |
| ).select_related("image") | |
| self.assertEqual(Device.objects.count(), 3) | |
| self.assertEqual(UpgradeOperation.objects.count(), 0) | |
| self.assertEqual(fw.count(), 0) | |
| with self.subTest("Invalid upgrade_options"): | |
| response = self.client.post( | |
| self.build_list_url, | |
| { | |
| "action": "upgrade_selected", | |
| "upgrade_related": "upgrade_related", | |
| "upgrade_options": "invalid", | |
| ACTION_CHECKBOX_NAME: (env["build2"].pk,), | |
| }, | |
| follow=True, | |
| ) | |
| id_attr = ( | |
| ' id="id_upgrade_options_error"' if django.VERSION >= (5, 2) else "" | |
| ) | |
| self.assertContains( | |
| response, | |
| f'<ul class="errorlist"{id_attr}><li>Enter a valid JSON.</li></ul>', | |
| @mock.patch(_mock_upgrade, return_value=True) | |
| def test_upgrade_selected_action_perms(self, *args): | |
| with mock.patch(self._mock_connect, return_value=True): | |
| _mock_upgrade = "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade" | |
| _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" | |
| def test_upgrade_selected_action_perms(self): | |
| with mock.patch(self._mock_upgrade, return_value=True), \ | |
| mock.patch(self._mock_connect, return_value=True): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_firmware_upgrader/tests/test_admin.py` around lines 658 - 663, The
test uses the class attribute _mock_upgrade in a decorator which is evaluated at
class-definition time and prevents subclasses from overriding it; change the
decorated patch on _mock_upgrade in test_upgrade_selected_action_perms to use a
runtime context manager (with mock.patch(self._mock_upgrade,
return_value=True):) like the existing self._mock_connect usage so subclass
overrides of _mock_upgrade are respected, and apply the same conversion to any
other test methods that currently patch _mock_upgrade via decorator (search for
occurrences of `@mock.patch`(_mock_upgrade) and replace them with with
mock.patch(self._mock_upgrade, ...): blocks).
Member
Author
There was a problem hiding this comment.
yes but this will make the change in behaviour bigger than it needs to be, so I will abstain until there is an actual need, or a specific request from maintainers to do so.
There was a problem hiding this comment.
@asmodehn, that's a reasonable call — keeping the PR focused on the minimum necessary refactor is sensible, and the inheritance concern only matters if a subclass actually needs to override _mock_upgrade. Feel free to revisit when there's a concrete need.
🧠 Learnings used
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
4 tasks
asmodehn
changed the title
nemesifier
reviewed
May 8, 2026
Member
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@asmodehn the change looks ok to merge, I don't see any problem with it, but I do not understand the need for this change and I think the pull request description fails to explain this. Why is this change needed, what does the current code prevent you to do?
github-project-automation
Bot
moved this to In progress
in OpenWISP Contributor's Board
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.
Checklist
Description of Changes
Refactoring one test file to allow for simple inheritance.
Part of #362 for simpler check