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 #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>
…ed (ROCMAI-679) (ROCm#598) * test(xtask): read wrapped step names, and say which run form is checked Follow-up to ROCm#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> * test(xtask): find GPU preflight steps through the step parser `gpu_preflight_steps` matched the literal line `- name: GPU preflight`, so a preflight whose name is wrapped into a block scalar (`name: >-`) dropped out of every check that reads from it: the APU GTT-pool check, the twin-drift check and the shell/PowerShell script extraction. The presence check already looks steps up through `job_steps`/`step_name`, which read wrapped names. Walk each job's `steps:` items via the same parser instead, jobs in file order, and keep returning the whole step block. The job-id scan moves out of `self_hosted_e2e_jobs` into a shared `job_ids` so both use one definition of what a job is. On e2e-selfhosted.yml (4 steps) and nightly.yml (6 steps) the returned blocks are byte-identical before and after. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): pin what the GPU preflight lookup leaves out The step-parser tests only fed `gpu_preflight_steps` preflight steps under one-line or wrapped names, so dropping its name filter, its handling of a job with no `steps:`, or the blank-line handling in `step_name` left every test green. The fixture now carries a step with another name, a commented-out preflight, a `- name: GPU preflight` line inside a script, a comment at the `steps:` indent, a `uses:`-only job, and a wrapped name with a blank line in it; the assertion is on exactly the two preflight scripts that should come back. The step-name test gains a wrapped name with blank lines among and after its lines. The lookup's doc no longer claims every step lookup here goes through the step parser (the clock-settling check matches whole lines on purpose), and the test comment now says the old raw-line match would have failed the checks that read it with a misleading count, not silently. `step_name`'s doc says its single-space join only approximates how YAML reads a block scalar. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): read a jobs block past a column-0 comment, and count jobs once top_level_block ended at any column-0 line, so a section-divider comment between jobs dropped every later lane out of the workflow checks. They then failed blaming a missing lane rather than the divider. A column-0 comment no longer ends the block, and comment lines just above the next key stay with that key. every_ci_job_declares_a_timeout now uses job_ids instead of its own copy of the job-id scan. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): describe top_level_block as it now behaves, and pin the trim The doc still said the block stops at a column-zero comment, which the previous commit reversed. It now says comments inside the block don't end it and that comment and blank lines just above the next key are left out, and the column-zero test pins that trim. The preflight-script doc and the clock-settling check now name the one run: form they read, as the presence check already did. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): read a wrapped name for a pwsh step too The step-name reader is now shared by the job-step and pwsh-step lookups, so the pwsh contract also reads a name wrapped into a block scalar. Its allow-list is keyed on the step name; read literally, every such step would be named `>-` and no entry could match it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- 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)