Repository navigation
ci: dedicated pytest --cov job to take coverage off the ansible-test fork path #587
Description
Follow-up to #585 / #586. This is the "dedicated coverage job" item from #585's follow-ups, promoted to its own issue so the trade-offs can be discussed against real numbers.
Where #586 leaves us
#586 gates coverage to one units matrix cell. Two CI runs of that PR, identical code and commands:
| develop (before) | #586 run 1 | #586 run 2 | |
|---|---|---|---|
| Non-coverage cell, total | 13m – 25m | ~2m | ~2m |
| Non-coverage cell, pytest itself | 7m 10s | 68s | 68s |
| Coverage cell: Run unit tests | 12m 33s | 11m 39s | 15m 14s |
| Coverage cell: Generate coverage report (combine) | 9m 05s | 7m 10s | 12m 31s |
| Coverage cell, total | 22m 08s | 19m 21s | 28m 43s |
| Whole run, runner-minutes | 309 | 96 | ~105 |
Runs: 35527908847 (develop), 35763080776, 35765703701.
Two things stand out:
- Coverage tracing under
ansible_forkedis the expensive part, not pytest. The same 4,994 tests take 68s without coverage and 11–15 min with it. The combine then adds another 7–12 min because ansible-test forks per test and writes ~5,000 data files. Coverage in one ansible-test cell costs roughly 20–30 runner-minutes for work that a single process would do in about two. - Variance is large and now sets the wall clock. The coverage cell swung 19m → 29m between two runs of the same commit, and it is the only thing left on the units stage's critical path. Every other cell is done at 2 minutes. Reviewers wait on that one cell.
Proposal
Add a coverage job that runs plain pytest with pytest-cov, outside ansible-test, and drop --coverage from the units matrix entirely:
coverage:
name: Unit Test Coverage
needs:
- ansible-galaxy-importer
runs-on: ubuntu-latest
permissions:
contents: read
id-token: write # Codecov OIDC upload
steps:
- uses: actions/checkout@v6
with:
path: ansible_collections/cisco/nd
- uses: actions/setup-python@v6
with:
python-version: "3.13"
- run: pip install https://github.com/ansible/ansible/archive/stable-2.19.tar.gz pytest pytest-ansible pytest-cov -r ansible_collections/cisco/nd/tests/unit/requirements.txt
- run: pytest --ansible-unit-inject-only --cov=plugins --cov-branch --cov-report=xml tests/unit
working-directory: ansible_collections/cisco/nd
env:
ANSIBLE_COLLECTIONS_PATH: ${{ github.workspace }}
- uses: codecov/codecov-action@v7
with:
files: ansible_collections/cisco/nd/coverage.xml
flags: unit
use_oidc: true
fail_ci_if_error: trueThis is the same invocation developers run locally today (pytest --ansible-unit-inject-only with ANSIBLE_COLLECTIONS_PATH set), so the CI number is reproducible on a laptop.
Pros
- Removes the 19–29 min tail from the units stage. With no coverage cell, the units stage finishes when the slowest 2-minute cell does. Expected coverage job time: pytest in one process under tracing, no fork, no combine, so a few minutes at most.
- Removes the variance. Fork-per-test under tracing is the part that swung 10 minutes between runs. A single-process run has far less to vary.
- Another ~20 runner-minutes per run. The coverage cell costs 19–29 min; the replacement should cost 3–5.
- Branch coverage (
--cov-branch), which ansible-test does not enable. Meaningful for the state-machine and orchestrator code. - Local parity. Same command as
ndpytest, same environment shape, same number.
Cons
- A second test runner in CI. The job needs its own Python,
ansible-core,pytest-ansible,pytest-cov,pydantic, and theansible_collections/cisco/nddirectory layout. All solved for local runs, but it is a second recipe that can drift from whatansible-test unitsdoes (different pytest config, noansible_forkedisolation, no default-test-container). - Path mapping to Codecov. ansible-test rewrites coverage paths to the collection root. Plain
pytest-covreports paths relative to the checkout, which is why the sketch checks out intoansible_collections/cisco/ndand hands Codecov that path. If this is wrong, Codecov silently shows 0% forplugins/. One run will tell. - The number will move. Fork-per-test attributes module-level code to every test that imports it; a single process counts it once. Totals will be close to the 87.68% Codecov shows today, not identical. Worth saying once in the PR so nobody chases it.
- Duplicate red on a real test failure. A broken test fails this job and 12 matrix cells. Harmless, but this job should not become a required status check.
-n autois a separate risk. Our tests are designed to be isolated (ResponseGeneratorfixtures, unique fixture keys per test), but parallel workers outsideansible_forkedwould be the first time that isolation is actually exercised. Land without it first; add it in its own PR.
Weighing them
Before #586 the case for this job was weak: coverage cost was spread across 12 cells and no single cell stood out. After #586 the evidence is one cell that costs 20–30 runner-minutes, varies by 50% run to run, and is the only thing anyone waits on. The cons are all one-time setup or things a first run will expose. Recommendation: do it, in this order:
- Add the
coveragejob as described, leave--coverageon the one units cell for one PR so both numbers land on the same commit and Codecov's path mapping can be checked side by side. - Once the new job's report looks right on Codecov, drop
coverage: trueand the gated steps from the units job. - Consider
-n autoafterwards, separately.
Open questions
- Is ~87% branch-aware coverage from a single process acceptable as "the" number, or does anyone rely on the ansible-test per-fork semantics?
- Should the coverage job pin
stable-2.19/ 3.13 like the current coverage cell, or follow the newest supported pair automatically? - Does anyone want the job to publish the HTML report as an artifact for local browsing?
Activity