Stop leaking StreamHandlers across test runs (#450)#457
Conversation
Test setUp methods across the suite added a fresh StreamHandler to a shared logger (mostly the root logger) and never removed it. Because loggers are singletons, by test N the logger held N handlers and each log line printed N times. Test correctness wasn't affected — no assertion depends on handler count — only output noise. Use unittest's addCleanup hook to remove the handler after each test. addCleanup runs even if setUp partially fails after the registration, so the cleanup is robust against initialization errors. Touches 10 setUp methods across 8 files (TestAppProcess / TestAppOneShotProcess and TestDeleteLocalProcess / TestDeleteRemoteProcess each have their own setUp). Closes #450 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds cleanup callbacks to 8 test ChangesLogger Handler Cleanup in Test Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Summary
self.addCleanup(logger.removeHandler, handler)after everyaddHandler(handler)call in testsetUpmethods (10 sites across 8 files)Why
Per #450: every test
setUpadds aStreamHandlerto a logger (mostly the root logger) and never removes it. Loggers are singletons, so by test N the logger has N handlers and every log line prints N times. Test correctness isn't affected — no assertion depends on handler count — but the suite output gets progressively noisier.Approach
Used unittest's
addCleanuprather than wiring uptearDown. Reasons:addHandler, so the registration sits next to the operation it cleans upsetUpraises after the registration — robust to partial initialization failurestearDown(tests/integration/test_lftp/test_lftp.py,tests/unittests/test_lftp/test_lftp.py)Files touched
tests/integration/test_web/test_web_app.pytests/integration/test_lftp/test_lftp.pytests/integration/test_controller/test_controller.pytests/unittests/test_common/test_app_process.pytests/unittests/test_ssh/test_sshcp.pytests/unittests/test_lftp/test_lftp.pytests/unittests/test_controller/test_delete/test_delete_process.pytests/unittests/test_controller/test_scan/test_active_scanner.pyTest plan
Closes #450
🤖 Generated with Claude Code
Summary by CodeRabbit