You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds two E2E scenarios that drive a real rocm binary under a real PTY to prove cli_progress::AnimatedSpinner actually renders and clears correctly outside of its existing in-process unit tests:
download-progress-01: TheRock tarball SDK install — download progress, extraction ("Extracting …") frame, and both spinner lines cleared on exit.
comfyui-04: ComfyUI source-archive install — download progress only (this path has no separate extraction phase) and the spinner line cleared on exit.
Adds a shared PacedDownloadServer fixture (tests/e2e-cucumber/src/paced_download.rs): a loopback HTTP server that streams one named file in fixed-size, delayed chunks (falling back to normal ServeDir for everything else), so the PTY has a real chance to observe an intermediate, sub-100% frame instead of the whole transfer completing within one screen poll.
Adds two e2e-test-hooks-gated hooks needed to make the ComfyUI scenario hermetic and fast: ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE (redirect the source-archive download to the fixture server) and ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS (skip the real system package-manager dependency check that otherwise runs both after a successful SDK install and during rocm engines install vllm). Both compile out entirely in non-e2e-test-hooks builds, so production behavior is unaffected.
Documents the new override in docs/release-trust.md, alongside the existing TheRock base-override documentation.
Fixes a real production bug in cli_progress::Spinner: tightening the new e2e assertion to require an actual intermediate (non-0%, non-100%) progress frame reproducibly failed, which traced back to render_current truncating the whole assembled status line from the tail. On an ordinary 80-column terminal, once the byte count grew past a couple of characters (e.g. "0 B" → "1.5 MiB"), the trailing "(NN%)" got silently ellipsized away — hiding the download percentage for the entire rest of the transfer, with only the very first (0%) frame ever visible. Fixed by splitting the label (the file name, already shown elsewhere) from the byte-count/percentage suffix, and truncating only the label when the line overflows, so the percentage always stays visible. Also hardens the new assemble_status_line helper against an even narrower-terminal edge case where the suffix itself doesn't fit.
Two rounds of PTY-harness hardening in response to review feedback: made the @serial-tagged scenarios' negative "spinner line cleared" assertions robust against both truncation (matching a prefix instead of the full, truncated label) and the emulated terminal's zero-scrollback screen (growing PTY rows so a later summary can't scroll a spinner row out of view before the check reads it).
Why: AnimatedSpinner only had in-process unit coverage before this — never run under a spawned subprocess — so a regression in its TTY detection, throttling, or line-clearing on Drop could ship unnoticed. This exercises both the tarball and ComfyUI download paths end to end against real fixture servers, and in doing so caught a genuine, independently-reproducible bug in the spinner's own rendering logic (described above).
Risk: low-to-medium. Most of the diff is test-only, but apps/rocm/src/cli_progress.rs's render_current/assemble_status_line change is a real behavioral fix shipped to production — it changes what gets truncated on narrow terminals during any download or long-running command that uses the spinner. Covered by unit tests (including two regression tests for both truncation edge cases) and the new e2e scenarios.
Known gaps
The dependency-install spinner added by the recent spinner work (apps/rocm/src/comfyui.rs's uv block) isn't exercised by this coverage — the ComfyUI fixture deliberately empties the dependency list so that code path never runs during these scenarios. Intentional scope limit, not an oversight; left for follow-up coverage.
download-progress-01's extraction-frame assertion is inherently timing-dependent: a 20ms poll against a real tar subprocess run. @serial (no concurrent CPU contention) and generous payload sizing mitigate this but don't eliminate it — it's the residual flake source to watch if this scenario ever becomes intermittent.
cargo xtask e2e -- -n "download-progress-01|comfyui-04" — both scenarios pass, run repeatedly, 11/11 steps each time
Full local suite (cargo test -p e2e-cucumber --test e2e, no filter — exercises cucumber's default 64-concurrent-scenario execution) run repeatedly with 0 unexpected failures, to confirm no contention-induced flake
prek run --all-files --no-group local-tools — clean
Copilot review comment on #421 pointed out that both PTY scenarios'
'intermediate download progress frame' assertions accepted the
unthrottled (0%) frame emitted before any bytes are read, so they
passed even if no real in-transfer frame ever rendered. Tightening
both to require a percentage strictly between 0% and 100%
(wait_for_screen_where over a percent that is neither (0%) nor
(100%)) turned up a real product bug: on an ordinary 80-column
terminal, Spinner::render_current truncated the whole assembled
'{frame} {label} {bytes}/{total} (pct%)' line from the tail once the
byte count grew past a couple of characters (e.g. '0 B' -> '1.5
MiB'), silently dropping the '(pct%)' suffix for the rest of the
transfer.
Fix: keep the byte-count/percentage suffix in its own field
(progress_suffix) and truncate the file name label instead when the
line doesn't fit -- the label was already printed in full earlier in
the command's output, so it's the safer thing to sacrifice, and the
percentage is the part a user actually watches move.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
screen_text() contains only the current 24-row VT screen. After extraction, this command prints roughly 21 summary lines, and its long URL/path lines wrap at the fixed 80-column width, so the spinner row is guaranteed to scroll out even if Spinner::clear is broken. These negative checks therefore cannot verify clearing. Capture the PTY transcript/erase sequence, or synchronize an assertion before the subsequent summary can scroll the row away.
Addressing Copilot's "Needs a closer look" finding from this review ("Spinner-clearing checks run after the row has scrolled off-screen"): confirmed real. The PTY's vt100::Parser has 0 lines of scrollback, and the tarball install's ~20-line summary output was enough to scroll the download/extraction spinner's row off the top of the default 24-row screen before assert_spinner_lines_cleared ever read it — a scrolled-off row and a genuinely cleared one are indistinguishable to screen_text(), so the check could pass even if Spinner::clear were broken.
Added TuiSession::grow_rows, which grows only the row count (not columns, unlike the existing use_detail_size) — widening columns too would stop the label from truncating, undermining the 80-column truncation behavior this scenario exists to exercise.
Called it right after spawning the tarball install session.
Verified two ways: dumped the screen at the assertion point and confirmed the whole session's output (from its first line onward) now fits with rows to spare; and temporarily neutered Spinner::clear() to confirm the assertion now genuinely fails when clearing is broken (it didn't catch this before the fix).
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Adds two PTY-driven Gherkin scenarios that observe the download/extraction progress spinner through a real pseudo-terminal, a paced chunked-HTTP fixture server to make intermediate frames observable, and — not purely test work — two production fixes to apps/rocm/src/cli_progress.rs (keep the byte/percentage suffix intact when the label overflows; never exceed max_width when the suffix alone overflows) plus two e2e-test-hooks-gated override accessors. No blocking findings. Verified: mutated each of the two new production branches separately in a scratch copy and confirmed each new unit test reddens on its own assertion (whole-line-truncation revert → the suffix-intact test fails; removing only the narrow-terminal guard → the max-width test fails, while the other still passes); the PTY scenarios themselves were not executed here and the full suite was not run. Check state at review: 19 success, 2 skipped, 0 failure. Blocking: 0 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
None.
Notes on the specific risks checked, since this PR's value rests entirely on whether the new assertions discriminate:
Vacuous passes. Every positive PTY assertion is a contains on rendered text, so an empty screen fails it. wait_for_screen_where/wait_for_screen return Err on timeout, on reader-thread panic, and on the child exiting early, so a missing binary or a process that dies does not pass. wait_for_exit asserts exit code 0, not merely that the process exited. TuiSession::spawn surfaces an OS spawn failure as an error.
Negative assertions vs. scrolling. The vt100 parser is built with zero scrollback, so a scrolled-off row is indistinguishable from a cleared one. therock_steps.rs handles this explicitly by growing the PTY to 60 rows before the summary prints. For the ComfyUI scenario the arithmetic holds independently: only lines printed after the spinner row can push it off a 24-row screen, and that path prints ~10, so the row cannot scroll away — see non-blocking #1 for the fragility this leaves behind.
Negative assertion vs. truncation.assert_spinner_lines_cleared correctly checks a 30-character prefix of the download label rather than the full string, because the live line is truncated on an 80-column terminal; the full string would have matched nothing either way. The extraction label carries no suffix, is 59 columns wide, and so is checked in full — correct. The permanent tarball: <file> line printed at apps/rocm/src/therock.rs:2611 does not contain the Downloading … prefix, so it cannot defeat the check.
Readiness waits. No wait in this diff is satisfied before the action it gates: each is a positive wait for output that only the action produces, and each fails loudly if it never appears.
@serial is real, not decorative. I initially concluded the tag was inert because the repo's own tag parser (tests/e2e-cucumber/src/expectation.rs) does not recognise it. That was reviewer error: @serial is consumed by cucumber-rs's default which_scenario, which maps it to ScenarioType::Serial; the repo sets only max_concurrent_scenarios and never overrides which_scenario, and the repo parser silently ignores unknown tags. The tag works as the feature-file comments claim. See non-blocking #4 for the one-line fix that stops this recurring.
Coverage vs. the previous version (focus g). The rename of format_download_progress to the private format_progress_suffix carries every prior assertion forward. One old assertion — that the rendered string starts with the caller's prefix — is dropped, and that vacation is correct, not a regression: the function no longer takes a prefix, so the assertion no longer has a subject; the prefix/suffix composition is now pinned structurally by assemble_status_line and by two new tests. The function had no other callers at the base, so nothing lost coverage elsewhere.
Sign-off and content. All 9 commits in the range carry exactly one Signed-off-by: trailer each (checked line-by-line from the raw commit objects). No internal names, hostnames, gateways, cluster names, registry paths or internal document links in the diff. No prompt-injection content found anywhere in the checkout.
Non-blocking
tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:540 — the "no ComfyUI download spinner line" check is safe today only because ~10 lines print after the spinner; the sibling tarball scenario protects the identical assertion explicitly with grow_rows(60). Any future growth of the ComfyUI completion report would silently turn this into a tautology with no test failure to warn you. Call grow_rows here too, or state the invariant in a comment.
apps/rocm/src/cli_progress.rs:65 — set_label now resets progress_suffix to None, and nothing tests it; a regression there would leave a stale byte count glued to an unrelated message. One unit test (set_progress, then set_label, then assert the suffix is gone) closes it.
tests/e2e-cucumber/tests/e2e/therock_steps.rs — &format!("Downloading {CURRENT_TARBALL}")[..30] is a raw byte slice with a magic length; it panics if the fixture file name is ever shortened or gains a non-ASCII character. A named constant with a one-line note on why 30, or a char-boundary-safe slice, removes the trap.
tests/e2e-cucumber/src/expectation.rs — every scenario tag the repo owns is documented on ScenarioDecl, and @serial is the only tag in the feature files that is not, because cucumber-rs owns it. One line there saying so would have prevented the wrong conclusion I reached above, and will otherwise invite the same conclusion from the next reader.
The dependency-install spinner added by the recent spinner work (apps/rocm/src/comfyui.rs:1712) is the one spinner this coverage does not reach — the ComfyUI fixture deliberately empties the dependency list so the uv block never runs. Worth naming as a known gap in the PR text rather than leaving it implicit. Separately, the PR title reads as test-only, but the diff changes production rendering in cli_progress.rs and adds a gated early-return in main.rs; the description should say so, since a maintainer skimming "e2e: … coverage" would not expect behaviour changes. The extraction-frame assertion also remains inherently timing-dependent (a 20 ms poll against a real tar run) — @serial plus the payload sizing mitigate it, but it is the residual flake source to watch.
comfyui_steps.rs — grew the ComfyUI PTY to 60 rows before install, mirroring the tarball scenario's fix, so a future-longer completion report can't turn the "cleared" check into a tautology the way it could have before.
cli_progress.rs — added a unit test proving set_label clears a stale progress_suffix.
therock_steps.rs — replaced the magic [..30] byte slice with a named, documented DOWNLOADING_PREFIX_LEN constant and a panic-safe .get(..).unwrap_or(...) lookup.
expectation.rs — documented that @serial is intentionally outside ScenarioDecl/from_tags (it's a cucumber-rs runner concern, not an expectation-resolution one).
PR title now names the production fix rather than reading as test-only; description gained a "Known gaps" section calling out the untested comfyui.rs dependency-install spinner path and the residual timing-dependence of the extraction-frame assertion.
All verified: full workspace build/clippy/test, targeted + full-suite (repeated, contention-inclusive) e2e runs, and prek, all clean.
Copilot review comment on #421 pointed out that both PTY scenarios'
'intermediate download progress frame' assertions accepted the
unthrottled (0%) frame emitted before any bytes are read, so they
passed even if no real in-transfer frame ever rendered. Tightening
both to require a percentage strictly between 0% and 100%
(wait_for_screen_where over a percent that is neither (0%) nor
(100%)) turned up a real product bug: on an ordinary 80-column
terminal, Spinner::render_current truncated the whole assembled
'{frame} {label} {bytes}/{total} (pct%)' line from the tail once the
byte count grew past a couple of characters (e.g. '0 B' -> '1.5
MiB'), silently dropping the '(pct%)' suffix for the rest of the
transfer.
Fix: keep the byte-count/percentage suffix in its own field
(progress_suffix) and truncate the file name label instead when the
line doesn't fit -- the label was already printed in full earlier in
the command's output, so it's the safer thing to sacrifice, and the
percentage is the part a user actually watches move.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Adds two PTY-driven Gherkin scenarios that observe the download/extraction progress spinner through a real pseudo-terminal, a paced chunked-HTTP fixture server that makes intermediate frames observable, and production changes to apps/rocm/src/cli_progress.rs (split the byte/percentage suffix out of the label so truncation can never eat it, plus a narrow-terminal clamp) alongside two e2e-test-hooks-gated override seams. Since the last round it also lands the five remediation items and one further production fix (a doubled space in the narrow-terminal fallback); no blocking findings. Verified: ran the cli_progress unit module in a scratch copy (16 passed) and then mutated each production branch separately there — reverting set_label's suffix reset reddens only set_label_clears_a_stale_progress_suffix; reintroducing the doubled space reddens only assemble_status_line_fallback_does_not_double_the_space_before_suffix; disabling the narrow-terminal guard reddens the two width tests; reverting to whole-line truncation reddens only keeps_the_progress_suffix_intact… — so each branch is pinned by its own assertion, not just by a wholesale revert. I also confirmed from source that "Downloading therock-dist-linux" (the exact 30-byte prefix) and "Extracting {tarball}" and "Fetching ComfyUI source archive" each occur exactly once in the product, as spinner labels only, so none of the negative checks is a tautology; that wait_for_exit asserts exit code 0 and wait_for_screen_where returns Err on timeout, child exit and reader panic, so nothing passes on an empty screen; that the only runtime source of "%)" reachable from these two commands is the spinner suffix itself; that the vt100 parser has zero scrollback while grow_rows(60) comfortably covers the ~20 lines printed after the spinner; that @serial is honoured by cucumber-rs's default which_scenario and @requires-os:linux resolves to Skip; that e2e-test-hooks is off by default with no --all-features anywhere in CI, so both new override seams are compiled out of release builds; and, measuring it rather than trusting the comment, that high-entropy filler really is gzip-incompressible (20 MB, ratio 1.0003), which is what keeps the pacing real. All 16 commits carry exactly one sign-off; leak and injection scans over the diff are clean. The merge base (93677c8) is behind the prw-base tip (3ecf484) by four commits, but the resulting diffs are byte-identical, so scope is unaffected. Not run here: the e2e/PTY scenarios themselves and the full workspace suite. I could not check the PR title and description text, since this review never contacts GitHub — the author's claim that item 5 was addressed there is unverified by me. Checks at this head: 18 success, 2 skipped. Blocking: 0 · Non-blocking: 5. One thing to flag about timing: this review was carried out against the commit named in the heading above, and the branch was force-updated onto a newer base while it was running. The branch's own commits look unchanged by that update, but everything above was assessed before it, so it speaks for the commit it names rather than for whatever is at the tip now. Checks at the new tip stood at 18 success, 2 skipped and 1 still running when this was posted.
🚫 Blocking (must fix before merge)
None.
Two things are worth stating plainly, because this PR's whole value rests on whether the new assertions discriminate.
The remediation round was weighed as original code, not as review answers. Item 2 (set_label clearing a stale suffix) and the unprompted item in 50fa83d (dropping the doubled space) are both real production edits, and both were mutation-tested above: each fails exactly one test and leaves the others green. Item 3 replaced the raw byte slice [..30] with DOWNLOADING_PREFIX_LEN plus .get(..).unwrap_or(&downloading), which is genuinely panic-safe — a shortened or non-ASCII fixture name now degrades to checking the full string rather than aborting. Item 7's shim dedup (write_gpu_probe_shim) is byte-identical to both previous inline copies. Item 6's rand swap replaces a hand-rolled PRNG with a seeded StdRng and honestly documents in the same comment that rand does not promise algorithm stability across versions, so a future bump could change the fixture bytes — that caveat is accurate, not hedging.
Where the Drop-time clear is actually proven.Spinner::clear on Drop is covered, but not by the half of the assertion a reader would assume — see non-blocking #1. The ComfyUI scenario is what carries it for a download spinner, and the extraction half of the tarball assertion carries it there; both were traced through the real output sequence rather than assumed.
Non-blocking
tests/e2e-cucumber/tests/e2e/therock_steps.rs:1300 — the "Downloading therock-dist-linux" half of assert_spinner_lines_cleared cannot fail for a broken Drop-time clear: the extraction spinner starts on the same row and its first repaint issues Clear(CurrentLine), erasing the download line whether or not Spinner::clear ran. The extraction half and the ComfyUI scenario do cover that defect, so nothing is untested — but the step's own comment reads as if both halves prove it. One sentence saying the download half guards in-place repaint (not Drop-clear) would stop the next reader drawing the same wrong conclusion I nearly did.
apps/rocm/src/cli_progress.rs:208 — assemble_status_line takes suffix: Option<&str> but its narrow-terminal branch silently depends on the caller's suffix carrying its own leading space; a future second caller passing a space-less suffix would glue frame and suffix together with no test to catch it. The invariant is pinned today only because format_progress_suffix is the sole producer.
apps/rocm/src/cli_progress.rs:123 — the Err(_) arm of render_current still emits an unbounded, untruncated line when terminal::size() fails. This is preserved base behaviour, not a regression, and it is the one rendering path with no coverage at any level.
tests/e2e-cucumber/tests/e2e/therock_steps.rs:1229 and comfyui_steps.rs:1042 — grow_rows(60) is issued immediately after TuiSession::spawn, with no synchronisation point, so there is a theoretical window where the child could emit more than 24 lines before the resize lands. Not reachable at real process-startup timings, but the assumption is unstated.
The extraction-frame assertion remains inherently timing-dependent (a 20 ms poll against a real tar over a ~20 MB archive); @serial plus the payload sizing give it roughly a 90-poll margin on the download side, but it stays the residual flake source to watch. Separately, apps/rocm/src/comfyui.rs:1712's dependency-install spinner is still the one spinner this coverage deliberately does not reach.
Addressed the non-blocking items from the automated review at commit 20dbbdbd:
Let Lemonade auto-select its llama.cpp backend #1 (download-half of assert_spinner_lines_cleared doesn't itself prove Drop-time clear) — fixed: reworded the comment to say explicitly that the extraction assertion (and the ComfyUI scenario) are what actually cover that, not this check.
Enable native-certs for ureq across all crates #2 (assemble_status_line's suffix-leading-space invariant unenforced) — fixed: added a debug_assert! so a future space-less caller fails loudly in tests instead of silently gluing frame/suffix together.
Migrate Python environment management from pip to uv #3 (render_current's Err(_) fallback emits an unbounded, untruncated line) — fixed: it now shares assemble_status_line with an 80-column fallback width, same as every other path.
Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex #4 (grow_rows(60) issued right after spawn with no sync point) — documented rather than eliminated: added a comment stating the timing assumption explicitly (fork/exec + first repaint take longer than the resize call), matching your own "not reachable at real timings" assessment.
Speed up CI #5 (extraction-frame timing dependence, ComfyUI dependency-install spinner not covered) — left as-is; both are already called out in the PR description's "Known gaps" as accepted scope limits, not regressions.
Also caught and fixed an off-by-one in assemble_status_line itself (exact-fit boundary produced a doubled space) via a separate review pass, plus a couple of minor cleanups (deduped two near-identical PTY step assertions onto TuiSession, resolved the ComfyUI source URL once instead of twice). All commits since your review: 3ef8bb8d, d103d4dc, 3b03f6d9, 6474f5a0. Full workspace build/clippy/fmt clean, and both download-progress-01/comfyui-04 PTY scenarios re-verified passing on the latest commit.
The reason will be displayed to describe this comment to others. Learn more.
Nice PR overall. The core fix in cli_progress.rs (assemble_status_line truncating the label instead of the whole line, so the download percentage suffix always survives) is exactly right, has a clear regression test proving the original bug, and the two new e2e-test-hooks production hooks compile out to hardcoded safe defaults outside test builds. CI is green including the new E2E scenarios.
I found a few issues in the new test infrastructure worth fixing before merge, left as inline comments. None of them touch the production fix itself.
build_gzip_tarball shells out to tar synchronously from inside async fn given-steps. That blocks a tokio worker thread for the duration of a ~20MB tar invocation, and with the mock lane's default concurrency that can starve other in-flight scenarios' async work, similar in spirit to the CPU contention the "Known gaps" section already calls out for a different reason.
assert_spinner_lines_cleared's prefix truncation falls back to the full untruncated string on a non-char-boundary split instead of failing loudly, which would silently defeat the exact regression the assertion exists to catch.
The "reuse existing source folder" path in comfyui.rs::install() records a fresh call to comfyui_source_archive_url() as the manifest's source_url, not the URL that actually produced the on-disk folder. Invisible in production today (hardcoded constant), but a latent provenance bug under the test override.
paced_download.rs's own unit tests only exercise single-segment routes, never the multi-segment routes both real E2E scenarios actually register.
Copilot review comment on #421 pointed out that both PTY scenarios'
'intermediate download progress frame' assertions accepted the
unthrottled (0%) frame emitted before any bytes are read, so they
passed even if no real in-transfer frame ever rendered. Tightening
both to require a percentage strictly between 0% and 100%
(wait_for_screen_where over a percent that is neither (0%) nor
(100%)) turned up a real product bug: on an ordinary 80-column
terminal, Spinner::render_current truncated the whole assembled
'{frame} {label} {bytes}/{total} (pct%)' line from the tail once the
byte count grew past a couple of characters (e.g. '0 B' -> '1.5
MiB'), silently dropping the '(pct%)' suffix for the rest of the
transfer.
Fix: keep the byte-count/percentage suffix in its own field
(progress_suffix) and truncate the file name label instead when the
line doesn't fit -- the label was already printed in full earlier in
the command's output, so it's the safer thing to sacrifice, and the
percentage is the part a user actually watches move.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…verflows
Follow-up review of 6d88429 found a narrower-terminal edge case:
when frame + " " + suffix already meets or exceeds max_width, the
label truncates to "" and the function fell back to printing the
untruncated suffix anyway, letting the assembled line exceed
max_width — the same truncation bug class this commit's predecessor
fixed, just at a more extreme width. Falls back to truncating
"{frame} {suffix}" as a whole in that case, matching the no-suffix
branch's existing behavior.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…lear check
CI's mock E2E lane runs up to 64 scenarios concurrently. The two new
PTY-driven progress scenarios (download-progress-01, comfyui-04) depend
on real wall-clock pacing between paced HTTP chunks and the PTY's
polling cadence; under that concurrency they were starved of CPU at
unpredictable moments, letting the whole paced transfer (or the `tar`
extraction) finish between polls with no intermediate frame ever
observed. Reproduced locally only by running the full suite, never by
running either scenario alone — confirmed CI-only failures were
resource contention, not a product bug. Tag both `@serial` so cucumber
runs them without concurrent siblings, matching the isolated
conditions they were written and verified under.
Also fixes a Copilot review comment on therock_steps.rs's "neither
spinner line" check: on an 80-column terminal, assemble_status_line's
progress suffix leaves too little room for the full "Downloading
<tarball>" label, so that string is truncated away before it's ever
rendered — the assertion passed regardless of whether the line was
actually cleared. Checks a short prefix instead, which truncation
(always keeping the label's head) leaves intact while the line is
live and removes once `Spinner::clear` erases it.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…scroll away
Copilot flagged that therock_steps.rs's "final terminal screen shows
neither spinner line" check can pass even if Spinner::clear is
broken: the PTY's vt100 screen has 0 lines of scrollback, and the
tarball install's ~20-line summary is enough to scroll the
download/extraction spinner's row off the top of the default 24-row
screen before the assertion ever reads it — a scrolled-off row and a
genuinely cleared one are indistinguishable to screen_text().
Verified this both ways: dumping the screen at the assertion point
showed the whole session's output, from its first line onward, fits
inside the old 24-row height with several rows to spare (i.e. nothing
had scrolled) once rows are grown; and temporarily neutering
Spinner::clear() reproduced a real, catchable failure that the old
assertion would have missed.
Adds TuiSession::grow_rows, sized like the existing use_detail_size
but leaving columns at the standard 80 — widening columns too (as
use_detail_size does) would stop the label truncating, undermining
the very truncation behavior this scenario exists to exercise.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Four small hardening/documentation fixes flagged by a second automated
review (0 blocking findings):
- comfyui_steps.rs: grow the ComfyUI PTY to 60 rows before the install
runs, mirroring therock_steps.rs's fix for the same scrolled-off-row
hazard. ComfyUI's ~10-line completion report doesn't scroll the
spinner row off the default 24-row screen today, but nothing guarded
against that changing and silently turning the "cleared" check into
a tautology.
- cli_progress.rs: add a unit test proving set_label clears a stale
progress_suffix left over from an earlier set_progress call — this
reset had no direct test.
- therock_steps.rs: replace the magic-number `[..30]` byte slice with
a named, documented DOWNLOADING_PREFIX_LEN constant and a
panic-safe `.get(..).unwrap_or(...)` lookup, so a future shortening
of CURRENT_TARBALL degrades to a clear assertion failure instead of
an unrelated byte-boundary panic.
- expectation.rs: document that `@serial` is intentionally absent from
ScenarioDecl/from_tags — it's a cucumber-rs runner concern consumed
directly by its default which_scenario, not an expectation-
resolution concern — to head off the same "is this tag even real?"
confusion the second review initially (and explicitly) hit.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…lback
/code-review found two issues in cli_progress.rs's assemble_status_line:
the narrow-terminal fallback (when frame+suffix alone overflow max_width)
built "{frame} {suffix}", but format_progress_suffix's output already
carries its own leading space, so the rendered line doubled up on the gap
("⠋ 1.5 MiB..."). Fixed by dropping the literal space and added a
regression test pinning the exact fallback output.
Also extracts the intermediate-download-progress-frame predicate
(screen.contains("%)") && !screen.contains("(0%)") && !screen.contains
("(100%)")) out of therock_steps.rs and comfyui_steps.rs, which each
carried an identical copy, into a single
paced_download::is_intermediate_download_progress_frame — the review
flagged this as a real drift risk (a future change to the heuristic
requiring two lockstep edits) even though the surrounding pacing
constants are deliberately different per scenario and were left alone.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…wrapped in a closure
CI runs a second, separate clippy pass just for the e2e test target
(cargo clippy -p e2e-cucumber --test e2e), since that target is excluded
from --all-targets (Cargo.toml's [[test]] test = false, to keep default
`cargo test`/`--all-targets` from running it unfiltered). My local
`cargo clippy --workspace --all-targets` never touched that target and
so never caught clippy::redundant_closure on `|screen|
is_intermediate_download_progress_frame(screen)` in both step files.
The function already matches wait_for_screen_where's expected
`impl FnMut(&str) -> bool`, so pass it directly.
Now running `cargo clippy -p e2e-cucumber --test e2e -- -D warnings`
locally alongside the workspace pass to keep this in sync with what CI
actually checks.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
assemble_status_line's narrow-terminal fallback doc claimed a literal
space between frame and suffix that the code never inserts, and two
other doc comments pointed at render_current for an explanation that
actually lives in assemble_status_line. Also strengthens the
None-total unit test to assert the leading space directly rather than
relying on an unrelated test to catch a regression.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The non-e2e-test-hooks accessor isn't absent, it's a cfg(not(...))
variant that unconditionally returns the hardcoded URL. Reword so the
safety property is stated accurately.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
xorshift_payload duplicated PRNG logic the workspace already depends
on for the same "deterministic filler bytes" purpose, and a separate
hand-rolled xorshift already exists in rocm-dash-daemon's demo
generator. Renamed to deterministic_payload and rewritten on top of
rand::rngs::StdRng, seeded the same way, so the paced-download
fixtures stay deterministic without a third bespoke implementation.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The 8-line shell script answering probe_comfyui's post-install GPU
check was duplicated verbatim across two scenario fixtures. Extracted
into write_gpu_probe_shim so a future probe-schema change only needs
updating in one place.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
assemble_status_line took the label-truncation branch even when the
reserved frame+suffix width exactly matched max_width, truncating the
label to nothing while still emitting both its own literal space and
the suffix's leading space -- one column narrower and the fallback
branch produced a single space instead. Route the exact-fit boundary
through that same fallback.
render_current's terminal::size() Err arm built its status line by
hand with no truncation at all, the one rendering path with no width
bound or test coverage; it now shares assemble_status_line with an
80-column fallback width like every other path.
Also folds Spinner's separately-mutated label/progress_suffix fields
into a single SpinnerText enum, so "a plain message never carries a
stale byte-count suffix" is structural rather than a convention every
setter has to remember, and asserts the suffix-has-a-leading-space
invariant assemble_status_line's narrow fallback depends on.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
assert_spinner_lines_cleared's download-line check reads as if it
proves Spinner::clear runs on Drop, but the extraction spinner's first
repaint overwrites the same row with Clear(CurrentLine) regardless --
the extraction assertion and the ComfyUI scenario's equivalent check
are what actually cover that. Note it explicitly so the next reader
doesn't draw the same wrong conclusion.
Also documents the unstated timing assumption behind issuing
grow_rows(60) right after TuiSession::spawn with no synchronization
point: it relies on fork/exec and the first repaint taking longer than
the resize call.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
therock_steps.rs and comfyui_steps.rs each hand-rolled the identical
"wait for an intermediate download frame" and "wait for clean exit"
step bodies, differing only in a panic-message context string. Move
both onto TuiSession as assert_intermediate_download_progress_frame
and assert_exits_cleanly, parameterized by that context, so the two
step files call a shared helper instead of drifting independently.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
install() called comfyui_source_archive_url() a second time when
recording the ComfyUI manifest, after download_and_extract_source had
already resolved it once for the actual download. Have that function
return the URL it used instead, so the manifest always records exactly
what was (or would have been) fetched, not a second, independent
resolution of the same accessor.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
- build_gzip_tarball now runs tar via spawn_blocking instead of
blocking a tokio worker thread synchronously from an async
given-step.
- The tarball spinner-cleared assertion now fails loudly instead of
silently degrading to an untruncated-label check if
DOWNLOADING_PREFIX_LEN ever lands off a UTF-8 char boundary.
- ComfyUI's "reuse existing source folder" path now records the
source_url from the prior manifest on disk instead of a fresh call
to comfyui_source_archive_url(), so the recorded provenance matches
what actually produced the folder.
- Added a paced_download unit test covering a multi-segment route,
matching the real callers' route shapes.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Reuse-source-folder path now falls back to the current source URL
whenever the on-disk manifest can't be read or parsed, instead of
propagating the error and aborting install. Previously install was
the one command that recovered from a broken manifest; the prior
cleanup regressed that by making a failed load_manifest fatal.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Extracts the reuse-existing-source-folder URL lookup into
reused_source_url() and adds unit tests for the no-manifest,
valid-manifest, and unparseable-manifest cases, closing the gap
where the unreadable-manifest branch had no regression coverage.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Non-blocking items from the automated review at 36a59e9: add an
exact-output test for the ordinary label-fits path (the existing
tests only checked contains/width, so a dropped separator space or
an off-by-one label_budget slipped through undetected), drop the
unreachable empty-suffix guard in assemble_status_line, tighten two
doc comments, and note that comfyui-04's short label never exercises
the truncation fix.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
siloteemu
left a comment
juhovainio
left a comment
Summary
rocmbinary under a real PTY to provecli_progress::AnimatedSpinneractually renders and clears correctly outside of its existing in-process unit tests:download-progress-01: TheRock tarball SDK install — download progress, extraction ("Extracting …") frame, and both spinner lines cleared on exit.comfyui-04: ComfyUI source-archive install — download progress only (this path has no separate extraction phase) and the spinner line cleared on exit.PacedDownloadServerfixture (tests/e2e-cucumber/src/paced_download.rs): a loopback HTTP server that streams one named file in fixed-size, delayed chunks (falling back to normalServeDirfor everything else), so the PTY has a real chance to observe an intermediate, sub-100% frame instead of the whole transfer completing within one screen poll.e2e-test-hooks-gated hooks needed to make the ComfyUI scenario hermetic and fast:ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE(redirect the source-archive download to the fixture server) andROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS(skip the real system package-manager dependency check that otherwise runs both after a successful SDK install and duringrocm engines install vllm). Both compile out entirely in non-e2e-test-hooksbuilds, so production behavior is unaffected.docs/release-trust.md, alongside the existing TheRock base-override documentation.cli_progress::Spinner: tightening the new e2e assertion to require an actual intermediate (non-0%, non-100%) progress frame reproducibly failed, which traced back torender_currenttruncating the whole assembled status line from the tail. On an ordinary 80-column terminal, once the byte count grew past a couple of characters (e.g."0 B"→"1.5 MiB"), the trailing"(NN%)"got silently ellipsized away — hiding the download percentage for the entire rest of the transfer, with only the very first(0%)frame ever visible. Fixed by splitting the label (the file name, already shown elsewhere) from the byte-count/percentage suffix, and truncating only the label when the line overflows, so the percentage always stays visible. Also hardens the newassemble_status_linehelper against an even narrower-terminal edge case where the suffix itself doesn't fit.@serial-tagged scenarios' negative "spinner line cleared" assertions robust against both truncation (matching a prefix instead of the full, truncated label) and the emulated terminal's zero-scrollback screen (growing PTY rows so a later summary can't scroll a spinner row out of view before the check reads it).Fixes #368
Why:
AnimatedSpinneronly had in-process unit coverage before this — never run under a spawned subprocess — so a regression in its TTY detection, throttling, or line-clearing onDropcould ship unnoticed. This exercises both the tarball and ComfyUI download paths end to end against real fixture servers, and in doing so caught a genuine, independently-reproducible bug in the spinner's own rendering logic (described above).Risk: low-to-medium. Most of the diff is test-only, but
apps/rocm/src/cli_progress.rs'srender_current/assemble_status_linechange is a real behavioral fix shipped to production — it changes what gets truncated on narrow terminals during any download or long-running command that uses the spinner. Covered by unit tests (including two regression tests for both truncation edge cases) and the new e2e scenarios.Known gaps
apps/rocm/src/comfyui.rs'suvblock) isn't exercised by this coverage — the ComfyUI fixture deliberately empties the dependency list so that code path never runs during these scenarios. Intentional scope limit, not an oversight; left for follow-up coverage.download-progress-01's extraction-frame assertion is inherently timing-dependent: a 20ms poll against a realtarsubprocess run.@serial(no concurrent CPU contention) and generous payload sizing mitigate this but don't eliminate it — it's the residual flake source to watch if this scenario ever becomes intermittent.Test plan
cargo build --workspace --all-targets— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --bin rocm cli_progress— 15 passedcargo test -p e2e-cucumber --lib— 122 passedcargo xtask e2e -- -n "download-progress-01|comfyui-04"— both scenarios pass, run repeatedly, 11/11 steps each timecargo test -p e2e-cucumber --test e2e, no filter — exercises cucumber's default 64-concurrent-scenario execution) run repeatedly with 0 unexpected failures, to confirm no contention-induced flakeprek run --all-files --no-group local-tools— clean