test: adding backend/e2e tests #97
Conversation
|
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. 📝 WalkthroughWalkthroughConsolidates and refactors test infrastructure (moves helpers into top-level conftest, adds many e2e suites and reclassifies markers), tightens event typing to EventType/DomainEvent, injects K8s clients into ResourceCleaner and DI providers, standardizes Kafka consumer/group naming, and introduces user delete/rate-limit models plus related repository/service/route signature updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventsAPI as Events API (/api/v1/events/executions/{id})
participant ExecSvc as ExecutionService
participant Repo as EventRepository
Client->>EventsAPI: GET /api/v1/events/executions/{execution_id}
EventsAPI->>ExecSvc: get_execution_result(execution_id)
ExecSvc-->>EventsAPI: Execution(owner_id=..., execution_id=...)
alt requester != owner and not admin
EventsAPI-->>Client: 403 Forbidden
else owner or admin
EventsAPI->>Repo: fetch_events(aggregate_id=execution_id, filters...)
Repo-->>EventsAPI: [DomainEvent...]
EventsAPI-->>Client: 200 OK (DomainEvent list)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ 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.github.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.
![coderabbitai[bot]](https://avatars-githubusercontent-com.github.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]
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
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.
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]
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
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, + ]
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/integration/test_events_routes.py (1)
103-106: Assertion logic is potentially incorrect.The condition
any(event_type in event.event_type for event_type in event_types)treatsevent.event_typeas a string and checks substring containment. However, with the migration toEventTypeenum,event.event_typeis now an enum value, not a string. This comparison may not work as intended.Consider using direct equality or membership check:
🔧 Suggested fix
# Filtered events should only contain specified types for event in events_response.events: if event.event_type: # Some events might have been created - assert any(event_type in event.event_type for event_type in event_types) or len( - events_response.events) == 0 + assert event.event_type in event_types or len(events_response.events) == 0
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/execution.py`:
- Around line 233-244: The function get_execution_events has a mismatch between
its FastAPI response_model (list[ExecutionDomainEvent]) and its Python return
type (list[DomainEvent]); update the function signature to return
list[ExecutionDomainEvent] and ensure the list returned by
event_service.get_events_by_aggregate (call site:
event_service.get_events_by_aggregate) is either typed as ExecutionDomainEvent
or converted/cast to that type before returning so static typing and runtime
serialization match.
In `@backend/tests/e2e/conftest.py`:
- Around line 78-87: The async fixtures created_execution and
created_execution_admin are decorated with `@pytest.fixture` which yields
coroutine objects; change both decorators to `@pytest_asyncio.fixture` so the
async functions are awaited by pytest_asyncio, ensuring the returned
ExecutionResponse values are awaited correctly; update the decorators above the
created_execution and created_execution_admin fixture definitions (and ensure
pytest_asyncio is imported if not already).
In `@backend/tests/e2e/test_resource_cleaner.py`:
- Around line 47-67: The test test_cleanup_nonexistent_pod can be flaky because
it asserts elapsed < 5 directly; change it to use a local timeout variable
(e.g., timeout = 5) and assert elapsed < timeout + small_buffer (e.g., 0.2s) to
account for scheduler jitter when calling
resource_cleaner.cleanup_pod_resources(pod_name=nonexistent_pod,
namespace=namespace, execution_id="test-exec-nonexistent", timeout=timeout);
update the assertion to compare against that timeout + buffer so the intent and
value remain consistent and less brittle.
- Around line 70-94: The test test_cleanup_orphaned_configmaps_dry_run uses a
seconds-based timestamp to build the ConfigMap name (variable name built from
int(datetime.now().timestamp())), which can collide in parallel runs; change the
naming to a collision-proof approach such as using uuid.uuid4().hex or
time.time_ns() when constructing name so each test gets a unique ConfigMap
identifier (update the line that assigns name in the test and keep the rest of
the test logic unchanged).
♻️ Duplicate comments (2)
backend/tests/e2e/test_events_routes.py (1)
385-389: Test has ambiguous success criteria.The test accepts both 200 and 404 responses, which means it can pass in multiple scenarios without clearly validating the dry-run behavior. This was previously flagged. Consider either ensuring deterministic test data via fixtures or explicitly skipping when no events are available.
backend/app/api/routes/events.py (1)
47-51: HandleExecutionNotFoundErrorto return 404 instead of 500.The
get_execution_resultcall at line 48 can raiseExecutionNotFoundErrorwhen the execution doesn't exist. This exception will propagate as an unhandled 500 error. This was previously flagged but appears unaddressed.🐛 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")
🧹 Nitpick comments (5)
backend/tests/integration/test_dlq_routes.py (1)
242-247: Consider validating the status value, not just its presence.The assertion checks that
"status"exists but doesn't verify it contains an expected value. The comment indicates it should be"success"or"failed".💡 Suggested improvement
if batch_result.details: assert isinstance(batch_result.details, list) for detail in batch_result.details: assert isinstance(detail, dict) assert "event_id" in detail - assert "status" in detail # status: "success" or "failed" + assert detail.get("status") in ("success", "failed"), \ + f"Unexpected status: {detail.get('status')}"backend/tests/conftest.py (1)
68-74: Redis DB isolation may collide under high parallelism.The formula
(_WORKER_NUM + sum(ord(c) for c in _RUN_ID)) % 16is deterministic but Redis only supports 16 databases (0-15) by default. With multiple concurrent CI runs or many xdist workers, different test sessions could map to the same Redis DB, causing key collisions for rate limiting and caching.This may be acceptable if:
- CI runs are serialized or use separate Redis instances
- Rate limit keys include additional unique identifiers
Otherwise, consider incorporating more entropy or using key prefixing within a single Redis DB instead of DB isolation.
backend/tests/helpers/cleanup.py (1)
11-27: Cleanup approach is sound; function is actively used.The UUID-based isolation pattern is a valid approach for parallel test execution without destructive cleanup. The thorough documentation explaining the rationale is helpful, and verification confirms the function is actively called in
backend/tests/integration/conftest.pyas part of the test fixture setup.The parameters
dbandredis_clientare not truly unused—they're passed from the pytest fixture dependency injection system. Adding underscore prefixes is optional and not necessary in this context since the pytest fixture pattern requires these parameters to be present for dependency resolution.backend/tests/e2e/test_events_routes.py (1)
233-243: Test can pass vacuously without exercising the endpoint.If
events_response.status_code != 200orresult.eventsis empty, the test passes without actually testing the single-event retrieval endpoint. Consider adding a precondition assertion or usingpytest.skip()to make it explicit when the test cannot run.💡 Suggested approach
`@pytest.mark.asyncio` async def test_get_event_by_id(self, test_user: AsyncClient) -> None: """Get single event by ID.""" # First get user events to find an event ID events_response = await test_user.get( "/api/v1/events/user", params={"limit": 1}, ) - 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 = DomainEventAdapter.validate_python(response.json()) - assert event.event_id == event_id + assert events_response.status_code == 200, "Failed to fetch user events" + result = EventListResponse.model_validate(events_response.json()) + + if not result.events: + pytest.skip("No events available to test single-event retrieval") + + event_id = result.events[0].event_id + response = await test_user.get(f"/api/v1/events/{event_id}") + + assert response.status_code == 200 + event = DomainEventAdapter.validate_python(response.json()) + assert event.event_id == event_idbackend/tests/e2e/test_execution_routes.py (1)
72-74: Useasyncio.get_running_loop()instead ofasyncio.get_event_loop().In async context,
asyncio.get_event_loop()is deprecated since Python 3.10 in favor ofasyncio.get_running_loop(). The current code works but may generate deprecation warnings.♻️ Suggested fix
async def wait_for_terminal_state( client: AsyncClient, execution_id: str, timeout: float = 90.0, poll_interval: float = 1.0, ) -> ExecutionResult: - deadline = asyncio.get_event_loop().time() + timeout + deadline = asyncio.get_running_loop().time() + timeout - while asyncio.get_event_loop().time() < deadline: + while asyncio.get_running_loop().time() < deadline: response = await client.get(f"/api/v1/executions/{execution_id}/result")
There was a problem hiding this comment.
1 issue found across 20 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/app/api/routes/execution.py">
<violation number="1" location="backend/app/api/routes/execution.py:239">
P2: Return type annotation `list[DomainEvent]` doesn't match the declared `response_model=list[ExecutionDomainEvent]`. This inconsistency can cause type checking issues and confusion. The return type should be `list[ExecutionDomainEvent]` to match the response model.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/tests/conftest.py`:
- Around line 19-35: The _get_worker_num function can raise when parsing
PYTEST_XDIST_WORKER (e.g., malformed "gwX"); update _get_worker_num to
defensively parse the environment variable by extracting digits (or using a
try/except around int conversion) and returning a safe fallback (e.g., 0) on
failure, then ensure test_settings still uses that value for REDIS_DB modulo 16;
reference the _get_worker_num function, PYTEST_XDIST_WORKER env var,
test_settings fixture and Settings.model_copy so reviewers can locate and apply
the defensive parsing and fallback logic.
- Around line 185-228: The factory function make_execution_requested_event
currently does list(runtime_command) which will split a plain string into
characters; update the normalization for the runtime_command parameter inside
make_execution_requested_event to ensure runtime_command is treated as a
sequence of strings: if runtime_command is a str, wrap it as [runtime_command];
otherwise convert an iterable of str to a list (e.g., list(runtime_command))
before passing into ExecutionRequestedEvent so callers passing a single string
are not turned into char arrays. Reference runtime_command and the
make_execution_requested_event return where runtime_command is used.
🧹 Nitpick comments (2)
backend/app/events/event_store_consumer.py (1)
55-58: Avoid relying on enum → str coercion in Kafka config.Line 57 now passes
self.group_iddirectly. IfGroupIdisn’t astrsubclass, this can break aiokafka config. Consider an explicitstr()cast (or verifyStringEnuminheritsstr).♻️ Proposed fix
- group_id=self.group_id, + group_id=str(self.group_id),backend/app/services/sse/kafka_redis_bridge.py (1)
66-70: Consider adding instance identity toclient_idfor observability.Line 69 now uses only the consumer index; across replicas this collides in Kafka metrics/logs. Optional: include hostname/pod UID to improve traceability.
There was a problem hiding this comment.
1 issue found across 68 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="docs/operations/cicd.md">
<violation number="1" location="docs/operations/cicd.md:166">
P3: The new step description says each E2E test sets up its own stack, but the updated diagram shows a shared stack setup feeding both tests. This is inconsistent and will mislead readers about the CI flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/e2e/services/sse/test_partitioned_event_router.py (1)
55-81: Session-scoped fixture mutation creates test isolation risk.Line 57 directly mutates the session-scoped
test_settingsfixture withtest_settings.SSE_CONSUMER_POOL_SIZE = 1. Since the fixture persists across all tests in the session, this mutation affects state visibility for any tests that depend on the original value, risking order-dependent failures or flakiness if test order changes.Use a local copy or the settings override pattern instead:
settings_copy = test_settings.model_copy(update={"SSE_CONSUMER_POOL_SIZE": 1})docs/operations/deployment.md (1)
143-143: Correct statement about worker container deployment in E2E tests.The statement claiming tests instantiate worker classes directly is outdated. The test suite is now E2E-based (as marked in pytest configuration) and requires worker containers to exercise the full system, including Kafka-dependent flows. Worker services (coordinator, k8s-worker, pod-monitor, result-processor, saga-orchestrator, event-replay, dlq-processor) are deployed as containers in docker-compose with continuous runtime. Update line 143 to reflect that workers are deployed as containers and run continuously to support E2E testing.
🤖 Fix all issues with AI agents
In `@docs/operations/cicd.md`:
- Around line 135-145: The diagram shows a single shared "Setup k3s + Stack"
node (node D) feeding both Backend E2E (E) and Frontend E2E (F), but the text
says "each setting up their own stack"; make them consistent by either (A)
changing the text that currently reads "each setting up their own stack" to
"Backend and frontend E2E tests run in parallel against a shared stack" to match
the diagram, or (B) if tests actually set up separate stacks, update the diagram
so there are two independent setup nodes (e.g., D1 -> E and D2 -> F) and adjust
any labels accordingly; ensure the chosen change updates both the diagram (nodes
D/E/F) and the corresponding sentence so they match.
♻️ Duplicate comments (2)
backend/tests/e2e/test_events_routes.py (2)
223-242: Test can pass vacuously without validating the endpoint.The nested conditionals at lines 232-234 allow the test to pass without executing any meaningful assertions if no events exist. Consider ensuring test data exists via a fixture or explicitly skipping when preconditions aren't met.
💡 Suggested fix
`@pytest.mark.asyncio` async def test_get_event_by_id(self, test_user: AsyncClient) -> None: """Get single event by ID.""" - # First get user events to find an event ID events_response = await test_user.get( "/api/v1/events/user", params={"limit": 1}, ) - - 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 = DomainEventAdapter.validate_python(response.json()) - assert event.event_id == event_id + assert events_response.status_code == 200 + result = EventListResponse.model_validate(events_response.json()) + + if not result.events: + pytest.skip("No events available to test single event retrieval") + + event_id = result.events[0].event_id + response = await test_user.get(f"/api/v1/events/{event_id}") + + assert response.status_code == 200 + event = DomainEventAdapter.validate_python(response.json()) + assert event.event_id == event_id
374-388: Test has ambiguous success criteria allowing silent pass on 404.The comment "May be 200 or 404 depending on event availability" and conditional assertion means the dry-run validation is skipped when no events exist. With
created_execution_adminfixture, consider either ensuring events exist or explicitly handling the 404 case.💡 Suggested fix
`@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 + 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/events/test_schema_registry_roundtrip.py (1)
25-25: Consider splitting the compound assertion for clearer failure messages.If this assertion fails, it won't be immediately clear which part failed (event_id mismatch vs execution_id mismatch).
♻️ Suggested improvement
- assert back.event_id == ev.event_id and getattr(back, "execution_id", None) == ev.execution_id + assert back.event_id == ev.event_id + assert getattr(back, "execution_id", None) == ev.execution_idbackend/tests/e2e/services/admin/test_admin_user_service.py (1)
16-26: Consider using unique IDs for test isolation.The hardcoded
user_id: "u1"could cause flaky tests in parallel execution or if the database isn't fully isolated per test. Since the PR mentions "ID-based isolation," consider generating a unique ID (e.g., UUID) for each test run.♻️ Suggested improvement
+import uuid + `@pytest.mark.asyncio` async def test_get_user_overview_basic(scope: AsyncContainer) -> None: svc: AdminUserService = await scope.get(AdminUserService) db: Database = await scope.get(Database) + user_id = f"test-{uuid.uuid4()}" await db.get_collection("users").insert_one({ - "user_id": "u1", + "user_id": user_id, "username": "bob", ... }) - overview = await svc.get_user_overview("u1", hours=1) + overview = await svc.get_user_overview(user_id, hours=1)backend/tests/e2e/db/repositories/test_admin_settings_repository.py (1)
33-37: Consider filtering audit log assertions by test-specific criteria.The assertions
count_documents({}) >= 1and>= 2count all documents in the audit log collection. This could pass spuriously if other tests leave behind audit entries (especially in parallel execution scenarios).Since the PR mentions ID-based isolation, consider filtering by specific criteria such as
user_id="u1"or a timestamp range to make assertions more precise and resilient.♻️ Suggested improvement
- assert await db.get_collection("audit_log").count_documents({}) >= 1 + assert await db.get_collection("audit_log").count_documents({"user_id": "u1"}) >= 1 reset = await repo.reset_system_settings("admin", "u1") assert isinstance(reset, SystemSettings) - assert await db.get_collection("audit_log").count_documents({}) >= 2 + assert await db.get_collection("audit_log").count_documents({"user_id": "u1"}) >= 2backend/tests/e2e/test_admin_routes.py (1)
356-364: Consider test isolation when modifying auth state.The test logs out the admin and logs in as a different user, modifying the
test_adminclient's session state. This works because fixtures typically provide fresh clients per test, but be mindful of this pattern if fixture scopes change.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/services/coordinator/test_execution_coordinator.py`:
- Around line 73-90: The test test_get_status_tracks_active_executions currently
asserts new_active >= initial_active which can pass without the handler adding
an execution; change the assertion so it requires exactly one more active
execution after invoking coord._handle_execution_requested(ev). Specifically,
after creating ev via
make_execution_requested_event(execution_id="e-status-track-1") and calling
await coord._handle_execution_requested(ev), fetch new_status with await
coord.get_status() and assert new_active == initial_active + 1 (referencing
ExecutionCoordinator.get_status and
ExecutionCoordinator._handle_execution_requested to locate the logic).
In `@backend/tests/e2e/services/saga/test_saga_service.py`:
- Around line 36-83: Seed sagas before calling SagaService.list_user_sagas so
assertions are deterministic: in test_list_user_sagas_empty ensure the user has
no sagas (delete/avoid creating any) and assert result.total == 0 and
len(result.sagas) == 0; in test_list_user_sagas_with_limit create >5 sagas for
the test user via the service (e.g., svc.create_saga or your test helper) then
call list_user_sagas(user, limit=5) and assert len(result.sagas) == 5 and
result.total == <created_count>; in test_list_user_sagas_with_skip create a
known ordered set of sagas, call list_user_sagas(user, skip=0, limit=10) and
assert returned list matches the expected subset when skipping is changed; in
test_list_user_sagas_filter_by_state create sagas with distinct SagaState values
and call list_user_sagas(user, state=SagaState.CREATED) asserting every
saga.state == SagaState.CREATED and result.total equals the number created with
that state; use SagaService (svc), list_user_sagas, SagaListResult, and
SagaState to locate and update the tests.
In `@backend/tests/e2e/services/user_settings/test_user_settings_service.py`:
- Around line 355-370: The test test_get_settings_history_with_limit should
assert that get_settings_history enforces the limit: after calling
svc.update_custom_setting(user_id, f"key_{i}", f"value_{i}") five times, call
svc.get_settings_history(user_id, limit=3) and assert the returned history has
length 3 and contains the most recent three updates (e.g., entries for key_4,
key_3, key_2 with their corresponding values) by checking the items/order
returned by UserSettingsService.get_settings_history rather than only checking
the type.
- Around line 394-410: The test test_restore_settings_to_past only asserts type;
capture the initial default settings before making changes (call
get_user_settings and store result, e.g., initial = await
svc.get_user_settings(user_id)), then after calling restore_settings_to_point
assert that the restored DomainUserSettings equals the initial defaults (or at
minimum that key fields like restored.theme match initial.theme) so the test
verifies the restore actually reverts changes; reference get_user_settings,
update_theme, restore_settings_to_point, and DomainUserSettings to locate where
to add the comparison.
- Around line 109-121: The test test_update_with_reason currently updates
settings via UserSettingsService.update_user_settings(user_id, updates,
reason="User preference") but never asserts the reason was persisted; modify the
test to call UserSettingsService.get_settings_history(user_id) and assert that
the returned history (list) contains at least one entry whose reason matches
"User preference" (e.g., check a top-level reason field or attribute on history
entries returned by get_settings_history), referencing the test function name
and the service methods update_user_settings and get_settings_history along with
DomainUserSettingsUpdate/Theme.DARK to locate the relevant code.
♻️ Duplicate comments (5)
backend/tests/e2e/test_admin_users_routes.py (1)
162-184: Add assertion on first user creation to prevent false positives.The first user creation response is discarded without verification. If the first POST fails (e.g., server error), the duplicate check could pass erroneously, making the test unreliable.
🐛 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, "First user creation must succeed" # Try to create second user with same usernamebackend/tests/e2e/test_admin_events_routes.py (4)
289-320: Don't silently pass when no events are found.If
browse_response.status_codeis not 200 orbrowse_result.eventsis empty, the test exits without any assertion, allowing regressions to slip through undetected.🐛 Proposed fix
browse_response = await test_admin.post( "/api/v1/admin/events/browse", json=request.model_dump() ) - 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) + 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") + + # 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)
436-460: Replay status test can silently pass on failures.If
replay_responseorstatus_responsereturns non-200, the test exits without assertions. Add explicit status checks or skips to prevent false green runs.🐛 Proposed fix
replay_response = await test_admin.post( "/api/v1/admin/events/replay", json=request.model_dump() ) - if replay_response.status_code == 200: + assert replay_response.status_code in [200, 400, 404], f"Unexpected: {replay_response.status_code}" + 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.") + else: status = EventReplayStatusResponse.model_validate( status_response.json() )
476-507: Delete test can silently pass when no events are found.Same pattern issue as
test_get_event_detail. Add assertions and explicit skip when prerequisites aren't met.🐛 Proposed fix
browse_response = await test_admin.post( "/api/v1/admin/events/browse", json=request.model_dump() ) - 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") + + # Delete event + 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()
344-363: Assert allowed status codes before conditional validation.The comment acknowledges multiple possible status codes (200, 400, 404), but no assertion enforces this. Unexpected 5xx errors would pass silently.
🐛 Proposed fix
response = await test_admin.post( "/api/v1/admin/events/replay", json=request.model_dump() ) - # May be 200, 400, or 404 depending on events availability + # Assert expected status codes to catch unexpected errors + assert response.status_code in [200, 400, 404], f"Unexpected status: {response.status_code}" + if response.status_code == 200: result = EventReplayResponse.model_validate(response.json()) assert result.dry_run is True
🧹 Nitpick comments (5)
backend/tests/e2e/services/coordinator/test_execution_coordinator.py (1)
25-38: Consider asserting priority handling explicitly.This test currently only checks that the execution is tracked, not that priority affects scheduling/queueing. If the coordinator or queue exposes priority, add a direct assertion to validate it.
backend/tests/e2e/services/saved_script/test_saved_script_service.py (2)
82-84: Minor: Redundant assertion.Line 84's substring check is redundant since line 83 already asserts exact equality of the entire script content. Consider removing it for clarity.
Suggested change
assert created.script == multiline_script - assert "def hello():" in created.script
32-452: Consider adding validation edge case tests.The test suite has excellent CRUD and access control coverage. For completeness, consider adding tests for validation edge cases such as:
- Empty script name or content
- Very long script content (if there are limits)
- Duplicate script names for the same user (if uniqueness is enforced)
These could be added in a follow-up if validation is handled at the service layer.
backend/tests/e2e/services/execution/test_execution_service.py (1)
186-192: Conditional event check could silently pass.If no events are generated due to timing issues, the test passes without validating anything meaningful. Consider adding an assertion or skip to make test behavior explicit.
💡 Suggested improvement
# Get events events = await svc.get_execution_events(exec_result.execution_id) assert isinstance(events, list) - # Should have at least EXECUTION_REQUESTED event - if events: - event_types = {e.event_type for e in events} - assert EventType.EXECUTION_REQUESTED in event_types + # Should have at least EXECUTION_REQUESTED event + assert len(events) > 0, "Expected at least one event for execution" + event_types = {e.event_type for e in events} + assert EventType.EXECUTION_REQUESTED in event_typesbackend/tests/e2e/services/replay/test_replay_service.py (1)
37-53: Potential flakiness with exact status assertion.Line 53 asserts
result.status == ReplayStatus.CREATED, but withmax_events=100the session might complete quickly. Consider matching the pattern used intest_create_session_execution_type(lines 30-34) which checks against multiple valid states.💡 Suggested fix
assert result.session_id is not None - assert result.status == ReplayStatus.CREATED + assert result.status in [ + ReplayStatus.CREATED, + ReplayStatus.RUNNING, + ReplayStatus.COMPLETED, + ]
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/services/execution/test_execution_service.py`:
- Around line 155-163: The test test_get_execution_result_not_found uses an
invalid hard-coded ID that may be rejected by the service’s ID format
validation; update the test (and the similar test around lines 364-371) to use a
syntactically valid ID (e.g., generate a UUID via uuid.uuid4() or use a valid
UUID string) when calling ExecutionService.get_execution_result so the call
proceeds to raise ExecutionNotFoundError; ensure references are adjusted in the
test file where ExecutionService and get_execution_result are used and assert
ExecutionNotFoundError as before.
- Around line 249-279: Test test_get_user_executions_pagination is flaky because
it assumes deterministic ordering from ExecutionService.get_user_executions;
ensure a stable sort by either (a) calling get_user_executions with an explicit
ordering parameter if the method supports it (e.g., order_by created_at then
execution_id) or (b) deterministically sorting the returned lists in the test
(sort page1 and page2 by created_at then execution_id) before comparing IDs;
update the test (test_get_user_executions_pagination) to use one of these
approaches referencing ExecutionService.get_user_executions so the disjoint-page
assertion is stable.
In `@backend/tests/e2e/services/replay/test_replay_service.py`:
- Around line 37-54: The test test_create_session_with_max_events asserts that
result.status == ReplayStatus.CREATED which is brittle because
ReplayService.create_session_from_config can immediately transition to RUNNING
or COMPLETED; update the assertion to accept multiple valid terminal/active
states (e.g., assert result.status in {ReplayStatus.CREATED,
ReplayStatus.RUNNING, ReplayStatus.COMPLETED}) or assert result.status !=
ReplayStatus.FAILED to avoid timing flakes while still ensuring session creation
succeeded.
- Around line 150-163: The test can be flaky because max_events=1000 doesn't
guarantee the session is RUNNING before cancellation; update the test around
svc.create_session_from_config and svc.cancel_session to first poll the session
via svc.get_session(created.session_id) (or equivalent) until the session.status
== ReplayStatus.RUNNING with a short timeout/retry loop, then call
svc.cancel_session and assert result.session_id == created.session_id and
result.status == ReplayStatus.CANCELLED; alternatively, if polling isn't
possible, relax the assertion to accept both ReplayStatus.CANCELLED and
ReplayStatus.COMPLETED to avoid false failures.
♻️ Duplicate comments (1)
backend/tests/e2e/services/coordinator/test_execution_coordinator.py (1)
72-89: Make the active execution count assertion meaningful.Line 89 uses
>=, which can pass even if the handler fails to add an execution. Tighten the expectation to validate the increment.💡 Suggested fix
- assert new_active >= initial_active + assert new_active == initial_active + 1
🧹 Nitpick comments (3)
backend/tests/e2e/services/coordinator/test_execution_coordinator.py (2)
24-37: Strengthen the priority test to validate priority handling.Line 28–37 only checks presence in
_active_executions, so it would pass even if priority is ignored. Consider asserting queue ordering or the stored priority for the enqueued item.
135-141:hasattris too weak for consumer readiness.Line 141 only checks attribute existence; the test can pass even if the consumer is never initialized. If the intent is to validate readiness, assert non-
Noneafter coordinator startup or ensure the fixture starts it.backend/tests/e2e/services/execution/test_execution_service.py (1)
169-192: Don’t let the events test pass on an empty list.
Right now a missing event stream still passes. Consider polling briefly and asserting at least one event to make this test meaningful.🔁 Suggested change (with short polling)
- events = await svc.get_execution_events(exec_result.execution_id) - - assert isinstance(events, list) - # Should have at least EXECUTION_REQUESTED event - if events: - event_types = {e.event_type for e in events} - assert EventType.EXECUTION_REQUESTED in event_types + events = [] + for _ in range(10): + events = await svc.get_execution_events(exec_result.execution_id) + if events: + break + await asyncio.sleep(0.5) + + assert events, "Expected at least one execution event" + event_types = {e.event_type for e in events} + assert EventType.EXECUTION_REQUESTED in event_types-import uuid +import asyncio +import uuid
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/routes/execution.py (1)
44-55: Return 404 when the execution is missing.
Ifget_execution_resultraisesExecutionNotFoundError, this currently surfaces as 500.🐛 Proposed fix
-from app.domain.exceptions import DomainError +from app.domain.exceptions import DomainError, ExecutionNotFoundError @@ - domain_exec = await execution_service.get_execution_result(execution_id) + try: + domain_exec = await execution_service.get_execution_result(execution_id) + except ExecutionNotFoundError: + raise HTTPException(status_code=404, detail="Execution not found")
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_resource_cleaner.py`:
- Around line 54-68: The test uses a very tight fixed timeout (timeout = 2) when
calling resource_cleaner.cleanup_pod_resources which can cause flakes on busy
CI; update the test to use a larger, configurable timeout (e.g., read from a
test settings/env var or increase to a safer default like 5–10s) and keep the
jitter_buffer logic; modify the variables timeout and jitter_buffer and the call
to cleanup_pod_resources so the timeout is sourced from settings (or a
TEST_TIMEOUT env) while still asserting elapsed < timeout + jitter_buffer;
reference the timeout, jitter_buffer, cleanup_pod_resources, nonexistent_pod and
namespace identifiers when making the change.
♻️ Duplicate comments (2)
backend/app/api/routes/events.py (1)
41-54: Return 404 when the execution is missing.
Ifget_execution_resultraisesExecutionNotFoundError, this currently surfaces as 500.🐛 Proposed fix
+from app.domain.exceptions import ExecutionNotFoundError @@ - 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")backend/tests/e2e/test_resource_cleaner.py (1)
1-3: Use a collision-proof ConfigMap name.
Seconds-based timestamps can collide in parallel runs.🧾 Proposed fix
-import asyncio -from datetime import datetime +import asyncio +from uuid import uuid4 @@ - name = f"int-test-cm-{int(datetime.now().timestamp())}" + name = f"int-test-cm-{uuid4().hex}"Also applies to: 82-83
🧹 Nitpick comments (5)
backend/tests/e2e/services/saga/test_saga_service.py (2)
179-189: Strengthen admin list test with cross-user visibility.
Right now it only checks types, so it could pass even if admin visibility is accidentally restricted. Consider seeding a non-admin saga and asserting total/len ≥ 1 (with a unique admin user ID).🔧 Suggested tweak
async def test_admin_can_list_all_sagas(self, scope: AsyncContainer) -> None: """Admin user can list all sagas.""" svc: SagaService = await scope.get(SagaService) - admin = make_test_user(user_id="admin_user", role=UserRole.ADMIN) + exec_repo: ExecutionRepository = await scope.get(ExecutionRepository) + saga_repo: SagaRepository = await scope.get(SagaRepository) + admin = make_test_user(user_id=f"admin_user_{uuid4().hex[:8]}", role=UserRole.ADMIN) + + # Seed a saga for a non-admin user to verify admin-wide visibility + other_user_id = f"admin_list_user_{uuid4().hex[:8]}" + exec_id = await create_execution_for_user(exec_repo, other_user_id) + await create_saga_for_execution(saga_repo, exec_id, saga_name="admin_visible") result = await svc.list_user_sagas(admin) assert isinstance(result, SagaListResult) - assert isinstance(result.sagas, list) + assert result.total >= 1 + assert len(result.sagas) >= 1
315-337: Seed a saga to validate retrieval, not just access.
The owner-access test currently passes even ifget_execution_sagasreturns empty; seeding one saga makes it meaningful.🔧 Suggested tweak
async def test_get_execution_sagas_owner_access( self, scope: AsyncContainer ) -> None: """Owner can get sagas for their execution.""" svc: SagaService = await scope.get(SagaService) exec_svc: ExecutionService = await scope.get(ExecutionService) + saga_repo: SagaRepository = await scope.get(SagaRepository) user_id = "saga_exec_owner" user = make_test_user(user_id=user_id) @@ exec_result = await exec_svc.execute_script( script="print('owner sagas')", user_id=user_id, client_ip="127.0.0.1", user_agent="pytest", lang="python", lang_version="3.11", ) + await create_saga_for_execution( + saga_repo, exec_result.execution_id, saga_name="owner_saga" + ) result = await svc.get_execution_sagas(exec_result.execution_id, user) assert isinstance(result, SagaListResult) - assert isinstance(result.sagas, list) + assert len(result.sagas) == 1 + assert result.sagas[0].execution_id == exec_result.execution_idbackend/tests/e2e/test_saga_routes.py (1)
16-49: Good helper pattern that addresses previous review concerns.The
wait_for_sagahelper properly polls with explicit assertions and raisesTimeoutErroron failure, addressing the previous review feedback about silent passes withif-guards.However,
asyncio.get_event_loop()is deprecated in Python 3.10+ for getting the running loop from coroutines. Consider usingasyncio.get_running_loop()instead.♻️ Suggested update
async def wait_for_saga( client: AsyncClient, execution_id: str, timeout: float = 30.0, poll_interval: float = 0.5, ) -> SagaStatusResponse: - deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout - while asyncio.get_event_loop().time() < deadline: + while loop.time() < deadline: response = await client.get(f"/api/v1/sagas/execution/{execution_id}")backend/tests/e2e/services/coordinator/test_execution_coordinator.py (1)
137-143: Consider strengthening the consumer assertion.
hasattr(coord, "consumer")only checks attribute existence, not that the consumer is properly initialized. Since the comment acknowledges it "may be None before", the test doesn't verify meaningful state. Consider asserting the consumer is not None after the coordinator starts, or document this as an existence-only check.backend/tests/e2e/test_notifications_routes.py (1)
19-50: Good helper pattern; same deprecation note applies.The
wait_for_notificationhelper properly addresses the silent pass concern from previous reviews. Same note aboutasyncio.get_event_loop()deprecation applies here—consider usingasyncio.get_running_loop().♻️ Suggested update
async def wait_for_notification( client: AsyncClient, timeout: float = 30.0, poll_interval: float = 0.5, ) -> NotificationResponse: - deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout - while asyncio.get_event_loop().time() < deadline: + while loop.time() < deadline: response = await client.get("/api/v1/notifications", params={"limit": 10})
There was a problem hiding this comment.
1 issue found across 11 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_admin_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_admin_events_routes.py:480">
P2: Strictly asserting SCHEDULED makes this E2E test timing-sensitive. The replay status can advance to RUNNING/COMPLETED within seconds, so the test may fail under slower or faster environments. Allow the valid post-schedule statuses (or poll until scheduled) to avoid flakiness.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/app/services/notification_service.py`:
- Around line 734-739: The total count currently calls
self.repository.count_notifications(user_id) without applying the incoming
status filter, causing pagination mismatch; update list_notifications to compute
and pass the same filter/condition used to fetch notifications into
count_notifications (e.g., use the same status condition or query object you
built earlier, or call self.repository.count_notifications(user_id,
status=status) if the repo API accepts it) so total reflects the filtered set,
and if your repo requires a document/condition type import (e.g.,
NotificationDocument) add that import; alternatively, if you intend total to be
unfiltered, add a clear code comment explaining that behavior.
In `@backend/tests/e2e/core/test_exception_handlers.py`:
- Around line 55-71: The test test_validation_error_format only asserts
status_code but its name implies checking the error payload; either rename the
test to something like test_validation_error_status or extend it to assert the
validation error schema: call the POST (as already done via test_user.post) and
check response.json() contains a "detail" list, then validate the first item has
keys "loc", "msg", and "type" and that "loc" or "msg" references "lang_version"
(to ensure the unsupported version triggered the error); update assertions in
test_validation_error_format accordingly.
In `@backend/tests/e2e/core/test_middlewares.py`:
- Around line 85-96: The test currently mutates test_user.headers by doing
csrf_token = test_user.headers.get("X-CSRF-Token") and del
test_user.headers["X-CSRF-Token"] which can leave shared client state
inconsistent on exceptions or if the header is missing; change this to remove
the header with pop (e.g. csrf_token = test_user.headers.pop("X-CSRF-Token",
None)) and wrap the call to test_user.post("/api/v1/execute", ...) in a
try/finally block that always restores test_user.headers["X-CSRF-Token"] =
csrf_token if csrf_token is not None (or omits re-adding when None) so the
client header state is reliably restored even on failure.
- Around line 152-153: The test currently asserts on response.json() which will
raise if the 413 body is plain text; update the assertion to first check
response.headers.get("Content-Type") for a JSON media type or attempt
response.json() in a try/except and fall back to response.text, then assert that
the string "too large" (lowercased) appears in the extracted body; modify the
assertions around the existing response.status_code == 413 and the following
line that inspects response.json()["detail"] to use this safe parsing approach
(operate on the existing response variable).
- Around line 206-210: The Cache-Control assertion in test_middlewares.py is too
permissive: when cache_control is present the test currently uses "or" allowing
public or max-age to slip through; change the assertion to require that neither
caching token is present (e.g., assert "public" not in cache_control and
"max-age" not in cache_control) so that if a Cache-Control header exists it must
not contain public or any max-age directive. Reference the cache_control
variable and the existing assertion in the POST test and update it accordingly.
♻️ Duplicate comments (3)
backend/tests/e2e/services/user_settings/test_user_settings_service.py (2)
333-341: Assert history is empty, not just a list.The test name says “no history,” but it never verifies emptiness.
✅ Suggested fix
history = await svc.get_settings_history(user_id) assert isinstance(history, list) + assert len(history) == 0
363-388: Align “most recent” expectation with the asserted ordering.The test says “most recent entries” but asserts
key_0..key_2(oldest). Please align assertions with the actual contract.✅ Suggested fix (if “most recent” is the contract)
- # History returns oldest entries first (ascending order) - # The reason field contains the key info (e.g., "Custom setting 'key_0' updated") - expected_keys = ["key_0", "key_1", "key_2"] + # History returns most recent entries first (descending order) + # The reason field contains the key info (e.g., "Custom setting 'key_4' updated") + expected_keys = ["key_4", "key_3", "key_2"]backend/tests/e2e/test_admin_events_routes.py (1)
433-495: Allow fast-completing replay statuses to reduce flakiness.A replay can complete quickly, so
COMPLETED/FAILED/CANCELLEDcan appear immediately.✅ Suggested change
- assert status.status in (ReplayStatus.SCHEDULED, ReplayStatus.RUNNING) + assert status.status in { + ReplayStatus.SCHEDULED, + ReplayStatus.RUNNING, + ReplayStatus.COMPLETED, + ReplayStatus.FAILED, + ReplayStatus.CANCELLED, + }
🧹 Nitpick comments (4)
backend/tests/e2e/core/test_exception_handlers.py (2)
9-14: Consolidate duplicate handler-registration assertion.This same check already exists in
backend/tests/e2e/app/test_main_app.py. Consider keeping it in one place to avoid redundant E2E runtime.
72-93: Prefer a single, contract-stable status (or assert an error code).Allowing both 400 and 409 makes it harder to detect a regression in duplicate-registration handling. If both are truly acceptable, assert a duplicate-specific error code/message.
backend/app/db/repositories/user_settings_repository.py (1)
30-48: Consider decoupling from Beanie's SortDirection in the public API.The repository method exposes
beanie.odm.enums.SortDirectionin its signature, coupling callers to the persistence layer. A domain-levelSortDirectionexists inapp.domain.events.event_modelsbut is currently unused. While this pattern is consistent across multiple repositories in the codebase, consider accepting the domain enum and mapping internally to maintain cleaner API boundaries.♻️ Proposed refactor
-from beanie.odm.enums import SortDirection +from beanie.odm.enums import SortDirection as BeanieSortDirection +from app.domain.events.event_models import SortDirection @@ - async def get_settings_events( + async def get_settings_events( self, user_id: str, event_types: list[EventType], since: datetime | None = None, until: datetime | None = None, limit: int | None = None, - sort_order: SortDirection = SortDirection.ASCENDING, + sort_order: SortDirection = SortDirection.ASCENDING, ) -> list[DomainUserSettingsChangedEvent]: @@ - find_query = EventDocument.find(*conditions).sort([("timestamp", sort_order)]) + beanie_sort = ( + BeanieSortDirection.DESCENDING + if sort_order == SortDirection.DESCENDING + else BeanieSortDirection.ASCENDING + ) + find_query = EventDocument.find(*conditions).sort([("timestamp", beanie_sort)])Note: Current call sites do not explicitly pass
sort_order, so this refactoring has no impact on existing callers.backend/tests/e2e/test_execution_routes.py (1)
51-90: Preferget_running_loop()to avoidget_event_loop()deprecation warnings.This is inside an async context, so use the running loop once and reuse it to keep timing calls consistent.
♻️ Suggested change
- deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout @@ - while asyncio.get_event_loop().time() < deadline: + while loop.time() < deadline:
There was a problem hiding this comment.
1 issue found across 8 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/app/services/notification_service.py">
<violation number="1" location="backend/app/services/notification_service.py:736">
P2: Total count no longer respects the status filter, so pagination totals will be incorrect for filtered lists. Pass the status condition when provided to keep total aligned with list_notifications.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 5 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_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_events_routes.py:406">
P2: The list-types test publishes an event using a regular user client, but the publish endpoint is admin-only. This call will be rejected (403), so the test may not create any event and can fail or rely on existing state. Use an admin client or assert the failure instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/services/coordinator/queue_manager.py`:
- Around line 208-212: The _get_queue_position method now returns 0-based
indexes; add unit tests that create multiple queued items, call
queue_manager._get_queue_position for each queued.execution_id and assert the
first item returns 0, second returns 1, etc., to lock in behavior; also review
places that surface the position (e.g., where ExecutionAcceptedEvent is
constructed or coordinator logic that checks position == 0) and update tests or
user-facing messages to either convert to 1-based for UX or document/log that
positions are 0-based so consumers are consistent.
♻️ Duplicate comments (1)
backend/tests/e2e/test_events_routes.py (1)
398-413: Use admin client (or assert failure) when seeding event types.
Line 401 usestest_userto call/api/v1/events/publish, but Line 350-366 asserts this route is admin-only. That call will likely 403 and is currently ignored, so the test can pass without seeding data. Consider usingtest_adminfor the publish step and assert success.🛠️ Proposed fix
- async def test_list_event_types(self, test_user: AsyncClient) -> None: + async def test_list_event_types( + self, test_admin: AsyncClient, test_user: AsyncClient + ) -> None: """List available event types.""" # First create an event so there's at least one type request = PublishEventRequest( event_type=EventType.SCRIPT_SAVED, payload={"script_id": "test-script", "name": "test_list_types"}, ) - await test_user.post("/api/v1/events/publish", json=request.model_dump()) + publish_response = await test_admin.post( + "/api/v1/events/publish", json=request.model_dump() + ) + assert publish_response.status_code == 200 response = await test_user.get("/api/v1/events/types/list")
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_events_routes.py`:
- Around line 526-541: The test test_user_events_only_shows_own_events currently
allows a vacuous pass when another_user has no events; update the test to
explicitly require that another_result.events is non-empty (e.g., assert
another_event_ids) or skip the disjointness check when empty, then perform the
disjoint assertion between user_event_ids and another_event_ids to validate
isolation; reference the variables another_event_ids and user_event_ids in the
change.
- Around line 403-409: The test currently posts a PublishEventRequest (using
PublishEventRequest and EventType.SCRIPT_SAVED) but doesn't assert the publish
succeeded; capture the response from test_admin.post("/api/v1/events/publish"),
assert the HTTP status is success (e.g., 200/201), and validate the response
body contains the expected event data (e.g., matching event_type and
payload/script_id) before continuing to list types so the test cannot pass due
to pre-existing data.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/tests/e2e/test_execution_routes.py`:
- Around line 541-543: Update the idempotency key used in the test to a
non-secret, clearly test-specific value to avoid secret-scanner hits: replace
the current headers["Idempotency-Key"] value assigned near the ExecutionRequest
instantiation (the variable named headers and the HTTP header "Idempotency-Key")
with a benign test token such as a descriptive test string (e.g.,
"test-idempotency-key-...") or generate a UUID-like value so it cannot be
mistaken for a real API key by secret scanners.
♻️ Duplicate comments (2)
backend/tests/e2e/test_events_routes.py (2)
401-411: Assert the admin publish succeeded before listing types.The publish response is not being validated. If the publish fails, this test can still pass due to pre-existing data, masking potential regressions.
🔧 Proposed fix
request = PublishEventRequest( event_type=EventType.SCRIPT_SAVED, payload={ "script_id": "test-script", "user_id": "test-user", "title": "Test Script", "language": "python", }, ) - await test_admin.post("/api/v1/events/publish", json=request.model_dump()) + publish_response = await test_admin.post( + "/api/v1/events/publish", json=request.model_dump() + ) + assert publish_response.status_code == 200
540-546: Avoid a vacuous pass whenanother_userhas no events.If
another_userhas no events,another_event_idswill be an empty set, andisdisjointwill always returnTrueregardless of what's inuser_event_ids. This means the isolation check isn't actually validated.🔧 Proposed fix
another_result = EventListResponse.model_validate(another_response.json()) + # If another_user has no events, we can't meaningfully verify isolation + if not another_result.events: + pytest.skip("No events for another_user; cannot verify isolation") another_event_ids = {e.event_id for e in another_result.events} assert user_event_ids.isdisjoint(another_event_ids)Alternatively, create events for
another_uservia a fixture to ensure deterministic test conditions.
🧹 Nitpick comments (3)
backend/tests/e2e/test_events_routes.py (1)
42-44: Consider usingasyncio.get_running_loop()instead of deprecatedget_event_loop().
asyncio.get_event_loop()is deprecated when called from within a running coroutine (Python 3.10+). Since these helpers areasyncfunctions, useasyncio.get_running_loop().time()for forward compatibility.♻️ Proposed fix
async def wait_for_user_events( client: AsyncClient, timeout: float = 30.0, poll_interval: float = 0.5, ) -> EventListResponse: - deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout - while asyncio.get_event_loop().time() < deadline: + while loop.time() < deadline: response = await client.get("/api/v1/events/user", params={"limit": 10})Apply similar change to
wait_for_aggregate_events.Also applies to: 78-80
backend/tests/e2e/test_execution_routes.py (2)
51-90: Preferasyncio.get_running_loop()and reuse the loop for timing.This avoids deprecated patterns and repeated loop lookups in async context.
♻️ Proposed refactor
- deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout - while asyncio.get_event_loop().time() < deadline: + while loop.time() < deadline:
400-408: SerializeEventTypeexplicitly in query params.Passing Enums directly can serialize to
EventType.Xinstead of the intended value depending on Enum base/httpxbehavior.♻️ Proposed refactor
- events_response = await test_user.get( + events_response = await test_user.get( f"/api/v1/executions/{exec_response.execution_id}/events", - params={"event_types": [EventType.EXECUTION_REQUESTED]}, + params={"event_types": [EventType.EXECUTION_REQUESTED.value]}, )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/services/notification_service.py`:
- Around line 154-163: The background loops exit immediately because
self.is_running (set by LifecycleEnabled.__aenter__) isn't true yet when
_start_background_tasks() spawns tasks inside _on_start; modify the service to
set an internal running flag or event (e.g., _internal_running or
_running_event) to True/set before calling _start_background_tasks(), have the
background loops check that internal flag/event instead of self.is_running, and
ensure _on_stop clears the internal flag/event so loops terminate correctly;
reference _on_start, _start_background_tasks, LifecycleEnabled.__aenter__,
is_running, and _on_stop when making the change.
There was a problem hiding this comment.
1 issue found across 7 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/app/services/saga/saga_orchestrator.py">
<violation number="1" location="backend/app/services/saga/saga_orchestrator.py:344">
P2: Execution sagas now wait for completion events, but the orchestrator only subscribes/handles trigger events, so completion events are dropped and execution sagas never complete. Register completion event types (or include them in trigger events) so `_handle_completion_event` can run.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/db/repositories/event_repository.py`:
- Around line 361-379: The aggregation $match uses Beanie field descriptors
(EventDocument.aggregate_id, EventDocument.execution_id) which serialize
incorrectly; update the Pipeline().match call to use explicit string field names
like "aggregate_id" and "execution_id" (i.e., .match({"$or": [{"aggregate_id":
aggregate_id}, {"execution_id": aggregate_id}]}) ) so the pipeline passed to
EventDocument.aggregate(pipeline.export()) correctly matches documents.
In `@backend/app/services/saga/saga_orchestrator.py`:
- Around line 222-234: The code currently treats non-EXECUTION_COMPLETED events
as FAILED; update the logic in the saga state update block to treat
EventType.EXECUTION_TIMEOUT specially: if event.event_type ==
EventType.EXECUTION_TIMEOUT set saga.state = SagaState.TIMEOUT (and set
saga.completed_at and any timeout-specific fields) otherwise keep the existing
FAILED branch (set saga.state = SagaState.FAILED and saga.error_message). Locate
the conditional around event.event_type in the method that updates saga state
(references: event.event_type, EventType.EXECUTION_COMPLETED,
EventType.EXECUTION_TIMEOUT, saga.state, SagaState.TIMEOUT, SagaState.FAILED,
saga.error_message, saga.completed_at) and add an explicit branch for the
EXECUTION_TIMEOUT case.
- Around line 211-216: The code currently calls
self._repo.get_saga_by_execution_id(execution_id) which returns the first saga
for that execution and can pick a non-execution saga; update the lookup to
explicitly fetch the execution saga only (e.g., replace that call with a repo
method that scopes by saga type or role such as
get_execution_saga_by_execution_id or get_saga_by_execution_id(execution_id,
saga_type="execution")), and update the repository implementation used by
SagaOrchestrator accordingly so the variable saga is guaranteed to be the
execution saga before proceeding.
🧹 Nitpick comments (2)
frontend/e2e/notifications.spec.ts (1)
58-70: Consider usingtoPass()for more resilient async assertions.The current pattern of
if (await locator.isVisible().catch(() => false)) { ... }skips assertions when elements aren't found, making tests pass vacuously. If notifications exist, the assertions should run reliably.♻️ Alternative using `toPass()` for retry logic
test('notification cards show severity badges when present', async ({ userPage }) => { await gotoAndWaitForNotifications(userPage); const notificationCard = userPage.locator('[aria-label="Mark notification as read"]').first(); // Skip test if no notifications exist if (!await notificationCard.isVisible({ timeout: 3000 }).catch(() => false)) { test.skip(); return; } // Assert severity badge is visible with retry const severityBadge = notificationCard.locator('span').filter({ hasText: /^(low|medium|high|urgent)$/i }).first(); await expect(async () => { await expect(severityBadge).toBeVisible(); await expect(severityBadge).toContainText(/low|medium|high|urgent/i); }).toPass({ timeout: 2000 }); });Also applies to: 72-83
backend/app/services/saga/saga_orchestrator.py (1)
354-359: UseExecutionSaga.get_name()instead of a string literal.Avoids drift if the saga name changes.
♻️ Proposed refactor
- if instance.saga_name == "execution_saga": + if instance.saga_name == ExecutionSaga.get_name(): self.logger.info(f"Saga {instance.saga_id} steps done, waiting for execution completion event") else: await self._complete_saga(instance)
|
Summary by cubic
Adds a comprehensive backend E2E test suite across core APIs, app lifecycle/DI, and Kafka flows, standardizes event types to EventType, and improves admin user rate-limit management. Also fixes DLQ event_type filtering, tightens execution event access, returns not-found for missing saved scripts, returns structured counts on admin delete user, and makes ResourceCleaner DI-backed with E2E coverage.
New Features
Refactors
Written for commit 3e5eca8. Summary will update on new commits.
Summary by CodeRabbit
Tests
Refactor
Bug Fixes
API Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.