test(e2e): cover rocm diagnose and fix on the mock lane - #127
Conversation
diagnose and fix (a P0 command per the coverage plan) had no E2E scenarios. Add six black-box, GPU-independent scenarios: a recognised symptom yields a scored cause with a fix plan; diagnosing any failure always offers an escalation route; the machine-readable form identifies the match; fix lists its recipes with auto/print-only markers; a dry-run previews without mutating managed state; an unknown fix-id is refused with a non-zero exit. diagnose probes the real host, so assertions cover only host-invariant contracts (exit code, escalation route, a known symptom yielding a scored match with an id and plan) — never match counts, scores, or which fix-id wins, which vary with a CI host's own faults. Verified in the Linux container. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
5355b24 to
5e074fa
Compare
volen-silo
left a comment
There was a problem hiding this comment.
Reviewed against the actual diagnose.rs/fix.rs/CLI wiring and by running the built binary. Nice, well-documented coverage — 4 of the 6 scenarios are genuinely platform-independent and pass against the real binary. But scenarios 1 and 3 are not as host-invariant as described and will fail on the strix-windows lane, so requesting changes on that.
🔴 Scenarios 1 & 3 assume a catalog match that only exists on non-WSL Linux
KNOWN_SYMPTOM = "unable to open /dev/kfd" scores only via KEYWORDS_KFD_PERMISSION, which is consumed only by check_4_render_group — and check_4 is LINUX_ONLY (diagnose.rs:1225). So this "known symptom" produces a match only when the bare-metal Linux catalog runs.
- Scenario 1 (
assert_reports_cause_and_fix) needs"score="in the text output. - Scenario 3 (
assert_json_identifies_match) needs a non-emptymatchedarray.
Neither holds where that keyword's checker doesn't run:
| Lane | Result |
|---|---|
e2e mock (ubuntu-latest), e2e-gpu (MI300X), strix-ubuntu |
✅ pass (non-WSL Linux, check_4 fires) |
strix-windows |
❌ fail — no Windows checker keys off /dev/kfd; on a healthy box matched is empty |
On Windows the only checkers that could fire for this symptom are incidental host faults (e.g. fix-13-hip-sdk-missing if the SDK is absent) — exactly the non-determinism the PR set out to avoid. On a healthy Strix-Windows box, matched is empty → no score= rendered → both scenarios fail. Confirmed the mechanism by running the binary (WSL2, which is out_of_scope, gives the same empty result):
$ rocm diagnose --symptom "unable to open /dev/kfd" --json → matched: 0
$ rocm diagnose --symptom "unable to open /dev/kfd" → "rocm diagnose: out of scope..." (no "score=")
Both scenarios are untagged, so resolve() marks them expect-pass on every lane including strix-windows. That lane is continue-on-error, so it won't block merge, but they'll be deterministic failures there — which defeats the goal of "GPU-independent coverage that runs on every tier."
Two ways to fix:
- Preferred — keep Windows coverage: switch
KNOWN_SYMPTOMto a symptom that feeds aLINUX_AND_WINDOWSchecker, e.g."HSA_STATUS_ERROR_INVALID_ISA"(scores 50 inKEYWORDS_INVALID_ISA→check_1_arch_not_in_wheel, which applies on both OSes). With no framework installed it stays at 50 (the-30covered-arch branch only fires whenframework_arch_listis populated), somatchedis non-empty and a scored block renders on both Linux and Windows. - Conservative: tag scenarios 1 & 3 with
@requires-os:linux(the convention already used once in the suite). Honest — the/dev/kfdcheck is Linux-specific — but drops Windows coverage.
(Neither makes them pass on a WSL2 host, since WSL is out_of_scope regardless and reports os=linux — but no CI lane is WSL, so that's fine.)
🟡 Minor: scenario 5 doesn't exercise dry-run's mutation suppression
PREVIEW_FIX_ID = "fix-1-arch" is print-only (runner: None), so apply() returns at the !auto_applicable branch before reaching any runner — --dry-run is a no-op and the fix would never mutate anything even without it. assert_no_mutation is therefore trivially true. The step comment is upfront about why auto-applicable recipes were avoided (host-dependent dry-run codes), so this is a reasonable tradeoff; just worth a note that it's a print-only path, since the scenario name implies more. Not a blocker.
✅ Verified good (ran against the built binary)
- S4
rocm fix→ exit 0,Available fix-idsheader +AUTO/PRINT-ONLYmarkers. - S5
rocm fix fix-1-arch --dry-run→ exit 0, output hasFix:andfix-1-arch. - S6
rocm fix fix-does-not-exist→ exit 2,Unknown fix-idon stderr (step correctly reads stderr). - S2 escalation route URL always present regardless of match/scope.
diagnosealways exits 0 (main.rs:1830); flags (--symptom,--json,--dry-run) match the clap defs;@ids unique.
Scenarios 1 & 3 keyed off "unable to open /dev/kfd", which only scores via check_4_render_group (LINUX_ONLY) — so on strix-windows the match is empty and both fail deterministically. Switch to HSA_STATUS_ERROR_INVALID_ISA, which scores via check_1_arch_not_in_wheel (LINUX_AND_WINDOWS) and stays non-zero with no framework installed, so a scored match renders on both Linux and Windows. Co-authored-by: claude <my-agent@users.noreply.github.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
|
Thanks for the thorough review — you're right, S1 & S3 weren't as host-invariant as the docs claimed. 🔴 S1 & S3 — fixed (preferred option, keeps Windows coverage). Swapped 🟡 S5 — keeping the print-only recipe (by design). Agreed the name implies more than the assertion proves. But switching to an auto-applicable recipe reintroduces exactly the host-dependence this PR set out to eliminate: |
rominf
left a comment
There was a problem hiding this comment.
Automated review: Approve.
The blocking concern from the earlier review is resolved at the current head. KNOWN_SYMPTOM now uses HSA_STATUS_ERROR_INVALID_ISA, which scores through a checker registered on both Linux and Windows; even the covered-architecture penalty leaves a positive match. The six new diagnose scenarios also pass on the GPU lane.
Non-blocking:
- The dry-run scenario uses the print-only
fix-1-arch, whose path returns beforeopts.dry_runis read. It verifies command wiring and no mutation but not the mutation-suppression branch itself; that branch has unit coverage. tests/e2e-cucumber/README.mdhand-lists feature and step files but now omitsdiagnose.featureanddiagnose_steps.rs.
All blocking checks are green. The non-blocking GPU lane failures are in unrelated pre-existing scenarios, while the new diagnose scenarios pass.
Summary
rocm diagnose(a P0 command per the E2E coverage plan) and its siblingrocm fixhad zero E2E coverage. Add adiagnose.featurewith six black-box, GPU-independent scenarios that run on the fast mock lane / per-PR tier — no serve, no download, no mutation.Scenarios
--json) form identifies the matched cause.fixlists its recipes, each marked AUTO or PRINT-ONLY.fix --dry-runpreviews the change without mutating managed state.Notes
expectations.tomlentries needed).Test plan
cargo clippy -p e2e-cucumber --test e2e -- -D warningsclean; all 6 scenarios pass viacargo xtask e2emock selection (0 unexpected failures).