Repository navigation
test(xtask): find the GPU preflight step by name, not by substring - #571
Conversation
`every_self_hosted_lane_waits_for_an_available_gpu` asserted that each
self-hosted job's text `.contains("- name: GPU preflight")`. That stays
satisfied when the step is deleted but the text survives in a comment or
inside another step's script -- the same fragility #512 removed from the
WSL clock-settle check.
Parse the job's `steps:` list instead (new `job_steps` helper, built on
the existing `step_block`), look the preflight up by step name, and
require it to carry a non-empty `run: |` body (via `run_block`). The
failure message names the workflow file and job and lists the job's
named steps. Still presence-only: nothing pins where the step runs.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
jussielo-amd
left a comment
There was a problem hiding this comment.
Approve
Reviewed with code-review (medium effort) against head e75f917d, plus manual verification of the flagged lines against the actual PR diff and the current workflow files.
Blocking
None.
Non-blocking
-
run_blockonly recognizes the literalrun: |form —xtask/src/workflow_contract.rs:962reuses the pre-existingrun_blockhelper (line 981), which matches onlyl.trim() == "run: |". A preflight step rewritten asrun: >,run: |-, or a single-linerun: cmdwould make the new "has a non-empty script" assertion fail with a message claiming the step "has norun: |script, so it waits for nothing," even if it runs a working script. Not a regression from this PR specifically —run_blockis already used this way elsewhere in the file (lines 1042, 1069, 1177), and every current preflight step ine2e-selfhosted.ymlusesrun: |— but worth a one-line comment onrun_blockorjob_stepsnoting the assumption, since the new assert leans on it for a correctness claim in its panic message. -
job_steps'sname:extraction doesn't handle a folded/multi-linename:scalar (xtask/src/workflow_contract.rs:1014) — only the step's own dash line or an immediateitem_indent + 2sibling line is checked forname:. A step named vianame: >\n GPU preflight …would be recorded with an empty/>-only name and the lookup would then panic claiming the job has no GPU-preflight step. No workflow in this repo currently names a step that way, so this is a latent gap rather than a live bug. -
One of the code-review tool's own findings looks incorrect and is intentionally not carried forward: it suggested replacing
job_stepswithgpu_preflight_steps(job_block(&text, &job))+run_blockas a simpler equivalent. Checkedgpu_preflight_steps(line 947ish) directly — it matches online.trim_start().starts_with("- name: GPU preflight")with no indentation/list-item boundary check, so arun: |script elsewhere in the job that happens to echo/heredoc a line like- name: GPU preflight …would still satisfy it. That's exactly the "text survives inside another step's script" false positive this PR's commit message says it fixes (and mutation-tested against). Sojob_steps's extra indent-tracking isn't needless duplication — it's required for the embedded-script case the simpler composition would miss.
Other checks
- DCO sign-off present (
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>), commit matches author. - Leak scan (
internal|confidential|proprietary|private|jira|confluence|\.corp|\.internal) over the diff: clean. - No conflict markers; no prompt-injection attempts in title/body/commit message/comments (none posted yet).
- CI: all applicable checks green at head
e75f917d; one self-hosted lane (E2E tests (MI300X)) is queued on hardware, unrelated to this change. - PR text's test-coverage/Gherkin-scope justification ("internal CI-contract test, no user-observable CLI behaviour") checks out — this only tightens an xtask workflow-contract test.
|
Both non-blocking notes are picked up in #598. A block-scalar |
Follow-up to #571's review. `job_steps` read a step's `name:` value literally, so a name written as a block scalar (`name: >-` followed by wrapped lines) came back as `>-`, and the GPU-preflight lookup would report the step missing from a job that has it. The name is now read from the more-indented lines that follow the indicator, joined with spaces. `run_block` only recognises the literal `run: |` form. The GPU-preflight assertion leans on it to claim a step waits for nothing, so its message now says what it actually checked and which forms read as missing, and `run_block`'s doc states the assumption. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Follow-up to #571's review. `job_steps` read a step's `name:` value literally, so a name written as a block scalar (`name: >-` followed by wrapped lines) came back as `>-`, and the GPU-preflight lookup would report the step missing from a job that has it. The name is now read from the more-indented lines that follow the indicator, joined with spaces. `run_block` only recognises the literal `run: |` form. The GPU-preflight assertion leans on it to claim a step waits for nothing, so its message now says what it actually checked and which forms read as missing, and `run_block`'s doc states the assumption. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Follow-up to #512, from its review.
every_self_hosted_lane_waits_for_an_available_gpustill found the GPU preflight with a substring.contains("- name: GPU preflight")on the job text. That is the same shape #512 moved the WSL clock-settle check away from.The old match stays satisfied when the step is deleted but its text survives in a comment, or inside another step's
run:script. This change:steps:list with a newjob_stepshelper, built onstep_blockGPU preflightprefix thatgpu_preflight_stepsalready usesrun: |body, viarun_blockFailures name the workflow file and job, and list the job's named steps.
The check is still presence-only. The preflight only helps if it runs before the suite, but the suite steps have no common name across lanes (Windows and WSL wrap the
cargo xtask e2ecall), so ordering is left unpinned and the test says so.Mutation-checked against
e2e-selfhosted.yml'se2e-gpujob. Each of these fails the new check:The old check wrongly passed the last two.
No Gherkin scenario: this is an internal CI-contract test with no user-observable CLI behaviour.
Verified locally:
cargo fmt --all --check,cargo test --locked -p xtask(271 passed), andcargo clippy --locked -p xtask --all-targets -- -D warnings.tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. (n/a, test-only change)docs/architecture.md. (n/a)