Repository navigation
Conversation
) - #19: add missing docstring to cleanup_globals._reset (docs-coverage 100%) - #20: guard websocket route introspection against FastAPI _IncludedRouter - #21: restructure tests/ to mirror src/pycharting/ so check_test_layout.py passes — flatten organizational grouping classes into module-level test functions, keep/add Test<SourceClass> classes (TestChartServer, TestDataManager, TestNoCacheStaticFiles, TestDataResponse, TestErrorResponse, TestDataValidationError), merge test_data_slicing into test_ingestion, fold test_import into test_interface. Gates: fmt PASS, docs-coverage 100%, 148 tests PASS (coverage 100%), test-layout OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from unittest.mock import MagicMock, patch | ||
|
|
||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| s.bind(("", 0)) |
|
Superseded by alihaskar#51, which targets the upstream motherrepo (alihaskar/pycharting) instead of the fork. Same commit (29bc1d4). |
There was a problem hiding this comment.
Pull request overview
This PR addresses the rhiza v1.2.1 quality-scorecard findings by (1) restoring full docstring coverage in the test suite, (2) hardening a FastAPI route-introspection test against newer FastAPI/Starlette route objects, and (3) reorganizing the tests/ tree to mirror the src/pycharting/ package layout for test-layout parity checks.
Changes:
- Reorganized and renamed tests under
tests/pycharting/<subpkg>/...to mirror source layout, flattening many grouping classes into module-level test functions. - Updated the lifecycle WebSocket-route test to read
route.pathdefensively (viagetattr) to tolerate_IncludedRouterentries inapp.routes. - Added/retained required
Test<SourceClass>parity classes and added model-construction tests forDataResponse/ErrorResponse.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_server.py | Removed legacy location; tests moved under tests/pycharting/core/test_server.py to match mirrored layout. |
| tests/test_interface.py | Removed legacy location; tests moved under tests/pycharting/api/test_interface.py and updated to include missing helper docstring. |
| tests/test_import.py | Removed standalone import smoke tests; folded into tests/pycharting/api/test_interface.py per new layout rules. |
| tests/test_data_slicing.py | Removed legacy location as part of test layout restructuring. |
| tests/test_data_ingestion.py | Removed legacy location as part of test layout restructuring. |
| tests/test_api_routes.py | Removed legacy location; tests moved under tests/pycharting/api/test_routes.py. |
| tests/pycharting/init.py | Added package marker to reflect mirrored test suite layout. |
| tests/pycharting/api/init.py | Added package marker for pycharting.api test subtree. |
| tests/pycharting/api/test_interface.py | New mirrored interface tests (including folded import smoke tests and global cleanup helper docstring). |
| tests/pycharting/api/test_routes.py | New mirrored routes tests and new DataResponse/ErrorResponse model tests. |
| tests/pycharting/core/init.py | Added package marker for pycharting.core test subtree. |
| tests/pycharting/core/test_server.py | New mirrored server tests (moved from legacy location). |
| tests/pycharting/core/test_lifecycle.py | Updated mirrored lifecycle tests (defensive route-path introspection; flattened grouping classes). |
Comments suppressed due to low confidence (3)
tests/pycharting/core/test_lifecycle.py:207
- The broad
except Exceptionwill also catchAssertionError, causing genuine failures (e.g. unexpected status code/body) to be reported as skipped tests instead of failing the suite. Split outAssertionError(or narrow the exception type) so assertions still fail and only environment/startup issues are skipped.
tests/pycharting/core/test_lifecycle.py:232 - This
except Exceptionwill also swallowAssertionErrorand turn real regressions into skipped tests. Add a separateexcept AssertionError: raise(or otherwise narrow the caught exceptions) so the test fails when the server responds incorrectly.
tests/pycharting/core/test_lifecycle.py:159 - If an assertion fails inside the loop, the server may be left running, leaking threads/ports into subsequent tests. Wrap the loop in a
try/finally(or otherwise guarantee cleanup) sostop_server()always runs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Addresses the three quality-scorecard findings surfaced by the rhiza v1.2.1 update (#18):
cleanup_globals._reset, bringing docstring coverage to 100%.test_websocket_endpoint_addednow reads route paths defensively (getattr(route, "path", None)), surviving FastAPI 0.138's_IncludedRouterentries inapp.routes.tests/to mirrorsrc/pycharting/socheck_test_layout.pyexits 0.#21 details
The layout checker enforces
Test<SourceClass>parity in both directions, so the many organizational grouping classes had to be dissolved:tests/pycharting/<subpkg>/test_<module>.py.TestHealthCheck,TestValidateInput, …) into module-level test functions.Test<SourceClass>classes the checker requires:TestChartServer,TestDataManager,TestNoCacheStaticFiles, and newTestDataResponse,TestErrorResponse,TestDataValidationError.test_data_slicing.py→test_ingestion.py; foldedtest_import.py→test_interface.py.from src.pycharting...import tofrom pycharting....Gates
make fmtmake docs-coveragemake testcheck_test_layout.pyNet: +5 tests (new model/exception tests), same 100% source coverage.
🤖 Generated with Claude Code