Repository navigation
ci: run each Windows acceptance self-test in its own step - #633
Conversation
pwsh carries on past a failing native command, and the step's result is the last command's exit code, so the three self-tests in one run: block could hide a failure of the first two. Give each its own step, as the Build and Clippy steps already are. The Linux job runs its block under bash -e, which already stops at the first failure. Follows up the review on #488. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Reviewed the diff against origin/main (merge-base, three-dot): this touches only .github/workflows/ci.yml, splitting the Windows job's "Acceptance harness self-tests" step (ci.yml:671-684 in the new file) into three separate steps, one per self-test script, each keeping the same if: needs.changes.outputs.heavy == 'true' gate and the shell: pwsh that every other step in this job sets explicitly (no job-level default). No prior reviews or comments existed on this PR, so this is a first pass.
The stated problem is real and the fix is correct. pwsh's default wrapper only checks $LASTEXITCODE once, at the end of a multi-line run: block, so an earlier failing python invocation inside the old combined step (still present unchanged in the Linux job at ci.yml:414, which is fine there because the default bash shell runs with -e and fails fast) would have been masked by a later passing one. Splitting into three steps not only reports the right step name on failure but also makes the job fail-fast at the first failing self-test rather than silently finishing green, which is a strict improvement over the previous behavior, not just cosmetic.
I confirmed the new steps are exercised by CI: windows-build-and-test passed on this PR's head commit, covering all three self-test steps. The one failing check on this PR (Coverage, rocm-dash crates) is an unrelated pre-existing flake, a "Text file busy" race in engines/vllm/src/install.rs's test harness when launching a temp python binary, nothing this diff touches.
This is a CI-only change with no user-observable behavior, so the "no Gherkin scenario" note in the PR description is appropriate and the regression-test bar doesn't apply here. No correctness, convention, or architecture issues found. Approving.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this one closely since it's a CI-only change. The step split is minimal and correct: all three new steps carry over the exact same if:, shell:, and script args from the original combined step, in the same order, with nothing dropped. It follows the same pattern already established by #488 for the identical masking problem, so this is consistent with how the file already handles pwsh's last-command-exit-code behavior.
Two small things worth a look, neither blocking:
- There's no workflow-contract test guarding "one command per pwsh step" going forward, so a future re-merge of these steps wouldn't get caught automatically. Might be worth a follow-up ticket.
- The Linux job's equivalent step is left unsplit. That looks intentional (bash's default
-e -o pipefailalready fails fast, so the masking bug doesn't apply there), but flagging in case it wasn't a deliberate call.
Also noticed the PR currently shows blocked on a failing required check, but that's the install_vllm_rocm10_discover_realigns_the_trio_even_if_pip_freeze_fails flaky test (a filesystem race) — unrelated to this diff, which only touches ci.yml. Should clear on a re-run.
Approving.
|
On @juhovainio's two notes:
|
|
The follow-up for the "no contract test" note is #658. It checks every pwsh step rather than counting lines: a native command must be the last statement, or have It also surfaced the nightly and release packaging steps, where an unchecked |
GitHub runs a `shell: pwsh` body under `$ErrorActionPreference = 'stop'`, which stops on a failing cmdlet but not on a failing native command; the step then reports only the last command's exit code. #633 split the Windows acceptance self-tests into one step each for that reason, but nothing stopped them being merged back. The new workflow-contract test walks every `shell: pwsh` step in every workflow and fails on a native command that is neither the body's last statement nor followed by a `$LASTEXITCODE` check. Three existing steps are allow-listed with the reason their failures still surface (the sccache probe decides for itself and ends in `exit 0`; the nightly and release packaging steps act on `cargo xtask package`'s outputs, which throw when missing), and a stale allow-list entry fails too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…m#658) * test(workflows): fail a pwsh step that can mask a native failure GitHub runs a `shell: pwsh` body under `$ErrorActionPreference = 'stop'`, which stops on a failing cmdlet but not on a failing native command; the step then reports only the last command's exit code. ROCm#633 split the Windows acceptance self-tests into one step each for that reason, but nothing stopped them being merged back. The new workflow-contract test walks every `shell: pwsh` step in every workflow and fails on a native command that is neither the body's last statement nor followed by a `$LASTEXITCODE` check. Three existing steps are allow-listed with the reason their failures still surface (the sccache probe decides for itself and ends in `exit 0`; the nightly and release packaging steps act on `cargo xtask package`'s outputs, which throw when missing), and a stale allow-list entry fails too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * refactor(xtask): share one step_name helper between job_steps and pwsh_steps Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): split pwsh statements at top-level `;` and past a here-string close The pwsh contract treated each line as one statement, so a native command chained before another with `;` was never checked, and a here-string's closing line was dropped whole, hiding anything after the `'@`. Split each logical line at `;`s outside quotes, `(...)`/`{...}` and comments; join backtick continuations; consume only the closing marker of a here-string and analyse the rest of its line; and check one-line `run:` values too, since they can chain with `;` as well. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
3 participants
Follows up #488.
The review on #488 pointed out that the Windows job's "Acceptance harness self-tests" step still runs three
python --self-testcommands in onepwshrun:block. That is the failure-masking pattern #488 fixed for the Build and Clippy steps.pwshcarries on past a failing native command, and GitHub reports the step's result from the last command's$LASTEXITCODE. So a failing ComfyUI or local-assistant self-test is hidden when the vLLM one passes. I reproduced this locally with the same wrapper GitHub uses ($ErrorActionPreference = 'stop', thenexit $LASTEXITCODE):python -c "exit(1)"followed bypython -c "exit(0)"exits 0.This gives each self-test its own step, with a comment saying why. The Linux job's copy of the step is unchanged: it runs under
bash -e, which already stops at the first failing command.Verification
Acceptance harness self-test (ComfyUI),(local assistant)and(vLLM).cargo test -p xtaskpasses (247), including the workflow-contract tests.check yaml, whitespace and line-ending hooks pass.No Gherkin scenario: this is a CI-only change with no user-observable behaviour.