From a1b3763191eec8f663a0a36f4206496a3104e3fc Mon Sep 17 00:00:00 2001 From: John Date: Mon, 17 Aug 2026 16:13:03 -0400 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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/5] 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.