refactor: test suite overhaul — real Redis, shared fixtures, E2E tests#899
Merged
refactor: test suite overhaul — real Redis, shared fixtures, E2E tests#899
Conversation
Comprehensive spec for eliminating mock abuse, consolidating duplicate fixtures, adding missing E2E tests, and establishing testing guidelines in TESTING.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sultBackend Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d fixtures Replace per-file session/client/mock_sio fixtures and helper functions with shared conftest fixtures (client, session, mock_sio) and helpers module (create_test_user_in_db, create_test_room, auth_header). Convert MagicMock SIO assertions to MockSioServer emitted list pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d fixtures - Delete per-file session/client/redis/sio fixtures and local helpers - Use shared client, session, mock_sio from conftest.py - Convert SIO assertions from MagicMock to MockSioServer.emitted pattern - Remove unused mock_sio params from tests that don't assert emissions - Remove unused event model imports (BookmarksInvalidate, etc.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ures Replace per-file session/client/mock_sio fixtures with shared conftest fixtures. Convert MagicMock SIO assertions to MockSioServer.emitted pattern. Replace mock Redis in screenshots tests with real Redis pre-population via RedisKey helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ures - Delete per-file session/client/redis/sio fixtures and local helpers - Use shared client, session, mock_sio from conftest.py - Convert SIO assertions to MockSioServer.emitted pattern - Remove unused imports and mock_sio/media_path params from non-asserting tests - screenshots retains per-file client fixture for MediaPathDep override Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… fixtures Remove per-file session/client/redis/sio fixtures and replace with shared conftest fixtures (session, client, redis_client, mock_sio, frame_storage). Also adds is_superuser param to create_test_user_in_db helper and converts mock_redis AsyncMock usage in progress tests to real Redis client. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fixtures - Delete per-file session/client/redis/sio fixtures and local helpers - Convert SIO assertions to MockSioServer.emitted pattern - Add is_superuser param to create_test_user_in_db helper - Remove unused imports (Room, LockUpdate, FrameSelectionUpdate) - Remove unused redis_client params from non-Redis-asserting tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… fixtures Replaces per-file session/client/mock_sio fixtures and _create_user/_create_room/_auth_header helpers with shared conftest fixtures (client, session, mock_sio, result_backend) and helpers.py utilities (create_test_user_in_db, create_test_room, auth_header). - test_routes_geometries.py: removed geometry_session/geometry_client/mock_sio fixtures; converted AsyncMock Redis to real Redis (flushed per test); seeded real Redis for test_delete_rejects_active_camera using RedisKey.active_cameras pattern; converted SIO assertions to MockSioServer.emitted pattern. - test_isosurface.py: removed iso_session/iso_client fixtures; replaced AsyncMock Redis + AsyncMock result_backend with real Redis + InMemoryResultBackend from shared client. - test_trajectory.py: removed traj_client fixture and per-file helpers; uses shared client. - test_frames_provider_dispatch.py: removed prov_session/prov_client/prov_result_backend fixtures and local InMemoryResultBackend class; uses shared client/session/result_backend. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…real servers - test_resolve_token.py: replace httpx.Client mocks with real server calls (guest_login, login_with_credentials) - test_cli_auth.py: replace StateFile and _resolve_url patches with real server + monkeypatch; keep httpx.Client mock for browser-flow login tests with # why: comments - test_cli_agent/test_auth.py: replace StateFile patch with monkeypatch; keep httpx.Client mock for orchestration tests with # why: comments - test_client_settings.py: replace StateFileSource.__call__ patch with StateFile(directory=tmp_path) redirect via autouse fixture - test_client_api.py: replace three StateFileSource.__call__ patches with StateFile(directory=tmp_path) monkeypatch; remove unused unittest.mock import - test_state_file_source.py: replace _is_pid_alive patches with real PID (os.getpid()) and _DEAD_PID=999999999; replace _is_url_healthy patches with real server_factory or _DEAD_URL; keep two patches for remote/preference logic with # why: comments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add inline # why: comments to all monkeypatch.setattr, @patch, and patch.dict calls across four test files. Each comment explains the testing rationale (e.g., filesystem isolation, preventing real I/O, orchestration testing) to improve code clarity and maintainability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Source Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates per-file test fixtures into shared fixtures/helpers, enforces explicit mocking constraints via new docs, updates many route tests to consume centralized fixtures ( Changes
Sequence Diagram(s)sequenceDiagram
participant ClientA as Client A
participant Server as Uvicorn Server
participant Storage as FrameStorage/DB/Redis
participant ClientB as Client B
ClientA->>Server: connect(room_id, token)
Server->>Storage: persist room state / frames
ClientA->>Server: append_frame(frame)
Server->>Storage: store frame + metadata
ClientA->>Server: disconnect()
ClientB->>Server: connect(same room_id, token_or_new)
Server->>Storage: fetch persisted frames for room_id
Storage-->>Server: return frames
Server-->>ClientB: deliver frames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. Comment |
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
- Coverage 91.94% 91.74% -0.20%
==========================================
Files 242 245 +3
Lines 23536 22791 -745
==========================================
- Hits 21639 20909 -730
+ Misses 1897 1882 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/zndraw/test_default_camera.py (1)
43-48: Consider using theauth_headerhelper for consistency.This file uses inline
{"Authorization": f"Bearer {token}"}dicts while other refactored test files use theauth_header(token)helper. Using the helper would improve consistency across the test suite.Example refactor
resp = await client.get( f"/v1/rooms/{room.id}/default-camera", - headers={"Authorization": f"Bearer {token}"}, + headers=auth_header(token), )Apply similar changes to other endpoints in this file (lines 58-62, 66-68, 81-85, 98-102, 113-117, 119-123, 127-130, 145-149, 152-155, 159-162).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_default_camera.py` around lines 43 - 48, Replace inline Authorization header dicts like {"Authorization": f"Bearer {token}"} used in the test HTTP calls (e.g., the client.get to "/v1/rooms/{room.id}/default-camera" and the other requests at the listed ranges) with the existing auth_header(token) helper for consistency; locate calls to client.get/client.post/client.patch/client.delete in tests/zndraw/test_default_camera.py and swap the headers argument to headers=auth_header(token) (or remove headers when auth_header is already being passed elsewhere) so all requests use the shared auth_header helper.tests/zndraw/test_routes_edit_lock.py (2)
143-167: Add type annotation formock_sioparameter.The
mock_sioparameter is missing its type annotation, unlike the same parameter in other test files (e.g.,test_routes_bookmarks.pyline 149).Suggested fix
`@pytest.mark.asyncio` async def test_acquire_edit_lock_broadcasts_lock_update( client: AsyncClient, session: AsyncSession, - mock_sio, + mock_sio: MockSioServer, ) -> None:Also update the import at line 7:
-from helpers import auth_header, create_test_room, create_test_user_in_db +from helpers import MockSioServer, auth_header, create_test_room, create_test_user_in_db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_routes_edit_lock.py` around lines 143 - 167, Add a type annotation for the mock_sio parameter in test_acquire_edit_lock_broadcasts_lock_update (e.g., use the same mock SocketIO server/test client type used in other tests) and update the module import to expose that type so the name is available for annotation; modify the test signature to annotate mock_sio with that imported type (matching other tests like test_routes_bookmarks.py) so linters and IDEs recognize the mock_sio fixture type.
385-413: Add type annotation formock_sioparameter.Same missing type annotation as noted above.
Suggested fix
`@pytest.mark.asyncio` async def test_release_edit_lock_broadcasts_lock_update( client: AsyncClient, session: AsyncSession, - mock_sio, + mock_sio: MockSioServer, ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_routes_edit_lock.py` around lines 385 - 413, The test function test_release_edit_lock_broadcasts_lock_update is missing a type annotation for the mock_sio parameter; add a simple annotation (e.g., mock_sio: Any) and import Any from typing at the top of the test module so the parameter is typed, or use the appropriate project fixture/interface type if available, ensuring the signature reads test_release_edit_lock_broadcasts_lock_update(client: AsyncClient, session: AsyncSession, mock_sio: Any) -> None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-03-26-test-suite-overhaul-design.md`:
- Around line 89-96: Update the fenced code block that lists the conftest.py
fixtures by adding a language specifier after the opening backticks (e.g.,
change ``` to ```text) so the block renders and lints correctly; ensure the
opening fence becomes ```text and the closing fence remains ``` in the same
snippet describing the conftest.py fixtures in the test-suite-overhaul design
doc.
In `@tests/zndraw/test_auth_endpoints.py`:
- Around line 111-115: The setup registration call using
client.post("/v1/auth/register", json={"email": setup_email, "password":
"password123"}) must be asserted to ensure the precondition actually succeeded;
after the await client.post(...) in the conditional that checks setup_email, add
an assertion checking the response status code (e.g., response.status_code ==
201 or response.status == 201) and optionally validate the response body to
guarantee the duplicate-email test is running from a clean, expected state.
In `@tests/zndraw/test_resolve_token.py`:
- Around line 32-38: The test named test_login_with_credentials_success
currently calls guest_login(server) and duplicates test_guest_login_success;
change it to call login_with_credentials using the admin credentials provided by
the server_auth fixture (e.g., obtain email and password from server_auth or
server_auth["admin"] and pass them to login_with_credentials(server, email,
password)) and assert the returned value is a non-empty JWT string;
alternatively, if credentials aren't available, remove this redundant test.
Ensure you update the docstring to reflect login_with_credentials and reference
the server_auth fixture in the test body.
In `@tests/zndraw/test_routes_geometries.py`:
- Around line 658-676: In
test_geometry_endpoints_return_404_for_nonexistent_room, the returned tuple from
create_test_user_in_db is unpacked into user, token but user is unused; change
the unpack to _user, token (or prefix user with an underscore) to satisfy the
linter and signal the unused variable while keeping the call to
create_test_user_in_db intact.
In `@tests/zndraw/test_state_file_source.py`:
- Around line 177-187: The tests are tripping Ruff S105/S106 due to hardcoded
token/password literals; update tests like
test_token_local_server_uses_access_token to stop using raw string secrets by
either (A) replacing literals passed into _local_entry and entry.access_token
("raw-admin", "real.jwt") with a shared test fixture or helper (e.g., a
fake_token fixture or a make_fake_token() helper) and consume that fixture in
server_factory/state_file/_make_source usages, or (B) explicitly suppress the
lint for the specific assignments by appending "# noqa: S105,S106" to the
offending lines; locate these changes around the test function name
test_token_local_server_uses_access_token and helper _local_entry to apply
consistently to the other listed tests.
---
Nitpick comments:
In `@tests/zndraw/test_default_camera.py`:
- Around line 43-48: Replace inline Authorization header dicts like
{"Authorization": f"Bearer {token}"} used in the test HTTP calls (e.g., the
client.get to "/v1/rooms/{room.id}/default-camera" and the other requests at the
listed ranges) with the existing auth_header(token) helper for consistency;
locate calls to client.get/client.post/client.patch/client.delete in
tests/zndraw/test_default_camera.py and swap the headers argument to
headers=auth_header(token) (or remove headers when auth_header is already being
passed elsewhere) so all requests use the shared auth_header helper.
In `@tests/zndraw/test_routes_edit_lock.py`:
- Around line 143-167: Add a type annotation for the mock_sio parameter in
test_acquire_edit_lock_broadcasts_lock_update (e.g., use the same mock SocketIO
server/test client type used in other tests) and update the module import to
expose that type so the name is available for annotation; modify the test
signature to annotate mock_sio with that imported type (matching other tests
like test_routes_bookmarks.py) so linters and IDEs recognize the mock_sio
fixture type.
- Around line 385-413: The test function
test_release_edit_lock_broadcasts_lock_update is missing a type annotation for
the mock_sio parameter; add a simple annotation (e.g., mock_sio: Any) and import
Any from typing at the top of the test module so the parameter is typed, or use
the appropriate project fixture/interface type if available, ensuring the
signature reads test_release_edit_lock_broadcasts_lock_update(client:
AsyncClient, session: AsyncSession, mock_sio: Any) -> None.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e1f8da5-13a6-445b-9ab0-77debdda32ad
📒 Files selected for processing (34)
docs/superpowers/specs/2026-03-26-test-suite-overhaul-design.mdtests/zndraw/README.mdtests/zndraw/conftest.pytests/zndraw/helpers.pytests/zndraw/test_auth_endpoints.pytests/zndraw/test_chat.pytests/zndraw/test_cli.pytests/zndraw/test_cli_agent/test_auth.pytests/zndraw/test_cli_auth.pytests/zndraw/test_client_api.pytests/zndraw/test_client_settings.pytests/zndraw/test_default_camera.pytests/zndraw/test_e2e_client_persistence.pytests/zndraw/test_e2e_guest_auth.pytests/zndraw/test_e2e_state_file_source.pytests/zndraw/test_frames_provider_dispatch.pytests/zndraw/test_gif.pytests/zndraw/test_isosurface.pytests/zndraw/test_local_token_auth.pytests/zndraw/test_local_token_jwt_regression.pytests/zndraw/test_progress.pytests/zndraw/test_resolve_token.pytests/zndraw/test_routes_bookmarks.pytests/zndraw/test_routes_edit_lock.pytests/zndraw/test_routes_figures.pytests/zndraw/test_routes_frame_selection.pytests/zndraw/test_routes_frames.pytests/zndraw/test_routes_geometries.pytests/zndraw/test_routes_presets.pytests/zndraw/test_routes_selection_groups.pytests/zndraw/test_routes_step.pytests/zndraw/test_screenshots.pytests/zndraw/test_state_file_source.pytests/zndraw/test_trajectory.py
docs/superpowers/specs/2026-03-26-test-suite-overhaul-design.md
Outdated
Show resolved
Hide resolved
Comment on lines
+177
to
187
| def test_token_local_server_uses_access_token(state_file, server_factory): | ||
| """Local server with access_token returns it as token.""" | ||
| instance = server_factory({}) | ||
| url = instance.url | ||
| pid = os.getpid() | ||
| entry = _local_entry(pid=pid, local_token="raw-admin") | ||
| entry.access_token = "real.jwt" | ||
| state_file.add_server("http://localhost:8000", entry) | ||
| state_file.add_server(url, entry) | ||
| source = _make_source(state_file) | ||
| with ( | ||
| patch("zndraw.settings_sources._is_pid_alive", return_value=True), | ||
| patch("zndraw.settings_sources._is_url_healthy", return_value=True), | ||
| ): | ||
| result = source() | ||
| result = source() | ||
| assert result["token"] == "real.jwt" |
There was a problem hiding this comment.
Address Ruff S105/S106 hits on test token/password literals.
Line 182, Line 183, Line 187, Line 195, Line 207, Line 218, and Line 240 currently trip security-lint rules. These are test fixtures, but they still need explicit suppression or fixture indirection to keep lint green.
✅ Minimal lint-safe patch pattern
- entry = _local_entry(pid=pid, local_token="raw-admin")
- entry.access_token = "real.jwt"
+ entry = _local_entry(pid=pid, local_token="raw-admin") # noqa: S106
+ entry.access_token = "real.jwt" # noqa: S105
@@
- assert result["token"] == "real.jwt"
+ assert result["token"] == "real.jwt" # noqa: S105
@@
- state_file.add_server(url, _local_entry(pid=pid, local_token="raw-only"))
+ state_file.add_server(url, _local_entry(pid=pid, local_token="raw-only")) # noqa: S106
@@
- access_token="stored.jwt",
+ access_token="stored.jwt", # noqa: S106
@@
- assert result["token"] == "stored.jwt"
+ assert result["token"] == "stored.jwt" # noqa: S105
@@
- access_token="override.jwt",
+ access_token="override.jwt", # noqa: S106Also applies to: 190-198, 201-218, 233-248
🧰 Tools
🪛 Ruff (0.15.7)
[error] 182-182: Possible hardcoded password assigned to argument: "local_token"
(S106)
[error] 183-183: Possible hardcoded password assigned to: "access_token"
(S105)
[error] 187-187: Possible hardcoded password assigned to: "token"
(S105)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/zndraw/test_state_file_source.py` around lines 177 - 187, The tests are
tripping Ruff S105/S106 due to hardcoded token/password literals; update tests
like test_token_local_server_uses_access_token to stop using raw string secrets
by either (A) replacing literals passed into _local_entry and entry.access_token
("raw-admin", "real.jwt") with a shared test fixture or helper (e.g., a
fake_token fixture or a make_fake_token() helper) and consume that fixture in
server_factory/state_file/_make_source usages, or (B) explicitly suppress the
lint for the specific assignments by appending "# noqa: S105,S106" to the
offending lines; locate these changes around the test function name
test_token_local_server_uses_access_token and helper _local_entry to apply
consistently to the other listed tests.
- Fix all 18 ruff errors (ASYNC109, PT006, ARG005, RUF059, E501, TC003) - Add # why: comments on all monkeypatch/patch calls in CLI auth tests - Refactor test_screenshots.py: replace per-file client with thin media_path fixture - Add MockSioServer type annotations in test_routes_edit_lock.py - Use auth_header() helper consistently in test_default_camera.py - Fix test_login_with_credentials_success to actually test login_with_credentials - Assert setup precondition in test_register_fails - Add language specifier to spec code block Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/zndraw/test_cli_auth.py (1)
152-156: Consider extracting repeated mock client setup to a helper or fixture.The
MagicMockwith__enter__/__exit__setup for the httpx context manager is duplicated in all three login tests (lines 152-156, 194-198, 232-236). A shared fixture could reduce this repetition.♻️ Example helper fixture
`@pytest.fixture` def mock_httpx_client(): """Reusable mock httpx.Client supporting context manager protocol.""" mock_client = MagicMock() mock_client.__enter__ = MagicMock(return_value=mock_client) mock_client.__exit__ = MagicMock(return_value=False) return mock_clientThen in tests:
- mock_client = MagicMock() - mock_client.__enter__ = MagicMock(return_value=mock_client) - mock_client.__exit__ = MagicMock(return_value=False) - mock_client.post.return_value = challenge_resp - mock_client.get.side_effect = mock_get + mock_httpx_client.post.return_value = challenge_resp + mock_httpx_client.get.side_effect = mock_get🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_cli_auth.py` around lines 152 - 156, Extract the repeated MagicMock context-manager setup into a pytest fixture (e.g., mock_httpx_client) that returns a MagicMock with __enter__ and __exit__ configured, then update the three tests (the ones currently creating mock_client and setting post/get/side_effect) to accept that fixture and only set mock_client.post.return_value and mock_client.get.side_effect inside each test; this replaces the duplicated block that creates mock_client and its context-manager methods and centralizes the reusable setup.tests/zndraw/test_e2e_state_file_source.py (1)
68-80: Consider suppressing S106 lint warning for test token literal.The static analysis flags
access_token="real.jwt.token"as a potential hardcoded password. Since this is test data, consider adding a# noqa: S106comment for lint compliance, or use a constant like_TEST_TOKEN = "real.jwt.token" # noqa: S105at module level.Proposed fix
+# Test token literal (not a real secret) +_TEST_TOKEN = "real.jwt.token" # noqa: S105 + def test_statefile_discovers_server_with_access_token(server_factory, tmp_path: Path): ... entry = ServerEntry( added_at=datetime.now(UTC), last_used=datetime.now(UTC), pid=os.getpid(), - access_token="real.jwt.token", + access_token=_TEST_TOKEN, ) ... - assert result.get("token") == "real.jwt.token" + assert result.get("token") == _TEST_TOKEN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_e2e_state_file_source.py` around lines 68 - 80, The test currently sets a literal access_token in the ServerEntry (access_token="real.jwt.token") which triggers S106; update the test to suppress the lint warning by either appending a noqa (e.g., add "# noqa: S106" to the access_token assignment) or move the value to a module-level constant like _TEST_TOKEN = "real.jwt.token" with a noqa and use ServerEntry(access_token=_TEST_TOKEN); modify the test in tests/zndraw/test_e2e_state_file_source.py around the ServerEntry construction to include one of these changes so the linter ignores the hardcoded test token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/zndraw/test_screenshots.py`:
- Around line 24-37: The fixture media_path_fixture sets
app.dependency_overrides[get_media_path] but never removes it, leaking the
override to other tests; change the fixture to use a yield (or try/finally) so
you set app.dependency_overrides[get_media_path] = lambda: media before yielding
the media Path and then remove the override (e.g., pop or del
app.dependency_overrides[get_media_path]) after the yield to ensure cleanup;
reference the media_path_fixture function, app.dependency_overrides, and
get_media_path when making the change.
---
Nitpick comments:
In `@tests/zndraw/test_cli_auth.py`:
- Around line 152-156: Extract the repeated MagicMock context-manager setup into
a pytest fixture (e.g., mock_httpx_client) that returns a MagicMock with
__enter__ and __exit__ configured, then update the three tests (the ones
currently creating mock_client and setting post/get/side_effect) to accept that
fixture and only set mock_client.post.return_value and
mock_client.get.side_effect inside each test; this replaces the duplicated block
that creates mock_client and its context-manager methods and centralizes the
reusable setup.
In `@tests/zndraw/test_e2e_state_file_source.py`:
- Around line 68-80: The test currently sets a literal access_token in the
ServerEntry (access_token="real.jwt.token") which triggers S106; update the test
to suppress the lint warning by either appending a noqa (e.g., add "# noqa:
S106" to the access_token assignment) or move the value to a module-level
constant like _TEST_TOKEN = "real.jwt.token" with a noqa and use
ServerEntry(access_token=_TEST_TOKEN); modify the test in
tests/zndraw/test_e2e_state_file_source.py around the ServerEntry construction
to include one of these changes so the linter ignores the hardcoded test token.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f44f851-c3a2-40e4-a7ac-1d7844fb96c7
📒 Files selected for processing (13)
docs/superpowers/specs/2026-03-26-test-suite-overhaul-design.mdtests/zndraw/helpers.pytests/zndraw/test_auth_endpoints.pytests/zndraw/test_cli_agent/test_auth.pytests/zndraw/test_cli_auth.pytests/zndraw/test_default_camera.pytests/zndraw/test_e2e_state_file_source.pytests/zndraw/test_local_token_jwt_regression.pytests/zndraw/test_resolve_token.pytests/zndraw/test_routes_edit_lock.pytests/zndraw/test_routes_geometries.pytests/zndraw/test_screenshots.pytests/zndraw/test_state_file_source.py
✅ Files skipped from review due to trivial changes (1)
- tests/zndraw/test_local_token_jwt_regression.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/zndraw/test_default_camera.py
- tests/zndraw/test_routes_edit_lock.py
- tests/zndraw/helpers.py
- tests/zndraw/test_auth_endpoints.py
- test_screenshots.py: media_path fixture now yields and cleans up override - test_cli_auth.py: extract mock_httpx_client fixture and _challenge_response helper to deduplicate login test setup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/zndraw/test_cli_auth.py (1)
190-214: Tighten the rejection/expiry assertions.Both tests currently pass on any non-zero exit, so a generic failure would satisfy them and the 404/410-specific behavior could drift unnoticed. Since these paths are supposed to exercise distinct branches, assert the exact exit code and user-facing message, and keep the no-token side effect checked as well.
🧪 Suggested strengthening
): result = runner.invoke(app, ["auth", "login"]) - assert result.exit_code != 0 + assert result.exit_code == 1 + assert "Login rejected." in result.output + assert state_file.get_token(server) is None): result = runner.invoke(app, ["auth", "login"]) - assert result.exit_code != 0 + assert result.exit_code == 1 + assert "Login challenge expired." in result.output + assert state_file.get_token(server) is NoneAlso applies to: 216-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_cli_auth.py` around lines 190 - 214, The test test_auth_login_rejected currently only asserts result.exit_code != 0 which is too weak; change it to assert the exact non-zero exit code expected from the CLI (e.g., 1), assert the CLI output (result.output or result.stdout) contains the specific user-facing message for a rejected challenge (mentioning 404 or "rejected"/"expired" as appropriate), and assert the StateFile (via the state_file fixture) still has no token saved; apply the same tightening to the companion expiry test (the one around lines 216-239) so it asserts the exact exit code, the specific expiry message (e.g., 410/expired), and that no token was written to StateFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/zndraw/test_cli_auth.py`:
- Around line 58-83: The test test_auth_status_with_stored_token currently only
asserts "server" in data which passes for other branches; update the assertions
to explicitly verify the stored-token branch by checking that data["server"] ==
server and data["token_source"] == "stored" (while keeping the existing setup
that mocks StateFile and _resolve_url and uses the real token), so the test
ensures status() returned the stored-token path.
---
Nitpick comments:
In `@tests/zndraw/test_cli_auth.py`:
- Around line 190-214: The test test_auth_login_rejected currently only asserts
result.exit_code != 0 which is too weak; change it to assert the exact non-zero
exit code expected from the CLI (e.g., 1), assert the CLI output (result.output
or result.stdout) contains the specific user-facing message for a rejected
challenge (mentioning 404 or "rejected"/"expired" as appropriate), and assert
the StateFile (via the state_file fixture) still has no token saved; apply the
same tightening to the companion expiry test (the one around lines 216-239) so
it asserts the exact exit code, the specific expiry message (e.g., 410/expired),
and that no token was written to StateFile.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c881e760-4e07-4fda-b536-3ee6b6574a13
📒 Files selected for processing (2)
tests/zndraw/test_cli_auth.pytests/zndraw/test_screenshots.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/zndraw/test_screenshots.py
…_token Replace weak `"server" in data` check with explicit value assertions so the test actually verifies the stored-token code path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add negative authorization test verifying guest B cannot write frames to a room locked by guest A (423). Remove leftover _create_user / _create_room / _auth_header aliases in test_routes_frames.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/zndraw/test_e2e_guest_auth.py (1)
146-163: Consider renaming test to match its behavior.The test name says "other room" but actually tests unauthenticated access to the same room the guest created. The docstring correctly describes the intent. Also, adding a status assertion on room creation (line 155-159) would improve debuggability.
Suggested rename and status assertion
`@pytest.mark.asyncio` -async def test_guest_cannot_access_other_room_without_auth(http_client: AsyncClient): +async def test_unauthenticated_frame_access_returns_401(http_client: AsyncClient): """Unauthenticated requests to frame endpoints return 401.""" # Create a room with auth auth_resp = await http_client.post("/v1/auth/guest") token = auth_resp.json()["access_token"] headers = {"Authorization": f"Bearer {token}"} room_id = uuid.uuid4().hex - await http_client.post( + create_resp = await http_client.post( "/v1/rooms", json={"room_id": room_id, "copy_from": "@none"}, headers=headers, ) + assert create_resp.status_code == 201🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_e2e_guest_auth.py` around lines 146 - 163, Rename the test function test_guest_cannot_access_other_room_without_auth to accurately describe its behavior (e.g., test_guest_cannot_access_room_without_auth) and add an assertion after creating the room to verify the POST /v1/rooms call succeeded (assert the response status_code is 200/201) using the response variable from the creation request; update the function name and add the status assertion around the room creation call that uses headers and room_id to ensure failures are easier to debug while keeping the rest of the test (including the unauthenticated GET to /v1/rooms/{room_id}/frames and its 401 assertion) unchanged.tests/zndraw/test_routes_frames.py (1)
379-382: Inconsistent room ID format in 404 test.This test uses string
"nonexistent-room"while all other room-not-found tests use numeric99999(e.g., lines 137, 296, 458, 642). Either format may work, but the inconsistency makes it unclear whether string IDs are intentionally being tested here.Consider aligning with the other tests for consistency:
Suggested fix
- "/v1/rooms/nonexistent-room/frames/0/metadata", + "/v1/rooms/99999/frames/0/metadata",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_routes_frames.py` around lines 379 - 382, The test at the client.get call using path "/v1/rooms/nonexistent-room/frames/0/metadata" is inconsistent with other room-not-found tests that use numeric IDs; update the room ID to match the other tests (e.g., use "99999") so the call becomes "/v1/rooms/99999/frames/0/metadata" in the same test (leave auth_header(token) unchanged) to keep ID format consistent across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/zndraw/test_e2e_guest_auth.py`:
- Around line 112-124: In test_guest_write_multiple_frames, add explicit status
assertions immediately after the HTTP calls that return auth_resp and the room
creation response (the calls to http_client.post for "/v1/auth/guest" and
"/v1/rooms") before you call .json() or proceed; check that
auth_resp.status_code (and the room creation response.status_code) equals the
expected 200/201 (or appropriate success code) and include a helpful message so
failures show the HTTP status and body instead of raising KeyError later when
accessing auth_resp.json()["access_token"] or assuming room creation succeeded.
---
Nitpick comments:
In `@tests/zndraw/test_e2e_guest_auth.py`:
- Around line 146-163: Rename the test function
test_guest_cannot_access_other_room_without_auth to accurately describe its
behavior (e.g., test_guest_cannot_access_room_without_auth) and add an assertion
after creating the room to verify the POST /v1/rooms call succeeded (assert the
response status_code is 200/201) using the response variable from the creation
request; update the function name and add the status assertion around the room
creation call that uses headers and room_id to ensure failures are easier to
debug while keeping the rest of the test (including the unauthenticated GET to
/v1/rooms/{room_id}/frames and its 401 assertion) unchanged.
In `@tests/zndraw/test_routes_frames.py`:
- Around line 379-382: The test at the client.get call using path
"/v1/rooms/nonexistent-room/frames/0/metadata" is inconsistent with other
room-not-found tests that use numeric IDs; update the room ID to match the other
tests (e.g., use "99999") so the call becomes
"/v1/rooms/99999/frames/0/metadata" in the same test (leave auth_header(token)
unchanged) to keep ID format consistent across tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fd27ff8-6025-416e-bc1b-57c78f93732e
📒 Files selected for processing (2)
tests/zndraw/test_e2e_guest_auth.pytests/zndraw/test_routes_frames.py
Comment on lines
+112
to
+124
| @pytest.mark.asyncio | ||
| async def test_guest_write_multiple_frames(http_client: AsyncClient): | ||
| """Guest writes multiple frames and reads them back by index.""" | ||
| auth_resp = await http_client.post("/v1/auth/guest") | ||
| token = auth_resp.json()["access_token"] | ||
| headers = {"Authorization": f"Bearer {token}"} | ||
|
|
||
| room_id = uuid.uuid4().hex | ||
| await http_client.post( | ||
| "/v1/rooms", | ||
| json={"room_id": room_id, "copy_from": "@none"}, | ||
| headers=headers, | ||
| ) |
There was a problem hiding this comment.
Add status assertions before accessing response JSON for clearer failure messages.
If auth or room creation fails, the test will raise a KeyError or fail on later assertions with confusing messages. Adding early status checks improves debuggability.
Proposed fix
`@pytest.mark.asyncio`
async def test_guest_write_multiple_frames(http_client: AsyncClient):
"""Guest writes multiple frames and reads them back by index."""
auth_resp = await http_client.post("/v1/auth/guest")
+ assert auth_resp.status_code == 200
token = auth_resp.json()["access_token"]
headers = {"Authorization": f"Bearer {token}"}
room_id = uuid.uuid4().hex
- await http_client.post(
+ create_resp = await http_client.post(
"/v1/rooms",
json={"room_id": room_id, "copy_from": "@none"},
headers=headers,
)
+ assert create_resp.status_code == 201🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/zndraw/test_e2e_guest_auth.py` around lines 112 - 124, In
test_guest_write_multiple_frames, add explicit status assertions immediately
after the HTTP calls that return auth_resp and the room creation response (the
calls to http_client.post for "/v1/auth/guest" and "/v1/rooms") before you call
.json() or proceed; check that auth_resp.status_code (and the room creation
response.status_code) equals the expected 200/201 (or appropriate success code)
and include a helpful message so failures show the HTTP status and body instead
of raising KeyError later when accessing auth_resp.json()["access_token"] or
assuming room creation succeeded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AsyncMock()andlambda: (None)with real Redis,InMemoryResultBackend, andMockSioServervia shared conftest fixturesconftest.py— net deletion of 1032 linestest_resolve_token.py,test_cli_auth.py,test_client_settings.py,test_client_api.py, andtest_state_file_source.pywith real server teststests/zndraw/README.mddocumenting test hierarchy, fixture rules, and mocking guidelines# why:justification comments to all remainingmonkeypatch/@patchcallsResult: 1429 tests pass, 0 failures. No real bugs discovered (mocks were not hiding production issues).
Test plan
uv run pytest tests/ -q→ 1429 passed, 1 skippedAsyncMock()as Redis/result backend substitute in route testslambda: (None)Redis overrides anywheretests/zndraw/README.mdexists and documents rules# why:justification comments🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests