Conversation
There was a problem hiding this comment.
14 issues found across 57 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="drift/instrumentation/django/e2e-tests/requirements.txt">
<violation number="1" location="drift/instrumentation/django/e2e-tests/requirements.txt:3">
P2: Version mismatch: project requires `requests>=2.32.5` but this file specifies `>=2.32.0`. E2E tests should use consistent dependency versions with the main project to ensure realistic testing.</violation>
</file>
<file name="drift/instrumentation/psycopg2/e2e-tests/.tusk/config.yaml">
<violation number="1" location="drift/instrumentation/psycopg2/e2e-tests/.tusk/config.yaml:10">
P2: Removing `-f` flag from curl makes the readiness check unreliable. Without `-f` (--fail), curl returns exit code 0 even on HTTP 4xx/5xx errors, so the check may pass when the service is unhealthy. Consider keeping at least `-f` to fail on HTTP errors.</violation>
</file>
<file name="drift/instrumentation/django/e2e-tests/run.sh">
<violation number="1" location="drift/instrumentation/django/e2e-tests/run.sh:47">
P1: With `set -e` enabled, if `docker compose run` fails (non-zero exit), the script exits immediately before capturing the exit code. The pass/fail message block (lines 52-60) will never execute for failed tests. Disable `set -e` before this command to allow proper exit code handling.</violation>
</file>
<file name="drift/instrumentation/e2e_common/base_runner.py">
<violation number="1" location="drift/instrumentation/e2e_common/base_runner.py:252">
P1: Bug in JSON parsing: `raw_decode()` returns an absolute index, not a relative offset. Using `idx += end_idx` will cause the parser to skip past valid JSON objects when multiple test results are in the output.</violation>
</file>
<file name="drift/instrumentation/django/e2e-tests/src/test_requests.py">
<violation number="1" location="drift/instrumentation/django/e2e-tests/src/test_requests.py:17">
P2: Missing timeout on `requests.request()` call. Without a timeout, the test can hang indefinitely if the server is unresponsive, potentially blocking CI pipelines. Consider adding a reasonable timeout (e.g., `timeout=30`).</violation>
</file>
<file name="drift/instrumentation/fastapi/e2e-tests/run.sh">
<violation number="1" location="drift/instrumentation/fastapi/e2e-tests/run.sh:47">
P2: With `set -e` enabled, if the docker compose command fails, the script exits immediately before `EXIT_CODE=$?` can capture the exit code. The "Test failed" message will never be displayed. Either disable `set -e` temporarily or capture the exit code inline.</violation>
</file>
<file name="drift/instrumentation/psycopg/e2e-tests/.tusk/config.yaml">
<violation number="1" location="drift/instrumentation/psycopg/e2e-tests/.tusk/config.yaml:10">
P1: Removing `-f` flag from curl makes the readiness check unreliable. Without `-f`, curl returns exit code 0 even when the health endpoint returns HTTP errors (4xx/5xx), causing the check to pass incorrectly. Consider keeping at least `-f` to ensure the check fails on HTTP errors.</violation>
</file>
<file name="drift/instrumentation/fastapi/e2e-tests/requirements.txt">
<violation number="1" location="drift/instrumentation/fastapi/e2e-tests/requirements.txt:4">
P2: Version mismatch: `requests>=2.32.0` is lower than the project's minimum `requests>=2.32.5`. This could cause e2e tests to pass with an unsupported version.</violation>
</file>
<file name="drift/instrumentation/fastapi/e2e-tests/src/test_requests.py">
<violation number="1" location="drift/instrumentation/fastapi/e2e-tests/src/test_requests.py:39">
P2: The success message is misleading since the code doesn't verify HTTP status codes. A 4xx or 5xx response would still print "completed successfully". Consider checking `response.raise_for_status()` in `make_request` or validating responses before claiming success.</violation>
</file>
<file name="drift/instrumentation/redis/e2e-tests/.tusk/config.yaml">
<violation number="1" location="drift/instrumentation/redis/e2e-tests/.tusk/config.yaml:10">
P1: Missing `-f` flag in curl command will cause readiness check to pass even when the health endpoint returns HTTP errors. The `-f` (--fail) flag ensures curl returns a non-zero exit code on HTTP 4xx/5xx responses, which is essential for accurate health checking.</violation>
</file>
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:131">
P2: `python -m json.tool` does not handle JSONL (JSON Lines) format. It expects a single JSON document, but JSONL files contain one JSON object per line. This command will fail for typical trace files. Use `jq` instead (as shown in README-e2e-tests.md) which natively handles JSONL.</violation>
</file>
<file name="drift/instrumentation/flask/e2e-tests/.tusk/config.yaml">
<violation number="1" location="drift/instrumentation/flask/e2e-tests/.tusk/config.yaml:10">
P1: Removing the `-f` flag from curl in the readiness check is problematic. Without `-f`, curl returns exit code 0 even when the server returns HTTP error status codes (4xx, 5xx), which could cause the readiness check to pass when the service is actually unhealthy. Consider keeping at least the `-f` flag: `curl -f http://localhost:8000/health`</violation>
</file>
<file name="drift/instrumentation/e2e_common/Dockerfile.base">
<violation number="1" location="drift/instrumentation/e2e_common/Dockerfile.base:32">
P2: Architecture is hardcoded to `x86_64`, which will fail on ARM64 systems. Consider detecting the architecture dynamically to support both x86_64 and ARM64 platforms.</violation>
</file>
<file name="drift/instrumentation/psycopg/e2e-tests/Dockerfile">
<violation number="1" location="drift/instrumentation/psycopg/e2e-tests/Dockerfile:4">
P2: Without a `.dockerignore` file, `COPY . /sdk` will copy the entire repository including `.git`, documentation, and other unnecessary files into the Docker image. Consider adding a `.dockerignore` file at the repository root to exclude `.git`, `*.md`, `__pycache__`, `.pytest_cache`, and other non-essential files to reduce image size and build time.</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 4 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="drift/instrumentation/fastapi/e2e-tests/src/test_requests.py">
<violation number="1" location="drift/instrumentation/fastapi/e2e-tests/src/test_requests.py:59">
P1: Potential TypeError if `parentSpanId` is `null` in JSON. Using `span.get('parentSpanId', 'none')` only provides the default when the key is missing, not when the value is `None`. Use `or` to handle both cases.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
Build a unified Python e2e test framework with a shared runner and base image, add FastAPI and Django coverage, and standardize existing tests for reliable record/replay with Tusk. Improves reliability and makes running all tests simple via a new run-all script and updated docs.
New Features
Refactors
Todo (after this PR)