[tests] Fix deactivation test to exercise correct code path #1221#1286
[tests] Fix deactivation test to exercise correct code path #1221#1286mn-ram wants to merge 3 commits intoopenwisp:masterfrom
Conversation
…1221 Replace bulk QuerySet.delete() with per-instance delete loops so that post_delete signals fire correctly, triggering peer cache invalidation, certificate revocation, and IP address release.
|
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:
📝 WalkthroughWalkthroughThis PR replaces bulk Sequence Diagram(s)sequenceDiagram
participant Admin as rgba(52,152,219,0.5) Admin/UI
participant Serializer as rgba(46,204,113,0.5) Serializer / Config Manager
participant DB as rgba(155,89,182,0.5) Database (VpnClient rows)
participant Signal as rgba(241,196,15,0.5) Django post_delete signal
participant Cert as rgba(231,76,60,0.5) Cert Service
participant IPAM as rgba(52,73,94,0.5) IP Management
participant Cache as rgba(26,188,156,0.5) Cache
Admin->>Serializer: remove template / deactivate device
Serializer->>DB: select_related(...).iterator() -> fetch VpnClient instances
Serializer->>DB: begin atomic transaction
loop per-instance
Serializer->>DB: instance.delete()
DB->>Signal: emit post_delete (VpnClient)
Signal->>Cert: revoke certificate
Signal->>IPAM: release IP
Signal->>Cache: invalidate entries
end
Serializer->>DB: commit transaction
Serializer->>Admin: respond (templates saved / device updated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
0ae82c2 to
b559029
Compare
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_controller/config/tests/test_vpn.py`:
- Around line 905-910: Remove or rename the redundant WireGuard "certificate
revoked for OpenVPN-style auto_cert" subTest: delete the subTest block that only
repeats self.assertFalse(VpnClient.objects.filter(pk=vpn_client.pk).exists()),
or replace it with a WireGuard-specific check (e.g., assert no related
Certificate/Key object exists or validate post_delete handler ran without
raising) so the test focuses on WireGuard behavior instead of duplicating the
VpnClient deletion assertion.
- Around line 892-903: Replace the weak global-count check with an object-level
assertion: before removing the template/ deleting the VpnClient, capture the
specific IpAddress instance associated with that client (e.g., via
device.config.vpnclient_set.first() or the client's related IpAddress), then
after the removal assert that the specific IpAddress no longer exists (e.g.,
that IpAddress.objects.filter(pk=<captured_pk>).exists() is False or that a get
raises IpAddress.DoesNotExist). Apply the same change to the similar assertion
block referenced around lines 916-927 so both tests verify deletion of the exact
IpAddress object rather than relying on overall IpAddress.count() deltas.
🪄 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: 7b8ae292-e57e-4115-8c7e-548633088f56
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_vpn.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~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_vpn.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
🔇 Additional comments (1)
openwisp_controller/config/tests/test_vpn.py (1)
918-921: Good fix: deactivation now exercises the intended code path.Switching to
device.deactivate()and assertinghandler.assert_called_once()is the right regression guard for issue#1221.
b559029 to
344fa8b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_vpn.py (1)
904-910:⚠️ Potential issue | 🟡 MinorRemove redundant “post_delete handler ran without error” subtest.
This block re-checks client deletion (
count() == 0) that is already asserted earlier and doesn’t add distinct post-delete signal verification.Suggested cleanup
- with self.subTest("post_delete handler ran without error"): - # WireGuard does not use x509 certs, so there is nothing to - # revoke. Verify the post_delete handler completed by checking - # that the peer cache was invalidated (signal was already - # asserted above) and no VpnClient remains. - self.assertEqual(device.config.vpnclient_set.count(), 0)As per coding guidelines "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_vpn.py` around lines 904 - 910, Remove the redundant subTest block labeled "post_delete handler ran without error" in openwisp_controller/config/tests/test_vpn.py: delete the with self.subTest("post_delete handler ran without error") block that only re-asserts self.assertEqual(device.config.vpnclient_set.count(), 0), since the same client-deletion assertion is already performed earlier and the subtest adds no new verification of the post-delete signal or cache invalidation.
🤖 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_controller/config/tests/test_vpn.py`:
- Around line 904-910: Remove the redundant subTest block labeled "post_delete
handler ran without error" in openwisp_controller/config/tests/test_vpn.py:
delete the with self.subTest("post_delete handler ran without error") block that
only re-asserts self.assertEqual(device.config.vpnclient_set.count(), 0), since
the same client-deletion assertion is already performed earlier and the subtest
adds no new verification of the post-delete signal or cache invalidation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78819d74-e8d4-48f1-ba61-06d63f7c364f
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_vpn.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). (9)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_vpn.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_vpn.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_vpn.py
🔇 Additional comments (1)
openwisp_controller/config/tests/test_vpn.py (1)
911-927: Good fix: deactivation test now exercises the intended path.Using
device.deactivate()before assertions correctly targets the deactivation-gated cleanup flow and validates signal/IP cleanup behavior.
344fa8b to
b96fec8
Compare
Black Formatting ErrorsHello @prakash-kalwaniya, There are Explanation: Remediation: openwisp-qa-format |
…1221 test_deactivation_fires_post_delete_signals was calling device.config.templates.clear() directly without first setting the config status to deactivating. This meant the per-instance delete path guarded by is_deactivating_or_deactivated() in manage_vpn_clients was never exercised, defeating the purpose of the regression test. Replace templates.clear() with device.deactivate() which sets the status to deactivating before clearing templates. Also replace global IP count checks with object-level assertions, remove the unused cert variable (F841), and remove the redundant "certificate revoked for OpenVPN-style auto_cert" subTest that duplicated the VpnClient deletion assertion. Related to openwisp#1221
b96fec8 to
eab33a6
Compare
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_controller/config/base/config.py`:
- Around line 343-347: Remove the transaction.atomic() wrappers surrounding the
per-instance deletion loops that call vpnclient.delete(); specifically, delete
the atomic context around the for loop that iterates over
instance.vpnclient_set.select_related(...).iterator() so each VpnClient.delete()
runs and commits independently. VpnClient.post_delete triggers immediate side
effects (cache invalidation, IP deletion, certificate revocation), so avoid
wrapping those deletes in transaction.atomic() to prevent DB rollback leaving
side effects applied; simply iterate and call vpnclient.delete() without the
atomic block.
In `@openwisp_controller/config/tests/test_vpn.py`:
- Around line 912-915: The test currently asserts a single signal call using
handler.assert_called_once() after calling device.deactivate(), but the intent
is to validate "for each client" behavior; capture the number of VPN
peers/clients before deactivation (e.g., len(peers) or a
pre_deactivation_client_count variable) and replace handler.assert_called_once()
with an assertion that handler.call_count == pre_deactivation_client_count so
the test verifies the signal fired once per client; apply the same change to the
other identical assertion block that also uses catch_signal(vpn_peers_changed)
and handler.assert_called_once().
🪄 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: 571149a2-7206-42fd-bf10-8c6cc8f79068
📒 Files selected for processing (2)
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.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). (9)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- 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.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_vpn.py
🔇 Additional comments (1)
openwisp_controller/config/tests/test_vpn.py (1)
883-903: Good regression coverage for template-removal cleanup path.This test now validates all three critical effects: signal emission,
VpnClientdeletion, and exactIpAddressrelease by object identity.
This comment was marked as spam.
This comment was marked as spam.
✅ Actions performedComments resolved and changes approved. |
Summary
test_deactivation_fires_post_delete_signalswas callingdevice.config.templates.clear()directly without setting config status to
deactivatingfirstis_deactivating_or_deactivated()inmanage_vpn_clientswas never exercised, defeating the purpose of the regression testtemplates.clear()withdevice.deactivate()which sets status todeactivatingbefore clearing, ensuring the correct code path is tested
handler.assert_called()to unconditionalhandler.assert_called_once()Refs #1221
Test plan
test_deactivation_fires_post_delete_signalspasses