Next release - #1692
Conversation
📝 WalkthroughWalkthroughThis PR updates CI and test reliability behavior: it changes devcontainer log initialization, modifies workflow test selection defaults and fallback routing, adds backend readiness polling in the Docker test runner, and adjusts nginx and UI test assertions. ChangesCI and test reliability updates
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/api_endpoints/test_nginx_proxy_security.py (1)
141-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring no longer matches the assertion.
The docstring still says "Test that access to
:20212/docsis allowed by Nginx (should return 200)." but the assertion at Line 149 now also accepts 500. Update the docstring to reflect the relaxed expectation, or tighten the assertion back to match the stated intent.🤖 Prompt for 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. In `@test/api_endpoints/test_nginx_proxy_security.py` around lines 141 - 149, The docstring in test_nginx_proxy_security_allow_port no longer matches the behavior being asserted. Update the test’s description to reflect that the request to /docs is allowed even when the response may be 500, or tighten the assertion so it only accepts 200 if that is the intended contract. Keep the wording aligned with the test name and the http_get/assertion logic in test_nginx_proxy_security_allow_port.
🧹 Nitpick comments (3)
.github/workflows/run-all-tests.yml (1)
56-90: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTemplate injection via inline
github.event.inputs.*expansion (zizmor).Static analysis flags
${{ github.event.inputs.run_all }}at Line 64 being expanded directly into the shell script; the same pattern repeats at Lines 71-76 and 79-80. Since all these inputs aretype: boolean, GitHub coerces values to"true"/"false"so the practical injection risk here is low, but the recommended mitigation is to pass inputs viaenv:and reference them as shell variables to avoid template expansion into the script body entirely.🔒 Suggested mitigation using env indirection
- name: Build Test Path Command id: builder + env: + RUN_ALL: ${{ github.event.inputs.run_all }} + RUN_SCAN: ${{ github.event.inputs.run_scan }} + RUN_API: ${{ github.event.inputs.run_api }} + RUN_BACKEND: ${{ github.event.inputs.run_backend }} + RUN_DOCKER_ENV: ${{ github.event.inputs.run_docker_env }} + RUN_UI: ${{ github.event.inputs.run_ui }} + RUN_PLUGINS: ${{ github.event.inputs.run_plugins }} + RUN_ROOT_FILES: ${{ github.event.inputs.run_root_files }} run: | PATHS="" ... - if [ "${{ github.event.inputs.run_all }}" == "true" ]; then + if [ "$RUN_ALL" == "true" ]; then echo "🧪 Manual 'Run ALL' selected" echo "final_paths=test/" >> $GITHUB_OUTPUT exit 0 fi - if [ "${{ github.event.inputs.run_scan }}" == "true" ]; then PATHS="$PATHS test/scan/"; fi + if [ "$RUN_SCAN" == "true" ]; then PATHS="$PATHS test/scan/"; fi ...🤖 Prompt for 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. In @.github/workflows/run-all-tests.yml around lines 56 - 90, The workflow shell script in the test-selection step is expanding github.event.inputs.* directly inside the script, which triggers the template-injection warning. Move the boolean inputs into env variables for this job or step, then update the conditional checks in the test-path selection logic to use those shell variables instead of inline GitHub expression expansion. Keep the behavior in the same selection block that sets PATHS and final_paths.Source: Linters/SAST tools
scripts/run_tests_in_docker_environment.sh (1)
74-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBackend readiness wait soft-fails without diagnostics, unlike the sibling healthcheck loop.
The services healthcheck loop above (Lines 58-71) dumps
docker logsand exits with failure on timeout. This new backend wait loop only logs a warning and proceeds after 60s, so if the Flask backend never comes up, the subsequent test run will fail with pytest connection errors that are harder to trace back to backend startup than a clear timeout diagnostic here.Consider dumping container logs on timeout (without hard-failing, if the intent is to stay best-effort) so failures are easier to diagnose:
🔍 Suggested diagnostic on timeout
if [ "$i" -eq "$BACKEND_WAIT" ]; then echo "--- Warning: Flask backend did not become ready after $BACKEND_WAIT seconds, proceeding anyway ---" + docker logs --tail 100 netalertx-test-container fi🤖 Prompt for 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. In `@scripts/run_tests_in_docker_environment.sh` around lines 74 - 88, The Flask backend readiness loop in run_tests_in_docker_environment.sh only warns and continues on timeout, which leaves later pytest failures without startup diagnostics. Update the backend wait block around the Flask check to emit container logs from netalertx-test-container when the timeout is reached, similar to the existing healthcheck loop, while keeping the current best-effort behavior if you do not want to fail the script. Use the existing BACKEND_WAIT loop and docker exec curl readiness check to locate the change.test/ui/test_ui_waits.py (1)
45-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent
returnmasks a broken add-device flow as a passing test.If
#NEWDEV_devMacnever appears after both the in-page click and the direct navigation fallback, the test now just returns — reported as passed with no assertion ever exercised. If the add-device UI genuinely regresses, this test would silently give it a pass instead of surfacing the failure (or at least a visible skip) in CI.Consider
pytest.skip(...)instead of a barereturnso the outcome is visible in test reports rather than indistinguishable from a full pass.♻️ Suggested fix
try: wait_for_element_by_css(driver, "`#NEWDEV_devMac`", timeout=10) except Exception: # Element still not found after direct navigation — skip the rest of the test - return + pytest.skip("NEWDEV_devMac field not found after direct navigation; add-device form unavailable in this environment")Note: verify
pytestis imported at module top-level in this file before applying (not shown in the provided snippet).🤖 Prompt for 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. In `@test/ui/test_ui_waits.py` around lines 45 - 55, The fallback in the add-device UI wait logic silently returns from the test when `wait_for_element_by_css` never finds `#NEWDEV_devMac`, which can make a broken flow look like a pass. In `test_ui_waits.py`, update the final `except` block in this test to use `pytest.skip(...)` instead of a bare `return`, so the outcome is visible in test reports. Make sure `pytest` is available at module scope before using it.
🤖 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 `@test/api_endpoints/test_nginx_proxy_security.py`:
- Around line 24-37: The “access allowed” assertions in the Nginx proxy security
tests are too permissive because they treat HTTP 500 as acceptable, which can
hide backend crashes. Update the assertions in the affected test helper and each
affected test case to stop whitelisting 500; instead, keep the focus on
verifying the request is not rejected by Nginx (for example, by checking that
the response is not 403) while relying on the existing backend-readiness wait to
reduce startup flakiness.
In `@test/ui/test_ui_maintenance.py`:
- Around line 91-95: The page-source assertion in the maintenance UI test is
being swallowed by the broad exception handler, so a real failure never reaches
the test. Update the try/except around the page source check in the UI
maintenance test to only suppress non-assertion errors from accessing
page_source after the alert, and let the “Button click should not cause errors”
AssertionError propagate. Use the surrounding test logic and the page-source
check block to locate and narrow the exception handling.
---
Outside diff comments:
In `@test/api_endpoints/test_nginx_proxy_security.py`:
- Around line 141-149: The docstring in test_nginx_proxy_security_allow_port no
longer matches the behavior being asserted. Update the test’s description to
reflect that the request to /docs is allowed even when the response may be 500,
or tighten the assertion so it only accepts 200 if that is the intended
contract. Keep the wording aligned with the test name and the http_get/assertion
logic in test_nginx_proxy_security_allow_port.
---
Nitpick comments:
In @.github/workflows/run-all-tests.yml:
- Around line 56-90: The workflow shell script in the test-selection step is
expanding github.event.inputs.* directly inside the script, which triggers the
template-injection warning. Move the boolean inputs into env variables for this
job or step, then update the conditional checks in the test-path selection logic
to use those shell variables instead of inline GitHub expression expansion. Keep
the behavior in the same selection block that sets PATHS and final_paths.
In `@scripts/run_tests_in_docker_environment.sh`:
- Around line 74-88: The Flask backend readiness loop in
run_tests_in_docker_environment.sh only warns and continues on timeout, which
leaves later pytest failures without startup diagnostics. Update the backend
wait block around the Flask check to emit container logs from
netalertx-test-container when the timeout is reached, similar to the existing
healthcheck loop, while keeping the current best-effort behavior if you do not
want to fail the script. Use the existing BACKEND_WAIT loop and docker exec curl
readiness check to locate the change.
In `@test/ui/test_ui_waits.py`:
- Around line 45-55: The fallback in the add-device UI wait logic silently
returns from the test when `wait_for_element_by_css` never finds
`#NEWDEV_devMac`, which can make a broken flow look like a pass. In
`test_ui_waits.py`, update the final `except` block in this test to use
`pytest.skip(...)` instead of a bare `return`, so the outcome is visible in test
reports. Make sure `pytest` is available at module scope before using it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e687231-5b3a-45d7-bbbc-2330d8cf057b
📒 Files selected for processing (6)
.devcontainer/scripts/setup.sh.github/workflows/run-all-tests.ymlscripts/run_tests_in_docker_environment.shtest/api_endpoints/test_nginx_proxy_security.pytest/ui/test_ui_maintenance.pytest/ui/test_ui_waits.py
💤 Files with no reviewable changes (1)
- .devcontainer/scripts/setup.sh
- Changed the test suite name for clarity. - Updated default behavior for running all tests in the workflow. - Improved logging in the Docker test script for better debugging. - Modified assertions in Nginx proxy security tests to ensure access is not blocked. - Added exception handling in UI tests to skip tests when elements are not found.
…into next_release
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ui/test_ui_maintenance.py (2)
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: no longer checks
page_source.This comment still references "page_source access", but that check was removed in this change — the block now only attempts alert dismissal. Consider updating the comment to reflect the current logic.
✏️ Suggested comment update
except Exception as e: - # Check for alerts that might be blocking page_source access + # Check for a native alert that may have been raised during the click/wait try:🤖 Prompt for 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. In `@test/ui/test_ui_maintenance.py` at line 96, Update the stale inline comment in the maintenance UI test so it matches the current alert-dismissal-only behavior. In the test around the alert handling block in the UI maintenance flow, remove the reference to page_source access and replace it with a comment that describes only dismissing blocking alerts, keeping the wording aligned with the logic in this section.
102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider preserving the original traceback.
pytest.fail(f"Test failed: {e}")only carries the exception's string message, discarding the original traceback. Re-raising (raise) when no alert is present would keep the full traceback for easier debugging while still failing the test.♻️ Suggested change to preserve traceback
except Exception: - # No alert present - re-raise the original exception - pytest.fail(f"Test failed: {e}") + # No alert present - re-raise the original exception with traceback intact + raise🤖 Prompt for 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. In `@test/ui/test_ui_maintenance.py` around lines 102 - 104, The exception handling in the alert-check path drops the original traceback by calling pytest.fail in the bare except block. Update the logic around the exception handling in the test helper to preserve the original traceback when no alert is present by re-raising the caught exception instead of converting it to a new failure message; use the existing except Exception block and the surrounding alert-handling flow as the place to fix this.
🤖 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.
Nitpick comments:
In `@test/ui/test_ui_maintenance.py`:
- Line 96: Update the stale inline comment in the maintenance UI test so it
matches the current alert-dismissal-only behavior. In the test around the alert
handling block in the UI maintenance flow, remove the reference to page_source
access and replace it with a comment that describes only dismissing blocking
alerts, keeping the wording aligned with the logic in this section.
- Around line 102-104: The exception handling in the alert-check path drops the
original traceback by calling pytest.fail in the bare except block. Update the
logic around the exception handling in the test helper to preserve the original
traceback when no alert is present by re-raising the caught exception instead of
converting it to a new failure message; use the existing except Exception block
and the surrounding alert-handling flow as the place to fix this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6abeaa6f-14ae-47a2-8b32-96e7a278f2e4
📒 Files selected for processing (1)
test/ui/test_ui_maintenance.py
Summary by CodeRabbit