From 5935746ffa850239f07224857e1e9b1bbf4bcf5d Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:56:35 -0700 Subject: [PATCH] Add: test-category axis so a suite-only change runs only its own suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tests/st/` + `examples/` and `tests/ut/` execute disjoint trees — `pytest examples tests/st` versus `pytest tests/ut` plus the C++ ctest — and no unit test reads `examples/` or `tests/st/`. The only cross-references are a faked nodeid string in `tests/ut/py/test_resource_failure_summary.py` and a comment in `test_error_code_names.cpp`; neither opens a file. So a diff confined to one suite cannot break the other, and until now every such diff ran both. Over the last 60 commits on main, 8 were scene-test/example-only and 3 were unit-test-only — around 18% paying for a suite that could not have regressed, including time on the self-hosted a2a3 and a5 pools. `st_affected` and `ut_affected` are written in the same shape as the existing arch flags: a partition is unaffected only when EVERY changed file belongs exclusively to a sibling partition, with `NON_CODE` subtracted first. Copying the shape is what buys the two properties that matter. Unrecognised paths survive both filters and turn the flag on, so a new directory over-runs CI rather than silently skipping it. And shared test infrastructure — the root `conftest.py`, `pyproject.toml`, `simpler_setup/`, `tests/lint/` — belongs to no single partition, so it matches no sibling pattern and flips every flag true, without needing to be enumerated anywhere. The fail-open guard also learns that a *failed* `git diff` is the same as an empty one. `run:` is `bash -e`, so a non-zero diff — unresolved base/head SHA — aborted the step before the guard and emitted no outputs at all; a failed `detect-changes` then leaves every downstream `needs:` unsatisfied, which SKIPS the matrix rather than running it. Fail-closed, the opposite of the intent. Only the exit status decides now, and a failure folds into the empty case. The UT jobs stay off the arch axis. `docs/ci.md` principle 5 argues that unit tests cover shared contracts and a falsely-skipped regression outweighs the minutes saved, which is a judgement about silicon attribution and still holds. Test category is a different question, and the answer differs. `packaging-matrix` keeps `non_code_only` alone. Neither `tests/` nor `examples/` ships in the wheel, so it over-runs on a tests-only diff, but it is cheap and GitHub-hosted and a fourth condition buys little. Simulated against thirteen file lists before pushing. `tests/st/`-only and `examples/`-only now reach scene tests alone; `tests/ut/py/`-only and `tests/ut/cpp/a5/`-only reach unit tests alone; `src/common/`, the root `conftest.py`, `pyproject.toml`, `simpler_setup/`, a mixed st+ut diff, an empty diff and an unresolvable-SHA diff all still reach both. `docs/ci.md` and `.claude/rules/ci-change-detection.md` move with it — the rule now describes three layered axes, states the shared shape as the reason a fourth axis must copy it rather than invent one, and covers the failed-diff case under "fail open once, at the top". Co-Authored-By: Claude Opus 5 (1M context) --- .claude/rules/ci-change-detection.md | 76 ++++++++++++++++++++++------ .github/workflows/ci.yml | 64 +++++++++++++++++++---- docs/ci.md | 25 ++++----- 3 files changed, 127 insertions(+), 38 deletions(-) diff --git a/.claude/rules/ci-change-detection.md b/.claude/rules/ci-change-detection.md index 91c9eef5f2..a5ca926c4a 100644 --- a/.claude/rules/ci-change-detection.md +++ b/.claude/rules/ci-change-detection.md @@ -53,28 +53,74 @@ never sit behind a *more permissive* condition than a GitHub-hosted one. If you find yourself writing a looser `if:` for the more expensive job, the vocabulary is wrong (see §1) — fix that instead of widening the gate. -## 4. Two axes, and why the arch axis already implies the other +## 4. Three axes, layered — and the non-code one is subtracted first -| Axis | Output | Used by | +| Axis | Output | Answers | | ---- | ------ | ------- | -| non-code | `non_code_only` | arch-agnostic jobs: `ut`, `packaging-matrix`, `ut-a2a3`, `ut-a5` | -| architecture | `a2a3_changed` / `a5_changed` | arch-specific jobs: `st-sim-*`, `st-onboard-*`, `profiling-flags-smoke` | +| non-code | `non_code_only` | can this diff change what the code does at all? | +| architecture | `a2a3_changed` / `a5_changed` | which silicon can it reach? | +| test category | `st_affected` / `ut_affected` | which suite can it break? | -These are layered, not redundant. `a2a3_changed` and `a5_changed` are computed -by subtracting `NON_CODE` *first*, so a non-code-only change already makes both -false. An arch-gated job therefore needs no separate non-code check, and adding -one would be noise. An arch-agnostic job cannot use the arch axis at all. +They are layered, not redundant. **Every arch and category flag subtracts +`NON_CODE` before deciding**, so a non-code-only change already makes all four +false. An arch- or category-gated job therefore needs no separate non-code +check, and adding one would be noise. -Keep that asymmetry deliberate. If a new job is arch-specific, gate it on the -arch axis; if not, on `non_code_only`. Mixing them per-job is how §1 gets -violated again. +A job composes the axes it is actually subject to: + +| Job family | Gate | +| ---------- | ---- | +| `st-sim-*`, `st-onboard-*` | `_changed && st_affected` | +| `profiling-flags-smoke` | `(a2a3_changed \|\| a5_changed) && st_affected` | +| `ut`, `ut-a2a3`, `ut-a5` | `non_code_only != true && ut_affected` | +| `packaging-matrix` | `non_code_only != true` | + +The UT jobs stay off the arch axis on purpose — unit tests cover shared +contracts, so the cost of a falsely-skipped regression outweighs the minutes. +That is a decision about the *arch* axis only; the category axis is a different +question, because a scene-test-only change genuinely cannot break a unit test. + +### Write every axis in the same shape + +The arch and category axes use one pattern: **a partition is unaffected only +when every changed file belongs exclusively to a sibling partition.** + +```bash +SIBLING='^(...)' # what this partition is NOT +REMAINING=$(echo "$FILES" | grep -vE "$SIBLING" | grep -vE "$NON_CODE" || true) +[ -n "$REMAINING" ] && flag=true || flag=false +``` + +Two properties come for free, and both are why a new axis must copy the shape +rather than invent one: + +- **Fail-safe direction.** Any path the patterns do not recognise survives the + filters, so it lands in `REMAINING` and turns the flag *on*. New directories + over-run CI rather than silently skipping it. +- **Shared infrastructure resolves correctly without being enumerated.** The + root `conftest.py`, `pyproject.toml`, `simpler_setup/` and `tests/lint/` + belong to no single partition, so they match no `SIBLING` and flip every flag + true — which is what they should do, since both suites load them. ## 5. Fail open once, at the top -An empty file list means attribution is impossible — a PR with no files, or -base/head SHAs that did not resolve. Every flag must then report "affected", -and **that decision belongs in one guard before any pattern runs**, not inside -each flag's branch. +No usable file list means attribution is impossible — a PR with no files, or a +`git diff` that failed on unresolved base/head SHAs. Every flag must then report +"affected", and **that decision belongs in one guard before any pattern runs**, +not inside each flag's branch. + +"No usable list" covers both cases, and the failure one is the easier to miss. +`run:` is `bash -e`, so a non-zero `git diff` aborts the step before any guard — +and a failed `detect-changes` leaves every downstream `needs:` unsatisfied, which +**skips** the matrix instead of running it. That is fail-closed, the opposite of +the intent. Let only the exit status decide and fold a failure into the empty +case: + +```bash +if ! FILES=$(git diff --name-only "$BASE"..."$HEAD"); then + FILES="" +fi +``` Deciding it per-flag is how the axes drift apart. The emptiness test once lived only inside the `non_code_only` branch, so an empty diff left that flag `false` diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f894a9e9c..e8df254128 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -107,7 +107,7 @@ jobs: # ---------- Unit tests (no hardware, Python + C++) ---------- ut: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.non_code_only != 'true' + if: needs.detect-changes.outputs.non_code_only != 'true' && needs.detect-changes.outputs.ut_affected == 'true' runs-on: ${{ matrix.os }} timeout-minutes: 15 strategy: @@ -163,7 +163,7 @@ jobs: # ---------- Simulation scene tests ---------- st-sim-a2a3: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.a2a3_changed == 'true' + if: needs.detect-changes.outputs.a2a3_changed == 'true' && needs.detect-changes.outputs.st_affected == 'true' runs-on: ${{ matrix.os }} timeout-minutes: 30 env: @@ -257,7 +257,7 @@ jobs: st-sim-a5: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.a5_changed == 'true' + if: needs.detect-changes.outputs.a5_changed == 'true' && needs.detect-changes.outputs.st_affected == 'true' runs-on: ${{ matrix.os }} timeout-minutes: 30 env: @@ -375,7 +375,7 @@ jobs: # corresponding profiling block — no cross-contamination from cmake cache. profiling-flags-smoke: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.a2a3_changed == 'true' || needs.detect-changes.outputs.a5_changed == 'true' + if: (needs.detect-changes.outputs.a2a3_changed == 'true' || needs.detect-changes.outputs.a5_changed == 'true') && needs.detect-changes.outputs.st_affected == 'true' runs-on: ubuntu-latest timeout-minutes: 30 @@ -473,7 +473,7 @@ jobs: # ---------- Unit tests (a2a3 hardware, Python + C++) ---------- ut-a2a3: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.non_code_only != 'true' + if: needs.detect-changes.outputs.non_code_only != 'true' && needs.detect-changes.outputs.ut_affected == 'true' runs-on: [self-hosted, a2a3] timeout-minutes: 30 env: @@ -559,7 +559,7 @@ jobs: # ---------- Scene tests (a2a3 hardware) ---------- st-onboard-a2a3: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.a2a3_changed == 'true' + if: needs.detect-changes.outputs.a2a3_changed == 'true' && needs.detect-changes.outputs.st_affected == 'true' runs-on: [self-hosted, a2a3] timeout-minutes: 60 env: @@ -733,6 +733,8 @@ jobs: a2a3_changed: ${{ steps.check.outputs.a2a3_changed }} a5_changed: ${{ steps.check.outputs.a5_changed }} non_code_only: ${{ steps.check.outputs.non_code_only }} + st_affected: ${{ steps.check.outputs.st_affected }} + ut_affected: ${{ steps.check.outputs.ut_affected }} steps: - name: Checkout repository uses: actions/checkout@v5 @@ -741,9 +743,19 @@ jobs: - name: Check file changes id: check run: | - FILES=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}) + # `run:` is `bash -e`, so a failing `git diff` would abort the step + # before the guard below and take every output with it — and a + # `detect-changes` failure leaves downstream `needs:` unsatisfied, which + # SKIPS the matrix rather than running it. Let only the exit status + # decide, and turn a failure into an empty list so the fail-open guard + # handles both cases identically. + if ! FILES=$(git diff --name-only \ + ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}); then + echo "git diff failed (unresolved base/head SHA?); treating as unattributable" + FILES="" + fi - # Fail open, in ONE place. An empty diff means attribution is + # Fail open, in ONE place. No usable file list means attribution is # impossible — a PR with no files, or base/head SHAs that did not # resolve — so nothing below can be trusted and every flag must say # "affected". Deciding this per-flag is how the axes drifted apart: @@ -751,10 +763,12 @@ jobs: # only, which left it false (UT and packaging ran) while both arch # flags independently came out false too (scene tests skipped). if [ -z "$FILES" ]; then - echo "Empty diff: cannot attribute changes, running everything" + echo "No usable file list: cannot attribute changes, running everything" echo "non_code_only=false" >> "$GITHUB_OUTPUT" echo "a2a3_changed=true" >> "$GITHUB_OUTPUT" echo "a5_changed=true" >> "$GITHUB_OUTPUT" + echo "st_affected=true" >> "$GITHUB_OUTPUT" + echo "ut_affected=true" >> "$GITHUB_OUTPUT" exit 0 fi @@ -803,10 +817,38 @@ jobs: echo "All changes are a2a3-only or non-code; skipping a5" fi + # Test-category axis, same shape as the arch axis: a category is + # unaffected only when EVERY changed file belongs exclusively to the + # other one. Shared test infra (root conftest.py, pyproject.toml, + # simpler_setup/, tests/lint/) matches neither ST_ONLY nor UT_ONLY, so + # it correctly flips both. The two suites execute disjoint trees — + # `pytest examples tests/st` vs `pytest tests/ut` + the cpp ctest — + # and no unit test reads examples/ or tests/st/. + ST_ONLY='^(tests/st/|examples/)' + UT_ONLY='^tests/ut/' + + # Scene tests are irrelevant only when every change is UT-exclusive. + ST_REMAINING=$(echo "$FILES" | grep -vE "$UT_ONLY" | grep -vE "$NON_CODE" || true) + if [ -n "$ST_REMAINING" ]; then + echo "st_affected=true" >> "$GITHUB_OUTPUT" + else + echo "st_affected=false" >> "$GITHUB_OUTPUT" + echo "All changes are unit-test-only or non-code; skipping scene tests" + fi + + # Unit tests are irrelevant only when every change is ST-exclusive. + UT_REMAINING=$(echo "$FILES" | grep -vE "$ST_ONLY" | grep -vE "$NON_CODE" || true) + if [ -n "$UT_REMAINING" ]; then + echo "ut_affected=true" >> "$GITHUB_OUTPUT" + else + echo "ut_affected=false" >> "$GITHUB_OUTPUT" + echo "All changes are scene-test/example-only or non-code; skipping unit tests" + fi + # ---------- Unit tests (a5 hardware, Python + C++) ---------- ut-a5: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.non_code_only != 'true' + if: needs.detect-changes.outputs.non_code_only != 'true' && needs.detect-changes.outputs.ut_affected == 'true' runs-on: [self-hosted, a5] timeout-minutes: 30 env: @@ -876,7 +918,7 @@ jobs: st-onboard-a5: needs: [detect-changes, pre-commit] - if: needs.detect-changes.outputs.a5_changed == 'true' + if: needs.detect-changes.outputs.a5_changed == 'true' && needs.detect-changes.outputs.st_affected == 'true' runs-on: [self-hosted, a5] timeout-minutes: 60 env: diff --git a/docs/ci.md b/docs/ci.md index e967678c66..a565d7f87d 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -10,7 +10,7 @@ Design principles: 2. **Runner matches hardware tier** — no-hardware tests run on `ubuntu-latest`; platform-specific tests run on self-hosted runners with the matching label (`a2a3`, `a5`). 3. **`--platform` is the only filter** — pytest uses `--platform` + the `requires_hardware` marker; ctest uses label `-LE` exclusion. No `-m st`, no `-m "not requires_hardware"`. 4. **sim = no hardware** — `a2a3sim`/`a5sim` jobs run on github-hosted runners alongside unit tests. -5. **Skip irrelevant platforms for scene tests** — `detect-changes` gates `st-sim-*` and `st-onboard-*` so pure-a5 PRs skip a2a3 scene-test runs and vice versa. **UT jobs (`ut`, `ut-a2a3`, `ut-a5`) are not gated by platform** — unit tests cover shared contracts and the cost of a falsely-skipped regression outweighs the savings. +5. **Skip irrelevant platforms, and irrelevant suites** — `detect-changes` gates `st-sim-*` and `st-onboard-*` by platform, so pure-a5 PRs skip a2a3 scene-test runs and vice versa. **UT jobs are still not gated by platform** — unit tests cover shared contracts and the cost of a falsely-skipped regression outweighs the savings. They *are* gated by test category: a diff confined to `tests/st/` or `examples/` cannot break a unit test, and one confined to `tests/ut/` cannot break a scene test, because the two suites execute disjoint trees (`pytest examples tests/st` vs `pytest tests/ut` plus the C++ ctest) and no unit test reads `examples/` or `tests/st/`. Shared test infrastructure — the root `conftest.py`, `pyproject.toml`, `simpler_setup/`, `tests/lint/` — belongs to neither category and runs both. 6. **Non-code PRs run pre-commit and docs, and nothing else** — `detect-changes` sets `non_code_only` when *no* changed file falls outside the `NON_CODE` set. Skipping the UT jobs is riskless there because nothing in that set can change what the code does, and each member already has its own gate: markdownlint inside `pre-commit` reads the markdown, `docs.yml` (unconditional on every PR) builds the site with `--strict`, and `pre-commit` itself is ungated so a `.pre-commit-config.yaml` change is fully exercised by it. ## Full Job Matrix @@ -28,14 +28,14 @@ The complete test-type × hardware-tier matrix. Empty cells have no tests yet; o PullRequest ├── pre-commit (ubuntu-latest) ├── packaging-matrix (ubuntu + macOS) — [skipped iff non_code_only] - ├── ut (ubuntu + macOS) — Python + C++ UT, no hardware [skipped iff non_code_only] - ├── detect-changes (ubuntu-latest) — outputs a{2a3,5}_changed + non_code_only - ├── st-sim-a2a3 (ubuntu + macOS) — gated by a2a3_changed - ├── st-sim-a5 (ubuntu + macOS) — gated by a5_changed - ├── ut-a2a3 (a2a3 self-hosted) — Python + C++ UT, a2a3 hardware [skipped iff non_code_only] - ├── st-onboard-a2a3 (a2a3 self-hosted) — gated by a2a3_changed - ├── ut-a5 (a5 self-hosted) — Python + C++ UT, a5 hardware [skipped iff non_code_only] - └── st-onboard-a5 (a5 self-hosted) — gated by a5_changed + ├── ut (ubuntu + macOS) — Python + C++ UT, no hardware [needs ut_affected] + ├── detect-changes (ubuntu-latest) — outputs non_code_only, a{2a3,5}_changed, {st,ut}_affected + ├── st-sim-a2a3 (ubuntu + macOS) — a2a3_changed && st_affected + ├── st-sim-a5 (ubuntu + macOS) — a5_changed && st_affected + ├── ut-a2a3 (a2a3 self-hosted) — Python + C++ UT, a2a3 hardware [needs ut_affected] + ├── st-onboard-a2a3 (a2a3 self-hosted) — a2a3_changed && st_affected + ├── ut-a5 (a5 self-hosted) — Python + C++ UT, a5 hardware [needs ut_affected] + └── st-onboard-a5 (a5 self-hosted) — a5_changed && st_affected ``` | Job | Runner | What it runs | @@ -129,9 +129,10 @@ not need `--max-parallel` manually. - Sim scene tests and no-hardware unit tests run on github-hosted runners (no hardware). - `detect-changes` computes three flags (`a2a3_changed`, `a5_changed`, `non_code_only`) from the PR diff, **all three derived from one `NON_CODE` set**: `docs/`, `.docs/`, `.claude/`, `mkdocs.yml`, `.github/workflows/docs.yml`, `.gitignore`, `.pre-commit-config.yaml`, and any `*.md` file anywhere. Membership follows a file's *effect*, not its path — `mkdocs.yml` and `docs.yml` are docs tooling that happens to live outside `docs/`. An arch flag is `false` only when every changed file is in the opposite platform's tree (`src/{arch}/`, `examples/{arch}/`, `tests/{st,ut/cpp}/{arch}/`) or in `NON_CODE`. Anything else — shared C++ (`src/common/`), Python (`python/`, `simpler_setup/`), build files (`CMakeLists.txt`, `pyproject.toml`), shared test infra (`tests/ut/py/`, `tests/lint/`), tooling (`tools/`), or **`.github/workflows/ci.yml` itself** — flips both flags to `true`. `ci.yml` is deliberately excluded from `NON_CODE`: a change to the gates must run everything, including whatever it just switched off. -- **Gated jobs (scene tests only):** `st-sim-{a2a3,a5}`, `st-onboard-{a2a3,a5}` run iff their platform's flag is `true`. -- **Platform-independent jobs (all UT + packaging):** `ut`, `ut-a2a3`, `ut-a5`, `packaging-matrix` ignore the platform flags — unit tests exercise shared contracts (nanobind bindings, RuntimeBuilder, ring buffers, etc.) and the risk of silently skipping a regression outweighs the CI minutes saved. The `tests/ut/cpp/{arch}/` entry in the gating regex only *attributes* an arch-specific C++ UT change to that platform (so it does not spuriously flip the other arch's scene-test flag); it does not gate the UT jobs themselves. -- **`non_code_only` is the same `NON_CODE` set, not a narrower one.** It is `true` when no changed file falls outside it. Nothing in the set can change what the code does, and no workflow consumes any of it beyond its own gate: `pre-commit` is ungated so it always exercises `.pre-commit-config.yaml`, `docs.yml` is unconditional on every PR so it always exercises `mkdocs.yml` / `docs/`, and **no workflow invokes anything under `.claude/`** (`grep -rn '\.claude' .github/workflows/` finds only comments). An **empty diff short-circuits the whole step**: attribution is impossible, so a single guard sets `non_code_only=false` and both arch flags `true` and returns, running the full matrix. That guard is deliberately one place — testing emptiness per flag is what previously left `non_code_only` false while both arch flags also came out false, running UT and packaging but skipping every scene test. +- **Test-category axis:** `ST_ONLY='^(tests/st/|examples/)'` and `UT_ONLY='^tests/ut/'`, applied in the same shape as the arch patterns — a category is unaffected only when *every* changed file is exclusively the other's. `st_affected` gates the four scene-test jobs and `profiling-flags-smoke`; `ut_affected` gates `ut`, `ut-a2a3`, `ut-a5`. Anything belonging to neither (root `conftest.py`, `pyproject.toml`, `simpler_setup/`, `tests/lint/`) flips both, so shared infrastructure always runs both suites. +- **Gated jobs (scene tests):** `st-sim-{a2a3,a5}`, `st-onboard-{a2a3,a5}` run iff their platform's flag **and** `st_affected` are `true`. +- **Platform-independent jobs (all UT + packaging):** `ut`, `ut-a2a3`, `ut-a5`, `packaging-matrix` ignore the *platform* flags — unit tests exercise shared contracts (nanobind bindings, RuntimeBuilder, ring buffers, etc.) and the risk of silently skipping a regression outweighs the CI minutes saved. The `tests/ut/cpp/{arch}/` entry in the gating regex only *attributes* an arch-specific C++ UT change to that platform (so it does not spuriously flip the other arch's scene-test flag); it does not gate the UT jobs themselves. The three UT jobs do respect `ut_affected`, which is a statement about test category rather than silicon. `packaging-matrix` stays on `non_code_only` alone: neither `tests/` nor `examples/` ships in the wheel (`wheel.packages = ["simpler_setup", "python/simpler"]`), so it over-runs slightly on a tests-only diff — cheap, GitHub-hosted, and not worth a fourth condition. +- **`non_code_only` is the same `NON_CODE` set, not a narrower one.** It is `true` when no changed file falls outside it. Nothing in the set can change what the code does, and no workflow consumes any of it beyond its own gate: `pre-commit` is ungated so it always exercises `.pre-commit-config.yaml`, `docs.yml` is unconditional on every PR so it always exercises `mkdocs.yml` / `docs/`, and **no workflow invokes anything under `.claude/`** (`grep -rn '\.claude' .github/workflows/` finds only comments). An **empty diff short-circuits the whole step**: attribution is impossible, so a single guard sets `non_code_only=false` and every arch and category flag `true`, then returns — running the full matrix. That guard is deliberately one place; testing emptiness per flag is what previously left `non_code_only` false while both arch flags also came out false, running UT and packaging but skipping every scene test. The arch flags subtract `NON_CODE` before deciding, so a non-code-only change already makes both `false`. An arch-gated job therefore needs no separate non-code check. See [`.claude/rules/ci-change-detection.md`](../.claude/rules/ci-change-detection.md) for the invariants these gates must keep.