test(podman): wire podman_preflight into CI and add a Podman e2e coverage check - #3749
politerealism wants to merge 3 commits into
Conversation
podman_preflight (added in NVIDIA#3690) never ran in CI: it needs only the standalone openshell-driver-podman binary, but the external-driver job that already builds and exports that artifact disables cargo test entirely for its own unrelated purpose. Chain a new e2e:podman:preflight mise task onto that job's existing command instead of routing through the gateway-backed PODMAN_CI_TESTS harness this test doesn't need. Add tasks/scripts/check-podman-e2e-coverage.sh (podman-e2e-coverage job in branch-checks.yml) so a new e2e-podman-eligible test target can't silently go unselected the way the 17 targets in NVIDIA#3712 did. It enumerates eligible targets via cargo metadata plus each auto-discovered file's own #![cfg(feature = ...)] gate, and requires every one to be accounted for in PODMAN_CI_TESTS, a perf-benchmark ignore list (verified via source inspection that every test in those files actually carries #[ignore], since cargo test -- --list does not distinguish ignored tests), a separately-wired list for tests like podman_preflight that don't fit the gateway harness, or a temporary, itemized known-gaps list tied to NVIDIA#3712 for the pre-existing gaps this check surfaces on introduction. The check also fails on stale known-gaps entries so that list can't rot in the other direction. Signed-off-by: politerealism <burdcat17@gmail.com>
No file described which e2e-podman-eligible test targets run where or how the exclusion mechanism works, making the gap in NVIDIA#3712 invisible from the docs as well as the code. Document PODMAN_CI_TESTS, the perf-ignore and known-gaps lists, and the new coverage-check script that enforces them. Signed-off-by: politerealism <burdcat17@gmail.com>
…kiness The chained "&&" command meant a failing or flaky e2e:podman:external-driver run would silently prevent e2e:podman:preflight from ever executing, recreating the exact invisible-coverage problem NVIDIA#3712 describes, just relocated to this job. Run both unconditionally and combine their exit codes instead. Signed-off-by: politerealism <burdcat17@gmail.com>
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Thanks for investigating the missing Podman coverage. I agree with the underlying problem, but I do not think we should merge this PR in its current form.
The proposed coverage check institutionalizes the legacy PODMAN_CI_TESTS allowlist just as #3597 and #3637 are replacing that model. #3597 moves portable behavior such as sync into driver-agnostic conformance, while #3637 builds a default-inclusive Podman nextest archive with an explicit follow-up list. Once those land, new eligible tests are included automatically, so the main invariant becomes exclusion hygiene—not reconciling several independent selection lists.
The new check also measures whether targets are “accounted for,” rather than whether they execute. Known gaps, benchmark exclusions, and SEPARATELY_WIRED_TARGETS all count as accounted for. In particular, podman_preflight remains accepted if its workflow invocation is later removed, so the same silent omission can recur while the coverage check remains green.
I also do not think podman_preflight belongs in the Podman E2E workflow. It does not require Podman or a gateway; it exercises the standalone driver’s missing-socket retry and error path. Those semantics should be covered by driver unit tests, with at most one executable integration test under crates/openshell-driver-podman/tests/. The normal workspace test lane can then run it without a special mise task, workflow command, or coverage exception.
There is also a correctness problem in the current workflow command: GitHub Actions runs run: scripts under bash -e, so if e2e:podman:external-driver fails, the shell exits before s1=$? and before e2e:podman:preflight runs. The final commit therefore does not actually guarantee that both tasks execute.
I suggest:
- Land #3597 and #3637 first.
- Move the preflight behavior into
openshell-driver-podmanunit/integration tests. - Use the tmachine archive’s default-inclusive selection as the source of truth.
- If an additional check is still needed, make it a small validator for stale or undocumented exclusions, ideally using the same declarative exclusion list consumed by archive construction.
- Report selected, excluded, and not-run targets separately instead of aggregating them as “accounted for.”
This preserves the useful goal of preventing silent coverage gaps without adding a new required CI job, a 228-line parser, and documentation for a selection model that is already being replaced.
This branch has not been deployed
2 participants
Summary
Track A of #3712: wires
podman_preflightinto CI (it never ran anywhere before this) and adds an automated check that fails CI if a newe2e-podman-eligible test target isn't accounted for anywhere, so the kind of silent gap this issue documents can't recur.Related Issue
Addresses part of #3712 (Track A only — see the issue comment recording the full plan). Does not duplicate open PR #3637, which independently covers most of the remaining 39-vs-20 gap via a different mechanism; that work is treated as a prerequisite for Track B, tracked separately.
Changes
podman_preflightwiring: it needs only the standaloneopenshell-driver-podmanbinary, not a gateway, so it never fit the gateway-backedPODMAN_CI_TESTSharness and simply never ran. Added a newe2e:podman:preflightmise task and chained it onto thepodman-external-driver-e2ejob, which already builds and exports that exact binary artifact for other reasons. Both commands now run unconditionally with combined exit-code checking, so a flaky/failinge2e:podman:external-driverrun can't silently maskpodman_preflightfrom ever executing — the same invisible-coverage problem this issue describes, just relocated, if chained with&&instead.tasks/scripts/check-podman-e2e-coverage.sh, newpodman-e2e-coveragejob inbranch-checks.yml): enumerates everye2e-podman-eligible test target viacargo metadataplus each auto-discovered file's own#![cfg(feature = ...)]gate, and requires every one to be accounted for inPODMAN_CI_TESTS, a perf-benchmark ignore list (verified via source inspection that every test in those files actually carries#[ignore], sincecargo test -- --listdoesn't distinguish ignored tests in its output), a separately-wired list (for tests likepodman_preflightthat don't fit the gateway harness), or a temporary, itemized known-gaps list tied to test(podman): run existing Podman behavioral e2e coverage in CI #3712 for the pre-existing gaps this check surfaces on introduction. It also fails on stale known-gaps entries, so that list can't rot in the other direction once Track B resolves them.CI.md.Testing
mise run pre-commitpassespodman_preflightbuilt and run for real locally:cargo build -p openshell-driver-podman && OPENSHELL_EXTERNAL_DRIVER_BIN=$PWD/target/debug/openshell-driver-podman mise run e2e:podman:preflight— both assertions pass.check-podman-e2e-coverage.shverified against both the happy path (39/39 accounted for, matching the issue's own count exactly) and three deliberately-broken cases: a real gap (removing an entry from the known-gaps list), a stale known-gaps entry (adding an already-accounted-for target), and a non-#[ignore]d perf-benchmark test — all three correctly fail with the expected error message and a real non-zero exit code.with-podman-gateway.shstops the driver process but never deletes the binary file, so the two chained tasks don't conflict over the artifact; confirmedOPENSHELL_EXTERNAL_DRIVER_BINis exported viaGITHUB_ENVin an earlier step and persists across both; confirmed the exit-code-combining shell logic behaves correctly for all three pass/fail combinations locally.with-podman-gateway.sh's bounded 30s/120s wait loops, and the job's 30-minutetimeout-minutes) is properly bounded — nothing can hang or retry indefinitely.Checklist
CI.mdinstead🤖 Generated with Claude Code