[fix:tests] Extended controller app tests part one#362
Conversation
asmodehn
force-pushed
the
extended_controller_app_tests_part_one
branch
from
eb3df37 to
7c1f2a7
Compare
nemesifier
changed the title
| from django.views.generic import RedirectView | ||
|
|
||
| # from openwisp_controller.config.api.urls import get_api_urls as get_config_api_urls | ||
| # from openwisp_controller.config.utils import get_controller_urls |
There was a problem hiding this comment.
I think I understand why you're leaving these commented out here, but usually we don't leave commented code in OpenWISP, someone looking at this may think it's forgotten removed code, so I suggest adding a comment above summing up why this is here, eg: # For example purposes, in case you may need to extend further.
There was a problem hiding this comment.
Actually, it was practical to have here, but I have this information in the other PR, so I removed it from here.
| # (get_config_api_urls(config_api_views), "config_api"), | ||
| # namespace="config_api", | ||
| # ), | ||
| # ), |
There was a problem hiding this comment.
Same here and I would group the two commented URL paths together and put them either in the beginning or at the end of the URL path list.
The floorplan,jpg is useful for sample_connection tests. #303 deals with extending the more troublesome config app. I will rebase #303 on master once this #362 is merged in, and continue working to extend config into a sample_config app for tests. |
asmodehn
force-pushed
the
extended_controller_app_tests_part_one
branch
from
4a19dec to
df6a7dc
Compare
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
| class TestNotifications(BaseTestNotifications): | ||
| app_label = "sample_connection" | ||
|
|
||
| def _generic_notification_test( |
There was a problem hiding this comment.
Is this method doing anything at all?
AFAIK, test methods need to start with test_ or they won't be executed in tests. If you try adding a broken line of code in this test I think it will not be executed.
| def _generic_notification_test( | |
| def test_generic_notification( |
There was a problem hiding this comment.
It is overiding this one https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/connection/tests/test_notifications.py#L28, called from the same python module
| class TestNotificationTransaction(BaseTestNotificationTransaction): | ||
| app_label = "sample_connection" | ||
|
|
||
| def _generic_notification_test( |
There was a problem hiding this comment.
This looks identical to the test above, are you sure it's really needed? We use the TransactionTestCase when DB transactions need to be enabled for the feature to work, I think you can remove this test because it won't be doing anything more than the other test method above.
There was a problem hiding this comment.
Ah yes it is the same as before, I will remove that one...
There was a problem hiding this comment.
Ah I see now why this is duplicated, it is overriding a method of a base test class, so i need the override in both places...
the only line hanges tho is :
config_app = (
"config" if not os.environ.get("SAMPLE_APP", False) else "sample_config"
)
so maybe I dont need it at this stage, I ll try and get rid of it...
There was a problem hiding this comment.
argh no... That test on an env var, embedded in the method, instead of relying on a class level attribute is forcing my hand... I ll see if I can find some trick...
There was a problem hiding this comment.
This is probably cleaner : https://github.com/openwisp/openwisp-controller/pull/1192/files
and should simplify things here... Let me know hat you think
| path("admin/", admin.site.urls), | ||
| path("", redirect_view, name="index"), | ||
| path("", include("openwisp_controller.urls")), | ||
| # path("accounts/", include("openwisp_users.accounts.urls")), # Not needed in development |
There was a problem hiding this comment.
I'd just remove this from here to avoid generating noise (it adds up in the long run).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a test sample app tests/openwisp2/sample_connection (models, migrations, apps, API view adapters, pytest wrapper) and wires it into test settings and URLs via new CONNECTION_* settings and conditional SAMPLE_APP URL inclusion. Refactors many firmware_upgrader tests to replace class-level decorator mocks with per-test/context-manager mocking for DeviceConnection.connect and OpenWrt.upgrade (new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openwisp_firmware_upgrader/tests/test_selenium.py (2)
21-35: Patch target may miss swappedDeviceConnectionimplementations.Since
DeviceConnection = swapper.load_model("connection", "DeviceConnection"), patching"openwisp_controller.connection.models.DeviceConnection.connect"might not intercept calls when the connection app/model is swapped. Consider patching the loaded model instead (module + class name).Proposed fix
class TestDeviceAdmin(TestUpgraderMixin, SeleniumTestMixin, StaticLiveServerTestCase): @@ - _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" + _mock_connect = f"{DeviceConnection.__module__}.{DeviceConnection.__name__}.connect"
114-331: Address Ruff warnings (ARG002,RUF059) to avoid lint-gated CI failures.You can keep the patch decorators but make the injected args explicit/ignored, and avoid binding unused unpacked values.
Proposed fix
@@ @capture_any_output() @patch( "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade", return_value=True, ) - def test_device_firmware_upgrade_options(self, *args): + def test_device_firmware_upgrade_options(self, _mock_upgrade): @@ @capture_any_output() @patch( "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade", return_value=True, ) - def test_batch_upgrade_upgrade_options(self, *args): + def test_batch_upgrade_upgrade_options(self, _mock_upgrade): @@ @capture_any_output() @patch( "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade", return_value=True, ) @patch.object(OpenWrt, "SCHEMA", None) - def test_upgrader_with_unsupported_upgrade_options(self, *args): + def test_upgrader_with_unsupported_upgrade_options(self, _mock_upgrade, _mock_schema): with patch(self._mock_connect, return_value=True): - org, category, build1, build2, image1, image2, device = self._set_up_env() + _org, _category, _build1, build2, _image1, image2, device = self._set_up_env()tests/openwisp2/sample_firmware_upgrader/tests.py (1)
124-130:TestModels.app_labelshould be"sample_firmware_upgrader"(not the dotted path).This is inconsistent with every other test class in the file (
TestAdmin,TestAdminTransaction,TestDeviceAdmin) which all use the short app label. The dotted path will break mock patching in the base class tests and violates Django conventions for app labels.Proposed fix
class TestModels(BaseTestModels): - app_label = "openwisp2.sample_firmware_upgrader" + app_label = "sample_firmware_upgrader"
🤖 Fix all issues with AI agents
In @tests/openwisp2/urls.py:
- Around line 10-38: The SAMPLE_APP check uses os.environ.get("SAMPLE_APP",
False) which returns a string so values like "0" or "false" are treated as
truthy; change the condition to explicitly parse the env var string (e.g., read
value = os.environ.get("SAMPLE_APP", "") and test value.lower() in
("1","true","yes") or use a canonical str-to-bool helper) before appending to
urlpatterns; update the conditional around urlpatterns (the if that references
SAMPLE_APP) to use this parsed boolean so only true-like values enable the
sample routes.
🧹 Nitpick comments (7)
tests/media/.gitignore (1)
1-3: Optional: Add a comment explaining the intent.Consider adding a comment to clarify why
floorplan.jpgis tracked, which will help future maintainers understand this is a deliberate test fixture.✨ Proposed enhancement (optional)
* !.gitignore +# Track test fixtures for sample_connection tests !floorplan.jpgtests/openwisp2/sample_connection/admin.py (1)
1-1: Use a specificnoqacode for clarity.The blanket
noqadirective can be replaced with a more specific one to clarify intent.Suggested fix
-from openwisp_controller.connection import admin # noqa +from openwisp_controller.connection import admin # noqa: F401tests/openwisp2/sample_connection/pytest.py (1)
1-10: Consider droppingdel BaseTestCommandsConsumerunless it’s solving a real collection issue.Since the imported class is aliased to
BaseTestCommandsConsumer(notTest*), pytest shouldn’t collect it anyway; thedeladds a bit of surprise/IDE friction. If you’ve seen unwanted discovery, a short comment explaining why would help.tests/openwisp2/sample_connection/models.py (1)
10-29: Prefer avoidingnull=TrueonCharFieldunless you need tri-state semantics.If
detailsdoesn’t need to distinguishNULLvs"", considernull=False+default=""to simplify queries/fixtures.Proposed change
class DetailsModel(models.Model): - details = models.CharField(max_length=64, blank=True, null=True) + details = models.CharField(max_length=64, blank=True, default="")tests/openwisp2/sample_connection/tests.py (1)
58-96: Useconfig_app_label(or a class attribute) instead of hardcoding"config"in notification assertions.This keeps the sample tests aligned with the broader “dynamic app label” direction in the PR.
Proposed tweak (one possible approach)
class TestNotifications(BaseTestNotifications): app_label = "sample_connection" + config_app_label = "config" @@ - config_app = "config" - device_url_path = reverse(f"admin:{config_app}_device_change", args=[self.d.id]) + device_url_path = reverse( + f"admin:{self.config_app_label}_device_change", args=[self.d.id] + )tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
186-193: Inconsistent model reference pattern between DeviceConnection and Command.
DeviceConnection.deviceuses a hardcoded string"config.Device"(line 191-192), whereasCommand.device(line 278-281) usesswapper.get_model_name("config", "Device"). For consistency and to support swappable Device models, consider using swapper here as well.♻️ Suggested consistency fix
( "device", models.ForeignKey( on_delete=django.db.models.deletion.CASCADE, - to="config.Device", + to=swapper.get_model_name("config", "Device"), ), ),tests/openwisp2/sample_connection/api/views.py (1)
1-18: Consider consolidating imports.Multiple separate import statements from the same module can be consolidated for improved readability.
♻️ Consolidated imports
-from openwisp_controller.connection.api.views import ( - CommandDetailsView as BaseCommandDetailsView, -) -from openwisp_controller.connection.api.views import ( - CommandListCreateView as BaseCommandListCreateView, -) -from openwisp_controller.connection.api.views import ( - CredentialDetailView as BaseCredentialDetailView, -) -from openwisp_controller.connection.api.views import ( - CredentialListCreateView as BaseCredentialListCreateView, -) -from openwisp_controller.connection.api.views import ( - DeviceConnectionDetailView as BaseDeviceConnectionDetailView, -) -from openwisp_controller.connection.api.views import ( - DeviceConnenctionListCreateView as BaseDeviceConnenctionListCreateView, -) +from openwisp_controller.connection.api.views import ( + CommandDetailsView as BaseCommandDetailsView, + CommandListCreateView as BaseCommandListCreateView, + CredentialDetailView as BaseCredentialDetailView, + CredentialListCreateView as BaseCredentialListCreateView, + DeviceConnectionDetailView as BaseDeviceConnectionDetailView, + DeviceConnenctionListCreateView as BaseDeviceConnenctionListCreateView, +)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/media/floorplan.jpgis excluded by!**/*.jpg
📒 Files selected for processing (20)
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/media/.gitignoretests/openwisp2/sample_connection/__init__.pytests/openwisp2/sample_connection/admin.pytests/openwisp2/sample_connection/api/__init__.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/apps.pytests/openwisp2/sample_connection/migrations/0001_initial.pytests/openwisp2/sample_connection/migrations/0002_default_group_permissions.pytests/openwisp2/sample_connection/migrations/0003_name_unique_per_organization.pytests/openwisp2/sample_connection/migrations/__init__.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/pytest.pytests/openwisp2/sample_connection/tests.pytests/openwisp2/sample_firmware_upgrader/tests.pytests/openwisp2/settings.pytests/openwisp2/urls.py
🧰 Additional context used
🧬 Code graph analysis (6)
openwisp_firmware_upgrader/tests/test_selenium.py (2)
openwisp_firmware_upgrader/models.py (3)
BatchUpgradeOperation(37-40)UpgradeOperation(43-46)DeviceFirmware(31-34)openwisp_firmware_upgrader/upgraders/openwrt.py (1)
OpenWrt(23-530)
openwisp_firmware_upgrader/tests/test_tasks.py (2)
openwisp_firmware_upgrader/base/models.py (2)
upgrade(523-528)upgrade(683-776)openwisp_firmware_upgrader/tasks.py (2)
batch_upgrade_operation(44-59)upgrade_firmware(24-40)
tests/openwisp2/sample_connection/migrations/0002_default_group_permissions.py (2)
tests/openwisp2/sample_connection/migrations/0001_initial.py (1)
Migration(21-292)tests/openwisp2/sample_connection/migrations/0003_name_unique_per_organization.py (1)
Migration(6-20)
tests/openwisp2/urls.py (1)
openwisp_firmware_upgrader/api/views.py (1)
get(94-105)
tests/openwisp2/sample_firmware_upgrader/tests.py (1)
openwisp_firmware_upgrader/tests/test_selenium.py (1)
TestDeviceAdmin(29-331)
tests/openwisp2/sample_connection/migrations/0001_initial.py (3)
tests/openwisp2/sample_connection/migrations/0002_default_group_permissions.py (1)
Migration(9-19)tests/openwisp2/sample_connection/migrations/0003_name_unique_per_organization.py (1)
Migration(6-20)openwisp_firmware_upgrader/swapper.py (1)
get_model_name(11-12)
🪛 Ruff (0.14.10)
openwisp_firmware_upgrader/tests/test_admin.py
526-526: Unused method argument: args
(ARG002)
558-558: Unused method argument: args
(ARG002)
612-612: Unused method argument: args
(ARG002)
690-690: Unused method argument: args
(ARG002)
728-728: Unused method argument: args
(ARG002)
742-742: Unused method argument: args
(ARG002)
756-756: Unused method argument: args
(ARG002)
773-773: Unused method argument: args
(ARG002)
793-793: Unused method argument: args
(ARG002)
850-850: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/tests/test_selenium.py
198-198: Unused method argument: args
(ARG002)
277-277: Unused method argument: args
(ARG002)
279-279: Unpacked variable org is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
279-279: Unpacked variable category is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
279-279: Unpacked variable build1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
279-279: Unpacked variable image1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
openwisp_firmware_upgrader/tests/test_tasks.py
40-40: Unused method argument: args
(ARG002)
52-52: Unused method argument: args
(ARG002)
62-62: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/tests/test_models.py
510-510: Unused method argument: args
(ARG002)
534-534: Unused method argument: args
(ARG002)
tests/openwisp2/sample_connection/admin.py
1-1: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
tests/openwisp2/sample_connection/migrations/0002_default_group_permissions.py
10-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-19: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_connection/migrations/0003_name_unique_per_organization.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-20: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_connection/migrations/0001_initial.py
24-30: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-292: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (16)
tests/media/.gitignore (1)
1-3: LGTM!The .gitignore correctly excludes all files in the media directory while intentionally tracking the test fixture
floorplan.jpgand the configuration file itself. Pattern order is correct for Git semantics.tests/openwisp2/sample_connection/apps.py (1)
1-9: LGTM!The
SampleConnectionConfigcorrectly extendsConnectionConfigwith appropriatenameandlabelattributes. Thedel ConnectionConfigpattern is a valid Django convention to prevent namespace pollution after subclassing.openwisp_firmware_upgrader/tests/test_models.py (2)
491-507: LGTM!The refactored context-manager pattern for mocking
DeviceConnection.connectcorrectly scopes the mock to the test's active code path. The*argsparameter captures decorator-injected mocks and is intentionally unused.
509-587: Consistent mocking pattern applied across all batch upgrade tests.The context-manager approach is consistently applied in
test_upgrade_related_devices,test_upgrade_firmwareless_devices, andtest_upgrade_related_devices_existing_fw. Test logic remains unchanged.tests/openwisp2/settings.py (1)
219-231: LGTM!The sample_connection app integration follows the established pattern from sample_firmware_upgrader. The app replacement in
INSTALLED_APPS, addition toEXTENDED_APPS, and swapper model settings are correctly configured.tests/openwisp2/sample_connection/migrations/0003_name_unique_per_organization.py (1)
1-20: LGTM!This migration correctly transitions the
credentials.namefield from globally unique to organization-scoped uniqueness viaunique_together. This is the expected pattern for multi-tenant credential management. The Ruff RUF012 warnings are false positives for Django migration files.tests/openwisp2/sample_connection/migrations/0002_default_group_permissions.py (1)
1-19: LGTM!The data migration correctly reuses permission assignment functions from
openwisp_controller.connection.migrations. Usingmigrations.RunPython.noopfor reverse is appropriate as permission removal on rollback is typically undesired.openwisp_firmware_upgrader/tests/test_tasks.py (1)
26-68: LGTM!The context-manager mocking pattern for
DeviceConnection.connectis consistently applied across all tests. The*argsparameter intentionally captures decorator-injected mocks and is a common pattern when the mock objects aren't explicitly referenced in test code.tests/openwisp2/urls.py (1)
10-49: Verify theapi/v1/include ordering andget_connection_api_urls(...)compatibility.Because this relies on
openwisp-controller==1.3.0URL factory behavior and you now have multipleapi/v1/includes, please confirm the intended routes are reachable and none are shadowed (especially if any patterns overlap).openwisp_firmware_upgrader/tests/test_admin.py (1)
48-114: Dynamic admin URL construction viaconfig_app_labellooks good.Also applies to: 188-196, 358-413
tests/openwisp2/sample_connection/tests.py (1)
98-112: The test code is likely correct as-is. This code is inheriting from a base test class in an external OpenWISP package and using Django REST Framework, which is installed in this project. REST Framework's test client properly handles JSON serialization when a dict is passed toclient.post()withcontent_type="application/json", or the code may be using the modernjson=parameter pattern. The fact that the test passes in CI (as confirmed by the workflow configuration showing successful test runs) strongly indicates the JSON is being serialized correctly. The review concern applies primarily to standard DjangoTestCaseusage, but does not account for REST Framework'sAPIClientbehavior.Likely an incorrect or invalid review comment.
tests/openwisp2/sample_connection/migrations/0001_initial.py (3)
1-31: LGTM on migration setup and dependencies.The migration correctly declares dependencies using swapper for flexible model resolution, ensuring proper migration ordering across swappable apps.
246-256: Django version-conditional choices pattern is correct.The version check
django.VERSION < (5, 0)properly handles Django 5.0+'s preference for callable choices viaget_command_choicesversus the staticCOMMAND_CHOICEStuple for earlier versions.
206-291: Command model configuration looks good.The Command model correctly uses swapper for both the
connectionForeignKey and theswappablesetting, enabling proper model substitution in extended apps.tests/openwisp2/sample_connection/api/views.py (2)
21-50: LGTM on view class structure.The pass-through subclasses and
as_view()instantiation follow the standard pattern for Django REST framework view extension in sample/test apps. This allows customization points while inheriting all base functionality.
16-18: Inherited typo from upstream base class.
DeviceConnenctionListCreateViewcontains a typo ("nnection" instead of "nection") from the upstreamopenwisp_controller.connection.api.viewsmodule. This sample app correctly mirrors the upstream class name, and the variabledeviceconnection_list_create_viewuses the correct spelling.Consider filing an issue or fixing the typo upstream in
openwisp_controllerto maintain naming consistency across the codebase.
| from openwisp_controller.connection.api.urls import ( | ||
| get_api_urls as get_connection_api_urls, | ||
| ) | ||
| from openwisp_users.api.urls import get_api_urls | ||
|
|
||
| from .sample_connection.api import views as connection_api_views | ||
|
|
||
| redirect_view = RedirectView.as_view(url=reverse_lazy("admin:index")) | ||
|
|
||
| urlpatterns = [ | ||
| urlpatterns = [] | ||
|
|
||
| if os.environ.get("SAMPLE_APP", False): | ||
| urlpatterns += [ | ||
| path( | ||
| "", | ||
| include(("openwisp_controller.config.urls", "config"), namespace="config"), | ||
| ), | ||
| path( | ||
| "api/v1/", | ||
| include( | ||
| ( | ||
| get_connection_api_urls(connection_api_views), | ||
| "connection_api", | ||
| ), | ||
| namespace="connection_api", | ||
| ), | ||
| ), | ||
| ] | ||
|
|
There was a problem hiding this comment.
SAMPLE_APP env var parsing is likely wrong for "0" / "false" values.
os.environ.get("SAMPLE_APP", False) yields a string when set, so SAMPLE_APP="0" will still enable the sample routes. Consider explicit parsing.
Proposed fix
+def _env_truthy(name: str) -> bool:
+ v = os.environ.get(name)
+ if v is None:
+ return False
+ return v.strip().lower() in {"1", "true", "yes", "on"}
+
-urlpatterns = []
-
-if os.environ.get("SAMPLE_APP", False):
+urlpatterns = []
+
+if _env_truthy("SAMPLE_APP"):
urlpatterns += [
path(
"",
include(("openwisp_controller.config.urls", "config"), namespace="config"),
),
path(
"api/v1/",
include(
(
get_connection_api_urls(connection_api_views),
"connection_api",
),
namespace="connection_api",
),
),
]🤖 Prompt for AI Agents
In @tests/openwisp2/urls.py around lines 10 - 38, The SAMPLE_APP check uses
os.environ.get("SAMPLE_APP", False) which returns a string so values like "0" or
"false" are treated as truthy; change the condition to explicitly parse the env
var string (e.g., read value = os.environ.get("SAMPLE_APP", "") and test
value.lower() in ("1","true","yes") or use a canonical str-to-bool helper)
before appending to urlpatterns; update the conditional around urlpatterns (the
if that references SAMPLE_APP) to use this parsed boolean so only true-like
values enable the sample routes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/openwisp2/sample_connection/tests.py:
- Around line 68-81: TestConnectionApi is missing the class attribute app_label
= "sample_connection" which other tests in this file set; add that attribute to
the TestConnectionApi class definition (class
TestConnectionApi(BaseTestConnectionApi)) so it matches the other tests, and
re-run tests to ensure the hardcoded with self.assertNumQueries(13): still
reflects expected DB queries—update or remove the assertNumQueries invocation in
TestConnectionApi if the query count changes.
🧹 Nitpick comments (1)
tests/openwisp2/urls.py (1)
10-15: Unconditional imports used only in conditional block.The imports for
get_connection_api_urlsandconnection_api_viewson lines 10-15 are always executed, but only used whenSAMPLE_APPis set. This is fine since this is a test module andsample_connectionalways exists in the test suite, but consider moving these imports inside the conditional block for clarity and slightly faster startup whenSAMPLE_APPis not set.♻️ Suggested refactor
-from openwisp_controller.connection.api.urls import ( - get_api_urls as get_connection_api_urls, -) from openwisp_users.api.urls import get_api_urls -from .sample_connection.api import views as connection_api_views - redirect_view = RedirectView.as_view(url=reverse_lazy("admin:index")) urlpatterns = [] if os.environ.get("SAMPLE_APP", False): + from openwisp_controller.connection.api.urls import ( + get_api_urls as get_connection_api_urls, + ) + + from .sample_connection.api import views as connection_api_views + urlpatterns += [Also applies to: 21-37
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/openwisp2/sample_connection/tests.pytests/openwisp2/urls.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/openwisp2/sample_connection/tests.py (2)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/api/views.py (1)
post(85-92)
⏰ 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.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.13 | 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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🔇 Additional comments (4)
tests/openwisp2/urls.py (1)
39-48: LGTM!The URL pattern structure is correct. The conditional sample app patterns are prepended, followed by the common admin, redirect, and API routes. The use of
+=to extendurlpatternsis appropriate.tests/openwisp2/sample_connection/tests.py (3)
30-52: LGTM!The test class structure properly extends base test classes with appropriate
app_labelandconfig_app_labelattributes for the sample_connection app context. The pattern of only overriding necessary attributes is clean.
84-92: Good practice: cleaning up the namespace.Deleting the base test classes from the module namespace prevents pytest/unittest from discovering and running them as separate test cases in this module.
55-55: I cannot provide a rewritten review comment because no review comment content was provided in the<review_comment>tags. Please provide:
- The review comment you want me to rewrite
- Any relevant code context or repository information needed for verification
Once you provide these details, I will rewrite the review comment in the required format and output only the rewritten comment with its classification tag between the
<!-- result-start -->and<!-- result-end -->markers.
| class TestConnectionApi(BaseTestConnectionApi): | ||
| def test_post_deviceconnection_list(self): | ||
| d1 = self._create_device() | ||
| self._create_config(device=d1) | ||
| path = reverse("connection_api:deviceconnection_list", args=(d1.pk,)) | ||
| data = { | ||
| "credentials": self._get_credentials().pk, | ||
| "update_strategy": conn_settings.UPDATE_STRATEGIES[0][0], | ||
| "enabled": True, | ||
| "failure_reason": "", | ||
| } | ||
| with self.assertNumQueries(13): | ||
| response = self.client.post(path, data, content_type="application/json") | ||
| self.assertEqual(response.status_code, 201) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the base class implementation
rg -n "class BaseTestConnectionApi|app_label|test_post_deviceconnection" --type py -B2 -A5 openwisp_controller/connection/tests/test_api.py 2>/dev/null | head -80Repository: openwisp/openwisp-firmware-upgrader
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the sample_connection tests directory structure
fd . tests/openwisp2/sample_connection/ --type f -name "*.py" | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Look at the target file to understand context
cat -n tests/openwisp2/sample_connection/tests.py | head -100Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3492
🏁 Script executed:
#!/bin/bash
# Search for app_label usage in sample_connection test files
rg "app_label" tests/openwisp2/sample_connection/ --type py -B2 -A2Repository: openwisp/openwisp-firmware-upgrader
Length of output: 1958
🏁 Script executed:
#!/bin/bash
# Search for the base test class and related tests
rg "class BaseTestConnectionApi|test_post_deviceconnection_list|assertNumQueries" openwisp_controller/connection/tests/test_api.py --type py -B3 -A10 | head -100Repository: openwisp/openwisp-firmware-upgrader
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if similar test classes in openwisp_controller use app_label
rg "class.*BaseTestConnectionApi|class.*BaseTestModels|app_label" openwisp_controller/connection/tests/ --type py | head -30Repository: openwisp/openwisp-firmware-upgrader
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look for test_post_deviceconnection or similar methods in base
rg "def test_post_deviceconnection|assertNumQueries\(" openwisp_controller/connection/tests/test_api.py --type py -B2 -A8Repository: openwisp/openwisp-firmware-upgrader
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or setup.py that explains dependencies
cat openwisp_firmware_upgrader/sample_connection/tests.py 2>/dev/null | head -50Repository: openwisp/openwisp-firmware-upgrader
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Look at the structure and find any documentation about test setup
ls -la tests/openwisp2/sample_connection/Repository: openwisp/openwisp-firmware-upgrader
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation in the tests directory
rg "app_label|BaseTestConnectionApi" tests/openwisp2/sample_connection/ --type py -B3 -A3Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3308
Add app_label = "sample_connection" to TestConnectionApi for consistency.
All other test classes in this file that inherit from base connection test classes (TestConnectionAdmin, TestModels, TestNotifications, etc.) set app_label = "sample_connection". TestConnectionApi should follow the same pattern.
The hardcoded assertNumQueries(13) is fragile and may need adjustment if the database query implementation changes; consider verifying this value matches expectations if the base test method is modified.
🤖 Prompt for AI Agents
In @tests/openwisp2/sample_connection/tests.py around lines 68 - 81,
TestConnectionApi is missing the class attribute app_label = "sample_connection"
which other tests in this file set; add that attribute to the TestConnectionApi
class definition (class TestConnectionApi(BaseTestConnectionApi)) so it matches
the other tests, and re-run tests to ensure the hardcoded with
self.assertNumQueries(13): still reflects expected DB queries—update or remove
the assertNumQueries invocation in TestConnectionApi if the query count changes.
…t_one # Conflicts: # openwisp_firmware_upgrader/tests/test_admin.py # openwisp_firmware_upgrader/tests/test_selenium.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/tests/test_admin.py (1)
659-660:⚠️ Potential issue | 🟠 MajorPatch target should resolve from the swapped
DeviceConnectionmodel, not a fixed module path.Using a literal default-model path can miss the effective model in extended/swap scenarios and let real
connect()behavior leak into tests.Proposed fix
- _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" + _mock_connect = ( + f"{DeviceConnection.__module__}.{DeviceConnection.__name__}.connect" + )#!/bin/bash set -euo pipefail # Verify which DeviceConnection model is active in tests and whether connect() is overridden. rg -n "swapper.load_model\\(\"connection\", \"DeviceConnection\"\\)|CONNECTION_.*MODEL|SWAPPABLE" tests openwisp_firmware_upgrader -g "*.py" -C2 # Inspect sample/extended connection model definitions. fd "models.py" tests/openwisp2 | xargs -I{} sh -c 'echo "## {}"; rg -n "class .*DeviceConnection|def connect\\(" "{}" -C2' # Locate hardcoded connect patch targets in tests. rg -n "openwisp_controller\\.connection\\.models\\.DeviceConnection\\.connect|_mock_connect" openwisp_firmware_upgrader/tests -C1Also applies to: 664-665
🤖 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 659 - 660, The test currently patches a hardcoded "openwisp_controller.connection.models.DeviceConnection.connect" via _mock_connect which breaks when the DeviceConnection model is swapped; instead, import swapper (or Django's apps.get_model) in the test and resolve the active model with swapper.load_model("connection", "DeviceConnection"), then build the patch target from the resolved model's __module__ and __name__ and patch "<resolved_module>.<resolved_class>.connect" (use that dynamic target everywhere _mock_connect is used, e.g., replace references to _mock_connect with the resolved target string).openwisp_firmware_upgrader/tests/test_selenium.py (1)
44-44:⚠️ Potential issue | 🟠 MajorUse a swapped-model-aware
_mock_connecttarget in this class too.The fixed default-model path is brittle under connection-app extension/swap; resolve from the loaded
DeviceConnectionclass.Proposed fix
- _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" + _mock_connect = ( + f"{DeviceConnection.__module__}.{DeviceConnection.__name__}.connect" + )Also applies to: 130-130, 209-210, 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_selenium.py` at line 44, _the _mock_connect target currently uses a hard-coded model path which breaks when the connection app/model is swapped; replace the fixed string with a dynamic target built from the loaded DeviceConnection class (e.g. construct the target as f"{DeviceConnection.__module__}.{DeviceConnection.__name__}.connect" after importing or resolving DeviceConnection) and use that value for _mock_connect in this test class; apply the same change to the other occurrences of _mock_connect in this file (the other two places referenced) so the mock respects swapped models.
🤖 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`:
- Line 55: Replace the hard-coded connection_app_label = "connection" with a
dynamic lookup from the model meta (e.g., connection_app_label =
Build._meta.app_label or Device._meta.app_label depending on which model the
test uses); import the referenced model (Build or Device) at the top of the test
module if not already imported and use its _meta.app_label to construct admin
URL namespaces so swapped models remain correct.
---
Duplicate comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 659-660: The test currently patches a hardcoded
"openwisp_controller.connection.models.DeviceConnection.connect" via
_mock_connect which breaks when the DeviceConnection model is swapped; instead,
import swapper (or Django's apps.get_model) in the test and resolve the active
model with swapper.load_model("connection", "DeviceConnection"), then build the
patch target from the resolved model's __module__ and __name__ and patch
"<resolved_module>.<resolved_class>.connect" (use that dynamic target everywhere
_mock_connect is used, e.g., replace references to _mock_connect with the
resolved target string).
In `@openwisp_firmware_upgrader/tests/test_selenium.py`:
- Line 44: _the _mock_connect target currently uses a hard-coded model path
which breaks when the connection app/model is swapped; replace the fixed string
with a dynamic target built from the loaded DeviceConnection class (e.g.
construct the target as
f"{DeviceConnection.__module__}.{DeviceConnection.__name__}.connect" after
importing or resolving DeviceConnection) and use that value for _mock_connect in
this test class; apply the same change to the other occurrences of _mock_connect
in this file (the other two places referenced) so the mock respects swapped
models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25bb800c-3349-4bd5-9826-59695cc40f29
📒 Files selected for processing (4)
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/settings.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.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (7)
📓 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: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
📚 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_selenium.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_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_selenium.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-01-29T18:09:27.769Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/settings.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pytests/openwisp2/settings.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_selenium.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
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.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (3)
tests/openwisp2/settings.py (1)
242-254: LGTM!The sample_connection app extension follows the established pattern used for sample_firmware_upgrader. The implementation correctly:
- Swaps
openwisp_controller.connectionwithopenwisp2.sample_connectioninINSTALLED_APPS- Adds the original app to
EXTENDED_APPS- Configures the swapper model settings to point to the sample_connection models
The model settings (
CONNECTION_CREDENTIALS_MODEL,CONNECTION_DEVICECONNECTION_MODEL,CONNECTION_COMMAND_MODEL) correctly reference the models defined intests/openwisp2/sample_connection/models.py.openwisp_firmware_upgrader/tests/test_models.py (1)
548-562: Good mock scoping improvement.Switching to per-test
with mock.patch(...)blocks improves isolation and avoids patch bleed between transaction tests.Also applies to: 566-587, 590-610, 622-643
openwisp_firmware_upgrader/tests/test_selenium.py (1)
113-117: Dynamic changelist URL assertion is a solid fix.Building the expected URL with
self.live_server_url + reverse(...)makes the redirect assertion environment-safe.
| class BaseTestAdmin(TestMultitenantAdminMixin, TestUpgraderMixin): | ||
| app_label = Build._meta.app_label | ||
| config_app_label = Device._meta.app_label | ||
| connection_app_label = "connection" |
There was a problem hiding this comment.
Make connection_app_label swapped-model-safe.
Use model metadata here to keep admin URL namespace construction aligned when the connection app is swapped.
Proposed fix
- connection_app_label = "connection"
+ connection_app_label = DeviceConnection._meta.app_labelBased on learnings: Base test labels should be derived from model meta (Build._meta.app_label / Device._meta.app_label) rather than hard-coded so swapped models keep tests correct.
📝 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.
| connection_app_label = "connection" | |
| connection_app_label = DeviceConnection._meta.app_label |
🤖 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` at line 55, Replace the
hard-coded connection_app_label = "connection" with a dynamic lookup from the
model meta (e.g., connection_app_label = Build._meta.app_label or
Device._meta.app_label depending on which model the test uses); import the
referenced model (Build or Device) at the top of the test module if not already
imported and use its _meta.app_label to construct admin URL namespaces so
swapped models remain correct.
There was a problem hiding this comment.
This seems unused? maybe in other files ? can you double check ?
There was a problem hiding this comment.
🧩 Analysis chain
Let me search the codebase to check if connection_app_label is used anywhere.
🏁 Script executed:
#!/bin/bash
rg -n "connection_app_label" --type py -C 3
Length of output: 636
@asmodehn, you're right — confirmed. connection_app_label is only defined in BaseTestAdmin and is never referenced anywhere else in the codebase (no test methods, no subclasses, no other files use it).
So there are two options:
- Remove it entirely — if it was added speculatively and there's no immediate plan to use it.
- Keep and use it — if there are places in the test suite that currently hard-code the string
"connection"in admin URL reversals (e.g.reverse("admin:connection_...")) that should be made swapper-safe. In that case it would need to be wired up consistently, usingDeviceConnection._meta.app_labelas the value.
Given the PR's incremental scope, removing it now and adding it back (properly wired) in a follow-up seems like the cleaner path. Would you like to go ahead with removing it?
🧠 Learnings used
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: 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.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/tests/test_admin.py (2)
659-663:⚠️ Potential issue | 🟠 MajorPatch the swapped
DeviceConnectionclass, not a hard-coded module path.Line 659 hard-codes
openwisp_controller...DeviceConnection.connect, but this module already resolvesDeviceConnectionviaswapper(Line 45). In extended-app mode, this can patch the wrong class method.Proposed fix
- _mock_connect = "openwisp_controller.connection.models.DeviceConnection.connect" + _mock_connect = f"{DeviceConnection.__module__}.DeviceConnection.connect"#!/bin/bash set -euo pipefail # Verify swapped DeviceConnection usage in this test module. rg -n 'swapper\.load_model\("connection",\s*"DeviceConnection"\)' openwisp_firmware_upgrader/tests/test_admin.py -C2 # Verify test settings/model wiring for sample_connection or connection model swaps. fd -t f "settings.py" tests | while read -r f; do rg -n 'CONNECTION_.*DeviceConnection|sample_connection|swapper' "$f" || true done # Inspect DeviceConnection class/connect definitions in test apps. fd -t f "models.py" tests | while read -r f; do rg -n 'class\s+DeviceConnection\b|def\s+connect\s*\(' "$f" -C2 || true done🤖 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 659 - 663, The test currently hard-codes _mock_connect to "openwisp_controller.connection.models.DeviceConnection.connect", which can patch the wrong class when swaps are used; change the test to patch the actual swapped class instead: obtain the DeviceConnection class via swapper.load_model("connection", "DeviceConnection") (or call swapper.load_model once and reuse it) and then patch its connect method (e.g., patch the attribute on the returned class or construct the module-qualified path from its __module__ and __name__). Update references in test_upgrade_selected_action_perms and the _mock_connect setup to use the swapped class rather than the static string.
863-866: 🛠️ Refactor suggestion | 🟠 MajorAvoid calling one test method from another test method.
Line 865 (
self.test_upgrade_all()) couples tests and nests decorators/patches, which makes failures harder to localize. Extract shared setup/execution into a helper and call that helper from both tests.Refactor direction
+ def _run_upgrade_all_flow(self): + # shared flow currently inside test_upgrade_all + ... `@mock.patch`(_mock_upgrade, return_value=True) def test_upgrade_all(self, *args): - with mock.patch(self._mock_connect, return_value=True): - ... + self._run_upgrade_all_flow() `@mock.patch`(_mock_upgrade, return_value=True) def test_massive_upgrade_operation_page(self, *args): with mock.patch(self._mock_connect, return_value=True): - self.test_upgrade_all() + self._run_upgrade_all_flow() uo = UpgradeOperation.objects.first()#!/bin/bash set -euo pipefail # Verify test-to-test invocations in this module. rg -nP 'self\.test_[A-Za-z0-9_]+\(' openwisp_firmware_upgrader/tests/test_admin.py -C2🤖 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 863 - 866, The test calls another test method (self.test_upgrade_all())—remove this coupling by extracting the common setup/execution code from test_upgrade_all into a shared helper (e.g., a private method like _prepare_and_run_upgrade or _create_upgrade_operations) and have both test_upgrade_all and test_massive_upgrade_operation_page call that helper instead; ensure any mocks/patches (like the one using self._mock_connect) are applied in the individual tests or passed into the helper as parameters so you do not nest decorators/patches when invoking the shared logic, then replace the direct self.test_upgrade_all() call with a call to the new helper and use UpgradeOperation.objects.first() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 659-663: The test currently hard-codes _mock_connect to
"openwisp_controller.connection.models.DeviceConnection.connect", which can
patch the wrong class when swaps are used; change the test to patch the actual
swapped class instead: obtain the DeviceConnection class via
swapper.load_model("connection", "DeviceConnection") (or call swapper.load_model
once and reuse it) and then patch its connect method (e.g., patch the attribute
on the returned class or construct the module-qualified path from its __module__
and __name__). Update references in test_upgrade_selected_action_perms and the
_mock_connect setup to use the swapped class rather than the static string.
- Around line 863-866: The test calls another test method
(self.test_upgrade_all())—remove this coupling by extracting the common
setup/execution code from test_upgrade_all into a shared helper (e.g., a private
method like _prepare_and_run_upgrade or _create_upgrade_operations) and have
both test_upgrade_all and test_massive_upgrade_operation_page call that helper
instead; ensure any mocks/patches (like the one using self._mock_connect) are
applied in the individual tests or passed into the helper as parameters so you
do not nest decorators/patches when invoking the shared logic, then replace the
direct self.test_upgrade_all() call with a call to the new helper and use
UpgradeOperation.objects.first() as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eab7bd0d-a963-42fb-8cd0-2dbbf2bbb279
📒 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.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | 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~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
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-02-25T00:17:23.479Z
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.
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
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/tests/test_admin.py (1)
661-694: Per-test mock scoping is a good improvement.Using method-level
@mock.patch(_mock_upgrade, ...)and local connect patch contexts reduces cross-test mock leakage.
|
Switching to draft again, as I need to make it work again with recent changes on master, some tests are failing in non-obvious ways. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_firmware_upgrader/tests/test_models.py (1)
557-557:⚠️ Potential issue | 🟡 MinorTypo in class attribute name:
_mock_updradeshould be_mock_upgrade.This typo could cause confusion when reading the code or when someone tries to reference this attribute consistently across test files (other files use
_mock_upgrade).🔧 Proposed fix
- _mock_updrade = "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade" + _mock_upgrade = "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade"Also update references on lines 562, 580, 604, 636:
- `@mock.patch`(_mock_updrade, return_value=True) + `@mock.patch`(_mock_upgrade, 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_models.py` at line 557, Rename the mistyped test attribute `_mock_updrade` to `_mock_upgrade` and update every reference to it in this test module (occurrences currently referencing `_mock_updrade` around other test helper usages and mock patches) so they use `_mock_upgrade`; ensure any patch strings or attribute accesses that reference `openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade` remain correct and only the attribute name changes to `_mock_upgrade` for consistency with other tests.openwisp_firmware_upgrader/tests/test_selenium.py (1)
462-481: 🧹 Nitpick | 🔵 TrivialConsider using
self._mock_connectfor consistency.This test uses decorator-style patching with a hardcoded path while the rest of the file uses
self._mock_connect. For consistency with the refactoring pattern applied elsewhere, consider using the class attribute.Suggested change
`@patch`( "openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade", return_value=True, ) - `@patch`( - "openwisp_controller.connection.models.DeviceConnection.connect", - return_value=True, - ) - def test_upgrade_operation_admin_no_submit_row(self, *args): + def test_upgrade_operation_admin_no_submit_row(self, _mock_upgrade): """Test that UpgradeOperation admin change page does not display submit-row""" - # Create device firmware and upgrade - self._create_device_firmware(upgrade=True) + with patch(self._mock_connect, return_value=True): + # Create device firmware and upgrade + self._create_device_firmware(upgrade=True) + ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_selenium.py` around lines 462 - 481, The test test_upgrade_operation_admin_no_submit_row currently uses a decorator patch on DeviceConnection.connect and accepts *args; replace that with the existing class attribute mock self._mock_connect for consistency: remove the `@patch` for "openwisp_controller.connection.models.DeviceConnection.connect" and the extra method arg, ensure self._mock_connect.return_value = True (or set it in test setup) before running the test, and keep the OpenWrt.upgrade patch (or switch to the class-level mock if one exists) so the test still creates the UpgradeOperation and asserts the submit-row is hidden.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/tests/test_admin.py (2)
662-662: 🧹 Nitpick | 🔵 TrivialUnused
*argsparameters should be named explicitly.The test methods decorated with
@mock.patchreceive the mock object as an argument, but*argsis unused. For clarity and to satisfy linting (ARG002), use explicit parameter names.Example fix pattern
- def test_upgrade_selected_action_perms(self, *args): + def test_upgrade_selected_action_perms(self, _mock_upgrade):Apply similarly to other methods:
test_upgrade_related,test_upgrade_all,test_mass_upgrade_shared_image, etc.Also applies to: 693-693, 747-747
🤖 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` at line 662, The test method test_upgrade_selected_action_perms (and the other tests noted: test_upgrade_related, test_upgrade_all, test_mass_upgrade_shared_image) currently accept an unused *args from `@mock.patch`; replace the varargs with explicit mock parameter names matching the applied patches (e.g., patcher1_mock, patcher2_mock) so each mock is named and used or ignored intentionally, removing the generic *args to satisfy ARG002 and improve clarity; update the function signatures for test_upgrade_selected_action_perms and the other listed test methods to list the exact mock argument names provided by their `@mock.patch` decorators.
863-865: 🧹 Nitpick | 🔵 TrivialTest-calling-test pattern creates hidden coupling.
test_massive_upgrade_operation_pagecallsself.test_upgrade_all()directly, which couples test execution and creates hidden side effects. The same pattern appears intest_upgrade_operation_change_breadcrumb_with_batch(line 879) andtest_upgrade_operation_change_breadcrumb_with_batch_no_permission(line 920).Consider extracting the shared setup logic into a helper method like
_upgrade_all_scenario().🤖 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 863 - 865, The test calls another test method directly (test_massive_upgrade_operation_page calls self.test_upgrade_all()), creating hidden coupling and side effects; refactor by extracting the shared setup and assertions into a new helper method (e.g., _upgrade_all_scenario()) that encapsulates the setup used by test_upgrade_all and the other affected tests (test_upgrade_operation_change_breadcrumb_with_batch and test_upgrade_operation_change_breadcrumb_with_batch_no_permission), move the mock.patch(self._mock_connect, return_value=True) and any common fixtures/assertions into that helper, and have each test call _upgrade_all_scenario() instead of invoking another test method to eliminate inter-test coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Line 557: Rename the mistyped test attribute `_mock_updrade` to
`_mock_upgrade` and update every reference to it in this test module
(occurrences currently referencing `_mock_updrade` around other test helper
usages and mock patches) so they use `_mock_upgrade`; ensure any patch strings
or attribute accesses that reference
`openwisp_firmware_upgrader.upgraders.openwrt.OpenWrt.upgrade` remain correct
and only the attribute name changes to `_mock_upgrade` for consistency with
other tests.
In `@openwisp_firmware_upgrader/tests/test_selenium.py`:
- Around line 462-481: The test test_upgrade_operation_admin_no_submit_row
currently uses a decorator patch on DeviceConnection.connect and accepts *args;
replace that with the existing class attribute mock self._mock_connect for
consistency: remove the `@patch` for
"openwisp_controller.connection.models.DeviceConnection.connect" and the extra
method arg, ensure self._mock_connect.return_value = True (or set it in test
setup) before running the test, and keep the OpenWrt.upgrade patch (or switch to
the class-level mock if one exists) so the test still creates the
UpgradeOperation and asserts the submit-row is hidden.
---
Duplicate comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Line 662: The test method test_upgrade_selected_action_perms (and the other
tests noted: test_upgrade_related, test_upgrade_all,
test_mass_upgrade_shared_image) currently accept an unused *args from
`@mock.patch`; replace the varargs with explicit mock parameter names matching the
applied patches (e.g., patcher1_mock, patcher2_mock) so each mock is named and
used or ignored intentionally, removing the generic *args to satisfy ARG002 and
improve clarity; update the function signatures for
test_upgrade_selected_action_perms and the other listed test methods to list the
exact mock argument names provided by their `@mock.patch` decorators.
- Around line 863-865: The test calls another test method directly
(test_massive_upgrade_operation_page calls self.test_upgrade_all()), creating
hidden coupling and side effects; refactor by extracting the shared setup and
assertions into a new helper method (e.g., _upgrade_all_scenario()) that
encapsulates the setup used by test_upgrade_all and the other affected tests
(test_upgrade_operation_change_breadcrumb_with_batch and
test_upgrade_operation_change_breadcrumb_with_batch_no_permission), move the
mock.patch(self._mock_connect, return_value=True) and any common
fixtures/assertions into that helper, and have each test call
_upgrade_all_scenario() instead of invoking another test method to eliminate
inter-test coupling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7445ca5-cb1c-4131-8187-fb748c373fb5
📒 Files selected for processing (3)
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.py
📜 Review details
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
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-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_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 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_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_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_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_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_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-01-29T18:09:27.769Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_selenium.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_selenium.py
📚 Learning: 2026-02-25T00:17:23.479Z
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.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (8)
openwisp_firmware_upgrader/tests/test_models.py (1)
564-578: LGTM - Context-manager mocking pattern correctly applied.The refactoring to use
with mock.patch(self._mock_connect, return_value=True):blocks within tests properly scopes the connection mock to specific test blocks. This pattern is consistent with the broader PR changes across other test files.openwisp_firmware_upgrader/tests/test_admin.py (2)
658-659: LGTM - Class-level mock attributes added correctly.The addition of
_mock_upgradeand_mock_connectas class attributes follows the same pattern established in other test files, enabling consistent and maintainable mock path references.
950-999: LGTM - Context-manager mocking pattern applied consistently.The refactoring to use
with mock.patch(self._mock_connect, return_value=True):is correctly applied across all test methods. The test logic remains intact within the context managers.openwisp_firmware_upgrader/tests/test_selenium.py (5)
46-46: LGTM - Class-level mock attribute added for consistent mocking.The
_mock_connectattribute enables consistent mock path references and aligns with the refactoring pattern across other test files.
118-121: LGTM - Dynamic URL construction improves test robustness.Using
self.live_server_url + reverse(f"admin:{self.config_app_label}_device_changelist")instead of hard-coded URLs makes the test resilient to URL pattern changes and properly leverages theconfig_app_labelderived from model metadata.
133-205: LGTM - Well-structured test refactoring with internal helper.The
save_device()helper function reduces duplication within the test, and the context-manager pattern for_mock_connectproperly scopes the mock. The test logic remains intact with appropriate Selenium waits and visibility checks.
207-282: LGTM - Batch upgrade tests correctly refactored.The context-manager mocking pattern is properly applied, and the assertions correctly verify the
upgrade_optionsdictionary values for bothBatchUpgradeOperationandUpgradeOperationobjects.
284-351: LGTM - Subtests for unsupported upgrader options are well-structured.The test properly uses
@patch.object(OpenWrt, "SCHEMA", None)to simulate an upgrader without schema support, and the subtests cleanly separate DeviceFirmware and BatchUpgradeOperation scenarios with appropriate cleanup between them.
|
@coderabbitai See what changed in this PR, with the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me look more closely at the actual changes in the test files to give you a precise answer. 🏁 Script executed: Length of output: 8222 🏁 Script executed: Length of output: 4541
Identified separable changes1. ✅ Pure mock refactoring —
|
| PR | Contents | Why |
|---|---|---|
| PR A | Mock refactoring of test_admin.py, test_models.py, test_tasks.py, test_selenium.py (+ URL fix) |
Pure structural refactoring, ~1200 lines but virtually all indentation, easy to review, mergeable independently |
| PR B (this PR, rebased) | New sample_connection app, settings.py, urls.py, sample_firmware_upgrader/tests.py, tests/media/.gitignore |
Depends on PR A; all new files = low conflict risk |
The key benefit: if master changes and causes conflicts, they'll almost certainly land only in the new files (PR B), not in the refactoring (PR A) which has no logic changes.
|
I extracted the mentioned test_selenium change there : #396 |
|
I also extracted test_admin refactor in #397, as this PR has proven difficult to review and keep uptodate with all tests passing... I m planning to do the same with the other tests files, one at a time to avoid overloading reviewers. |
2 participants
Checklist
Reference to Existing Issue
This is a part of #303 which was too ambitious.
The goal here is to add a sample_connection extended app, to make sure the case where connection is an extended app is tested.
Making a sample_config extend app is trickier than expected, and will be done later.
Description of Changes
Added sample_connection extended app.
Based on #303 changes.
Aiming to get all tests to pass with this as a first step.