Skip to content

Ci/trustworthy system tests - #403

Open
pvkumara wants to merge 3 commits into
developfrom
ci/trustworthy-system-tests
Open

Ci/trustworthy system tests#403
pvkumara wants to merge 3 commits into
developfrom
ci/trustworthy-system-tests

Conversation

@pvkumara

Copy link
Copy Markdown
Collaborator

What features did you add and/or bugs did you address?

  • Which GitHub issue does this address?

No linked GitHub issue. This is ci/trustworthy-system-testsdevelop. It closes the original PR #384 / run 32786410874 metrics-job bug (same red X later PRs still hit on develop).

  • Additional description if not fully described in the GitHub issue

This is not a new hole and not AirSim, OptiTrack, or GPU. The tests passed. The red X is the old metrics-job bug: a parser crash that CI then mislabels as a metric regression.

What actually happened on run 32786410874:

Job Result Meaning
Run Tests green, ~7m Automatic PR ran build_packages and it passed
Metrics Report red, ~20s Report Python crashed; the workflow called that a regression

parse_metrics.py died immediately:

ModuleNotFoundError: No module named 'yaml'

The report job still does pip install tabulate only. The parser now imports harness.run_metadiscovery.pyyaml. No PyYAML → crash → no report.md → the summary says “No metrics report generated.”

The next step is Fail on regression. It treats any exit code 1 as:

Metric regression detected — see the report above for details.

There was no baseline (No matching workflow run found with any artifacts on develop), and no numeric comparison ran. The banner is wrong.

This PR is the unmerged fix for that exact failure. Until it lands, develop still runs the old policy.

It also makes the rest of CI outcomes trustworthy so a missing package, a dead simulator, a missing image, or a 20% Hz/CPU change cannot be scored as “the algorithm got worse”:

  • Separate assertion, infrastructure, collection/integrity, and advisory metric failures.

  • Compare metrics only when two runs have the same completed campaign fingerprint (same tests and same CLI/config: sim, robot count, trajectories, velocities, etc.).

  • Do not fail CI on numeric metric deltas. Fail CI on real test failures, infrastructure/prerequisite failures, or a broken report.

  • Capture a bounded, secret-free diagnostics/ bundle when bring-up or readiness fails.

  • Let maintainers run a minimal flight campaign from workflow_dispatch (trajectory_types, takeoff_velocities) before a full matrix.

  • Please add videos and images to demonstrate the feature. Please upload videos to somewhere persistent (e.g. YouTube or Vimeo) for archival purposes.

No flight video (this is CI/report-job reliability, not a vehicle feature). Useful screenshots if attaching: run 32786410874 annotations (green Run Tests, red Metrics Report, No module named 'yaml', “Metric regression detected”); after this lands, a PR Test Metrics comment that treats numeric deltas as advisory (not “Fail on regression”).

How did you implement it?

  • Algorithm details, design decisions, engineering notes, and any other relevant information about the implementation should be included

Three commits:

  1. Make system-test outcomes trustworthy and actionable
  2. Classify readiness failures as infrastructure
  3. Allow focused manual flight campaigns

Direct fix for #384 / run 32786410874

  • Report job installs tests/report-requirements.txt (PyYAML + tabulate), not pip install tabulate only. That is the No module named 'yaml' fix.
  • parse_metrics.py numeric deltas are advisory and exit 0. Parser/integrity failures exit 2 and write a real error into report.md.
  • Workflow step renamed Fail on report integrity error. The string Metric regression detected is removed. A missing baseline or a parser crash is no longer labeled a metric regression.

Failure classes (run_meta.json schema v2)

tests/harness/run_meta.py records failure_class (assertion / infrastructure / collection / ci_integrity / no_tests / none) and a campaign fingerprint over selected sim tests plus behavior-changing options (--sim, --num-robots, --trajectory-types, --takeoff-velocities, tolerances, etc.). Setup/teardown failures and @pytest.mark.infrastructure / “infrastructure prerequisite” / “simulator process failure” messages stay infrastructure, not algorithm assertions.

Advisory metrics

parse_metrics.py always exits 0 on numeric deltas. Parser/integrity failures exit 2. Timeouts/missing values are not scored as numeric regressions. Per-robot keys stay visible (no replica pooling).

Baseline selection

Dropped dawidd6/action-download-artifact. CI downloads recent successful test-results-* artifacts with gh, then harness.baseline.select_baseline_path picks the newest completed, fingerprint-identical campaign.

Diagnostics

harness.diagnostics.collect_failure_diagnostics writes tests/results/<run>/diagnostics/<test>.json: last 200 container log lines, tmux panes, ROS node/topic lists, GPU snapshot, last 30 commands. Secrets are excluded (SAFE_ENV_KEYS only).

Fail-fast readiness

wait_for_first_message takes a health_check. Liveliness/sensors abort if the AirSim/Isaac process dies while waiting for /clock. OptiTrack missing SDK/images fail as infrastructure instead of skip/timeout.

Identity and image prep

/pytest refuses to run if the PR head SHA is unresolved (no default-branch fallback). Image prep writes image-preparation.json (pulled / cache-retagged / locally-built / missing / delegated-to-build-docker).

Config-only CLI

airstack up --config-only validates launch intent without requiring Docker, omni_pass.env, or the Pegasus submodule — used by launch-intent contract tests.

How do you run and use it?

  • What commands and button presses do you use to manually launch the stack to use your new feature?

There is no new robot/sim stack feature to launch. This changes how CI classifies and reports results.

To inspect launch config without Docker/credentials (host):

./airstack.sh up --sim isaac --robots 1 --config-only --dry-run

To exercise the report job on a PR (GitHub UI / comment, not a local stack launch):

  1. Open the PR → comment /pytest -m liveliness --sim isaacsim --num-robots 1 --stress-iterations 1 (write access, same-repo only), or
  2. Actions → System TestsRun workflow → set marks, sim, num_robots, and optionally trajectory_types=Circle / takeoff_velocities=0.5 for a minimal campaign.

Then: Checks → the workflow run → Run Tests for pytest logs; Metrics Report / job Summary for the markdown report; run Summary → Artifacts → test-results-* for summary.txt, run_meta.json, metrics.json, and diagnostics/ on failure.

  • Write a detailed procedure with EXACT BASH COMMANDS so that another maintainer can replicate and understand the benefits of your feature, and reproduce the videos and images you added above.

From the repo root, on ci/trustworthy-system-tests:

# 1) Fast hermetic gate (same mark as unit-tests.yml) — proves the #384 contracts
export AIRSTACK_ROOT="$(pwd)"
python3 -m venv .venv
. .venv/bin/activate
pip install -r tests/requirements.txt
pytest tests/ -m unit -v

# 2) The contracts that specifically lock the metrics-job lie
pytest tests/meta/test_workflow_contract.py \
  tests/meta/test_campaign_reporting_contract.py -v

# 3) Launch-intent / config-only (no Docker, no omni_pass.env, no GPU)
./airstack.sh up --sim isaac --robots 1 --config-only --dry-run

# 4) After a CI run, the report job must install PyYAML (this is what develop lacked)
pip install -r tests/report-requirements.txt
python tests/parse_metrics.py --current tests/results/<timestamp>/ --output report.md
# exits 0 on advisory numeric deltas; exits 2 only if report generation/integrity fails

On the PR comment (not local bash):

/pytest -m liveliness --sim isaacsim --num-robots 1 --stress-iterations 1

Expected benefit vs develop: a build_packages-only run no longer dies with No module named 'yaml' and then prints Metric regression detected. Maintainer looks at report.md / the PR Test Metrics comment, not a fake regression banner.

Testing with PyTest

  • What pytests did you add to ensure the feature is reliable and robust? What metrics are used?

Hermetic unit/contract tests (no GPU, no stack bring-up). They assert classification, fingerprinting, baseline selection, advisory exits, bounded diagnostics, and workflow YAML policy. No new flight metrics; existing Hz/compute/timing metrics stay recorded but are advisory.

New / extended files:

  • tests/meta/test_campaign_reporting_contract.py — assertion vs infrastructure, fingerprint includes robot count, baseline ignores mismatches, timeout ≠ regression, parse_metrics exits 0 on deltas / 2 on parser crash (test_metric_delta_cli_is_advisory, test_report_parser_crash_remains_blocking)
  • tests/meta/test_diagnostics_contract.py — secret-free bounded bundle; dead sim process abort; OptiTrack missing SDK is infrastructure
  • tests/meta/test_workflow_contract.py — no SHA fallback, cancel-in-progress on /pytest, report-requirements.txt instead of pip install tabulate, no Metric regression detected, fingerprint baseline download, minimal sweep inputs
  • Updates to test_metrics_reporting_contract.py, test_collection_contract.py, test_launch_intent_contract.py

The two tests that specifically gate run 32786410874: test_report_job_installs_declared_dependencies and test_metric_deltas_are_advisory_but_parser_errors_block.

  • What's the exact command to run the pytests that test your feature? i.e. airstack test -m ...
AIRSTACK_ROOT="$(pwd)" pytest tests/ -m unit -v

Or only the contracts for this bug:

AIRSTACK_ROOT="$(pwd)" pytest tests/meta/test_workflow_contract.py tests/meta/test_campaign_reporting_contract.py -v

Equivalent via the CLI wrapper:

airstack test -m unit -v
  • What are the expected results of the tests? What should a maintainer look at to understand whether the test succeeded?

All selected tests PASSED.

For the #384 bug specifically, a maintainer should confirm:

  • test_report_job_installs_declared_dependencies PASSES — workflow contains pip install -r tests/report-requirements.txt and does not contain a bare pip install tabulate; the requirements file lists PyYAML and tabulate.
  • test_metric_deltas_are_advisory_but_parser_errors_block PASSES — step name is Fail on report integrity error; string Metric regression detected is absent.
  • test_metric_delta_cli_is_advisory PASSES — parse_metrics.main() exits 0 when the report marks a numeric change.
  • test_report_parser_crash_remains_blocking PASSES — a parser crash exits 2 and writes “Report generation failed”, not a fake regression banner.

After merge, a run like 32786410874 should either produce report.md or fail as report integrity with a real traceback — never “Metric regression detected” on a missing Python package, and never a claim that build_packages made the algorithm worse.

Documentation

  • Was mkdocs.yml updated? (y/n)

n — nav already points at the pages we edited.

  • Do the docs have sufficient scope such that a newcomer can easily reproduce and use your feature?

Yes for this change:

  • tests/README.md — artifact layout, advisory comparison, dispatch inputs, report job behavior

  • docs/development/intermediate/testing/ci_cd.md — what fails the build, baseline fingerprinting, diagnostics, troubleshooting

  • .agents/skills/run-system-tests/SKILL.md — agent-facing interpretation of run_meta.json / diagnostics

  • Is there sufficient visual media?

No new diagrams beyond the existing CI flowchart (report job now finalizes the Check Run after posting). Screenshots of a metrics comment would help review.

Versioning

No. This does not change Docker image contents (no robot/sim/GCS Dockerfile or workspace package change). check-version-increment should not require a bump for harness/workflow/docs-only work.

Separate infrastructure failures, test assertions, and advisory metric changes so CI blocks only on real test or integrity failures while preserving comparable performance evidence.
Preserve explicit infrastructure intent through pytest call reports so simulator startup crashes cannot be misreported as algorithm assertions.
Expose trajectory and takeoff sweeps in workflow dispatch so CI validation can run minimal algorithm samples before expanding to expensive matrices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants