Skip to content

fix(tests): preflight python3 + PyYAML in e2e-proxy-probe.bats (#827) - #830

Merged
aptracebloc merged 1 commit into
developfrom
fix/827-bats-yaml-preflight
Aug 25, 2026
Merged

fix(tests): preflight python3 + PyYAML in e2e-proxy-probe.bats (#827)#830
aptracebloc merged 1 commit into
developfrom
fix/827-bats-yaml-preflight

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Closes#827

Problem

e2e-proxy-probe.bats's manifest-landing test (the probe lands INSIDE the pod manifest's args…) parses the rendered pod YAML with python3 -c 'import yaml',
but its setup() never preflighted python3/PyYAML. A local run without PyYAML
died with an opaque ModuleNotFoundError: No module named 'yaml' from deep
inside the assertion instead of a clean skip naming the missing tool — unlike
chart-pull-secret.bats, which preflights its render tooling.

Fix

Adds a require_yaml_tooling guard, called at the top of the one test that
needs YAML tooling
rather than in setup():

  • The other five tests are curl-probe regression tests (the reason this file
    exists) that use only POSIX sh/awk/sed/grep. Guarding per-test keeps
    them running when only PyYAML is absent, instead of skipping the whole suite.
    This is the faithful analogue of chart-pull-secret.bats's require_tool:
    there every test renders a chart, so it guards in setup(); here only one
    test needs the tool.
  • CI=true (set by GitHub Actions) turns a missing tool into a hard failure
    with an ::error:: annotation, so a required gate can't go green on a skip —
    the inert-verification trap fix(chart): inject the release-scoped pull-secret name for training pods (backend#2119) #751 hit.
  • It also closes a gap the model file still has: chart-pull-secret.bats's
    require_tool only checks the python3 binary, not the PyYAML module,
    so its missing-PyYAML case is still opaque. The elif here checks the module.

Verification (bats 1.14)

ScenarioResult
Local, no PyYAMLYAML test skips cleanly (# skip python3 module 'yaml' (PyYAML) not installed (local run)); the 5 curl tests pass
Local, with PyYAMLall 6 tests pass
CI=true, no PyYAMLYAML test fails with ::error:: naming PyYAML (no green skip)

Also ran bats-hygiene.bats (18/18), the unenforced-assertions.awk scanner
(clean), and shellcheck (no new warnings — the only SC2034s are pre-existing
eval-interpolated locals).

Same-class findings (task step 2 — noted, not fixed here to keep this PR scoped to #827)

A grep of scripts/tests/*.bats for tool shell-outs without a preflight found
the identical defect in siblings, worth a small follow-up sweep:

  • image-refresh-skip-streak.bats (setup_file) — python3 -c 'import yaml', no guard
  • image-refresh-stale-pin.bats (setup) — python3 -c 'import yaml', no guard
  • chart-pull-secret.bats — guards the python3 binary but not the PyYAML module (narrower sub-gap)

Broader, different tools (larger scope, likely a separate hygiene PR): jq in
assess.bats / cluster.bats / telemetry.bats, and openssl in
preflight.bats / setup-linux.bats, are also shelled out without a preflight.

Happy to fold the three import yaml siblings into this PR or file a follow-up —
whichever the reviewer prefers.

🤖 Generated with Claude Code


Note

Low Risk
Test-only change to skip/fail behavior around optional local tooling; no production or cluster behavior is modified.

Overview
Adds require_yaml_tooling to e2e-proxy-probe.bats so the manifest-landing test checks for python3 and the PyYAML module before parsing rendered pod YAML, instead of failing with a deep ModuleNotFoundError.

The guard runs only at the start of that one test, not in setup(), so the five curl-probe regression tests still execute when PyYAML is missing locally. When CI=true, a missing tool fails the job with ::error:: annotations instead of a silent skip, avoiding green gates with inert verification.

Reviewed by Cursor Bugbot for commit df76217. Bugbot is set up for automated code reviews on this repo. Configure here.

The manifest-landing test parses the rendered pod YAML with
`python3 -c 'import yaml'` but never checked the tooling first, so a box without
PyYAML failed with an opaque `ModuleNotFoundError: No module named 'yaml'`
instead of a clean skip that names the missing tool.
Add a require_yaml_tooling guard called at the top of that one test. The five
curl-probe tests use only POSIX sh/awk/sed/grep, so guarding there rather than
in setup() keeps them running when only PyYAML is absent — the faithful analogue
of chart-pull-secret.bats's require_tool (there every test renders a chart, so it
guards in setup(); here only one test needs the tool). CI=true (set by GitHub
Actions) turns a missing tool into a hard failure so a required gate can't go
green on a skip.
Verified with bats 1.14:
- without PyYAML: the YAML test skips cleanly naming PyYAML, curl tests pass
- with PyYAML: all six tests pass
- CI=true without PyYAML: the test fails with an ::error:: annotation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Three things make this better than a copied preflight.

Guarding the one test rather than setup(), with the reason stated. The five curl-probe tests are why this file exists and they need only POSIX tools, so a setup() guard would skip them all because PyYAML is absent. Explaining that chart-pull-secret.bats guards in setup() because every test there renders a chart is what makes this the faithful analogue instead of a cargo-culted one — same intent, different placement, and the difference is argued.

CI=true turning a missing tool into a hard failure. In a required gate a silent skip is indistinguishable from a pass, and the ::error:: lines say what to do about it rather than just what happened — "install it in the job instead of accepting a green skip" is the sentence that stops someone treating the annotation as noise.

Naming the gap in the file it is modelled on.require_tool there covers the python3 binary but not the PyYAML module, so its missing-module case stays opaque; saying so, and closing it here rather than inheriting it, is the part I would not have expected. The elif ordering is also right — you cannot probe the module before the interpreter.

One non-blocking note, and it is the reflexive version of the trap you name: the CI=true arm is the load-bearing half of this guard and nothing exercises it. Confirming it fails rather than skips needs a run with CI=true and the tool absent, which is exactly the shape of "a check that cannot fail is not a gate". Not asking for it here — this is a 34-line test helper, and the cost of proving it likely exceeds the risk — but worth knowing it is asserted rather than demonstrated.

27 checks green, no open threads. The earlier PATH persist — debian:12 red cleared on this same head, so it was a flake rather than a real failure.

@aptracebloc
aptracebloc merged commit 2fa178c into developAug 25, 2026
62 of 63 checks passed
@aptracebloc
aptracebloc deleted the fix/827-bats-yaml-preflight branch August 25, 2026 10:45
Sign up for freeto 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.

e2e-proxy-probe.bats does not preflight python3, so a missing PyYAML gives an opaque error instead of a skip

3 participants

@aptracebloc@saadqbal@LukasWodka