test(user_shares): add store + route tests with conftest integration#1945
test(user_shares): add store + route tests with conftest integration#1945hognek wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 50 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| r = await shares_client.post( | ||
| "/api/shares", | ||
| json={ | ||
| "resource_type": "project", |
There was a problem hiding this comment.
SUGGESTION: Direction filtering is not actually verified.
Both test_list_incoming_shares and test_list_outgoing_shares filter results only by resource_type (if s["resource_type"] == "project"), never by the direction the route is supposed to honour. A regression where the route ignores direction and returns all shares would still pass these tests, defeating their purpose. Add an assertion that the opposite direction contains none of the created resource ids (e.g. assert the incoming list is empty when only outgoing shares were created, or check a distinguishing field).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| resp = await shares_client.post( | ||
| "/api/shares", | ||
| json={ | ||
| "resource_type": "project", |
There was a problem hiding this comment.
SUGGESTION: Redundant bob user creation duplicates the shares_client logic.
shares_client already guarantees the bob user exists (lines 60-65). This fixture re-checks find_user("bob") and re-invites/completes again. While idempotent-ish, if complete_invite raises on an already-completed invite, bob_client would fail. The duplicate invite/complete block can be removed since shares_client runs first as a dependency.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "write", | ||
| tier="always", | ||
| expires_at="2030-01-01T00:00:00+00:00", | ||
| ) |
There was a problem hiding this comment.
WARNING: These tests assert implementation defaults (tier == "once", status == "pending") and call accept_share/list_shares_received that live in the not-yet-merged store PRs (#1897/#1908).
This PR's description states it Depends on #1897 and #1908. If those PRs change the default tier/status values or rename/remove accept_share, these tests will fail to even import/collect once this PR is merged ahead of them (or vice-versa). Confirm the merge order guarantees the store API matches these assumptions, or mark this PR as blocked until #1897/#1908 land to avoid a red CI state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| r2 = await shares_client.post("/api/shares", json=body) | ||
| assert r2.status_code == 200 | ||
|
|
There was a problem hiding this comment.
SUGGESTION: Hardcoded "admin" username for the self-share rejection check.
test_create_share_to_self_rejected posts to_username="admin" expecting a 400, relying on the authenticated user being named exactly admin. If the conftest client fixture's admin user is ever renamed, this test would flip to a 404 (unknown username) and still pass the == 400? No — it would then fail with 404, so it is somewhat self-checking, but the intent is fragile. Prefer deriving the current username from the authenticated session (e.g. app.state.auth.find_user(...) or the fixture's known admin id) instead of hardcoding "admin".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All four previous findings have been re-verified against current HEAD (
No new issues were found in the changed lines. Files Reviewed (3 files)
Previous Review Summaries (2 snapshots, latest commit b8d7a5e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b8d7a5e)Status: No Issues Found | Recommendation: Merge All four previous findings (the WARNING on
No new issues were found in the changed lines. Files Reviewed (3 files)
Previous review (commit 63712f3)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by hy3:free · Input: 41.2K · Output: 2K · Cached: 153.2K |
…1945) - Use pytest.importorskip for UserSharesStore to decouple merge order (jaylfc#1897) - Add _require_method() guards for accept_share/list_shares_received (jaylfc#1897) - Verify direction filtering excludes incoming shares from outgoing list - Remove redundant bob user creation in bob_client fixture - Replace hardcoded 'admin' username with dynamic session lookup Resolves: 1W + 3S from Kilo review on jaylfc#1945
|
Kilo bot findings (1W + 3S) fixed in b8d7a5e:
Test results: 13/13 store tests skip cleanly when the store module isn't available. Route tests skip at fixture level. Existing aiosqlite daemon-thread hang on teardown is pre-existing. |
|
Heads up: the real CI workflow (test 3.12/3.13 + lint + spa-build) did not trigger on this one, only CLA + the automerge skip registered. A tests-only change can miss the workflow path filters. An empty commit or a rebase onto latest dev should kick the full run, then I can verify and merge. #1946 I have just approved the pending fork CI run. |
Store tests (13): add_share row/optionals/idempotent/uninit, list_shares all/scoped/received/empty, revoke/exists/nonexistent, user_can_access active/expired/none. Follows test_agent_grants_store.py patterns. Route tests (9): create/unknown-404/idempotent/self-400, list out/in, revoke/not-found-404/non-owner-403. Uses conftest client fixture. Conftest: init user_shares store in client fixture (lifespan-owned, bypassed in tests) with graceful fallback for pre-merge state.
…1945) - Use pytest.importorskip for UserSharesStore to decouple merge order (jaylfc#1897) - Add _require_method() guards for accept_share/list_shares_received (jaylfc#1897) - Verify direction filtering excludes incoming shares from outgoing list - Remove redundant bob user creation in bob_client fixture - Replace hardcoded 'admin' username with dynamic session lookup Resolves: 1W + 3S from Kilo review on jaylfc#1945
b8d7a5e to
3dd4211
Compare
2 participants
Summary
Tests for the user-to-user resource sharing feature (taOS #1893 C3), targeting the implementation in PRs #1897 (UserSharesStore) and #1908 (share routes + consent wiring).
Depends on: #1897, #1908
Files
Store tests (13)
add_share: returns_inserted_row, with_optional_fields, idempotent_replace, uninitialised_raises
list_shares: returns_all, empty_for_unknown, scoped_by_owner, received
revoke_share: removes_it, nonexistent_does_not_crash
user_can_access: true_for_active, false_for_expired, false_for_none
Route tests (9)
create_share (200), unknown_username (404), duplicate_idempotent (200), to_self_rejected (400)
list_outgoing (200), list_incoming (200)
revoke (200), nonexistent (404), non_owner_unauthorized (403)
Test results
Store: 13/13 pass
Route: 9/9 pass
Full suite: known aiosqlite daemon-thread hang on teardown (pre-existing issue)