Split CI tests into separate tiers - #707
Conversation
Replace the monolithic integration job with multiple tiers using llm, docker, and live_server markers so distinct situations like fork PRs can use a different set based on specific needs. Signed-off-by: pdobrei <pdobrei@cisco.com>
delthazor
added
documentation
pregnor
left a comment
pregnor
left a comment
There was a problem hiding this comment.
I'm not on board with the marker approach, I would favor a suite or test package level separation of the tests over these markers.
I'm also against the dynamic marker application code like in pytest_collection_modifyitems, because it makes it difficult to read the code effectively.
Also let's eliminate LLM key based tests from the CI and do those manually, that simplifies things in the CI workflows as well.
Signed-off-by: pdobrei <pdobrei@cisco.com>
delthazor
changed the title
| ## Directory layout | ||
|
|
||
| - Session / infra orchestration fixtures & agent/client fixtures: [`conftest.py`](coffeeAGNTCY/coffee_agents/corto/tests/integration/conftest.py:1) | ||
| - Docker Compose lifecycle helpers (bring up transport and observability components): [`docker_helpers.py`](coffeeAGNTCY/coffee_agents/corto/tests/integration/docker_helpers.py:1) | ||
| - Lightweight subprocess runner used for agent processes: [`process_helper.py`](coffeeAGNTCY/coffee_agents/corto/tests/integration/process_helper.py:13) | ||
| - Sommelier (flavor profile) integration test: [`test_sommelier.py`](coffeeAGNTCY/coffee_agents/corto/tests/integration/test_sommelier.py:1) | ||
| | Directory | Purpose | CI | | ||
| |-----------|---------|-----| | ||
| | `tests/unit/` | Mocks only | Yes | | ||
| | `tests/integration/` | Docker-compose session; no LLM | Yes | | ||
| | `tests/integration_llm/` | Docker + LLM credentials | No (local manual) | |
There was a problem hiding this comment.
Question: This differs from .github/workflows/README.md even though structurally live should also be present here, because it's similar architecture, is it because we have no tests in that category or something else?
| | corto | `tests/unit tests/integration` | | ||
| | lungo | `tests/unit tests/live tests/integration` | | ||
| | recruiter | `tests/unit tests/integration` | |
There was a problem hiding this comment.
Question: Shouldn't these be analogous with the naming above?
Also shouldn't we use single word folder names instead?
| cd coffeeAGNTCY/coffee_agents/lungo && uv run pytest tests/integration_llm -q | ||
| ``` | ||
|
|
||
| Do not run `pytest tests/` as a single full-suite invocation when both `integration/` and `integration_llm/` exist — session fixtures may load twice via `pytest_plugins`. |
There was a problem hiding this comment.
Question: why is this a requirement?
I thought it's going to be disjunctive sets that together gives a whole.
| ### Branch protection | ||
|
|
||
| | Remove (legacy) | Add (required on PRs) | | ||
| |-----------------|----------------------| | ||
| | `integration-tests-*` | `tests / *` | | ||
| | `fast / *`, `integration / *` | `tests / *` | |
There was a problem hiding this comment.
Question: What is this for?
2 participants
Description
Replace the monolithic integration job with multiple tiers so distinct situations like fork PRs can use a different set based on specific needs.
The goal of this PR is to prepare the test suite for later configuration for specific environments and use cases, like for PR runs.
Important: tests requiring LLM secrets are left out from CI suite, due to security concerns. The test are still part of the codebase, and can be ran locally by manually calling them.
UPDATE: Renamed issue, as the basis of the split was changed from markers to directories. Description changed accordingly.
Issue Link
Closely relates to (but not fully fixes): #595
Type of Change
Checklist