Add route tests for torrent - #2249
Conversation
📝 WalkthroughWalkthroughAdded asynchronous route tests for torrent settings retrieval and updates. The tests also cover response types, persisted values, unavailable settings storage, and invalid upload limits. ChangesTorrent settings route coverage
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
PR Summary by QodoAdd route tests for torrent settings endpoints
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
Reviewed: genuine gap filled - existing torrent tests were store/unit-level only (zero route calls); this adds real-caller route coverage with proper negatives (503 store-missing with exact error, 422 validation). Verified app.py:835 does bind torrent_settings_store in prod, so the fixture graft is compensating for the test app's lifespan, not masking a wiring hole - the grafted object is the same store instance prod binds. Nit only, not blocking: if the client fixture ran the full lifespan the graft would be unnecessary. Merging on green. |
![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 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_torrent.py`:
- Around line 56-62: Extend the test after the existing PUT response assertions
by sending a GET request to the same torrent settings endpoint and asserting a
successful response. Parse the GET payload and verify seed_enabled,
upload_rate_limit_kbps, and max_active_seeds match the values submitted,
confirming persistence through the settings store.
🪄 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: aca1cb96-c229-44e7-98ec-21f1484b2a4c
📒 Files selected for processing (1)
tests/test_routes_torrent.py
| resp = await client.put("/api/torrent/settings", json=body) | ||
| assert resp.status_code == 200 | ||
| data = resp.json() | ||
| assert data["status"] == "saved" | ||
| assert data["seed_enabled"] is False | ||
| assert data["upload_rate_limit_kbps"] == 2048 | ||
| assert data["max_active_seeds"] == 5 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the persisted settings after the PUT request.
The test only checks the PUT response. A route can return these values without writing them to the settings store. Send a GET request after Line 62 and assert the saved values.
Proposed test update
assert data["seed_enabled"] is False
assert data["upload_rate_limit_kbps"] == 2048
assert data["max_active_seeds"] == 5
+
+ persisted = (await client.get("/api/torrent/settings")).json()
+ assert persisted["seed_enabled"] is False
+ assert persisted["upload_rate_limit_kbps"] == 2048
+ assert persisted["max_active_seeds"] == 5📝 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.
| resp = await client.put("/api/torrent/settings", json=body) | |
| assert resp.status_code == 200 | |
| data = resp.json() | |
| assert data["status"] == "saved" | |
| assert data["seed_enabled"] is False | |
| assert data["upload_rate_limit_kbps"] == 2048 | |
| assert data["max_active_seeds"] == 5 | |
| resp = await client.put("/api/torrent/settings", json=body) | |
| assert resp.status_code == 200 | |
| data = resp.json() | |
| assert data["status"] == "saved" | |
| assert data["seed_enabled"] is False | |
| assert data["upload_rate_limit_kbps"] == 2048 | |
| assert data["max_active_seeds"] == 5 | |
| persisted = (await client.get("/api/torrent/settings")).json() | |
| assert persisted["seed_enabled"] is False | |
| assert persisted["upload_rate_limit_kbps"] == 2048 | |
| assert persisted["max_active_seeds"] == 5 |
🤖 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_torrent.py` around lines 56 - 62, Extend the test after the
existing PUT response assertions by sending a GET request to the same torrent
settings endpoint and asserting a successful response. Parse the GET payload and
verify seed_enabled, upload_rate_limit_kbps, and max_active_seeds match the
values submitted, confirming persistence through the settings store.
|
nemotron-ultra-kilo review VERDICT: Tests are functional but have significant brittleness and coverage gaps.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: Tests cover basic happy/error paths but have fragile setup, missing validation cases, and potential state leakage.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Private test internals
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| store = client._transport.app.state.download_manager._torrent_settings_store | ||
| monkeypatch.setattr( | ||
| client._transport.app.state, "torrent_settings_store", store, raising=False | ||
| ) |
There was a problem hiding this comment.
1. Private test internals 🐞 Bug ⚙ Maintainability
The new tests depend on private implementation details (client._transport.app and DownloadManager._torrent_settings_store), which makes the test suite fragile to httpx/transport changes and internal refactors. This can cause CI failures even when the actual torrent settings API behavior remains correct.
Agent Prompt
### Issue description
`tests/test_routes_torrent.py` accesses `client._transport.app` (httpx private field) and `download_manager._torrent_settings_store` (DownloadManager private field) to reach `app.state` and the torrent settings store. This creates brittle tests that can break on dependency upgrades or refactors unrelated to route correctness.
### Issue Context
- The route under test reads the store via `getattr(request.app.state, "torrent_settings_store", None)`.
- The `client` fixture constructs an `AsyncClient(transport=ASGITransport(app=app), ...)` but tests should not rely on `AsyncClient._transport` being present or having an `.app` attribute.
### Fix Focus Areas
- tests/test_routes_torrent.py[8-13]
- tests/test_routes_torrent.py[18-24]
- tests/test_routes_torrent.py[35-40]
- tests/test_routes_torrent.py[45-50]
- tests/test_routes_torrent.py[65-70]
### Suggested fix
1. Add the `app` fixture to the tests’ parameters (pytest will provide the *same* `app` instance used by `client`, since `client` depends on `app`).
2. Patch `app.state.torrent_settings_store` directly via `monkeypatch` (no `client._transport` access).
3. Avoid `download_manager._torrent_settings_store` by creating a store explicitly:
```py
from tinyagentos.torrent_settings import TorrentSettingsStore
store = TorrentSettingsStore(app.state.data_dir / "torrent_settings.json")
monkeypatch.setattr(app.state, "torrent_settings_store", store, raising=False)
```
This keeps the tests aligned to the public contract: routes read the store from `app.state.torrent_settings_store`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_torrent_settings_returns_200(client, monkeypatch): | ||
| store = client._transport.app.state.download_manager._torrent_settings_store |
There was a problem hiding this comment.
CRITICAL: Unchecked chained attribute access on download_manager._torrent_settings_store
If download_manager is ever None or lacks _torrent_settings_store, the test setup raises AttributeError before the endpoint is called, masking actual endpoint failures. Use getattr with a fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_torrent_settings_response_shape(client, monkeypatch): | ||
| store = client._transport.app.state.download_manager._torrent_settings_store |
There was a problem hiding this comment.
CRITICAL: Unchecked chained attribute access on download_manager._torrent_settings_store
If download_manager is ever None or lacks _torrent_settings_store, the test setup raises AttributeError before the endpoint is called, masking actual endpoint failures. Use getattr with a fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_put_torrent_settings_happy_path(client, monkeypatch): | ||
| store = client._transport.app.state.download_manager._torrent_settings_store |
There was a problem hiding this comment.
CRITICAL: Unchecked chained attribute access on download_manager._torrent_settings_store
If download_manager is ever None or lacks _torrent_settings_store, the test setup raises AttributeError before the endpoint is called, masking actual endpoint failures. Use getattr with a fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| resp = await client.put("/api/torrent/settings", json=body) | ||
| assert resp.status_code == 200 | ||
| data = resp.json() | ||
| assert data["status"] == "saved" |
There was a problem hiding this comment.
WARNING: Test doesn't verify store.save() was called
The test checks the response says "saved" but doesn't verify that store.save() was actually invoked. A buggy implementation that skips save() would still pass this test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "/api/torrent/settings", | ||
| json={"upload_rate_limit_kbps": -1}, | ||
| ) | ||
| assert resp.status_code == 422 |
There was a problem hiding this comment.
WARNING: 422 test doesn't verify response body
The test only checks resp.status_code == 422 without asserting the response body contains validation error details. A 422 could be returned for reasons other than Pydantic validation failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (1 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 89.7K · Output: 22.4K · Cached: 716.7K |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
1 participant
CARD TITLE (intent, not commit subject): Add route tests for torrent
Autonomous build of board card tsk-j7dgjw.
Files:
tests/test_routes_torrent.py | 81 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
Summary by CodeRabbit