test: adding backend/e2e tests #97
Open
+5,180
−364
Conversation
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
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds many end-to-end tests and fixtures; tightens event/execution typing to use enums; enforces execution ownership checks; extends admin user rate‑limit request models and user-delete result; fixes DLQ nested field access; refactors DI/provider wiring (replay, resource cleaner) and savescript deletion to error when missing. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
16 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/e2e/conftest.py">
<violation number="1" location="backend/tests/e2e/conftest.py:27">
P2: Async fixtures should use pytest_asyncio.fixture so pytest awaits them. Using @pytest.fixture can return a coroutine object instead of the resolved value.</violation>
</file>
<file name="backend/tests/e2e/test_admin_users_routes.py">
<violation number="1" location="backend/tests/e2e/test_admin_users_routes.py:545">
P2: The test should not accept a 500 status for a missing user. This masks real server errors. Assert a 404 (or the intended not-found status) so failures surface correctly.</violation>
</file>
<file name="backend/tests/e2e/test_saga_routes.py">
<violation number="1" location="backend/tests/e2e/test_saga_routes.py:26">
P2: These tests gate all assertions behind `if` checks on the response status and non-empty saga lists, so the tests can pass even when the API returns errors or no sagas. Convert the guards into explicit assertions so failures are caught.</violation>
</file>
<file name="backend/tests/e2e/test_replay_routes.py">
<violation number="1" location="backend/tests/e2e/test_replay_routes.py:134">
P2: These tests skip all assertions when session creation fails, so a broken create endpoint would still pass. Assert the create response is 200 and fail the test otherwise.</violation>
<violation number="2" location="backend/tests/e2e/test_replay_routes.py:161">
P2: Accepting HTTP 500 as a valid outcome hides server-side errors and reduces test effectiveness. These tests should fail on 500 responses and assert the expected 4xx status.</violation>
</file>
<file name="backend/tests/e2e/test_notifications_routes.py">
<violation number="1" location="backend/tests/e2e/test_notifications_routes.py:230">
P3: This test can silently pass without validating the delete response (e.g., when the list call fails or returns no notifications). Explicitly assert or skip so the test doesn’t produce false positives.</violation>
</file>
<file name="backend/tests/e2e/test_admin_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_admin_events_routes.py:291">
P2: The delete test can silently pass when no events are returned, so it won't catch regressions. Add assertions or an explicit skip if no events are found.</violation>
<violation number="2" location="backend/tests/e2e/test_admin_events_routes.py:346">
P2: The test doesn't assert the allowed status codes for replay; it will pass even on unexpected 5xx errors. Assert the response status before the conditional.</violation>
</file>
<file name="backend/tests/e2e/test_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_events_routes.py:228">
P2: This test can pass without asserting anything if the user has no events. That silently skips validation of the single-event endpoint. Assert the precondition (at least one event) or explicitly skip/fail so the test doesn’t pass vacuously.</violation>
<violation number="2" location="backend/tests/e2e/test_events_routes.py:332">
P2: The test silently passes when the publish step fails, which masks regressions in the admin publish/delete flow. Assert that the publish succeeded before continuing.</violation>
</file>
<file name="backend/tests/e2e/test_dlq_routes.py">
<violation number="1" location="backend/tests/e2e/test_dlq_routes.py:237">
P2: Retry policy tests use an unsupported strategy value. The API expects `exponential_backoff`, so this request will be rejected with 422 and the test will fail.</violation>
<violation number="2" location="backend/tests/e2e/test_dlq_routes.py:258">
P2: Retry policy tests use an unsupported strategy value. The API expects `fixed_interval`, so this request will be rejected with 422 and the test will fail.</violation>
<violation number="3" location="backend/tests/e2e/test_dlq_routes.py:279">
P2: Retry policy tests use an unsupported strategy value. The API does not accept `linear`, so this request will be rejected with 422 and the test will fail.</violation>
</file>
<file name="backend/tests/e2e/test_user_settings_routes.py">
<violation number="1" location="backend/tests/e2e/test_user_settings_routes.py:26">
P2: The assertion allows a "system" theme that isn’t defined in the Theme enum (AUTO uses "auto"). This will fail whenever the default theme is returned. Use Theme.AUTO or "auto" instead.</violation>
<violation number="2" location="backend/tests/e2e/test_user_settings_routes.py:54">
P2: Notification settings test uses unsupported fields (email_enabled/push_enabled). This will fail validation against NotificationSettings or not exercise the intended API behavior.</violation>
<violation number="3" location="backend/tests/e2e/test_user_settings_routes.py:60">
P2: Editor update test uses the wrong field name ("line_numbers"). The API expects "show_line_numbers" so this request likely fails validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # --- Request fixtures --- | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
P2: Async fixtures should use pytest_asyncio.fixture so pytest awaits them. Using @pytest.fixture can return a coroutine object instead of the resolved value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/conftest.py, line 27:
<comment>Async fixtures should use pytest_asyncio.fixture so pytest awaits them. Using @pytest.fixture can return a coroutine object instead of the resolved value.</comment>
<file context>
@@ -1,19 +1,113 @@
+# --- Request fixtures ---
+
+
+@pytest.fixture
+def simple_execution_request() -> ExecutionRequest:
+ """Simple python print execution."""
</file context>
| @@ -0,0 +1,229 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
P2: These tests gate all assertions behind if checks on the response status and non-empty saga lists, so the tests can pass even when the API returns errors or no sagas. Convert the guards into explicit assertions so failures are caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_saga_routes.py, line 26:
<comment>These tests gate all assertions behind `if` checks on the response status and non-empty saga lists, so the tests can pass even when the API returns errors or no sagas. Convert the guards into explicit assertions so failures are caught.</comment>
<file context>
@@ -0,0 +1,229 @@
+ f"/api/v1/sagas/execution/{created_execution.execution_id}"
+ )
+
+ if sagas_response.status_code == 200:
+ sagas = SagaListResponse.model_validate(sagas_response.json())
+ if sagas.sagas:
</file context>
| params={"limit": 1}, | ||
| ) | ||
|
|
||
| if list_response.status_code == 200: |
There was a problem hiding this comment.
P3: This test can silently pass without validating the delete response (e.g., when the list call fails or returns no notifications). Explicitly assert or skip so the test doesn’t produce false positives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_notifications_routes.py, line 230:
<comment>This test can silently pass without validating the delete response (e.g., when the list call fails or returns no notifications). Explicitly assert or skip so the test doesn’t produce false positives.</comment>
<file context>
@@ -0,0 +1,244 @@
+ params={"limit": 1},
+ )
+
+ if list_response.status_code == 200:
+ result = NotificationListResponse.model_validate(list_response.json())
+ if result.notifications:
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_admin_settings_routes.py`:
- Around line 75-117: The test test_update_system_settings_full mutates global
SystemSettings and doesn't restore them; to make tests order-independent, fetch
and store the current settings before the PUT (e.g., GET
"/api/v1/admin/settings/" and parse via SystemSettings.model_validate), perform
the update, and in a finally block send a request to restore the saved settings
(using the same test_admin client and the PUT to "/api/v1/admin/settings/" with
the original settings payload or call any existing reset endpoint); ensure you
reference the saved variable when restoring so the global settings are returned
to their prior state regardless of test outcome.
In `@backend/tests/e2e/test_admin_users_routes.py`:
- Around line 537-545: Update the test_delete_user_not_found test to assert a
not-found response instead of accepting a 500; specifically change the assertion
so the response.status_code is 404 (or 404/410 if you intentionally support gone
semantics) for the DELETE to "/api/v1/admin/users/nonexistent-user-id" to ensure
server errors aren't masked by returning 500 in the test.
In `@backend/tests/e2e/test_notifications_routes.py`:
- Around line 89-98: In test_mark_nonexistent_notification_read, replace the
invalid id string "nonexistent-id" with a valid UUID (e.g., generate one with
uuid.uuid4() or use a hard-coded valid UUID string) when calling test_user.put
so the request passes route validation and exercises the 404 branch, and change
the assertion to expect response.status_code == 404 (remove the [404, 400]
alternative); the test function name to update is
test_mark_nonexistent_notification_read and the HTTP call to change is the PUT
to "/api/v1/notifications/{id}/read" invoked via test_user.put.
In `@backend/tests/e2e/test_saga_routes.py`:
- Around line 44-61: The test_get_saga_access_denied currently silently passes
when the initial GET doesn't return 200 or returns an empty saga list; update it
to explicitly assert the initial response is successful and that at least one
saga exists before proceeding: assert sagas_response.status_code == 200, parse
into SagaListResponse.model_validate(...), then assert sagas.sagas is not empty
(or add a retry loop that fails after N attempts), and only then extract saga_id
and call another_user.get(...) expecting 403; refer to
test_get_saga_access_denied, SagaListResponse, created_execution, and saga_id to
locate where to add these assertions/retry.
- Around line 168-196: The test_cancel_saga currently silently returns if no
saga is found; update test_cancel_saga to fail loudly by asserting the GET
/api/v1/sagas/execution/{execution_id} response is 200 and that
SagaListResponse.model_validate(...).sagas is non-empty (or implement a short
retry/poll loop with a timeout that re-GETs the same endpoint until sagas
appear), then proceed to pick saga_id and call POST
/api/v1/sagas/{saga_id}/cancel and validate using SagaCancellationResponse;
ensure assertions cover both the status_code and presence of sagas so the test
fails when none are returned.
- Around line 208-229: The test test_cancel_other_users_saga_forbidden silently
passes when no saga is returned; change it to assert that a saga exists (i.e.,
validate SagaListResponse and assert sagas.sagas is not empty) before extracting
saga_id, or implement a short retry loop querying
/api/v1/sagas/execution/{execution_id} until a saga appears (with a sensible
timeout) to avoid intermittent false negatives; then continue to call
/api/v1/sagas/{saga_id}/cancel and assert the response status is in [403, 404].
- Around line 17-36: The test_get_saga_status currently uses conditional
if-guards that let the test silently pass when no saga is returned; change it to
assert the expected responses and fail loudly: after calling
test_user.get(f"/api/v1/sagas/execution/{created_execution.execution_id}")
assert sagas_response.status_code == 200, parse with
SagaListResponse.model_validate and assert sagas.sagas is not empty, then
extract saga_id and call test_user.get(f"/api/v1/sagas/{saga_id}"), assert the
second response.status_code == 200, validate with
SagaStatusResponse.model_validate and assert saga.saga_id == saga_id and
saga.state in list(SagaState); optionally add a short retry loop around the
first request if eventual consistency is expected.
In `@backend/tests/e2e/test_sse_routes.py`:
- Around line 99-126: The test test_execution_stream_other_users_execution
currently contradicts its docstring by allowing a 200 response and uses .get()
instead of streaming; update the test to require a 403 response when attempting
to access another user's execution events by replacing the loose assert with
assert response.status_code == 403, and switch the request to use the streaming
client method (matching other SSE tests) when calling
/api/v1/events/executions/{execution_id} so the test actually exercises the SSE
authorization path in the streaming code path.
🧹 Nitpick comments (13)
backend/tests/e2e/conftest.py (1)
92-113: Usepytest_asyncio.fixturefor async fixtures (or confirm auto mode).Async fixtures decorated with
@pytest.fixturecan be treated as plain coroutines in strict mode. Prefer@pytest_asyncio.fixtureunlessasyncio_mode=autois explicitly enabled.♻️ Proposed fix
-@pytest.fixture +@pytest_asyncio.fixture async def created_execution( test_user: AsyncClient, simple_execution_request: ExecutionRequest ) -> ExecutionResponse: """Execution created by test_user.""" resp = await test_user.post( "/api/v1/execute", json=simple_execution_request.model_dump() ) assert resp.status_code == 200 return ExecutionResponse.model_validate(resp.json()) -@pytest.fixture +@pytest_asyncio.fixture async def created_execution_admin( test_admin: AsyncClient, simple_execution_request: ExecutionRequest ) -> ExecutionResponse: """Execution created by test_admin.""" resp = await test_admin.post( "/api/v1/execute", json=simple_execution_request.model_dump() ) assert resp.status_code == 200 return ExecutionResponse.model_validate(resp.json())backend/tests/e2e/test_health_routes.py (1)
47-57: Assert status codes before parsing liveness responses.This makes failures clearer and avoids validating error payloads as successful responses.
♻️ Proposed fix
response1 = await client.get("/api/v1/health/live") +assert response1.status_code == 200 result1 = LivenessResponse.model_validate(response1.json()) response2 = await client.get("/api/v1/health/live") +assert response2.status_code == 200 result2 = LivenessResponse.model_validate(response2.json())backend/tests/e2e/test_saga_routes.py (1)
98-110: Pass enum.valuein query params to avoid stringification quirks.
httpxwill stringifyEnumvalues; using.valueavoids sendingSagaState.RUNNINGinstead of the expected raw value.♻️ Proposed fix
-params={"state": SagaState.RUNNING}, +params={"state": SagaState.RUNNING.value},-params={"state": SagaState.COMPLETED}, +params={"state": SagaState.COMPLETED.value},Also applies to: 128-141
backend/tests/e2e/test_saved_scripts_routes.py (1)
36-47:test_create_saved_script_minimalisn’t minimal.It posts the full request payload. If you want to validate defaults, send only required fields.
♻️ Proposed fix
response = await test_user.post( - "/api/v1/scripts", json=new_script_request.model_dump() + "/api/v1/scripts", + json={"name": new_script_request.name, "script": new_script_request.script}, )backend/tests/e2e/test_admin_users_routes.py (1)
70-83: UseUserRole.valuefor the role query param.Query params are stringified; passing
.valueavoids sendingUserRole.USERinstead of the raw role value.♻️ Proposed fix
-params={"role": UserRole.USER}, +params={"role": UserRole.USER.value},backend/tests/e2e/test_notifications_routes.py (1)
224-244: Make the delete-format test explicitly fail/skip when there’s no data.Right now a 500 (or any non‑200) from the list call would silently pass the test. Consider asserting the list call succeeds and skipping when no notifications exist.
✅ Suggested update
- list_response = await test_user.get( - "/api/v1/notifications", - params={"limit": 1}, - ) - - if list_response.status_code == 200: - result = NotificationListResponse.model_validate(list_response.json()) - if result.notifications: - notification_id = result.notifications[0].notification_id + list_response = await test_user.get( + "/api/v1/notifications", + params={"limit": 1}, + ) + assert list_response.status_code == 200 + result = NotificationListResponse.model_validate(list_response.json()) + if not result.notifications: + pytest.skip("No notifications to delete in this environment") + + notification_id = result.notifications[0].notification_idbackend/tests/e2e/test_execution_routes.py (1)
384-399: Avoid reusing a static Idempotency-Key across test runs.A constant key can collide across parallel runs or shared environments, causing false positives/negatives. Use a unique key per test.
♻️ Suggested update
-from uuid import UUID +from uuid import UUID, uuid4 @@ - headers = {"Idempotency-Key": "it-idem-key-123"} + headers = {"Idempotency-Key": f"it-idem-key-{uuid4()}"}backend/tests/e2e/test_events_routes.py (1)
222-238: Avoid silent pass when no events are present.If
/api/v1/events/userfails, this test currently does nothing. Consider asserting the list call succeeds and skipping when empty.✅ Suggested update
- if events_response.status_code == 200: - result = EventListResponse.model_validate(events_response.json()) - if result.events: - event_id = result.events[0].event_id + assert events_response.status_code == 200 + result = EventListResponse.model_validate(events_response.json()) + if not result.events: + pytest.skip("No events available for ID lookup") + event_id = result.events[0].event_idbackend/tests/e2e/test_dlq_routes.py (1)
138-162: Make DLQ detail tests explicit when no messages exist.The test silently no‑ops if listing fails or is empty, which can hide real failures. Consider asserting the list call succeeds and skipping if there are no messages.
✅ Suggested update
- if list_response.status_code == 200: - result = DLQMessagesResponse.model_validate(list_response.json()) - if result.messages: - event_id = result.messages[0].event.event_id + assert list_response.status_code == 200 + result = DLQMessagesResponse.model_validate(list_response.json()) + if not result.messages: + pytest.skip("No DLQ messages available for detail test") + event_id = result.messages[0].event.event_idbackend/tests/e2e/test_grafana_alerts_routes.py (1)
224-234: Asserting on specific message content may be brittle.Line 234 asserts an exact substring in the message. Consider whether this level of specificity is necessary, as message text changes could cause test failures unrelated to functionality.
Consider relaxing the assertion
async def test_grafana_test_endpoint_as_admin( self, test_admin: AsyncClient ) -> None: """Admin can access test endpoint.""" response = await test_admin.get("/api/v1/alerts/grafana/test") assert response.status_code == 200 result = response.json() assert result["status"] == "ok" - assert "Grafana webhook endpoint is ready" in result["message"] + assert "message" in result # Message content may varybackend/tests/e2e/test_replay_routes.py (1)
118-151: Conditional assertion pattern silently passes on precondition failure.If
create_response.status_codeis not 200, the test completes without asserting anything meaningful. This masks failures and gives a false sense of coverage. Consider usingpytest.skip()or asserting the precondition unconditionally.Use explicit precondition assertion or skip
async def test_start_replay_session( self, test_admin: AsyncClient ) -> None: """Start a created replay session.""" # Create session first create_response = await test_admin.post( "/api/v1/replay/sessions", json={ "replay_type": ReplayType.TIME_RANGE, "target": ReplayTarget.KAFKA, "filter": {}, "batch_size": 10, }, ) - if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) + assert create_response.status_code == 200, f"Precondition failed: {create_response.text}" + session = ReplayResponse.model_validate(create_response.json()) - # Start session - response = await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/start" - ) + # Start session + response = await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/start" + ) - # May be 200 or error depending on session state - if response.status_code == 200: - result = ReplayResponse.model_validate(response.json()) - assert result.session_id == session.session_id - assert result.status in [ - ReplayStatus.RUNNING, - ReplayStatus.COMPLETED, - ReplayStatus.FAILED, - ] + # May be 200 or error depending on session state + if response.status_code == 200: + result = ReplayResponse.model_validate(response.json()) + assert result.session_id == session.session_id + assert result.status in [ + ReplayStatus.RUNNING, + ReplayStatus.COMPLETED, + ReplayStatus.FAILED, + ]The same pattern applies to other tests:
test_pause_replay_session(line 182),test_resume_replay_session(line 229),test_cancel_replay_session(line 269), andtest_get_replay_session(line 379).backend/tests/e2e/test_user_settings_routes.py (2)
18-30: Inconsistent theme value checking.Line 26 uses hardcoded strings
["light", "dark", "system"]while other tests in this file useTheme.DARK,Theme.LIGHT,Theme.AUTOenums. This inconsistency could cause the assertion to fail if the enum values differ from these strings (e.g.,Theme.AUTOvs"system").Use Theme enum values for consistency
async def test_get_user_settings(self, test_user: AsyncClient) -> None: """Get current user settings.""" response = await test_user.get("/api/v1/user/settings/") assert response.status_code == 200 settings = UserSettings.model_validate(response.json()) - assert settings.theme in ["light", "dark", "system"] + assert settings.theme in [Theme.LIGHT, Theme.DARK, Theme.AUTO] assert settings.timezone is not None assert settings.notifications is not None assert settings.editor is not None
226-252: Conditional assertion pattern may silently pass without testing anything.If
history_response.status_codeis not 200 orhistory.historyis empty, the test completes without any meaningful assertions. Consider adding explicit handling or usingpytest.skip()when preconditions aren't met.Add explicit precondition handling
async def test_restore_settings(self, test_user: AsyncClient) -> None: """Restore settings to a previous point.""" # Get history first history_response = await test_user.get("/api/v1/user/settings/history") + assert history_response.status_code == 200 - if history_response.status_code == 200: - history = SettingsHistoryResponse.model_validate( - history_response.json() - ) + history = SettingsHistoryResponse.model_validate( + history_response.json() + ) - if history.history: - # Try to restore to first entry - restore_req = RestoreSettingsRequest( - timestamp=history.history[0].timestamp - ) - restore_response = await test_user.post( - "/api/v1/user/settings/restore", - json=restore_req.model_dump(mode="json"), - ) + if not history.history: + pytest.skip("No history entries available to test restore") - # May succeed or fail depending on implementation - assert restore_response.status_code in [200, 400, 404] + # Try to restore to first entry + restore_req = RestoreSettingsRequest( + timestamp=history.history[0].timestamp + ) + restore_response = await test_user.post( + "/api/v1/user/settings/restore", + json=restore_req.model_dump(mode="json"), + ) + + # May succeed or fail depending on implementation + assert restore_response.status_code in [200, 400, 404]
Comment on lines
75
to
117
| async def test_update_system_settings_full( | ||
| self, test_admin: AsyncClient | ||
| ) -> None: | ||
| """Admin can update all system settings.""" | ||
| response = await test_admin.put( | ||
| "/api/v1/admin/settings/", | ||
| json={ | ||
| "execution_limits": { | ||
| "max_timeout_seconds": 600, | ||
| "max_memory_mb": 1024, | ||
| "max_cpu_cores": 4, | ||
| "max_concurrent_executions": 20, | ||
| }, | ||
| "security_settings": { | ||
| "password_min_length": 10, | ||
| "session_timeout_minutes": 120, | ||
| "max_login_attempts": 5, | ||
| "lockout_duration_minutes": 30, | ||
| }, | ||
| "monitoring_settings": { | ||
| "metrics_retention_days": 60, | ||
| "log_level": "WARNING", | ||
| "enable_tracing": True, | ||
| "sampling_rate": 0.2, | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| settings = SystemSettings.model_validate(response.json()) | ||
|
|
||
| assert settings.execution_limits.max_timeout_seconds == 600 | ||
| assert settings.execution_limits.max_memory_mb == 1024 | ||
| assert settings.execution_limits.max_cpu_cores == 4 | ||
| assert settings.execution_limits.max_concurrent_executions == 20 | ||
|
|
||
| assert settings.security_settings.password_min_length == 10 | ||
| assert settings.security_settings.session_timeout_minutes == 120 | ||
|
|
||
| assert settings.monitoring_settings.metrics_retention_days == 60 | ||
| assert settings.monitoring_settings.log_level == "WARNING" | ||
| assert settings.monitoring_settings.sampling_rate == 0.2 | ||
|
|
There was a problem hiding this comment.
Restore settings after mutation to keep tests order‑independent.
These tests change global settings but don’t revert them, so subsequent tests that assume defaults may become flaky depending on order. Consider snapshotting and restoring (or calling reset) in a finally block.
🧹 Suggested pattern (apply to update tests)
- response = await test_admin.put(
+ before = await test_admin.get("/api/v1/admin/settings/")
+ assert before.status_code == 200
+ previous = SystemSettings.model_validate(before.json())
+ try:
+ response = await test_admin.put(
"/api/v1/admin/settings/",
json={
@@
- assert response.status_code == 200
- settings = SystemSettings.model_validate(response.json())
+ assert response.status_code == 200
+ settings = SystemSettings.model_validate(response.json())
@@
- assert settings.monitoring_settings.sampling_rate == 0.2
+ assert settings.monitoring_settings.sampling_rate == 0.2
+ finally:
+ await test_admin.put(
+ "/api/v1/admin/settings/",
+ json=previous.model_dump(),
+ )🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_admin_settings_routes.py` around lines 75 - 117, The
test test_update_system_settings_full mutates global SystemSettings and doesn't
restore them; to make tests order-independent, fetch and store the current
settings before the PUT (e.g., GET "/api/v1/admin/settings/" and parse via
SystemSettings.model_validate), perform the update, and in a finally block send
a request to restore the saved settings (using the same test_admin client and
the PUT to "/api/v1/admin/settings/" with the original settings payload or call
any existing reset endpoint); ensure you reference the saved variable when
restoring so the global settings are returned to their prior state regardless of
test outcome.
Comment on lines
89
to
98
| async def test_mark_nonexistent_notification_read( | ||
| self, test_user: AsyncClient | ||
| ) -> None: | ||
| """Marking nonexistent notification returns 404.""" | ||
| response = await test_user.put( | ||
| "/api/v1/notifications/nonexistent-id/read" | ||
| ) | ||
|
|
||
| # Should be 404 or similar error | ||
| assert response.status_code in [404, 400] |
There was a problem hiding this comment.
Use a valid UUID to exercise the 404 path.
"nonexistent-id" is likely failing validation (400/422) rather than truly testing the not‑found branch. Use a valid UUID and assert 404 for a clearer signal.
🐛 Suggested update
- response = await test_user.put(
- "/api/v1/notifications/nonexistent-id/read"
- )
-
- # Should be 404 or similar error
- assert response.status_code in [404, 400]
+ nonexistent_id = "00000000-0000-0000-0000-000000000000"
+ response = await test_user.put(
+ f"/api/v1/notifications/{nonexistent_id}/read"
+ )
+
+ assert response.status_code == 404🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_notifications_routes.py` around lines 89 - 98, In
test_mark_nonexistent_notification_read, replace the invalid id string
"nonexistent-id" with a valid UUID (e.g., generate one with uuid.uuid4() or use
a hard-coded valid UUID string) when calling test_user.put so the request passes
route validation and exercises the 404 branch, and change the assertion to
expect response.status_code == 404 (remove the [404, 400] alternative); the test
function name to update is test_mark_nonexistent_notification_read and the HTTP
call to change is the PUT to "/api/v1/notifications/{id}/read" invoked via
test_user.put.
Comment on lines
+17
to
+36
| @pytest.mark.asyncio | ||
| async def test_get_saga_status( | ||
| self, test_user: AsyncClient, created_execution: ExecutionResponse | ||
| ) -> None: | ||
| """Get saga status by ID after creating an execution.""" | ||
| sagas_response = await test_user.get( | ||
| f"/api/v1/sagas/execution/{created_execution.execution_id}" | ||
| ) | ||
|
|
||
| if sagas_response.status_code == 200: | ||
| sagas = SagaListResponse.model_validate(sagas_response.json()) | ||
| if sagas.sagas: | ||
| saga_id = sagas.sagas[0].saga_id | ||
| response = await test_user.get(f"/api/v1/sagas/{saga_id}") | ||
|
|
||
| assert response.status_code == 200 | ||
| saga = SagaStatusResponse.model_validate(response.json()) | ||
| assert saga.saga_id == saga_id | ||
| assert saga.state in list(SagaState) | ||
|
|
There was a problem hiding this comment.
Avoid silent passes when no saga is returned.
The if guards can make the test pass even if /sagas/execution regresses. Prefer explicit assertions (or a retry) so failures surface.
🐛 Proposed fix
sagas_response = await test_user.get(
f"/api/v1/sagas/execution/{created_execution.execution_id}"
)
-if sagas_response.status_code == 200:
- sagas = SagaListResponse.model_validate(sagas_response.json())
- if sagas.sagas:
- saga_id = sagas.sagas[0].saga_id
- response = await test_user.get(f"/api/v1/sagas/{saga_id}")
-
- assert response.status_code == 200
- saga = SagaStatusResponse.model_validate(response.json())
- assert saga.saga_id == saga_id
- assert saga.state in list(SagaState)
+assert sagas_response.status_code == 200
+sagas = SagaListResponse.model_validate(sagas_response.json())
+assert sagas.sagas, "Expected at least one saga for execution"
+saga_id = sagas.sagas[0].saga_id
+response = await test_user.get(f"/api/v1/sagas/{saga_id}")
+
+assert response.status_code == 200
+saga = SagaStatusResponse.model_validate(response.json())
+assert saga.saga_id == saga_id
+assert saga.state in list(SagaState)📝 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.
Suggested change
| @pytest.mark.asyncio | |
| async def test_get_saga_status( | |
| self, test_user: AsyncClient, created_execution: ExecutionResponse | |
| ) -> None: | |
| """Get saga status by ID after creating an execution.""" | |
| sagas_response = await test_user.get( | |
| f"/api/v1/sagas/execution/{created_execution.execution_id}" | |
| ) | |
| if sagas_response.status_code == 200: | |
| sagas = SagaListResponse.model_validate(sagas_response.json()) | |
| if sagas.sagas: | |
| saga_id = sagas.sagas[0].saga_id | |
| response = await test_user.get(f"/api/v1/sagas/{saga_id}") | |
| assert response.status_code == 200 | |
| saga = SagaStatusResponse.model_validate(response.json()) | |
| assert saga.saga_id == saga_id | |
| assert saga.state in list(SagaState) | |
| `@pytest.mark.asyncio` | |
| async def test_get_saga_status( | |
| self, test_user: AsyncClient, created_execution: ExecutionResponse | |
| ) -> None: | |
| """Get saga status by ID after creating an execution.""" | |
| sagas_response = await test_user.get( | |
| f"/api/v1/sagas/execution/{created_execution.execution_id}" | |
| ) | |
| assert sagas_response.status_code == 200 | |
| sagas = SagaListResponse.model_validate(sagas_response.json()) | |
| assert sagas.sagas, "Expected at least one saga for execution" | |
| saga_id = sagas.sagas[0].saga_id | |
| response = await test_user.get(f"/api/v1/sagas/{saga_id}") | |
| assert response.status_code == 200 | |
| saga = SagaStatusResponse.model_validate(response.json()) | |
| assert saga.saga_id == saga_id | |
| assert saga.state in list(SagaState) |
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_saga_routes.py` around lines 17 - 36, The
test_get_saga_status currently uses conditional if-guards that let the test
silently pass when no saga is returned; change it to assert the expected
responses and fail loudly: after calling
test_user.get(f"/api/v1/sagas/execution/{created_execution.execution_id}")
assert sagas_response.status_code == 200, parse with
SagaListResponse.model_validate and assert sagas.sagas is not empty, then
extract saga_id and call test_user.get(f"/api/v1/sagas/{saga_id}"), assert the
second response.status_code == 200, validate with
SagaStatusResponse.model_validate and assert saga.saga_id == saga_id and
saga.state in list(SagaState); optionally add a short retry loop around the
first request if eventual consistency is expected.
Comment on lines
+44
to
+61
| async def test_get_saga_access_denied( | ||
| self, | ||
| test_user: AsyncClient, | ||
| another_user: AsyncClient, | ||
| created_execution: ExecutionResponse, | ||
| ) -> None: | ||
| """Cannot access another user's saga.""" | ||
| sagas_response = await test_user.get( | ||
| f"/api/v1/sagas/execution/{created_execution.execution_id}" | ||
| ) | ||
|
|
||
| if sagas_response.status_code == 200: | ||
| sagas = SagaListResponse.model_validate(sagas_response.json()) | ||
| if sagas.sagas: | ||
| saga_id = sagas.sagas[0].saga_id | ||
| response = await another_user.get(f"/api/v1/sagas/{saga_id}") | ||
| assert response.status_code == 403 | ||
|
|
There was a problem hiding this comment.
Avoid silent passes when no saga is returned.
Same pattern here: if the list is empty or status isn’t 200, the test does nothing. Please assert explicitly (or add a retry) to avoid false positives.
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_saga_routes.py` around lines 44 - 61, The
test_get_saga_access_denied currently silently passes when the initial GET
doesn't return 200 or returns an empty saga list; update it to explicitly assert
the initial response is successful and that at least one saga exists before
proceeding: assert sagas_response.status_code == 200, parse into
SagaListResponse.model_validate(...), then assert sagas.sagas is not empty (or
add a retry loop that fails after N attempts), and only then extract saga_id and
call another_user.get(...) expecting 403; refer to test_get_saga_access_denied,
SagaListResponse, created_execution, and saga_id to locate where to add these
assertions/retry.
Comment on lines
+168
to
+196
| @pytest.mark.asyncio | ||
| async def test_cancel_saga( | ||
| self, | ||
| test_user: AsyncClient, | ||
| long_running_execution_request: ExecutionRequest, | ||
| ) -> None: | ||
| """Cancel a running saga.""" | ||
| exec_response = await test_user.post( | ||
| "/api/v1/execute", json=long_running_execution_request.model_dump() | ||
| ) | ||
| execution_id = exec_response.json()["execution_id"] | ||
|
|
||
| sagas_response = await test_user.get( | ||
| f"/api/v1/sagas/execution/{execution_id}" | ||
| ) | ||
|
|
||
| if sagas_response.status_code == 200: | ||
| sagas = SagaListResponse.model_validate(sagas_response.json()) | ||
| if sagas.sagas: | ||
| saga_id = sagas.sagas[0].saga_id | ||
| response = await test_user.post(f"/api/v1/sagas/{saga_id}/cancel") | ||
|
|
||
| if response.status_code == 200: | ||
| result = SagaCancellationResponse.model_validate(response.json()) | ||
| assert result.saga_id == saga_id | ||
| assert isinstance(result.success, bool) | ||
| else: | ||
| assert response.status_code in [400, 404] | ||
|
|
There was a problem hiding this comment.
Avoid silent passes when no saga is returned.
If /sagas/execution/{execution_id} doesn’t return sagas, the test exits without asserting anything. Please assert the status and presence of sagas (or retry) so failures surface.
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_saga_routes.py` around lines 168 - 196, The
test_cancel_saga currently silently returns if no saga is found; update
test_cancel_saga to fail loudly by asserting the GET
/api/v1/sagas/execution/{execution_id} response is 200 and that
SagaListResponse.model_validate(...).sagas is non-empty (or implement a short
retry/poll loop with a timeout that re-GETs the same endpoint until sagas
appear), then proceed to pick saga_id and call POST
/api/v1/sagas/{saga_id}/cancel and validate using SagaCancellationResponse;
ensure assertions cover both the status_code and presence of sagas so the test
fails when none are returned.
Comment on lines
+208
to
+229
| async def test_cancel_other_users_saga_forbidden( | ||
| self, | ||
| test_user: AsyncClient, | ||
| another_user: AsyncClient, | ||
| long_running_execution_request: ExecutionRequest, | ||
| ) -> None: | ||
| """Cannot cancel another user's saga.""" | ||
| exec_response = await test_user.post( | ||
| "/api/v1/execute", json=long_running_execution_request.model_dump() | ||
| ) | ||
| execution_id = exec_response.json()["execution_id"] | ||
|
|
||
| sagas_response = await test_user.get( | ||
| f"/api/v1/sagas/execution/{execution_id}" | ||
| ) | ||
|
|
||
| if sagas_response.status_code == 200: | ||
| sagas = SagaListResponse.model_validate(sagas_response.json()) | ||
| if sagas.sagas: | ||
| saga_id = sagas.sagas[0].saga_id | ||
| response = await another_user.post(f"/api/v1/sagas/{saga_id}/cancel") | ||
| assert response.status_code in [403, 404] |
There was a problem hiding this comment.
Avoid silent passes when no saga is returned.
Same guard pattern: if the saga list is empty, the test does nothing. Consider asserting presence (or retrying) so access-control regressions are caught.
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_saga_routes.py` around lines 208 - 229, The test
test_cancel_other_users_saga_forbidden silently passes when no saga is returned;
change it to assert that a saga exists (i.e., validate SagaListResponse and
assert sagas.sagas is not empty) before extracting saga_id, or implement a short
retry loop querying /api/v1/sagas/execution/{execution_id} until a saga appears
(with a sensible timeout) to avoid intermittent false negatives; then continue
to call /api/v1/sagas/{saga_id}/cancel and assert the response status is in
[403, 404].
backend/tests/e2e/test_sse_routes.py
Outdated
Comment on lines
99
to
126
| @pytest.mark.asyncio | ||
| async def test_execution_stream_other_users_execution( | ||
| self, test_user: AsyncClient, another_user: AsyncClient | ||
| ) -> None: | ||
| """Cannot stream another user's execution events.""" | ||
| # Create execution as test_user | ||
| exec_response = await test_user.post( | ||
| "/api/v1/execute", | ||
| json={ | ||
| "script": "print('private')", | ||
| "lang": "python", | ||
| "lang_version": "3.11", | ||
| }, | ||
| ) | ||
| execution_id = exec_response.json()["execution_id"] | ||
|
|
||
| # Try to stream as another_user | ||
| response = await another_user.get( | ||
| f"/api/v1/events/executions/{execution_id}" | ||
| ) | ||
|
|
||
| # Should be forbidden or return empty stream | ||
| assert response.status_code in [200, 403] | ||
| if response.status_code == 200: | ||
| # If 200, content type should still be event-stream | ||
| # but stream should be empty or close quickly | ||
| content_type = response.headers.get("content-type", "") | ||
| assert "text/event-stream" in content_type |
There was a problem hiding this comment.
Test assertion contradicts the docstring security claim.
The docstring states "Cannot stream another user's execution events" but the test accepts 200 as a valid response. If cross-user access should be forbidden, the test should assert 403 exclusively. Accepting 200 means this test will pass even if the authorization check is broken.
Additionally, this test uses get() instead of stream() unlike the other SSE tests, which may behave differently for streaming endpoints.
Suggested fix to enforce 403 or clarify the test intent
async def test_execution_stream_other_users_execution(
self, test_user: AsyncClient, another_user: AsyncClient
) -> None:
- """Cannot stream another user's execution events."""
+ """Access to another user's execution stream should be forbidden or return empty."""
# Create execution as test_user
exec_response = await test_user.post(
"/api/v1/execute",
json={
"script": "print('private')",
"lang": "python",
"lang_version": "3.11",
},
)
execution_id = exec_response.json()["execution_id"]
# Try to stream as another_user
- response = await another_user.get(
- f"/api/v1/events/executions/{execution_id}"
- )
-
- # Should be forbidden or return empty stream
- assert response.status_code in [200, 403]
- if response.status_code == 200:
- # If 200, content type should still be event-stream
- # but stream should be empty or close quickly
- content_type = response.headers.get("content-type", "")
- assert "text/event-stream" in content_type
+ async with another_user.stream(
+ "GET", f"/api/v1/events/executions/{execution_id}"
+ ) as response:
+ # Should be forbidden or return empty stream
+ assert response.status_code in [200, 403]
+ if response.status_code == 200:
+ content_type = response.headers.get("content-type", "")
+ assert "text/event-stream" in content_type🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_sse_routes.py` around lines 99 - 126, The test
test_execution_stream_other_users_execution currently contradicts its docstring
by allowing a 200 response and uses .get() instead of streaming; update the test
to require a 403 response when attempting to access another user's execution
events by replacing the loose assert with assert response.status_code == 403,
and switch the request to use the streaming client method (matching other SSE
tests) when calling /api/v1/events/executions/{execution_id} so the test
actually exercises the SSE authorization path in the streaming code path.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/events.py`:
- Around line 47-51: When calling
execution_service.get_execution_result(execution_id) handle the
ExecutionNotFoundError so it returns a 404 HTTPException instead of bubbling up
as a 500; wrap the call in a try/except that catches ExecutionNotFoundError and
raises HTTPException(status_code=404, detail="Execution not found") before
performing the existing ownership check that uses execution.user_id,
current_user and UserRole.ADMIN.
In `@backend/tests/e2e/test_admin_events_routes.py`:
- Around line 420-449: The test currently can silently pass when replay_response
or status_response are non-200; add explicit checks that assert
replay_response.status_code == 200 and assert status_response.status_code == 200
(or call pytest.skip with a clear message when upstream service/fixture is
unavailable) immediately after the test_admin.post to
"/api/v1/admin/events/replay" and immediately after the test_admin.get to
"/api/v1/admin/events/replay/{session_id}/status"; keep the existing parsing
with EventReplayResponse.model_validate and
EventReplayStatusResponse.model_validate only after those assertions/skips so
failures don’t produce false green runs.
- Around line 283-309: Add explicit assertions and a clear skip when no events
are found: assert that browse_response.status_code == 200 immediately after
calling test_admin.post, then validate the body with
EventBrowseResponse.model_validate and assert that browse_result.events is
non-empty; if it is empty, call pytest.skip("no events found for aggregate_id
...") (or similar) before proceeding to extract event_id and call
test_admin.get; update references to browse_response,
EventBrowseResponse.model_validate, browse_result.events, and the subsequent
get/validation block (detail, EventDetailResponse.model_validate) so the test
fails loudly on bad status or skips explicitly when there are no events.
In `@backend/tests/e2e/test_events_routes.py`:
- Around line 369-373: The test in test_events_routes.py currently allows both
200 and 404 which makes the dry-run assertion non-deterministic; either ensure
deterministic data setup or split into two tests. Fix by (a) creating the
required event/execution fixture or calling the setup helper so the endpoint
returns 200 and then assert
ReplayAggregateResponse.model_validate(response.json()) has dry_run True and
aggregate_id == created_execution_admin.execution_id, or (b) split into two
tests: one that prepares events and asserts the 200 path and dry_run behavior,
and one that ensures the 404 path when no events exist; adjust test names and
fixtures accordingly to guarantee deterministic outcomes for functions using
ReplayAggregateResponse, result.dry_run, and
created_execution_admin.execution_id.
♻️ Duplicate comments (10)
backend/tests/e2e/test_user_settings_routes.py (4)
53-56: Notification settings fields may not match the schema.The fields
email_enabledandpush_enabledwere flagged in a previous review as potentially unsupported by theNotificationSettingsschema. This test may fail validation or not exercise the intended API behavior.
57-62: Editor settings field name mismatch.The field
line_numberswas flagged in a previous review — the API likely expectsshow_line_numbers. This will cause the request to fail validation or silently ignore the field.
141-144: Same field name issue as noted above.Uses
email_enabledandpush_enabledwhich may not match the schema.
162-167: Sameline_numbersfield name issue.The field should likely be
show_line_numbersper the previous review comment.backend/tests/e2e/test_replay_routes.py (2)
125-136: Fail fast if session creation fails instead of skipping assertions.These tests currently bypass all assertions when the create call fails, which allows a broken create endpoint to pass. Assert the 200 response and proceed unconditionally.
🔧 Proposed fix (apply to each create flow)
- if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) + assert create_response.status_code == 200 + session = ReplayResponse.model_validate(create_response.json())Also applies to: 183-185, 230-232, 270-272, 380-382
161-162: Do not treat HTTP 500 as an acceptable outcome in tests.Allowing 500 masks server-side regressions. Assert expected 4xx statuses per the API contract and remove 500 from the accepted list.
🔧 Proposed fix (use contract-specific 4xx)
- assert response.status_code in [404, 500] + assert response.status_code == 404- assert response.status_code in [200, 400, 500] + assert response.status_code in [200, 400]Also applies to: 209-209, 239-239, 249-249, 294-294, 407-408
backend/tests/e2e/test_admin_events_routes.py (2)
346-352: Assert allowed replay statuses before branching.
Line 346: the test currently skips all assertions when status ≠ 200, which can mask failures. Add a status assertion and explicit skip.✅ Suggested fix
- # May be 200, 400, or 404 depending on events availability - if response.status_code == 200: + # May be 200, 400, or 404 depending on events availability + assert response.status_code in (200, 400, 404) + if response.status_code != 200: + pytest.skip("No events available to replay in dry-run.") + else: result = EventReplayResponse.model_validate(response.json()) assert result.dry_run is True
470-497: Delete test can pass without actually testing deletion.
Line 479: if browse returns empty events, the test completes with no assertions. Add a status assertion and explicit skip when no events exist.✅ Suggested fix
- if browse_response.status_code == 200: - browse_result = EventBrowseResponse.model_validate( - browse_response.json() - ) - if browse_result.events: - event_id = browse_result.events[0].get("event_id") + assert browse_response.status_code == 200 + browse_result = EventBrowseResponse.model_validate( + browse_response.json() + ) + if not browse_result.events: + pytest.skip("No events found for deletion.") + event_id = browse_result.events[0].get("event_id")backend/tests/e2e/test_events_routes.py (2)
229-239: Test can pass vacuously when no events exist.This test passes without asserting anything if
events_response.status_code != 200or ifresult.eventsis empty. Consider asserting the precondition (at least one event exists) or usingpytest.skip()to make it explicit when the test cannot run meaningfully.
333-344: Test silently passes if publish fails.The conditional
if publish_response.status_code == 200means this test passes without verifying the delete flow if the publish step fails. Assert that publish succeeds before proceeding.
🧹 Nitpick comments (9)
backend/tests/e2e/test_user_settings_routes.py (3)
147-149: Assertion doesn't verify the update took effect.The test only checks that
settings.notifications is not None, but doesn't assert that the specific field values were actually updated. Consider adding assertions that verify the notification settings reflect the intended changes.
170-174: Missing assertion for line numbers setting.The test verifies
tab_size,font_size, andword_wrapbut doesn't assert on the line numbers field (once the field name is corrected).
235-251: Test may silently pass without exercising restore functionality.The nested conditionals cause the test to pass without any assertions if the history fetch fails or returns an empty list. Consider restructuring to ensure the restore path is always exercised:
- Explicitly create settings changes before the test to guarantee history exists
- Use
pytest.skip()or fail with a clear message if preconditions aren't met♻️ Suggested approach
`@pytest.mark.asyncio` async def test_restore_settings(self, test_user: AsyncClient) -> None: """Restore settings to a previous point.""" + # Create history by making a change + await test_user.put( + "/api/v1/user/settings/theme", + json={"theme": "dark"}, + ) + # Get history first history_response = await test_user.get("/api/v1/user/settings/history") - - if history_response.status_code == 200: - history = SettingsHistoryResponse.model_validate( - history_response.json() - ) - - if history.history: - # Try to restore to first entry - restore_req = RestoreSettingsRequest( - timestamp=history.history[0].timestamp - ) - restore_response = await test_user.post( - "/api/v1/user/settings/restore", - json=restore_req.model_dump(mode="json"), - ) - - # May succeed or fail depending on implementation - assert restore_response.status_code in [200, 400, 404] + assert history_response.status_code == 200 + + history = SettingsHistoryResponse.model_validate(history_response.json()) + assert history.history, "Expected settings history to exist" + + restore_req = RestoreSettingsRequest(timestamp=history.history[0].timestamp) + restore_response = await test_user.post( + "/api/v1/user/settings/restore", + json=restore_req.model_dump(mode="json"), + ) + + # May succeed or fail depending on implementation + assert restore_response.status_code in [200, 400, 404]backend/tests/e2e/test_auth_routes.py (4)
35-38: Minor redundancy in CSRF token assertion.The check
result.csrf_token and len(result.csrf_token) > 0is redundant—a truthy non-empty string already implieslen > 0. Consider simplifying toassert result.csrf_token.Additionally, since the user was just registered (not promoted), the role assertion could be stricter:
assert result.role == UserRole.USERrather than allowingADMIN.♻️ Suggested simplification
assert result.username == new_user_request.username - assert result.role in [UserRole.USER, UserRole.ADMIN] - assert result.csrf_token and len(result.csrf_token) > 0 + assert result.role == UserRole.USER + assert result.csrf_token assert result.message == "Login successful"
92-93: Consider explicit assertion for UUID validation.The bare
uuid.UUID(result.user_id)call validates the format by raisingValueErroron failure, but the intent isn't immediately clear. A brief comment or wrapping it in an explicit check would improve readability.♻️ Optional: make UUID validation intent explicit
- # Validate UUID format - uuid.UUID(result.user_id) + # Validate UUID format (raises ValueError if invalid) + assert uuid.UUID(result.user_id)
148-156: Inconsistent role type in test payload.Line 154 uses the string
"user"while other tests useUserRole.USERenum. For consistency, consider usingUserRole.USER.valueor the enum directly (if serialization handles it).♻️ Suggested consistency fix
json={ "username": f"user_{uid}", "email": "not-an-email", "password": "Pass123!", - "role": "user", + "role": UserRole.USER.value, },
213-220: Docstring claims cookie clearing but no assertion verifies it.The test docstring says "clears cookies" but there's no assertion checking that
access_tokenandcsrf_tokencookies are actually cleared (e.g., checking they're set to empty or havemax-age=0). Consider adding verification if this behavior is expected.♻️ Suggested cookie clearing verification
async def test_logout_success(self, test_user: AsyncClient) -> None: """Logout returns success message and clears cookies.""" response = await test_user.post("/api/v1/auth/logout") assert response.status_code == 200 result = MessageResponse.model_validate(response.json()) assert result.message == "Logout successful" + # Verify cookies are cleared (typically set with max-age=0 or empty value) + assert "access_token" in response.cookies or response.headers.get("set-cookie")Note: The exact assertion depends on how your API clears cookies. You may need to inspect
response.cookiesfor empty values or checkSet-Cookieheaders formax-age=0.backend/app/api/routes/events.py (1)
61-62: Consider removing redundant access check.With the pre-authorization check at lines 47-50 now validating ownership, the
if result is Nonecheck at line 61 may be redundant for the 403 case. Ifevent_service.get_execution_eventscan returnNonefor reasons other than access denial, consider distinguishing between "no access" and "no events found."backend/app/schemas_pydantic/user.py (1)
162-172: Consider adding validation constraints forrequestsandwindow_seconds.The
requestsandwindow_secondsfields should likely be positive integers. Consider addingField(gt=0)constraints to prevent invalid rate limit configurations.♻️ Proposed improvement
+from pydantic import Field + class RateLimitRuleRequest(BaseModel): """Request model for rate limit rule.""" endpoint_pattern: str group: EndpointGroup - requests: int - window_seconds: int + requests: int = Field(..., gt=0, description="Number of requests allowed") + window_seconds: int = Field(..., gt=0, description="Time window in seconds") algorithm: RateLimitAlgorithm = RateLimitAlgorithm.SLIDING_WINDOW - burst_multiplier: float = 1.5 + burst_multiplier: float = Field(default=1.5, gt=0) priority: int = 0 enabled: bool = True
Comment on lines
+47
to
+51
| # Check execution ownership first (before checking events) | ||
| execution = await execution_service.get_execution_result(execution_id) | ||
| if execution.user_id and execution.user_id != current_user.user_id and current_user.role != UserRole.ADMIN: | ||
| raise HTTPException(status_code=403, detail="Access denied") | ||
|
|
There was a problem hiding this comment.
Handle ExecutionNotFoundError to return 404 instead of 500.
The get_execution_result method raises ExecutionNotFoundError when the execution doesn't exist (per the relevant code snippet). This exception will propagate as an unhandled 500 error. Consider catching it and returning a proper 404 response.
🐛 Proposed fix
+from app.domain.exceptions import ExecutionNotFoundError
+
`@router.get`("/executions/{execution_id}/events", response_model=EventListResponse)
async def get_execution_events(
execution_id: str,
current_user: Annotated[UserResponse, Depends(current_user)],
event_service: FromDishka[EventService],
execution_service: FromDishka[ExecutionService],
include_system_events: bool = Query(False, description="Include system-generated events"),
limit: int = Query(100, ge=1, le=1000),
skip: int = Query(0, ge=0),
) -> EventListResponse:
# Check execution ownership first (before checking events)
+ try:
+ execution = await execution_service.get_execution_result(execution_id)
+ except ExecutionNotFoundError:
+ raise HTTPException(status_code=404, detail="Execution not found")
- execution = await execution_service.get_execution_result(execution_id)
if execution.user_id and execution.user_id != current_user.user_id and current_user.role != UserRole.ADMIN:
raise HTTPException(status_code=403, detail="Access denied")📝 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.
Suggested change
| # Check execution ownership first (before checking events) | |
| execution = await execution_service.get_execution_result(execution_id) | |
| if execution.user_id and execution.user_id != current_user.user_id and current_user.role != UserRole.ADMIN: | |
| raise HTTPException(status_code=403, detail="Access denied") | |
| from app.domain.exceptions import ExecutionNotFoundError | |
| # Check execution ownership first (before checking events) | |
| try: | |
| execution = await execution_service.get_execution_result(execution_id) | |
| except ExecutionNotFoundError: | |
| raise HTTPException(status_code=404, detail="Execution not found") | |
| if execution.user_id and execution.user_id != current_user.user_id and current_user.role != UserRole.ADMIN: | |
| raise HTTPException(status_code=403, detail="Access denied") | |
🤖 Prompt for AI Agents
In `@backend/app/api/routes/events.py` around lines 47 - 51, When calling
execution_service.get_execution_result(execution_id) handle the
ExecutionNotFoundError so it returns a 404 HTTPException instead of bubbling up
as a 500; wrap the call in a try/except that catches ExecutionNotFoundError and
raises HTTPException(status_code=404, detail="Execution not found") before
performing the existing ownership check that uses execution.user_id,
current_user and UserRole.ADMIN.
Comment on lines
283
to
309
| # Browse to find an event | ||
| browse_response = await test_admin.post( | ||
| "/api/v1/admin/events/browse", | ||
| json={ | ||
| "filters": {"aggregate_id": created_execution_admin.execution_id}, | ||
| "limit": 10, | ||
| }, | ||
| ) | ||
|
|
||
| if browse_response.status_code == 200: | ||
| browse_result = EventBrowseResponse.model_validate( | ||
| browse_response.json() | ||
| ) | ||
| if browse_result.events: | ||
| event_id = browse_result.events[0].get("event_id") | ||
|
|
||
| # Get event detail | ||
| response = await test_admin.get( | ||
| f"/api/v1/admin/events/{event_id}" | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| detail = EventDetailResponse.model_validate(response.json()) | ||
|
|
||
| assert detail.event is not None | ||
| assert isinstance(detail.related_events, list) | ||
| assert isinstance(detail.timeline, list) |
There was a problem hiding this comment.
Don’t silently pass when no events are found.
Line 292: if browse returns non‑200 or empty results, the test exits without any assertion, so regressions can slip. Add a status assertion and explicit skip.
✅ Suggested fix
- if browse_response.status_code == 200:
- browse_result = EventBrowseResponse.model_validate(
- browse_response.json()
- )
- if browse_result.events:
- event_id = browse_result.events[0].get("event_id")
+ assert browse_response.status_code == 200
+ browse_result = EventBrowseResponse.model_validate(
+ browse_response.json()
+ )
+ if not browse_result.events:
+ pytest.skip("No events found for execution; skipping detail test.")
+ event_id = browse_result.events[0].get("event_id")🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_admin_events_routes.py` around lines 283 - 309, Add
explicit assertions and a clear skip when no events are found: assert that
browse_response.status_code == 200 immediately after calling test_admin.post,
then validate the body with EventBrowseResponse.model_validate and assert that
browse_result.events is non-empty; if it is empty, call pytest.skip("no events
found for aggregate_id ...") (or similar) before proceeding to extract event_id
and call test_admin.get; update references to browse_response,
EventBrowseResponse.model_validate, browse_result.events, and the subsequent
get/validation block (detail, EventDetailResponse.model_validate) so the test
fails loudly on bad status or skips explicitly when there are no events.
Comment on lines
420
to
449
| # Start replay (not dry run) | ||
| replay_response = await test_admin.post( | ||
| "/api/v1/admin/events/replay", | ||
| json={ | ||
| "aggregate_id": execution_id, | ||
| "dry_run": False, | ||
| }, | ||
| ) | ||
|
|
||
| if replay_response.status_code == 200: | ||
| replay_result = EventReplayResponse.model_validate( | ||
| replay_response.json() | ||
| ) | ||
| if replay_result.session_id: | ||
| # Get status | ||
| status_response = await test_admin.get( | ||
| f"/api/v1/admin/events/replay/{replay_result.session_id}/status" | ||
| ) | ||
|
|
||
| if status_response.status_code == 200: | ||
| status = EventReplayStatusResponse.model_validate( | ||
| status_response.json() | ||
| ) | ||
| assert status.session_id == replay_result.session_id | ||
| assert status.status in [ | ||
| "pending", "in_progress", "completed", "failed" | ||
| ] | ||
| assert status.total_events >= 0 | ||
| assert status.replayed_events >= 0 | ||
| assert status.progress_percentage >= 0.0 |
There was a problem hiding this comment.
Replay status test can silently pass.
Line 429 onward: if replay or status calls return non‑200, the test exits without assertions. Add explicit status assertions + skips to avoid false green runs.
✅ Suggested fix
- if replay_response.status_code == 200:
+ assert replay_response.status_code in (200, 400, 404)
+ if replay_response.status_code != 200:
+ pytest.skip("Replay not started; skipping status check.")
+ else:
replay_result = EventReplayResponse.model_validate(
replay_response.json()
)
if replay_result.session_id:
# Get status
status_response = await test_admin.get(
f"/api/v1/admin/events/replay/{replay_result.session_id}/status"
)
- if status_response.status_code == 200:
+ assert status_response.status_code in (200, 404)
+ if status_response.status_code != 200:
+ pytest.skip("Replay status not available yet.")
+ else:
status = EventReplayStatusResponse.model_validate(
status_response.json()
)🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_admin_events_routes.py` around lines 420 - 449, The
test currently can silently pass when replay_response or status_response are
non-200; add explicit checks that assert replay_response.status_code == 200 and
assert status_response.status_code == 200 (or call pytest.skip with a clear
message when upstream service/fixture is unavailable) immediately after the
test_admin.post to "/api/v1/admin/events/replay" and immediately after the
test_admin.get to "/api/v1/admin/events/replay/{session_id}/status"; keep the
existing parsing with EventReplayResponse.model_validate and
EventReplayStatusResponse.model_validate only after those assertions/skips so
failures don’t produce false green runs.
Comment on lines
+369
to
+373
| # May be 200 or 404 depending on event availability | ||
| if response.status_code == 200: | ||
| result = ReplayAggregateResponse.model_validate(response.json()) | ||
| assert result.dry_run is True | ||
| assert result.aggregate_id == created_execution_admin.execution_id |
There was a problem hiding this comment.
Test has ambiguous success criteria.
The comment "May be 200 or 404 depending on event availability" and the conditional assertion means this test can pass in multiple scenarios without clearly testing the intended dry-run behavior. Consider either ensuring test data exists (via fixtures) so the response is deterministic, or splitting into separate tests for the 200 and 404 cases.
💡 Suggested approach
`@pytest.mark.asyncio`
async def test_replay_events_dry_run(
self, test_admin: AsyncClient, created_execution_admin: ExecutionResponse
) -> None:
"""Replay events in dry run mode."""
response = await test_admin.post(
f"/api/v1/events/replay/{created_execution_admin.execution_id}",
params={"dry_run": True},
)
- # May be 200 or 404 depending on event availability
- if response.status_code == 200:
- result = ReplayAggregateResponse.model_validate(response.json())
- assert result.dry_run is True
- assert result.aggregate_id == created_execution_admin.execution_id
+ # With created_execution_admin fixture, events should exist
+ assert response.status_code in (200, 404), f"Unexpected status: {response.status_code}"
+ if response.status_code == 404:
+ pytest.skip("No events available for replay test")
+
+ result = ReplayAggregateResponse.model_validate(response.json())
+ assert result.dry_run is True
+ assert result.aggregate_id == created_execution_admin.execution_id🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_events_routes.py` around lines 369 - 373, The test in
test_events_routes.py currently allows both 200 and 404 which makes the dry-run
assertion non-deterministic; either ensure deterministic data setup or split
into two tests. Fix by (a) creating the required event/execution fixture or
calling the setup helper so the endpoint returns 200 and then assert
ReplayAggregateResponse.model_validate(response.json()) has dry_run True and
aggregate_id == created_execution_admin.execution_id, or (b) split into two
tests: one that prepares events and asserts the 200 path and dry_run behavior,
and one that ensures the 404 path when no events exist; adjust test names and
fixtures accordingly to guarantee deterministic outcomes for functions using
ReplayAggregateResponse, result.dry_run, and
created_execution_admin.execution_id.
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/e2e/test_sse_routes.py">
<violation number="1" location="backend/tests/e2e/test_sse_routes.py:50">
P2: Using `move_on_after` can silently skip the assertions on timeout, letting a hanging SSE stream pass the test. Use `fail_after` (or check the cancel scope) so timeouts fail the test.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """Notification stream returns SSE content type.""" | ||
| # Note: httpx doesn't fully support SSE streaming in tests, | ||
| # but we can verify the content type and initial response | ||
| with anyio.move_on_after(SSE_TIMEOUT_SECONDS): |
There was a problem hiding this comment.
P2: Using move_on_after can silently skip the assertions on timeout, letting a hanging SSE stream pass the test. Use fail_after (or check the cancel scope) so timeouts fail the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_sse_routes.py, line 50:
<comment>Using `move_on_after` can silently skip the assertions on timeout, letting a hanging SSE stream pass the test. Use `fail_after` (or check the cancel scope) so timeouts fail the test.</comment>
<file context>
@@ -39,21 +42,22 @@ class TestNotificationStream:
- assert response.status_code == 200
- content_type = response.headers.get("content-type", "")
- assert "text/event-stream" in content_type
+ with anyio.move_on_after(SSE_TIMEOUT_SECONDS):
+ async with test_user.stream(
+ "GET", "/api/v1/events/notifications/stream"
</file context>
Suggested change
| with anyio.move_on_after(SSE_TIMEOUT_SECONDS): | |
| with anyio.fail_after(SSE_TIMEOUT_SECONDS): |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_dlq_routes.py`:
- Around line 332-341: The test test_discard_dlq_message_requires_reason is too
permissive (accepts 404 or 422) and thus doesn't verify the "reason required"
behavior; change it to reliably target an existing DLQ message id before calling
DELETE /api/v1/dlq/messages/{id} so the missing-reason validation is exercised:
use test_user to list or create a DLQ message (e.g., GET /api/v1/dlq/messages to
pick an existing event id or create one via the appropriate helper), assert the
id exists, then call DELETE on that id without the reason and assert
response.status_code == 422 to confirm the validation triggers (keep the test
name test_discard_dlq_message_requires_reason and update only the setup and
assertion).
In `@backend/tests/e2e/test_notifications_routes.py`:
- Around line 209-217: Test uses an invalid ID string which triggers UUID
validation errors; change the nonexistent ID to a valid UUID format. In
test_delete_nonexistent_notification replace "nonexistent-notification-id" with
a valid UUID string (e.g. str(uuid.uuid4()) or a constant like
"11111111-1111-1111-1111-111111111111") and, if generating, add "import uuid" at
the top so the request to "/api/v1/notifications/{uuid}" hits the 404-not-found
path rather than a 422/400 validation error.
In `@backend/tests/e2e/test_sse_routes.py`:
- Line 23: The assertion in backend/tests/e2e/test_sse_routes.py currently
checks result.status against ["healthy","degraded","unhealthy"], which
mismatches the SSE contract; update the test to assert the status values used by
the API contract (e.g., "healthy" and "draining") or, better, import and compare
against the canonical enum/constant from the API contract (for example the
HealthStatus enum or SSE health constant) instead of hardcoding strings so the
test stays aligned with the contract.
- Around line 50-56: Tests currently use anyio.move_on_after (with
SSE_TIMEOUT_SECONDS) which silently cancels the block on timeout and can mask
hung streams; replace anyio.move_on_after(...) with anyio.fail_after(...) in the
test_sse_routes.py blocks that wrap the async with test_user.stream(...) calls
(references: SSE_TIMEOUT_SECONDS, test_user.stream) so a timeout raises
TimeoutError and fails the test instead of skipping assertions.
♻️ Duplicate comments (6)
backend/tests/e2e/test_sse_routes.py (1)
108-126: Docstring says “cannot stream,” but the test still allows 200.Either enforce
403or update the docstring/expectation to reflect allowed empty-stream behavior.backend/tests/e2e/test_notifications_routes.py (2)
89-98: Use a valid UUID to exercise the 404 branch.
"nonexistent-id"likely fails validation and may return 400/422, so this test doesn’t reliably prove not‑found behavior. Use a valid UUID and assert 404.🐛 Proposed fix
- response = await test_user.put( - "/api/v1/notifications/nonexistent-id/read" - ) - - # Should be 404 or similar error - assert response.status_code in [404, 400] + nonexistent_id = "00000000-0000-0000-0000-000000000000" + response = await test_user.put( + f"/api/v1/notifications/{nonexistent_id}/read" + ) + assert response.status_code == 404
224-244: Avoid silent pass when no notification exists or delete fails.This test currently does nothing when list/delete fails or returns empty results, so it can pass without validating deletion. Assert or explicitly skip to prevent false positives.
🐛 Proposed fix
- if list_response.status_code == 200: - result = NotificationListResponse.model_validate(list_response.json()) - if result.notifications: - notification_id = result.notifications[0].notification_id - - # Delete it - delete_response = await test_user.delete( - f"/api/v1/notifications/{notification_id}" - ) - - if delete_response.status_code == 200: - delete_result = DeleteNotificationResponse.model_validate( - delete_response.json() - ) - assert "deleted" in delete_result.message.lower() + assert list_response.status_code == 200 + result = NotificationListResponse.model_validate(list_response.json()) + if not result.notifications: + pytest.skip("No notifications available to delete in this environment") + notification_id = result.notifications[0].notification_id + + delete_response = await test_user.delete( + f"/api/v1/notifications/{notification_id}" + ) + assert delete_response.status_code == 200 + delete_result = DeleteNotificationResponse.model_validate( + delete_response.json() + ) + assert "deleted" in delete_result.message.lower()backend/app/api/routes/events.py (1)
38-51: Handle missing executions to avoid 500s.
get_execution_resultraisesExecutionNotFoundError; without handling, this becomes a 500 instead of 404.🐛 Proposed fix
+from app.domain.exceptions import ExecutionNotFoundError + `@router.get`("/executions/{execution_id}/events", response_model=EventListResponse) async def get_execution_events( @@ ) -> EventListResponse: # Check execution ownership first (before checking events) - execution = await execution_service.get_execution_result(execution_id) + try: + execution = await execution_service.get_execution_result(execution_id) + except ExecutionNotFoundError: + raise HTTPException(status_code=404, detail="Execution not found") if execution.user_id and execution.user_id != current_user.user_id and current_user.role != UserRole.ADMIN: raise HTTPException(status_code=403, detail="Access denied")backend/tests/e2e/test_events_routes.py (2)
221-239: Prevent vacuous pass when the user has no events.
Ifresult.eventsis empty, the test makes no assertions and passes silently.✅ Suggested fix
if events_response.status_code == 200: result = EventListResponse.model_validate(events_response.json()) - if result.events: - event_id = result.events[0].event_id - - # Get single event - response = await test_user.get(f"/api/v1/events/{event_id}") - - assert response.status_code == 200 - event = EventResponse.model_validate(response.json()) - assert event.event_id == event_id + if not result.events: + pytest.skip("No events available to test single-event endpoint") + event_id = result.events[0].event_id + + # Get single event + response = await test_user.get(f"/api/v1/events/{event_id}") + + assert response.status_code == 200 + event = EventResponse.model_validate(response.json()) + assert event.event_id == event_id
371-385: Make dry-run replay test deterministic (avoid silent pass).
The test currently allows a 404 path without asserting anything, so it can pass without validating dry-run behavior.✅ Suggested fix
response = await test_admin.post( f"/api/v1/events/replay/{created_execution_admin.execution_id}", params={"dry_run": True}, ) - # May be 200 or 404 depending on event availability - if response.status_code == 200: - result = ReplayAggregateResponse.model_validate(response.json()) - assert result.dry_run is True - assert result.aggregate_id == created_execution_admin.execution_id + # Be explicit about the outcome to avoid silent passes + assert response.status_code in (200, 404), f"Unexpected status: {response.status_code}" + if response.status_code == 404: + pytest.skip("No events available for replay dry-run test") + result = ReplayAggregateResponse.model_validate(response.json()) + assert result.dry_run is True + assert result.aggregate_id == created_execution_admin.execution_id
🧹 Nitpick comments (4)
backend/tests/e2e/test_dlq_routes.py (2)
90-116: Consider validating filter effectiveness when messages exist.The
test_get_dlq_messages_by_topicandtest_get_dlq_messages_by_event_typetests only verify the response is a list, but don't assert that returned messages match the filter criteria (unliketest_get_dlq_messages_by_statuswhich correctly validates each message's status).💡 Suggested improvement
`@pytest.mark.asyncio` async def test_get_dlq_messages_by_topic( self, test_user: AsyncClient ) -> None: """Filter DLQ messages by topic.""" response = await test_user.get( "/api/v1/dlq/messages", params={"topic": "execution-events"}, ) assert response.status_code == 200 result = DLQMessagesResponse.model_validate(response.json()) - assert isinstance(result.messages, list) + for msg in result.messages: + assert msg.original_topic == "execution-events" `@pytest.mark.asyncio` async def test_get_dlq_messages_by_event_type( self, test_user: AsyncClient ) -> None: """Filter DLQ messages by event type.""" response = await test_user.get( "/api/v1/dlq/messages", params={"event_type": EventType.EXECUTION_REQUESTED}, ) assert response.status_code == 200 result = DLQMessagesResponse.model_validate(response.json()) - assert isinstance(result.messages, list) + for msg in result.messages: + assert msg.event.event_type == EventType.EXECUTION_REQUESTED
132-161: Consider usingpytest.skip()for explicit skip behavior.The nested conditionals allow this test to pass silently without actually testing the detail endpoint when no messages exist. Using
pytest.skip()would make test runs more informative.💡 Suggested improvement
`@pytest.mark.asyncio` async def test_get_dlq_message_detail( self, test_user: AsyncClient ) -> None: """Get DLQ message detail if messages exist.""" # First list messages to find one list_response = await test_user.get( "/api/v1/dlq/messages", params={"limit": 1}, ) - if list_response.status_code == 200: - result = DLQMessagesResponse.model_validate(list_response.json()) - if result.messages: - event_id = result.messages[0].event.event_id - - # Get detail - response = await test_user.get( - f"/api/v1/dlq/messages/{event_id}" - ) - - if response.status_code == 200: - detail = DLQMessageDetail.model_validate(response.json()) - assert detail.event is not None - assert detail.original_topic is not None - assert detail.error is not None - assert detail.retry_count >= 0 - assert detail.failed_at is not None - assert detail.status is not None + assert list_response.status_code == 200 + result = DLQMessagesResponse.model_validate(list_response.json()) + if not result.messages: + pytest.skip("No DLQ messages available for detail test") + + event_id = result.messages[0].event.event_id + response = await test_user.get(f"/api/v1/dlq/messages/{event_id}") + + assert response.status_code == 200 + detail = DLQMessageDetail.model_validate(response.json()) + assert detail.event is not None + assert detail.original_topic is not None + assert detail.error is not None + assert detail.retry_count >= 0 + assert detail.failed_at is not None + assert detail.status is not Nonebackend/app/api/routes/execution.py (1)
42-52: Parameter shadows imported dependency function.The parameter
current_useron line 44 shadows the importedcurrent_userfunction fromapp.api.dependencies. While this works becauseDepends(current_user)captures the function reference at decoration time, it can cause confusion and potential issues if someone tries to use the dependency elsewhere in the function.Consider renaming the parameter to avoid shadowing:
♻️ Suggested rename
`@inject` async def get_execution_with_access( execution_id: Annotated[str, Path()], - current_user: Annotated[UserResponse, Depends(current_user)], + user: Annotated[UserResponse, Depends(current_user)], execution_service: FromDishka[ExecutionService], ) -> ExecutionInDB: domain_exec = await execution_service.get_execution_result(execution_id) - if domain_exec.user_id and domain_exec.user_id != current_user.user_id and current_user.role != UserRole.ADMIN: + if domain_exec.user_id and domain_exec.user_id != user.user_id and user.role != UserRole.ADMIN: raise HTTPException(status_code=403, detail="Access denied") return ExecutionInDB.model_validate(domain_exec)Note: This same shadowing pattern appears in other functions (
cancel_execution,retry_execution,get_user_executions). A consistent rename across all would improve clarity.backend/tests/e2e/test_execution_routes.py (1)
213-237: Avoid skipping on 5xx to surface cancel regressions.
Skipping on server errors can hide real failures. Consider failing fast or explicitly handling “unsupported” status codes.♻️ Possible tightening
- if cancel_response.status_code >= 500: - pytest.skip("Cancellation not wired; backend returned 5xx") - # Should succeed or fail if already completed - assert cancel_response.status_code in [200, 400, 404] + if cancel_response.status_code >= 500: + pytest.fail(f"Cancel endpoint error: {cancel_response.status_code}") + # Should succeed or fail if already completed / not found + assert cancel_response.status_code in [200, 400, 404]
backend/tests/e2e/test_dlq_routes.py
Outdated
Comment on lines
332
to
341
| @pytest.mark.asyncio | ||
| async def test_discard_dlq_message_requires_reason( | ||
| self, test_user: AsyncClient | ||
| ) -> None: | ||
| """Discard requires reason parameter.""" | ||
| response = await test_user.delete( | ||
| "/api/v1/dlq/messages/some-event-id" | ||
| ) | ||
| # 422 if reason is required but not provided | ||
| assert response.status_code in [404, 422] |
There was a problem hiding this comment.
Imprecise status code assertion masks test intent.
The test is named test_discard_dlq_message_requires_reason but accepts both 404 and 422. If the event ID "some-event-id" doesn't exist, the API returns 404, and the test passes without validating that missing reason causes 422.
Consider using an existing event ID (from listing) to isolate the "reason required" behavior:
💡 Suggested improvement
`@pytest.mark.asyncio`
async def test_discard_dlq_message_requires_reason(
self, test_user: AsyncClient
) -> None:
"""Discard requires reason parameter."""
+ # First find an existing message to test against
+ list_response = await test_user.get(
+ "/api/v1/dlq/messages",
+ params={"limit": 1},
+ )
+ if list_response.status_code == 200:
+ result = DLQMessagesResponse.model_validate(list_response.json())
+ if result.messages:
+ event_id = result.messages[0].event.event_id
+ response = await test_user.delete(
+ f"/api/v1/dlq/messages/{event_id}"
+ )
+ assert response.status_code == 422
+ return
+ # Fallback if no messages exist
response = await test_user.delete(
"/api/v1/dlq/messages/some-event-id"
)
- # 422 if reason is required but not provided
assert response.status_code in [404, 422]🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_dlq_routes.py` around lines 332 - 341, The test
test_discard_dlq_message_requires_reason is too permissive (accepts 404 or 422)
and thus doesn't verify the "reason required" behavior; change it to reliably
target an existing DLQ message id before calling DELETE
/api/v1/dlq/messages/{id} so the missing-reason validation is exercised: use
test_user to list or create a DLQ message (e.g., GET /api/v1/dlq/messages to
pick an existing event id or create one via the appropriate helper), assert the
id exists, then call DELETE on that id without the reason and assert
response.status_code == 422 to confirm the validation triggers (keep the test
name test_discard_dlq_message_requires_reason and update only the setup and
assertion).
Comment on lines
+209
to
+217
| async def test_delete_nonexistent_notification( | ||
| self, test_user: AsyncClient | ||
| ) -> None: | ||
| """Deleting nonexistent notification returns 404.""" | ||
| response = await test_user.delete( | ||
| "/api/v1/notifications/nonexistent-notification-id" | ||
| ) | ||
|
|
||
| assert response.status_code == 404 |
There was a problem hiding this comment.
Use a valid UUID for the nonexistent delete case.
If the route validates UUIDs, "nonexistent-notification-id" will produce a 422/400 instead of a true 404. Use a valid UUID to keep the test signal clean.
🐛 Proposed fix
- response = await test_user.delete(
- "/api/v1/notifications/nonexistent-notification-id"
- )
-
- assert response.status_code == 404
+ nonexistent_id = "00000000-0000-0000-0000-000000000000"
+ response = await test_user.delete(
+ f"/api/v1/notifications/{nonexistent_id}"
+ )
+ assert response.status_code == 404🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_notifications_routes.py` around lines 209 - 217, Test
uses an invalid ID string which triggers UUID validation errors; change the
nonexistent ID to a valid UUID format. In test_delete_nonexistent_notification
replace "nonexistent-notification-id" with a valid UUID string (e.g.
str(uuid.uuid4()) or a constant like "11111111-1111-1111-1111-111111111111")
and, if generating, add "import uuid" at the top so the request to
"/api/v1/notifications/{uuid}" hits the 404-not-found path rather than a 422/400
validation error.
| assert response.status_code == 200 | ||
| result = SSEHealthResponse.model_validate(response.json()) | ||
|
|
||
| assert result.status in ["healthy", "degraded", "unhealthy"] |
There was a problem hiding this comment.
Align expected SSE health statuses with the contract.
The assertion allows "degraded"/"unhealthy", but the SSE health schema description suggests "healthy"/"draining". If the backend returns "draining", this test will fail despite a valid response. Consider referencing the canonical enum/constant used by the API contract to avoid drift.
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_sse_routes.py` at line 23, The assertion in
backend/tests/e2e/test_sse_routes.py currently checks result.status against
["healthy","degraded","unhealthy"], which mismatches the SSE contract; update
the test to assert the status values used by the API contract (e.g., "healthy"
and "draining") or, better, import and compare against the canonical
enum/constant from the API contract (for example the HealthStatus enum or SSE
health constant) instead of hardcoding strings so the test stays aligned with
the contract.
Comment on lines
+50
to
+56
| with anyio.move_on_after(SSE_TIMEOUT_SECONDS): | ||
| async with test_user.stream( | ||
| "GET", "/api/v1/events/notifications/stream" | ||
| ) as response: | ||
| assert response.status_code == 200 | ||
| content_type = response.headers.get("content-type", "") | ||
| assert "text/event-stream" in content_type |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AnyIO move_on_after vs fail_after timeout behavior documentation
💡 Result:
In AnyIO, both move_on_after() and fail_after() create a new cancel scope with a deadline = now + delay (or no deadline if delay=None). (anyio.readthedocs.io)
move_on_after(delay, *, shield=False)
- On timeout, it cancels the scope and then exits the
withblock without raising a timeout exception (execution continues after the block). (anyio.readthedocs.io) - You can detect that a timeout happened by checking the yielded scope (e.g.
scope.cancelled_caughtin newer docs; older docs usedcancel_called). (anyio.readthedocs.io)
fail_after(delay, *, shield=False)
- On timeout, it raises
TimeoutError. (anyio.readthedocs.io) - AnyIO’s docs note a caveat: don’t directly cancel a scope created by
fail_after(), because it may lead to aTimeoutErrorbeing raised “erroneously” if scope exit is delayed long enough for the deadline to pass. (anyio.readthedocs.io)
Deadline details (common to both)
- The cancel scope exposes a
deadlineattribute, but an outer scope may have an earlier deadline; usecurrent_effective_deadline()to get the actual nearest effective deadline. (anyio.readthedocs.io)
Citations:
- 1: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
- 2: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
- 3: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
- 4: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
- 5: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
- 6: https://anyio.readthedocs.io/en/latest/cancellation.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check if the test file exists and examine the context around the mentioned lines
if [ -f "backend/tests/e2e/test_sse_routes.py" ]; then
echo "=== File found, checking line count ==="
wc -l backend/tests/e2e/test_sse_routes.py
echo ""
echo "=== Lines 45-130 (showing context around all three occurrences) ==="
sed -n '45,130p' backend/tests/e2e/test_sse_routes.py
else
echo "File not found at backend/tests/e2e/test_sse_routes.py"
fiRepository: HardMax71/Integr8sCode
Length of output: 3784
🏁 Script executed:
#!/bin/bash
# Search for all move_on_after usages in the file
echo "=== All move_on_after usages ==="
rg -n "move_on_after" backend/tests/e2e/test_sse_routes.py -B 2 -A 8Repository: HardMax71/Integr8sCode
Length of output: 1770
Replace move_on_after with fail_after so timeouts fail the test explicitly.
move_on_after silently cancels and exits the block without raising an exception, allowing assertions to be skipped if a timeout occurs—masking hung streams as passing tests. Use fail_after instead, which raises TimeoutError on timeout, ensuring deterministic test failures.
🧩 Proposed changes
- with anyio.move_on_after(SSE_TIMEOUT_SECONDS):
+ with anyio.fail_after(SSE_TIMEOUT_SECONDS):
async with test_user.stream(
"GET", "/api/v1/events/notifications/stream"
) as response:
assert response.status_code == 200
content_type = response.headers.get("content-type", "")
assert "text/event-stream" in content_type- with anyio.move_on_after(SSE_TIMEOUT_SECONDS):
+ with anyio.fail_after(SSE_TIMEOUT_SECONDS):
async with test_user.stream(
"GET", f"/api/v1/events/executions/{execution.execution_id}"
) as response:
assert response.status_code == 200
content_type = response.headers.get("content-type", "")
assert "text/event-stream" in content_type- with anyio.move_on_after(SSE_TIMEOUT_SECONDS):
+ with anyio.fail_after(SSE_TIMEOUT_SECONDS):
async with another_user.stream(
"GET", f"/api/v1/events/executions/{execution_id}"
) as response:
# Should be forbidden or return empty stream
assert response.status_code in [200, 403]
if response.status_code == 200:
# If 200, content type should still be event-stream
content_type = response.headers.get("content-type", "")
assert "text/event-stream" in content_typeApplies to lines: 50–56, 88–94, 121–126
📝 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.
Suggested change
| with anyio.move_on_after(SSE_TIMEOUT_SECONDS): | |
| async with test_user.stream( | |
| "GET", "/api/v1/events/notifications/stream" | |
| ) as response: | |
| assert response.status_code == 200 | |
| content_type = response.headers.get("content-type", "") | |
| assert "text/event-stream" in content_type | |
| with anyio.fail_after(SSE_TIMEOUT_SECONDS): | |
| async with test_user.stream( | |
| "GET", "/api/v1/events/notifications/stream" | |
| ) as response: | |
| assert response.status_code == 200 | |
| content_type = response.headers.get("content-type", "") | |
| assert "text/event-stream" in content_type |
🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_sse_routes.py` around lines 50 - 56, Tests currently
use anyio.move_on_after (with SSE_TIMEOUT_SECONDS) which silently cancels the
block on timeout and can mask hung streams; replace anyio.move_on_after(...)
with anyio.fail_after(...) in the test_sse_routes.py blocks that wrap the async
with test_user.stream(...) calls (references: SSE_TIMEOUT_SECONDS,
test_user.stream) so a timeout raises TimeoutError and fails the test instead of
skipping assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_admin_users_routes.py`:
- Around line 157-179: Add an assertion that the initial user creation succeeded
so the duplicate check is valid: capture the response from the first
test_admin.post call that creates the first user (the one with username
f"duplicate_{uid}") and assert it returned a success status (e.g., 200/201)
and/or expected body before attempting the second post; update the existing
variable usage (e.g., reuse or rename the first response) and ensure the check
is done immediately after the first create call and before the duplicate POST
and final 400 assertion.
♻️ Duplicate comments (6)
backend/tests/e2e/test_replay_routes.py (3)
182-198: Same silent skip pattern asTestStartReplaySession.
266-280: Same silent skip pattern asTestStartReplaySession.
135-151: Silent skip on failure masks broken endpoints.The
if create_response.status_code == 200:pattern causes the test to pass without any assertions if session creation fails. A broken create endpoint would result in a green test. This pattern also appears inTestPauseReplaySessionandTestCancelReplaySession.Proposed fix
- if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) - - # Start session - response = await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/start" - ) - - # May be 200 or error depending on session state - if response.status_code == 200: - result = ReplayResponse.model_validate(response.json()) - assert result.session_id == session.session_id - assert result.status in [ - ReplayStatus.RUNNING, - ReplayStatus.COMPLETED, - ReplayStatus.FAILED, - ] + assert create_response.status_code == 200 + session = ReplayResponse.model_validate(create_response.json()) + + # Start session + response = await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/start" + ) + + # May be 200 or error depending on session state + if response.status_code == 200: + result = ReplayResponse.model_validate(response.json()) + assert result.session_id == session.session_id + assert result.status in [ + ReplayStatus.RUNNING, + ReplayStatus.COMPLETED, + ReplayStatus.FAILED, + ]backend/tests/e2e/test_notifications_routes.py (3)
88-96: Use a valid UUID to exercise the 404 path.
"nonexistent-id"is not a valid UUID and will likely trigger a 422/400 validation error rather than the intended 404 not-found response. Use a valid but non-existent UUID for a cleaner test signal.Suggested fix
`@pytest.mark.asyncio` async def test_mark_nonexistent_notification_read( self, test_user: AsyncClient ) -> None: """Marking nonexistent notification returns 404.""" + nonexistent_id = "00000000-0000-0000-0000-000000000000" response = await test_user.put( - "/api/v1/notifications/nonexistent-id/read" + f"/api/v1/notifications/{nonexistent_id}/read" ) assert response.status_code == 404
206-215: Use a valid UUID for the nonexistent delete case.
"nonexistent-notification-id"will fail UUID validation, returning 422/400 instead of the intended 404. Use a valid UUID format.Suggested fix
`@pytest.mark.asyncio` async def test_delete_nonexistent_notification( self, test_user: AsyncClient ) -> None: """Deleting nonexistent notification returns 404.""" + nonexistent_id = "00000000-0000-0000-0000-000000000000" response = await test_user.delete( - "/api/v1/notifications/nonexistent-notification-id" + f"/api/v1/notifications/{nonexistent_id}" ) - assert response.status_code == 404
217-242: Test can silently pass without validating delete behavior.This test has multiple conditional branches that allow it to pass without actually exercising the delete logic or its assertions. If
list_responsefails, returns no notifications, or delete returns non-200, the test passes without any meaningful validation.Suggested fix — fail explicitly or skip when preconditions aren't met
`@pytest.mark.asyncio` async def test_delete_notification_response_format( self, test_user: AsyncClient ) -> None: """Delete response has correct format (when notification exists).""" # Get notifications first list_response = await test_user.get( "/api/v1/notifications", params={"limit": 1}, ) + assert list_response.status_code == 200, "Failed to list notifications" + result = NotificationListResponse.model_validate(list_response.json()) - if list_response.status_code == 200: - result = NotificationListResponse.model_validate(list_response.json()) - if result.notifications: - notification_id = result.notifications[0].notification_id + if not result.notifications: + pytest.skip("No notifications available to test delete response format") - # Delete it - delete_response = await test_user.delete( - f"/api/v1/notifications/{notification_id}" - ) + notification_id = result.notifications[0].notification_id + delete_response = await test_user.delete( + f"/api/v1/notifications/{notification_id}" + ) - if delete_response.status_code == 200: - delete_result = DeleteNotificationResponse.model_validate( - delete_response.json() - ) - assert "deleted" in delete_result.message.lower() + assert delete_response.status_code == 200, f"Delete failed: {delete_response.text}" + delete_result = DeleteNotificationResponse.model_validate(delete_response.json()) + assert "deleted" in delete_result.message.lower()
🧹 Nitpick comments (1)
backend/tests/e2e/test_dlq_routes.py (1)
133-161: Avoid silent pass when DLQ is empty; skip explicitly.If no messages exist, this test currently does nothing and passes, which reduces signal. Consider asserting the list call and
pytest.skipwhen there’s nothing to validate. This same pattern appears in retry/discard tests too.♻️ Proposed fix
list_response = await test_user.get( "/api/v1/dlq/messages", params={"limit": 1}, ) - if list_response.status_code == 200: - result = DLQMessagesResponse.model_validate(list_response.json()) - if result.messages: - event_id = result.messages[0].event.event_id + assert list_response.status_code == 200 + result = DLQMessagesResponse.model_validate(list_response.json()) + if not result.messages: + pytest.skip("No DLQ messages available to validate detail response") + + event_id = result.messages[0].event.event_id - # Get detail - response = await test_user.get( - f"/api/v1/dlq/messages/{event_id}" - ) + # Get detail + response = await test_user.get( + f"/api/v1/dlq/messages/{event_id}" + ) - if response.status_code == 200: - detail = DLQMessageDetail.model_validate(response.json()) - assert detail.event is not None - assert detail.original_topic is not None - assert detail.error is not None - assert detail.retry_count >= 0 - assert detail.failed_at is not None - assert detail.status is not None + assert response.status_code == 200 + detail = DLQMessageDetail.model_validate(response.json()) + assert detail.event is not None + assert detail.original_topic is not None + assert detail.error is not None + assert detail.retry_count >= 0 + assert detail.failed_at is not None + assert detail.status is not None
Comment on lines
+157
to
+179
| # Create first user | ||
| await test_admin.post( | ||
| "/api/v1/admin/users/", | ||
| json={ | ||
| "username": f"duplicate_{uid}", | ||
| "email": f"first_{uid}@example.com", | ||
| "password": "password123", | ||
| "role": UserRole.USER, | ||
| }, | ||
| ) | ||
|
|
||
| # Try to create second user with same username | ||
| response = await test_admin.post( | ||
| "/api/v1/admin/users/", | ||
| json={ | ||
| "username": f"duplicate_{uid}", | ||
| "email": f"second_{uid}@example.com", | ||
| "password": "password123", | ||
| "role": UserRole.USER, | ||
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 400 |
There was a problem hiding this comment.
Add assertion on first user creation to prevent false positives.
The first user creation response doesn't verify success. If this step fails (e.g., due to a server error), the test could pass erroneously without actually testing duplicate handling.
🐛 Proposed fix
# Create first user
- await test_admin.post(
+ first_response = await test_admin.post(
"/api/v1/admin/users/",
json={
"username": f"duplicate_{uid}",
"email": f"first_{uid}@example.com",
"password": "password123",
"role": UserRole.USER,
},
)
+ assert first_response.status_code == 200
# Try to create second user with same username🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_admin_users_routes.py` around lines 157 - 179, Add an
assertion that the initial user creation succeeded so the duplicate check is
valid: capture the response from the first test_admin.post call that creates the
first user (the one with username f"duplicate_{uid}") and assert it returned a
success status (e.g., 200/201) and/or expected body before attempting the second
post; update the existing variable usage (e.g., reuse or rename the first
response) and ensure the check is done immediately after the first create call
and before the duplicate POST and final 400 assertion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_user_settings_routes.py`:
- Around line 235-262: The test_restore_settings in class TestRestoreSettings
can exit without assertions if the history fetch fails or returns no entries;
add an explicit assertion that history_response.status_code == 200 after calling
test_user.get("/api/v1/user/settings/history") and, after validating into
SettingsHistoryResponse (variable history), if history.history is empty call
pytest.skip with a clear message so the test doesn't silently pass; ensure
subsequent restore_attempts still assert restore_response.status_code is in
[200, 400, 404].
♻️ Duplicate comments (9)
backend/tests/e2e/test_admin_settings_routes.py (1)
74-209: Restore global settings after mutation to keep tests order‑independent.
These updates change system‑wide settings without reverting, so later tests can observe modified defaults and become flaky. Consider snapshotting and restoring (or resetting in a fixture) in afinallyblock.🧹 Suggested pattern
- response = await test_admin.put( - "/api/v1/admin/settings/", json=request.model_dump() - ) - - assert response.status_code == 200 - settings = SystemSettings.model_validate(response.json()) + before = await test_admin.get("/api/v1/admin/settings/") + assert before.status_code == 200 + previous = SystemSettings.model_validate(before.json()) + try: + response = await test_admin.put( + "/api/v1/admin/settings/", json=request.model_dump() + ) + + assert response.status_code == 200 + settings = SystemSettings.model_validate(response.json()) + finally: + await test_admin.put( + "/api/v1/admin/settings/", + json=previous.model_dump(), + )backend/tests/e2e/test_admin_users_routes.py (1)
151-179: Assert the first create succeeds before testing duplicates.
Without checking the initial create, the duplicate check can pass even if user creation fails upstream.✅ Suggested fix
- await test_admin.post( + first_response = await test_admin.post( "/api/v1/admin/users/", json={ "username": f"duplicate_{uid}", "email": f"first_{uid}@example.com", "password": "password123", "role": UserRole.USER, }, ) + assert first_response.status_code == 200 # Try to create second user with same usernamebackend/tests/e2e/test_admin_events_routes.py (4)
279-310: Don’t silently pass when no events are found.
Right now the test exits without assertions if browse is non‑200 or empty. Assert the browse status and explicitly skip if there are no events.✅ Suggested fix
- if browse_response.status_code == 200: - browse_result = EventBrowseResponse.model_validate( - browse_response.json() - ) - if browse_result.events: - event_id = browse_result.events[0].get("event_id") + assert browse_response.status_code == 200 + browse_result = EventBrowseResponse.model_validate( + browse_response.json() + ) + if not browse_result.events: + pytest.skip("No events found for execution; skipping detail test.") + event_id = browse_result.events[0].get("event_id")
334-354: Assert allowed replay statuses (avoid silent pass on 5xx).
The dry-run replay test currently skips all assertions on non‑200 responses. Add an explicit status check and a skip with a message when replay isn’t available.✅ Suggested fix
- # May be 200, 400, or 404 depending on events availability - if response.status_code == 200: - result = EventReplayResponse.model_validate(response.json()) + assert response.status_code in (200, 400, 404) + if response.status_code != 200: + pytest.skip("Replay not available for this execution.") + result = EventReplayResponse.model_validate(response.json())
412-451: Replay status test can silently pass.
If replay/start or status calls fail, the test exits without any assertion. Make the allowed statuses explicit and skip with a reason when replay/status is unavailable.✅ Suggested fix
- if replay_response.status_code == 200: - replay_result = EventReplayResponse.model_validate( - replay_response.json() - ) - if replay_result.session_id: - # Get status - status_response = await test_admin.get( - f"/api/v1/admin/events/replay/{replay_result.session_id}/status" - ) - - if status_response.status_code == 200: - status = EventReplayStatusResponse.model_validate( - status_response.json() - ) + assert replay_response.status_code in (200, 400, 404) + if replay_response.status_code != 200: + pytest.skip("Replay not started; skipping status check.") + replay_result = EventReplayResponse.model_validate( + replay_response.json() + ) + if not replay_result.session_id: + pytest.skip("Replay session_id missing; skipping status check.") + status_response = await test_admin.get( + f"/api/v1/admin/events/replay/{replay_result.session_id}/status" + ) + assert status_response.status_code in (200, 404) + if status_response.status_code != 200: + pytest.skip("Replay status not available yet.") + status = EventReplayStatusResponse.model_validate( + status_response.json() + )
466-498: Delete test can silently pass when no events are available.
Assert the browse status and skip explicitly if no events exist; also assert the delete status to avoid false greens.✅ Suggested fix
- if browse_response.status_code == 200: - browse_result = EventBrowseResponse.model_validate( - browse_response.json() - ) - if browse_result.events: - event_id = browse_result.events[0].get("event_id") - - # Delete event - response = await test_admin.delete( - f"/api/v1/admin/events/{event_id}" - ) - - if response.status_code == 200: - result = EventDeleteResponse.model_validate( - response.json() - ) - assert result.event_id == event_id - assert "deleted" in result.message.lower() + assert browse_response.status_code == 200 + browse_result = EventBrowseResponse.model_validate( + browse_response.json() + ) + if not browse_result.events: + pytest.skip("No events found for execution; skipping delete test.") + event_id = browse_result.events[0].get("event_id") + + response = await test_admin.delete( + f"/api/v1/admin/events/{event_id}" + ) + assert response.status_code == 200 + result = EventDeleteResponse.model_validate(response.json()) + assert result.event_id == event_id + assert "deleted" in result.message.lower()backend/tests/e2e/test_replay_routes.py (3)
119-152: Don’t silently pass if session creation/start fails.
The current conditional means this test can pass without assertions. Assert session creation succeeded and explicitly skip when start is rejected.✅ Suggested fix
- if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) - - # Start session - response = await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/start" - ) - - # May be 200 or error depending on session state - if response.status_code == 200: - result = ReplayResponse.model_validate(response.json()) - assert result.session_id == session.session_id - assert result.status in [ - ReplayStatus.RUNNING, - ReplayStatus.COMPLETED, - ReplayStatus.FAILED, - ] + assert create_response.status_code == 200 + session = ReplayResponse.model_validate(create_response.json()) + + response = await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/start" + ) + if response.status_code != 200: + pytest.skip(f"Start rejected: {response.status_code}") + result = ReplayResponse.model_validate(response.json()) + assert result.session_id == session.session_id + assert result.status in [ + ReplayStatus.RUNNING, + ReplayStatus.COMPLETED, + ReplayStatus.FAILED, + ]
168-199: Pause test can silently pass when creation/pause fails.
Assert creation succeeds and explicitly skip when pause is rejected to avoid false greens.✅ Suggested fix
- if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) - - # Start - await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/start" - ) - - # Pause - response = await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/pause" - ) - - # May succeed or fail depending on session state - if response.status_code == 200: - result = ReplayResponse.model_validate(response.json()) - assert result.session_id == session.session_id + assert create_response.status_code == 200 + session = ReplayResponse.model_validate(create_response.json()) + + await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/start" + ) + + response = await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/pause" + ) + if response.status_code != 200: + pytest.skip(f"Pause rejected: {response.status_code}") + result = ReplayResponse.model_validate(response.json()) + assert result.session_id == session.session_id
252-281: Cancel test can silently pass when creation/cancel fails.
Assert creation succeeds and explicitly skip when cancel is rejected.✅ Suggested fix
- if create_response.status_code == 200: - session = ReplayResponse.model_validate(create_response.json()) - - # Cancel - response = await test_admin.post( - f"/api/v1/replay/sessions/{session.session_id}/cancel" - ) - - if response.status_code == 200: - result = ReplayResponse.model_validate(response.json()) - assert result.session_id == session.session_id - assert result.status in [ - ReplayStatus.CANCELLED, - ReplayStatus.COMPLETED, - ] + assert create_response.status_code == 200 + session = ReplayResponse.model_validate(create_response.json()) + + response = await test_admin.post( + f"/api/v1/replay/sessions/{session.session_id}/cancel" + ) + if response.status_code != 200: + pytest.skip(f"Cancel rejected: {response.status_code}") + result = ReplayResponse.model_validate(response.json()) + assert result.session_id == session.session_id + assert result.status in [ + ReplayStatus.CANCELLED, + ReplayStatus.COMPLETED, + ]
Comment on lines
+235
to
+262
| class TestRestoreSettings: | ||
| """Tests for POST /api/v1/user/settings/restore.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_restore_settings(self, test_user: AsyncClient) -> None: | ||
| """Restore settings to a previous point.""" | ||
| # Get history first | ||
| history_response = await test_user.get("/api/v1/user/settings/history") | ||
|
|
||
| if history_response.status_code == 200: | ||
| history = SettingsHistoryResponse.model_validate( | ||
| history_response.json() | ||
| ) | ||
|
|
||
| if history.history: | ||
| # Try to restore to first entry | ||
| restore_req = RestoreSettingsRequest( | ||
| timestamp=history.history[0].timestamp | ||
| ) | ||
| restore_response = await test_user.post( | ||
| "/api/v1/user/settings/restore", | ||
| json=restore_req.model_dump(mode="json"), | ||
| ) | ||
|
|
||
| # May succeed or fail depending on implementation | ||
| assert restore_response.status_code in [200, 400, 404] | ||
|
|
||
|
|
There was a problem hiding this comment.
Restore test can silently pass without assertions.
If history fetch fails or returns no entries, the test exits without checks. Assert the history response and skip explicitly when empty.
✅ Suggested fix
- if history_response.status_code == 200:
- history = SettingsHistoryResponse.model_validate(
- history_response.json()
- )
-
- if history.history:
- # Try to restore to first entry
- restore_req = RestoreSettingsRequest(
- timestamp=history.history[0].timestamp
- )
- restore_response = await test_user.post(
- "/api/v1/user/settings/restore",
- json=restore_req.model_dump(mode="json"),
- )
-
- # May succeed or fail depending on implementation
- assert restore_response.status_code in [200, 400, 404]
+ assert history_response.status_code == 200
+ history = SettingsHistoryResponse.model_validate(
+ history_response.json()
+ )
+
+ if not history.history:
+ pytest.skip("No settings history to restore.")
+
+ restore_req = RestoreSettingsRequest(
+ timestamp=history.history[0].timestamp
+ )
+ restore_response = await test_user.post(
+ "/api/v1/user/settings/restore",
+ json=restore_req.model_dump(mode="json"),
+ )
+
+ assert restore_response.status_code in (200, 400, 404)🤖 Prompt for AI Agents
In `@backend/tests/e2e/test_user_settings_routes.py` around lines 235 - 262, The
test_restore_settings in class TestRestoreSettings can exit without assertions
if the history fetch fails or returns no entries; add an explicit assertion that
history_response.status_code == 200 after calling
test_user.get("/api/v1/user/settings/history") and, after validating into
SettingsHistoryResponse (variable history), if history.history is empty call
pytest.skip with a clear message so the test doesn't silently pass; ensure
subsequent restore_attempts still assert restore_response.status_code is in
[200, 400, 404].
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by cubic
Adds a comprehensive backend E2E test suite across core APIs and Kafka flows, standardizes event types to EventType, enriches admin user listing with rate limit summaries, adds rate limit update request models, fixes DLQ event_type filtering, and hardens access checks for execution event APIs. Saved script deletion now returns not found, and admin delete user responds with structured counts.
New Features
Refactors
Written for commit a13b1a4. Summary will update on new commits.
Summary by CodeRabbit
Tests
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.