feat: fixed fixtures in backend/e2e - #237
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughReplaces pytest-asyncio fixtures in E2E tests with imperative async helper functions in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (helper)
participant Client as AsyncClient (API)
participant Redis as Redis
participant SagaAPI as Saga service / API
Test->>Client: POST /api/v1/execute (create_execution)
Client-->>Test: ExecutionResponse (execution_id)
Test->>Redis: subscribe/wait for POD_CREATED or notification (by execution_id)
Redis-->>Test: event (POD_CREATED / notification)
Test->>SagaAPI: GET /api/v1/sagas/{execution_id} (create_execution_with_saga)
SagaAPI-->>Test: SagaStatusResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates backend end-to-end tests to stop relying on async fixture chaining for “created execution / execution with saga / execution with notification”, replacing them with explicit async helper functions to make test setup more deterministic and easier to control.
Changes:
- Replace
created_execution,execution_with_saga, andexecution_with_notificationfixtures withcreate_execution*async helper functions intests/e2e/conftest.py. - Update SSE, saga, and notification E2E tests to call the new helpers directly and pass required clients/Redis.
- Simplify and remove now-unused imports/types in the updated tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/tests/e2e/test_sse_routes.py | Switch execution stream tests to create executions via create_execution() instead of created_execution fixture. |
| backend/tests/e2e/test_saga_routes.py | Replace execution_with_saga fixture usage with create_execution_with_saga() helper across saga route tests. |
| backend/tests/e2e/test_notifications_routes.py | Replace execution_with_notification fixture usage with create_execution_with_notification() helper and clean up imports. |
| backend/tests/e2e/conftest.py | Remove async fixtures and introduce create_execution* helper functions for explicit, reusable test setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/test_notifications_routes.py (1)
360-397:⚠️ Potential issue | 🟡 MinorStrengthen 404 isolation checks with response body assertions.
Lines 374 and 396 assert only status code. Please also validate the error payload to verify the API contract for unauthorized resource access.
Based on learnings: "Applies to backend/tests/**/*.py : Backend tests must test API responses precisely including status code and response body contents; use pytest.mark.parametrize over duplicate test bodies; mock at service boundaries using unittest.mock.AsyncMock".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/test_notifications_routes.py` around lines 360 - 397, Update the two tests (test_cannot_mark_other_users_notification_read and test_cannot_delete_other_users_notification) to also assert the JSON error payload from the 404 responses so the API contract for unauthorized resource access is validated (e.g., assert response.json() matches the expected error shape/message). Refactor the duplicate logic by parameterizing the HTTP method and endpoint assertion (use pytest.mark.parametrize over method/route pairs) and reuse create_execution_with_notification, test_user and another_user fixtures; ensure the owner-success path still asserts the 204 for the PUT case.backend/tests/e2e/test_saga_routes.py (1)
47-58:⚠️ Potential issue | 🟡 MinorValidate 403 payloads in access-control tests.
Line 58 and Line 246 assert only status code. Add response body checks to enforce the exact error contract for forbidden access.
Based on learnings: "Applies to backend/tests/**/*.py : Backend tests must test API responses precisely including status code and response body contents; use pytest.mark.parametrize over duplicate test bodies; mock at service boundaries using unittest.mock.AsyncMock".
Also applies to: 232-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/test_saga_routes.py` around lines 47 - 58, The test test_get_saga_access_denied currently only asserts response.status_code == 403; update it to also assert the exact JSON error body contract (e.g., response.json() == {"detail": "Forbidden", "code": "forbidden"} or whatever your API uses) so the forbidden response payload is verified after calling create_execution_with_saga and another_user.get; apply the same change to the other access-control test that currently only checks status (the duplicate test around lines 232-247) and consolidate duplicate assertions using pytest.mark.parametrize to avoid copy/paste.
🧹 Nitpick comments (2)
backend/tests/e2e/conftest.py (2)
157-171:create_executiontakesredis_clientbut never uses it.This parameter currently adds coupling without behavior. Consider removing it from
create_executionand keeping Redis only in helpers that actually wait on Redis events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 157 - 171, The create_execution helper currently accepts an unused redis_client parameter; remove the unused parameter from the function signature (create_execution) and from every test/caller that passes it, leaving the function to accept only client, request, script, lang, lang_version and return ExecutionResponse; keep Redis usage confined to helpers that explicitly wait for Redis events (do not reintroduce redis_client into create_execution), and update any type hints or imports referencing redis_client accordingly (e.g., callers that relied on redis_client should instead call the Redis-wait helper where needed).
166-167: Use Google-style docstrings for the new helper functions.The added helper docstrings don’t include
Args/Returns(andRaiseswhere relevant), which is required by the repo guidelines.As per coding guidelines: "Use Google-style docstrings with Args/Returns/Raises sections for all functions and classes".
Also applies to: 183-187, 212-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 166 - 167, Update the new helper function docstrings in backend/tests/e2e/conftest.py (the helper that "POST /execute and return response (does NOT wait for completion)" and the two other helpers referenced at the other ranges) to use Google-style format: add an Args section documenting parameters, a Returns section describing the return value, and a Raises section where appropriate; ensure each section is concise and matches the function signature and behavior so tests and linters accept the docstrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/e2e/test_sse_routes.py`:
- Around line 167-181: Update the test_execution_stream_other_users_execution
test to assert the error response body in addition to status code: after calling
sse_client_another.get(f"/api/v1/events/executions/{execution.execution_id}"),
parse the response body (e.g., response.json()) and assert it matches the
expected error payload shape/content (status/message/code or whatever API error
schema your handlers return). Reference the test function name
test_execution_stream_other_users_execution and the sse_client_another.get call
to locate where to add assertions and ensure the error fields are explicitly
checked rather than only response.status_code.
---
Outside diff comments:
In `@backend/tests/e2e/test_notifications_routes.py`:
- Around line 360-397: Update the two tests
(test_cannot_mark_other_users_notification_read and
test_cannot_delete_other_users_notification) to also assert the JSON error
payload from the 404 responses so the API contract for unauthorized resource
access is validated (e.g., assert response.json() matches the expected error
shape/message). Refactor the duplicate logic by parameterizing the HTTP method
and endpoint assertion (use pytest.mark.parametrize over method/route pairs) and
reuse create_execution_with_notification, test_user and another_user fixtures;
ensure the owner-success path still asserts the 204 for the PUT case.
In `@backend/tests/e2e/test_saga_routes.py`:
- Around line 47-58: The test test_get_saga_access_denied currently only asserts
response.status_code == 403; update it to also assert the exact JSON error body
contract (e.g., response.json() == {"detail": "Forbidden", "code": "forbidden"}
or whatever your API uses) so the forbidden response payload is verified after
calling create_execution_with_saga and another_user.get; apply the same change
to the other access-control test that currently only checks status (the
duplicate test around lines 232-247) and consolidate duplicate assertions using
pytest.mark.parametrize to avoid copy/paste.
---
Nitpick comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 157-171: The create_execution helper currently accepts an unused
redis_client parameter; remove the unused parameter from the function signature
(create_execution) and from every test/caller that passes it, leaving the
function to accept only client, request, script, lang, lang_version and return
ExecutionResponse; keep Redis usage confined to helpers that explicitly wait for
Redis events (do not reintroduce redis_client into create_execution), and update
any type hints or imports referencing redis_client accordingly (e.g., callers
that relied on redis_client should instead call the Redis-wait helper where
needed).
- Around line 166-167: Update the new helper function docstrings in
backend/tests/e2e/conftest.py (the helper that "POST /execute and return
response (does NOT wait for completion)" and the two other helpers referenced at
the other ranges) to use Google-style format: add an Args section documenting
parameters, a Returns section describing the return value, and a Raises section
where appropriate; ensure each section is concise and matches the function
signature and behavior so tests and linters accept the docstrings.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/e2e/conftest.pybackend/tests/e2e/test_notifications_routes.pybackend/tests/e2e/test_saga_routes.pybackend/tests/e2e/test_sse_routes.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/e2e/conftest.py">
<violation number="1" location="backend/tests/e2e/conftest.py:159">
P3: `create_execution` has an unused `redis_client` parameter, which adds dead API surface and unnecessary coupling in test helpers.</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
🧹 Nitpick comments (1)
backend/tests/e2e/conftest.py (1)
164-204: Upgrade helper docstrings to Google style sections.The new helper docstrings are clear, but they don’t include explicit
Args/Returns/Raisessections.As per coding guidelines, "Use Google-style docstrings with Args/Returns/Raises sections for all functions and classes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 164 - 204, Update the triple-quoted docstrings for create_execution_with_saga, create_execution_with_notification (and the helper create_execution if applicable) to Google-style format: add an "Args:" section listing parameters (client: AsyncClient, redis_client: redis.Redis, request: ExecutionRequest, and optional timeout: float) with types and brief descriptions, a "Returns:" section describing the returned tuple types (e.g., tuple[ExecutionResponse, SagaStatusResponse] or tuple[ExecutionResponse, NotificationResponse]), and a "Raises:" section enumerating possible exceptions (e.g., AssertionError when saga/notification not found, timeout-related errors). Keep the existing brief summary lines, then append these Args/Returns/Raises sections in each function docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 209-216: The test currently returns result.notifications[0] which
can be a stale item; update the helper to select the notification that
corresponds to the newly created execution instead of the first item. After
fetching NotificationListResponse.model_validate(resp.json()), filter
result.notifications for the one that matches the created execution (e.g.,
compare notification.execution_id or notification.resource_id/subject_id to
execution.id), and if that field is not available, fall back to choosing the
most-recent notification by sorting result.notifications by created_at (or
equivalent timestamp) and returning the latest; keep the helper functions
create_execution, wait_for_notification and variables
execution/result.notifications unchanged otherwise.
- Around line 162-163: The three test helpers create_execution,
create_execution_with_saga, and create_execution_with_notification currently use
the shared mutable _DEFAULT_REQUEST as a default argument; change each signature
to accept request: ExecutionRequest | None = None and at the start of each
function add request = request or _DEFAULT_REQUEST so a fresh reference is used
per call, avoiding state leakage from mutating the shared _DEFAULT_REQUEST.
---
Nitpick comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 164-204: Update the triple-quoted docstrings for
create_execution_with_saga, create_execution_with_notification (and the helper
create_execution if applicable) to Google-style format: add an "Args:" section
listing parameters (client: AsyncClient, redis_client: redis.Redis, request:
ExecutionRequest, and optional timeout: float) with types and brief
descriptions, a "Returns:" section describing the returned tuple types (e.g.,
tuple[ExecutionResponse, SagaStatusResponse] or tuple[ExecutionResponse,
NotificationResponse]), and a "Raises:" section enumerating possible exceptions
(e.g., AssertionError when saga/notification not found, timeout-related errors).
Keep the existing brief summary lines, then append these Args/Returns/Raises
sections in each function docstring.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Refactored e2e tests to replace fixtures with async helpers for creating executions, sagas, and notifications, making setup deterministic and faster. Added a simple exec_request fixture and tightened readiness waits to reduce flakiness.
Refactors
Bug Fixes
Written for commit f0f63d0. Summary will update on new commits.
Summary by CodeRabbit