PR 3: Python — Web App Job & Context tests#446
Conversation
Cover two previously untested modules: - web_app_job: setup creates server and starts thread, execute calls process, cleanup stops server and joins thread, never-initialized server logs warning, request logging middleware logs method/path/status - context: create_child_context returns copy with child logger, print_to_log redacts sensitive fields, handles present/absent path pairs, Args.as_dict serializes all fields as strings 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 introduces two new unittest files that establish baseline test coverage for core components: one validates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
|
@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: 1
🤖 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/unittests/test_web/test_web_app_job.py`:
- Around line 138-140: The test currently nests self.assertRaises(RuntimeError)
outside self.assertLogs, so the RuntimeError propagates and assertLogs cannot
verify captured records; modify the nesting so
self.assertLogs("test_request_logging_error", level="DEBUG") is the outer
context and self.assertRaises(RuntimeError) wraps the call to
list(middleware(environ, start_response)) inside it, ensuring the RuntimeError
is caught by assertRaises and assertLogs can exit normally and assert that
_RequestLoggingMiddleware emitted the expected logs.
🪄 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: aa6ee468-5370-4ebb-9d4f-7418d32a723b
📒 Files selected for processing (2)
src/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_web/test_web_app_job.py
| with self.assertRaises(RuntimeError): | ||
| with self.assertLogs("test_request_logging_error", level="DEBUG"): | ||
| list(middleware(environ, start_response)) |
There was a problem hiding this comment.
assertLogs does not verify log emission when an exception propagates through it — test makes an unenforceable claim.
assertLogs.__exit__ short-circuits its records check when exc_type is not None. Because RuntimeError is raised inside the inner with self.assertLogs(...) block and propagates outward to assertRaises, assertLogs exits with a live exception and never confirms that any log records were captured. The test would pass identically if the finally block in _RequestLoggingMiddleware were removed, completely invalidating the intended assertion.
Fix: invert the nesting so assertRaises (inner) catches and suppresses the RuntimeError first, allowing assertLogs (outer) to exit cleanly and verify its records:
🐛 Proposed fix
- with self.assertRaises(RuntimeError):
- with self.assertLogs("test_request_logging_error", level="DEBUG"):
- list(middleware(environ, start_response))
+ with self.assertLogs("test_request_logging_error", level="DEBUG") as log_ctx:
+ with self.assertRaises(RuntimeError):
+ list(middleware(environ, start_response))
+ self.assertTrue(any("POST" in msg and "/fail" in msg for msg in log_ctx.output))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self.assertRaises(RuntimeError): | |
| with self.assertLogs("test_request_logging_error", level="DEBUG"): | |
| list(middleware(environ, start_response)) | |
| with self.assertLogs("test_request_logging_error", level="DEBUG") as log_ctx: | |
| with self.assertRaises(RuntimeError): | |
| list(middleware(environ, start_response)) | |
| self.assertTrue(any("POST" in msg and "/fail" in msg for msg in log_ctx.output)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/unittests/test_web/test_web_app_job.py` around lines 138 -
140, The test currently nests self.assertRaises(RuntimeError) outside
self.assertLogs, so the RuntimeError propagates and assertLogs cannot verify
captured records; modify the nesting so
self.assertLogs("test_request_logging_error", level="DEBUG") is the outer
context and self.assertRaises(RuntimeError) wraps the call to
list(middleware(environ, start_response)) inside it, ensuring the RuntimeError
is caught by assertRaises and assertLogs can exit normally and assert that
_RequestLoggingMiddleware emitted the expected logs.
assertLogs must be the outer context so it can validate captured log records on normal exit. With assertRaises as the outer context, the RuntimeError propagated through assertLogs preventing its validation from running — making the log assertion a no-op. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
WebAppJob.setup()creates server on configured port and starts thread,execute()delegates toweb_app.process(),cleanup()stops server and joins thread,MyWSGIRefServer.stop()on never-initialized server logs warning without crash,_RequestLoggingMiddlewarelogs method/path/status/duration including on app errorscreate_child_contextreturns shallow copy with child logger while sharing config/status/args,print_to_logredacts sensitive fields (passwords, API keys), handles present/absent path pairs gracefully,Args.as_dict()serializes all fields as strings including None valuesTest plan
cd src/python && PYTHONPATH=. python3 -m pytest tests/unittests/test_web/test_web_app_job.py tests/unittests/test_common/test_context.py -v— 14 passedpython3 -m ruff check— all checks passedpython3 -m ruff format --check— already formatted🤖 Generated with Claude Code
Summary by CodeRabbit