From a1b3763191eec8f663a0a36f4206496a3104e3fc Mon Sep 17 00:00:00 2001 From: John Date: Mon, 17 Aug 2026 16:13:03 -0400 Subject: [PATCH 1/6] docs(tests): align unit-test docs with the co-located layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit test source moved into /test/ and is collected from colcon_unit_test_packages.yaml, but the surrounding documentation still described the mirror-directory-and-proxy scheme that replaced. Six per-layer stubs under tests/robot/ told authors to add tests in directories tests no longer live in, and tests/sim/motive_emulator/README.md proposed a NatNet emulator that was built at simulation/isaac-sim/extensions/optitrack.natnet.emulator/ instead. Remove them and rewrite the two tree READMEs as signposts. Correct the add-unit-tests and run-system-tests skills, which future agents read to work in this area, on four points they had wrong: - Running them. `pytest tests/` does not collect co-located unit tests — the injection in conftest.pytest_configure is skipped whenever a path is given on the command line. It reports "no tests collected" and exits 5, which reads as a failure but means nothing ran. `airstack test -m unit` and `cd tests && pytest -m unit` are the working forms; verified 155 passed vs exit 5. - CI. No workflow runs unit tests. system-tests.yml invokes `pytest tests/`, and fires only on PR-open, /pytest, or workflow_dispatch. - The mark. pytest_itemcollected applies @pytest.mark.unit by file location, so test sources should not declare it. The skill previously said "always decorate", which is where the redundant declarations came from. - colcon. It runs only what a package's CMakeLists registers. natnet_ros2 has ament_add_gtest but no ament_add_pytest_test, so its Python tests run only under the root harness. Also fixes a pytest_args example that would silently do nothing (`-m not linter`; ament's pytest runner ignores -m via PYTEST_ADDOPTS, and the real value is []), and the same stale layout claim in the testing docs and the emulator README. Co-Authored-By: Claude Opus 5 --- .agents/skills/add-unit-tests/SKILL.md | 71 ++++++++++++------- .agents/skills/run-system-tests/SKILL.md | 9 +-- .env | 2 +- CHANGELOG.md | 5 ++ .../development/intermediate/testing/ci_cd.md | 2 +- .../development/intermediate/testing/index.md | 13 ++-- .../intermediate/testing/unit_testing.md | 32 +++++---- .../optitrack.natnet.emulator/README.md | 6 +- tests/integration/natnet/README.md | 2 +- tests/robot/README.md | 10 +-- tests/robot/behavior/README.md | 3 - tests/robot/global/README.md | 3 - tests/robot/interface/README.md | 3 - tests/robot/local/README.md | 3 - tests/robot/perception/README.md | 3 - tests/robot/sensors/README.md | 4 -- tests/sim/README.md | 24 ++++--- tests/sim/motive_emulator/README.md | 61 ---------------- 18 files changed, 104 insertions(+), 152 deletions(-) delete mode 100644 tests/robot/behavior/README.md delete mode 100644 tests/robot/global/README.md delete mode 100644 tests/robot/interface/README.md delete mode 100644 tests/robot/local/README.md delete mode 100644 tests/robot/perception/README.md delete mode 100644 tests/robot/sensors/README.md delete mode 100644 tests/sim/motive_emulator/README.md diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index d49a36d73..986457fd6 100644 --- a/.agents/skills/add-unit-tests/SKILL.md +++ b/.agents/skills/add-unit-tests/SKILL.md @@ -1,6 +1,6 @@ --- name: add-unit-tests -description: Add Python or C++ unit tests to an AirStack ROS 2 package. Covers the co-location pattern (test source in package/test/), registering the package in colcon_unit_test_packages.yaml so pytest tests/ and airstack test -m unit collect it, and how to extend to sim and GCS modules. +description: Add Python or C++ unit tests to an AirStack ROS 2 package. Covers the co-location pattern (test source in package/test/), registering the package in colcon_unit_test_packages.yaml so airstack test -m unit collects it, and how to extend to sim components. license: MIT metadata: author: AirLab CMU @@ -15,8 +15,8 @@ Use this skill when: - Adding Python unit tests for a ROS 2 package (perception, sensors, local, global, behavior, interface) - Adding C++ unit tests (`gtest`) to a package already using `ament_cmake` -- Extending unit tests to sim-side Python (`tests/sim/`) or GCS modules (`tests/gcs/`) -- Verifying that `airstack test -m unit` and `pytest tests/` (CI) pick up your new tests +- Extending unit tests to sim-side Python (`simulation/**//test/`) +- Verifying that `airstack test -m unit` picks up your new tests For system tests (full Docker stack, sim, sensors, takeoff/hover/land) see the `run-system-tests` skill instead. @@ -24,8 +24,8 @@ For system tests (full Docker stack, sim, sensors, takeoff/hover/land) see the ## Architecture Overview Unit test **source lives co-located with its package** (ROS 2 / colcon convention). -`tests/colcon_unit_test_packages.yaml` lists which packages have unit tests, and -`pytest tests/` collects them from there — you only edit files under the package itself. +`tests/colcon_unit_test_packages.yaml` lists which packages have unit tests, and the root +harness collects them from there — you only edit files under the package itself. ``` robot/ros_ws/src/// @@ -47,10 +47,17 @@ auto-tagged `@pytest.mark.unit` by path, so `-m unit` selects it. ament lint fil | Invocation | What runs | |---|---| -| `pytest tests/ -m unit` | Package `test/test_*.py`, collected directly from source | -| `airstack test -m unit` | Same path | -| CI `system-tests.yml` (PR open / approved) | Same path via `pytest tests/` | -| `colcon test --packages-select ` | Real test in `package/test/` (incl. linters + C++) | +| `airstack test -m unit` | Package `test/test_*.py`, collected directly from source | +| `cd tests && pytest -m unit` | Same path — the containerless equivalent | +| `pytest tests/ -m unit` | **Nothing.** See the warning below | +| `colcon test --packages-select ` | Only what the package's `CMakeLists.txt` registers (C++ gtests, linters) | + +> **`pytest tests/` does not collect unit tests.** The injection in +> `tests/conftest.py::pytest_configure` is skipped whenever a path is given on the command +> line, and `tests/` is a path. The run reports `no tests collected` and exits **5**, which +> looks like a failure but means the tests never ran. Use `airstack test -m unit`, or +> `cd tests` first so `testpaths` applies. This also means **no CI workflow currently runs +> unit tests** — `system-tests.yml` invokes `pytest tests/`. ## Step-by-Step: Adding a Python Unit Test @@ -87,13 +94,17 @@ if str(_src) not in sys.path: from my_module import my_function # noqa: E402 -@pytest.mark.unit def test_my_function_basic(): assert my_function(1, 2) == 3 ``` **Key points:** -- Always decorate with `@pytest.mark.unit` — this is the filter for fast runs. +- **Do not write `@pytest.mark.unit`.** `pytest_itemcollected` in `tests/conftest.py` + applies it by file location to everything under a registered package's `test/` dir. + Writing it by hand is redundant, and it warns (`PytestUnknownMarkWarning`) under any + invocation where `tests/pytest.ini` is not the configfile — e.g. `colcon test`. +- Import `pytest` only if you need its API (`approx`, `raises`, `parametrize`, + `importorskip`). - Compute paths relative to `__file__` (`parent.parent / "src"`) — never hardcode absolute paths. - For packages with a Python module directory (`//`), add the package @@ -127,35 +138,41 @@ robot: - natnet_ros2 - lidar_point_cloud_filter - # ← add here - pytest_args: "-m not linter" + pytest_args: [] ``` +Leave `pytest_args` empty. It is forwarded to `colcon test` via `PYTEST_ADDOPTS`, and +ament's pytest runner ignores `-m` there — a marker expression in this field silently +does nothing. + That's the whole registration. `conftest.py` globs `robot/ros_ws/src/**//test`, collects its non-linter `test_*.py`, and marks them `unit`. The test file must be self-contained: if it imports package code, set up `sys.path` at the top of the test file (see `test_validation_core.py`, which inserts its package root). Same YAML, different workspace key (`sim:`), for Isaac-extension unit tests. -### 5. Run locally to verify +### 4. Run locally to verify ```bash -# From repo root — no container needed -cd tests -pytest -m unit -v -# or airstack test -m unit -v +# or, containerless — note the `cd`, it is load-bearing: +cd tests && pytest -m unit -v ``` -All 14+ existing tests plus your new ones should pass. Collected items point straight +All 155 existing tests plus your new ones should pass. Collected items point straight at the co-located source: ``` ../robot/ros_ws/src///test/test_.py::test_my_function_basic PASSED ``` -### 6. CI picks it up automatically +If you see `no tests collected` and exit code 5, you ran `pytest tests/` — see the +warning in *Architecture Overview*. + +### 5. Running in CI -Unit tests are discovered by `pytest tests/` and run as part of `system-tests.yml` -(triggered on PR open) — no changes to CI needed. +There is currently **no CI workflow that runs unit tests.** `system-tests.yml` invokes +`pytest tests/`, which does not collect them, and it only triggers on PR-open, a +`/pytest` comment, or `workflow_dispatch`. Run them locally before pushing. --- @@ -242,11 +259,11 @@ sim: | Where does test source live? | `/…//test/` (co-located with the package) | | Where does pytest discover tests? | From the package `test/` dir listed in `colcon_unit_test_packages.yaml` | | How are duplicate basenames handled? | `--import-mode=importlib` (set in `pytest.ini`) | -| What mark do all unit tests use? | `@pytest.mark.unit` (auto-applied by path in `conftest.py`) | -| What CI workflow runs them? | `system-tests.yml` — runs `pytest tests/` which includes unit tests | -| When does that workflow trigger? | PR opened, `/pytest` comment, `workflow_dispatch` | +| What mark do all unit tests use? | `@pytest.mark.unit` — auto-applied by path in `conftest.py`; do not write it yourself | +| How do I run them? | `airstack test -m unit`, or `cd tests && pytest -m unit`. **Not** `pytest tests/` | +| What CI workflow runs them? | None today — see §5 | | Do system tests (`liveliness`, etc.) run too? | No — `-m unit` filters to hermetic tests only | -| Does `colcon test` also run these? | Yes — Python tests in `package/test/` are discovered by colcon's pytest runner | +| Does `colcon test` also run these? | Only if the package registers them. `ament_add_gtest` covers C++; a Python test needs `ament_add_pytest_test`, which `natnet_ros2` does **not** have — its Python tests run only under the root harness | | Can I add pure C++ gtests? | Yes — `ament_add_gtest` in CMakeLists.txt | ## Reference Implementations @@ -261,8 +278,8 @@ Both are collected from their package `test/` dir. ## Files to Know -- `.github/workflows/system-tests.yml` — CI workflow (runs `pytest tests/` including unit tests) -- `tests/pytest.ini` — mark registration + `--import-mode=importlib` +- `.airstack/modules/dev.sh` — what `airstack test` runs (bare `pytest` with `working_dir` `tests/`) +- `tests/pytest.ini` — mark registration + `--import-mode=importlib` + `testpaths` - `tests/colcon_unit_test_packages.yaml` — the package list driving unit-test collection - `tests/conftest.py` — `unit_test_files()` / `pytest_configure` inject package tests; `pytest_itemcollected` auto-marks `unit` - `tests/README.md` — full test harness reference diff --git a/.agents/skills/run-system-tests/SKILL.md b/.agents/skills/run-system-tests/SKILL.md index 22ce20520..d5a6a9bb2 100644 --- a/.agents/skills/run-system-tests/SKILL.md +++ b/.agents/skills/run-system-tests/SKILL.md @@ -33,8 +33,8 @@ The suite lives at `tests/` (repo root) and is fully pytest-based. Configuration | Concern | Unit (`-m unit`) | System (`-m liveliness` etc.) | |---|---|---| | Hardware required | None — pure Python | Docker daemon, NVIDIA GPU, sim license | -| CI workflow | `system-tests.yml` (included in `pytest tests/`) | `system-tests.yml` (GPU OpenStack VM) | -| Trigger | Every push + PR (automatic) | PR opened, `/pytest` comment, `workflow_dispatch` | +| CI workflow | None — run them locally before pushing | `system-tests.yml` (GPU OpenStack VM) | +| Trigger | n/a | PR opened, `/pytest` comment, `workflow_dispatch` | | Source location | `/test/test_*.py` (collected directly, listed in `colcon_unit_test_packages.yaml`) | `tests/system/` | | How to add | See `add-unit-tests` skill | See *Adding a New System Test* below | @@ -42,8 +42,9 @@ Run unit tests without any Docker stack: ```bash airstack test -m unit -v -# or -pytest tests/ -m unit -v # AIRSTACK_ROOT=$(pwd) for direct pytest +# or, containerless — the `cd` is load-bearing; `pytest tests/ -m unit` +# collects nothing (see the add-unit-tests skill): +cd tests && AIRSTACK_ROOT=$(git rev-parse --show-toplevel) pytest -m unit -v ``` For details on the co-located layout and adding new unit tests, see the diff --git a/.env b/.env index ea6070100..c97a01e5b 100644 --- a/.env +++ b/.env @@ -12,7 +12,7 @@ PROJECT_NAME="airstack" # If you've run ./airstack.sh setup, then this will auto-generate from the git commit hash every time a change is made # to a Dockerfile or docker-compose.yaml file. Otherwise this can also be set explicitly to make a release version. # auto-generated from git commit hash -VERSION="0.19.0-alpha.16" +VERSION="0.19.0-alpha.17" # Choose "dev" or "prebuilt". "dev" is for mounted code that must be built live. "prebuilt" is for built ros_ws baked into the image DOCKER_IMAGE_BUILD_MODE="dev" # Where to push and pull images from. Can replace with your docker hub username if using docker hub. diff --git a/CHANGELOG.md b/CHANGELOG.md index 15da258c6..7576f395c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Unit-test documentation now matches the co-located layout: the `add-unit-tests` and `run-system-tests` skills and the testing docs name `airstack test -m unit` (or `cd tests && pytest -m unit`) as the way to run unit tests, record that `pytest tests/` does not collect them, state that no CI workflow runs them today, and stop instructing authors to write `@pytest.mark.unit` by hand — `conftest.py` applies it by file location - Ephemeral CI GPU runners spawn via NVIDIA OSMO (not OpenStack); `system-tests.yml` / `docker-build.yml` still use `airstack-ephemeral` - Default system-test `--sim` is `isaacsim`; pass `--sim msairsim` to opt in to Microsoft AirSim - `-m build_packages` CI runs pull `cache_*` images instead of baking sim images @@ -33,6 +34,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `zed-l4t` image: ZED SDK 4.2 → 5.2 with the coupled ROS deps (`zed_msgs` 5.2.1, `point_cloud_transport(_plugins)` 4.x, add `backward_ros`) - Unit tests are defined by `tests/colcon_unit_test_packages.yaml`: `conftest.py` collects each listed package's co-located `test/` dir under `--import-mode=importlib` and marks it `unit` (ament lint files are skipped and run under `colcon test`) +### Removed + +- Pre-co-location unit-test scaffolding: the six per-layer stub READMEs under `tests/robot/` (which instructed authors to add tests in directories tests no longer live in) and `tests/sim/motive_emulator/README.md` (superseded by `simulation/isaac-sim/extensions/optitrack.natnet.emulator/` and `tests/integration/natnet/`) + ### Fixed - Isaac Sim image: PX4 `ubuntu.sh` no longer fails dpkg configure on the NVIDIA base (`ca-certificates` / `software-properties-common`); use `--no-nuttx --no-sim-tools` like ms-airsim diff --git a/docs/development/intermediate/testing/ci_cd.md b/docs/development/intermediate/testing/ci_cd.md index a53f5926d..8e618efaf 100644 --- a/docs/development/intermediate/testing/ci_cd.md +++ b/docs/development/intermediate/testing/ci_cd.md @@ -329,7 +329,7 @@ flowchart LR | Mark | Module | What it verifies | Bugs it is good at catching | |---|---|---|---| -| `unit` | `tests/robot/`, `tests/sim/` proxies | Hermetic Python/numpy logic co-located with each ROS 2 package | Off-by-one and boundary errors in filters, converters, validators; regressions in pure algorithm code | +| `unit` | `/test/` (co-located) | Hermetic Python/numpy logic co-located with each ROS 2 package | Off-by-one and boundary errors in filters, converters, validators; regressions in pure algorithm code | | `build_docker` | `system/test_build_docker.py` | Every image builds; records image sizes | Broken Dockerfiles, deleted apt packages, upstream base-image drift, accidental image bloat | | `build_packages` | `system/test_build_packages.py` | `colcon build` inside robot, GCS, and ms-airsim workspaces | Missing `package.xml` dependencies, uninstalled launch/config files, C++ breakage on a clean tree | | `liveliness` | `system/test_liveliness.py` | Containers reach Running, `/clock` publishes, tmux panes alive, sentinel ROS 2 nodes present, compute snapshot, stability poll | Launch files that crash on start, nodes that die after 30 s, `ROBOT_NAME`/domain-ID misconfiguration, runaway CPU or memory | diff --git a/docs/development/intermediate/testing/index.md b/docs/development/intermediate/testing/index.md index 07c5fb887..f3a6ddbc5 100644 --- a/docs/development/intermediate/testing/index.md +++ b/docs/development/intermediate/testing/index.md @@ -5,25 +5,24 @@ hardware requirement: | Layer | Where | Mark / Tool | Hardware | |---|---|---|---| -| **Unit tests** | `tests/robot/`, `tests/sim/` | `pytest -m unit` | None — pure Python | +| **Unit tests** | `/test/` (co-located) | `airstack test -m unit` | None — pure Python | | **Package tests** | `/test/` | `colcon test` | Robot container | | **System tests** | `tests/system/` | `pytest -m liveliness` etc. | Docker, GPU, sim license | -## Unit tests (`pytest -m unit`) +## Unit tests (`airstack test -m unit`) Fast, hermetic Python tests that run in seconds with no Docker or GPU. Test source lives **co-located with its ROS 2 package** (`/test/`); the packages with unit -tests are listed in `tests/colcon_unit_test_packages.yaml`, and `pytest tests/` collects +tests are listed in `tests/colcon_unit_test_packages.yaml`, and the root harness collects them from there. ```bash airstack test -m unit -v -# or directly: -pytest tests/ -m unit -v +# or containerless — the `cd` is load-bearing: +cd tests && pytest -m unit -v ``` -Unit tests run as part of `system-tests.yml` via `pytest tests/` and can also be -run locally with no Docker or GPU needed. +No CI workflow currently runs unit tests, so run them locally before pushing. → **[Unit Testing Guide](unit_testing.md)** — patterns, CI workflow, how to add tests for new packages (Python and C++ gtest). diff --git a/docs/development/intermediate/testing/unit_testing.md b/docs/development/intermediate/testing/unit_testing.md index dd2f72cf3..d1fa9168c 100644 --- a/docs/development/intermediate/testing/unit_testing.md +++ b/docs/development/intermediate/testing/unit_testing.md @@ -1,12 +1,12 @@ # Unit Testing -AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds and gate every pull request via a dedicated GitHub Actions workflow on a standard `ubuntu-latest` runner. +AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds via `airstack test -m unit`. No CI workflow runs them today, so run them yourself before pushing. ## Design principles - **Co-located with source.** Test files live in `/test/` alongside the code they test. This is the standard ROS 2 / colcon convention and ensures tests are discovered by both `colcon test` and `pytest`. - **Listed in one place.** `tests/colcon_unit_test_packages.yaml` lists which packages have unit tests. `tests/conftest.py` resolves each to its `test/` dir and collects the non-linter `test_*.py` files under `--import-mode=importlib`. To add a package's unit tests, list it in that YAML. -- **`@pytest.mark.unit` on every test.** Auto-applied by path in `conftest.py` (source files may also declare it). The `unit` mark keeps unit tests isolated from system tests that need Docker, GPUs, and sim licenses. +- **`@pytest.mark.unit` on every test, applied for you.** `conftest.py` marks items by file location, so test sources should not declare it themselves. The `unit` mark keeps unit tests isolated from system tests that need Docker, GPUs, and sim licenses. ## Repository layout @@ -35,24 +35,25 @@ Collected items point straight at the co-located source: # Locally — no container or Docker stack required airstack test -m unit -v -# Or directly with pytest (AIRSTACK_ROOT must point to the repo root) +# Or directly with pytest. The `cd` is load-bearing: pytest only injects the +# co-located tests when no path is given on the command line. export AIRSTACK_ROOT=$(pwd) -pip install pytest numpy -pytest tests/ -m unit -v +pip install -r tests/requirements.txt +cd tests && pytest -m unit -v ``` Unit tests complete in under one second for the current suite. ## CI -Unit tests are collected and run as part of `system-tests.yml` via `pytest tests/` -(no marks specified on PR open = all tests including `unit`). Run them locally at -any time with no infrastructure required: +No workflow runs unit tests today. `system-tests.yml` invokes `pytest tests/`, which +does not collect them, and it only triggers on PR open, a `/pytest` comment, or +`workflow_dispatch`. Run them locally before pushing — no infrastructure required: ```bash airstack test -m unit -v # or directly (requires tests/requirements.txt installed): -AIRSTACK_ROOT=$(pwd) pytest tests/ -m unit -v +cd tests && AIRSTACK_ROOT=$(git rev-parse --show-toplevel) pytest -m unit -v ``` ## Current test coverage @@ -73,7 +74,6 @@ AIRSTACK_ROOT=$(pwd) pytest tests/ -m unit -v # robot/ros_ws/src///test/test_my_module.py import sys from pathlib import Path -import pytest # Make the package importable without a colcon install _src = Path(__file__).resolve().parent.parent / "src" @@ -83,11 +83,13 @@ if str(_src) not in sys.path: from my_module import my_function # noqa: E402 -@pytest.mark.unit def test_basic(): assert my_function(1, 2) == 3 ``` +No `@pytest.mark.unit` — `conftest.py` applies it by file location. Import `pytest` +only if you need its API (`approx`, `raises`, `parametrize`, `importorskip`). + If the production code inherits from `rclpy.node.Node`, stub ROS at the import boundary: @@ -117,7 +119,7 @@ sys.modules["rclpy.node"] = _rclpy_node_mod robot: packages: - # ← add here; conftest.py collects /test/test_*.py - pytest_args: "-m not linter" + pytest_args: [] # forwarded to colcon via PYTEST_ADDOPTS; `-m` is ignored there ``` That's the whole registration. If the test imports package code, set up `sys.path` at the @@ -128,7 +130,7 @@ across packages don't collide. **3. Verify:** ```bash -pytest tests/ -m unit -v +airstack test -m unit -v ``` ### C++ (gtest) @@ -179,8 +181,8 @@ sim: - # → simulation/**//test collected directly ``` -`pytest tests/ -m unit` discovers them automatically — no changes to `pytest.ini` -or CI changes needed. +`airstack test -m unit` discovers them automatically — no changes to `pytest.ini` +needed. ## See also diff --git a/simulation/isaac-sim/extensions/optitrack.natnet.emulator/README.md b/simulation/isaac-sim/extensions/optitrack.natnet.emulator/README.md index 3fedb5ecb..cd822ac5f 100644 --- a/simulation/isaac-sim/extensions/optitrack.natnet.emulator/README.md +++ b/simulation/isaac-sim/extensions/optitrack.natnet.emulator/README.md @@ -15,7 +15,7 @@ optitrack.natnet.emulator/ ├── schema/schema.usda # Typed NatNet interface attribute definitions ├── setup.py ├── docs/ # (legacy design notes — see docs/simulation/isaac_sim/natnet_emulator.md) -├── test/ # Co-located unit tests (proxied by tests/sim/) +├── test/ # Co-located unit tests (listed in colcon_unit_test_packages.yaml) └── optitrack/natnet/emulator/ ├── defaults.py # Reference Drone → prim bindings for tests ├── server/ # NatNet UDP server (transport + protocol) @@ -144,11 +144,11 @@ Full handshake layouts and sniffing workflow: [optitrack-development skill](../. | Unit | `unit` | Serializers, protocol, config, USD authoring, catalog, pose sampling, server lifecycle, scene setup | | Integration | `integration` | Host emulator → robot `natnet_ros2` pose Hz | -Co-located tests live in `test/`. Pytest discovers them via thin proxies in [`tests/sim/optitrack_natnet_emulator/`](../../../../tests/sim/optitrack_natnet_emulator/). +Co-located tests live in `test/`. The root harness collects them via the `sim:` key in [`colcon_unit_test_packages.yaml`](../../../../tests/colcon_unit_test_packages.yaml). ```bash # Unit (no Docker / no SDK) -pytest tests/sim/optitrack_natnet_emulator/ -m unit -v +airstack test -m unit -v # Integration (robot container + NatNet SDK) pytest tests/integration/natnet/ -m integration -v diff --git a/tests/integration/natnet/README.md b/tests/integration/natnet/README.md index db4617312..f373e7d85 100644 --- a/tests/integration/natnet/README.md +++ b/tests/integration/natnet/README.md @@ -147,5 +147,5 @@ docker exec airstack-robot-desktop-1 bash -lc 'bws --packages-select natnet_ros2 Unit tests (protocol, serializers, Isaac wrapper loopback): ```bash -pytest tests/sim/optitrack_natnet_emulator/ -m unit -v +airstack test -m unit -v ``` diff --git a/tests/robot/README.md b/tests/robot/README.md index 3961d90cc..facecdc61 100644 --- a/tests/robot/README.md +++ b/tests/robot/README.md @@ -1,7 +1,7 @@ # Robot-side unit tests Unit-test **source is co-located** with each ROS 2 package (the standard colcon -convention) and is collected by `pytest tests/`: +convention): ``` robot/ros_ws/src///test/test_.py ← source of truth @@ -10,8 +10,10 @@ robot/ros_ws/src///test/test_.py ← source of truth [`../colcon_unit_test_packages.yaml`](../colcon_unit_test_packages.yaml) lists which packages have unit tests; `tests/conftest.py` resolves each to its `test/` dir and collects the non-linter `test_*.py` files under `--import-mode=importlib`, tagging each -`@pytest.mark.unit`. Both `airstack test -m unit` and `colcon test --packages-select ` -run the same source. +`@pytest.mark.unit` by path — you do not write the mark yourself. + +Run them with `airstack test -m unit`, or `cd tests && pytest -m unit`. C++ gtests in the +same `test/` dir run under `colcon test --packages-select `. To add a package's unit tests, list it under `robot.packages` in the YAML — see the -`add-unit-tests` agent skill. The per-layer subdirectories here hold only documentation. +`add-unit-tests` agent skill. diff --git a/tests/robot/behavior/README.md b/tests/robot/behavior/README.md deleted file mode 100644 index 713fd31f2..000000000 --- a/tests/robot/behavior/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# Unit tests — behavior layer - -Add `@pytest.mark.unit` tests for `robot/ros_ws/src/behavior/` packages here. diff --git a/tests/robot/global/README.md b/tests/robot/global/README.md deleted file mode 100644 index 280c41dec..000000000 --- a/tests/robot/global/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# Unit tests — global layer - -Add `@pytest.mark.unit` tests for `robot/ros_ws/src/global/` packages here. diff --git a/tests/robot/interface/README.md b/tests/robot/interface/README.md deleted file mode 100644 index ea4ee8b5c..000000000 --- a/tests/robot/interface/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# Unit tests — interface layer - -Add `@pytest.mark.unit` tests for `robot/ros_ws/src/interface/` packages here. diff --git a/tests/robot/local/README.md b/tests/robot/local/README.md deleted file mode 100644 index 118cc2071..000000000 --- a/tests/robot/local/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# Unit tests — local layer - -Add `@pytest.mark.unit` tests for `robot/ros_ws/src/local/` packages here. diff --git a/tests/robot/perception/README.md b/tests/robot/perception/README.md deleted file mode 100644 index 350ef9fb0..000000000 --- a/tests/robot/perception/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# Unit tests — perception layer - -Add `@pytest.mark.unit` tests for `robot/ros_ws/src/perception/` packages here. diff --git a/tests/robot/sensors/README.md b/tests/robot/sensors/README.md deleted file mode 100644 index 8a44129eb..000000000 --- a/tests/robot/sensors/README.md +++ /dev/null @@ -1,4 +0,0 @@ -# Unit tests — sensors layer - -Package-specific folders (for example `lidar_point_cloud_filter/`) mirror -`robot/ros_ws/src/sensors//`. diff --git a/tests/sim/README.md b/tests/sim/README.md index 09f45f6a6..46344d94b 100644 --- a/tests/sim/README.md +++ b/tests/sim/README.md @@ -1,14 +1,20 @@ # Simulation-side unit tests -Tests for **simulation components** that are not part of the onboard ROS workspace -(for example an OptiTrack Motive / NatNet emulator, Isaac launch helpers, or -AirSim bridge utilities). +Unit-test **source is co-located** with each simulation component, the same way the +robot workspace works: -Mark fast, hermetic checks with `@pytest.mark.unit`. Tests that require a GPU, -full sim, or Docker belong in [`tests/system/`](../system/) instead. +``` +simulation/**//test/test_.py ← source of truth +``` -Suggested layout: +[`../colcon_unit_test_packages.yaml`](../colcon_unit_test_packages.yaml) lists which +components have unit tests, under the `sim:` key; `tests/conftest.py` resolves each to +its `test/` dir and tags the collected items `@pytest.mark.unit` by path. -| Directory | Purpose | -|-----------|---------| -| `motive_emulator/` | Motive / NatNet protocol emulation / parsing | +Run them with `airstack test -m unit`, or `cd tests && pytest -m unit`. These components +are not part of the onboard ROS workspace, so `colcon test` does not run them. + +Tests needing a GPU, a full sim, or Docker belong in [`../system/`](../system/) instead. + +Currently listed: `optitrack.natnet.emulator` +([source](../../simulation/isaac-sim/extensions/optitrack.natnet.emulator/)). diff --git a/tests/sim/motive_emulator/README.md b/tests/sim/motive_emulator/README.md deleted file mode 100644 index 0e682c448..000000000 --- a/tests/sim/motive_emulator/README.md +++ /dev/null @@ -1,61 +0,0 @@ -# Motive / NatNet Emulator - -This directory is the future home of **integration tests** that drive a real -NatNet wire-protocol mock server against `natnet_ros2_node`. - -## Why here, not in the package test/ dir? - -Unit tests for pure logic live in -`tests/robot/perception/natnet_ros2/test_natnet_logic.cpp` and run via `colcon -test` with no network or SDK required (uses `FakeNatNetClient`). - -The emulator tests here will require an actual UDP server that speaks the NatNet -protocol, so they belong in the `sensors` mark of the system test suite alongside -other topic-streaming tests. - -## Planned implementation - -The mock server should: - -1. Open a UDP socket on the NatNet command port (default 1510). -2. Respond to `NAT_CONNECT` (message type 0) with a `NAT_SERVERINFO` (type 1) - packet containing a canned `sServerDescription`. -3. Respond to `NAT_REQUEST_MODELDEF` (type 4) with a `NAT_MODELDEF` (type 5) - packet describing one or more rigid bodies. -4. Stream `NAT_FRAMEOFDATA` (type 7) packets to the client's data port at a - configurable rate with synthetic pose data. - -### Reference - -The NatNet wire format is documented in the NatNet SDK developer notes and the -`PacketClient` example shipped with the SDK (available inside the robot Docker -container after `airstack setup --natnet`). - -## Relationship to `FakeNatNetClient` - -``` - ┌──────────────────────────────────────┐ - │ Test boundary │ - colcon gtest │ FakeNatNetClient (in-process) │ ← unit tests (no network) - │ test_natnet_logic.cpp │ - └──────────────────────────────────────┘ - - ┌──────────────────────────────────────┐ - │ Network boundary │ - pytest sensors │ MotiveEmulator (UDP server, Python) │ ← integration tests - │ NatNetClientAdapter → NatNetClient │ - │ natnet_ros2_node (full ROS node) │ - └──────────────────────────────────────┘ -``` - -The `FakeNatNetClient` seam (already implemented) lets unit tests verify all -connection-outcome logic paths. The emulator here will verify the full -end-to-end path including the NatNet SDK's own parser. - -## When to add this - -Implement the emulator when: -- The OptiTrack emulator service is placed under `simulation/optitrack-emulator/` - or `tests/sim/motive_emulator/` -- The `sensors` test mark is extended to include `natnet_ros2` topic checks -- CI has access to the robot container with the NatNet SDK installed From 30f0a79d209b3a4ddaf497367f2884b9a7882868 Mon Sep 17 00:00:00 2001 From: John Date: Mon, 17 Aug 2026 18:01:18 -0400 Subject: [PATCH 2/6] docs(tests): record how C++ and Python unit tests reach CI C++ gtests run under colcon test, which CI executes inside the robot container via the build_packages mark (test_build_packages.py::test_colcon_test_robot). Python unit tests run under the root harness, which no workflow invokes. Whether colcon test also picks up a package's Python tests depends on its build type: lidar_point_cloud_filter is ament_python and exposes them via setup.cfg (testpaths = test), so they run in both places; natnet_ros2 is ament_cmake and registers only ament_add_gtest, so its Python tests run nowhere in CI. Co-Authored-By: Claude Opus 5 --- .agents/skills/add-unit-tests/SKILL.md | 16 +++++++++++++- .../intermediate/testing/unit_testing.md | 22 ++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index 986457fd6..a89decbf6 100644 --- a/.agents/skills/add-unit-tests/SKILL.md +++ b/.agents/skills/add-unit-tests/SKILL.md @@ -50,7 +50,21 @@ auto-tagged `@pytest.mark.unit` by path, so `-m unit` selects it. ament lint fil | `airstack test -m unit` | Package `test/test_*.py`, collected directly from source | | `cd tests && pytest -m unit` | Same path — the containerless equivalent | | `pytest tests/ -m unit` | **Nothing.** See the warning below | -| `colcon test --packages-select ` | Only what the package's `CMakeLists.txt` registers (C++ gtests, linters) | +| `colcon test --packages-select ` | C++ gtests and linters; Python only for `ament_python` packages (see below) | + +**Two runners, split by language.** C++ gtests run only under `colcon test`, which CI +executes inside the robot container via the **`build_packages`** mark +(`tests/system/test_build_packages.py::test_colcon_test_robot`). Python unit tests run +under the root harness described above. Whether `colcon test` *also* picks up a package's +Python tests depends on its build type: + +| Package | Build type | Python tests under `colcon test` | +|---|---|---| +| `natnet_ros2` | `ament_cmake` | **No** — `CMakeLists.txt` registers `ament_add_gtest` but no `ament_add_pytest_test` | +| `lidar_point_cloud_filter` | `ament_python` | **Yes** — `setup.cfg` sets `testpaths = test`, so colcon's pytest runner finds them | + +So a Python test in an `ament_cmake` package runs *only* via the root harness, and today +that means only when someone runs it locally. > **`pytest tests/` does not collect unit tests.** The injection in > `tests/conftest.py::pytest_configure` is skipped whenever a path is given on the command diff --git a/docs/development/intermediate/testing/unit_testing.md b/docs/development/intermediate/testing/unit_testing.md index d1fa9168c..ffc8aada8 100644 --- a/docs/development/intermediate/testing/unit_testing.md +++ b/docs/development/intermediate/testing/unit_testing.md @@ -46,9 +46,25 @@ Unit tests complete in under one second for the current suite. ## CI -No workflow runs unit tests today. `system-tests.yml` invokes `pytest tests/`, which -does not collect them, and it only triggers on PR open, a `/pytest` comment, or -`workflow_dispatch`. Run them locally before pushing — no infrastructure required: +**C++ gtests are gated; Python unit tests are not.** The two languages take different +runners: + +| Test | Runner | In CI | +|---|---|---| +| C++ gtest | `colcon test` inside the robot container | Yes — the `build_packages` mark (`tests/system/test_build_packages.py::test_colcon_test_robot`) | +| Python, `ament_python` package | root harness **and** `colcon test` | Via `build_packages` only | +| Python, `ament_cmake` package | root harness only | No | + +`colcon test` picks up Python tests only when the package's build type makes it: an +`ament_python` package like `lidar_point_cloud_filter` exposes them through +`setup.cfg` (`testpaths = test`), while an `ament_cmake` package like `natnet_ros2` +would need an explicit `ament_add_pytest_test` — it has none, so its Python tests run +nowhere in CI. + +No workflow runs the Python unit tests directly. `system-tests.yml` invokes +`pytest tests/`, which does not collect them, and it only triggers on PR open, a +`/pytest` comment, or `workflow_dispatch`. Run them locally before pushing — no +infrastructure required: ```bash airstack test -m unit -v From a615a2466e95133e85998c2f1c5b7c6a28fd6de0 Mon Sep 17 00:00:00 2001 From: John Date: Mon, 17 Aug 2026 18:14:22 -0400 Subject: [PATCH 3/6] fix(tests): collect co-located unit tests when the run is not narrowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit-test source lives outside tests/, so pytest_configure appends it to the collection args. That injection was gated on args_source != ARGS, which pytest sets for any positional path — including `tests/`. The intent was that `pytest tests/system/foo.py` should not drag in 155 unrelated tests, but the guard could not tell narrowing from naming the whole suite, so CI's `pytest tests/` collected 97 of 252 items and the Python unit tests ran nowhere. Decide on the paths instead: a positional is broad when it names tests/ itself or an ancestor, narrow otherwise. `pytest tests/` and `pytest .` inject; `pytest tests/system`, a single file, and a node id do not. Node ids are split on `::` first, since only the part before it addresses the filesystem. `any` rather than `all` is deliberate — pytest_configure appends the co-located files (narrow, absolute) to config.args, so `all` would flip the answer for anything re-deriving it after that mutation. The decision is also stashed on config for the contract test to read. tests/meta/test_collection_contract.py pins the behaviour: a table over broad/narrow invocations, a check that the command in system-tests.yml is classified broad (the test that would have caught this), and a check that every discovered file produced collected items. It lives under tests/ on purpose — co-located, it would stop being collected at the same moment it stopped guarding anything. Verified: `pytest tests/ -m unit` 0 -> 170 passed; `cd tests && pytest -m unit` unchanged at 170; `pytest tests/system/test_liveliness.py` still collects 16. Unit tests now run with every system-tests.yml invocation. That workflow's triggers are unchanged and intentional — PR open, /pytest, workflow_dispatch — since the same run drives the GPU system tests. Co-Authored-By: Claude Opus 5 --- .agents/skills/add-unit-tests/SKILL.md | 36 ++++---- .agents/skills/run-system-tests/SKILL.md | 9 +- CHANGELOG.md | 3 +- .../development/intermediate/testing/index.md | 7 +- .../intermediate/testing/unit_testing.md | 36 ++++---- tests/conftest.py | 13 +-- tests/harness/__init__.py | 8 +- tests/harness/discovery.py | 48 +++++++++- tests/meta/test_collection_contract.py | 88 +++++++++++++++++++ 9 files changed, 190 insertions(+), 58 deletions(-) create mode 100644 tests/meta/test_collection_contract.py diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index a89decbf6..a60129370 100644 --- a/.agents/skills/add-unit-tests/SKILL.md +++ b/.agents/skills/add-unit-tests/SKILL.md @@ -1,6 +1,6 @@ --- name: add-unit-tests -description: Add Python or C++ unit tests to an AirStack ROS 2 package. Covers the co-location pattern (test source in package/test/), registering the package in colcon_unit_test_packages.yaml so airstack test -m unit collects it, and how to extend to sim components. +description: Add Python or C++ unit tests to an AirStack ROS 2 package. Covers the co-location pattern (test source in package/test/), registering the package in colcon_unit_test_packages.yaml so pytest tests/ and airstack test -m unit collect it, and how to extend to sim components. license: MIT metadata: author: AirLab CMU @@ -49,7 +49,7 @@ auto-tagged `@pytest.mark.unit` by path, so `-m unit` selects it. ament lint fil |---|---| | `airstack test -m unit` | Package `test/test_*.py`, collected directly from source | | `cd tests && pytest -m unit` | Same path — the containerless equivalent | -| `pytest tests/ -m unit` | **Nothing.** See the warning below | +| `pytest tests/ -m unit` | Same path — what CI runs | | `colcon test --packages-select ` | C++ gtests and linters; Python only for `ament_python` packages (see below) | **Two runners, split by language.** C++ gtests run only under `colcon test`, which CI @@ -63,15 +63,13 @@ Python tests depends on its build type: | `natnet_ros2` | `ament_cmake` | **No** — `CMakeLists.txt` registers `ament_add_gtest` but no `ament_add_pytest_test` | | `lidar_point_cloud_filter` | `ament_python` | **Yes** — `setup.cfg` sets `testpaths = test`, so colcon's pytest runner finds them | -So a Python test in an `ament_cmake` package runs *only* via the root harness, and today -that means only when someone runs it locally. +So a Python test in an `ament_cmake` package runs *only* via the root harness — which is +fine, since that is what CI invokes. -> **`pytest tests/` does not collect unit tests.** The injection in -> `tests/conftest.py::pytest_configure` is skipped whenever a path is given on the command -> line, and `tests/` is a path. The run reports `no tests collected` and exits **5**, which -> looks like a failure but means the tests never ran. Use `airstack test -m unit`, or -> `cd tests` first so `testpaths` applies. This also means **no CI workflow currently runs -> unit tests** — `system-tests.yml` invokes `pytest tests/`. +Naming a path *below* `tests/` narrows the run and skips the injection, so +`pytest tests/system/test_x.py` stays fast and does not drag in unit tests. The rule lives +in `harness.discovery.collection_is_broad` and is pinned by +`tests/meta/test_collection_contract.py`. ## Step-by-Step: Adding a Python Unit Test @@ -169,8 +167,8 @@ package root). Same YAML, different workspace key (`sim:`), for Isaac-extension ```bash airstack test -m unit -v -# or, containerless — note the `cd`, it is load-bearing: -cd tests && pytest -m unit -v +# or, containerless: +AIRSTACK_ROOT=$(pwd) pytest tests/ -m unit -v ``` All 155 existing tests plus your new ones should pass. Collected items point straight @@ -179,14 +177,12 @@ at the co-located source: ../robot/ros_ws/src///test/test_.py::test_my_function_basic PASSED ``` -If you see `no tests collected` and exit code 5, you ran `pytest tests/` — see the -warning in *Architecture Overview*. - ### 5. Running in CI -There is currently **no CI workflow that runs unit tests.** `system-tests.yml` invokes -`pytest tests/`, which does not collect them, and it only triggers on PR-open, a -`/pytest` comment, or `workflow_dispatch`. Run them locally before pushing. +Unit tests ride along with every `system-tests.yml` run — it invokes `pytest tests/`, +which collects them. That workflow triggers on PR open, a `/pytest` comment, or +`workflow_dispatch` — deliberately not on every push, since the same run also drives the +GPU system tests. Run them locally in the meantime. --- @@ -274,8 +270,8 @@ sim: | Where does pytest discover tests? | From the package `test/` dir listed in `colcon_unit_test_packages.yaml` | | How are duplicate basenames handled? | `--import-mode=importlib` (set in `pytest.ini`) | | What mark do all unit tests use? | `@pytest.mark.unit` — auto-applied by path in `conftest.py`; do not write it yourself | -| How do I run them? | `airstack test -m unit`, or `cd tests && pytest -m unit`. **Not** `pytest tests/` | -| What CI workflow runs them? | None today — see §5 | +| How do I run them? | `airstack test -m unit`, `cd tests && pytest -m unit`, or `pytest tests/ -m unit` | +| What CI workflow runs them? | `system-tests.yml`, via `pytest tests/` — see §5 | | Do system tests (`liveliness`, etc.) run too? | No — `-m unit` filters to hermetic tests only | | Does `colcon test` also run these? | Only if the package registers them. `ament_add_gtest` covers C++; a Python test needs `ament_add_pytest_test`, which `natnet_ros2` does **not** have — its Python tests run only under the root harness | | Can I add pure C++ gtests? | Yes — `ament_add_gtest` in CMakeLists.txt | diff --git a/.agents/skills/run-system-tests/SKILL.md b/.agents/skills/run-system-tests/SKILL.md index d5a6a9bb2..29a2d46cb 100644 --- a/.agents/skills/run-system-tests/SKILL.md +++ b/.agents/skills/run-system-tests/SKILL.md @@ -33,8 +33,8 @@ The suite lives at `tests/` (repo root) and is fully pytest-based. Configuration | Concern | Unit (`-m unit`) | System (`-m liveliness` etc.) | |---|---|---| | Hardware required | None — pure Python | Docker daemon, NVIDIA GPU, sim license | -| CI workflow | None — run them locally before pushing | `system-tests.yml` (GPU OpenStack VM) | -| Trigger | n/a | PR opened, `/pytest` comment, `workflow_dispatch` | +| CI workflow | `system-tests.yml` (included in `pytest tests/`) | `system-tests.yml` (GPU OpenStack VM) | +| Trigger | PR opened, `/pytest` comment, `workflow_dispatch` | PR opened, `/pytest` comment, `workflow_dispatch` | | Source location | `/test/test_*.py` (collected directly, listed in `colcon_unit_test_packages.yaml`) | `tests/system/` | | How to add | See `add-unit-tests` skill | See *Adding a New System Test* below | @@ -42,9 +42,8 @@ Run unit tests without any Docker stack: ```bash airstack test -m unit -v -# or, containerless — the `cd` is load-bearing; `pytest tests/ -m unit` -# collects nothing (see the add-unit-tests skill): -cd tests && AIRSTACK_ROOT=$(git rev-parse --show-toplevel) pytest -m unit -v +# or directly: +AIRSTACK_ROOT=$(pwd) pytest tests/ -m unit -v ``` For details on the co-located layout and adding new unit tests, see the diff --git a/CHANGELOG.md b/CHANGELOG.md index 7576f395c..b6a0e9dc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Unit-test documentation now matches the co-located layout: the `add-unit-tests` and `run-system-tests` skills and the testing docs name `airstack test -m unit` (or `cd tests && pytest -m unit`) as the way to run unit tests, record that `pytest tests/` does not collect them, state that no CI workflow runs them today, and stop instructing authors to write `@pytest.mark.unit` by hand — `conftest.py` applies it by file location +- Unit-test documentation now matches the co-located layout: the `add-unit-tests` and `run-system-tests` skills and the testing docs record which runner each language uses (C++ gtests via `colcon test` under the `build_packages` mark; Python via the root harness, plus `colcon test` for `ament_python` packages), and stop instructing authors to write `@pytest.mark.unit` by hand — `conftest.py` applies it by file location - Ephemeral CI GPU runners spawn via NVIDIA OSMO (not OpenStack); `system-tests.yml` / `docker-build.yml` still use `airstack-ephemeral` - Default system-test `--sim` is `isaacsim`; pass `--sim msairsim` to opt in to Microsoft AirSim - `-m build_packages` CI runs pull `cache_*` images instead of baking sim images @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `pytest tests/` now collects the co-located unit tests, so they run with every `system-tests.yml` invocation. The guard in `tests/conftest.py` skipped injection whenever any path was on the command line, and `tests/` is a path — CI collected 97 of 252 items and the Python unit tests ran nowhere. Narrowing (`pytest tests/system/test_x.py`) still skips injection; the rule is `harness.discovery.collection_is_broad`, pinned by `tests/meta/test_collection_contract.py` - Isaac Sim image: PX4 `ubuntu.sh` no longer fails dpkg configure on the NVIDIA base (`ca-certificates` / `software-properties-common`); use `--no-nuttx --no-sim-tools` like ms-airsim - Robot image: pin `pytest<8.1` and disable `launch_testing` for colcon unit tests so ROS Jazzy's outdated pytest hook does not abort `colcon test` - Robot name resolution now honors a pre-set `ROBOT_NAME` (e.g. injected via docker compose) instead of always overriding it from the container/hostname mapping (`robot/docker/.bashrc`) diff --git a/docs/development/intermediate/testing/index.md b/docs/development/intermediate/testing/index.md index f3a6ddbc5..8332c6e77 100644 --- a/docs/development/intermediate/testing/index.md +++ b/docs/development/intermediate/testing/index.md @@ -18,11 +18,12 @@ them from there. ```bash airstack test -m unit -v -# or containerless — the `cd` is load-bearing: -cd tests && pytest -m unit -v +# or directly: +pytest tests/ -m unit -v ``` -No CI workflow currently runs unit tests, so run them locally before pushing. +Unit tests run as part of `system-tests.yml` via `pytest tests/`, and can also be run +locally with no Docker or GPU needed. → **[Unit Testing Guide](unit_testing.md)** — patterns, CI workflow, how to add tests for new packages (Python and C++ gtest). diff --git a/docs/development/intermediate/testing/unit_testing.md b/docs/development/intermediate/testing/unit_testing.md index ffc8aada8..68cbd49a2 100644 --- a/docs/development/intermediate/testing/unit_testing.md +++ b/docs/development/intermediate/testing/unit_testing.md @@ -1,6 +1,6 @@ # Unit Testing -AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds via `airstack test -m unit`. No CI workflow runs them today, so run them yourself before pushing. +AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds via `airstack test -m unit`, and ride along with every `system-tests.yml` run in CI. ## Design principles @@ -35,41 +35,39 @@ Collected items point straight at the co-located source: # Locally — no container or Docker stack required airstack test -m unit -v -# Or directly with pytest. The `cd` is load-bearing: pytest only injects the -# co-located tests when no path is given on the command line. +# Or directly with pytest export AIRSTACK_ROOT=$(pwd) pip install -r tests/requirements.txt -cd tests && pytest -m unit -v +pytest tests/ -m unit -v ``` Unit tests complete in under one second for the current suite. ## CI -**C++ gtests are gated; Python unit tests are not.** The two languages take different -runners: +**The two languages take different runners, and both are gated:** -| Test | Runner | In CI | +| Test | Runner | In CI via | |---|---|---| -| C++ gtest | `colcon test` inside the robot container | Yes — the `build_packages` mark (`tests/system/test_build_packages.py::test_colcon_test_robot`) | -| Python, `ament_python` package | root harness **and** `colcon test` | Via `build_packages` only | -| Python, `ament_cmake` package | root harness only | No | +| C++ gtest | `colcon test` inside the robot container | the `build_packages` mark (`tests/system/test_build_packages.py::test_colcon_test_robot`) | +| Python, `ament_python` package | root harness **and** `colcon test` | `pytest tests/` **and** `build_packages` | +| Python, `ament_cmake` package | root harness only | `pytest tests/` | `colcon test` picks up Python tests only when the package's build type makes it: an `ament_python` package like `lidar_point_cloud_filter` exposes them through `setup.cfg` (`testpaths = test`), while an `ament_cmake` package like `natnet_ros2` -would need an explicit `ament_add_pytest_test` — it has none, so its Python tests run -nowhere in CI. +would need an explicit `ament_add_pytest_test` — it has none, so its Python tests reach +CI only through the root harness. -No workflow runs the Python unit tests directly. `system-tests.yml` invokes -`pytest tests/`, which does not collect them, and it only triggers on PR open, a -`/pytest` comment, or `workflow_dispatch`. Run them locally before pushing — no -infrastructure required: +Python unit tests are collected by `system-tests.yml`'s `pytest tests/` invocation, so +they run on every trigger of that workflow: PR open, a `/pytest` comment, or +`workflow_dispatch`. That is deliberately not every push — the same run also drives the +GPU system tests. Run them locally in the meantime, no infrastructure required: ```bash airstack test -m unit -v # or directly (requires tests/requirements.txt installed): -cd tests && AIRSTACK_ROOT=$(git rev-parse --show-toplevel) pytest -m unit -v +AIRSTACK_ROOT=$(pwd) pytest tests/ -m unit -v ``` ## Current test coverage @@ -197,8 +195,8 @@ sim: - # → simulation/**//test collected directly ``` -`airstack test -m unit` discovers them automatically — no changes to `pytest.ini` -needed. +`pytest tests/ -m unit` discovers them automatically — no changes to `pytest.ini` +or CI needed. ## See also diff --git a/tests/conftest.py b/tests/conftest.py index 29a7c7220..d79b2f5f7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -91,12 +91,15 @@ def pytest_configure(config): if str(root) not in sys.path: sys.path.insert(0, str(root)) - # Collect co-located unit tests: their files live outside tests/, so add the - # explicit non-linter test files to the collection args. Skip when an explicit - # path was given on the CLI (args_source == ARGS) so `pytest tests/system/foo.py` - # still narrows as expected. + # Collect co-located unit tests: their files live outside tests/, so pytest never + # reaches them by recursion — append the non-linter test files explicitly. Only for + # a run that means "everything": `pytest tests/system/foo.py` must still narrow. + # See harness.discovery.collection_is_broad. src_name = getattr(getattr(config, "args_source", None), "name", "TESTPATHS") - if src_name != "ARGS": + config.airstack_unit_tests_injected = src_name != "ARGS" or collection_is_broad( + config.args, config.invocation_params.dir + ) + if config.airstack_unit_tests_injected: for f in unit_test_files(): entry = str(f) if entry not in config.args: diff --git a/tests/harness/__init__.py b/tests/harness/__init__.py index d3fcf4e32..7b1210a7c 100644 --- a/tests/harness/__init__.py +++ b/tests/harness/__init__.py @@ -25,7 +25,9 @@ from harness.discovery import ( AIRSTACK_ROOT, COLCON_UNIT_TEST_PACKAGES_YAML, + TESTS_DIR, colcon_test_robot_command, + collection_is_broad, format_pytest_addopts, load_colcon_unit_test_config, repo_path, @@ -44,9 +46,9 @@ __all__ = [ # discovery - "AIRSTACK_ROOT", "COLCON_UNIT_TEST_PACKAGES_YAML", "repo_path", - "colcon_test_robot_command", "format_pytest_addopts", "load_colcon_unit_test_config", - "unit_test_dirs", "unit_test_files", + "AIRSTACK_ROOT", "COLCON_UNIT_TEST_PACKAGES_YAML", "TESTS_DIR", "repo_path", + "colcon_test_robot_command", "collection_is_broad", "format_pytest_addopts", + "load_colcon_unit_test_config", "unit_test_dirs", "unit_test_files", # session "logger", # commands diff --git a/tests/harness/discovery.py b/tests/harness/discovery.py index 28e03737a..1d4731f1a 100644 --- a/tests/harness/discovery.py +++ b/tests/harness/discovery.py @@ -1,8 +1,9 @@ """Unit-test discovery: which packages have unit tests and where their files live. Driven by ``tests/colcon_unit_test_packages.yaml``. ``conftest.pytest_configure`` adds -``unit_test_files()`` to the pytest run, and ``pytest_itemcollected`` marks each of those -items ``unit`` via ``_is_unit_item``. ament lint tests are excluded here — they run under +``unit_test_files()`` to the pytest run whenever ``collection_is_broad`` says the command +line did not narrow the run, and ``pytest_itemcollected`` marks each of those items +``unit`` via ``_is_unit_item``. ament lint tests are excluded here — they run under ``colcon test`` (linter skip is in package pytest config; see ``colcon_test_robot_command``). """ @@ -17,6 +18,10 @@ Path(AIRSTACK_ROOT) / "tests" / "colcon_unit_test_packages.yaml" ) +# The tests/ tree, derived from this file rather than AIRSTACK_ROOT so the guard always +# agrees with the conftest that is actually running. +TESTS_DIR = Path(__file__).resolve().parents[1] + def repo_path(*parts: str) -> Path: """Resolve a path relative to the repo root (``AIRSTACK_ROOT``). @@ -147,3 +152,42 @@ def unit_test_files(): if f.name not in _LINTER_TEST_FILENAMES: files.append(f) return files + + +def _arg_path(arg, invocation_dir): + """Absolute path addressed by one pytest positional. + + Positionals are raw CLI strings and may be node ids + (``system/test_x.py::TestY::test_z``); only the part before ``::`` addresses the + filesystem. The path need not exist — pytest reports bad paths itself. + """ + return Path(invocation_dir, str(arg).split("::", 1)[0]).resolve() + + +def collection_is_broad(args, invocation_dir, tests_root=None) -> bool: + """True when the positionals do not narrow the run below ``tests/``. + + Co-located unit tests live outside ``tests/``, so ``pytest_configure`` appends them + to ``config.args`` by hand. It must do that only for a run that already means + "everything", or ``pytest tests/system/test_x.py`` would drag in every unit test. + + Broad == a positional names ``tests/`` itself or an ancestor of it:: + + pytest (testpaths ``.``, cwd tests/) -> broad + pytest tests/ (CI, and the documented commands) -> broad + pytest . (cwd repo root or tests/) -> broad + pytest tests/system -> narrow + pytest tests/system/test_x.py::TestY::test_z -> narrow + pytest ../simulation/.../test/test_frames.py -> narrow + + ``any`` rather than ``all`` is deliberate: ``pytest_configure`` appends the + co-located files (narrow, absolute) to ``config.args``, so ``all`` would flip the + answer for anything re-deriving it after that mutation. + """ + root = Path(tests_root or TESTS_DIR).resolve() + invocation_dir = Path(invocation_dir).resolve() + return any( + root.is_relative_to(_arg_path(a, invocation_dir)) + for a in args + if not str(a).startswith("-") + ) diff --git a/tests/meta/test_collection_contract.py b/tests/meta/test_collection_contract.py new file mode 100644 index 000000000..8f6f4fbca --- /dev/null +++ b/tests/meta/test_collection_contract.py @@ -0,0 +1,88 @@ +# Copyright (c) 2024 Carnegie Mellon University +# MIT License - see LICENSE in the repository root for full text. +"""Contract tests for co-located unit-test collection. + +Unit-test source lives outside ``tests/``, so ``conftest.pytest_configure`` appends it to +the collection args when ``collection_is_broad`` says the command line did not narrow the +run. Get that wrong in the permissive direction and a narrowed run drags in every unit +test; get it wrong the other way and CI silently runs none of them. + +These live under ``tests/`` on purpose. Co-located, they would stop being collected at +the same moment they stopped guarding anything — here plain recursion finds them, so a +broken guard makes them run and fail. +""" +import re +from pathlib import Path + +import pytest + +from conftest import repo_path # noqa: E402 — pytest adds tests/ to sys.path +from harness.discovery import TESTS_DIR, collection_is_broad, unit_test_files + +# Not co-located, so `_is_unit_item` will not mark it — the one place the mark is +# written by hand. +pytestmark = pytest.mark.unit + +_REPO = TESTS_DIR.parent + + +@pytest.mark.parametrize( + "cwd, args", + [ + (_REPO, ["tests/"]), # CI, and the documented commands + (_REPO, ["tests"]), + (_REPO, ["./tests/"]), + (_REPO, ["."]), + (_REPO, [str(TESTS_DIR)]), + (TESTS_DIR, ["."]), # testpaths, i.e. `airstack test` + (TESTS_DIR, [str(TESTS_DIR)]), + ], +) +def test_broad_invocations_collect_unit_tests(cwd, args): + assert collection_is_broad(args, cwd) is True + + +@pytest.mark.parametrize( + "cwd, args", + [ + (TESTS_DIR, ["system"]), + (TESTS_DIR, ["system/test_liveliness.py"]), + (TESTS_DIR, ["system/test_liveliness.py::TestLiveliness::test_x"]), + (_REPO, ["tests/system/test_sensors.py"]), + (_REPO, ["tests/integration/natnet"]), + (_REPO, ["simulation/isaac-sim/extensions/optitrack.natnet.emulator/test/test_frames.py"]), + ], +) +def test_narrowed_invocations_do_not(cwd, args): + assert collection_is_broad(args, cwd) is False + + +def test_ci_invocation_is_broad(): + """The command system-tests.yml runs must collect unit tests. + + This is the test that would have caught the original bug: CI ran `pytest tests/`, + which the guard classified as a narrowing run, so no unit test ever executed in CI. + """ + workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() + match = re.search(r"^\s*pytest\s+(\S+)", workflow, re.M) + assert match, "no `pytest ` invocation found in system-tests.yml" + assert collection_is_broad([match.group(1)], _REPO), ( + f"system-tests.yml runs `pytest {match.group(1)}`, which does not collect " + "co-located unit tests" + ) + + +def test_injection_actually_produced_items(request): + """Every discovered unit-test file contributed at least one collected item. + + Catches breakage below the guard — YAML drift, a glob change, an import error that + turns a module into a collection error rather than tests. + """ + if not getattr(request.config, "airstack_unit_tests_injected", False): + pytest.skip("narrowed run — co-located tests are not injected by design") + + collected = {Path(str(item.path)).resolve() for item in request.session.items} + missing = [f for f in unit_test_files() if f.resolve() not in collected] + assert not missing, "discovered but not collected: " + ", ".join( + str(f.relative_to(_REPO)) for f in missing + ) From 6a77c39c44c786282086521799b21a2a91996263 Mon Sep 17 00:00:00 2001 From: John Date: Mon, 17 Aug 2026 18:22:21 -0400 Subject: [PATCH 4/6] docs(tests): explain why C++ and Python unit tests use different runners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The split was documented as a fact without its reason. A gtest is a binary compiled against the package's headers and rclcpp, so it can only run where the ROS toolchain is — colcon test inside the robot container, which build_packages reaches after building with -DBUILD_TESTING=ON. Python unit tests stub ROS at the import boundary and touch no ROS runtime, so they need neither a build nor a container, which is what keeps the suite under a second. State the invariant that follows: a Python test needing a live ROS node belongs in tests/integration/ or tests/system/, not in a package test/ dir. Co-Authored-By: Claude Opus 5 --- .agents/skills/add-unit-tests/SKILL.md | 18 +++++++++++++----- .../intermediate/testing/unit_testing.md | 7 ++++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index a60129370..6f7134491 100644 --- a/.agents/skills/add-unit-tests/SKILL.md +++ b/.agents/skills/add-unit-tests/SKILL.md @@ -52,11 +52,19 @@ auto-tagged `@pytest.mark.unit` by path, so `-m unit` selects it. ament lint fil | `pytest tests/ -m unit` | Same path — what CI runs | | `colcon test --packages-select ` | C++ gtests and linters; Python only for `ament_python` packages (see below) | -**Two runners, split by language.** C++ gtests run only under `colcon test`, which CI -executes inside the robot container via the **`build_packages`** mark -(`tests/system/test_build_packages.py::test_colcon_test_robot`). Python unit tests run -under the root harness described above. Whether `colcon test` *also* picks up a package's -Python tests depends on its build type: +**Two runners, split by language — because C++ needs a build and Python does not.** A +gtest is a binary: it must be compiled against the package's headers and rclcpp, so it can +only run where the ROS toolchain is. That is `colcon test` inside the robot container, +which CI reaches via the **`build_packages`** mark +(`tests/system/test_build_packages.py::test_colcon_test_robot`, which builds with +`-DBUILD_TESTING=ON` first). Python unit tests are deliberately hermetic — they stub ROS +at the import boundary and touch no ROS runtime — so they need no build and no container, +which is what lets the root harness run all of them in about a second. + +Preserve that property when adding tests: a Python test that needs a live ROS node belongs +in `tests/integration/` or `tests/system/`, not here. + +Whether `colcon test` *also* picks up a package's Python tests depends on its build type: | Package | Build type | Python tests under `colcon test` | |---|---|---| diff --git a/docs/development/intermediate/testing/unit_testing.md b/docs/development/intermediate/testing/unit_testing.md index 68cbd49a2..a616170b0 100644 --- a/docs/development/intermediate/testing/unit_testing.md +++ b/docs/development/intermediate/testing/unit_testing.md @@ -45,7 +45,12 @@ Unit tests complete in under one second for the current suite. ## CI -**The two languages take different runners, and both are gated:** +**The two languages take different runners because C++ needs a build and Python does +not.** A gtest is a binary compiled against the package's headers and rclcpp, so it only +runs where the ROS toolchain is — `colcon test` inside the robot container. Python unit +tests stub ROS at the import boundary and touch no ROS runtime, so they need neither a +build nor a container, which is what keeps the whole suite under a second. Both are +gated in CI: | Test | Runner | In CI via | |---|---|---| From 25c266fa1484ca163c064a30ca0f802920e9a7b7 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 18 Aug 2026 11:42:00 -0400 Subject: [PATCH 5/6] test: run the collection contract tests with the fast tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They are hermetic and they guard the collection of everything above them, so running them after the GPU sim suites is backwards — a hung flight test would mean they never execute. Rank them in _MODULE_ORDER right after the co-located unit tests, ahead of system.test_build_docker. Also drop the `from conftest import repo_path` in favour of harness.discovery, which the module already imports from — one less thing between the test and the function it needs. Co-Authored-By: Claude Opus 5 --- tests/harness/collection.py | 3 +++ tests/meta/test_collection_contract.py | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/harness/collection.py b/tests/harness/collection.py index ae78ed94c..6bf209abf 100644 --- a/tests/harness/collection.py +++ b/tests/harness/collection.py @@ -14,6 +14,9 @@ # Unit tests first — fast, hermetic, no Docker. Co-located package unit tests # (see unit_test_files) sort into this leading slot via the path check below. "__unit__", + # Harness contract tests: hermetic, and they guard the collection of everything + # above, so they belong with the fast tier rather than after the sim suites. + "test_collection_contract", # System tests follow in dependency order. "system.test_build_docker", "system.test_build_packages", diff --git a/tests/meta/test_collection_contract.py b/tests/meta/test_collection_contract.py index 8f6f4fbca..0df4751fb 100644 --- a/tests/meta/test_collection_contract.py +++ b/tests/meta/test_collection_contract.py @@ -16,8 +16,12 @@ import pytest -from conftest import repo_path # noqa: E402 — pytest adds tests/ to sys.path -from harness.discovery import TESTS_DIR, collection_is_broad, unit_test_files +from harness.discovery import ( # noqa: E402 — pytest adds tests/ to sys.path + TESTS_DIR, + collection_is_broad, + repo_path, + unit_test_files, +) # Not co-located, so `_is_unit_item` will not mark it — the one place the mark is # written by hand. From 4a78ea28cf52e64664e87bc1b1a4369a1bd7e403 Mon Sep 17 00:00:00 2001 From: Pranav Kumara Date: Tue, 18 Aug 2026 17:46:06 -0400 Subject: [PATCH 6/6] fix(ci): make PR validation and metrics trustworthy Run fast unit checks automatically, constrain host collection, and distinguish infrastructure failures from comparable simulation results. --- .agents/skills/add-unit-tests/SKILL.md | 11 +- .../skills/bump-version-and-release/SKILL.md | 2 +- .agents/skills/run-system-tests/SKILL.md | 27 +- .github/workflows/system-tests.yml | 163 ++++++--- .github/workflows/unit-tests.yml | 46 +++ AGENTS.md | 7 +- CHANGELOG.md | 6 +- .../development/intermediate/testing/ci_cd.md | 73 +++-- .../development/intermediate/testing/index.md | 5 +- .../intermediate/testing/unit_testing.md | 19 +- osmo/README.md | 12 +- tests/README.md | 31 +- tests/conftest.py | 35 +- tests/harness/collection.py | 1 + tests/harness/discovery.py | 21 +- tests/harness/run_meta.py | 310 ++++++++++++++++++ tests/harness/test_ids.py | 15 + tests/meta/test_collection_contract.py | 66 +++- tests/meta/test_metrics_reporting_contract.py | 263 +++++++++++++++ tests/parse_metrics.py | 141 ++++++-- tests/run_summary.py | 42 +-- 21 files changed, 1118 insertions(+), 178 deletions(-) create mode 100644 .github/workflows/unit-tests.yml create mode 100644 tests/harness/run_meta.py create mode 100644 tests/harness/test_ids.py create mode 100644 tests/meta/test_metrics_reporting_contract.py diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index 6f7134491..27aeb3a87 100644 --- a/.agents/skills/add-unit-tests/SKILL.md +++ b/.agents/skills/add-unit-tests/SKILL.md @@ -187,10 +187,11 @@ at the co-located source: ### 5. Running in CI -Unit tests ride along with every `system-tests.yml` run — it invokes `pytest tests/`, -which collects them. That workflow triggers on PR open, a `/pytest` comment, or -`workflow_dispatch` — deliberately not on every push, since the same run also drives the -GPU system tests. Run them locally in the meantime. +`unit-tests.yml` invokes `pytest tests/ -m unit` on GitHub-hosted `ubuntu-latest` +whenever a PR targeting `main` or `develop` is opened, synchronized, or reopened. +It does not consume an OSMO GPU. +C++ gtests still run through the OSMO `build_packages` mark because they require the +ROS workspace and toolchain inside the robot container. --- @@ -279,7 +280,7 @@ sim: | How are duplicate basenames handled? | `--import-mode=importlib` (set in `pytest.ini`) | | What mark do all unit tests use? | `@pytest.mark.unit` — auto-applied by path in `conftest.py`; do not write it yourself | | How do I run them? | `airstack test -m unit`, `cd tests && pytest -m unit`, or `pytest tests/ -m unit` | -| What CI workflow runs them? | `system-tests.yml`, via `pytest tests/` — see §5 | +| What CI workflow runs them? | Python: `unit-tests.yml`; C++: the `build_packages` path in `system-tests.yml` — see §5 | | Do system tests (`liveliness`, etc.) run too? | No — `-m unit` filters to hermetic tests only | | Does `colcon test` also run these? | Only if the package registers them. `ament_add_gtest` covers C++; a Python test needs `ament_add_pytest_test`, which `natnet_ros2` does **not** have — its Python tests run only under the root harness | | Can I add pure C++ gtests? | Yes — `ament_add_gtest` in CMakeLists.txt | diff --git a/.agents/skills/bump-version-and-release/SKILL.md b/.agents/skills/bump-version-and-release/SKILL.md index 18792a965..a4a74c77e 100644 --- a/.agents/skills/bump-version-and-release/SKILL.md +++ b/.agents/skills/bump-version-and-release/SKILL.md @@ -260,5 +260,5 @@ For a true release (dropping the pre-release suffix): ## Related Skills -- [`run-system-tests`](../run-system-tests) — what fires on every PR alongside the version check +- [`run-system-tests`](../run-system-tests) — automatic unit/package gates and how to request simulation campaigns - [`update-documentation`](../update-documentation) — for docs-only PRs that may still need a VERSION bump to clear the gate diff --git a/.agents/skills/run-system-tests/SKILL.md b/.agents/skills/run-system-tests/SKILL.md index 29a2d46cb..c2260d29e 100644 --- a/.agents/skills/run-system-tests/SKILL.md +++ b/.agents/skills/run-system-tests/SKILL.md @@ -1,6 +1,6 @@ --- name: run-system-tests -description: Run, interpret, and extend AirStack's pytest system test suite (build_packages, build_docker, liveliness, sensors, takeoff_hover_land, autonomy), trigger runs via /pytest PR comments, and read metrics.json regression reports. Use for invoking tests, debugging failures from results.xml/metrics.json, or adding a new system test. +description: Run, interpret, and extend AirStack's pytest system test suite (build_packages, build_docker, liveliness, sensors, takeoff_hover_land, autonomy), trigger runs via /pytest PR comments, and read run_meta.json/metrics.json reports. Use for invoking tests, distinguishing infrastructure failures from policy regressions, or adding a new system test. license: Apache-2.0 metadata: author: AirLab CMU @@ -14,7 +14,7 @@ metadata: Use this skill when you need to: - Invoke the pytest system tests locally (via `airstack test`) or on CI (via `/pytest` PR comment or `workflow_dispatch`) -- Diagnose a failing system test — interpret `summary.txt`, `results.xml`, and `metrics.json` from `tests/results//` +- Diagnose a failing system test — interpret `summary.txt`, `results.xml`, `run_meta.json`, and `metrics.json` from `tests/results//` - Compare metrics against a baseline run (`parse_metrics.py --baseline`) to confirm a regression or improvement - Add a new system test to `tests/`: pick the right mark, wire up `airstack_env` parametrization, and record metrics with `MetricsRecorder` @@ -33,8 +33,8 @@ The suite lives at `tests/` (repo root) and is fully pytest-based. Configuration | Concern | Unit (`-m unit`) | System (`-m liveliness` etc.) | |---|---|---| | Hardware required | None — pure Python | Docker daemon, NVIDIA GPU, sim license | -| CI workflow | `system-tests.yml` (included in `pytest tests/`) | `system-tests.yml` (GPU OpenStack VM) | -| Trigger | PR opened, `/pytest` comment, `workflow_dispatch` | PR opened, `/pytest` comment, `workflow_dispatch` | +| CI workflow | `unit-tests.yml` (`ubuntu-latest`) | `system-tests.yml` (ephemeral OSMO GPU pod) | +| Trigger | PR opened, synchronized, or reopened | Automatic `build_packages` on PR open/update/reopen; simulation via `/pytest` or `workflow_dispatch` | | Source location | `/test/test_*.py` (collected directly, listed in `colcon_unit_test_packages.yaml`) | `tests/system/` | | How to add | See `add-unit-tests` skill | See *Adding a New System Test* below | @@ -180,7 +180,10 @@ Total parametrize cardinality for sim tests = `len(sims) × len(num_robots) × s The `system-tests.yml` workflow accepts three trigger paths: -1. **PR opened** (same-repo only) — auto-runs pytest with conftest defaults. Fork PRs are skipped to keep arbitrary code off the self-hosted runner. +1. **PR opened, synchronized, or reopened** (same-repo only) — auto-runs the + `build_packages` mark. Fork PRs are skipped to keep arbitrary code off the + privileged self-hosted runner. Python unit tests run separately in + `unit-tests.yml`, including for fork PRs. 2. **`/pytest` issue comment** on a PR — only honored from users with `OWNER`, `MEMBER`, or `COLLABORATOR` author association. Fork PRs are explicitly rejected by the `Resolve PR head` step (the PR's head repo must equal `${context.repo.owner}/${context.repo.repo}`). 3. **`workflow_dispatch`** — manual run from the Actions tab with form inputs (`marks`, `sim`, `num_robots`, `stress_iterations`, `stable_duration`, `baseline_run_id`). @@ -205,14 +208,14 @@ notes: testing the new altitude controller The workflow: 1. Posts an acknowledgment PR comment showing the resolved `pytest tests/ ` command and a link to the run 2. Opens an in-progress GitHub Check Run on the PR's head SHA so the run shows up in the **Checks** tab (issue_comment events otherwise associate runs with the default branch) -3. Runs pytest on a freshly-spawned ephemeral OpenStack runner (`runs-on: [self-hosted, airstack-ephemeral]`) +3. Runs pytest on a freshly-spawned ephemeral OSMO GPU pod (`runs-on: [self-hosted, airstack-ephemeral]`) 4. Uploads `tests/results/` as artifact `test-results--` (90-day retention) -5. The downstream `report` job runs `parse_metrics.py` against the latest baseline artifact from the PR's base branch and posts a markdown table back as a PR comment + job summary +5. The downstream `report` job runs `parse_metrics.py`, compares only a matching complete simulation baseline, posts the result, and finalizes the PR-head Check Run 6. Closes the Check Run with the final conclusion ### Why fork PRs are blocked -The runner is GPU-equipped, has Docker root access, and is reused (briefly) across the lifetime of one job. Running arbitrary fork code on it would let a contributor exfiltrate registry creds, mine crypto, or pivot into the OpenStack tenant. The same-repo guard is the only line of defense and **must not be removed**. If you need to test a fork PR, mirror the branch into the upstream repo first. +The runner is GPU-equipped, has Docker root access, and is reused (briefly) across the lifetime of one job. Running arbitrary fork code on it would let a contributor exfiltrate registry creds, mine crypto, or abuse the privileged OSMO CI pool. The same-repo guard is the only line of defense and **must not be removed**. If you need to test a fork PR, mirror the branch into the upstream repo first. ## Interpreting Results and Metrics @@ -273,7 +276,7 @@ The report has three sections per test module: - **Sim publishing rates** — pivoted Hz aggregates per topic (`mean`, `start_mean`, `end_mean`, `min`, `max`) from the `sensors` mark (sim + robot streams) - **Compute usage** — pivoted CPU/mem/GPU per container -Regressions exceeding `--threshold` (default 20%) are flagged `:red_circle:`; improvements beyond threshold get `:green_circle:`. CI fails the job on any regression. +Regressions exceeding `--threshold` (default 20%) are flagged `:red_circle:`; improvements beyond threshold get `:green_circle:`. CI fails only when both artifacts are complete and have the same simulation campaign fingerprint. When local-debugging a CI regression, download both artifacts (`test-results--` from the PR run and from the base branch's most recent run), unzip them under `tests/results/`, and run `parse_metrics.py` locally to see the same table the bot posted. @@ -360,7 +363,7 @@ If multiple tests need the same setup, add a fixture in `conftest.py` (not in yo - **Letting parametrize cardinality explode**. Default `--num-robots 1,3` (and `--sim msairsim` if you opt in) multiplies stack bring-ups for each selected mark (`liveliness`, `sensors`, `takeoff_hover_land`, …) — expensive. Override locally to a single tuple while iterating. `--sim` defaults to `isaacsim` only. - **Hardcoded container names**. Always use `find_container`, `get_robot_containers`, or `wait_for_container` — replica suffixes (`-1`, `-2`, `-3`) and compose project prefixes change. - **Asserting on stdout instead of using `read_log_tail`**. The conftest captures each subprocess's combined stdout/stderr in memory; assertions should reference it via `read_log_tail()` (`f"airstack up failed:\n{read_log_tail()}"`) so failures attach the relevant context to the JUnit XML. -- **Trying to SSH into a CI runner mid-job**. Workers are ephemeral OpenStack VMs destroyed within ~30s of job completion. Re-running the job creates a fresh VM. For genuine debugging on the runner, see `.github/orchestrator/README.md` (also exposed at `tests/ci-cd-orchestrator.md`) — but in 99% of cases, reproduce locally with `airstack test`. +- **Trying to SSH into a CI runner mid-job**. Workers are ephemeral OSMO pods destroyed after job completion. Re-running creates a fresh pod. For genuine runner debugging, see `.github/orchestrator/README.md` (also exposed at `tests/ci-cd-orchestrator.md`) — but in most cases, reproduce locally with `airstack test`. - **Forgetting to register a new mark**. Adding `@pytest.mark.my_new_mark` without updating `tests/pytest.ini` produces "PytestUnknownMarkWarning" and makes `-m my_new_mark` fail to filter as expected. ## Quick Reference @@ -424,12 +427,12 @@ python tests/parse_metrics.py \ ### Files to know - `tests/conftest.py` — pytest hooks + the `airstack_env` / `robot_autonomy_stack` fixtures (re-exports the harness API) -- `tests/harness/` — helpers split by concern: `session`, `discovery`, `commands`, `containers`, `metrics` (`MetricsRecorder`), `sim`, `collection` (ordering) +- `tests/harness/` — helpers split by concern: `session`, `discovery`, `commands`, `containers`, `metrics` (`MetricsRecorder`), `run_meta`, `test_ids`, `sim`, `collection` (ordering) - `tests/pytest.ini` — mark registration, log format - `tests/parse_metrics.py` — markdown reporter, regression diff - `tests/README.md` — user-facing docs (CLI options, output layout, CI/CD orchestrator) - `.github/workflows/system-tests.yml` — CI workflow with `/pytest` comment trigger -- `.github/orchestrator/README.md` — ephemeral OpenStack runner setup and SSH-debug procedure +- `.github/orchestrator/README.md` — ephemeral OSMO runner setup and worker-debug procedure ## References diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 69a961a48..48ab4f639 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -2,7 +2,7 @@ name: System Tests on: pull_request: - types: [opened] + types: [opened, synchronize, reopened] issue_comment: types: [created] workflow_dispatch: @@ -41,7 +41,7 @@ jobs: runs-on: [self-hosted, airstack-ephemeral] # Triggers: # - workflow_dispatch (manual) - # - PR opened from the same repo (not a fork) — same-repo guard + # - PR opened, synchronized, or reopened from the same repo — same-repo guard # prevents arbitrary code execution on the self-hosted runner from # untrusted contributors. # - PR comment starting with `/pytest` from a user with write access @@ -57,6 +57,9 @@ jobs: github.event.issue.pull_request != null && startsWith(github.event.comment.body, '/pytest') && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + concurrency: + group: system-tests-${{ github.event_name == 'pull_request' && github.event.pull_request.number || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} timeout-minutes: 120 # Adding any `permissions:` entry disables GITHUB_TOKEN's defaults, so # every scope used here has to be re-granted explicitly: @@ -76,6 +79,10 @@ jobs: # is not addressable from `if:` expressions. env: DOCKER_REGISTRY_PASSWORD: ${{ secrets.DOCKER_REGISTRY_PASSWORD }} + outputs: + tested_sha: ${{ steps.identity.outputs.tested_sha }} + pr_number: ${{ steps.identity.outputs.pr_number }} + check_run_id: ${{ steps.check_create.outputs.id }} steps: # Uses actions/github-script (Node, bundled with the runner) instead # of `gh` so we don't depend on system tools — the ephemeral @@ -103,6 +110,24 @@ jobs: core.setOutput('head_sha', pr.data.head.sha); core.setOutput('base_ref', pr.data.base.ref); + - name: Resolve tested revision identity + if: always() + id: identity + env: + EVENT_NAME: ${{ github.event_name }} + COMMENT_HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + EVENT_SHA: ${{ github.sha }} + COMMENT_PR_NUMBER: ${{ github.event.issue.number }} + EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + if [[ "$EVENT_NAME" == "issue_comment" ]]; then + echo "tested_sha=${COMMENT_HEAD_SHA:-$EVENT_SHA}" >> "$GITHUB_OUTPUT" + echo "pr_number=$COMMENT_PR_NUMBER" >> "$GITHUB_OUTPUT" + else + echo "tested_sha=$EVENT_SHA" >> "$GITHUB_OUTPUT" + echo "pr_number=$EVENT_PR_NUMBER" >> "$GITHUB_OUTPUT" + fi + # Parsed up-front (before checkout) so the acknowledgment comment below # can echo the resolved args. This step only reads env vars, so it # doesn't need the working tree. @@ -134,9 +159,10 @@ jobs: if (st := os.environ.get('INPUT_STABLE', '').strip()): args.extend(['--stable-duration', st]) elif event == 'pull_request': - # PR-opened auto-run uses pytest's conftest defaults — same as - # /pytest with no args. - args = [] + # Automatic PR validation is deliberately build-scoped. Fast + # Python unit tests run in unit-tests.yml; GPU simulation remains + # selectable via /pytest or workflow_dispatch. + args = ['-m', 'build_packages'] else: body = os.environ.get('COMMENT_BODY', '') # Only the first line is parsed — everything below it is @@ -223,7 +249,7 @@ jobs: # Reply on the PR thread so the commenter sees their /pytest was # picked up and can confirm we parsed the args correctly. The # workflow_dispatch path skips this (no PR to comment on); the - # pull_request-opened path skips it too (the PR Checks tab is + # pull_request path skips it too (the PR Checks tab is # already showing the native run). - name: Post acknowledgment comment if: github.event_name == 'issue_comment' @@ -403,57 +429,75 @@ jobs: run: | # Re-split the shell-quoted args from the parse step so we forward # them to pytest as a proper argv list (preserving values like - # `-m 'a or b'`). Empty PYTEST_ARGS yields an empty array, so - # pytest falls back to its conftest defaults. - mapfile -t ARGS < <(python3 -c "import os, shlex; print('\n'.join(shlex.split(os.environ['PYTEST_ARGS'])))") + # `-m 'a or b'`). sys.stdout.write is intentional: print('') emits + # one blank line, which mapfile turns into an empty positional path + # and makes pytest recurse from the repository root. + mapfile -t ARGS < <(python3 -c "import os, shlex, sys; sys.stdout.write(''.join(f'{arg}\\n' for arg in shlex.split(os.environ['PYTEST_ARGS'])))") + for arg in "${ARGS[@]}"; do + if [[ -z "$arg" ]]; then + echo "::error::Refusing an empty pytest argument because it expands collection to the repository root." + exit 2 + fi + done + set +e pytest tests/ \ "${ARGS[@]}" \ -v -s \ --log-cli-level=INFO \ --log-cli-format='%(asctime)s [%(levelname)s] %(name)s: %(message)s' \ --log-cli-date-format='%H:%M:%S' + pytest_status=$? + set -e + if (( pytest_status != 0 )); then + exit "$pytest_status" + fi + + # A successful collect-only/non-executed campaign is not a passing + # system test. Make that distinction visible in the job conclusion. + python3 <<'PYEOF' + import json + from pathlib import Path + + candidates = list(Path("tests/results").glob("*/run_meta.json")) + if not candidates: + raise SystemExit("::error::pytest succeeded without run_meta.json") + latest = max(candidates, key=lambda path: path.stat().st_mtime) + outcome = json.loads(latest.read_text()).get("outcome") + if outcome not in {"simulation", "non_simulation"}: + raise SystemExit( + f"::error::pytest did not execute a complete campaign ({outcome})" + ) + PYEOF - name: Upload test results uses: actions/upload-artifact@v4 if: always() with: - name: test-results-${{ github.sha }}-${{ github.run_id }} + name: test-results-${{ steps.identity.outputs.tested_sha }}-${{ github.run_id }} path: tests/results/ retention-days: 90 - # Close out the Check Run with the job's final conclusion. The - # `steps.check_create.outputs.id` guard skips this when the open - # step didn't run (workflow_dispatch) or failed before producing - # an id. - - name: Finalize check on PR head - if: always() && github.event_name == 'issue_comment' && steps.check_create.outputs.id - uses: actions/github-script@v7 - with: - script: | - await github.rest.checks.update({ - owner: context.repo.owner, - repo: context.repo.repo, - check_run_id: ${{ steps.check_create.outputs.id }}, - status: 'completed', - conclusion: '${{ job.status }}', - details_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); - report: name: Metrics Report runs-on: ubuntu-latest needs: run-tests # Skip when run-tests was skipped (e.g., comment didn't match `/pytest`) # so we don't post empty-report comments on every PR comment. - if: always() && needs.run-tests.result != 'skipped' + if: > + always() && + needs.run-tests.result != 'skipped' && + (needs.run-tests.result != 'cancelled' || github.event_name != 'pull_request') permissions: + actions: read + checks: write + contents: read pull-requests: write steps: - name: Checkout uses: actions/checkout@v4 with: - submodules: recursive + ref: ${{ needs.run-tests.outputs.tested_sha }} - name: Set up Python uses: actions/setup-python@v5 @@ -478,8 +522,9 @@ jobs: - name: Download current test results uses: actions/download-artifact@v4 + continue-on-error: true with: - name: test-results-${{ github.sha }}-${{ github.run_id }} + name: test-results-${{ needs.run-tests.outputs.tested_sha }}-${{ github.run_id }} path: current-results/ # PR mode (opened or comment-triggered): fetch latest artifact from @@ -504,9 +549,10 @@ jobs: uses: actions/download-artifact@v4 continue-on-error: true with: + github-token: ${{ github.token }} run-id: ${{ inputs.baseline_run_id }} - name_is_regexp: true - name: "test-results-.*" + pattern: "test-results-*" + merge-multiple: true path: baseline-results/ # Manual dispatch without explicit baseline: fetch latest from main @@ -548,6 +594,19 @@ jobs: CURRENT="${{ steps.dirs.outputs.current }}" BASELINE="${{ steps.dirs.outputs.baseline }}" + if [ -z "$CURRENT" ]; then + cat > report.md <<'EOF' + ## Run status + + **Simulation metrics are not comparable.** The test job produced no finalized `results.xml` artifact. The runner may have timed out, been cancelled, or failed before pytest started. + + Pass-rate and regression tables are suppressed because no completed test campaign is available. + EOF + echo "parser_exit=0" >> "$GITHUB_OUTPUT" + exit 0 + fi + + set +e if [ -n "$BASELINE" ]; then python tests/parse_metrics.py \ --current "$CURRENT" \ @@ -558,6 +617,10 @@ jobs: --current "$CURRENT" \ --output report.md fi + parser_exit=$? + set -e + echo "parser_exit=$parser_exit" >> "$GITHUB_OUTPUT" + exit "$parser_exit" - name: Post PR comment if: github.event_name == 'issue_comment' || github.event_name == 'pull_request' @@ -571,9 +634,9 @@ jobs: } catch { body = '_No metrics report generated._'; } - const header = `## Test Metrics — \`${{ github.sha }}\`\n\n`; + const header = `## Test Metrics — \`${{ needs.run-tests.outputs.tested_sha }}\`\n\n`; await github.rest.issues.createComment({ - issue_number: context.issue.number, + issue_number: Number('${{ needs.run-tests.outputs.pr_number }}'), owner: context.repo.owner, repo: context.repo.repo, body: header + body, @@ -583,7 +646,7 @@ jobs: if: always() run: | if [ -f report.md ]; then - echo "## Test Metrics — \`${{ github.sha }}\`" >> "$GITHUB_STEP_SUMMARY" + echo "## Test Metrics — \`${{ needs.run-tests.outputs.tested_sha }}\`" >> "$GITHUB_STEP_SUMMARY" echo "" >> "$GITHUB_STEP_SUMMARY" cat report.md >> "$GITHUB_STEP_SUMMARY" else @@ -593,5 +656,31 @@ jobs: - name: Fail on regression if: steps.report.outcome == 'failure' run: | - echo "::error::Metric regression detected — see the report above for details." + if [ "${{ steps.report.outputs.parser_exit }}" = "1" ]; then + echo "::error::Metric regression detected — see the report above for details." + else + echo "::error::Metrics report generation failed — see the report step log." + fi exit 1 + + - name: Finalize check on PR head + if: always() && github.event_name == 'issue_comment' && needs.run-tests.outputs.check_run_id + uses: actions/github-script@v7 + with: + script: | + const tests = '${{ needs.run-tests.result }}'; + const report = '${{ steps.report.outcome }}'; + let conclusion = 'failure'; + if (tests === 'success' && report === 'success') { + conclusion = 'success'; + } else if (tests === 'cancelled') { + conclusion = 'cancelled'; + } + await github.rest.checks.update({ + owner: context.repo.owner, + repo: context.repo.repo, + check_run_id: Number('${{ needs.run-tests.outputs.check_run_id }}'), + status: 'completed', + conclusion, + details_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + }); diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml new file mode 100644 index 000000000..a2bf33022 --- /dev/null +++ b/.github/workflows/unit-tests.yml @@ -0,0 +1,46 @@ +name: Unit Tests + +on: + pull_request: + branches: [main, develop] + types: [opened, synchronize, reopened] + +permissions: + contents: read + +concurrency: + group: unit-tests-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + unit: + name: Python unit and harness contracts + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + cache: pip + cache-dependency-path: tests/requirements.txt + + - name: Install test dependencies + run: python -m pip install -r tests/requirements.txt + + - name: Run unit tests + env: + AIRSTACK_ROOT: ${{ github.workspace }} + run: pytest tests/ -m unit + + - name: Upload unit-test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: unit-test-results-${{ github.sha }}-${{ github.run_id }} + path: tests/results/ + retention-days: 30 diff --git a/AGENTS.md b/AGENTS.md index 76987f8eb..8c113f219 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -222,7 +222,7 @@ docker exec airstack-robot-desktop-1 bash -c "ros2 topic echo --onc docker exec airstack-robot-desktop-1 bash -c "sws && colcon test --packages-select natnet_ros2 --event-handlers console_direct+" ``` -2. **Unit tests (`pytest`, `unit` mark):** Fast, hermetic checks. Test **source** lives co-located with each ROS 2 package in `/test/` (standard colcon convention). [`tests/colcon_unit_test_packages.yaml`](tests/colcon_unit_test_packages.yaml) lists which packages have unit tests, and `tests/conftest.py` collects them from there under `--import-mode=importlib`. To add a package's unit tests, list it in that YAML. Unit tests run as part of the `system-tests.yml` suite. Example: `airstack test -m unit -v`. See `add-unit-tests` skill. +2. **Unit tests (`pytest`, `unit` mark):** Fast, hermetic checks. Test **source** lives co-located with each ROS 2 package in `/test/` (standard colcon convention). [`tests/colcon_unit_test_packages.yaml`](tests/colcon_unit_test_packages.yaml) lists which packages have unit tests, and `tests/conftest.py` collects them from there under `--import-mode=importlib`. To add a package's unit tests, list it in that YAML. Python unit tests run automatically in `unit-tests.yml` on `ubuntu-latest`; C++ gtests run through the `system-tests.yml` `build_packages` path. Example: `airstack test -m unit -v`. See `add-unit-tests` skill. 3. **System Level (`tests/system/`):** Full simulation tests (Isaac Sim or Microsoft AirSim legacy) - End-to-end autonomy stack testing @@ -244,7 +244,7 @@ Pytest-based system tests live under [`tests/system/`](tests/system/). They brin | [`tests/system/test_fixed_trajectory.py`](tests/system/test_fixed_trajectory.py) | `autonomy` | 4-phase flight chain (PX4 ready → takeoff → execute Circle/Figure8/Racetrack/Line trajectory → land) per (sim, num_robots, iter, trajectory_type); records cross-track error and path RMSE | Docker, GPU, sim license | | [`tests/system/test_waypoint_flight.py`](tests/system/test_waypoint_flight.py) | `waypoint_flight` | 4-phase flight chain (PX4 ready → takeoff → NavigateTask waypoint route → land) per (sim, num_robots, iter); pass/fail judged on the odometry track by the standalone [`tests/waypoint_checker.py`](tests/waypoint_checker.py) (in-order corridor arrival within `--waypoint-tolerance`, final goal within `--goal-tolerance`, per-waypoint `--waypoint-timeout`) | Docker, GPU, sim license | -The pytest hooks and the `airstack_env` / `robot_autonomy_stack` fixtures live in [`tests/conftest.py`](tests/conftest.py); the shared helpers are split by concern into the [`tests/harness/`](tests/harness/) package (`session`, `discovery`, `commands`, `containers`, `metrics` (with `MetricsRecorder`), `sim`, `collection`) and re-exported through `conftest`, so `from conftest import ` still resolves. Each run produces a timestamped directory under `tests/results//` with `summary.txt`, `results.xml`, and `metrics.json` (no per-test log files — live output streams to the terminal via `log_cli`). [`tests/parse_metrics.py`](tests/parse_metrics.py) generates a markdown report (single-run or diff-vs-baseline; exits 1 on regression). +The pytest hooks and the `airstack_env` / `robot_autonomy_stack` fixtures live in [`tests/conftest.py`](tests/conftest.py); the shared helpers are split by concern into the [`tests/harness/`](tests/harness/) package (`session`, `discovery`, `commands`, `containers`, `metrics` (with `MetricsRecorder`), `run_meta`, `test_ids`, `sim`, `collection`) and re-exported through `conftest`, so `from conftest import ` still resolves. Each run produces a timestamped directory under `tests/results//` with `summary.txt`, `results.xml`, `run_meta.json`, and `metrics.json` (no per-test log files — live output streams to the terminal via `log_cli`). [`tests/parse_metrics.py`](tests/parse_metrics.py) compares only matching, complete simulation campaigns and exits 1 on a genuine metric regression. **Run via the CLI** (containerized runner — no local Python needed): @@ -280,7 +280,8 @@ GitHub Actions workflows live in [`.github/workflows/`](.github/workflows/): | Workflow | Trigger | Purpose | |----------|---------|---------| -| [`system-tests.yml`](.github/workflows/system-tests.yml) | PR opened, `/pytest` PR comment (write-access only), or `workflow_dispatch` | Runs the `tests/` suite on an ephemeral GPU runner; posts metrics report (with regression diff vs base branch / `main`) as a PR comment and to the job summary | +| [`unit-tests.yml`](.github/workflows/unit-tests.yml) | PR to `main`/`develop` opened, synchronized, or reopened | Runs Python unit tests and harness contracts on `ubuntu-latest` | +| [`system-tests.yml`](.github/workflows/system-tests.yml) | PR opened/synchronized/reopened, `/pytest` PR comment (write-access only), or `workflow_dispatch` | Runs automatic package builds or selected simulation marks on an ephemeral GPU runner; only complete simulation campaigns are compared in metrics reports | | [`docker-build.yml`](.github/workflows/docker-build.yml) | Push to `main`/`develop` that changes `.env` (`VERSION=`), or manual dispatch | Builds, pushes, and cosign-signs all compose images on the ephemeral runner | | [`check-version-increment.yml`](.github/workflows/check-version-increment.yml) | Pull request | Validates `.env` `VERSION=` is valid semver and strictly greater than the base branch | | `deploy_docs_from_{main,develop,release}.yaml` | Push to the matching branch (`docs/**`, `mkdocs.yml`, `*.md`) | Publishes versioned MkDocs site via `mike` | diff --git a/CHANGELOG.md b/CHANGELOG.md index b6a0e9dc6..d1409cd61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Automatic `unit-tests.yml` PR gate on `ubuntu-latest`, plus `run_meta.json` outcome metadata so reports distinguish completed simulation campaigns from collection errors, empty selections, timeouts, and cancellations - `overrides/isaac-optitrack-simulation.env` — brings up Isaac Sim with the NatNet emulator and PX4 flying on mocap EKF2 external vision (GPS/baro/range aiding off), i.e. the configuration `tests/system/test_optitrack_e2e.py` runs, reproducible by hand - `overrides/l4t-optitrack-realrobot.env` — deployment override for a real Jetson robot flying on OptiTrack mocap (PX4 EKF2 external vision instead of GPS): the NatNet server/body settings, plus the multi-NIC and FCU-parameter notes that path needs - Feature notebook workflow (`use-feature-notebook` skill): every agent-implemented feature gets a local, gitignored `notebook/NNN-feature-slug/` entry with a status-tracked `design_spec.md` (written before coding) and `results/` artifacts + self-contained `results_summary.md` that populate the feature's PR description @@ -29,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Default system-test `--sim` is `isaacsim`; pass `--sim msairsim` to opt in to Microsoft AirSim - `-m build_packages` CI runs pull `cache_*` images instead of baking sim images - `docker-build.yml` retags unchanged images on VERSION bumps (content fingerprint) instead of always rebuilding; floating `cache_*` tags still seed PR layer cache -- The PR-open test run is unchanged (pytest's full defaults) and now also covers the OptiTrack Circle-trajectory e2e, which configures its own mocap-EV stack. `optitrack` joins the `heavy` mark list in `system-tests.yml`, so an optitrack run is never misclassified as colcon-only and sent down the pull-only image path +- Automatic OSMO validation runs the pull-only `build_packages` gate whenever a PR is opened, updated, or reopened; GPU-intensive simulation campaigns (including OptiTrack) are selected through `/pytest` or `workflow_dispatch` - `robot-l4t` compose service knobs are now env-overridable (`AUTONOMY_ROLE`, `FCU_URL`, and the rosbag path via `BAG_STORAGE_PATH`); `FCU_URL` unquoted so the literal serial path reaches MAVROS - `zed-l4t` image: ZED SDK 4.2 → 5.2 with the coupled ROS deps (`zed_msgs` 5.2.1, `point_cloud_transport(_plugins)` 4.x, add `backward_ros`) - Unit tests are defined by `tests/colcon_unit_test_packages.yaml`: `conftest.py` collects each listed package's co-located `test/` dir under `--import-mode=importlib` and marks it `unit` (ament lint files are skipped and run under `colcon test`) @@ -40,7 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `pytest tests/` now collects the co-located unit tests, so they run with every `system-tests.yml` invocation. The guard in `tests/conftest.py` skipped injection whenever any path was on the command line, and `tests/` is a path — CI collected 97 of 252 items and the Python unit tests ran nowhere. Narrowing (`pytest tests/system/test_x.py`) still skips injection; the rule is `harness.discovery.collection_is_broad`, pinned by `tests/meta/test_collection_contract.py` +- `pytest tests/` now collects the co-located unit tests before mark filtering. The old guard skipped injection whenever any path was on the command line, and `tests/` is a path — CI collected 97 of 252 items and the Python unit tests ran nowhere. Narrowing (`pytest tests/system/test_x.py`) still skips injection; repository-root and empty-path collection are rejected +- Empty CI pytest arguments no longer become `pytest tests/ ""` and recurse through the repository; collection/import, setup/teardown, partial, and interrupted artifacts are reported as non-comparable instead of false 0% simulation-policy results, and metric regression runs only for an identical simulation campaign fingerprint - Isaac Sim image: PX4 `ubuntu.sh` no longer fails dpkg configure on the NVIDIA base (`ca-certificates` / `software-properties-common`); use `--no-nuttx --no-sim-tools` like ms-airsim - Robot image: pin `pytest<8.1` and disable `launch_testing` for colcon unit tests so ROS Jazzy's outdated pytest hook does not abort `colcon test` - Robot name resolution now honors a pre-set `ROBOT_NAME` (e.g. injected via docker compose) instead of always overriding it from the container/hostname mapping (`robot/docker/.bashrc`) diff --git a/docs/development/intermediate/testing/ci_cd.md b/docs/development/intermediate/testing/ci_cd.md index 8e618efaf..3727642bd 100644 --- a/docs/development/intermediate/testing/ci_cd.md +++ b/docs/development/intermediate/testing/ci_cd.md @@ -1,11 +1,12 @@ # CI/CD Pipeline on OSMO -AirStack's continuous integration runs the **full drone stack** — simulator, -robot autonomy, and GCS — on a GPU for every change. Because that needs a -GPU, a Docker daemon, and a clean filesystem, jobs cannot run on GitHub's -hosted runners and should not run on a shared always-on machine. Instead, a -small orchestrator service watches the GitHub Actions queue and submits one -**ephemeral [NVIDIA OSMO](https://nvidia.github.io/OSMO/) pod per job**. The pod +AirStack continuous integration has two tiers. Fast Python unit and harness +contract tests run on GitHub-hosted runners for updates to PRs targeting +`main` or `develop`. Container, +ROS workspace, and selectable **full drone stack** campaigns — simulator, +robot autonomy, and GCS — run on an ephemeral GPU worker. A small orchestrator +service watches the GitHub Actions queue and submits one **ephemeral +[NVIDIA OSMO](https://nvidia.github.io/OSMO/) pod per system-test job**. The pod registers as a single-use GitHub Actions runner, executes exactly one job, and is destroyed. @@ -24,11 +25,11 @@ to fit CI into your day-to-day development loop. | Question | Answer | |---|---| -| Where do CI jobs run? | A fresh GPU pod on the OSMO `airstack` pool, one per job, destroyed after. | -| What triggers a run? | A PR being **opened**, a `/pytest` comment from a maintainer, or manual `workflow_dispatch`. | -| What gets tested? | Docker image builds, `colcon` builds, unit tests, stack liveliness, sensor rates, takeoff/hover/land, fixed-trajectory tracking. | -| How do I see results? | A metrics report comment on the PR, plus the `test-results-*` artifact (`summary.txt`, `results.xml`, `metrics.json`). | -| What fails the build? | Any failed test, **or** a metric regressing more than 20 % against the base branch's last run. | +| Where do CI jobs run? | Python unit tests: `ubuntu-latest`. Build and simulation tests: a fresh OSMO GPU pod, destroyed afterward. | +| What triggers a run? | PR open/update/reopen runs unit + package-build gates; maintainers select simulations with `/pytest`; `workflow_dispatch` is also available. | +| What gets tested? | Automatically: Python units/contracts and ROS package builds/tests. Selectably: Docker builds, liveliness, sensors, flight policies, and OptiTrack. | +| How do I see results? | Checks plus a report comment and `test-results-*` artifact (`summary.txt`, `results.xml`, `run_meta.json`, `metrics.json`). | +| What fails the build? | Any failed test, or a comparable simulation metric regressing more than 20%. Invalid/incomplete campaigns are labeled, not scored as policy failures. | | Who holds the secrets? | Only the orchestrator host. Workers get a single-use JIT token valid for one registration. | --- @@ -181,12 +182,14 @@ it to Harbor. | Trigger | When it fires | What it runs | |---|---|---| -| `pull_request` (`types: [opened]`) | Only when the PR is first opened, and only for same-repo branches | pytest's `conftest` defaults — the full mark set | +| `unit-tests.yml` pull request | PR to `main`/`develop` opened, synchronized, or reopened (including forks) | `pytest tests/ -m unit` on `ubuntu-latest` | +| `system-tests.yml` pull request | PR opened, synchronized, or reopened, same-repo branches only | `-m build_packages` on an OSMO worker | | `/pytest` PR comment | Any time, from a user with `OWNER`/`MEMBER`/`COLLABORATOR` association | Whatever args you put on the first line of the comment | | `workflow_dispatch` | Manual, from the Actions tab | The form inputs: `marks`, `sim`, `num_robots`, `stress_iterations`, `stable_duration`, `baseline_run_id` | -Pushes to an open PR deliberately do **not** re-trigger. GPU pods are a shared -resource, so re-runs are opt-in via `/pytest`. +PR pushes re-run the fast unit gate and the pull-only `build_packages` gate. +GPU-intensive simulations do **not** run automatically; select the campaign +whose policy or integration changed with `/pytest`. ### Comment syntax @@ -382,7 +385,8 @@ Run one mark at a time unless you genuinely need both. After `run-tests` finishes — pass or fail — a `report` job on `ubuntu-latest` downloads the current artifact plus a **baseline** artifact and runs [`parse_metrics.py`](https://github.com/castacks/AirStack/blob/main/tests/parse_metrics.py) -in diff mode. +in diff mode only when both artifacts have the same complete simulation +campaign fingerprint (selected tests and parameters). | Run type | Baseline used | |---|---| @@ -390,13 +394,19 @@ in diff mode. | `workflow_dispatch` with `baseline_run_id` | That specific run | | `workflow_dispatch` without it | Latest artifact on `main` | -The comment has three sections per test module: a flat **Metrics** table, a -**Sim publishing rates** pivot (topic Hz aggregates from the `sensors` mark), -and a **Compute usage** pivot (CPU / memory / GPU per container). Regressions -are marked with a red circle, improvements with a green one, and the job -**fails** if any metric moves more than the 20 % threshold in the wrong -direction. That is the mechanism that catches slow degradation — the kind of -change where nothing throws but the tracker is quietly 30 % worse. +For a complete simulation campaign, the comment has pass rates plus a flat +**Metrics** table, a **Sim publishing rates** pivot (topic Hz aggregates from +the `sensors` mark), and a **Compute usage** pivot (CPU / memory / GPU per +container). Regressions are marked with a red circle, improvements with a +green one, and the job **fails** if any comparable metric moves more than the +20% threshold in the wrong direction. + +`run_meta.json` separates those policy results from CI failures. A collection +error, zero-test selection, internal pytest error, cancellation, or timeout is +reported as **simulation metrics are not comparable**. Pass-rate and regression +tables are suppressed in that case; the infrastructure problem cannot appear as +a false 0% policy score. A policy assertion that runs and fails remains a real +simulation result and keeps its recorded error metrics. ### The artifact @@ -406,6 +416,7 @@ change where nothing throws but the tracker is quietly 30 % worse. tests/results/2026-08-06_14-30-00/ ├── summary.txt # human-readable per-chain summary — open this first ├── results.xml # JUnit XML: durations, pass/fail per test +├── run_meta.json # completion state, pytest exit, selected/executed sim counts └── metrics.json # every recorded metric, including time series ``` @@ -442,9 +453,12 @@ flowchart TD l --> pr s --> pr a --> pr - pr --> ci["Full suite runs on the ephemeral GPU pod"] - ci --> rep["Read the metrics comment"] - rep --> iter["/pytest with a narrowed mark to confirm a fix"] + pr --> fast["unit-tests.yml on ubuntu-latest"] + pr --> ci["build_packages on an ephemeral OSMO pod"] + fast --> rep["Read automatic check results"] + ci --> rep + rep --> iter["/pytest with the relevant simulation mark"] + iter --> metrics["Read like-for-like policy metrics"] ``` Practical rules that follow from how the system is built: @@ -453,7 +467,7 @@ Practical rules that follow from how the system is built: - **Narrow before you re-run.** A `/pytest` with no args re-runs everything. `/pytest -m autonomy --sim msairsim --trajectory-types Circle` re-runs the one chain you are fixing, in a fraction of the time. - **Never trust a green launch test against a stale build.** This is why `build_packages` is auto-prepended; keep it that way when writing your own `/pytest` line. - **Read `summary.txt` before the raw log.** It groups each flight chain with per-phase wall times and status, so the failing phase is obvious without scrolling a 40-minute log. -- **Treat the metrics diff as a review artifact.** A PR that turns a metric red needs an explanation in the thread, even when every test passed. +- **Treat a like-for-like metrics diff as a review artifact.** The reporter compares only identical selected simulation campaigns; a PR that turns a metric red needs an explanation even when every assertion passed. - **Bump `VERSION` in `.env` when image content changes.** [`check-version-increment.yml`](https://github.com/castacks/AirStack/blob/main/.github/workflows/check-version-increment.yml) gates the PR on a strictly-greater semver, and merging that bump is what triggers the release build below. --- @@ -486,7 +500,8 @@ covers that digest; the job re-signs the same digest under the new tags’ refs) | Workflow | Runner | Purpose | |---|---|---| -| `system-tests.yml` | Ephemeral OSMO GPU pod | Full test suite + metrics report | +| `unit-tests.yml` | `ubuntu-latest` | Python unit tests and harness contracts on updates to PRs targeting `main`/`develop` | +| `system-tests.yml` | Ephemeral OSMO GPU pod | Automatic package-build gate and selectable simulation campaigns + metrics report | | `docker-build.yml` | Ephemeral OSMO GPU pod | Retag or rebuild, push, and sign compose images | | `check-version-increment.yml` | `ubuntu-latest` | Semver gate on `.env` `VERSION=` | | `deploy_docs_from_*.yaml` | `ubuntu-latest` | Versioned MkDocs publish via `mike` | @@ -525,7 +540,8 @@ down the list. | `Cannot connect to the Docker daemon` mid-test | Pod | Inner dockerd crashed — `osmo workflow exec "$WF" runner`, then read `/var/log/dockerd.log` | | `No space left on device` | Pod | Bump `storage` in `config.yaml`; Isaac assets plus all images are large | | Runner registered, then pytest failed | Tests | A real test failure — the GitHub Actions log and `summary.txt` are canonical | -| Metrics report job failed with no test failures | Report | A metric regressed past the 20 % threshold; read the diff table | +| Report says “simulation metrics are not comparable” | Collection/infrastructure | Read the run outcome and pytest exit status in `run_meta.json`; no policy regression was scored | +| Metrics report job failed with no test failures | Report | A like-for-like metric regressed past the 20% threshold, or report generation itself failed; read the report step log | To map a GitHub job to its pod: @@ -548,6 +564,7 @@ Full runbook, including credential rotation and worker-side diagnostics: | Path | Role | |---|---| +| [`.github/workflows/unit-tests.yml`](../../../../.github/workflows/unit-tests.yml) | Fast Python unit/harness gate on GitHub-hosted runners | | [`.github/workflows/system-tests.yml`](https://github.com/castacks/AirStack/blob/main/.github/workflows/system-tests.yml) | The test workflow: triggers, arg parsing, image prep, pytest, artifact, metrics report | | [`.github/orchestrator/orchestrator.py`](https://github.com/castacks/AirStack/blob/main/.github/orchestrator/orchestrator.py) | The spawn and reap loops, GitHub polling, JIT minting, OSMO CLI plumbing | | [`.github/orchestrator/runner-workflow.yaml.j2`](https://github.com/castacks/AirStack/blob/main/.github/orchestrator/runner-workflow.yaml.j2) | Per-job OSMO workflow template | diff --git a/docs/development/intermediate/testing/index.md b/docs/development/intermediate/testing/index.md index 8332c6e77..8d35551b5 100644 --- a/docs/development/intermediate/testing/index.md +++ b/docs/development/intermediate/testing/index.md @@ -22,7 +22,8 @@ airstack test -m unit -v pytest tests/ -m unit -v ``` -Unit tests run as part of `system-tests.yml` via `pytest tests/`, and can also be run +Unit tests run automatically on every update to PRs targeting `main` or +`develop` through `unit-tests.yml` on `ubuntu-latest`, and can also be run locally with no Docker or GPU needed. → **[Unit Testing Guide](unit_testing.md)** — patterns, CI workflow, @@ -87,4 +88,4 @@ airstack test -m "build_packages or autonomy" \ - [Unit Testing](unit_testing.md) — `@pytest.mark.unit`, co-located tests, CI workflow - [Testing frameworks](testing_frameworks.md) — `colcon test`, rostest patterns - [Integration testing](integration_testing.md) -- [CI/CD Pipeline on OSMO](ci_cd.md) — how CI runs the full stack on ephemeral GPU pods: architecture, triggers, what each mark catches, and the metrics regression gate +- [CI/CD Pipeline on OSMO](ci_cd.md) — automatic unit/build gates, selectable full-stack GPU campaigns, triggers, and like-for-like metrics reporting diff --git a/docs/development/intermediate/testing/unit_testing.md b/docs/development/intermediate/testing/unit_testing.md index a616170b0..84423ceb0 100644 --- a/docs/development/intermediate/testing/unit_testing.md +++ b/docs/development/intermediate/testing/unit_testing.md @@ -1,6 +1,6 @@ # Unit Testing -AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds via `airstack test -m unit`, and ride along with every `system-tests.yml` run in CI. +AirStack unit tests are **fast, hermetic, and purely Python** — no Docker stack, no GPU, no running containers. They run locally in seconds via `airstack test -m unit` and automatically on every update to PRs targeting `main` or `develop` through `unit-tests.yml`. ## Design principles @@ -41,7 +41,7 @@ pip install -r tests/requirements.txt pytest tests/ -m unit -v ``` -Unit tests complete in under one second for the current suite. +The current suite completes in about 20 seconds on a developer workstation. ## CI @@ -49,14 +49,14 @@ Unit tests complete in under one second for the current suite. not.** A gtest is a binary compiled against the package's headers and rclcpp, so it only runs where the ROS toolchain is — `colcon test` inside the robot container. Python unit tests stub ROS at the import boundary and touch no ROS runtime, so they need neither a -build nor a container, which is what keeps the whole suite under a second. Both are +build nor a container, which keeps the whole suite in the fast feedback tier. Both are gated in CI: | Test | Runner | In CI via | |---|---|---| | C++ gtest | `colcon test` inside the robot container | the `build_packages` mark (`tests/system/test_build_packages.py::test_colcon_test_robot`) | -| Python, `ament_python` package | root harness **and** `colcon test` | `pytest tests/` **and** `build_packages` | -| Python, `ament_cmake` package | root harness only | `pytest tests/` | +| Python, `ament_python` package | root harness **and** `colcon test` | `unit-tests.yml` **and** `build_packages` | +| Python, `ament_cmake` package | root harness only | `unit-tests.yml` | `colcon test` picks up Python tests only when the package's build type makes it: an `ament_python` package like `lidar_point_cloud_filter` exposes them through @@ -64,10 +64,11 @@ gated in CI: would need an explicit `ament_add_pytest_test` — it has none, so its Python tests reach CI only through the root harness. -Python unit tests are collected by `system-tests.yml`'s `pytest tests/` invocation, so -they run on every trigger of that workflow: PR open, a `/pytest` comment, or -`workflow_dispatch`. That is deliberately not every push — the same run also drives the -GPU system tests. Run them locally in the meantime, no infrastructure required: +Python unit tests are collected by `unit-tests.yml`'s `pytest tests/ -m unit` +invocation on PR open, synchronize, and reopen. That job uses GitHub-hosted +`ubuntu-latest`; it does not queue for an OSMO GPU. The OSMO `system-tests.yml` +invocation uses the same safe `tests/` collection boundary, but mark filtering may +deselect unit tests for targeted build/simulation runs. Run the same gate locally with: ```bash airstack test -m unit -v diff --git a/osmo/README.md b/osmo/README.md index 91b41dbd5..e3f6041bd 100644 --- a/osmo/README.md +++ b/osmo/README.md @@ -21,9 +21,9 @@ README is the **lab admin / operator** reference: pool requirements, workspace image build & push, validation stages, plus a credential summary for context. -> **Scope:** developer workflow only. CI/CD on OSMO is **not** part of this -> integration — the existing `system-tests.yml` + OpenStack orchestrator path -> is unchanged. +> **Scope:** this directory documents the interactive developer workflow. +> AirStack CI also uses OSMO, but through the separate ephemeral-runner +> orchestrator in [`.github/orchestrator/`](../.github/orchestrator/). ## Architecture in one minute @@ -296,6 +296,6 @@ If you see Isaac Sim's "Login Required" popup at startup: layout leaves room for additional workflow files when this is done. - **Persistent workspace** — mount `/root/AirStack` to a PVC so uncommitted edits survive `osmo workflow cancel`. Pool-policy dependent. -- **CI/CD on OSMO** — the existing `.github/workflows/system-tests.yml` + - OpenStack ephemeral runner path is unchanged. Migrating CI to OSMO is a - separate effort. +- **Shared interactive/CI worker design** — CI already runs on one-shot OSMO + pods through `.github/orchestrator/`; this interactive workspace intentionally + remains a separate image and lifecycle. diff --git a/tests/README.md b/tests/README.md index 558c3fe15..306567a1a 100644 --- a/tests/README.md +++ b/tests/README.md @@ -117,7 +117,7 @@ Writes custom metrics to `tests/results//metrics.json` after each `re ### Output files Every test run produces a timestamped directory containing only `summary.txt`, -`results.xml`, and `metrics.json` — there is **no** `logs/` subdirectory and no +`results.xml`, `run_meta.json`, and `metrics.json` — there is **no** `logs/` subdirectory and no per-test log files are written under the run directory. ``` @@ -125,6 +125,7 @@ tests/results/ └── 2025-04-21_14-30-00/ ├── summary.txt # Human-readable key metrics — open this first ├── results.xml # JUnit XML — test durations and pass/fail status + ├── run_meta.json # Completion/outcome and campaign fingerprint └── metrics.json # Custom metrics (image sizes, Hz, compute, timing) ``` @@ -517,13 +518,17 @@ python tests/parse_metrics.py \ Prints a side-by-side comparison. Exits **1** if any metric regresses beyond the threshold; exits 0 otherwise. -The report has three sections per test module: +For a completed test campaign, the report has three sections per test module: - **Metrics** — flat table of scalar metrics (test name, metric key, value/baseline, change%) - **Sim publishing rates** — pivot table of topic Hz aggregates from the `sensors` mark (`mean`, `start_mean`, `end_mean`, `min`, `max`; sim + robot topics) - **Compute usage** — pivot table of CPU/memory/GPU metrics per container Regressions are flagged with :red_circle:, improvements with :green_circle:. +Collection errors, command/internal errors, zero-test runs, and jobs that stop before +pytest finalizes are labeled **not comparable**. Their pass-rate and regression tables +are suppressed so an infrastructure failure cannot appear as 0% policy performance. +`run_meta.json` records the pytest exit status and simulation tests selected/completed. --- @@ -534,19 +539,25 @@ Regressions are flagged with :red_circle:, improvements with :green_circle:. reference, what each mark catches, and how to fold CI into your development loop — see **[CI/CD Pipeline on OSMO](../docs/development/intermediate/testing/ci_cd.md)**. -### Workflow: `system-tests.yml` +### CI workflows -[`.github/workflows/system-tests.yml`](../../../../.github/workflows/system-tests.yml) runs on: +[`.github/workflows/unit-tests.yml`](../.github/workflows/unit-tests.yml) +runs all Python unit and harness-contract tests on `ubuntu-latest` whenever a PR is +opened, updated, or reopened against `main` or `develop`. -- **Pull requests** to `main` or `develop` — automatically runs `build_docker or build_packages` tests (no GPU-intensive liveliness run on every PR) +[`.github/workflows/system-tests.yml`](../.github/workflows/system-tests.yml) runs on: + +- **Same-repository pull requests** when opened, updated, or reopened — automatically + runs `build_packages` on OSMO (no GPU-intensive simulation campaign on every push) +- **`/pytest` PR comments** from maintainers — runs the requested registered marks - **Manual dispatch** (`workflow_dispatch`) — fully configurable for liveliness runs and metric comparisons #### Manual dispatch inputs | Input | Default | Description | |-------|---------|-------------| -| `marks` | `liveliness` | pytest marks expression | -| `sim` | `msairsim` | Sim targets | +| `marks` | `liveliness or takeoff_hover_land` | pytest marks expression | +| `sim` | `isaacsim` | Sim targets | | `num_robots` | `1` | Robot counts | | `stress_iterations` | `1` | Iterations per config | | `stable_duration` | `120` | Stability polling seconds | @@ -560,9 +571,9 @@ Regressions are flagged with :red_circle:, improvements with :green_circle:. 1. Downloads the current artifact 2. Downloads a baseline artifact (from the base branch for PRs, from `main` for manual runs, or from the specified `baseline_run_id`) -3. Runs `parse_metrics.py` in diff mode if a baseline is found, otherwise in single-run mode +3. Runs `parse_metrics.py` in diff mode only when both artifacts have the same complete simulation campaign fingerprint; otherwise reports the current run without comparison 4. Posts the markdown report as a PR comment (PR runs) or to the job summary (all runs) -5. Fails with `::error::` if `parse_metrics.py` exits 1 (regression detected) +5. Fails with `::error::` only for a comparable metric regression; invalid/incomplete campaigns are reported as infrastructure outcomes #### Required third-party action @@ -610,7 +621,7 @@ AirStack's tests require a GPU, Docker, and a clean filesystem per run, so they ### Setup -The orchestrator service code, OSMO runner-workflow template, runner image, systemd unit, and full setup runbook live in [`.github/orchestrator/`](../../../../.github/orchestrator/). See [`.github/orchestrator/README.md`](ci-cd-orchestrator.md) for: +The orchestrator service code, OSMO runner-workflow template, runner image, systemd unit, and full setup runbook live in [`.github/orchestrator/`](../.github/orchestrator/). See [`.github/orchestrator/README.md`](ci-cd-orchestrator.md) for: - obtaining the OSMO service-account token and a dedicated CI GPU pool (with privileged mode enabled) - building and pushing the runner image (`runner.Dockerfile`) diff --git a/tests/conftest.py b/tests/conftest.py index d79b2f5f7..38aec5c5f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,12 +11,13 @@ if _TESTS_DIR not in sys.path: sys.path.insert(0, _TESTS_DIR) -from harness import collection, session +from harness import collection, session as harness_session # Re-export the harness helper API so existing `from conftest import ` in the # system tests + sensor_probes keeps working unchanged. from harness import * # noqa: F401,F403 from harness.commands import _nodeid_dotted from harness.discovery import _is_unit_item +from harness.run_meta import write_run_meta # ── pytest config / hooks ────────────────────────────────────────────────── @@ -74,7 +75,7 @@ def pytest_addoption(parser): def pytest_configure(config): - run_dir = session.init_run_dir(AIRSTACK_ROOT) + run_dir = harness_session.init_run_dir(AIRSTACK_ROOT) config.option.xmlpath = str(run_dir / "results.xml") # Co-located unit tests import their own package (e.g. `optitrack.natnet.emulator`, @@ -116,18 +117,36 @@ def pytest_itemcollected(item): def pytest_runtest_setup(item): - session.set_current_item(item) + harness_session.set_current_item(item) def pytest_runtest_teardown(item): - session.set_current_item(None) + harness_session.set_current_item(None) -def pytest_sessionfinish(exitstatus): - """Write summary.txt with key metrics so users don't need to dig through logs.""" - run_dir = session.run_dir() +@pytest.hookimpl(trylast=True) +def pytest_sessionfinish(session, exitstatus): + """Persist run outcome metadata and a human-readable summary.""" + run_dir = harness_session.run_dir() if run_dir is None: return + try: + terminal = session.config.pluginmanager.getplugin("terminalreporter") + reports = [ + report + for entries in getattr(terminal, "stats", {}).values() + for report in entries + ] + meta_path = write_run_meta( + run_dir, + session.items, + exitstatus, + session.config.option.markexpr, + reports, + ) + logger.info("Wrote run metadata to %s", meta_path) + except Exception as exc: + logger.warning("Failed to write run metadata: %s", exc) try: from run_summary import write_summary summary_path = write_summary(run_dir) @@ -186,7 +205,7 @@ def airstack_env(request): # test id (see pytest_collection_modifyitems), so airstack up/down output # lands next to the triggering test's own log instead of under pytest's # stale callspec.id. - log = f"airstack_env.{_nodeid_dotted(session.current_item().nodeid, with_path_sep=True)}" + log = f"airstack_env.{_nodeid_dotted(harness_session.current_item().nodeid, with_path_sep=True)}" headless = not request.config.getoption("--gui") env_overrides = { diff --git a/tests/harness/collection.py b/tests/harness/collection.py index 6bf209abf..f0eada6ba 100644 --- a/tests/harness/collection.py +++ b/tests/harness/collection.py @@ -17,6 +17,7 @@ # Harness contract tests: hermetic, and they guard the collection of everything # above, so they belong with the fast tier rather than after the sim suites. "test_collection_contract", + "test_metrics_reporting_contract", # System tests follow in dependency order. "system.test_build_docker", "system.test_build_packages", diff --git a/tests/harness/discovery.py b/tests/harness/discovery.py index 1d4731f1a..b54fb5644 100644 --- a/tests/harness/discovery.py +++ b/tests/harness/discovery.py @@ -165,29 +165,28 @@ def _arg_path(arg, invocation_dir): def collection_is_broad(args, invocation_dir, tests_root=None) -> bool: - """True when the positionals do not narrow the run below ``tests/``. + """True only when the positional names the complete ``tests/`` harness. Co-located unit tests live outside ``tests/``, so ``pytest_configure`` appends them to ``config.args`` by hand. It must do that only for a run that already means "everything", or ``pytest tests/system/test_x.py`` would drag in every unit test. - Broad == a positional names ``tests/`` itself or an ancestor of it:: + Repository-root collection is intentionally *not* broad: importing every + ``test_*.py`` under ROS, Isaac Sim, and vendored submodules on the host is invalid. pytest (testpaths ``.``, cwd tests/) -> broad pytest tests/ (CI, and the documented commands) -> broad - pytest . (cwd repo root or tests/) -> broad + pytest . (cwd repo root) -> invalid/narrow pytest tests/system -> narrow pytest tests/system/test_x.py::TestY::test_z -> narrow pytest ../simulation/.../test/test_frames.py -> narrow - ``any`` rather than ``all`` is deliberate: ``pytest_configure`` appends the - co-located files (narrow, absolute) to ``config.args``, so ``all`` would flip the - answer for anything re-deriving it after that mutation. + Exactly one non-empty positional is required so an accidental empty argument + cannot silently add the repository root to pytest's recursion. """ root = Path(tests_root or TESTS_DIR).resolve() invocation_dir = Path(invocation_dir).resolve() - return any( - root.is_relative_to(_arg_path(a, invocation_dir)) - for a in args - if not str(a).startswith("-") - ) + positionals = [str(arg) for arg in args if not str(arg).startswith("-")] + if len(positionals) != 1 or not positionals[0]: + return False + return _arg_path(positionals[0], invocation_dir) == root diff --git a/tests/harness/run_meta.py b/tests/harness/run_meta.py new file mode 100644 index 000000000..8cd0821a9 --- /dev/null +++ b/tests/harness/run_meta.py @@ -0,0 +1,310 @@ +"""Run-level outcome metadata for honest CI and metrics reporting.""" + +from __future__ import annotations + +import hashlib +import json +import xml.etree.ElementTree as ET +from pathlib import Path + +from harness.test_ids import canonical_test_id + + +RUN_META_FILENAME = "run_meta.json" + +SIMULATION_MODULES = ( + "system.test_liveliness.", + "system.test_sensors.", + "system.test_takeoff_hover_land.", + "system.test_fixed_trajectory.", + "system.test_waypoint_flight.", + "system.test_optitrack_e2e.", +) + + +def is_simulation_test_id(test_id: str) -> bool: + """Whether a test belongs to a GPU/simulation campaign.""" + canonical = canonical_test_id(test_id) + return canonical.startswith(SIMULATION_MODULES) + + +def campaign_fingerprint(test_ids) -> str: + """Stable identity for the exact selected simulation campaign.""" + canonical_ids = sorted( + canonical_test_id(str(test_id).replace("::", ".")).replace(".py.", ".") + for test_id in test_ids + ) + if not canonical_ids: + return "" + payload = "\n".join(canonical_ids).encode() + return hashlib.sha256(payload).hexdigest() + + +def _item_outcome(item) -> str | None: + """Return the final outcome recorded on a pytest item.""" + reports = [ + getattr(item, "_rep_setup", None), + getattr(item, "_rep_call", None), + getattr(item, "_rep_teardown", None), + ] + if any(rep is not None and rep.failed for rep in reports): + return "failed" + if any(rep is not None and rep.skipped for rep in reports): + return "skipped" + call = getattr(item, "_rep_call", None) + if call is not None and call.passed: + return "passed" + return None + + +def _report_details(reports) -> tuple[dict[str, str], set[str], set[str]]: + """Collapse phase reports and identify items that reached call phase.""" + priority = {"passed": 0, "skipped": 1, "failed": 2} + outcomes = {} + call_nodeids = set() + infrastructure_error_nodeids = set() + for report in reports or []: + nodeid = getattr(report, "nodeid", None) + when = getattr(report, "when", None) + if not nodeid or when not in ("setup", "call", "teardown"): + continue + if report.failed: + outcome = "failed" + if when != "call": + infrastructure_error_nodeids.add(nodeid) + elif report.skipped: + outcome = "skipped" + elif when == "call" and report.passed: + outcome = "passed" + else: + continue + if when == "call": + call_nodeids.add(nodeid) + previous = outcomes.get(nodeid, "passed") + outcomes[nodeid] = max((previous, outcome), key=priority.get) + return outcomes, call_nodeids, infrastructure_error_nodeids + + +def build_run_meta(items, exitstatus: int, mark_expression: str = "", + reports=None) -> dict: + """Build serializable run metadata from a completed pytest session.""" + report_outcomes, call_nodeids, infrastructure_error_nodeids = _report_details( + reports + ) + if report_outcomes: + completed_by_id = report_outcomes + else: + completed_by_id = { + str(item.nodeid): outcome + for item in items + if (outcome := _item_outcome(item)) is not None + } + call_nodeids = { + str(item.nodeid) + for item in items + if getattr(item, "_rep_call", None) is not None + } + infrastructure_error_nodeids = { + str(item.nodeid) + for item in items + if any( + report is not None and report.failed + for report in ( + getattr(item, "_rep_setup", None), + getattr(item, "_rep_teardown", None), + ) + ) + } + completed = list(completed_by_id.values()) + simulation_items = [ + item for item in items if is_simulation_test_id(str(item.nodeid)) + ] + simulation_completed = [ + item for item in simulation_items if str(item.nodeid) in call_nodeids + ] + simulation_infrastructure_errors = [ + item + for item in simulation_items + if str(item.nodeid) in infrastructure_error_nodeids + ] + + if exitstatus == 2: + # Pytest uses exit 2 for both collection aborts and user/runner + # interruption. Reports prove that execution had already begun. + outcome = "incomplete" if completed else "collection_error" + elif exitstatus in (3, 4): + outcome = "internal_error" + elif exitstatus == 5 or not items: + outcome = "no_tests" + elif not call_nodeids: + outcome = ( + "simulation_not_executed" if simulation_items + else "tests_not_executed" + ) + elif simulation_items and not simulation_completed: + outcome = "simulation_not_executed" + elif simulation_infrastructure_errors: + outcome = "incomplete" + elif len(simulation_completed) != len(simulation_items): + outcome = "incomplete" + elif simulation_items: + outcome = "simulation" + else: + outcome = "non_simulation" + + return { + "schema_version": 1, + "complete": outcome != "incomplete", + "outcome": outcome, + "pytest_exitstatus": int(exitstatus), + "mark_expression": mark_expression, + "selected_tests": len(items), + "completed_tests": len(completed), + "passed": completed.count("passed"), + "failed": completed.count("failed"), + "skipped": completed.count("skipped"), + "simulation_selected": len(simulation_items), + "simulation_completed": len(simulation_completed), + "campaign_fingerprint": campaign_fingerprint( + item.nodeid for item in simulation_items + ), + } + + +def write_run_meta(run_dir: Path, items, exitstatus: int, + mark_expression: str = "", reports=None) -> Path: + """Write ``run_meta.json`` for a normally completed pytest session.""" + path = Path(run_dir) / RUN_META_FILENAME + path.write_text(json.dumps( + build_run_meta(items, exitstatus, mark_expression, reports), + indent=2, + sort_keys=True, + ) + "\n") + return path + + +def _classify_junit(results_xml: Path) -> dict: + """Infer legacy run state when ``run_meta.json`` is unavailable.""" + cases = list(ET.parse(results_xml).iter("testcase")) + errors = sum(tc.find("error") is not None for tc in cases) + failures = sum(tc.find("failure") is not None for tc in cases) + skipped = sum(tc.find("skipped") is not None for tc in cases) + simulation = sum( + is_simulation_test_id(f"{tc.get('classname')}.{tc.get('name')}") + for tc in cases + ) + simulation_ids = [ + f"{tc.get('classname')}.{tc.get('name')}" + for tc in cases + if is_simulation_test_id(f"{tc.get('classname')}.{tc.get('name')}") + ] + simulation_errors = sum( + tc.find("error") is not None + for tc in cases + if is_simulation_test_id(f"{tc.get('classname')}.{tc.get('name')}") + ) + + if errors: + outcome = "incomplete" if simulation else "collection_error" + elif not cases: + outcome = "no_tests" + elif simulation: + outcome = "simulation" + else: + outcome = "non_simulation" + + return { + "schema_version": 1, + "complete": outcome != "incomplete", + "outcome": outcome, + "pytest_exitstatus": None, + "mark_expression": "", + "selected_tests": len(cases), + "completed_tests": len(cases), + "passed": len(cases) - errors - failures - skipped, + "failed": errors + failures, + "skipped": skipped, + "simulation_selected": simulation, + "simulation_completed": simulation - simulation_errors, + "campaign_fingerprint": campaign_fingerprint(simulation_ids), + "inferred": True, + } + + +def classify_run(run_dir: Path) -> dict: + """Read run metadata or infer whether an artifact is comparable.""" + run_dir = Path(run_dir) + meta_path = run_dir / RUN_META_FILENAME + if meta_path.exists(): + try: + meta = json.loads(meta_path.read_text()) + except (OSError, json.JSONDecodeError) as exc: + return { + "schema_version": 1, + "complete": False, + "outcome": "incomplete", + "reason": f"Run metadata could not be read: {exc}", + } + if meta.get("outcome") in ("simulation", "non_simulation"): + results_xml = run_dir / "results.xml" + if not results_xml.exists(): + return { + "schema_version": 1, + "complete": False, + "outcome": "incomplete", + "reason": "Run metadata exists but JUnit results are missing.", + } + try: + ET.parse(results_xml) + except (OSError, ET.ParseError) as exc: + return { + "schema_version": 1, + "complete": False, + "outcome": "incomplete", + "reason": f"JUnit results were not finalized: {exc}", + } + return meta + + results_xml = run_dir / "results.xml" + if results_xml.exists(): + try: + return _classify_junit(results_xml) + except (OSError, ET.ParseError) as exc: + return { + "schema_version": 1, + "complete": False, + "outcome": "incomplete", + "reason": f"JUnit results were not finalized: {exc}", + } + + if (run_dir / "metrics.json").exists(): + return { + "schema_version": 1, + "complete": False, + "outcome": "incomplete", + "reason": "Metrics exist but pytest did not finalize JUnit/run metadata.", + } + + return { + "schema_version": 1, + "complete": False, + "outcome": "missing_results", + "reason": "No pytest result artifact was produced.", + } + + +def simulation_metrics_comparable(meta: dict, baseline: dict | None = None) -> bool: + """Whether a complete simulation may be compared with a like campaign.""" + valid = bool( + meta + and meta.get("complete") + and meta.get("outcome") == "simulation" + and meta.get("campaign_fingerprint") + ) + if not valid or baseline is None: + return valid + return bool( + baseline.get("complete") + and baseline.get("outcome") == "simulation" + and baseline.get("campaign_fingerprint") == meta["campaign_fingerprint"] + ) diff --git a/tests/harness/test_ids.py b/tests/harness/test_ids.py new file mode 100644 index 000000000..5b0fd825c --- /dev/null +++ b/tests/harness/test_ids.py @@ -0,0 +1,15 @@ +"""Canonical test identifiers shared by metrics and summary reporting.""" + + +def canonical_test_id(name: str) -> str: + """Unify pytest node-id path slashes with JUnit classname dots. + + ``metrics.json`` keys start with paths such as + ``system/test_liveliness.Class.test`` while JUnit uses + ``system.test_liveliness.Class.test``. + """ + head, dot, rest = name.partition(".") + if "/" in head: + head = head.replace("/", ".") + return head + dot + rest if dot else head + return name diff --git a/tests/meta/test_collection_contract.py b/tests/meta/test_collection_contract.py index 0df4751fb..998de89a4 100644 --- a/tests/meta/test_collection_contract.py +++ b/tests/meta/test_collection_contract.py @@ -36,7 +36,6 @@ (_REPO, ["tests/"]), # CI, and the documented commands (_REPO, ["tests"]), (_REPO, ["./tests/"]), - (_REPO, ["."]), (_REPO, [str(TESTS_DIR)]), (TESTS_DIR, ["."]), # testpaths, i.e. `airstack test` (TESTS_DIR, [str(TESTS_DIR)]), @@ -55,6 +54,11 @@ def test_broad_invocations_collect_unit_tests(cwd, args): (_REPO, ["tests/system/test_sensors.py"]), (_REPO, ["tests/integration/natnet"]), (_REPO, ["simulation/isaac-sim/extensions/optitrack.natnet.emulator/test/test_frames.py"]), + (_REPO, ["."]), # never recurse over the repo on host + (_REPO, [""]), + (_REPO, ["tests/", ""]), + (_REPO, ["tests/", "tests/system"]), + (_REPO, []), ], ) def test_narrowed_invocations_do_not(cwd, args): @@ -62,10 +66,11 @@ def test_narrowed_invocations_do_not(cwd, args): def test_ci_invocation_is_broad(): - """The command system-tests.yml runs must collect unit tests. + """The shared system harness path must permit co-located injection. - This is the test that would have caught the original bug: CI ran `pytest tests/`, - which the guard classified as a narrowing run, so no unit test ever executed in CI. + This catches the original bug: CI ran `pytest tests/`, which the guard classified + as narrowed. The CPU workflow executes the injected tests with ``-m unit``; + mark-scoped system runs may intentionally deselect them after safe collection. """ workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() match = re.search(r"^\s*pytest\s+(\S+)", workflow, re.M) @@ -76,6 +81,59 @@ def test_ci_invocation_is_broad(): ) +def test_ci_empty_args_do_not_emit_an_empty_positional(): + """Guard the mapfile bug that changed bare `/pytest` into `pytest tests/ ""`.""" + workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() + assert "sys.stdout.write" in workflow + assert "print('\\\\n'.join(shlex.split" not in workflow + assert "Refusing an empty pytest argument" in workflow + + +def test_cpu_unit_workflow_uses_the_broad_harness_path(): + workflow = repo_path(".github", "workflows", "unit-tests.yml").read_text() + match = re.search(r"^\s*run:\s+pytest\s+(\S+)", workflow, re.M) + assert match, "no `pytest ` invocation found in unit-tests.yml" + assert collection_is_broad([match.group(1)], _REPO) + + +def test_automatic_pr_gates_are_fast_and_repeat_on_updates(): + system = repo_path(".github", "workflows", "system-tests.yml").read_text() + unit = repo_path(".github", "workflows", "unit-tests.yml").read_text() + trigger = "types: [opened, synchronize, reopened]" + assert trigger in system + assert trigger in unit + assert "args = ['-m', 'build_packages']" in system + assert "runs-on: ubuntu-latest" in unit + assert "run: pytest tests/ -m unit" in unit + + +def test_report_uses_the_revision_that_was_actually_tested(): + workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() + assert "tested_sha: ${{ steps.identity.outputs.tested_sha }}" in workflow + assert "test-results-${{ steps.identity.outputs.tested_sha }}" in workflow + assert "test-results-${{ needs.run-tests.outputs.tested_sha }}" in workflow + assert "Number('${{ needs.run-tests.outputs.pr_number }}')" in workflow + + +def test_pr_head_check_is_finalized_after_metrics(): + workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() + assert workflow.index("- name: Finalize check on PR head") > workflow.index( + "- name: Fail on regression" + ) + assert "ref: ${{ needs.run-tests.outputs.tested_sha }}" in workflow + assert "conclusion: '${{ job.status }}'" not in workflow + + +def test_cross_run_baseline_uses_supported_download_inputs(): + workflow = repo_path(".github", "workflows", "system-tests.yml").read_text() + explicit = workflow.split( + "- name: Download baseline results (manual, explicit run ID)", 1 + )[1].split("- name:", 1)[0] + assert "github-token:" in explicit + assert 'pattern: "test-results-*"' in explicit + assert "name_is_regexp:" not in explicit + + def test_injection_actually_produced_items(request): """Every discovered unit-test file contributed at least one collected item. diff --git a/tests/meta/test_metrics_reporting_contract.py b/tests/meta/test_metrics_reporting_contract.py new file mode 100644 index 000000000..f016ec71f --- /dev/null +++ b/tests/meta/test_metrics_reporting_contract.py @@ -0,0 +1,263 @@ +# Copyright (c) 2024 Carnegie Mellon University +# MIT License - see LICENSE in the repository root for full text. +"""Contracts that keep infrastructure failures out of simulation metrics.""" + +import json +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from harness.run_meta import ( + build_run_meta, + campaign_fingerprint, + classify_run, + simulation_metrics_comparable, +) +from parse_metrics import generate_report, merge_metrics + + +pytestmark = pytest.mark.unit + + +def _write_junit(run_dir: Path, testcase: str) -> None: + run_dir.mkdir() + (run_dir / "results.xml").write_text( + '' + f'{testcase}' + ) + + +def _item(nodeid: str, *, failed=False, skipped=False): + report = SimpleNamespace( + failed=failed, + skipped=skipped, + passed=not failed and not skipped, + ) + return SimpleNamespace( + nodeid=nodeid, + _rep_setup=SimpleNamespace(failed=False, skipped=False, passed=True), + _rep_call=report, + _rep_teardown=SimpleNamespace(failed=False, skipped=False, passed=True), + ) + + +def _setup_failed_item(nodeid: str): + return SimpleNamespace( + nodeid=nodeid, + _rep_setup=SimpleNamespace(failed=True, skipped=False, passed=False), + _rep_call=None, + _rep_teardown=None, + ) + + +def _phase_report(nodeid: str, when: str, outcome: str): + return SimpleNamespace( + nodeid=nodeid, + when=when, + failed=outcome == "failed", + skipped=outcome == "skipped", + passed=outcome == "passed", + ) + + +def test_completed_simulation_failure_is_a_valid_campaign(): + meta = build_run_meta( + [_item( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_circle", + failed=True, + )], + exitstatus=1, + mark_expression="autonomy", + ) + assert meta["outcome"] == "simulation" + assert meta["simulation_completed"] == 1 + assert meta["failed"] == 1 + + +def test_collection_exit_is_not_simulation_performance(): + meta = build_run_meta([], exitstatus=2, mark_expression="optitrack") + assert meta["outcome"] == "collection_error" + assert meta["simulation_completed"] == 0 + + +def test_interrupted_partial_simulation_is_not_comparable(): + meta = build_run_meta( + [_item( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_takeoff", + )], + exitstatus=2, + mark_expression="autonomy", + ) + assert meta["outcome"] == "incomplete" + assert meta["complete"] is False + + +def test_setup_only_failure_is_not_policy_performance(): + nodeid = ( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_takeoff" + ) + meta = build_run_meta( + [_setup_failed_item(nodeid)], + exitstatus=1, + mark_expression="autonomy", + reports=[_phase_report(nodeid, "setup", "failed")], + ) + assert meta["outcome"] == "simulation_not_executed" + assert meta["simulation_completed"] == 0 + + +def test_fail_fast_partial_campaign_is_not_comparable(): + first = ( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_takeoff" + ) + meta = build_run_meta( + [ + _item(first, failed=True), + SimpleNamespace( + nodeid=( + "system/test_fixed_trajectory.py::" + "TestFixedTrajectory::test_circle" + ), + _rep_setup=None, + _rep_call=None, + _rep_teardown=None, + ), + ], + exitstatus=1, + mark_expression="autonomy", + reports=[_phase_report(first, "call", "failed")], + ) + assert meta["outcome"] == "incomplete" + assert meta["simulation_completed"] == 1 + assert meta["simulation_selected"] == 2 + + +def test_teardown_error_makes_campaign_incomplete(): + nodeid = ( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_circle" + ) + meta = build_run_meta( + [_item(nodeid)], + exitstatus=1, + mark_expression="autonomy", + reports=[ + _phase_report(nodeid, "call", "passed"), + _phase_report(nodeid, "teardown", "failed"), + ], + ) + assert meta["outcome"] == "incomplete" + assert meta["complete"] is False + + +def test_only_identical_campaigns_are_comparable(): + current = build_run_meta( + [_item( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_circle[a]", + )], + exitstatus=0, + mark_expression="autonomy", + ) + same = dict(current) + different = dict(current, campaign_fingerprint="different") + assert simulation_metrics_comparable(current, same) + assert not simulation_metrics_comparable(current, different) + + +def test_campaign_fingerprint_matches_pytest_and_junit_ids(): + pytest_id = ( + "system/test_fixed_trajectory.py::TestFixedTrajectory::test_circle[a]" + ) + junit_id = ( + "system.test_fixed_trajectory.TestFixedTrajectory.test_circle[a]" + ) + assert campaign_fingerprint([pytest_id]) == campaign_fingerprint([junit_id]) + + +def test_collection_error_report_suppresses_pass_rates(tmp_path): + run_dir = tmp_path / "collection-error" + _write_junit( + run_dir, + '' + "", + ) + + assert classify_run(run_dir)["outcome"] == "collection_error" + markdown, regressed = generate_report(run_dir) + assert "Simulation metrics are not comparable" in markdown + assert "Pass rates" not in markdown + assert regressed is False + + +def test_sim_setup_error_report_is_not_policy_performance(tmp_path): + run_dir = tmp_path / "setup-error" + _write_junit( + run_dir, + '' + '', + ) + + markdown, regressed = generate_report(run_dir) + assert "Simulation metrics are not comparable" in markdown + assert "Pass rates" not in markdown + assert regressed is False + + +def test_incomplete_artifact_is_not_simulation_performance(tmp_path): + run_dir = tmp_path / "incomplete" + run_dir.mkdir() + (run_dir / "metrics.json").write_text("{}") + + markdown, regressed = generate_report(run_dir) + assert "timeout or cancellation" in markdown + assert "Pass rates" not in markdown + assert regressed is False + + +def test_truncated_junit_is_not_simulation_performance(tmp_path): + run_dir = tmp_path / "truncated" + run_dir.mkdir() + (run_dir / "results.xml").write_text("") + + markdown, regressed = generate_report(run_dir) + assert "Simulation metrics are not comparable" in markdown + assert "Pass rates" not in markdown + assert regressed is False + + +def test_real_sim_failure_keeps_metrics_and_pass_rate(tmp_path): + run_dir = tmp_path / "sim-failure" + _write_junit( + run_dir, + '' + '', + ) + (run_dir / "metrics.json").write_text(json.dumps({ + "system/test_fixed_trajectory.TestFixedTrajectory." + "test_circle[isaacsim-iter1]": { + "cross_track_error_mean_m": { + "value": 4.2, + "unit": "m", + "direction": "lower_is_better", + }, + }, + })) + + merged = merge_metrics(run_dir) + assert list(merged) == [ + "system.test_fixed_trajectory.TestFixedTrajectory." + "test_circle[isaacsim]" + ] + only_metrics = next(iter(merged.values())) + assert only_metrics["status"] == "failed" + assert only_metrics["cross_track_error_mean_m"]["value"] == 4.2 + + markdown, regressed = generate_report(run_dir) + assert "### Pass rates" in markdown + assert "cross_track_error_mean_m" in markdown + assert "0%" in markdown + assert "not comparable" not in markdown + assert regressed is False diff --git a/tests/parse_metrics.py b/tests/parse_metrics.py index f2267e627..9d8d77210 100644 --- a/tests/parse_metrics.py +++ b/tests/parse_metrics.py @@ -21,6 +21,9 @@ from tabulate import tabulate +from harness.run_meta import classify_run, simulation_metrics_comparable +from harness.test_ids import canonical_test_id + FLAG_SUFFIX = {"regression": " :red_circle:", "improved": " :green_circle:"} ITER_RE = re.compile(r"-iter(\d+)(?=\])") @@ -173,15 +176,20 @@ def parse_results_xml(path): tree = ET.parse(path) metrics = {} for tc in tree.iter("testcase"): - name = f"{tc.get('classname')}.{tc.get('name')}" - failed = tc.find("failure") is not None + name = canonical_test_id(f"{tc.get('classname')}.{tc.get('name')}") + if tc.find("failure") is not None or tc.find("error") is not None: + status = "failed" + elif tc.find("skipped") is not None: + status = "skipped" + else: + status = "passed" metrics[name] = { "duration_s": { "value": float(tc.get("time", 0)), "unit": "s", "direction": "lower_is_better", }, - "status": "failed" if failed else "passed", + "status": status, } return metrics @@ -210,7 +218,10 @@ def parse_passrates(path): def parse_metrics_json(path): if not path.exists(): return {} - return json.loads(path.read_text()) + return { + canonical_test_id(test_name): metrics + for test_name, metrics in json.loads(path.read_text()).items() + } def merge_metrics(run_dir): @@ -250,10 +261,9 @@ def _collapse_iterations(merged): bucket = out.setdefault(base, {}) for key, val in metrics.items(): if key == "status": - if val == "failed" or bucket.get("status") == "failed": - bucket["status"] = "failed" - else: - bucket["status"] = val + priority = {"passed": 0, "skipped": 1, "failed": 2} + previous = bucket.get("status", "passed") + bucket["status"] = max((previous, val), key=priority.get) continue if isinstance(val, dict) and "samples" in val: series.setdefault((base, key), []).append(val["samples"]) @@ -603,6 +613,92 @@ def render_passrates(mod): return "\n\n".join(sections), has_regression +def _non_comparable_report(meta): + outcome = meta.get("outcome", "unknown") + explanations = { + "collection_error": ( + "Pytest collection failed before a simulation campaign could run." + ), + "internal_error": ( + "Pytest exited with an internal or command-line error." + ), + "no_tests": "No tests were selected or executed.", + "simulation_not_executed": ( + "Simulation tests were selected, but none reached a recorded outcome." + ), + "incomplete": ( + "The runner stopped before pytest finalized its result artifacts " + "(for example, a timeout or cancellation)." + ), + "missing_results": "The test job produced no pytest result artifact.", + } + explanation = explanations.get( + outcome, meta.get("reason", "The run did not complete as a valid test campaign.") + ) + fields = [ + ("Outcome", outcome), + ("Pytest exit status", meta.get("pytest_exitstatus", "unavailable")), + ("Selected tests", meta.get("selected_tests", "unavailable")), + ("Completed tests", meta.get("completed_tests", "unavailable")), + ("Simulation tests completed", meta.get("simulation_completed", 0)), + ] + rows = "\n".join(f"- **{label}:** {value}" for label, value in fields) + return ( + "## Run status\n\n" + f"**Simulation metrics are not comparable.** {explanation}\n\n" + f"{rows}\n\n" + "Pass-rate and regression tables are suppressed because they would " + "misrepresent an infrastructure/collection failure as policy performance." + ) + + +def generate_report(current_dir, baseline_dir=None, threshold=20): + """Generate report markdown and whether a comparable regression exists.""" + current_dir = Path(current_dir) + current_meta = classify_run(current_dir) + if current_meta.get("outcome") not in ("simulation", "non_simulation"): + return _non_comparable_report(current_meta), False + + baseline_meta = classify_run(Path(baseline_dir)) if baseline_dir else None + diff_mode = bool( + baseline_dir + and simulation_metrics_comparable(current_meta, baseline_meta) + ) + + current = merge_metrics(current_dir) + baseline = merge_metrics(Path(baseline_dir)) if diff_mode else {} + current_pr = parse_passrates(current_dir / "results.xml") + baseline_pr = ( + parse_passrates(Path(baseline_dir) / "results.xml") if diff_mode else {} + ) + main_rows, hz_rows, compute_rows, iter_counts = build_rows(current, baseline) + md, has_regression = format_markdown( + main_rows, + hz_rows, + compute_rows, + iter_counts, + current_pr, + baseline_pr, + threshold, + diff_mode, + ) + + notices = [] + if current_meta.get("outcome") == "non_simulation": + notices.append( + "> This was a unit/build-only run. Simulation regression comparison " + "does not apply." + ) + elif baseline_dir and not diff_mode: + notices.append( + "> The baseline is not the same complete simulation campaign. " + "Showing current results without a regression comparison." + ) + if not md: + md = "_No per-test metrics were recorded._" + return "\n\n".join([*notices, md]), has_regression + + def main(): parser = argparse.ArgumentParser( description="Render a markdown report for a test run, or a diff if --baseline is supplied.") @@ -612,23 +708,28 @@ def main(): parser.add_argument("--output", help="Write markdown report to file") args = parser.parse_args() - current = merge_metrics(Path(args.current)) - baseline = merge_metrics(Path(args.baseline)) if args.baseline else {} - current_pr = parse_passrates(Path(args.current) / "results.xml") - baseline_pr = (parse_passrates(Path(args.baseline) / "results.xml") - if args.baseline else {}) - diff_mode = bool(args.baseline) - - main_rows, hz_rows, compute_rows, iter_counts = build_rows(current, baseline) - md, has_regression = format_markdown( - main_rows, hz_rows, compute_rows, iter_counts, - current_pr, baseline_pr, args.threshold, diff_mode) + try: + md, has_regression = generate_report( + args.current, + args.baseline, + args.threshold, + ) + except Exception as exc: + md = ( + "## Report generation failed\n\n" + f"`{type(exc).__name__}: {exc}`\n\n" + "The test result is not being interpreted as a policy regression." + ) + print(md) + if args.output: + Path(args.output).write_text(md) + sys.exit(2) print(md) if args.output: Path(args.output).write_text(md) - sys.exit(1 if diff_mode and has_regression else 0) + sys.exit(1 if has_regression else 0) if __name__ == "__main__": diff --git a/tests/run_summary.py b/tests/run_summary.py index c8ebaac48..3e60e7112 100644 --- a/tests/run_summary.py +++ b/tests/run_summary.py @@ -14,6 +14,9 @@ import xml.etree.ElementTree as ET from pathlib import Path +from harness.run_meta import classify_run +from harness.test_ids import canonical_test_id + PARAM_RE = re.compile(r"\[(.+)\]$") ITER_RE = re.compile(r"-iter\d+$") ROBOT_METRIC_RE = re.compile(r"^robot_\d+\.(.+)$") @@ -61,24 +64,11 @@ } -def _canonical_test_id(name: str) -> str: - """Unify metrics.json path slashes with JUnit classname dots. - - metrics.json keys look like ``system/test_fixed_trajectory.Class.test_x[...]`` - (pytest nodeid). results.xml uses ``system.test_fixed_trajectory.Class.test_x[...]``. - """ - head, dot, rest = name.partition(".") - if "/" in head: - head = head.replace("/", ".") - return head + dot + rest if dot else head - return name - - def _normalize_keyed_map(raw: dict) -> dict: """Merge entries that differ only by path-slash vs dot classname form.""" out: dict = {} for key, value in raw.items(): - out[_canonical_test_id(key)] = value + out[canonical_test_id(key)] = value return out @@ -86,10 +76,14 @@ def _parse_results_xml(path: Path) -> tuple[dict[str, str], dict[str, float]]: """Return ({full_test_name: status}, {full_test_name: wall_time_s}).""" if not path.exists(): return {}, {} + try: + testcases = ET.parse(path).iter("testcase") + except (OSError, ET.ParseError): + return {}, {} statuses: dict[str, str] = {} durations: dict[str, float] = {} - for tc in ET.parse(path).iter("testcase"): - full = _canonical_test_id(f"{tc.get('classname')}.{tc.get('name')}") + for tc in testcases: + full = canonical_test_id(f"{tc.get('classname')}.{tc.get('name')}") if tc.find("failure") is not None or tc.find("error") is not None: statuses[full] = "FAILED" elif tc.find("skipped") is not None: @@ -116,7 +110,7 @@ def _param_id(test_name: str) -> str: def _module_name(test_name: str) -> str: - canonical = _canonical_test_id(test_name) + canonical = canonical_test_id(test_name) match = MODULE_RE.search(canonical) if match: return match.group(1) @@ -125,7 +119,7 @@ def _module_name(test_name: str) -> str: def _phase_name(test_name: str) -> str: """test_fixed_trajectory.TestFixedTrajectory.test_takeoff[...] -> test_takeoff""" - canonical = _canonical_test_id(test_name) + canonical = canonical_test_id(test_name) match = PHASE_RE.search(canonical) if match: return match.group(1) @@ -187,7 +181,7 @@ def _collect_scalar_metrics(metrics_blob: dict) -> dict[str, list[dict]]: def _metrics_blob(metrics: dict, test_name: str) -> dict: - canonical = _canonical_test_id(test_name) + canonical = canonical_test_id(test_name) return metrics.get(canonical, {}) @@ -245,7 +239,7 @@ def _group_tests( ) -> dict[tuple[str, str], list[str]]: """Group full test names by (module, base_param_id) across stress iterations.""" groups: dict[tuple[str, str], list[str]] = {} - all_names = {_canonical_test_id(name) for name in set(metrics) | set(statuses)} + all_names = {canonical_test_id(name) for name in set(metrics) | set(statuses)} for name in sorted(all_names): module = _module_name(name) param = _base_param_id(_param_id(name)) @@ -288,6 +282,7 @@ def _chain_status(test_names: list[str], statuses: dict[str, str]) -> str: def build_summary_lines(run_dir: Path) -> list[str]: metrics_path = run_dir / "metrics.json" results_path = run_dir / "results.xml" + run_meta = classify_run(run_dir) statuses, durations = _parse_results_xml(results_path) metrics = _load_metrics(metrics_path) @@ -302,6 +297,13 @@ def build_summary_lines(run_dir: Path) -> list[str]: f"Overall: {passed} passed, {failed} failed, {skipped} skipped ({total} tests)", "", ] + if run_meta.get("outcome") not in ("simulation", "non_simulation"): + reason = run_meta.get("reason", run_meta.get("outcome", "unknown")) + lines.extend([ + f"Run status: {run_meta.get('outcome', 'unknown')}", + f"Simulation metrics are not comparable: {reason}.", + "", + ]) groups = _group_tests(metrics, statuses, durations) if not groups: