[fix] Fixed test environment pollution in TestAdminMedia #405#447
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe tests were updated to avoid polluting test state and to simplify script execution. In test_admin, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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_notifications/tests/test_admin.py`:
- Around line 343-358: The test assigns responses from self.client.get when
calling reverse for f"admin:{self.users_app_label}_user_change" after toggling
UserAdmin.Media but never asserts them; either replace the unused local name
with _ to show intentional discard or add explicit assertions (e.g., check
response.status_code == 200 and presence/absence of the widget JS/CSS in
response.content) after the calls that invoke _add_object_notification_widget
and self.client.get, referencing the UserAdmin.Media mutation,
_add_object_notification_widget(), reverse(...), self.client.get(...) and
self.admin.pk to locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35571361-ce5f-4601-82c3-00ac24765e05
📒 Files selected for processing (1)
openwisp_notifications/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). (14)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (3)
openwisp_notifications/tests/test_admin.py (3)
1-8: LGTM!The new imports are necessary and correctly used:
copyfor deep copying mutablejs/cssattributes, andMediaOrderConflictWarningfor asserting the expected warning when the widget is added multiple times.
316-322: LGTM - state preservation logic is correct.Deep copying the mutable
jsandcssattributes ensures they can be properly restored after in-place mutations by_add_object_notification_widget().Minor: For consistency with line 360's
if original_media is not None:, consider using the same pattern here on line 317.
359-372: LGTM - restoration logic correctly handles all cases.The
finallyblock properly restores the original state:
- When
Mediaexisted originally: restores the class reference and itsjs/cssattributes from deep copies- When
Mediadidn't exist: removes theMediaclass that was added during the test- Correctly handles cases where
js/cssattributes didn't exist originally by deleting themThis effectively prevents test environment pollution as intended by the PR.
shivsubh
force-pushed
the
issues/405-improve-test-admin-media
branch
2 times, most recently
from
f644384 to
10d4374
Compare
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_notifications/tests/test_admin.py`:
- Around line 316-322: The test fails to preserve the Media.extend attribute
when patching UserAdmin.Media; capture the original extend value before
modifying by doing original_extend = getattr(original_media, "extend", None)
(alongside original_js/original_css) and in the finally block restore it with
setattr(UserAdmin, "Media", original_media) or if restoring piecewise, set
getattr(UserAdmin, "Media", "extend") back to original_extend; reference the
UserAdmin.Media object and the IgnoreObjectNotificationWidgetMedia class to
ensure Media.extend is preserved and restored to avoid test pollution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 577a9354-086d-4803-89e2-77e86209b465
📒 Files selected for processing (1)
openwisp_notifications/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). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (3)
openwisp_notifications/tests/test_admin.py (3)
337-358: Unusedresponsevariables should either be asserted or discarded.The
responseassignments on lines 337-342, 346-350, and 354-358 are never asserted. If the intent is only to verify no exceptions are raised, consider using_to indicate intentional discard, or add explicit assertions to validate the expected behavior.
1-1: LGTM!The
copyimport is appropriate for deep copying thejsandcssattributes.
359-372: LGTM!The
try...finallypattern correctly ensuresUserAdmin.Mediais restored regardless of test outcome. The restoration logic properly handles all edge cases:
- Original Media with/without
js/cssattributes- Original Media not existing at all
- Media being replaced entirely by
_add_object_notification_widget()This addresses the test pollution issue described in
#405.
The test_object_notification_setting_configured test case was modifying UserAdmin.Media without proper restoration, causing pollution in the test environment. Added a try...finally block to ensure UserAdmin.Media is restored to its original state. Closes openwisp#405
shivsubh
force-pushed
the
issues/405-improve-test-admin-media
branch
from
10d4374 to
3687a8a
Compare
nemesifier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_notifications/tests/test_admin.py`:
- Around line 315-320: The check for original_media uses truthiness; change it
to an identity None check to match the finally block: in the test where
original_media is obtained from getattr(UserAdmin, "Media", None), replace the
conditional "if original_media:" with "if original_media is not None:" so
assignments to original_js and original_css (via copy.deepcopy of
getattr(original_media, 'js', None) and getattr(original_media, 'css', None))
only run when original_media is not None, keeping behavior symmetric with the
finally block that uses "is not None" and preventing surprises if Media defines
a custom __bool__.
- Around line 361-369: The finally block must remove any js/css attributes that
were added to the original Media class when those attributes did not exist
originally: after restoring UserAdmin.Media = original_media (i.e., referencing
original_media, original_js, original_css), check if original_js is None and
hasattr(UserAdmin.Media, "js") and if so delattr(UserAdmin.Media, "js"); do the
same for original_css: if original_css is None and hasattr(UserAdmin.Media,
"css") then delattr(UserAdmin.Media, "css"). This ensures UserAdmin.Media is
returned to the exact pre-test shape even when original Media lacked js/css.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee4754d4-bacd-4290-b08f-16f253a3f019
📒 Files selected for processing (1)
openwisp_notifications/tests/test_admin.py
2 participants
The test_object_notification_setting_configured test case was modifying UserAdmin.Media without proper restoration, causing pollution in the test environment. Added a try...finally block to ensure UserAdmin.Media is restored to its original state.
Closes #405
Checklist