diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 64cb14e39..81a8fba0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,9 +3,22 @@ name: CI # Pre-merge test gate. The deploy workflows (backend.yml, platform.yml, # frontend-deploy.yml) only run on push to develop/main — they run the # test jobs as a gate *before deploying*, so unit tests never ran on the -# PR itself. This workflow runs the full test suites on every pull request -# into develop/main, with no deploy/build/AWS steps, so regressions are -# caught before merge. Deploys remain push-only. +# PR itself. This workflow runs the test suites on every pull request into +# develop/main, with no deploy/build/AWS steps, so regressions are caught +# before merge. Deploys remain push-only. +# +# Which suites run is decided per PR by the `changes` job from the PR's file +# list (scripts/ci/classify-changes.sh): a docs-only PR runs nothing, a +# frontend-only PR runs the two SPA jobs, an infrastructure PR runs jest plus +# the backend suites that read the CDK tree. The mapping is fail-open — an +# unrecognised path, a workflow_dispatch, or a failed file-list call runs +# everything — and is pinned by backend/tests/supply_chain/test_ci_path_filter.py. +# +# The filtering lives on the JOBS, not on the trigger's `paths:`. A workflow +# that does not trigger leaves a required status check "Expected" forever +# and blocks the merge; a job that is skipped reports as skipped and does +# not. develop has no required checks today, but this shape survives adding +# them. on: workflow_dispatch: pull_request: @@ -23,25 +36,71 @@ concurrency: cancel-in-progress: true jobs: + # Classify the PR's changed files into the suites that can observe them. + # The file list comes from the REST API rather than a checkout diff: no + # fetch-depth games, no third-party action, and it is exact for renames + # (both names are fed through). Any failure to obtain the list yields an + # empty stdin, which the classifier treats as "run everything". + changes: + name: Classify changed paths + runs-on: ubuntu-24.04 + permissions: + contents: read + pull-requests: read + outputs: + backend: ${{ steps.classify.outputs.backend }} + backend_contracts: ${{ steps.classify.outputs.backend_contracts }} + frontend: ${{ steps.classify.outputs.frontend }} + infra: ${{ steps.classify.outputs.infra }} + load: ${{ steps.classify.outputs.load }} + scripts: ${{ steps.classify.outputs.scripts }} + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + sparse-checkout: scripts/ci + - name: Classify + id: classify + env: + GH_TOKEN: ${{ github.token }} + EVENT_NAME: ${{ github.event_name }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + files="" + if [ "$EVENT_NAME" = "pull_request" ]; then + files=$(gh api "repos/$REPO/pulls/$PR_NUMBER/files" --paginate \ + --jq '.[] | .filename, (.previous_filename // empty)') || files="" + echo "$(printf '%s\n' "$files" | grep -c .) changed path(s) on PR #$PR_NUMBER" + else + echo "$EVENT_NAME is not a pull request: running every suite" + fi + printf '%s\n' "$files" | bash scripts/ci/classify-changes.sh | tee -a "$GITHUB_OUTPUT" + tests: name: Tests + needs: changes permissions: contents: read uses: ./.github/workflows/tests.yml with: - run_backend: true - run_frontend: true + run_backend: ${{ needs.changes.outputs.backend == 'true' }} + # The supply-chain/architecture subset, for PRs that touch what those + # tests read (CDK, scripts, workflows) without touching backend/. A + # no-op inside tests.yml when run_backend is also true. + run_backend_contracts: ${{ needs.changes.outputs.backend_contracts == 'true' }} + run_frontend: ${{ needs.changes.outputs.frontend == 'true' }} # The frontend suite again under --coverage — the nightly's invocation. # Coverage changes how the specs are built (see tests.yml), so specs can # pass here and fail there. Only the PR gate pays for it; the deploy # workflows keep running the plain suite. - run_frontend_coverage: true - run_infra: true + run_frontend_coverage: ${{ needs.changes.outputs.frontend == 'true' }} + run_infra: ${{ needs.changes.outputs.infra == 'true' }} # Pure-logic tests for tests/load. Deliberately only on the PR gate: the # deploy workflows run test suites to protect a deploy, and the load # suite has no bearing on one. - run_load_unit: true + run_load_unit: ${{ needs.changes.outputs.load == 'true' }} # Offline unit tests for scripts/restore-data. PR gate only, like the # load suite: a deploy gains nothing from them. - run_scripts: true + run_scripts: ${{ needs.changes.outputs.scripts == 'true' }} diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 422505cc2..fba4bb96f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,6 +16,11 @@ on: required: false default: false type: boolean + run_backend_contracts: + description: 'Run only the backend suites that read the CDK tree, scripts and workflows (supply_chain, architecture); a no-op when run_backend is also set' + required: false + default: false + type: boolean run_frontend: description: 'Run frontend test suite' required: false @@ -67,10 +72,13 @@ jobs: run: uv sync --extra agentcore --extra dev - name: Run pytest working-directory: backend - # `-n auto` fans the suite across all runner cores (pytest-xdist). - # Verbosity/--tb live in backend/pytest.ini. The nightly coverage run - # (scripts/backend/test.sh) is deliberately left serial. - run: uv run pytest tests/ -n auto + # `-n logical` fans the suite across every vCPU (pytest-xdist). Not + # `auto`: with psutil installed, `auto` counts PHYSICAL cores, and the + # 4-vCPU runner reports 2, so it started 2 workers for ~11k tests + # (`created: 2/2 workers` in the log). Verbosity/--tb live in + # backend/pytest.ini. The nightly coverage run (scripts/backend/test.sh) + # is deliberately left serial. + run: uv run pytest tests/ -n logical # The repo-ROOT supply-chain suite (not backend/tests/supply_chain): # cross-package contracts that read CDK, the backup/restore scripts and # the python env-var readers side by side, so it lives above backend/. @@ -81,6 +89,38 @@ jobs: - name: Run root supply-chain pytest run: uv run --project backend --no-sync pytest tests/supply_chain/ -v + # The subset of the backend suite that reads OUTSIDE backend/: the + # supply-chain and architecture tests assert against the CDK tree, the + # scripts and the workflows side by side with the Python that consumes + # them, so an infrastructure-only PR still needs them — and nothing else in + # backend/tests/. About a hundred tests, well under a minute, against seven + # minutes for the full suite. Skipped whenever run_backend is set, because + # the full run already covers it. + test-backend-contracts: + if: ${{ inputs.run_backend_contracts && !inputs.run_backend }} + name: Test backend contracts (pytest) + runs-on: ubuntu-24.04 + permissions: + contents: read + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ inputs.ref }} + persist-credentials: false + - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 + with: + version: '0.7.12' + enable-cache: true + cache-dependency-glob: 'backend/uv.lock' + - name: Sync deps + working-directory: backend + run: uv sync --extra agentcore --extra dev + - name: Run backend contract suites + working-directory: backend + run: uv run pytest tests/supply_chain/ tests/architecture/ -v + - name: Run root supply-chain pytest + run: uv run --project backend --no-sync pytest tests/supply_chain/ -v + test-frontend: if: ${{ inputs.run_frontend }} name: Test frontend diff --git a/backend/tests/supply_chain/test_ci_path_filter.py b/backend/tests/supply_chain/test_ci_path_filter.py new file mode 100644 index 000000000..4bf3289b7 --- /dev/null +++ b/backend/tests/supply_chain/test_ci_path_filter.py @@ -0,0 +1,191 @@ +"""The PR gate runs only the suites a change can reach. + +ci.yml's ``changes`` job feeds the pull request's file list through +``scripts/ci/classify-changes.sh`` and wires each output to a ``tests.yml`` +input. These tests pin three things: + +1. The classifier's rules — which paths reach which suite — and above all its + fail-open cases: an unknown directory, an empty list, a workflow or script + change all run everything. Only an explicit allowlist runs nothing. +2. That every ``run_*`` input ``tests.yml`` offers is wired on the PR gate. A + new suite whose input ci.yml never passes is a suite that runs nowhere on a + pull request, and nothing else would notice. +3. That the backend job fans out over logical CPUs. ``-n auto`` counts + physical cores when psutil is installed, which on the 4-vCPU runner is 2. +""" + +import subprocess +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[3] +SCRIPT = REPO_ROOT / "scripts" / "ci" / "classify-changes.sh" +WORKFLOWS = REPO_ROOT / ".github" / "workflows" + +SUITES = ("backend", "backend_contracts", "frontend", "infra", "load", "scripts") + + +def classify(paths: list[str]) -> dict[str, bool]: + stdin = "".join(f"{p}\n" for p in paths) + completed = subprocess.run( + ["bash", str(SCRIPT)], + input=stdin, + capture_output=True, + text=True, + check=True, + ) + result: dict[str, bool] = {} + for line in completed.stdout.splitlines(): + key, _, value = line.partition("=") + assert value in ("true", "false"), line + result[key] = value == "true" + assert set(result) == set(SUITES), completed.stdout + return result + + +def on(*names: str) -> dict[str, bool]: + unknown = set(names) - set(SUITES) + assert not unknown, unknown + return {suite: suite in names for suite in SUITES} + + +ALL = on(*SUITES) +NONE = on() + + +def _load_workflow(name: str) -> dict: + with open(WORKFLOWS / name) as f: + return yaml.safe_load(f) + + +def _triggers(workflow: dict) -> dict: + # PyYAML reads a bare `on:` key as the boolean True. + return workflow.get("on", workflow.get(True)) + + +class TestRules: + @pytest.mark.parametrize( + "paths, expected", + [ + # --- nothing reads these ----------------------------------- + (["docs-site/src/content/docs/configuration/feature-flags.md"], NONE), + (["docs/specs/turn-path-ttft.md", "docs/kaizen/review-queue.md"], NONE), + (["CHANGELOG.md", "README.md", "CONTRIBUTING.md", "CLAUDE.MD"], NONE), + (["LICENSE", ".gitignore"], NONE), + (["tui/src/app.py", "tui/pyproject.toml"], NONE), + # --- one package --------------------------------------------- + (["backend/src/apis/shared/feature_flags.py"], on("backend", "load")), + (["backend/tests/shared/test_anything.py"], on("backend", "load")), + (["backend/pyproject.toml", "backend/uv.lock"], on("backend", "load")), + (["backend/README.md"], on("backend", "load")), + (["frontend/ai.client/src/app/app.ts"], on("frontend")), + (["frontend/ai.client/src/branding/README.md"], on("frontend")), + (["infrastructure/lib/config.ts"], on("infra", "backend_contracts")), + (["infrastructure/test/platform-stack.test.ts"], on("infra", "backend_contracts")), + (["tests/supply_chain/test_backup_coverage.py"], on("backend_contracts")), + (["tests/load/locustfile.py"], on("load")), + # --- cross-package reads pinned by name ---------------------- + # pending-backfills.test.ts asserts the backfill scripts exist. + (["backend/scripts/backfill_something.py"], on("backend", "load", "infra")), + # pending-backfills.test.ts reads RELEASE_NOTES.md. + (["RELEASE_NOTES.md"], on("infra", "backend_contracts")), + # gsi-update-limit.test.ts asserts against the release skill. + ([".claude/skills/cutting-a-release/SKILL.md"], on("infra")), + # --- unions ---------------------------------------------------- + ( + ["frontend/ai.client/src/app/app.ts", "backend/src/x.py", "docs/x.md"], + on("frontend", "backend", "load"), + ), + ( + ["infrastructure/lib/x.ts", "backend/src/x.py"], + on("infra", "backend_contracts", "backend", "load"), + ), + ], + ) + def test_paths_reach_exactly_the_suites_that_read_them(self, paths, expected): + assert classify(paths) == expected + + @pytest.mark.parametrize( + "paths", + [ + # No PR, or the file-list call failed: nothing to reason from. + [], + [""], + # Workflows and composite actions are read by the supply-chain + # suites and shape every job. + [".github/workflows/tests.yml"], + [".github/workflows/ci.yml"], + [".github/actions/something/action.yml"], + [".github/dependabot.yml"], + [".github/ARTIFACT_RETENTION.md"], + # scripts/ feeds four different suites. + ["scripts/frontend/test.sh"], + ["scripts/release/check-pending-backfills.mjs"], + ["scripts/restore-data/restore.py"], + ["scripts/ci/classify-changes.sh"], + # A directory this script has never heard of. + ["brand-new-package/src/main.py"], + # A root file this script has never heard of. + ["Makefile"], + ["VERSION"], + # One unknown path in a docs PR still runs everything. + ["docs/x.md", "brand-new-package/main.py"], + ], + ) + def test_anything_unrecognised_runs_everything(self, paths): + assert classify(paths) == ALL + + def test_a_rename_is_classified_under_both_names(self): + # ci.yml feeds `.filename` and `.previous_filename` as separate lines. + assert classify(["frontend/new.ts", "backend/old.py"]) == on( + "frontend", "backend", "load" + ) + + +class TestWiring: + def test_ci_wires_every_suite_tests_yml_offers(self): + ci = _load_workflow("ci.yml") + tests = _load_workflow("tests.yml") + + offered = { + name + for name in _triggers(tests)["workflow_call"]["inputs"] + if name.startswith("run_") + } + wired = ci["jobs"]["tests"]["with"] + assert set(wired) == offered, ( + "ci.yml must pass every run_* input tests.yml declares; " + f"missing={offered - set(wired)} extra={set(wired) - offered}" + ) + for name, value in wired.items(): + assert isinstance(value, str) and "needs.changes.outputs." in value, ( + f"{name} must be decided by the changes job, not hard-coded: {value!r}" + ) + + def test_changes_job_exposes_every_classifier_output(self): + ci = _load_workflow("ci.yml") + changes = ci["jobs"]["changes"] + assert set(changes["outputs"]) == set(SUITES) + assert ci["jobs"]["tests"]["needs"] == "changes" + run_blocks = "\n".join(step.get("run", "") for step in changes["steps"]) + assert "scripts/ci/classify-changes.sh" in run_blocks + + def test_changes_job_only_reads(self): + ci = _load_workflow("ci.yml") + perms = ci["jobs"]["changes"]["permissions"] + assert perms == {"contents": "read", "pull-requests": "read"} + + def test_contracts_job_yields_to_the_full_backend_run(self): + tests = _load_workflow("tests.yml") + job = tests["jobs"]["test-backend-contracts"] + assert "inputs.run_backend_contracts" in job["if"] + assert "!inputs.run_backend" in job["if"] + + def test_backend_pytest_fans_out_over_logical_cpus(self): + tests = _load_workflow("tests.yml") + steps = tests["jobs"]["test-backend"]["steps"] + pytest_step = next(s for s in steps if s.get("name") == "Run pytest") + assert "-n logical" in pytest_step["run"] + assert "-n auto" not in pytest_step["run"] diff --git a/docs-site/src/content/docs/development/testing.md b/docs-site/src/content/docs/development/testing.md index d6731b0d3..a359fd5f3 100644 --- a/docs-site/src/content/docs/development/testing.md +++ b/docs-site/src/content/docs/development/testing.md @@ -1,12 +1,38 @@ --- title: Testing -description: Backend and frontend tests. +description: What each test suite covers, how to run it locally, and what runs on a pull request. sidebar: order: 2 --- -:::caution[Draft] -This page is a scaffolded placeholder — content to be written. -::: +## Suites -pytest (not run in CI), Vitest, jest for infra, and import-boundary tests. +| Suite | Where | Run locally | +|---|---|---| +| Backend (pytest, ~11k tests) | `backend/tests/` | `cd backend && uv run pytest tests/ -n logical` | +| Root supply-chain contracts | `tests/supply_chain/` | `uv run --project backend --no-sync pytest tests/supply_chain/` | +| Frontend (Vitest via Analog) | `frontend/ai.client/src/**/*.spec.ts` | `cd frontend/ai.client && npm test` | +| Infrastructure (jest) | `infrastructure/test/` | `cd infrastructure && npx jest` | +| Load-test unit tests | `tests/load/tests/` | `cd tests/load && uv run pytest tests/` | +| Operational scripts | `scripts/restore-data/test_restore.py` | `cd scripts/restore-data && uv run --frozen pytest test_restore.py` | + +The backend suite is hermetic: a conftest guard fails any test that opens a socket to a non-local host, so a partially mocked test cannot quietly reach AWS. Run it with `-n logical` rather than `-n auto`; with psutil installed, `auto` counts physical cores and starts half as many workers as the machine has. + +## What runs on a pull request + +`.github/workflows/ci.yml` runs on every pull request into `develop` or `main`, but it does not run every suite every time. A `changes` job reads the PR's file list and maps each path to the suites that can observe it (`scripts/ci/classify-changes.sh`): + +| Changed paths | Suites | +|---|---| +| `docs/`, `docs-site/`, `tui/`, root markdown, `LICENSE` | none | +| `backend/` | backend, load-test unit | +| `backend/scripts/` | backend, load-test unit, infrastructure (jest asserts the backfill scripts exist) | +| `frontend/` | frontend, frontend under `--coverage` | +| `infrastructure/`, `RELEASE_NOTES.md` | infrastructure, plus the backend supply-chain and architecture suites that read the CDK tree | +| `.claude/` | infrastructure (one jest test reads the release skill) | +| `tests/load/` | load-test unit | +| `.github/`, `scripts/`, anything unrecognised | everything | + +The mapping is fail-open. A new top-level directory, an empty file list, or a `workflow_dispatch` run turns every suite on; only the explicit list in the first row turns none on. The rules are pinned by `backend/tests/supply_chain/test_ci_path_filter.py`, so extend the script and the test together. + +The deploy workflows (`backend.yml`, `platform.yml`, `frontend-deploy.yml`) are unaffected: on push to `develop` and `main` they still run their full suites before deploying, so the PR gate is the first line, not the last. diff --git a/docs/kaizen/review-queue.md b/docs/kaizen/review-queue.md index bb0374add..0404e47e7 100644 --- a/docs/kaizen/review-queue.md +++ b/docs/kaizen/review-queue.md @@ -5,6 +5,15 @@ Items added by `kaizen-research`, consumed by `kaizen-review-prep`. ## Open +### [2026-09-30] Backend test suite: shard across runners (#1390) and fix fixture scope (#1391) +- **Source**: Phil-initiated. The PR gate ran every suite on every pull request: a two-file docs PR paid the same 7–8 minutes as a backend change, and the backend pytest job alone was 7:02 with every other job under 2 minutes. Two things landed together in the path-filter PR: the `changes` job in `ci.yml` now runs only the suites a PR's paths can reach (`scripts/ci/classify-changes.sh`, fail-open, pinned by `test_ci_path_filter.py`), and the backend job runs `-n logical` — `-n auto` counts physical cores when psutil is installed, so the 4-vCPU runner started 2 workers. + - **What the profile showed.** Of the time measured across the suite (every phase ≥50 ms), fixture setup was 1,846 s against 399 s of test body. 77 files enter `mock_aws()` and create tables per test function; 5 files use any broader scope. One file alone: 21.8 s setup, 0.9 s test code. The top 25 files are ~950 s of ~2,245 s and are listed in #1391. + - **Two follow-ups, complementary.** #1390 shards the backend job across 3 runners with `pytest-split` (new dev dep, needs approval) — a CI-only win, floor ~2–2.5 min. #1391 scopes the moto fixtures at module level file by file — the bigger lever, and the only one that speeds up local runs. +- **Surface**: CI only for #1390; test files only for #1391. No production code. +- **Effort × Impact**: #1390 M × M (one workflow change plus a durations file with an owner). #1391 M × H, incremental (one PR per handful of files; track the setup:call ratio). +- **Subtracts**: no. Both reduce PR wall clock without dropping a test. +- **Status**: open; issues carry the acceptance criteria. Also worth a decision: `develop` has no required status checks, so the PR gate is advisory today. The job-level filter shape was chosen so that adding them later does not strand a skipped suite as "Expected". + ### [2026-09-26] Verify native token counts for `global.*` models in production — after #1343 reaches `main` - **Source**: Phil-initiated, from #1343 and its dev validation. - **The gap.** `base_foundation_model_id` never stripped `global.`, so every prod model counted with the heuristic. Since #1337, that means prod records no `prefixTokens` and no `contextBreakdown` at all. diff --git a/scripts/ci/classify-changes.sh b/scripts/ci/classify-changes.sh new file mode 100755 index 000000000..7019e9358 --- /dev/null +++ b/scripts/ci/classify-changes.sh @@ -0,0 +1,83 @@ +#!/usr/bin/env bash +# Map a pull request's changed paths to the test suites that can observe them. +# +# stdin: one repository-relative path per line (the PR's changed files; a +# rename is listed under both its old and new name). +# stdout: `key=true|false` lines for $GITHUB_OUTPUT, one per tests.yml suite: +# backend, backend_contracts, frontend, infra, load, scripts. +# +# Called by the `changes` job in .github/workflows/ci.yml. Before it existed +# the PR gate ran every suite on every pull request, so a two-line docs change +# paid the full backend suite (seven minutes on the runner) for a result that +# could not have changed. +# +# Fail-open by construction: +# * a path that matches no rule below turns EVERY suite on, so a new +# top-level directory is never silently untested; +# * an empty file list (no PR, or the API call failed) does the same; +# * only an explicit allowlist of paths that no suite reads turns nothing on. +# Which suite reads what is pinned by +# backend/tests/supply_chain/test_ci_path_filter.py — extend both together. +set -euo pipefail + +backend=false +backend_contracts=false +frontend=false +infra=false +load=false +scripts=false +all=false +seen=0 + +while IFS= read -r path; do + [ -z "$path" ] && continue + seen=$((seen + 1)) + case "$path" in + # Workflows, composite actions, dependabot and the retention doc are read + # by the supply-chain suites. scripts/ feeds the frontend coverage job + # (scripts/frontend/test.sh), the infra jest suite (scripts/release), the + # backend supply-chain suite (scripts/build) and the restore-data suite. + # Both change rarely and both reach several suites, so run everything. + .github/*|scripts/*) all=true ;; + # backend/scripts holds the backfills that infrastructure/test asserts + # exist by path (pending-backfills.test.ts). + backend/scripts/*) backend=true; load=true; infra=true ;; + # tests/load hand-encodes the SSE event names and the chat payload, so it + # runs with the backend: a rename in agents/main_agent/streaming should + # fail there rather than produce a load test that never sees a turn end. + backend/*) backend=true; load=true ;; + frontend/*) frontend=true ;; + # The backend supply-chain suite reads the CDK tree side by side with the + # env-var readers; pending-backfills.test.ts reads RELEASE_NOTES.md. + infrastructure/*|RELEASE_NOTES.md) infra=true; backend_contracts=true ;; + # gsi-update-limit.test.ts asserts against the cutting-a-release skill. + .claude/*) infra=true ;; + # The repo-root supply-chain suite runs inside the backend jobs. + tests/supply_chain/*) backend_contracts=true ;; + tests/load/*) load=true ;; + # No suite reads these. docs-site has its own build on push to main, and + # the TUI is not wired into CI at all. + docs/*|docs-site/*|tui/*) ;; + # Any other directory is unknown to this script: run everything. + */*) all=true ;; + # Root files no suite reads (RELEASE_NOTES.md was matched above). + *.md|*.MD|LICENSE|.gitignore|.editorconfig) ;; + *) all=true ;; + esac +done + +if [ "$seen" -eq 0 ] || [ "$all" = true ]; then + backend=true + backend_contracts=true + frontend=true + infra=true + load=true + scripts=true +fi + +printf 'backend=%s\n' "$backend" +printf 'backend_contracts=%s\n' "$backend_contracts" +printf 'frontend=%s\n' "$frontend" +printf 'infra=%s\n' "$infra" +printf 'load=%s\n' "$load" +printf 'scripts=%s\n' "$scripts"