PR 4: Python — Handler Integration Test Expansion#447
Conversation
Add integration tests to three existing handler test files: - test_server (+5): response body text, 200 status, state transition False->True, restart idempotency, state persistence - test_status (+5): full response structure (server + controller sections), null scan times initially, no_enabled_pairs reflects actual state - test_stream_log (+5): QueueLogHandler attach/remove, multiple handlers independent operation, CachedQueueLogHandler delivers history, cache=0 sends no historical records Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR extends integration test coverage for three web handler endpoints: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
nitrobass24
changed the title
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/tests/integration/test_web/test_handler/test_status.py`:
- Around line 25-27: The test_full_response_structure is missing assertions for
two controller keys that the serializer now emits; update the test to assert
that json_dict["controller"] contains "latest_remote_scan_failed" and
"latest_remote_scan_error". Locate the assertions block in
test_full_response_structure (where it currently asserts
"latest_local_scan_time", "latest_remote_scan_time", "no_enabled_pairs") and add
two corresponding self.assertIn checks for "latest_remote_scan_failed" and
"latest_remote_scan_error" to ensure the test covers the full serialized
controller contract.
In `@src/python/tests/integration/test_web/test_handler/test_stream_log.py`:
- Line 94: The test docstring is inaccurate: it claims to verify delivery
“before the QueueLogHandler connects” but the test never instantiates or
connects a QueueLogHandler and instead calls
CachedQueueLogHandler.get_cached_records() directly; update the docstring to
describe the actual behavior being tested (e.g., that CachedQueueLogHandler
stores and returns cached records via get_cached_records() without needing a
QueueLogHandler connection) and mention the relevant symbols
CachedQueueLogHandler.get_cached_records and QueueLogHandler for clarity.
- Around line 56-60: Replace direct post-assert removal of logging handlers with
failure-safe cleanup by registering handler removal immediately after each
logger.addHandler(handler); specifically, after every logger.addHandler(handler)
call in this file (instances around the blocks that later call
logger.removeHandler(handler) at lines shown in the comment), call
self.addCleanup(lambda: logger.removeHandler(handler)) so the handler is always
removed even if assertions fail; ensure you keep or remove the later explicit
logger.removeHandler(handler) (it can be left or omitted) but do not rely on it
for cleanup.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ec64a8f5-cb71-4e3f-840a-26e5000c900e
📒 Files selected for processing (3)
src/python/tests/integration/test_web/test_handler/test_server.pysrc/python/tests/integration/test_web/test_handler/test_status.pysrc/python/tests/integration/test_web/test_handler/test_stream_log.py
| logger.addHandler(handler) | ||
| self.assertEqual(len(logger.handlers), initial_count + 1) | ||
|
|
||
| logger.removeHandler(handler) | ||
| self.assertEqual(len(logger.handlers), initial_count) |
There was a problem hiding this comment.
Make handler cleanup failure-safe to prevent cross-test state leakage.
Current removeHandler(...) calls run only on the happy path. If any assertion fails before teardown, handlers stay attached and can pollute later tests. Register cleanup immediately after each addHandler(...) with self.addCleanup(...).
Suggested patch
@@
logger.addHandler(handler)
+ self.addCleanup(logger.removeHandler, handler)
self.assertEqual(len(logger.handlers), initial_count + 1)
logger.removeHandler(handler)
@@
logger.addHandler(h1)
+ self.addCleanup(logger.removeHandler, h1)
logger.addHandler(h2)
+ self.addCleanup(logger.removeHandler, h2)
@@
logger.addHandler(cache)
+ self.addCleanup(logger.removeHandler, cache)
@@
logger.addHandler(cache)
+ self.addCleanup(logger.removeHandler, cache)Also applies to: 68-69, 91-91, 97-97, 106-106, 112-112, 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/integration/test_web/test_handler/test_stream_log.py` around
lines 56 - 60, Replace direct post-assert removal of logging handlers with
failure-safe cleanup by registering handler removal immediately after each
logger.addHandler(handler); specifically, after every logger.addHandler(handler)
call in this file (instances around the blocks that later call
logger.removeHandler(handler) at lines shown in the comment), call
self.addCleanup(lambda: logger.removeHandler(handler)) so the handler is always
removed even if assertions fail; ensure you keep or remove the later explicit
logger.removeHandler(handler) (it can be left or omitted) but do not rely on it
for cleanup.
- test_status: assert latest_remote_scan_failed and latest_remote_scan_error are present in controller response (both are emitted by the serializer but were not checked) - test_stream_log: fix docstring that incorrectly described QueueLogHandler connection behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/python/tests/integration/test_web/test_handler/test_stream_log.py (1)
56-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandler cleanup is still not failure-safe (prior finding unresolved).
All four test methods remove their handlers only on the happy path. A failing assertion before any
logger.removeHandler(...)call leaves the handler attached, which can pollute subsequent tests.🛡️ Proposed fix — register cleanup immediately after each addHandler
logger.addHandler(handler) + self.addCleanup(logger.removeHandler, handler) self.assertEqual(len(logger.handlers), initial_count + 1)logger.addHandler(h1) + self.addCleanup(logger.removeHandler, h1) logger.addHandler(h2) + self.addCleanup(logger.removeHandler, h2)logger.addHandler(cache) # test_cached_handler_delivers_history + self.addCleanup(logger.removeHandler, cache)logger.addHandler(cache) # test_cache_zero_sends_no_history + self.addCleanup(logger.removeHandler, cache)Also applies to: 68-69, 82-91, 97-106, 112-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/tests/integration/test_web/test_handler/test_stream_log.py` around lines 56 - 60, Tests add logging handlers with logger.addHandler(handler) but only remove them on the happy path; register cleanup immediately after adding the handler so a failure won't leak handlers. Replace/remove the direct paired remove calls with a failure-safe cleanup by calling self.addCleanup(logger.removeHandler, handler) immediately after logger.addHandler(handler) (or wrap the add/remove in a try/finally and call logger.removeHandler(handler) in finally) for each occurrence so logger.removeHandler is always invoked even if an assertion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/python/tests/integration/test_web/test_handler/test_stream_log.py`:
- Around line 56-60: Tests add logging handlers with logger.addHandler(handler)
but only remove them on the happy path; register cleanup immediately after
adding the handler so a failure won't leak handlers. Replace/remove the direct
paired remove calls with a failure-safe cleanup by calling
self.addCleanup(logger.removeHandler, handler) immediately after
logger.addHandler(handler) (or wrap the add/remove in a try/finally and call
logger.removeHandler(handler) in finally) for each occurrence so
logger.removeHandler is always invoked even if an assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46b047ec-b789-47c8-8f67-e346f05147f3
📒 Files selected for processing (2)
src/python/tests/integration/test_web/test_handler/test_status.pysrc/python/tests/integration/test_web/test_handler/test_stream_log.py
1 participant
Summary
is_restart_requested()state transition False->True, restart endpoint idempotent (multiple calls return 200), restart state persists across callslatest_remote_scan_timenull initially,latest_local_scan_timenull initially,no_enabled_pairsreflects default False,no_enabled_pairsreflects True when setQueueLogHandlerattach/remove from logger, multipleQueueLogHandlers operate independently (both receive messages, removing one doesn't affect the other),CachedQueueLogHandlerdelivers historical records, cache withhistory_size_in_ms=0sends no historical recordsTest plan
cd src/python && PYTHONPATH=. python3 -m pytest tests/integration/test_web/test_handler/test_server.py tests/integration/test_web/test_handler/test_status.py tests/integration/test_web/test_handler/test_stream_log.py::TestLogStreamHandlerCleanup -v— 16 passedpython3 -m ruff check— all checks passedpython3 -m ruff format --check— already formatted🤖 Generated with Claude Code
Summary by CodeRabbit