diff --git a/.agents/skills/add-unit-tests/SKILL.md b/.agents/skills/add-unit-tests/SKILL.md index d49a36d73..6f7134491 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 pytest tests/ and airstack test -m unit collect 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,37 @@ 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` | 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 — 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` | +|---|---|---| +| `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 — which is +fine, since that is what CI invokes. + +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 @@ -87,13 +114,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 +158,39 @@ 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: +AIRSTACK_ROOT=$(pwd) pytest tests/ -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 +### 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. +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. --- @@ -242,11 +277,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`, `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? | 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 +296,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..29a2d46cb 100644 --- a/.agents/skills/run-system-tests/SKILL.md +++ b/.agents/skills/run-system-tests/SKILL.md @@ -34,7 +34,7 @@ The suite lives at `tests/` (repo root) and is fully pytest-based. Configuration |---|---|---| | 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` | +| 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,8 +42,8 @@ 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 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/.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..b6a0e9dc6 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 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 @@ -33,8 +34,13 @@ 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 +- `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/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..8332c6e77 100644 --- a/docs/development/intermediate/testing/index.md +++ b/docs/development/intermediate/testing/index.md @@ -5,15 +5,15 @@ 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 @@ -22,8 +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 locally with no Docker or GPU needed. +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 dd2f72cf3..a616170b0 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`, and ride along with every `system-tests.yml` run in CI. ## 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,9 +35,9 @@ 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 export AIRSTACK_ROOT=$(pwd) -pip install pytest numpy +pip install -r tests/requirements.txt pytest tests/ -m unit -v ``` @@ -45,9 +45,29 @@ 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: +**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 | +|---|---|---| +| 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 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: ```bash airstack test -m unit -v @@ -73,7 +93,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 +102,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 +138,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 +149,7 @@ across packages don't collide. **3. Verify:** ```bash -pytest tests/ -m unit -v +airstack test -m unit -v ``` ### C++ (gtest) @@ -180,7 +201,7 @@ sim: ``` `pytest tests/ -m unit` discovers them automatically — no changes to `pytest.ini` -or CI changes needed. +or CI 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/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/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/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/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/meta/test_collection_contract.py b/tests/meta/test_collection_contract.py new file mode 100644 index 000000000..0df4751fb --- /dev/null +++ b/tests/meta/test_collection_contract.py @@ -0,0 +1,92 @@ +# 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 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. +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 + ) 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