Skip to content

Address round-2 CodeRabbit test-quality findings#470

Merged
nitrobass24 merged 2 commits into
developfrom
fix/coderabbit-findings
May 5, 2026
Merged

Address round-2 CodeRabbit test-quality findings#470
nitrobass24 merged 2 commits into
developfrom
fix/coderabbit-findings

Conversation

@nitrobass24
Copy link
Copy Markdown
Owner

Second round of CodeRabbit findings on PR #467 (develop → master). All 7 findings verified against current code; all valid; all fixed. Test-only — production findings remain tracked in #469.

Acted on (7 of 7)

# File Change
1 integrations.spec.ts:349-353 GET on the "should not exist" assertion now fails fast on !res.ok instead of returning [] (which would always satisfy the "not found" check, masking real failures)
2 integrations.spec.ts (5 sites) Capture each POST-fixture response and pass it to a small expectFixtureOk helper so 4xx/5xx surfaces as \"fixture creation failed: …\" instead of cascading into a confusing UI assertion failure
3 settings.spec.ts:472-498 Log Level select-change test waits for toBeEnabled({ timeout: 10_000 }) before calling selectOption — same SSE-delivery race the Hash Algorithm test had
4 test_context.py:71-77 Use ppc.pairs = [pair] (public property setter, acquires the internal lock) instead of touching ppc._pairs directly
5 test_active_scanner.py (6 sites) Drop the redundant trailing scanner.close() calls now that addCleanup handles the close path. One cleanup mechanism instead of two.
6 test_web_app_job.py:105-118 Drop the unused captured_status dict from the WSGI middleware logging test; simplify start_response to a no-op
7 test_web_app_job.py:114-118 Replace log_ctx.output[0] indexing with any(...) over all captured lines so other loggers emitting first don't break the test

Validation

  • All Python files parse via python3 -m py_compile
  • TS diagnostics show only pre-existing issues (unused page param at unrelated line, WSGI start_response typing for unrelated middleware test paths)
  • CI on this PR will run the full Vitest + pytest + Playwright suites against the rebuilt image

What this is not

The 4 production-code findings tracked in #469 (handler error handling and persistence ordering) are still deferred to their own PRs — same rationale as last round.

🤖 Generated with Claude Code

All 7 findings from this round are valid; all fixed. Test-only — no
production code changes (the production findings tracked in #469 are
still unaddressed in this PR by design).

- integrations.spec.ts: GET on the "should not exist" assertion now
  fails fast instead of returning [] when the API errors. Five
  POST-fixture sites now check `res.ok` via a small `expectFixtureOk`
  helper so a 4xx/5xx surfaces as "fixture creation failed" instead of
  cascading into a confusing UI failure.

- settings.spec.ts: Log Level select-change test waits for the select
  to be enabled before calling selectOption — same SSE-delivery race
  the Hash Algorithm test had.

- test_context.py: use the public PathPairsConfig.pairs property setter
  (which acquires the internal lock) instead of touching ._pairs.

- test_active_scanner.py: drop the redundant trailing scanner.close()
  calls now that addCleanup handles the close path. Six sites — one
  cleanup mechanism instead of two.

- test_web_app_job.py: drop the unused captured_status dict from the
  middleware logging test, and replace `log_ctx.output[0]` indexing
  with an `any(...)` scan over all captured lines so other loggers
  emitting first don't break the test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

Warning

Rate limit exceeded

@nitrobass24 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 36 minutes and 52 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22031f65-dc87-4244-9bda-a09b1f5799ce

📥 Commits

Reviewing files that changed from the base of the PR and between 04d6320 and 3267fb0.

📒 Files selected for processing (5)
  • src/e2e-playwright/tests/integrations.spec.ts
  • src/e2e-playwright/tests/settings.spec.ts
  • src/python/tests/unittests/test_common/test_context.py
  • src/python/tests/unittests/test_controller/test_scan/test_active_scanner.py
  • src/python/tests/unittests/test_web/test_web_app_job.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/coderabbit-findings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Round-2 commit wrapped the new any(...) assertion across three lines;
ruff format prefers it on one. No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nitrobass24 nitrobass24 merged commit a958f6a into develop May 5, 2026
18 checks passed
@nitrobass24 nitrobass24 deleted the fix/coderabbit-findings branch May 5, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant