[tests] Fixed isolation bugs in functional tests and updated tests broken due to changes in openwisp-utils#651
Conversation
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
|
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 the shared test utility base with Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
✨ 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/runtests.py`:
- Around line 16-30: The no-op setUpClass/tearDownClass overrides in Pretest are
duplicated in TestUtils and should be consolidated into a shared mixin such as
NonSeleniumTestMixin. Move the empty classmethod overrides into the mixin, have
both Pretest and TestUtils inherit it, and add a brief why-comment before the
shared block explaining that the override intentionally skips the
Selenium-oriented parent setup that requires Django settings these tests do not
need.
🪄 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: 41478fa7-fb6b-4948-b708-aa01cc3dcb3b
📒 Files selected for processing (1)
tests/runtests.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CI Build
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
tests/runtests.py
**/*.{sh,py,yml,yaml,dockerfile,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it
Files:
tests/runtests.py
🪛 ast-grep (0.44.0)
tests/runtests.py
[info] 490-490: Make sure cookies are safe and secure
Context: verify=False
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute.
(secure-cookie)
[warning] 482-491: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
url,
json={
"username": "signup-user",
"email": "user@signup.com",
"password1": "rLx6OH%[",
"password2": "rLx6OH%[",
},
verify=False,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[info] 482-491: no timeout was given on call to external resource
Context: requests.post(
url,
json={
"username": "signup-user",
"email": "user@signup.com",
"password1": "rLx6OH%[",
"password2": "rLx6OH%[",
},
verify=False,
)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[error] 490-490: verify should be True
Context: verify=False
Note: [CWE-295] Improper Certificate Validation (TLS verification disabled).
(request-verify)
🪛 OpenGrep (1.23.0)
tests/runtests.py
[WARNING] 483-492: SSL/TLS verification is disabled (verify=False). This allows man-in-the-middle attacks. Remove verify=False or set it to True.
(coderabbit.tls.verify-disabled-python)
🔇 Additional comments (2)
tests/runtests.py (2)
139-139: LGTM!Also applies to: 290-294
6-6: LGTM!Also applies to: 481-492
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
SeleniumTestMixin brings a lot of logic which is not needed for the precondition checks. It recently introduced logic for dealing with flaky tests caused by sqlite, and depends on django SETTINGS being loaded, which is not the case for the precondition checks.
nemesifier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/runtests.py`:
- Around line 412-429: The RADIUS registration test in
test_radius_user_registration uses requests.post without a timeout, which can
hang CI if the API is unresponsive. Update the requests.post call in this test
to include a reasonable timeout while keeping verify=False unchanged, and ensure
the rest of the cleanup in finally remains intact.
- Around line 172-173: The unguarded delete_test_users call in tearDownClass can
abort cleanup if docker compose fails. Wrap the delete_test_users invocation in
the same kind of try/except protection used for the objects_to_delete loop, so
_execute_docker_compose_command RuntimeError does not stop the subsequent
second_driver.quit(), base_driver.quit(), and CSS file cleanup from running.
Keep cls.delete_test_users and tearDownClass as the key locations when applying
the fix.
- Around line 94-95: The OPENWISP_ADMIN_THEME_LINKS favicon entry is using a
relative path, so update the settings injection in runtests.py to make the
favicon href absolute like the other static assets. In the printf that writes
OPENWISP_ADMIN_THEME_LINKS, change the favicon item in the same block that
builds the list so it points to the /static/ui/openwisp/images/favicon.svg path,
keeping the existing admin theme link entries intact.
In `@tests/utils.py`:
- Around line 31-55: The _execute_docker_compose_command helper currently calls
subprocess.run without any timeout, so a hung docker compose invocation can
block CI forever. Update _execute_docker_compose_command in tests/utils.py to
pass a reasonable timeout to subprocess.run, handle the timeout exception by
logging the partial output/error to the existing logs_file, and raise a
RuntimeError that clearly identifies the timed-out command.
- Around line 57-75: docker_compose_get_container_id currently repeats the same
subprocess execution logic as the shared helper and bypasses its logging/error
handling. Update docker_compose_get_container_id to call
_execute_docker_compose_command instead of creating its own subprocess.Popen,
then decode the returned output to get the container ID while preserving the
existing method behavior.
🪄 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: 890c2d71-bb36-4c16-83db-965bdd49b9bc
📒 Files selected for processing (2)
tests/runtests.pytests/utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CI Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
tests/utils.pytests/runtests.py
**/*.{sh,py,yml,yaml,dockerfile,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it
Files:
tests/utils.pytests/runtests.py
🪛 ast-grep (0.44.0)
tests/utils.py
[warning] 46-46: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cls.config["logs_file"], "a")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[error] 40-40: Command coming from incoming request
Context: subprocess.run(cmd_args, check=False, **kwargs)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 66-71: Command coming from incoming request
Context: subprocess.Popen(
["docker", "compose", "ps", "--quiet", container_name],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=self.root_location,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 40-40: Use of unsanitized data to create processes
Context: subprocess.run(cmd_args, check=False, **kwargs)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
tests/runtests.py
[info] 416-425: no timeout was given on call to external resource
Context: requests.post(
url,
json={
"username": username,
"email": "user@signup.com",
"password1": "rLx6OH%[",
"password2": "rLx6OH%[",
},
verify=False,
)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 424-424: Make sure cookies are safe and secure
Context: verify=False
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute.
(secure-cookie)
[error] 424-424: verify should be True
Context: verify=False
Note: [CWE-295] Improper Certificate Validation (TLS verification disabled).
(request-verify)
[warning] 416-425: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
url,
json={
"username": username,
"email": "user@signup.com",
"password1": "rLx6OH%[",
"password2": "rLx6OH%[",
},
verify=False,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 OpenGrep (1.23.0)
tests/runtests.py
[WARNING] 417-426: SSL/TLS verification is disabled (verify=False). This allows man-in-the-middle attacks. Remove verify=False or set it to True.
(coderabbit.tls.verify-disabled-python)
🔇 Additional comments (7)
tests/utils.py (4)
14-56: LGTM!
77-108: LGTM!
185-185: LGTM!
327-328: LGTM!tests/runtests.py (3)
15-19: LGTM! Resolves the previously flagged duplicate no-op setUpClass/tearDownClass pattern by no longer needing it.
310-333: LGTM!
482-483: LGTM! Resolves the previously flagged duplicate no-op setUpClass/tearDownClass pattern.
github-project-automation
Bot
moved this from Reviewer approved
to In progress
in OpenWISP Priorities for next releases
nemesifier
changed the title
nemesifier
changed the title
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
1 participant
Checklist
Description of Changes
This pull request fixes docker-openwisp functional test failures exposed by recent changes in
openwisp-utilsand by pre-existing test isolation issues in the Selenium suite.Problems Solved
Non-Selenium tests inherited Selenium setup
openwisp-utilsrecently changedSeleniumTestMixin.setUpClass()so it accesses Django settings while preparing Selenium database connection handling.In docker-openwisp,
PretestandTestUtilswere not Selenium tests, but they inheritedSeleniumTestMixinindirectly through the sharedTestUtilitiesclass. These tests run outside a configuredDJANGO_SETTINGS_MODULE, so the full suite failed with:The test utilities are now split into
BaseTestUtilsandFunctionalTestUtils, so non-Selenium tests do not run Selenium class setup.The custom theme setup injected a stale favicon path
openwisp-utilschanged the admin favicon fromfavicon.pngtofavicon.svg.The docker-openwisp functional test setup was still injecting this value through
OPENWISP_ADMIN_THEME_LINKS:Because that path is relative, the browser resolved it below
/admin/, producing requests like:That produced a
404browser console error and madetest_console_errorsfail.The setup now injects the SVG favicon with the correct MIME type:
It also replaces any existing
OPENWISP_ADMIN_THEME_LINKSassignment before appending the test value, so retry runs do not keep stale favicon settings.Functional tests depended on shared browser state
Some tests created users through the admin UI or through the RADIUS API, then cleaned them up later through Selenium. That cleanup depended on the browser still being logged in, which could fail after tests that intentionally logged out.
Test-created users are now removed through the Django ORM with
manage.py shell, avoiding browser-state-dependent cleanup. This makestest_radius_user_registrationindependent from the rest ofTestServices.The password reset test depended on previous login state
test_forgot_passwordpreviously assumed another test had already logged the browser in. It now logs in explicitly before testing logout and password reset behavior.Verbose unittest output was noisy
The shared base test utility now keeps verbose unittest output focused on test names instead of method docstrings.
Reference to Existing Issue
Not needed. This fixes CI failures in the test suite.