CI/CD Tuning PR - pytest collection bug fix - #384
Merged
Conversation
Unit test source moved into <package>/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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Run fast unit checks automatically, constrain host collection, and distinguish infrastructure failures from comparable simulation results.
Test Metrics —
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What features did you add and/or bugs did you address?
No dedicated issue. This consolidates the unit-test collection improvements from PR #383 and addresses the CI collection/reporting failures encountered while testing PR #376.
This PR prevents bare /pytest runs from accidentally scanning the entire repository and importing ROS, Isaac Sim, Torch, and other container-only dependencies on the host runner.
It also:
No videos or images are applicable because this is CI/test infrastructure rather than a visual feature.
How did you implement it?
The implementation combines PR #383’s co-located unit-test discovery with additional CI safety and reporting changes:
How do you run and use it?
No manual stack launch is required for Python unit tests. Opening or updating a PR targeting main or develop automatically starts:
pytest tests/ -m unit
Same-repository PRs also automatically run the OSMO package gate:
pytest tests/ -m build_packages
To launch an OptiTrack simulation campaign, add this PR comment:
/pytest -m optitrack --sim isaacsim --num-robots 1 --stress-iterations 1
A bare comment safely runs the default suite:
/pytest
From the repository root:
git fetch origin
git checkout ci/reliable-pr-validation
python3 -m venv /tmp/airstack-ci-tests
source /tmp/airstack-ci-tests/bin/activate
python -m pip install -r tests/requirements.txt
export AIRSTACK_ROOT="$PWD"
pytest tests/ -m unit -q
pytest tests/ --collect-only -q
pytest tests/system/test_liveliness.py --collect-only -q
The equivalent AirStack CLI command is:
airstack test -m unit -v
After opening the PR, confirm the Unit Tests and System Tests checks appear. Use a /pytest comment for the specific simulation mark affected by the PR.
Testing with PyTest
Metrics include:
airstack test -m ...airstack test -m unit -v
Direct host-side equivalent:
export AIRSTACK_ROOT="$PWD"
pytest tests/ -m unit -q
Contract tests only:
export AIRSTACK_ROOT="$PWD"
pytest
tests/meta/test_collection_contract.py
tests/meta/test_metrics_reporting_contract.py
-q
Local validation produced:
193 passed unit/contract tests.
97 correctly deselected non-unit tests.
290 tests collected by the bare harness without import errors.
16 tests collected by the narrowed liveliness command.
Maintainers should verify:
unit-tests.yml passes on ubuntu-latest.
The automatic OSMO build_packages check passes.
Bare collection does not import unrelated repository tests.
Narrowed collection remains narrow.
A real simulation assertion failure retains pass rates and metrics.
Infrastructure failures display “Simulation metrics are not comparable.”
Regression tables appear only when campaign fingerprints match.
Documentation
No. No new documentation page was added, so no navigation change was required.
Yes. The testing documentation, tests/README.md, AGENTS.md, OSMO documentation, changelog, and testing skills now explain:
Automatic unit and package-build gates.
How to request simulation campaigns.
Exact local and CI commands.
How co-located tests are discovered.
How to interpret run_meta.json.
How infrastructure failures differ from policy regressions.
Visual media is not applicable because this change affects CI workflows, pytest collection, and metrics reporting. The relevant outputs are GitHub checks, PR comments, job summaries, and test artifacts.
Versioning
.envfile according to semantic versioning?Yes. The branch uses:
VERSION="0.19.0-alpha.17"