test: add end-to-end testing infrastructure #28
Conversation
📝 WalkthroughWalkthroughAdds end-to-end test support and CI wiring: new GitHub Actions Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GitHubActions as GitHub Actions
participant Repo as Repository
participant Mise as Mise (uv)
participant ImmichRepo as Immich Git Repo
participant Docker as Docker Compose
participant Pytest as pytest (e2e)
Developer ->> GitHubActions: push / PR (or label run-e2e)
GitHubActions ->> Repo: checkout repository
GitHubActions ->> Mise: install tooling (mise + --env ci), read IMMICH-VERSION
Mise ->> ImmichRepo: clone specified Immich version
Mise ->> Docker: docker-compose up (start Immich e2e environment)
GitHubActions ->> Docker: poll container logs until server ready
GitHubActions ->> Pytest: run `mise run ci:test:e2e` against IMMICH_URL/IMMICH_API_URL
alt tests fail
Docker ->> GitHubActions: collect & print container logs
end
GitHubActions ->> Docker: docker-compose down (always)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Comment |
coderabbitai
bot
changed the title
![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
🤖 Fix all issues with AI agents
In @tests/e2e/client/test_wrappers.py:
- Around line 85-87: The unit test job is running E2E tests (e.g.,
test_assets_upload marked with @pytest.mark.e2e) because mise.toml's test:unit
task runs "uv run pytest tests/" without excluding e2e; update mise.toml to
change the test:unit pytest command to exclude e2e-marked tests (for example add
"-m 'not e2e'" or equivalent pytest args) and likewise update the coverage task
to run pytest with the same exclusion so coverage only covers unit tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/cd.ymlbin/generate.pymise.tomlpyproject.tomltests/e2e/client/__init__.pytests/e2e/client/test_wrappers.pytests/unit/test_async_client.pytests/unit/test_async_custom_session.pytests/unit/test_download.pytests/unit/test_upload.py
💤 Files with no reviewable changes (1)
- bin/generate.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T00:43:43.563Z
Learnt from: CR
Repo: timonrieger/immich-python-client PR: 0
File: .cursor/rules/dev-tools.mdc:0-0
Timestamp: 2026-01-07T00:43:43.563Z
Learning: Use the mise tasks to run commands on the project. See the mise.toml file for the available tasks.
Applied to files:
mise.toml
🧬 Code graph analysis (1)
tests/e2e/client/test_wrappers.py (4)
immich/sdk.py (1)
AsyncClient(72-365)immich/client_wrapper/assets_api_wrapped.py (1)
upload(167-260)immich/client_wrapper/download_api_wrapped.py (1)
download_archive_to_file(17-110)immich/client_wrapper/users_api_wrapped.py (1)
get_profile_image_to_file(12-51)
🪛 GitHub Actions: Continuous Delivery
tests/e2e/client/test_wrappers.py
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_assets_upload setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_assets_download_asset_to_file setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_assets_view_asset_to_file setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_assets_play_asset_video_to_file setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_download_archive_to_file setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
[error] 38-38: Cannot connect to API at 127.0.0.1:2285 during test_users_get_profile_image_to_file setup (sign_up_admin). The API server may not be running or is unreachable. Command: uv run pytest tests/.
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
29-29: Unused function argument: tmp_path
(ARG001)
39-39: Possible hardcoded password assigned to argument: "password"
(S106)
44-44: Possible hardcoded password assigned to argument: "password"
(S106)
87-87: Unused function argument: tmp_path
(ARG001)
🔇 Additional comments (7)
pyproject.toml (1)
87-90: LGTM!The pytest configuration properly enables async mode and defines the
e2emarker with a helpful deselection hint. This aligns well with the new e2e test suite structure.mise.toml (1)
21-24: LGTM!The new e2e tasks follow the established patterns (
test:unit/ci:test:unit) and correctly chain the install step before running tests in CI. Based on learnings, this aligns with the project's task-based workflow.Also applies to: 87-92
tests/e2e/client/test_wrappers.py (3)
28-66: Well-structured fixture with proper resource cleanup.The fixture correctly handles client lifecycle with
try/finallyensuringsetup_clientis closed even on exceptions, and the yieldedclientis closed after test completion. The hardcoded credentials are acceptable here since they're used against an ephemeral Docker-based test server.
103-124: LGTM!The test correctly verifies the download functionality by comparing the downloaded bytes against the original test image. Good use of the arrange-act-assert pattern.
173-197: LGTM!Good coverage of the archive download functionality. The test properly uses
DownloadInfoDtoto request an archive and validates both existence and file extension..github/workflows/cd.yml (2)
49-108: Well-structured e2e job with proper setup/teardown.Good practices observed:
- Timeout-based readiness polling with informative progress output
- Docker logs printed on failure for debugging
- Unconditional teardown with
if: always()ensures cleanup regardless of test outcome
110-112: E2E tests not gating the release.The
releasejob depends only on[lint, test]but note2e-tests. If e2e tests should be required before releasing, add the dependency:♻️ Optional: Add e2e-tests as release prerequisite
release: runs-on: ubuntu-latest - needs: [lint, test] + needs: [lint, test, e2e-tests] if: github.ref == 'refs/heads/main' && github.event_name == 'push'If e2e tests are intentionally informational-only (non-blocking), this is fine as-is—please confirm the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
65-73: Consider adding error handling for missing IMMICH-VERSION file.If the
IMMICH-VERSIONfile doesn't exist or is empty, the workflow will fail with an unclear error during the git clone step. Adding validation would improve debuggability.🛠️ Suggested improvement
- name: Read Immich version id: immich-version - run: echo "version=$(cat IMMICH-VERSION | tr -d '[:space:]')" >> $GITHUB_OUTPUT + run: | + if [ ! -f IMMICH-VERSION ]; then + echo "::error::IMMICH-VERSION file not found" + exit 1 + fi + version=$(cat IMMICH-VERSION | tr -d '[:space:]') + if [ -z "$version" ]; then + echo "::error::IMMICH-VERSION file is empty" + exit 1 + fi + echo "version=$version" >> $GITHUB_OUTPUT
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cd.ymlmise.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T00:43:43.563Z
Learnt from: CR
Repo: timonrieger/immich-python-client PR: 0
File: .cursor/rules/dev-tools.mdc:0-0
Timestamp: 2026-01-07T00:43:43.563Z
Learning: Use the mise tasks to run commands on the project. See the mise.toml file for the available tasks.
Applied to files:
mise.toml
🔇 Additional comments (7)
.github/workflows/cd.yml (4)
49-63: LGTM on job setup and tooling installation.The job structure with read-only permissions and pinned action versions follows the existing patterns in the workflow.
75-92: Server readiness polling looks solid.The timeout mechanism with incremental sleep and progress logging is well-implemented. Showing docker logs on timeout failure aids debugging.
One minor consideration: the
curl -fonly checks HTTP success, but if the endpoint returns a non-JSON response or unexpected content, tests could still fail unexpectedly. This is likely fine given you control the Immich version being tested.
94-108: Teardown and failure handling are well-structured.Using
if: always()for teardown andif: failure()for logs ensures proper cleanup and debugging capability.
112-112: Verify: e2e-tests job not included in release dependencies.The
releasejob depends on[lint, test]but note2e-tests. This means failing e2e tests won't block releases. Please confirm this is intentional—if e2e failures should gate releases, adde2e-teststo theneedsarray.🔧 If e2e tests should gate releases
release: runs-on: ubuntu-latest - needs: [lint, test] + needs: [lint, test, e2e-tests]mise.toml (3)
17-23: Clean separation of unit and e2e test paths.The updated
test:unitpath and newtest:e2etask follow consistent naming conventions and prevent accidental cross-execution of test suites.
25-27: Coverage task correctly updated to match unit test path.Ensures coverage reporting only runs against unit tests, which is appropriate since e2e tests typically shouldn't contribute to unit coverage metrics.
87-92: CI e2e task follows established patterns.The
ci:test:e2etask mirrors the structure ofci:test:unit, maintaining consistency across the task definitions. Based on learnings, using mise tasks is the standard approach for this project.
…ise rebuild causing github api rate limits
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/cd.yml:
- Around line 114-116: The release job's needs array currently lists only lint
and test, so E2E failures won't block releases; update the release job
definition (the release job's "needs" array) to include the e2e-tests job name
(add "e2e-tests" to the needs list) so the release job depends on successful
completion of E2E tests.
🧹 Nitpick comments (2)
tests/e2e/client/test_wrappers.py (2)
28-68: Remove unused parameter and improve client cleanup.
- The
tmp_pathparameter is unused and should be removed.- If the test raises an exception,
await client.close()(line 66) will be skipped, causing a resource leak. Consider wrapping theclientin its own try/finally block.♻️ Suggested fix
@pytest.fixture -async def client_with_api_key(tmp_path: Path): +async def client_with_api_key(): """Set up admin user, create API key, and return authenticated client.""" base_url = os.environ.get("IMMICH_API_URL", "http://127.0.0.1:2285/api") # Create unauthenticated client for setup setup_client = AsyncClient(base_url=base_url) try: # Sign up admin await setup_client.authentication.sign_up_admin( SignUpDto( email="admin@immich.cloud", name="Immich Admin", password="password" ) ) # Login to get access token login_response = await setup_client.authentication.login( LoginCredentialDto(email="admin@immich.cloud", password="password") ) # Mark admin as onboarded await setup_client.system_metadata.update_admin_onboarding( AdminOnboardingUpdateDto(is_onboarded=True), _headers={"Authorization": f"Bearer {login_response.access_token}"}, ) # Create API key with all permissions api_key_response = await setup_client.api_keys.create_api_key( APIKeyCreateDto(name="e2e", permissions=[Permission.ALL]), _headers={"Authorization": f"Bearer {login_response.access_token}"}, ) # Create authenticated client with API key client = AsyncClient(base_url=base_url, api_key=api_key_response.secret) - yield client - - await client.close() + try: + yield client + finally: + await client.close() finally: await setup_client.close()
87-104: Remove unusedtmp_pathparameter.The
tmp_pathparameter is not used in this test function.♻️ Suggested fix
@pytest.mark.asyncio @pytest.mark.e2e async def test_assets_upload( - client_with_api_key: AsyncClient, test_image: Path, test_video: Path, tmp_path: Path + client_with_api_key: AsyncClient, test_image: Path, test_video: Path ):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cd.ymlbin/generate.pytests/e2e/client/test_wrappers.py
💤 Files with no reviewable changes (1)
- bin/generate.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
29-29: Unused function argument: tmp_path
(ARG001)
40-40: Possible hardcoded password assigned to argument: "password"
(S106)
46-46: Possible hardcoded password assigned to argument: "password"
(S106)
90-90: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (8)
tests/e2e/client/test_wrappers.py (7)
1-25: LGTM!Imports are appropriate for the E2E test module, and the minimal base64-encoded test assets provide self-contained test data without external dependencies.
71-84: LGTM!Clean implementation of test asset fixtures using
tmp_pathfor automatic cleanup.
107-128: LGTM!Good test coverage: uploads an asset, downloads it, and verifies the content matches the original.
131-151: LGTM!Appropriate assertions for thumbnail download—verifying existence without content comparison since thumbnails are server-generated transformations.
154-174: LGTM!Video playback/download test correctly verifies the output file exists.
177-201: LGTM!Archive download test correctly verifies the output is a zip file.
204-225: LGTM!Complete workflow test for profile image upload and download.
.github/workflows/cd.yml (1)
49-112: Well-structured E2E test job.Good implementation with proper health checking, timeout handling, log capture on failure, and guaranteed teardown with
if: always().
…ss check from 120 to 180 seconds.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
timonrieger
added
the
run-e2e
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/client/test_wrappers.py (2)
28-69: Remove unusedtmp_pathparameter and consider more robust client cleanup.The
tmp_pathparameter on line 29 is unused. Additionally, if an exception occurs afterclientis created but beforeyield, the client won't be properly closed.Suggested improvement
@pytest.fixture -async def client_with_api_key(tmp_path: Path): +async def client_with_api_key(): """Set up admin user, create API key, and return authenticated client.""" base_url = os.environ.get("IMMICH_API_URL", "http://127.0.0.1:2285/api") # Create unauthenticated client for setup setup_client = AsyncClient(base_url=base_url) + client = None try: # Sign up admin await setup_client.authentication.sign_up_admin( SignUpDto( email="admin@immich.cloud", name="Immich Admin", password="password" ) ) # Login to get access token login_response = await setup_client.authentication.login( LoginCredentialDto(email="admin@immich.cloud", password="password") ) # Mark admin as onboarded await setup_client.system_metadata.update_admin_onboarding( # NOTE: type ignore likely a ty issue AdminOnboardingUpdateDto(is_onboarded=True), # type: ignore[missing-argument] _headers={"Authorization": f"Bearer {login_response.access_token}"}, ) # Create API key with all permissions api_key_response = await setup_client.api_keys.create_api_key( APIKeyCreateDto(name="e2e", permissions=[Permission.ALL]), _headers={"Authorization": f"Bearer {login_response.access_token}"}, ) # Create authenticated client with API key client = AsyncClient(base_url=base_url, api_key=api_key_response.secret) yield client - - await client.close() finally: + if client is not None: + await client.close() await setup_client.close()
88-106: Remove unusedtmp_pathparameter.The
tmp_pathparameter is not used in this test function.Suggested fix
@pytest.mark.asyncio @pytest.mark.e2e async def test_assets_upload( - client_with_api_key: AsyncClient, test_image: Path, test_video: Path, tmp_path: Path + client_with_api_key: AsyncClient, test_image: Path, test_video: Path ):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cd.ymltests/e2e/client/test_wrappers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/cd.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
29-29: Unused function argument: tmp_path
(ARG001)
40-40: Possible hardcoded password assigned to argument: "password"
(S106)
46-46: Possible hardcoded password assigned to argument: "password"
(S106)
91-91: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (5)
tests/e2e/client/test_wrappers.py (5)
108-129: LGTM!Good test structure with thorough content verification - comparing downloaded bytes against the original ensures download integrity.
132-152: LGTM!Thumbnail tests appropriately verify existence rather than exact content, since the server may process and resize the image.
155-175: LGTM!Video stream test appropriately verifies existence, as the server may transcode the video.
178-202: LGTM!Archive download test validates the expected output correctly.
205-226: LGTM!Profile image test correctly exercises the upload and download workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/cd.yml:
- Line 51: The if condition using github.ref, github.event_name and
contains(...) has ambiguous operator precedence; update the expression to add
explicit parentheses so the intent is clear: group (github.ref ==
'refs/heads/main' && github.event_name == 'push') as one branch OR
(github.event_name == 'pull_request' &&
contains(github.event.pull_request.labels.*.name, 'run-e2e')) as the other
branch, ensuring the label check only applies to pull_request events.
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
80-102: Consider checking if docker-compose process is still alive during polling.If
docker compose upfails early (e.g., image pull error, port conflict), the script will wait the full 60 seconds before timing out. Adding a process check would fail faster:Proposed improvement
docker compose up --build --renew-anon-volumes --force-recreate --remove-orphans > /tmp/docker-compose.log 2>&1 & COMPOSE_PID=$! echo "Waiting for Immich server to be ready..." timeout=60 elapsed=0 while ! grep -q "Immich Microservices is running" /tmp/docker-compose.log 2>/dev/null; do + if ! kill -0 $COMPOSE_PID 2>/dev/null; then + echo "Docker compose process exited unexpectedly" + cat /tmp/docker-compose.log + exit 1 + fi if [ $elapsed -ge $timeout ]; then
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yml
🔇 Additional comments (4)
.github/workflows/cd.yml (4)
55-64: LGTM!Checkout and mise setup are consistent with other jobs in the workflow.
66-74: LGTM!Good use of shallow clone to minimize checkout time. Reading the version from a dedicated file keeps version management clean.
104-118: LGTM!Good practices: conditional log output on failure for debugging, and
if: always()ensures cleanup runs regardless of test outcome. The-vflag properly removes volumes to prevent state leakage between runs.
122-122: Verify behavior whene2e-testsis skipped.When
e2e-testsis skipped (PRs withoutrun-e2elabel), thereleasejob will also be skipped due to theneedsdependency. This appears intentional since releases only happen on main pushes wheree2e-testswill run, but worth confirming this matches expected behavior.
…am e2e setup, improving readiness check and simplifying logs handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/cd.yml:
- Around line 80-87: The readiness check in the "Start Immich server" step is
racy because the command uses docker compose logs -f --no-color --tail=0 | grep
-m 1 "Immich Microservices is running", which misses messages emitted before
logs start; fix by either removing --tail=0 so existing log history is included,
or replace the timeout grep line with a polling loop that checks a stable
readiness endpoint (e.g., curl http://localhost:<port>/health or the Immich
service health path) with retries and sleeps until successful; update the
timeout 60s bash -lc line accordingly to implement the chosen approach.
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
49-54: Consider adding explicit parentheses for clarity.While GitHub Actions expression precedence (
&&>||) makes this evaluate correctly, the intent is clearer with explicit grouping:if: (github.ref == 'refs/heads/main' && github.event_name == 'push') || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'run-e2e'))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (2)
.github/workflows/cd.yml (2)
89-103: Good failure handling and teardown practices.The conditional log output on failure and guaranteed teardown with
if: always()are well implemented.
105-111: Dependency chain looks correct.The
e2e-testsjob'sifcondition covers main pushes, which aligns with the release job's own condition. This ensures e2e tests run before releases while not blocking unrelated PR workflows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
51-51: Add parentheses to clarify operator precedence.The condition works correctly due to
&&having higher precedence than||, but explicit parentheses improve readability and prevent future maintenance errors.🔧 Suggested improvement
- if: github.ref == 'refs/heads/main' && github.event_name == 'push' || github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'run-e2e') + if: (github.ref == 'refs/heads/main' && github.event_name == 'push') || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'run-e2e'))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (5)
.github/workflows/cd.yml (5)
66-74: LGTM!The version reading and shallow clone approach is efficient for CI. The
tr -d '[:space:]'ensures clean version strings.
76-78: LGTM!Token passed via
--password-stdinis the correct approach to avoid exposing credentials in process lists.
89-103: LGTM!Good CI patterns: environment variables for test configuration, conditional logging on failure, and
always()for guaranteed cleanup.
105-111: LGTM!The dependency chain is correct. When
e2e-testsis skipped (PR without label), GitHub Actions treats it as successful for theneedsconstraint, and the release job's ownifcondition correctly prevents it from running on PRs anyway.
80-87: The values used in this workflow (container nameimmich-e2e-server, timeout of 60 seconds, and log message "Immich Microservices is running") are directly from Immich's official e2e setup configuration, not arbitrary hardcoded choices. The upstreamdocker-compose.tsuses the same 60-second timeout and identical log message check, so this implementation faithfully follows Immich's pattern rather than introducing additional brittleness.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/cd.yml (2)
66-68: Consider adding validation for the version file.If
IMMICH-VERSIONis missing or empty, the git clone step will fail with an unclear error. Consider adding validation for earlier failure with a clear message.💡 Optional improvement
- name: Read Immich version id: immich-version - run: echo "version=$(cat IMMICH-VERSION | tr -d '[:space:]')" >> $GITHUB_OUTPUT + run: | + if [ ! -f IMMICH-VERSION ]; then + echo "::error::IMMICH-VERSION file not found" + exit 1 + fi + version=$(cat IMMICH-VERSION | tr -d '[:space:]') + if [ -z "$version" ]; then + echo "::error::IMMICH-VERSION is empty" + exit 1 + fi + echo "version=$version" >> $GITHUB_OUTPUT
80-87: Readiness check relies on exact log message.The wait loop searches for
"Immich Microservices is running"which couples this workflow to Immich's internal log format. If upstream changes the message, this will time out. Consider adding a comment noting this dependency, or use an HTTP health check endpoint if available.The 60-second timeout should be sufficient for container startup when images are pre-pulled, but may be tight if images need to be downloaded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yml
🔇 Additional comments (4)
.github/workflows/cd.yml (4)
49-54: LGTM!The job conditional correctly gates expensive e2e tests to main branch pushes or PRs explicitly labeled with
run-e2e. Permissions are appropriately scoped to read-only.
76-78: LGTM!The GHCR login correctly uses
--password-stdinto avoid exposing the token in the process list, following security best practices.
89-103: LGTM!The e2e test execution, failure logging, and teardown are well-structured:
- Environment variables correctly configure the test target
if: failure()captures logs only when needed for debuggingif: always()ensures proper cleanup regardless of test outcome
105-108: LGTM!The dependency chain correctly ensures e2e tests pass before releasing. The conditional logic aligns well: on main branch pushes, both
e2e-testsandreleaseconditions are satisfied, so releases will be gated by e2e test results.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.github/workflows/cd.yml:
- Around line 49-79: The e2e-tests job overrides permissions to only "contents:
read" which removes GHCR access used by the "Login to GitHub Container Registry"
step; update the job's permissions block in the e2e-tests job to include
"packages: read" alongside "contents: read" so the `${{ github.token }}` can
pull images from ghcr.io (keep the permissions keys under the e2e-tests job and
ensure the Login to GitHub Container Registry step remains unchanged).
- Around line 66-75: Validate and/or provide a fallback for IMMICH_VERSION
before using it in the git clone step: read the output from step immich-version
(steps.immich-version.outputs.version), ensure it's non-empty and corresponds to
an existing ref (e.g., use git ls-remote to check tags/branches for that ref)
and if the ref is missing or invalid, set IMMICH_VERSION to a safe default like
"main" (or normalize a missing "v" prefix to the expected form) so the git clone
--branch $IMMICH_VERSION command cannot fail catastrophically.
In @tests/e2e/client/generators.py:
- Around line 42-72: _create_minimal_mp4 currently base64-decodes the template
on every call and mutates hard-coded offsets [100,200,300,400], which is
fragile; instead decode the base64 once at module import into a MODULE_TEMPLATE
bytes object and in _create_minimal_mp4 copy that template before mutating,
locate a safe payload region by scanning the copied bytes for the b"mdat" box
(or if not found, append a small harmless free/uuid box) and apply seed-based
mutations inside that payload region using seed.to_bytes(...) so offsets are
computed relative to the located payload start rather than fixed positions;
update references to _create_minimal_mp4 and _new_mp4_factory accordingly so
uniqueness is preserved without repeated decoding or brittle fixed offsets.
In @tests/e2e/client/test_wrappers.py:
- Around line 123-149: The test function test_assets_upload has an unused
parameter tmp_path causing a Ruff ARG001; remove tmp_path from the async def
signature so the parameters list becomes (client_with_api_key: AsyncClient,
test_image: Path, test_video: Path, asset_cleanup: dict), and run tests/linter
to ensure no other references to tmp_path exist in this file.
- Around line 25-71: The client_with_api_key fixture uses hardcoded admin
credentials and a brittle error-body check; make the admin email and password
configurable via environment variables (e.g., read ADMIN_EMAIL and
ADMIN_PASSWORD with safe defaults) and replace the fragile substring check in
the BadRequestException handler by normalizing e.body to a string (e.g.,
str(e.body)) or, if available, checking a structured error code field; update
references in the fixture (SignUpDto, BadRequestException, e.body) accordingly
so sign_up_admin and the exception handling remain robust and configurable.
🧹 Nitpick comments (4)
pyproject.toml (1)
49-55: Pillow pin may break on newer Python (e.g., 3.14) depending on wheel support—please verify.Given CI runs Python 3.14, double-check that
pillow>=10,<11has wheels for your runners; otherwise this will fall back to source builds (slow/fail) or fail outright.Proposed tweak (if you hit wheel/build issues)
- "pillow>=10.0.0,<11.0.0", + "pillow>=10.0.0,<12.0.0",.github/workflows/cd.yml (1)
80-91: Readiness check is log-string + 60s timeout → likely flaky on cold runners.Consider increasing the timeout and/or using a real health endpoint (or
docker compose ps+ HTTP probe) so transient log timing doesn’t fail the job.tests/e2e/client/generators.py (1)
12-40: JPEG generator: global mutable iterator is fine for single-worker runs, but can race under parallel test execution.If you ever run e2e with xdist (or any parallelism), this shared global
_jpeg_factorycan interleave and re-emit duplicates. Safer pattern: make the generator instance fixture-scoped (per test/session) or makemake_random_image(seed: int)pure.tests/e2e/client/test_wrappers.py (1)
57-71: API key is created but never deleted (local runs will accumulate keys).CI is probably ephemeral, but for local/dev repeat runs this pollutes the instance. If
create_api_keyreturns an ID, store it and delete it in teardown.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/cd.ymlpyproject.tomltests/e2e/client/generators.pytests/e2e/client/test_wrappers.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
38-38: Possible hardcoded password assigned to argument: "password"
(S106)
47-47: Possible hardcoded password assigned to argument: "password"
(S106)
112-113: try-except-pass detected, consider logging the exception
(S110)
112-112: Do not catch blind exception: Exception
(BLE001)
119-120: try-except-pass detected, consider logging the exception
(S110)
119-119: Do not catch blind exception: Exception
(BLE001)
129-129: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
pyproject.toml (1)
86-91: Theasyncio_mode = "auto"configuration is fully supported by the project's pytest-asyncio version constraint (>=1.2.0), which exceeds the minimum version (0.17) where this mode was introduced. No version-related warnings or incompatibilities should occur.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/client/test_wrappers.py (2)
99-120: Consider logging cleanup errors for debugging flaky tests.Silent
except Exception: passblocks can hide issues when tests fail intermittently. While best-effort cleanup is appropriate, logging at debug level helps diagnose problems.♻️ Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + @pytest.fixture(autouse=True) async def cleanup_assets_teardown( client_with_api_key: AsyncClient, asset_cleanup: dict ): """Autouse fixture to clean up uploaded assets after each test.""" yield # Teardown: Clean up all uploaded assets asset_ids = asset_cleanup.get("asset_ids", []) if asset_ids: try: await client_with_api_key.assets.delete_assets( AssetBulkDeleteDto(ids=asset_ids, force=True) ) - except Exception: - pass # Ignore cleanup errors + except Exception: + logger.debug("Failed to cleanup assets: %s", asset_ids, exc_info=True) # Teardown: Clean up profile image if uploaded if asset_cleanup.get("profile_image", False): try: await client_with_api_key.users.delete_profile_image() - except Exception: - pass # Ignore cleanup errors + except Exception: + logger.debug("Failed to cleanup profile image", exc_info=True)
123-148: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but not used in this test.♻️ Suggested fix
@pytest.mark.asyncio @pytest.mark.e2e async def test_assets_upload( client_with_api_key: AsyncClient, test_image: Path, test_video: Path, - tmp_path: Path, asset_cleanup: dict, ):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomltests/e2e/client/generators.pytests/e2e/client/test_wrappers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/client/test_wrappers.py (7)
immich/sdk.py (1)
AsyncClient(72-365)immich/client/exceptions.py (1)
BadRequestException(182-183)tests/e2e/client/generators.py (2)
make_random_image(30-38)make_random_video(78-80)immich/client/api/api_keys_api.py (1)
create_api_key(42-103)immich/client/api/assets_api.py (1)
delete_assets(1063-1124)immich/client_wrapper/assets_api_wrapped.py (1)
upload(167-260)immich/client_wrapper/users_api_wrapped.py (1)
get_profile_image_to_file(12-51)
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
38-38: Possible hardcoded password assigned to argument: "password"
(S106)
47-47: Possible hardcoded password assigned to argument: "password"
(S106)
112-113: try-except-pass detected, consider logging the exception
(S110)
112-112: Do not catch blind exception: Exception
(BLE001)
119-120: try-except-pass detected, consider logging the exception
(S110)
119-119: Do not catch blind exception: Exception
(BLE001)
129-129: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (12)
tests/e2e/client/generators.py (5)
11-16: LGTM!Clean implementation for generating minimal test JPEGs.
19-24: LGTM!The step of 16 provides 4,096 unique images, which is ample for E2E testing.
27-38: LGTM!The reset logic correctly handles factory exhaustion. Note that global state means tests sharing this module must run sequentially (not with pytest-xdist parallel workers) to avoid race conditions, which is typically fine for E2E tests.
41-64: LGTM!The seed-based mutation approach correctly generates unique MP4 files. Python's arbitrary-precision integers handle large seed values safely, and the
% 256ensures valid byte outputs.
67-80: LGTM!The infinite generator with seed wrapping provides sufficient uniqueness for E2E tests.
tests/e2e/client/test_wrappers.py (7)
25-70: LGTM!The fixture handles both initial setup and subsequent runs gracefully. The hardcoded credentials are acceptable for E2E test fixtures running against ephemeral test servers.
Minor consideration: API keys accumulate across test runs since they're not cleaned up. This is typically fine for short-lived E2E environments but worth noting if tests run against a persistent server.
73-86: LGTM!Simple, clean fixtures that leverage the generators module.
151-176: LGTM!Good round-trip test verifying upload and download content integrity.
179-203: LGTM!Correctly tests thumbnail download. Checking only existence (not content) is appropriate since thumbnails are server-generated.
206-230: LGTM!Correctly tests video stream download functionality.
233-261: LGTM!Good coverage for the archive download functionality.
264-292: LGTM!Good coverage for user profile image upload and download. The tuple format
("profile.jpg", img_bytes)correctly handles content type detection.
…t in unit and e2e tests, ensuring consistent interpreter usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cd.yml (1)
89-99: Consider increasing timeout for slower CI environments.The 60-second timeout may be tight on slower CI runners or when pulling images for the first time. The documentation comments referencing the source of the log message are excellent for maintainability.
💡 Optional: Increase timeout for reliability
- timeout 60s bash -lc 'until docker logs immich-e2e-server 2>&1 | grep -q "Immich Microservices is running"; do sleep 1; done' + timeout 120s bash -lc 'until docker logs immich-e2e-server 2>&1 | grep -q "Immich Microservices is running"; do sleep 1; done'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cd.ymlimmich/_internal/upload.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (7)
immich/_internal/upload.py (1)
260-260: LGTM!Using
os.fspath(path)correctly converts thePathobject to a string before passing it tostatx(). This is the standard approach for ensuring compatibility with libraries that expect filesystem path strings rather thanPathobjects..github/workflows/cd.yml (6)
44-49: LGTM!Good documentation explaining why
mise_tomlis used overinstall_argsfor matrix Python version override. The inline configuration approach ensures the matrix version becomes the active interpreter.
75-83: LGTM!Good approach: reading the version from a tracked file ensures consistency, and the shallow clone minimizes checkout time. The git clone will fail naturally if the specified tag doesn't exist, providing implicit validation.
85-87: LGTM!Correctly pipes the token to
docker loginto avoid command-line exposure. Standard pattern for GHCR authentication in GitHub Actions.
101-115: LGTM!Good job structure: environment variables are correctly set for the Immich e2e server, failure logs are captured for debugging, and the
if: always()teardown ensures Docker resources are cleaned up even on failure or cancellation.
119-119: LGTM!The dependency chain is correctly configured. When
e2e-testsis skipped due to its conditional, GitHub Actions treats it as successful, so the release job will still run on main branch pushes (where both jobs' conditions are satisfied).
54-73: Python 3.14 is stable and available as of January 2026.The job correctly gates e2e tests to main pushes or PRs with the
run-e2elabel, and uses minimal permissions. Python 3.14.0 was released on October 7, 2025, and is widely available in pyenv and similar package managers. Pinning to Python 3.14 is appropriate and poses no stability concerns.
… to load it for Python version management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/cd.yml:
- Around line 64-65: The "Install mise" GitHub Actions step currently using
jdx/mise-action@146a281... needs to pass the --env ci flag so mise loads
mise.ci.toml and uses the pinned uv version; update the "Install mise" step (the
step named "Install mise" that invokes jdx/mise-action) to include the action
input for arguments (e.g., add a with/args entry supplying "--env ci" or
equivalent per the action’s docs) so the CI environment file is loaded
consistently with the "test" job.
In @mise.ci.toml:
- Around line 1-3: Update the uv tool version defined in the tools table from
"0.9.21" to a current patched release (preferably "0.9.24", or "0.9.23" if you
prefer the slightly older stable patch) in mise.ci.toml; locate the tools table
entry with the key uv and change its string value accordingly to pick up the
security/stability fixes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cd.ymlmise.ci.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (4)
.github/workflows/cd.yml (4)
44-47: LGTM! Clear workaround with helpful documentation.The comment explains the rationale well, and the approach cleanly separates constant tools (in mise.ci.toml) from dynamic Python version management.
81-97: Well-structured e2e setup with proper readiness check and cleanup.Good practices observed:
- Timeout-based readiness check with clear log message pattern
- Helpful inline comment referencing the source of the expected log message
- Environment variables properly configured for test execution
99-107: LGTM! Proper failure diagnostics and guaranteed cleanup.Using
if: failure()for logs andif: always()for teardown ensures good debuggability while guaranteeing resource cleanup.
111-111: This concern does not apply to the current configuration.The
releasejob is already gated to main branch pushes via its ownifcondition on line 112. Sincee2e-testsis configured to run on all main branch pushes (the first part of its conditional on line 54), it will never be skipped whenreleaseattempts to execute. Therefore, the dependency one2e-testsin theneedsarray does not create a blocking issue with conditional skipping.
…mplate, enhancing uniqueness through seed-based mutations in the payload region.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/client/generators.py (1)
31-39: Consider clarifying the naming/docstring.The function name and docstring use "random" but the generator is deterministic and sequential. For clarity, consider renaming to
make_unique_image()or updating the docstring to say "unique" rather than "random unique" to accurately reflect the deterministic cycling behavior.tests/e2e/client/test_wrappers.py (1)
89-96: Consider using TypedDict for stronger typing.The current union type
dict[str, list[UUID] | bool]works but loses type safety when accessing keys. A TypedDict would provide better IDE support and type checking:from typing import TypedDict class CleanupData(TypedDict): asset_ids: list[UUID] profile_image: boolThis is optional since the current implementation is functional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/client/generators.pytests/e2e/client/test_wrappers.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: timonrieger
Repo: timonrieger/immich-python-client PR: 28
File: .github/workflows/cd.yml:64-65
Timestamp: 2026-01-12T20:50:31.491Z
Learning: In the `.github/workflows/cd.yml` workflow, the `--env ci` flag for mise should only be used when overriding specific tool versions (like in the `test` job where Python version is overridden via matrix), not universally across all jobs. The `e2e-tests` job doesn't need this flag since it uses the default `mise.toml` configuration without overrides.
📚 Learning: 2026-01-12T20:50:31.491Z
Learnt from: timonrieger
Repo: timonrieger/immich-python-client PR: 28
File: .github/workflows/cd.yml:64-65
Timestamp: 2026-01-12T20:50:31.491Z
Learning: In the `.github/workflows/cd.yml` workflow, the `--env ci` flag for mise should only be used when overriding specific tool versions (like in the `test` job where Python version is overridden via matrix), not universally across all jobs. The `e2e-tests` job doesn't need this flag since it uses the default `mise.toml` configuration without overrides.
Applied to files:
tests/e2e/client/test_wrappers.py
🧬 Code graph analysis (1)
tests/e2e/client/test_wrappers.py (8)
immich/sdk.py (1)
AsyncClient(72-365)immich/client/exceptions.py (1)
BadRequestException(182-183)immich/client/models/asset_bulk_delete_dto.py (1)
AssetBulkDeleteDto(26-84)tests/e2e/client/generators.py (2)
make_random_image(31-39)make_random_video(85-87)immich/client/api/assets_api.py (1)
delete_assets(1063-1124)immich/client/api/users_api.py (2)
delete_profile_image(304-361)get_my_user(1190-1247)immich/client_wrapper/download_api_wrapped.py (1)
download_archive_to_file(17-110)immich/client_wrapper/users_api_wrapped.py (1)
get_profile_image_to_file(12-51)
🪛 Ruff (0.14.10)
tests/e2e/client/test_wrappers.py
38-38: Possible hardcoded password assigned to argument: "password"
(S106)
47-47: Possible hardcoded password assigned to argument: "password"
(S106)
112-113: try-except-pass detected, consider logging the exception
(S110)
112-112: Do not catch blind exception: Exception
(BLE001)
119-120: try-except-pass detected, consider logging the exception
(S110)
119-119: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (15)
tests/e2e/client/generators.py (5)
12-17: LGTM!Clean and efficient implementation for generating minimal test JPEGs.
20-28: LGTM!The stepped RGB cycling provides 4096 unique images, which should be ample for E2E test coverage.
42-71: LGTM!The MP4 mutation strategy is well-designed:
- Safely copies the template before mutating
- Correctly locates the mdat payload region
- Provides a fallback with a harmless free box if mdat is absent
- Bounds checking prevents buffer overflows
74-79: LGTM!Infinite generator with 2³² unique seed values ensures the factory never needs reinitialization during test runs.
85-87: LGTM!Straightforward delegation to the infinite factory.
tests/e2e/client/test_wrappers.py (10)
1-22: LGTM!Clean import organization with all necessary dependencies for the E2E test suite.
25-70: LGTM!Well-structured fixture with:
- Idempotent admin setup (gracefully handles "already has an admin")
- Proper resource cleanup with try/finally
- Environment variable support for base URL
The hardcoded credentials flagged by static analysis (S106) are acceptable for E2E tests running against isolated test environments.
73-86: LGTM!Clean fixtures leveraging pytest's
tmp_pathfor automatic file cleanup.
99-120: LGTM!The autouse fixture ensures reliable test isolation with automatic cleanup. The broad exception handling (flagged by S110/BLE001) is appropriate here—cleanup failures shouldn't cause test failures, and debugging cleanup issues is secondary to the actual test outcomes.
If you'd like to improve debuggability, consider logging cleanup failures at DEBUG level, but this is optional.
123-147: LGTM!Good test coverage for the upload workflow with:
- Concurrent upload testing (
concurrency=2)- Comprehensive assertions on stats and results
- Proper asset tracking for cleanup
150-175: LGTM!Excellent round-trip verification—the byte-level comparison at line 175 ensures data integrity through the upload/download cycle.
178-202: LGTM!Correctly tests the thumbnail download path. Since thumbnails are server-generated, existence/file-type verification is the appropriate level of assertion here.
205-229: LGTM!Properly tests the video playback download path. Similar to thumbnails, server-side processing means existence verification is the correct assertion level.
232-260: LGTM!Good coverage of the archive download functionality with appropriate assertions for ZIP file creation.
263-291: LGTM!Complete test of the profile image workflow with proper cleanup tracking. The tuple format
("profile.jpg", img_bytes)at line 280 correctly ensures content-type detection.
@coderabbitai review
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Chores
Dev Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.