Skip to content

No required check runs the Python test suite: five of six gate the retired Rust crate #392

Description

@hkngln

Scope

Deep review of dev at ac79d4c7ffe00e43f26f3b7c6265436afbff5b0e.

Problem

Branch protection on both main and dev requires the identical six contexts, with strict: true and enforce_admins: true. Measured this session:

$ gh api repos/alplabai/tan-cli/branches/dev/protection --jq .required_status_checks.contexts
["lint","test (ubuntu-latest)","test (windows-latest)","test (macos-latest)","msrv","seam1 -- plan-shape parity"]

$ gh api repos/alplabai/tan-cli/branches/main/protection --jq .required_status_checks.contexts
["lint","test (ubuntu-latest)","test (windows-latest)","test (macos-latest)","msrv","seam1 -- plan-shape parity"]

$ gh api repos/alplabai/tan-cli/branches/dev/protection --jq '{strict: .required_status_checks.strict, enforce_admins: .enforce_admins.enabled}'
{"enforce_admins":true,"strict":true}

Five of the six are cargo jobs on the crate being retired (#269):

  • lint.github/workflows/ci.yml:40, running cargo fmt --all --check (:53) and cargo clippy --all-targets --locked -- -D warnings (:55)
  • test (ubuntu-latest) / test (windows-latest) / test (macos-latest).github/workflows/ci.yml:59, running cargo build --all-targets --locked (:72) and cargo test --locked (:74)
  • msrv.github/workflows/ci.yml:190, running cargo check --workspace --all-targets --locked (:209)

The sixth, seam1 -- plan-shape parity (.github/workflows/parity.yml:218-219), is the only required context that runs any Python. Its Python selection is exactly three invocations:

:361  python3 -m pytest tests/parity/test_seam1_field_diff.py -q            (repo-root tests/)
:370  python3 -m pytest tests/parity/test_seam1_tan_reconciliation.py -q    (repo-root tests/)
:561  python3 -m pytest tests/gates -v --deselect tests/gates/test_jlink_aen_device_freshness.py
:595  python3 -m pytest tests/gates/test_jlink_aen_device_freshness.py -q

The job that actually runs the port's test suite AND builds the oracle it compares against is python-tests in .github/workflows/parity.yml:1273-1274, whose context names are python -- pytest across python/ (ubuntu-latest) / (windows-latest) / (macos-latest). It does cargo build --locked --bin tan (:1321) and then python -m pytest -q --ignore=tests/gates (:1328) with ALP_SDK_ROOT bound. None of its three contexts is required. Neither is python (.github/workflows/ci.yml:87, python -m pytest tests -q at :164).

The size of the hole, collected on this tree:

$ python -m pytest tests -q --collect-only -p no:cacheprovider   # from python/
2716 tests collected in 0.61s

$ python -m pytest tests/gates -q --collect-only -p no:cacheprovider
347 tests collected in 0.15s

tests/gates — 347 tests — is the entire python/tests slice any required context executes. The remaining 2369 of 2716 collected tests (87%) have no vote on a merge, including tests/commands (1406), tests/core (519), tests/parity (346), tests/conformance (25) and tests/installers (15). By line count that is 41,539 of the 44,857 lines under python/tests.

Separately, ci.yml's python job has no cargo build step at all, so the 18 test functions decorated with an oracle-presence skipif — for example python/tests/parity/test_support_bundle_oracle_parity.py:65-68:

_ORACLE_REQUIRED = pytest.mark.skipif(
    RUST is None,
    reason="needs a built Rust tan; run `cargo build --bin tan` (or set TAN_RUST_BINARY)",
)

— skip in that job across all five files that define it (test_run_oracle_parity.py, test_support_bundle_oracle_parity.py, test_build_sdk_root_oracle_parity.py, tests/commands/test_diff_command.py, tests/commands/test_pinmux_command.py). A skip is green, so even the non-required python context reports success having never compared the two binaries.

Impact

The python -- pytest across python/ legs and python do run on every PR and did pass on the last merge (#352, merged into dev), so nothing is broken today. The defect is that nothing stops them from going red and the PR merging anyway: with all six required contexts green, GitHub permits the merge, and enforce_admins: true gives no extra protection because the failing job is not in the list to begin with.

Concretely, a regression in python/tan/envelope.py or any command's --format json output turns python/tests/commands red (1406 tests, none required) while lint, the three cargo test legs, msrv and seam1 -- plan-shape parity all stay green — the six checks that gate the merge compile a different language in a different directory and cannot see the change. The {command,ok,exitCode,project,data,issues} envelope is the contract alp-sdk-vscode consumes, so the merge that breaks the extension's parsing is exactly the merge no required check can block. The same holds for python/tests/commands/test_flash_command.py (142 tests), which covers the refusals standing between a user and a wrong-device flash.

The exposure grows, not shrinks: #269 deletes crates/, which removes the five cargo contexts' subject matter entirely and would leave seam1 -- plan-shape parity — 347 gate tests — as the sole gate on the whole repository.

Acceptance criteria

  • Add python -- pytest across python/ (ubuntu-latest), python -- pytest across python/ (windows-latest) and python -- pytest across python/ (macos-latest) to required_status_checks.contexts on both main and dev; verify with gh api repos/alplabai/tan-cli/branches/{dev,main}/protection --jq .required_status_checks.contexts.
  • Add the python context from .github/workflows/ci.yml:87 as well, or state in that job's comment why it is deliberately advisory.
  • Add a cargo build --locked --bin tan step to ci.yml's python job, so the 18 _ORACLE_REQUIRED tests execute there instead of skipping green — or make that job assert that none of them skipped, the way .github/workflows/parity.yml:561-582 already asserts PASSED by node id for the two planner-freshness tests.
  • Add a test under python/tests/gates/ — the only slice a required context runs today — that reads .github/workflows/*.yml, collects every job whose steps invoke pytest against python/tests, and fails when any of the resulting context names is absent from a committed list of required contexts. That test would have caught this, and it keeps catching it when a new suite is added under a new job name.
  • Once crates/ is removed (Retire the Rust oracle: delete crates/, and decide who enforces contract/ afterwards #269), demote lint, the three test (<os>) legs and msrv out of the required set in the same change that deletes the jobs, so protection is never left requiring contexts that no workflow can report.

Related: #269.

Metadata

Metadata

Assignees

No one assigned

    Labels

    ciCI workflows, gates and test wiring

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions