[tests] Fixed hardcoded config_app_label and app_label in device test_admin#748
Conversation
📝 WalkthroughWalkthroughThis change updates tests in openwisp_monitoring/device/tests/test_admin.py to remove hard-coded admin URL reversals and use per-class attributes for app labels. TestAdmin now defines public attributes Sequence Diagram(s)(Skipped — changes are test-only URL construction updates and do not introduce a new multi-component control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 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)
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_monitoring/device/tests/test_admin.py`:
- Around line 46-52: Remove the duplicate assignment to the module-level
variable app_label (it’s assigned twice to "config"); keep a single app_label =
"config" (the existing one is sufficient), and leave config_app_label = "config"
and the modification to resources_fields
(TestImportExportMixin.resource_fields.append("monitoring_status")) unchanged;
update the block that currently contains two app_label = "config" lines to only
include one.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/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). (11)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
🔇 Additional comments (2)
openwisp_monitoring/device/tests/test_admin.py (2)
125-125: LGTM!The dynamic URL construction using
self.config_app_labelenables test extensibility for derived apps while maintaining the same default behavior.
915-916: LGTM!Adding
config_app_labeltoTestWifiSessionAdmincorrectly enables dynamic URL construction for this test class. Since it doesn't inherit fromTestImportExportMixin, omittingapp_labelhere is appropriate.
asmodehn
force-pushed
the
config_app_label_in_monitoring_device_tests
branch
from
8986308 to
d923e30
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_monitoring/device/tests/test_admin.py (1)
46-52:⚠️ Potential issue | 🟠 MajorRemove the hardcoded
app_labeloverride.Line 52 (
app_label = "config") overrides the dynamic assignment at Line 47, which defeats the goal of making this test class derivable across apps.🔧 Proposed fix
class TestAdmin( TestWifiClientSessionMixin, TestImportExportMixin, DeviceMonitoringTestCase ): @@ # Since TestImportExportMixin expects app_label for config app app_label = Device._meta.app_label config_app_label = Device._meta.app_label @@ resources_fields = TestImportExportMixin.resource_fields resources_fields.append("monitoring_status") - app_label = "config" _device_params = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/tests/test_admin.py` around lines 46 - 52, The test currently overrides the dynamically set app_label by hardcoding app_label = "config"; remove that hardcoded assignment so app_label remains derived from Device._meta.app_label; ensure TestImportExportMixin.resource_fields is still appended with "monitoring_status" (reference: Device, TestImportExportMixin.resource_fields, monitoring_status, app_label) so the test stays derivable across apps.
🤖 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_monitoring/device/tests/test_admin.py`:
- Around line 46-52: The test currently overrides the dynamically set app_label
by hardcoding app_label = "config"; remove that hardcoded assignment so
app_label remains derived from Device._meta.app_label; ensure
TestImportExportMixin.resource_fields is still appended with "monitoring_status"
(reference: Device, TestImportExportMixin.resource_fields, monitoring_status,
app_label) so the test stays derivable across apps.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/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). (11)
- 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.1.0
- GitHub Check: Python==3.12 | 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.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
🔇 Additional comments (2)
openwisp_monitoring/device/tests/test_admin.py (2)
125-125: Good migration to dynamic admin URL construction.Using
self.config_app_labelin these reversals removes hard-coded app namespace coupling and makes test reuse across derived apps much cleaner.Also applies to: 196-196, 236-236, 261-261, 336-336, 342-342, 363-363, 382-382, 387-387, 394-394, 423-423, 491-491, 502-502, 538-538, 567-567, 612-612, 755-755, 870-877, 1060-1062, 1087-1089
915-915:config_app_labelonTestWifiSessionAdminis a good extensibility hook.Adding this class attribute makes downstream overrides straightforward where device admin namespace differs.
| TestWifiClientSessionMixin, | ||
| TestCase, | ||
| ): | ||
| config_app_label = "config" |
There was a problem hiding this comment.
I think this is missing right?
| config_app_label = "config" | |
| config_app_label = Device._meta.app_label |
There was a problem hiding this comment.
correct. I ll fix it asap.
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_monitoring/device/tests/test_admin.py`:
- Around line 46-48: Tests currently hardcode "device" in admin URL names and
paths; add a dynamic config_model_name = Device._meta.model_name and replace all
hardcoded "device" usages (e.g., admin:{self.config_app_label}_device_change,
device_changelist, and /admin/{self.config_app_label}/device/recover/) with the
model-name variable (use
admin:{self.config_app_label}_{self.config_model_name}_change,
{self.config_app_label}_{self.config_model_name}_changelist, and
/admin/{self.config_app_label}/{self.config_model_name}/recover/). Update every
occurrence noted in the review (including the blocks around lines 124-125, 341,
869-876, 914-915, 1059-1061, 1086-1088) to use config_model_name so the tests
work when a concrete Device model has a different name, following the pattern
used for WifiSession._meta.model_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d37804cc-09eb-468b-9c3b-a84db7d91a12
📒 Files selected for processing (1)
openwisp_monitoring/device/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_admin.py
| # Since TestImportExportMixin expects app_label for config app | ||
| app_label = Device._meta.app_label | ||
| config_app_label = Device._meta.app_label |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to understand the current implementation
cat -n openwisp_monitoring/device/tests/test_admin.py | head -150Repository: openwisp/openwisp-monitoring
Length of output: 7299
🏁 Script executed:
# Check lines around the mentioned segments
sed -n '40,130p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 4271
🌐 Web query:
Django admin URL names app_label model_name convention
💡 Result:
Django admin URL names for a model follow this convention (in the admin URL namespace):
admin:{app_label}_{model_name}_changelistadmin:{app_label}_{model_name}_addadmin:{app_label}_{model_name}_change(expectsargs=(object_id,))admin:{app_label}_{model_name}_delete(expectsargs=(object_id,))admin:{app_label}_{model_name}_history(expectsargs=(object_id,))
These are generated from ModelAdmin.get_urls() using info = (self.opts.app_label, self.opts.model_name) (note: model_name is the lowercased _meta.model_name). [1]
Example:
from django.urls import reverse
reverse("admin:auth_user_changelist")
reverse("admin:polls_choice_change", args=(obj.pk,))If you have a custom AdminSite(name="custom"), pass current_app="custom" to reverse the URL for that specific admin instance. [2]
Related: the per-app index is admin:app_list with kwargs={"app_label": "auth"}. [3]
Sources: Django admin source/docs for ModelAdmin.get_urls() and reversing admin URLs. [1][2][3]
🏁 Script executed:
# Check all the mentioned line ranges
sed -n '120,130p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '335,345p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '865,880p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '910,920p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 2339
🏁 Script executed:
# Check how check_model_name and metric_model_name are used
rg -n "check_model_name|metric_model_name" openwisp_monitoring/device/tests/test_admin.py | head -30Repository: openwisp/openwisp-monitoring
Length of output: 2836
🏁 Script executed:
# Search for TestWifiSessionAdmin class
sed -n '900,950p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 2539
Add dynamic model name for Device admin URLs.
Django admin URLs require both app_label and model_name. While the code makes app_label dynamic with config_app_label = Device._meta.app_label, it hardcodes "device" in all reversals and path expectations: admin:{self.config_app_label}_device_change, device_changelist, and /admin/{self.config_app_label}/device/recover/. If a derived app swaps config.Device with a differently named concrete model, these will break. Add config_model_name = Device._meta.model_name and use it consistently, following the same pattern already demonstrated in TestWifiSessionAdmin for WifiSession._meta.model_name (lines 917, 924-925).
Also applies to: 124-125, 341-341, 869-876, 914-915, 1059-1061, 1086-1088
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/tests/test_admin.py` around lines 46 - 48, Tests
currently hardcode "device" in admin URL names and paths; add a dynamic
config_model_name = Device._meta.model_name and replace all hardcoded "device"
usages (e.g., admin:{self.config_app_label}_device_change, device_changelist,
and /admin/{self.config_app_label}/device/recover/) with the model-name variable
(use admin:{self.config_app_label}_{self.config_model_name}_change,
{self.config_app_label}_{self.config_model_name}_changelist, and
/admin/{self.config_app_label}/{self.config_model_name}/recover/). Update every
occurrence noted in the review (including the blocks around lines 124-125, 341,
869-876, 914-915, 1059-1061, 1086-1088) to use config_model_name so the tests
work when a concrete Device model has a different name, following the pattern
used for WifiSession._meta.model_name.
|
Hi @asmodehn 👋, This pull request has been automatically closed due to 62 days of inactivity. After changes were requested, the PR remained inactive. We understand that life gets busy, and we appreciate your initial contribution! 💙 The door is always open for you to come back:
If you have any questions or need help, don't hesitate to reach out. We're here to support you! Thank you for your interest in contributing to OpenWISP! 🙏 |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
nemesifier
changed the title
2 participants
Checklist
Description of Changes
This adds config_app_label attrs in device test_admin tests to make them derivable in other django apps.
I currently also need to set app_label, since the derived TestImportExportMixin requires it... we could probably devise a slightly nicer way of dealing with it, like f.i. renaming the variable config_app_label (in controller) in TestImportExportMixin to make it more explicit which app it is referring to...