Skip to content

test(xtask): find the GPU preflight step by name, not by substring - #571

Merged
rominf merged 1 commit into
mainfrom
test/gpu-wait-step-structured-lookup
Oct 8, 2026
Merged

rominf merged 1 commit into
mainfrom
test/gpu-wait-step-structured-lookup

Conversation

@rominf

@rominf rominf commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #512, from its review. every_self_hosted_lane_waits_for_an_available_gpu still 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:

  • parses the job's steps: list with a new job_steps helper, built on step_block
  • looks the preflight up by step name, using the GPU preflight prefix that gpu_preflight_steps already uses
  • requires a non-empty run: | body, via run_block

Failures 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 e2e call), so ordering is left unpinned and the test says so.

Mutation-checked against e2e-selfhosted.yml's e2e-gpu job. Each of these fails the new check:

  • renaming the step
  • deleting it
  • leaving its old text only in a comment
  • leaving its old text only inside another step's script

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), and cargo clippy --locked -p xtask --all-targets -- -D warnings.

  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (n/a, test-only change)
  • If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (n/a)
  • Every new or changed user-facing message was read against the code path that runs after it. (n/a, no CLI message changes)

`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>
@rominf
rominf requested a review from a team as a code owner October 6, 2026 13:11
@rominf
rominf requested a review from r0x0r October 6, 2026 13:11

@jussielo-amd jussielo-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

  1. run_block only recognizes the literal run: | form — xtask/src/workflow_contract.rs:962 reuses the pre-existing run_block helper (line 981), which matches only l.trim() == "run: |". A preflight step rewritten as run: >, run: |-, or a single-line run: cmd would make the new "has a non-empty script" assertion fail with a message claiming the step "has no run: | script, so it waits for nothing," even if it runs a working script. Not a regression from this PR specifically — run_block is already used this way elsewhere in the file (lines 1042, 1069, 1177), and every current preflight step in e2e-selfhosted.yml uses run: | — but worth a one-line comment on run_block or job_steps noting the assumption, since the new assert leans on it for a correctness claim in its panic message.

  2. job_steps's name: extraction doesn't handle a folded/multi-line name: scalar (xtask/src/workflow_contract.rs:1014) — only the step's own dash line or an immediate item_indent + 2 sibling line is checked for name:. A step named via name: >\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.

  3. One of the code-review tool's own findings looks incorrect and is intentionally not carried forward: it suggested replacing job_steps with gpu_preflight_steps(job_block(&text, &job)) + run_block as a simpler equivalent. Checked gpu_preflight_steps (line 947ish) directly — it matches on line.trim_start().starts_with("- name: GPU preflight") with no indentation/list-item boundary check, so a run: | 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). So job_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.

@rominf
rominf added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit 2023d76 Oct 8, 2026
30 of 31 checks passed
@rominf
rominf deleted the test/gpu-wait-step-structured-lookup branch October 8, 2026 08:09
@rominf

rominf commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Both non-blocking notes are picked up in #598. A block-scalar name: is now read from its wrapped lines rather than as >-/|, and the GPU-preflight assertion's message says it only recognises the literal run: | form.

rominf added a commit that referenced this pull request Oct 15, 2026
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>
rominf added a commit that referenced this pull request Oct 21, 2026
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>
Sign up for free to 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.

2 participants