test: add coverage, integration tests, and CI pipeline - #3
Conversation
Add pytest-cov with 80% coverage threshold (currently 85.62%), shared test fixtures in conftest.py, 39 new unit tests covering DockerAPI methods and untested command handler branches, 8 integration tests against Docker-in-Docker, and a GitHub Actions CI pipeline with lint, unit-test, and integration-test jobs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00cae5526e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not api.ping(): | ||
| pytest.skip(f"Docker API at {DOCKER_URL} requires authentication") | ||
| except requests.exceptions.ConnectionError: | ||
| pytest.skip(f"Docker API at {DOCKER_URL} is not reachable") |
There was a problem hiding this comment.
The live_api fixture converts both auth-required and unreachable Docker endpoints into pytest.skip, and every integration test depends on this fixture; in the new CI workflow this means a broken Docker-in-Docker setup can still produce a green integration-tests job with all tests skipped (pytest exits 0), so the pipeline no longer guarantees any real integration coverage. In CI contexts this should error/fail instead of skipping so setup regressions are caught.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR strengthens the project’s quality gates by adding coverage reporting, expanding the automated test suite (unit + live Docker integration), and introducing a GitHub Actions CI workflow to run linting and tests on every PR.
Changes:
- Add
pytest-covand coverage configuration with an 80% minimum threshold. - Add/expand unit tests and introduce Docker-in-Docker–backed integration tests with shared pytest fixtures/markers.
- Add a GitHub Actions CI workflow (lint, unit+coverage, integration) plus supporting docs/ignore entries.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Locks new dev dependency (pytest-cov) and its transitive deps. |
pyproject.toml |
Adds pytest-cov, pytest markers/addopts, and coverage configuration. |
tests/conftest.py |
Introduces shared fixtures and auto-marking for unit vs integration tests. |
tests/test_docker_api.py |
Adds unit tests for DockerAPI behavior via mocked HTTP session. |
tests/test_integration.py |
Adds live integration tests against a real Docker daemon endpoint. |
tests/test_ahab.py |
Expands coverage for CLI flows, completer setup, and error branches. |
docker-compose.test.yml |
Adds a Docker-in-Docker service to support integration tests in CI. |
.github/workflows/ci.yml |
Adds CI pipeline with separate lint/unit/integration jobs. |
README.md |
Updates dev instructions for uv/ruff/pytest/coverage/integration runs. |
.gitignore |
Ignores coverage outputs (.coverage, coverage.xml, htmlcov/). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| environment: | ||
| DOCKER_TLS_CERTDIR: "" | ||
| ports: | ||
| - "2375:2375" |
There was a problem hiding this comment.
The Docker daemon port is published as 2375:2375, which binds on all host interfaces by default. Since this is an unauthenticated Docker Remote API endpoint, it should be bound to localhost only (or avoided entirely) to prevent exposing a root-equivalent control socket to the LAN/Wi‑Fi network.
| - "2375:2375" | |
| - "127.0.0.1:2375:2375" |
| try: | ||
| if not api.ping(): | ||
| pytest.skip(f"Docker API at {DOCKER_URL} requires authentication") | ||
| except requests.exceptions.ConnectionError: |
There was a problem hiding this comment.
live_api only skips on requests.exceptions.ConnectionError, but DockerAPI.ping() can also raise Timeout or other RequestExceptions (e.g., DNS issues). As written, those failures will error the whole integration suite instead of skipping with a clear message; consider catching requests.exceptions.RequestException (or at least Timeout) here and calling pytest.skip.
| except requests.exceptions.ConnectionError: | |
| except requests.exceptions.RequestException: |
| config = { | ||
| "Image": "alpine:latest", | ||
| "Cmd": ["sleep", "30"], | ||
| "Tty": False, | ||
| "HostConfig": {}, | ||
| } |
There was a problem hiding this comment.
These lifecycle tests assume alpine:latest already exists in the target Docker daemon. On a clean daemon, create_container will fail with an image-not-found error before the test can proceed. To keep the integration suite self-contained (and less dependent on CI pre-steps), pull the image (or skip with a clear message) when it’s missing.
Summary
conftest.pywith auto-appliedunit/integrationmarkersTest plan
uv sync --extra devresolves cleanlyuv run pytest -qpasses all 120 unit testsuv run pytest --cov --cov-report=term-missingreports 80%+ coverageuv run ruff check ahab.py tests/ && uv run ruff format --check ahab.py tests/is cleandocker compose -f docker-compose.test.yml up -d --waitthenuv run pytest -m integration -qpasses integration tests