add route tests for canvas endpoints - #2209
Conversation
📝 WalkthroughWalkthroughCanvas route tests now run against a configured FastAPI app with seeded authentication. Coverage verifies canvas creation, persistence, retrieval, updates, authorization failures, deletion, listing, and WebSocket authentication. ChangesCanvas route tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. Comment |
PR Summary by QodoAdd route tests for canvas HTTP + WebSocket endpoints
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
nemotron-super review VERDICT: Blocking issues found.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: Test refactor improves structure and adds WebSocket auth tests, but one test makes unverified assumptions about response fields.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.github.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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_routes_canvas.py`:
- Line 19: Update the “server” host value in the test configuration to use the
loopback address 127.0.0.1 instead of the wildcard address 0.0.0.0, while
preserving the existing port.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 7c7d0e5c-e672-4cb8-88fc-90257176849c
📒 Files selected for processing (1)
tests/test_routes_canvas.py
| from tinyagentos.app import create_app | ||
|
|
||
| config = { | ||
| "server": {"host": "0.0.0.0", "port": 6969}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use a loopback address in the test config.
0.0.0.0 triggers Ruff S104. TestClient does not need a wildcard listener; use 127.0.0.1 instead.
Proposed fix
- "server": {"host": "0.0.0.0", "port": 6969},
+ "server": {"host": "127.0.0.1", "port": 6969},📝 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.
| "server": {"host": "0.0.0.0", "port": 6969}, | |
| "server": {"host": "127.0.0.1", "port": 6969}, |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 19-19: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_routes_canvas.py` at line 19, Update the “server” host value in
the test configuration to use the loopback address 127.0.0.1 instead of the
wildcard address 0.0.0.0, while preserving the existing port.
Source: Linters/SAST tools
|
nemotron-ultra-kilo review VERDICT: Significant test coverage improvements but introduces test isolation issues and potential flakiness.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
Code Review by Qodo
1. Hardcoded adminpass test password
|
![qodo-code-review[bot]](https://avatars.github.com/in/484649?s=60&v=4){kind=small}
| app = create_app(data_dir=tmp_path) | ||
| app.state.auth.setup_user("admin", "Admin", "", "adminpass") | ||
| return app |
There was a problem hiding this comment.
1. Hardcoded adminpass test password 📜 Skill insight ⛨ Security
The test helper hardcodes a password value (adminpass) in source code. This violates the requirement to avoid storing credentials in code/config and increases the risk of accidental reuse or leakage.
Agent Prompt
## Issue description
`tests/test_routes_canvas.py` hardcodes a password (`adminpass`) when creating the test admin user.
## Issue Context
Compliance requires that credentials (passwords/tokens/keys) are not hardcoded in source files; even in tests, use a generated ephemeral value or a secrets-store-backed value.
## Fix Focus Areas
- tests/test_routes_canvas.py[29-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def test_ws_canvas_unauthenticated_rejected(unauthed_client): | ||
| with pytest.raises(WebSocketDisconnect) as exc_info: | ||
| with unauthed_client.websocket_connect("/ws/canvas/test-canvas-id") as ws: | ||
| ws.receive_text() | ||
| assert exc_info.value.code == 1008 | ||
|
|
||
|
|
||
| # WebSocket endpoint (/ws/canvas/{canvas_id}) requires a live WebSocket | ||
| # connection and real-time hub interaction; skipped as it needs an external | ||
| # transport not available through the async HTTP client fixture. | ||
| def test_ws_canvas_authenticated_accepted(authed_client): | ||
| with authed_client.websocket_connect("/ws/canvas/test-canvas-id") as ws: | ||
| ws.send_text("ping") | ||
| # Connection staying open means auth passed and hub.join succeeded. |
There was a problem hiding this comment.
2. Duplicate canvas ws auth tests 🐞 Bug ⚙ Maintainability
tests/test_routes_canvas.py adds /ws/canvas unauthenticated/authenticated tests that already exist in tests/test_ws_auth.py, creating redundant coverage and two places to update expected close codes/behavior. This increases suite runtime and maintenance burden with no additional assertions beyond what’s already covered.
Agent Prompt
## Issue description
`tests/test_routes_canvas.py` adds WebSocket auth tests for `/ws/canvas/{canvas_id}` that duplicate existing coverage in `tests/test_ws_auth.py`, increasing runtime and maintenance burden.
## Issue Context
There is already a dedicated WebSocket auth test module that covers unauthenticated, invalid-session, and authenticated connects for `/ws/canvas/test-canvas-id`.
## Fix Focus Areas
- tests/test_routes_canvas.py[213-223]
- tests/test_ws_auth.py[127-149]
## Implementation notes
- Prefer removing the redundant `test_ws_canvas_*` tests from `tests/test_routes_canvas.py`.
- If the intent is canvas-specific WS behavior (not just auth), replace the duplicated auth checks with a distinct assertion (e.g., connect, trigger an update broadcast, and assert the WS receives a `canvas_update` event).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def _make_client(tmp_path): | ||
| from tinyagentos.app import create_app | ||
|
|
||
| config = { | ||
| "server": {"host": "0.0.0.0", "port": 6969}, | ||
| "backends": [], | ||
| "qmd": {"url": "http://localhost:7832"}, | ||
| "agents": [], | ||
| "metrics": {"poll_interval": 30, "retention_days": 30}, | ||
| } | ||
| import yaml | ||
|
|
||
| (tmp_path / "config.yaml").write_text(yaml.dump(config)) | ||
| (tmp_path / ".setup_complete").touch() | ||
| app = create_app(data_dir=tmp_path) | ||
| app.state.auth.setup_user("admin", "Admin", "", "adminpass") | ||
| return app | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def ws_app(tmp_path): | ||
| return _make_client(tmp_path) | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def unauthed_client(ws_app): | ||
| with TestClient(ws_app, raise_server_exceptions=False) as c: | ||
| yield c | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def authed_client(ws_app): | ||
| record = ws_app.state.auth.find_user("admin") | ||
| token = ws_app.state.auth.create_session(user_id=record["id"], long_lived=True) | ||
| with TestClient(ws_app, raise_server_exceptions=False) as c: | ||
| c.cookies.set("taos_session", token) | ||
| yield c |
There was a problem hiding this comment.
3. Ws fixtures run full lifespan 🐞 Bug ➹ Performance
The new ws_app/authed_client/unauthed_client fixtures build a full create_app() and wrap it in TestClient, which runs the full FastAPI lifespan (initializing many stores and starting background services). This makes these canvas tests significantly heavier than the existing async client fixture pattern that bypasses lifespan, increasing test runtime and the risk of lifecycle-related flakes.
Agent Prompt
## Issue description
WebSocket tests in `tests/test_routes_canvas.py` create a full application via `create_app()` and run it under `TestClient`, triggering the full app lifespan (large init + background services). This is disproportionate for validating the `/ws/canvas/{canvas_id}` auth gate.
## Issue Context
`tinyagentos.app.create_app()` defines a lifespan that initializes many stores and starts background services (cluster manager, backend catalog, provider refresher, etc.). The main async `client` fixture in `tests/conftest.py` explicitly bypasses lifespan and manually initializes only what’s needed.
## Fix Focus Areas
- tests/test_routes_canvas.py[15-51]
- tinyagentos/app.py[503-520]
- tinyagentos/app.py[960-1016]
- tests/conftest.py[279-466]
## Implementation notes
Choose one:
1) Remove these WS tests (preferred if redundant with `tests/test_ws_auth.py`).
2) If you keep WS tests here, avoid full `create_app()`:
- Build a minimal `FastAPI()` instance.
- Attach `app.state.auth = AuthManager(tmp_path)` and `app.state.chat_hub = ChatHub()`.
- Include only `tinyagentos.routes.canvas.router`.
- Then use `TestClient(minimal_app)` for websocket_connect.
This keeps the test scoped to the endpoint under test and avoids the expensive global lifespan.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Reviewed by step-3.7-flash · Input: 202.6K · Output: 20.5K · Cached: 1.2M |
1 participant
Autonomous build of board card tsk-6pf7bz.
Files:
tests/test_routes_canvas.py | 285 ++++++++++++++++++++++++--------------------
1 file changed, 156 insertions(+), 129 deletions(-)
Summary by CodeRabbit