From 75cf4c2ff6c97678d67d3ba3bc50b5458911dbcd Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 07:01:15 +0000 Subject: [PATCH 01/25] wip(e2e): PTY-observed download/extraction spinner for tarball installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a PacedDownloadServer fixture (tests/e2e-cucumber/src/paced_download.rs) that serves a file in delayed chunks over loopback HTTP, so a PTY-driven E2E scenario can observe an intermediate download-progress frame instead of the transfer completing within a single screen poll. Wires this into therock_steps.rs: a real gzip tarball built over xorshift filler bytes (to defeat compression) is served through the paced server via the existing ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE override, and a new download_progress_pty.feature scenario drives `rocm install sdk` under a real pty, asserting a sub-100% progress frame and a clean final screen. Adds two feature-gated (e2e-test-hooks) test hooks: main.rs can skip the torch runtime dependency install step, and comfyui.rs gains an overridable source-archive URL for a future ComfyUI-side PTY scenario. Checkpoint commit — WIP_PLAN.md tracks reconciliation notes and the remaining work (feature-key registration, extraction-visibility assertion, ComfyUI scenario) against the approved plan. Signed-off-by: Jussi Elo --- WIP_PLAN.md | 263 ++++++++++++++++++ apps/rocm/src/comfyui.rs | 21 +- apps/rocm/src/main.rs | 20 ++ docs/release-trust.md | 16 ++ .../features/download_progress_pty.feature | 17 ++ tests/e2e-cucumber/src/lib.rs | 1 + tests/e2e-cucumber/src/paced_download.rs | 192 +++++++++++++ tests/e2e-cucumber/tests/e2e.rs | 7 + tests/e2e-cucumber/tests/e2e/therock_steps.rs | 178 ++++++++++++ 9 files changed, 711 insertions(+), 4 deletions(-) create mode 100644 WIP_PLAN.md create mode 100644 tests/e2e-cucumber/features/download_progress_pty.feature create mode 100644 tests/e2e-cucumber/src/paced_download.rs diff --git a/WIP_PLAN.md b/WIP_PLAN.md new file mode 100644 index 000000000..85f5922ff --- /dev/null +++ b/WIP_PLAN.md @@ -0,0 +1,263 @@ +# WIP checkpoint: download/extraction spinner PTY coverage (issue #368) + +This file is a checkpoint of the approved implementation plan for finishing +this worktree's WIP (paced download server, tarball PTY scenario) and adding +the missing ComfyUI half, per issue #368. It is not meant to ship as part of +the final PR — delete it once the work lands on a proper branch off `main` +(see "Implementation approach" step 1 below). + +--- + +# Review of issue #368's plan + refined implementation plan + +## Context + +Issue #368 proposes a 4-step plan to make the download/extraction spinner +(`apps/rocm/src/cli_progress.rs`) observable in E2E tests: (1) a URL-override +test hook gated behind the `e2e-test-hooks` Cargo feature, (2) a local fixture +HTTP server, (3) driving the real install paths under a PTY, (4) new Gherkin +scenarios. The user asked me to review that plan specifically for dead code +and test-coverage sufficiency, and to fold BDD scenarios into the result. + +Investigation turned up a directly relevant fact: an **uncommitted worktree** +(`.claude/worktrees/e2e-download-spinner-pty`, branch +`worktree-e2e-download-spinner-pty`) already implements ~90% of the tarball +half of this exact plan — a paced fixture HTTP server, a real gzip tarball +fixture that defeats compression, the PTY scenario itself, and the necessary +test-hook plumbing. It is unregistered (fails `feature_naming.rs`'s key +check) and incomplete (no ComfyUI half), but its design is sound and directly +reusable. The plan below is built around finishing and reconciling this WIP +rather than re-deriving it from scratch. + +(Aside, not part of this plan: two other worktrees — +`progress-indication-downloads`, `progress-indicator-gaps` — were confirmed +to be stale, unmerged, pre-#347 drafts. They predate the spinner feature +that's already on `main` and are superseded/irrelevant. No action needed on +them.) + +## Answering the two review questions + +### Does issue #368's plan leave dead code? + +Partially, but not for the reason the issue assumed, and the WIP's actual +choices avoid it. Concretely: + +- The issue's plan says **neither** download path has an existing override + hook. That's true for ComfyUI but **false** for the tarball path: the + tarball catalog base URL is already overridable via `env_override_base` + + `ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE`/`ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE` + (`therock.rs:214-260`), a runtime-gated (not feature-gated) mechanism + already documented in `docs/release-trust.md` and already exercised by + `therock_steps.rs`'s `tarball_index_fixtures` for dry-run scenarios. Adding + a second, `e2e-test-hooks`-gated override for the same URL would be + redundant dead weight. **The WIP correctly reuses the existing mechanism + as-is for the tarball path** — no new override code needed there at all. +- For ComfyUI, there genuinely is no existing override (confirmed: + `COMFYUI_SOURCE_ARCHIVE_URL` is a hardcoded const, zero indirection). The + WIP adds a new `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE`, gated behind + `e2e-test-hooks`. This is the right call, not dead code: it mirrors the + established pattern in `dash.rs` (`dash_test_clock_offset_path`, feature-gated + accessor with a `#[cfg(not(...))]` `None` twin so it compiles away entirely + in release builds) and in `engines/lemonade/src/lib.rs`. Unlike the tarball + base, ComfyUI's archive URL has no legitimate production override use case + (no documented proxy/mirror story), so a test-only, compile-time-gated hook + is the correct fit — not a third inconsistent mechanism, but the second + instance of an existing one. +- Net conclusion: **use the right existing pattern per surface** — reuse + `env_override_base` for tarball, add a `dash.rs`-style `e2e-test-hooks` + accessor for ComfyUI. Do not introduce a uniform new mechanism across both, + which is what the issue's plan implicitly proposed and which would have + been the actual source of inconsistency/dead code. +- One real (small, mechanical) defect in the WIP as it stands: the new + `download_progress_pty.feature` isn't registered in + `tests/e2e-cucumber/tests/feature_naming.rs`'s `FEATURE_KEYS`, so + `cargo test` fails immediately on the naming-convention check. This isn't + dead code, just an incomplete registration — fixed as part of this plan + (see Implementation approach). + +### Is the test coverage sufficient? + +Mostly, once one gap is closed: + +- **Good**: the WIP's tarball scenario already asserts an intermediate + `%)`-bearing frame (proving live progress renders, not just start/end), + then a clean exit, then that no `"Downloading …"`/`"Extracting …"` text + remains on screen. This is genuinely new coverage — nothing today drives + either install path under a real PTY. +- **Gap**: the final "neither string remains" assertion doesn't prove the + *extraction* spinner ever appeared — it's equally true if the extraction + spinner never rendered at all. Since `therock.rs`'s install has two + independent spinners (download-with-progress, then extraction-without-progress, + each explicitly `drop()`ped — see `therock.rs:2695-2713`), a regression that + silently dropped the extraction spinner would not be caught. Fix: assert an + intermediate frame containing `"Extracting …"` is observed before the final + cleared state (see acceptance scenario below). To make this reliably + observable (extraction of a tiny fixture completes near-instantly, which + would race the PTY polling interval), size the fixture tarball's filler + content large enough (tens of MB) that extraction takes a measurable amount + of wall time — reusing the same xorshift-filler trick the WIP already uses + to defeat gzip compression, just scaled up. This avoids adding a new + artificial-delay test hook for a `therock.rs`-internal step. +- **Correctly scoped out (no new coverage needed here)**: the monotonic + progress clamp on retry (`set_progress_never_displays_fewer_bytes_than_already_shown`) + and all formatting/truncation logic are already thoroughly unit-tested in + `cli_progress.rs`. E2E scenarios should not re-assert these — only true + end-to-end TTY rendering belongs at this level. +- **Gap, straightforward to close**: no ComfyUI-side scenario exists yet in + the WIP (the URL-override plumbing was added but no `.feature`/steps). This + plan adds one, extending the existing `comfyui.feature` (`comfyui-01..03`) + rather than creating a new file, since it belongs with the other ComfyUI + install scenarios. +- **Not a gap**: non-interactive (piped-stdio) install behavior for both + paths is already exercised by existing non-PTY scenarios today (e.g. + `comfyui_steps.rs`'s `cli_succeeds_and_shows_progress`, explicitly testing + the non-TTY fallback). This plan only adds the missing PTY-observed half; + it doesn't touch or duplicate that coverage. +- **Not a gap**: Linux-only scoping (`@requires-os:linux`) for both new + scenarios matches the suite's existing convention — the only other PTY + scenario in the whole suite (`install_lifecycle.feature`'s `lifecycle-08`) + is also Linux-only. This is a deliberate, already-established choice, not + an oversight to flag. + +## Acceptance scenarios + +Following the `bdd-scenarios` skill's quality rules and this repo's existing +Gherkin conventions (`@id:`, `@requires-os:`, `-NN - ` naming from +`feature_naming.rs`). + +New file `tests/e2e-cucumber/features/download_progress_pty.feature` +(key: `download-progress`): + +```gherkin +@id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux +Scenario: download-progress-01 - Linux - installing an SDK tarball over an interactive terminal shows live progress + Given a paced fixture tarball is served as the release tarball + When the user installs the SDK tarball through a pseudo-terminal + Then the interactive terminal shows download progress advancing before the download completes + And the interactive terminal shows the archive being extracted + And the install completes successfully + And no download or extraction progress text remains on screen +``` + +Extend existing `tests/e2e-cucumber/features/comfyui.feature` (key: `comfyui`) +with a fourth scenario: + +```gherkin +@id:comfyui-linux-source-download-shows-live-progress @requires-os:linux +Scenario: comfyui-04 - Linux - installing ComfyUI over an interactive terminal shows live download progress + Given a paced fixture archive is served as the ComfyUI source archive + When the user installs ComfyUI through a pseudo-terminal + Then the interactive terminal shows download progress advancing before the download completes + And the install completes successfully + And no download progress text remains on screen +``` + +(No extraction-spinner assertion here — ComfyUI's install has no separate +extraction spinner, unlike the tarball path; the two surfaces are not +symmetric.) + +## Implementation approach + +1. **Reconcile the WIP worktree** (`.claude/worktrees/e2e-download-spinner-pty`) + onto a proper branch off current `main` rather than restarting: cherry-pick + or manually port `tests/e2e-cucumber/src/paced_download.rs` (the + `PacedDownloadServer`, already unit-tested), the `therock_steps.rs` fixture + setup (real gzip tarball over xorshift filler bytes, `PacedDownloadServer` + wiring, existing `ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE` + + `ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE` reuse), and the + `ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS` test hook in `main.rs`. +2. **Fix the feature registration**: add `download_progress_pty.feature` to + `FEATURE_KEYS` in `tests/e2e-cucumber/tests/feature_naming.rs` with key + `download-progress`, and rename the scenario name/id to + `download-progress-01 - ...` / `@id:download-progress-...` (the WIP + currently uses `download-progress-pty` as if it were the registered key, + which it isn't). +3. **Add the missing extraction-visibility assertion** to the tarball + scenario, sizing the fixture tarball's filler payload large enough that + extraction is reliably observable by the PTY driver's polling. +4. **Add the ComfyUI half from scratch**: port the WIP's + `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE` test hook in `comfyui.rs` + (already drafted in the WIP), add a `PacedDownloadServer`-backed fixture + in `comfyui_steps.rs` mirroring the tarball fixture pattern, and add + `comfyui-04` to `comfyui.feature` plus its step glue. +5. **Reuse `TuiSession`** (`tests/e2e-cucumber/tests/e2e/tui_driver.rs`) + unmodified for both scenarios — confirmed to be a correct fit as-is: it + spawns a real PTY (satisfying the spinner's `stderr().is_terminal()` gate) + and asserts via `vt100`'s terminal emulation, which correctly interprets + single-line CR/clear-line repaints without any special-casing needed. +6. **Document** the new ComfyUI override env var in `docs/release-trust.md` + (the WIP already drafted this section) alongside the existing TheRock + override documentation, but explicitly note it as test-only (unlike the + TheRock overrides, which are real operator-facing knobs). + +**Verification**: run `cargo test -p e2e-cucumber` (or the equivalent +naming-convention test target) to confirm `feature_naming.rs`'s checks pass +after registration. Run the two new scenarios directly against a real build +via the repo's E2E harness (`cargo xtask e2e` or equivalent, per +`install_lifecycle.feature`'s documented invocation) — this requires a live +build and real fixture servers, so it cannot be verified via unit tests or +dry runs alone; both scenarios must be run to a real pass/fail before this +work is considered done. + +## Tradeoffs + +- **Reuse vs. rewrite the WIP worktree**: the WIP is uncommitted, ~30 commits + behind `main`, and has one known defect (unregistered feature key). Given + its design (paced server, xorshift fixture trick, correct reuse of + `env_override_base`) is already sound and matches this plan's conclusions + independently, reconciling it is far cheaper than rewriting equivalent code + from scratch. Recommendation: reconcile, don't rewrite. +- **Extraction observability via a bigger fixture vs. a new artificial-delay + hook**: a delay hook (à la `ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS`) + would be more deterministic but adds another test-only code path inside + `therock.rs`'s extraction step. Sizing the fixture tarball larger achieves + the same observability with zero production-code changes. Recommendation: + bigger fixture first; only add a delay hook if timing proves flaky in + practice. + +## Open questions / Risks + +- `E2eWorld`'s expectation-matrix loading (`load_expectations()`/ + `expectation::Expectations::parse`) may require a per-scenario entry in + `expectations.toml` — not confirmed as a blocker, should be checked while + implementing rather than assumed away. +- Extraction-timing observability (the fixture-size approach) is an + assumption, not yet empirically verified against the PTY driver's actual + polling cadence — flagged above as a tradeoff with a fallback (delay hook) + if it proves unreliable. + +--- + +## Reconciliation notes (added after the plan was approved) + +Investigation into porting this WIP onto a fresh branch off `main` confirmed: + +- `git log --oneline main..HEAD` in this worktree is **empty** — this + branch has zero commits ahead of `main`. Every change described above is a + purely uncommitted working-tree modification sitting on a branch tip that + is not ahead of current `main`. +- `tests/e2e-cucumber/tests/e2e.rs`'s WIP diff against current `main` is + exactly 4 surgical additions: the `PacedDownloadServer` import, the + `paced_download_server: Option` field + doc comment on + `E2eWorld`, one `Default` line, and one `Drop` line. All `run_rocm*` + invocation helpers are byte-for-byte unmodified. +- **Flagged risk, not yet confirmed via `git diff`**: the WIP's `mod e2e { }` + block in `e2e.rs` appears to be missing `pub mod service_cleanup_steps;`, + which current `main` has (backing `service_record_cleanup.feature`). Since + the branch has no commits ahead of `main`, this must be sitting inside the + uncommitted diff itself — likely accidental. **Do not port this removal.** + When reconciling `e2e.rs`, apply only the 4 confirmed additions above to + the current `main` version of the file; never replace `main`'s file with + the WIP's version wholesale. +- Remaining exact hunks for `apps/rocm/src/main.rs`, `apps/rocm/src/comfyui.rs`, + `docs/release-trust.md`, `tests/e2e-cucumber/src/lib.rs`, and + `tests/e2e-cucumber/tests/e2e/therock_steps.rs` still need a `git diff` + pull (only `git status` file-level modification flags were confirmed + before this checkpoint) before porting them onto the fresh branch. +- `download_progress_pty.feature`'s current WIP text still needs the rename + called out in Implementation approach step 2 (`download-progress-pty-01` + → `download-progress-01`, id prefix fixed) and the extraction-visibility + step added. +- `PACED_TARBALL_PAYLOAD_BYTES` in the WIP's `therock_steps.rs` is currently + `400_000` (390KB) — must be enlarged to "tens of MB" per the plan's step 3 + before extraction timing is reliably observable. diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 2f45da87f..0f3c3fe01 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -31,6 +31,18 @@ const COMFYUI_SOURCE_ARCHIVE_NAME: &str = "ComfyUI-master.tar.gz"; const COMFYUI_DEFAULT_HOST: &str = "127.0.0.1"; const COMFYUI_DEFAULT_PORT: u16 = 8188; +/// The ComfyUI source archive URL, overridable only in `e2e-test-hooks` +/// builds so a fixture server can exercise the real download path. +#[cfg(feature = "e2e-test-hooks")] +fn comfyui_source_archive_url() -> String { + std::env::var("ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE") + .unwrap_or_else(|_| COMFYUI_SOURCE_ARCHIVE_URL.to_owned()) +} +#[cfg(not(feature = "e2e-test-hooks"))] +fn comfyui_source_archive_url() -> String { + COMFYUI_SOURCE_ARCHIVE_URL.to_owned() +} + #[derive(Debug, Clone, Eq, PartialEq)] pub(crate) struct ComfyUiInstallOptions { pub runtime_id: Option, @@ -384,7 +396,7 @@ pub(crate) fn install( runtime_version: runtime.manifest.version.clone(), runtime_root: runtime.manifest.install_root.clone(), python_executable: runtime.python.clone(), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url: comfyui_source_archive_url(), source_path: source_path.clone(), requirements_path, pip_cache_dir: None, @@ -1396,11 +1408,12 @@ fn download_and_extract_source( archive_path.display() )?; } else { - writeln!(log, "Downloading {COMFYUI_SOURCE_ARCHIVE_URL}.")?; + let source_url = comfyui_source_archive_url(); + writeln!(log, "Downloading {source_url}.")?; let download_label = "Fetching ComfyUI source archive…"; let spinner = AnimatedSpinner::start(download_label); let download_result = download_file( - COMFYUI_SOURCE_ARCHIVE_URL, + &source_url, &archive_path, &mut |bytes, total| { spinner.set_progress(download_label, bytes, total); @@ -2174,7 +2187,7 @@ mod tests { runtime_version: "7.13.0a20260511".to_owned(), runtime_root: paths.data_dir.join("runtimes").join("runtime"), python_executable: paths.data_dir.join("runtimes").join("python.exe"), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url: comfyui_source_archive_url(), source_path: source_path(&paths), requirements_path: source_path(&paths).join("requirements.txt"), pip_cache_dir: None, diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 281094944..c9163d669 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -9519,6 +9519,23 @@ fn ensure_libnuma_for_torch(approved: bool) { ); } +/// Whether an E2E scenario has asked to skip the torch runtime dependency +/// checks entirely. These checks run a real system package-manager install +/// (`apt-get` or equivalent) whenever a dependency happens to be missing on +/// the host, which is slow, network-dependent, and mutates host state — none +/// of which a PTY scenario testing an unrelated concern (e.g. the download +/// spinner) should depend on. Only active under `e2e-test-hooks`; production +/// builds always run the real check. +#[cfg(feature = "e2e-test-hooks")] +fn torch_runtime_dep_checks_disabled() -> bool { + std::env::var_os("ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS").is_some() +} + +#[cfg(not(feature = "e2e-test-hooks"))] +const fn torch_runtime_dep_checks_disabled() -> bool { + false +} + /// Shared control flow behind [`ensure_libatomic_for_torch`] and /// [`ensure_libnuma_for_torch`]: detect the dependency, print the distro-aware /// plan, and (when approved or auto-installable) run it via @@ -9528,6 +9545,9 @@ fn ensure_torch_runtime_dep(approved: bool, dep: &TorchRuntimeDep) { if cfg!(windows) { return; } + if torch_runtime_dep_checks_disabled() { + return; + } if (dep.present)() { return; } diff --git a/docs/release-trust.md b/docs/release-trust.md index 9edb6ddca..a7fda9fd1 100644 --- a/docs/release-trust.md +++ b/docs/release-trust.md @@ -228,6 +228,22 @@ Set the gate and the specific base variable together, and only in tests or deliberate manual QA against a fixture server. The `therock-next` E2E scenarios use exactly this pairing to exercise next-layout dispatch hermetically. +## ComfyUI Source Archive Override + +rocm-cli hardcodes the URL it downloads the ComfyUI source archive from. It +can be overridden, for fixture-server testing only, and only in builds +compiled with the `e2e-test-hooks` Cargo feature: + +```text +ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE +``` + +Unlike the TheRock base overrides above, this needs no separate "allow" gate: +the override accessor does not exist at all in a build without +`e2e-test-hooks`, so a stray environment variable can never redirect a +production install. A production build always resolves the hardcoded default +URL. + ## Remaining Owner Step The repo still needs a real project-owned public signing key and matching diff --git a/tests/e2e-cucumber/features/download_progress_pty.feature b/tests/e2e-cucumber/features/download_progress_pty.feature new file mode 100644 index 000000000..fb4477ec1 --- /dev/null +++ b/tests/e2e-cucumber/features/download_progress_pty.feature @@ -0,0 +1,17 @@ +Feature: Download-progress spinner under a real terminal + + # `cli_progress::AnimatedSpinner` only has in-process unit coverage today — + # it never runs under a spawned subprocess, so a regression that broke its + # TTY detection, throttling, or line-clearing on `Drop` could ship + # unnoticed. This proves it end to end: a real `rocm` binary, under a real + # PTY, downloading from a server paced slowly enough to observe an + # intermediate progress frame, and confirms the spinner line is gone once + # the process exits. + + @id:download-progress-pty-01-therock-tarball-spinner-renders @requires-os:linux + Scenario: download-progress-pty-01 - The tarball download spinner renders progress and clears on completion + Given a paced canonical release tarball fixture + When the user installs the tarball SDK for family gfx120X-all under a real terminal + Then the terminal shows an intermediate download progress frame + And the tarball install exits cleanly + And the final terminal screen shows neither spinner line diff --git a/tests/e2e-cucumber/src/lib.rs b/tests/e2e-cucumber/src/lib.rs index d56451520..f82bde9f6 100644 --- a/tests/e2e-cucumber/src/lib.rs +++ b/tests/e2e-cucumber/src/lib.rs @@ -10,6 +10,7 @@ pub mod loopback_http; pub mod mock_server; pub mod model_id; pub mod monotonic_clock; +pub mod paced_download; pub mod panic_capture; pub mod reader_failure; pub mod send_until; diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs new file mode 100644 index 000000000..d4e35bd4d --- /dev/null +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -0,0 +1,192 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Loopback HTTP server that serves one named file in delayed chunks. +//! +//! [`crate::loopback_http::LoopbackServer`] answers every request from +//! `ServeDir` as fast as the OS can read the file, which never gives a PTY +//! test harness a chance to observe an intermediate download-progress frame +//! from `cli_progress::AnimatedSpinner` — the whole transfer completes within +//! a single poll of the emulated screen. This server keeps `ServeDir` as the +//! fallback for every other path, but answers one specific file itself, with +//! an accurate `Content-Length` header and the body written as fixed-size +//! chunks separated by a fixed delay. `rocm-core`'s download client reads +//! `Content-Length` to compute the progress percentage and reads the body in +//! an ordinary streaming loop, so pacing here needs nothing special on the +//! client side — it behaves exactly as if a slow network served the file. + +use std::path::Path; +use std::sync::Arc; +use std::time::Duration; + +use axum::Router; +use axum::body::{Body, Bytes}; +use axum::http::header; +use axum::response::{IntoResponse, Response}; +use axum::routing::get; +use futures::stream; +use tower_http::services::ServeDir; + +use crate::http_server::{self, ServerHandle}; + +/// A loopback HTTP server that serves one named file in paced chunks and +/// falls back to serving `root` normally (via `ServeDir`) for every other +/// path. Shuts down on drop, like [`crate::loopback_http::LoopbackServer`]. +#[derive(Debug)] +pub struct PacedDownloadServer { + server: ServerHandle, +} + +impl PacedDownloadServer { + /// Bind an ephemeral loopback port and serve `root` (via `ServeDir`) + /// until dropped, except for `GET /`, which streams + /// `contents` in `chunk_size`-byte pieces with `delay` between each. + /// + /// Blocks until the port is bound, matching `LoopbackServer::start`, so + /// [`Self::base_url`] is immediately usable. + pub fn start( + root: &Path, + paced_file: &str, + contents: Vec, + chunk_size: usize, + delay: Duration, + ) -> Self { + let contents = Arc::new(contents); + let route = format!("/{paced_file}"); + let app = Router::new() + .route( + &route, + get(move || paced_response(contents, chunk_size, delay)), + ) + .fallback_service(ServeDir::new(root)); + Self { + server: http_server::spawn_on_own_thread(app), + } + } + + /// The served root, without a trailing slash — see + /// [`crate::loopback_http::LoopbackServer::base_url`]. + pub fn base_url(&self) -> String { + self.server.base_url() + } +} + +/// Stream `contents` as an HTTP response with an explicit `Content-Length`, +/// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the +/// first. +async fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { + let total_len = contents.len(); + let chunk_size = chunk_size.max(1); + let body = Body::from_stream(stream::unfold(0_usize, move |offset| { + let contents = Arc::clone(&contents); + async move { + if offset >= contents.len() { + return None; + } + if offset > 0 { + tokio::time::sleep(delay).await; + } + let end = (offset + chunk_size).min(contents.len()); + let chunk = Bytes::copy_from_slice(&contents[offset..end]); + Some((Ok::<_, std::io::Error>(chunk), end)) + } + })); + ([(header::CONTENT_LENGTH, total_len.to_string())], body).into_response() +} + +#[cfg(test)] +mod tests { + use std::time::Instant; + + use super::*; + + /// GET `path` from `server`, resolved against its root URL. + async fn get(server: &PacedDownloadServer, path: &str) -> reqwest::Response { + let url = server + .server + .url() + .join(path) + .unwrap_or_else(|e| panic!("{path} is not a valid relative URL: {e}")); + reqwest::get(url) + .await + .unwrap_or_else(|e| panic!("request for {path} failed: {e}")) + } + + #[tokio::test] + async fn serves_the_paced_file_byte_for_byte() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let contents: Vec = (0..10_000).map(|i| (i % 251) as u8).collect(); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + contents.clone(), + 2_000, + Duration::from_millis(1), + ); + + let response = get(&server, "archive.tar.gz").await; + assert!(response.status().is_success()); + assert_eq!( + response + .headers() + .get(reqwest::header::CONTENT_LENGTH) + .and_then(|v| v.to_str().ok()), + Some("10000"), + "Content-Length must report the exact total so the client can compute a percentage" + ); + assert_eq!( + response.bytes().await.expect("no body").as_ref(), + &contents[..] + ); + } + + #[tokio::test] + async fn pacing_delays_the_response_by_roughly_one_delay_per_chunk_boundary() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + // 3 chunks of 10 bytes: 2 chunk boundaries after the first, so the + // full transfer should take at least 2 delays. + let contents = vec![0_u8; 30]; + let delay = Duration::from_millis(50); + let server = + PacedDownloadServer::start(dir.path(), "paced.bin", contents, 10, delay); + + let started = Instant::now(); + let response = get(&server, "paced.bin").await; + let _ = response.bytes().await.expect("no body"); + let elapsed = started.elapsed(); + + assert!( + elapsed >= delay * 2, + "expected the paced response to take at least {:?}, took {elapsed:?} — \ + pacing did not actually delay the chunks", + delay * 2 + ); + } + + #[tokio::test] + async fn falls_back_to_serving_other_files_from_root() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + std::fs::write(dir.path().join("index.html"), b"") + .expect("failed to write fallback file"); + let server = + PacedDownloadServer::start(dir.path(), "archive.tar.gz", vec![1, 2, 3], 1, Duration::ZERO); + + let response = get(&server, "index.html").await; + assert!(response.status().is_success()); + assert_eq!( + response.text().await.expect("no body"), + "" + ); + } + + #[tokio::test] + async fn missing_file_is_a_404() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let server = + PacedDownloadServer::start(dir.path(), "archive.tar.gz", vec![1, 2, 3], 1, Duration::ZERO); + + let response = get(&server, "absent.zip").await; + assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); + } +} diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 57d2e001c..96503f241 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -14,6 +14,7 @@ use cucumber::{World as _, WriterExt as _}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; use e2e_cucumber::mock_server::{MockServer, ServiceRecordOptions, write_service_record_with}; +use e2e_cucumber::paced_download::PacedDownloadServer; use tempfile::TempDir; mod e2e { @@ -49,6 +50,10 @@ pub struct E2eWorld { /// Loopback file server used by artifact-prefetch scenarios. Kept on the /// World so it remains alive while the real `rocmd` subprocess downloads. pub artifact_server: Option, + /// Paced download server used by the download-progress-spinner PTY + /// scenario. Kept on the World so it remains alive while the real `rocm` + /// subprocess downloads. + pub paced_download_server: Option, /// Cache-marker destination discovered from `rocmd`'s own JSON report. pub artifact_marker_path: Option, pub endpoint: Option, @@ -220,6 +225,7 @@ impl Default for E2eWorld { Self { mock: None, artifact_server: None, + paced_download_server: None, artifact_marker_path: None, endpoint: None, model_name: None, @@ -542,6 +548,7 @@ impl Drop for E2eWorld { mock.stop(); } self.artifact_server.take(); + self.paced_download_server.take(); // A scenario that ran `rocm serve --managed` left a DETACHED supervisor + // engine process (vLLM / llama-server) that outlives this harness — the // TempDir drop below removes the on-disk record but never kills those diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index cbf87d58b..baeaa7372 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -23,12 +23,15 @@ use std::fmt::Write as _; use std::path::Path; +use std::time::Duration; use cucumber::{given, then, when}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; +use e2e_cucumber::paced_download::PacedDownloadServer; use crate::E2eWorld; +use crate::e2e::tui_driver::TuiSession; /// The exact GFX arch the next layout needs. Not a group label: the aggregate /// source publishes one `rocm-sdk-device-` payload per arch, and @@ -313,6 +316,181 @@ async fn tarball_index_fixtures(world: &mut E2eWorld) { .push(("ROCM_CLI_THEROCK_NEXT_TARBALL_BASE", next.into())); } +/// Size and pacing for the paced tarball fixture below: large enough (versus +/// the chunk size) that several chunk boundaries — and therefore several +/// observable progress frames — land before the transfer completes, and slow +/// enough per chunk that the PTY's 20ms poll cadence reliably samples an +/// intermediate, sub-100% frame rather than racing straight to completion. +const PACED_TARBALL_PAYLOAD_BYTES: usize = 400_000; +const PACED_TARBALL_CHUNK_BYTES: usize = 32_768; +const PACED_TARBALL_CHUNK_DELAY: Duration = Duration::from_millis(150); +/// Wait budget for the PTY-driven download scenario below, mirroring +/// `engines_steps.rs`'s file-local `SCREEN_TIMEOUT` convention. +const PTY_SCREEN_TIMEOUT: Duration = Duration::from_secs(30); + +/// High-entropy filler bytes for the paced tarball fixture's payload. +/// +/// Deliberately not just "looks scrambled" — see the comment at its call +/// site: an earlier multiplicative-hash sequence looked pseudo-random but +/// gzip still compressed it by over 99%. A fixed seed keeps the fixture +/// (and therefore the archive's compressed size) deterministic across runs. +fn xorshift_payload(len: usize) -> Vec { + let mut state: u64 = 0x9E37_79B9_7F4A_7C15; + (0..len) + .map(|_| { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + (state >> 56) as u8 + }) + .collect() +} + +#[given("a paced canonical release tarball fixture")] +async fn paced_tarball_fixture(world: &mut E2eWorld) { + let served = root(world).join("therock-paced-tarball-fixture"); + write_fixture( + &served.join("tarball").join("current").join("index.html"), + &tarball_index_html(&[(CURRENT_TARBALL, 1_787_000_000.0)]), + ); + + // Build a real gzip tarball so `extract_tarball` (auto-detecting `-xf`) has + // a genuine archive to unpack once the paced download completes. The + // payload bytes come from a small xorshift PRNG rather than a simple + // multiplicative-hash sequence: the latter looked scrambled but gzip still + // crushed it down to under 2 KB (well under one paced chunk), collapsing + // the whole "transfer" into a single unpaced chunk and defeating the + // pacing entirely. Xorshift output is high-entropy enough that gzip + // cannot shrink it, keeping the wire transfer close to + // `PACED_TARBALL_PAYLOAD_BYTES`. + let build_dir = root(world).join("therock-paced-tarball-build"); + let payload_dir = build_dir.join("payload"); + std::fs::create_dir_all(&payload_dir).expect("failed to create tarball payload directory"); + let payload = xorshift_payload(PACED_TARBALL_PAYLOAD_BYTES); + std::fs::write(payload_dir.join("payload.bin"), &payload) + .expect("failed to write tarball payload"); + let archive_path = build_dir.join(CURRENT_TARBALL); + let status = std::process::Command::new("tar") + .arg("-czf") + .arg(&archive_path) + .arg("-C") + .arg(&build_dir) + .arg("payload") + .status(); + match status { + Ok(status) if status.success() => {} + Ok(status) => panic!("tar exited with {status} while building the paced tarball fixture"), + Err(error) => panic!("tar is required to build the paced tarball fixture: {error}"), + } + let contents = std::fs::read(&archive_path).expect("failed to read the built tarball archive"); + + world.paced_download_server = Some(PacedDownloadServer::start( + &served, + &format!("tarball/current/{CURRENT_TARBALL}"), + contents, + PACED_TARBALL_CHUNK_BYTES, + PACED_TARBALL_CHUNK_DELAY, + )); + allow_base_overrides(world); + let base = world + .paced_download_server + .as_ref() + .expect("paced download server was just started") + .base_url(); + world.command_env.push(( + "ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE", + format!("{base}/tarball/current/").into(), + )); +} + +#[when("the user installs the tarball SDK for family gfx120X-all under a real terminal")] +async fn install_tarball_sdk_under_pty(world: &mut E2eWorld) { + // No `--version`: tarball installs only accept an explicit version pin for + // a stable ROCm 10+ selector (see `resolve_tarball_artifact_with_timeout`'s + // rejection message), so an unpinned request is what reaches the canonical + // release catalog here — which the given-step populated with exactly one + // candidate, `CURRENT_TARBALL`. + // + // This scenario is testing the download spinner, not the post-install + // torch-runtime-dependency setup — but a real SDK install completing + // successfully triggers `ensure_libatomic_for_torch`/`ensure_libnuma_for_torch`, + // which install a missing PyTorch runtime library through the real system + // package manager whenever the test host happens to lack it. That is slow, + // network-dependent, and mutates host state, none of which this scenario + // should depend on, so disable it here. + world.command_env.push(( + "ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS", + "1".into(), + )); + let session = TuiSession::spawn( + world, + &[ + "install", + "sdk", + "--channel", + "release", + "--format", + "tarball", + "--family", + GROUP_FAMILY, + "--yes", + ], + ) + .unwrap_or_else(|e| panic!("failed to spawn `rocm install sdk` under a pty: {e}")); + world.tui = Some(session); +} + +#[then("the terminal shows an intermediate download progress frame")] +async fn assert_intermediate_download_progress_frame(world: &mut E2eWorld) { + let session = world + .tui + .as_mut() + .expect("no pty session for the tarball install"); + // Any progress-bearing frame contains "%)"; the label prefix alone + // (`Downloading {file}…`) can render before the first byte count does, so + // waiting on the percent marker is what actually proves a progress frame + // — not just the spinner — was observed. + session + .wait_for_screen("%)", PTY_SCREEN_TIMEOUT) + .await + .unwrap_or_else(|e| panic!("download progress frame never appeared: {e}")); + let screen = session.screen_text(); + assert!( + !screen.contains("(100%)"), + "expected an intermediate (sub-100%) download progress frame, but the \ + first observed percent frame was already complete:\n{screen}" + ); +} + +#[then("the tarball install exits cleanly")] +async fn assert_tarball_install_exits_cleanly(world: &mut E2eWorld) { + let session = world + .tui + .as_mut() + .expect("no pty session for the tarball install"); + session + .wait_for_exit(PTY_SCREEN_TIMEOUT) + .await + .unwrap_or_else(|e| panic!("tarball install did not exit cleanly: {e}")); +} + +#[then("the final terminal screen shows neither spinner line")] +async fn assert_spinner_lines_cleared(world: &mut E2eWorld) { + let session = world + .tui + .as_ref() + .expect("no pty session for the tarball install"); + let screen = session.screen_text(); + assert!( + !screen.contains(&format!("Downloading {CURRENT_TARBALL}")), + "download spinner line was not cleared on completion:\n{screen}" + ); + assert!( + !screen.contains(&format!("Extracting {CURRENT_TARBALL}")), + "extract spinner line was not cleared on completion:\n{screen}" + ); +} + fn preview(world: &mut E2eWorld, args: &[&str]) -> i32 { let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env(world, args); world.cli_output = Some(stdout); From 8432c8cd33fa0303cc68502d00e6c8fa392811fe Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 07:37:40 +0000 Subject: [PATCH 02/25] feat(e2e): add ComfyUI PTY download-progress scenario, finish reconciliation Registers download_progress_pty.feature in feature_naming.rs's FEATURE_KEYS and renames its scenario/id to match the download-progress key convention. Adds an extraction-visibility assertion to the tarball PTY scenario: the fixture payload is enlarged so `tar -xf` takes measurable wall time, making the "Extracting ..." spinner frame reliably observable before the process exits, closing a gap where a dropped extraction spinner would go unnoticed. Adds the missing ComfyUI half (comfyui-04): a PacedDownloadServer-backed fixture archive, a real-PTY install step, and assertions for an intermediate download-progress frame and a cleared spinner line on exit. ComfyUI's extraction is an in-process, instantaneous unpack with no separate spinner, so unlike the tarball scenario there is no extraction frame to assert. Both new/updated scenarios pass against a real built `rocm` binary via `cargo xtask e2e`. Removes WIP_PLAN.md now that the plan it checkpointed has landed. Signed-off-by: Jussi Elo --- Cargo.lock | 1 + WIP_PLAN.md | 263 ------------------ tests/e2e-cucumber/Cargo.toml | 2 + tests/e2e-cucumber/features/comfyui.feature | 16 ++ .../features/download_progress_pty.feature | 5 +- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 177 ++++++++++++ tests/e2e-cucumber/tests/e2e/therock_steps.rs | 23 +- tests/e2e-cucumber/tests/feature_naming.rs | 1 + 8 files changed, 221 insertions(+), 267 deletions(-) delete mode 100644 WIP_PLAN.md diff --git a/Cargo.lock b/Cargo.lock index 8b8bf4d6f..4f560814a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1269,6 +1269,7 @@ dependencies = [ "axum", "cucumber", "e2e-report", + "futures", "portable-pty", "reqwest 0.13.4", "serde", diff --git a/WIP_PLAN.md b/WIP_PLAN.md deleted file mode 100644 index 85f5922ff..000000000 --- a/WIP_PLAN.md +++ /dev/null @@ -1,263 +0,0 @@ -# WIP checkpoint: download/extraction spinner PTY coverage (issue #368) - -This file is a checkpoint of the approved implementation plan for finishing -this worktree's WIP (paced download server, tarball PTY scenario) and adding -the missing ComfyUI half, per issue #368. It is not meant to ship as part of -the final PR — delete it once the work lands on a proper branch off `main` -(see "Implementation approach" step 1 below). - ---- - -# Review of issue #368's plan + refined implementation plan - -## Context - -Issue #368 proposes a 4-step plan to make the download/extraction spinner -(`apps/rocm/src/cli_progress.rs`) observable in E2E tests: (1) a URL-override -test hook gated behind the `e2e-test-hooks` Cargo feature, (2) a local fixture -HTTP server, (3) driving the real install paths under a PTY, (4) new Gherkin -scenarios. The user asked me to review that plan specifically for dead code -and test-coverage sufficiency, and to fold BDD scenarios into the result. - -Investigation turned up a directly relevant fact: an **uncommitted worktree** -(`.claude/worktrees/e2e-download-spinner-pty`, branch -`worktree-e2e-download-spinner-pty`) already implements ~90% of the tarball -half of this exact plan — a paced fixture HTTP server, a real gzip tarball -fixture that defeats compression, the PTY scenario itself, and the necessary -test-hook plumbing. It is unregistered (fails `feature_naming.rs`'s key -check) and incomplete (no ComfyUI half), but its design is sound and directly -reusable. The plan below is built around finishing and reconciling this WIP -rather than re-deriving it from scratch. - -(Aside, not part of this plan: two other worktrees — -`progress-indication-downloads`, `progress-indicator-gaps` — were confirmed -to be stale, unmerged, pre-#347 drafts. They predate the spinner feature -that's already on `main` and are superseded/irrelevant. No action needed on -them.) - -## Answering the two review questions - -### Does issue #368's plan leave dead code? - -Partially, but not for the reason the issue assumed, and the WIP's actual -choices avoid it. Concretely: - -- The issue's plan says **neither** download path has an existing override - hook. That's true for ComfyUI but **false** for the tarball path: the - tarball catalog base URL is already overridable via `env_override_base` + - `ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE`/`ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE` - (`therock.rs:214-260`), a runtime-gated (not feature-gated) mechanism - already documented in `docs/release-trust.md` and already exercised by - `therock_steps.rs`'s `tarball_index_fixtures` for dry-run scenarios. Adding - a second, `e2e-test-hooks`-gated override for the same URL would be - redundant dead weight. **The WIP correctly reuses the existing mechanism - as-is for the tarball path** — no new override code needed there at all. -- For ComfyUI, there genuinely is no existing override (confirmed: - `COMFYUI_SOURCE_ARCHIVE_URL` is a hardcoded const, zero indirection). The - WIP adds a new `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE`, gated behind - `e2e-test-hooks`. This is the right call, not dead code: it mirrors the - established pattern in `dash.rs` (`dash_test_clock_offset_path`, feature-gated - accessor with a `#[cfg(not(...))]` `None` twin so it compiles away entirely - in release builds) and in `engines/lemonade/src/lib.rs`. Unlike the tarball - base, ComfyUI's archive URL has no legitimate production override use case - (no documented proxy/mirror story), so a test-only, compile-time-gated hook - is the correct fit — not a third inconsistent mechanism, but the second - instance of an existing one. -- Net conclusion: **use the right existing pattern per surface** — reuse - `env_override_base` for tarball, add a `dash.rs`-style `e2e-test-hooks` - accessor for ComfyUI. Do not introduce a uniform new mechanism across both, - which is what the issue's plan implicitly proposed and which would have - been the actual source of inconsistency/dead code. -- One real (small, mechanical) defect in the WIP as it stands: the new - `download_progress_pty.feature` isn't registered in - `tests/e2e-cucumber/tests/feature_naming.rs`'s `FEATURE_KEYS`, so - `cargo test` fails immediately on the naming-convention check. This isn't - dead code, just an incomplete registration — fixed as part of this plan - (see Implementation approach). - -### Is the test coverage sufficient? - -Mostly, once one gap is closed: - -- **Good**: the WIP's tarball scenario already asserts an intermediate - `%)`-bearing frame (proving live progress renders, not just start/end), - then a clean exit, then that no `"Downloading …"`/`"Extracting …"` text - remains on screen. This is genuinely new coverage — nothing today drives - either install path under a real PTY. -- **Gap**: the final "neither string remains" assertion doesn't prove the - *extraction* spinner ever appeared — it's equally true if the extraction - spinner never rendered at all. Since `therock.rs`'s install has two - independent spinners (download-with-progress, then extraction-without-progress, - each explicitly `drop()`ped — see `therock.rs:2695-2713`), a regression that - silently dropped the extraction spinner would not be caught. Fix: assert an - intermediate frame containing `"Extracting …"` is observed before the final - cleared state (see acceptance scenario below). To make this reliably - observable (extraction of a tiny fixture completes near-instantly, which - would race the PTY polling interval), size the fixture tarball's filler - content large enough (tens of MB) that extraction takes a measurable amount - of wall time — reusing the same xorshift-filler trick the WIP already uses - to defeat gzip compression, just scaled up. This avoids adding a new - artificial-delay test hook for a `therock.rs`-internal step. -- **Correctly scoped out (no new coverage needed here)**: the monotonic - progress clamp on retry (`set_progress_never_displays_fewer_bytes_than_already_shown`) - and all formatting/truncation logic are already thoroughly unit-tested in - `cli_progress.rs`. E2E scenarios should not re-assert these — only true - end-to-end TTY rendering belongs at this level. -- **Gap, straightforward to close**: no ComfyUI-side scenario exists yet in - the WIP (the URL-override plumbing was added but no `.feature`/steps). This - plan adds one, extending the existing `comfyui.feature` (`comfyui-01..03`) - rather than creating a new file, since it belongs with the other ComfyUI - install scenarios. -- **Not a gap**: non-interactive (piped-stdio) install behavior for both - paths is already exercised by existing non-PTY scenarios today (e.g. - `comfyui_steps.rs`'s `cli_succeeds_and_shows_progress`, explicitly testing - the non-TTY fallback). This plan only adds the missing PTY-observed half; - it doesn't touch or duplicate that coverage. -- **Not a gap**: Linux-only scoping (`@requires-os:linux`) for both new - scenarios matches the suite's existing convention — the only other PTY - scenario in the whole suite (`install_lifecycle.feature`'s `lifecycle-08`) - is also Linux-only. This is a deliberate, already-established choice, not - an oversight to flag. - -## Acceptance scenarios - -Following the `bdd-scenarios` skill's quality rules and this repo's existing -Gherkin conventions (`@id:`, `@requires-os:`, `-NN - ` naming from -`feature_naming.rs`). - -New file `tests/e2e-cucumber/features/download_progress_pty.feature` -(key: `download-progress`): - -```gherkin -@id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux -Scenario: download-progress-01 - Linux - installing an SDK tarball over an interactive terminal shows live progress - Given a paced fixture tarball is served as the release tarball - When the user installs the SDK tarball through a pseudo-terminal - Then the interactive terminal shows download progress advancing before the download completes - And the interactive terminal shows the archive being extracted - And the install completes successfully - And no download or extraction progress text remains on screen -``` - -Extend existing `tests/e2e-cucumber/features/comfyui.feature` (key: `comfyui`) -with a fourth scenario: - -```gherkin -@id:comfyui-linux-source-download-shows-live-progress @requires-os:linux -Scenario: comfyui-04 - Linux - installing ComfyUI over an interactive terminal shows live download progress - Given a paced fixture archive is served as the ComfyUI source archive - When the user installs ComfyUI through a pseudo-terminal - Then the interactive terminal shows download progress advancing before the download completes - And the install completes successfully - And no download progress text remains on screen -``` - -(No extraction-spinner assertion here — ComfyUI's install has no separate -extraction spinner, unlike the tarball path; the two surfaces are not -symmetric.) - -## Implementation approach - -1. **Reconcile the WIP worktree** (`.claude/worktrees/e2e-download-spinner-pty`) - onto a proper branch off current `main` rather than restarting: cherry-pick - or manually port `tests/e2e-cucumber/src/paced_download.rs` (the - `PacedDownloadServer`, already unit-tested), the `therock_steps.rs` fixture - setup (real gzip tarball over xorshift filler bytes, `PacedDownloadServer` - wiring, existing `ROCM_CLI_THEROCK_RELEASE_TARBALL_BASE` + - `ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE` reuse), and the - `ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS` test hook in `main.rs`. -2. **Fix the feature registration**: add `download_progress_pty.feature` to - `FEATURE_KEYS` in `tests/e2e-cucumber/tests/feature_naming.rs` with key - `download-progress`, and rename the scenario name/id to - `download-progress-01 - ...` / `@id:download-progress-...` (the WIP - currently uses `download-progress-pty` as if it were the registered key, - which it isn't). -3. **Add the missing extraction-visibility assertion** to the tarball - scenario, sizing the fixture tarball's filler payload large enough that - extraction is reliably observable by the PTY driver's polling. -4. **Add the ComfyUI half from scratch**: port the WIP's - `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE` test hook in `comfyui.rs` - (already drafted in the WIP), add a `PacedDownloadServer`-backed fixture - in `comfyui_steps.rs` mirroring the tarball fixture pattern, and add - `comfyui-04` to `comfyui.feature` plus its step glue. -5. **Reuse `TuiSession`** (`tests/e2e-cucumber/tests/e2e/tui_driver.rs`) - unmodified for both scenarios — confirmed to be a correct fit as-is: it - spawns a real PTY (satisfying the spinner's `stderr().is_terminal()` gate) - and asserts via `vt100`'s terminal emulation, which correctly interprets - single-line CR/clear-line repaints without any special-casing needed. -6. **Document** the new ComfyUI override env var in `docs/release-trust.md` - (the WIP already drafted this section) alongside the existing TheRock - override documentation, but explicitly note it as test-only (unlike the - TheRock overrides, which are real operator-facing knobs). - -**Verification**: run `cargo test -p e2e-cucumber` (or the equivalent -naming-convention test target) to confirm `feature_naming.rs`'s checks pass -after registration. Run the two new scenarios directly against a real build -via the repo's E2E harness (`cargo xtask e2e` or equivalent, per -`install_lifecycle.feature`'s documented invocation) — this requires a live -build and real fixture servers, so it cannot be verified via unit tests or -dry runs alone; both scenarios must be run to a real pass/fail before this -work is considered done. - -## Tradeoffs - -- **Reuse vs. rewrite the WIP worktree**: the WIP is uncommitted, ~30 commits - behind `main`, and has one known defect (unregistered feature key). Given - its design (paced server, xorshift fixture trick, correct reuse of - `env_override_base`) is already sound and matches this plan's conclusions - independently, reconciling it is far cheaper than rewriting equivalent code - from scratch. Recommendation: reconcile, don't rewrite. -- **Extraction observability via a bigger fixture vs. a new artificial-delay - hook**: a delay hook (à la `ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS`) - would be more deterministic but adds another test-only code path inside - `therock.rs`'s extraction step. Sizing the fixture tarball larger achieves - the same observability with zero production-code changes. Recommendation: - bigger fixture first; only add a delay hook if timing proves flaky in - practice. - -## Open questions / Risks - -- `E2eWorld`'s expectation-matrix loading (`load_expectations()`/ - `expectation::Expectations::parse`) may require a per-scenario entry in - `expectations.toml` — not confirmed as a blocker, should be checked while - implementing rather than assumed away. -- Extraction-timing observability (the fixture-size approach) is an - assumption, not yet empirically verified against the PTY driver's actual - polling cadence — flagged above as a tradeoff with a fallback (delay hook) - if it proves unreliable. - ---- - -## Reconciliation notes (added after the plan was approved) - -Investigation into porting this WIP onto a fresh branch off `main` confirmed: - -- `git log --oneline main..HEAD` in this worktree is **empty** — this - branch has zero commits ahead of `main`. Every change described above is a - purely uncommitted working-tree modification sitting on a branch tip that - is not ahead of current `main`. -- `tests/e2e-cucumber/tests/e2e.rs`'s WIP diff against current `main` is - exactly 4 surgical additions: the `PacedDownloadServer` import, the - `paced_download_server: Option` field + doc comment on - `E2eWorld`, one `Default` line, and one `Drop` line. All `run_rocm*` - invocation helpers are byte-for-byte unmodified. -- **Flagged risk, not yet confirmed via `git diff`**: the WIP's `mod e2e { }` - block in `e2e.rs` appears to be missing `pub mod service_cleanup_steps;`, - which current `main` has (backing `service_record_cleanup.feature`). Since - the branch has no commits ahead of `main`, this must be sitting inside the - uncommitted diff itself — likely accidental. **Do not port this removal.** - When reconciling `e2e.rs`, apply only the 4 confirmed additions above to - the current `main` version of the file; never replace `main`'s file with - the WIP's version wholesale. -- Remaining exact hunks for `apps/rocm/src/main.rs`, `apps/rocm/src/comfyui.rs`, - `docs/release-trust.md`, `tests/e2e-cucumber/src/lib.rs`, and - `tests/e2e-cucumber/tests/e2e/therock_steps.rs` still need a `git diff` - pull (only `git status` file-level modification flags were confirmed - before this checkpoint) before porting them onto the fresh branch. -- `download_progress_pty.feature`'s current WIP text still needs the rename - called out in Implementation approach step 2 (`download-progress-pty-01` - → `download-progress-01`, id prefix fixed) and the extraction-visibility - step added. -- `PACED_TARBALL_PAYLOAD_BYTES` in the WIP's `therock_steps.rs` is currently - `400_000` (390KB) — must be enlarged to "tens of MB" per the plan's step 3 - before extraction timing is reliably observable. diff --git a/tests/e2e-cucumber/Cargo.toml b/tests/e2e-cucumber/Cargo.toml index 88e74611d..65eeacced 100644 --- a/tests/e2e-cucumber/Cargo.toml +++ b/tests/e2e-cucumber/Cargo.toml @@ -26,6 +26,8 @@ path = "src/bin/fake-tailscale.rs" axum.workspace = true cucumber = { version = "0.23", features = ["output-json", "output-junit"] } e2e-report = { path = "../../crates/e2e-report" } +# Builds the paced download fixture's chunked response body (`stream::unfold`). +futures = "0.3" # Drive the interactive dash TUI black-box: spawn the real `rocm` binary under a # pseudo-terminal (`portable-pty`, cross-platform openpty/ConPTY) and parse the # emitted terminal stream into the current on-screen grid (`vt100`). This is the diff --git a/tests/e2e-cucumber/features/comfyui.feature b/tests/e2e-cucumber/features/comfyui.feature index 8dcbbdec3..3096095b4 100644 --- a/tests/e2e-cucumber/features/comfyui.feature +++ b/tests/e2e-cucumber/features/comfyui.feature @@ -61,3 +61,19 @@ Feature: ComfyUI install reports progress and makes failures actionable And the refusal names the --runtime-id flag And the refusal names rocm runtimes activate And the refusal lists both runtime keys + + # `download_and_extract_source` reports its download the same way TheRock's + # tarball install does (`cli_progress::AnimatedSpinner`), but its extraction + # is an in-process `GzDecoder`/`tar` unpack with no separate progress phase — + # unlike TheRock's subprocess `tar -xf`, it never renders its own frame. This + # proves the download half end to end: a real `rocm` binary, under a real + # PTY, fetching from a server paced slowly enough to observe an intermediate + # progress frame, and confirms the spinner line is gone once the process + # exits. See `download_progress_pty.feature` for the TheRock counterpart. + @id:comfyui-source-download-shows-live-progress @requires-os:linux + Scenario: comfyui-04 - The source-archive download spinner renders progress and clears on completion + Given a paced ComfyUI source archive fixture + When the user installs ComfyUI under a real terminal + Then the terminal shows an intermediate ComfyUI download progress frame + And the ComfyUI install exits cleanly + And the final terminal screen shows no ComfyUI download spinner line diff --git a/tests/e2e-cucumber/features/download_progress_pty.feature b/tests/e2e-cucumber/features/download_progress_pty.feature index fb4477ec1..8a6c5a844 100644 --- a/tests/e2e-cucumber/features/download_progress_pty.feature +++ b/tests/e2e-cucumber/features/download_progress_pty.feature @@ -8,10 +8,11 @@ Feature: Download-progress spinner under a real terminal # intermediate progress frame, and confirms the spinner line is gone once # the process exits. - @id:download-progress-pty-01-therock-tarball-spinner-renders @requires-os:linux - Scenario: download-progress-pty-01 - The tarball download spinner renders progress and clears on completion + @id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux + Scenario: download-progress-01 - The tarball download spinner renders progress and clears on completion Given a paced canonical release tarball fixture When the user installs the tarball SDK for family gfx120X-all under a real terminal Then the terminal shows an intermediate download progress frame + And the terminal shows the archive being extracted And the tarball install exits cleanly And the final terminal screen shows neither spinner line diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 6f6dae113..e26e923fb 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -34,14 +34,29 @@ //! (`crates/rocm-dash-tui/src/app/mod.rs`) asserts that envelope is collapsed //! out of the chat. //! +//! `comfyui-04` covers the source-archive download spinner under a real PTY, +//! mirroring `download_progress_pty.feature`'s tarball scenario but for +//! `download_and_extract_source`'s in-process `GzDecoder`/`tar` unpack, which +//! has no separate extraction phase (unlike TheRock's subprocess `tar -xf`, it +//! never renders its own "Extracting…" frame — only the download spinner +//! line matters here). It plants a ready wheel runtime, points +//! `ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE` at a paced loopback server, +//! and gives the fixture's `requirements.txt` only torch-stack entries so +//! `install()`'s dependency filter empties out and skips the `uv` block +//! entirely — this scenario is about the download spinner, not the +//! dependency install already covered above. +//! //! Black-box throughout: the planted registry manifests are plain JSON matching //! the CLI's on-disk schema, not typed imports from the product crates. use std::path::{Path, PathBuf}; +use std::time::Duration; use cucumber::{given, then, when}; +use e2e_cucumber::paced_download::PacedDownloadServer; use crate::E2eWorld; +use crate::e2e::tui_driver::TuiSession; const RUNTIME_KEY: &str = "e2e-comfyui-runtime"; @@ -409,3 +424,165 @@ async fn refusal_lists_both_keys(world: &mut E2eWorld) { ); } } + +/// Pacing knobs for `comfyui-04`'s download server, mirroring +/// `therock_steps.rs`'s `PACED_TARBALL_*` constants: large enough that several +/// chunk boundaries land before the transfer completes, slow enough per chunk +/// that the PTY's poll cadence reliably samples an intermediate frame. Smaller +/// than TheRock's tarball fixture since there is no extraction phase here to +/// also keep observable — only the download needs to take a moment. +const PACED_ARCHIVE_PAYLOAD_BYTES: usize = 8_000_000; +const PACED_ARCHIVE_CHUNK_BYTES: usize = 650_000; +const PACED_ARCHIVE_CHUNK_DELAY: Duration = Duration::from_millis(150); +/// Wait budget for this scenario's PTY assertions, matching +/// `therock_steps.rs`'s file-local `PTY_SCREEN_TIMEOUT` convention. +const PTY_SCREEN_TIMEOUT: Duration = Duration::from_secs(30); + +/// High-entropy filler bytes padding the paced archive fixture. A naive +/// multiplicative-hash sequence looked pseudo-random but gzip still crushed it +/// by over 99%, collapsing the paced transfer into a single unpaced chunk — +/// see `therock_steps.rs::xorshift_payload`, duplicated here rather than +/// shared since the two step files are separate private modules. +fn xorshift_payload(len: usize) -> Vec { + let mut state: u64 = 0x9E37_79B9_7F4A_7C15; + (0..len) + .map(|_| { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + (state >> 56) as u8 + }) + .collect() +} + +#[given("a paced ComfyUI source archive fixture")] +async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { + let data = data_dir(world); + plant_ready_runtime(&data, RUNTIME_KEY); + + // `probe_comfyui`'s post-install GPU check runs unconditionally in + // `install()`, regardless of whether the `uv` block ran, so the runtime's + // stub Python must answer it — `plant_ready_runtime`'s default (`exit 0`, + // no output) is not enough. This scenario's requirements.txt (torch-stack + // only, below) empties the dependency list and skips the `-c` probe, but + // the shim answers both forms anyway for parity with the other fixtures. + let python = data + .join("runtimes") + .join("roots") + .join(RUNTIME_KEY) + .join("bin") + .join("python3"); + write_shim( + &python, + "#!/bin/sh\n\ + if [ \"$1\" = \"-c\" ]; then\n\ + \tprintf '{}'\n\ + \texit 0\n\ + fi\n\ + cat > \"$2\" <<'JSON'\n\ + {\"torch_version\": \"2.4.0\", \"torch_cuda_available\": true, \"device_count\": 1, \"devices\": [\"Fake GPU\"]}\n\ + JSON\n", + ); + + // Build a real `.tar.gz`: one top-level directory holding a + // `requirements.txt` naming only the torch stack, so `install()`'s + // dependency filter empties the spec list and skips `uv` entirely — this + // scenario is about the download spinner, not the dependency install. + // Padded with high-entropy filler (`xorshift_payload`) so the paced + // server has enough incompressible bytes to stream in more than one + // chunk. + let build_dir = root(world).join("comfyui-fixture").join("archive-build"); + let source_dir = build_dir.join("ComfyUI-master"); + write_fixture( + &source_dir.join("requirements.txt"), + "torch==2.4.0\ntorchvision==0.19.0\ntorchaudio==2.4.0\n", + ); + std::fs::write( + source_dir.join("payload.bin"), + xorshift_payload(PACED_ARCHIVE_PAYLOAD_BYTES), + ) + .expect("failed to write archive filler payload"); + let archive_path = build_dir.join("comfyui-source.tar.gz"); + let status = std::process::Command::new("tar") + .arg("-czf") + .arg(&archive_path) + .arg("-C") + .arg(&build_dir) + .arg("ComfyUI-master") + .status(); + match status { + Ok(status) if status.success() => {} + Ok(status) => panic!("tar exited with {status} while building the paced archive fixture"), + Err(error) => panic!("tar is required to build the paced archive fixture: {error}"), + } + let contents = std::fs::read(&archive_path).expect("failed to read the built archive"); + + let served = root(world).join("comfyui-fixture").join("archive-serve"); + std::fs::create_dir_all(&served).expect("failed to create the archive fixture serve root"); + world.paced_download_server = Some(PacedDownloadServer::start( + &served, + "archive/comfyui-source.tar.gz", + contents, + PACED_ARCHIVE_CHUNK_BYTES, + PACED_ARCHIVE_CHUNK_DELAY, + )); + let base = world + .paced_download_server + .as_ref() + .expect("paced download server was just started") + .base_url(); + world.command_env.push(( + "ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE", + format!("{base}/archive/comfyui-source.tar.gz").into(), + )); +} + +#[when("the user installs ComfyUI under a real terminal")] +async fn install_comfyui_under_pty(world: &mut E2eWorld) { + let session = TuiSession::spawn(world, &["comfyui", "install", "--runtime-id", RUNTIME_KEY]) + .unwrap_or_else(|e| panic!("failed to spawn `rocm comfyui install` under a pty: {e}")); + world.tui = Some(session); +} + +#[then("the terminal shows an intermediate ComfyUI download progress frame")] +async fn assert_intermediate_comfyui_download_progress_frame(world: &mut E2eWorld) { + let session = world + .tui + .as_mut() + .expect("no pty session for the ComfyUI install"); + session + .wait_for_screen("%)", PTY_SCREEN_TIMEOUT) + .await + .unwrap_or_else(|e| panic!("download progress frame never appeared: {e}")); + let screen = session.screen_text(); + assert!( + !screen.contains("(100%)"), + "expected an intermediate (sub-100%) download progress frame, but the \ + first observed percent frame was already complete:\n{screen}" + ); +} + +#[then("the ComfyUI install exits cleanly")] +async fn assert_comfyui_install_exits_cleanly(world: &mut E2eWorld) { + let session = world + .tui + .as_mut() + .expect("no pty session for the ComfyUI install"); + session + .wait_for_exit(PTY_SCREEN_TIMEOUT) + .await + .unwrap_or_else(|e| panic!("ComfyUI install did not exit cleanly: {e}")); +} + +#[then("the final terminal screen shows no ComfyUI download spinner line")] +async fn assert_comfyui_spinner_line_cleared(world: &mut E2eWorld) { + let session = world + .tui + .as_ref() + .expect("no pty session for the ComfyUI install"); + let screen = session.screen_text(); + assert!( + !screen.contains("Fetching ComfyUI source archive"), + "download spinner line was not cleared on completion:\n{screen}" + ); +} diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index baeaa7372..0e8ea730b 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -321,8 +321,15 @@ async fn tarball_index_fixtures(world: &mut E2eWorld) { /// observable progress frames — land before the transfer completes, and slow /// enough per chunk that the PTY's 20ms poll cadence reliably samples an /// intermediate, sub-100% frame rather than racing straight to completion. -const PACED_TARBALL_PAYLOAD_BYTES: usize = 400_000; -const PACED_TARBALL_CHUNK_BYTES: usize = 32_768; +/// +/// The payload is tens of MB, not a few hundred KB, so that `tar -xf` +/// (spawned synchronously once the download completes — see +/// `extract_tarball`) takes long enough for the PTY's 20ms poll cadence to +/// reliably catch the "Extracting …" spinner frame before the process moves +/// on. The chunk size scales with it, so the number of paced chunks — and +/// therefore the download's observed wall time — stays the same as before. +const PACED_TARBALL_PAYLOAD_BYTES: usize = 20_000_000; +const PACED_TARBALL_CHUNK_BYTES: usize = 1_600_000; const PACED_TARBALL_CHUNK_DELAY: Duration = Duration::from_millis(150); /// Wait budget for the PTY-driven download scenario below, mirroring /// `engines_steps.rs`'s file-local `SCREEN_TIMEOUT` convention. @@ -462,6 +469,18 @@ async fn assert_intermediate_download_progress_frame(world: &mut E2eWorld) { ); } +#[then("the terminal shows the archive being extracted")] +async fn assert_extraction_frame_is_shown(world: &mut E2eWorld) { + let session = world + .tui + .as_mut() + .expect("no pty session for the tarball install"); + session + .wait_for_screen(&format!("Extracting {CURRENT_TARBALL}"), PTY_SCREEN_TIMEOUT) + .await + .unwrap_or_else(|e| panic!("extraction spinner frame never appeared: {e}")); +} + #[then("the tarball install exits cleanly")] async fn assert_tarball_install_exits_cleanly(world: &mut E2eWorld) { let session = world diff --git a/tests/e2e-cucumber/tests/feature_naming.rs b/tests/e2e-cucumber/tests/feature_naming.rs index 2e4a1de9a..a241c7230 100644 --- a/tests/e2e-cucumber/tests/feature_naming.rs +++ b/tests/e2e-cucumber/tests/feature_naming.rs @@ -30,6 +30,7 @@ const FEATURE_KEYS: &[(&str, &str)] = &[ ("dash.feature", "dash"), ("dependency_guard.feature", "deps-guard"), ("diagnose.feature", "diagnose"), + ("download_progress_pty.feature", "download-progress"), ("driver_install.feature", "driver-install"), ("engine_shell.feature", "engine-shell"), ("examine.feature", "examine"), From 48b3e752c0ad5b678d40c0c0ed4dc52d85a8c1ea Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 08:07:43 +0000 Subject: [PATCH 03/25] fix(e2e): satisfy clippy and dedup paced-download payload fixture - split paced_download.rs's module doc so its first paragraph passes clippy::too_long_first_doc_paragraph - drop the async keyword from paced_response, whose only .await lives in the inner stream::unfold closure, to satisfy clippy::unused_async, and adapt its call site with std::future::ready - move xorshift_payload out of comfyui_steps.rs and therock_steps.rs (duplicated under a comment claiming the two files could not share code) into paced_download.rs as a shared pub fn, since both are pub mod siblings of the same e2e module and can import it directly - point the status_labels_managed_rocm_as_runtime_not_install test at comfyui_source_archive_url() instead of the raw COMFYUI_SOURCE_ARCHIVE_URL constant, matching its sibling test Signed-off-by: Jussi Elo --- apps/rocm/src/comfyui.rs | 2 +- tests/e2e-cucumber/src/paced_download.rs | 29 ++++++++++++++++--- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 19 +----------- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 20 +------------ 4 files changed, 28 insertions(+), 42 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 0f3c3fe01..0a90d8531 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -2100,7 +2100,7 @@ mod tests { runtime_version: runtime.version.clone(), runtime_root: runtime.install_root.clone(), python_executable: paths.data_dir.join("runtimes").join("python.exe"), - source_url: COMFYUI_SOURCE_ARCHIVE_URL.to_owned(), + source_url: comfyui_source_archive_url(), source_path: source_path(&paths), requirements_path: source_path(&paths).join("requirements.txt"), pip_cache_dir: None, diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs index d4e35bd4d..a7a462565 100644 --- a/tests/e2e-cucumber/src/paced_download.rs +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -30,8 +30,9 @@ use tower_http::services::ServeDir; use crate::http_server::{self, ServerHandle}; -/// A loopback HTTP server that serves one named file in paced chunks and -/// falls back to serving `root` normally (via `ServeDir`) for every other +/// A loopback HTTP server that serves one named file in paced chunks. +/// +/// Falls back to serving `root` normally (via `ServeDir`) for every other /// path. Shuts down on drop, like [`crate::loopback_http::LoopbackServer`]. #[derive(Debug)] pub struct PacedDownloadServer { @@ -57,7 +58,7 @@ impl PacedDownloadServer { let app = Router::new() .route( &route, - get(move || paced_response(contents, chunk_size, delay)), + get(move || std::future::ready(paced_response(contents, chunk_size, delay))), ) .fallback_service(ServeDir::new(root)); Self { @@ -72,10 +73,30 @@ impl PacedDownloadServer { } } +/// High-entropy filler bytes for a paced-fixture payload. +/// +/// A naive multiplicative-hash sequence looked pseudo-random but gzip still +/// compressed it by over 99%, collapsing a paced transfer into a single +/// unpaced chunk. A fixed seed keeps the fixture (and therefore the archive's +/// compressed size) deterministic across runs. Shared by both the TheRock +/// tarball and ComfyUI source-archive fixtures, which each need enough +/// incompressible bytes to stream in more than one paced chunk. +pub fn xorshift_payload(len: usize) -> Vec { + let mut state: u64 = 0x9E37_79B9_7F4A_7C15; + (0..len) + .map(|_| { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + (state >> 56) as u8 + }) + .collect() +} + /// Stream `contents` as an HTTP response with an explicit `Content-Length`, /// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the /// first. -async fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { +fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { let total_len = contents.len(); let chunk_size = chunk_size.max(1); let body = Body::from_stream(stream::unfold(0_usize, move |offset| { diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index e26e923fb..ae637ee6e 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -53,7 +53,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use cucumber::{given, then, when}; -use e2e_cucumber::paced_download::PacedDownloadServer; +use e2e_cucumber::paced_download::{PacedDownloadServer, xorshift_payload}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -438,23 +438,6 @@ const PACED_ARCHIVE_CHUNK_DELAY: Duration = Duration::from_millis(150); /// `therock_steps.rs`'s file-local `PTY_SCREEN_TIMEOUT` convention. const PTY_SCREEN_TIMEOUT: Duration = Duration::from_secs(30); -/// High-entropy filler bytes padding the paced archive fixture. A naive -/// multiplicative-hash sequence looked pseudo-random but gzip still crushed it -/// by over 99%, collapsing the paced transfer into a single unpaced chunk — -/// see `therock_steps.rs::xorshift_payload`, duplicated here rather than -/// shared since the two step files are separate private modules. -fn xorshift_payload(len: usize) -> Vec { - let mut state: u64 = 0x9E37_79B9_7F4A_7C15; - (0..len) - .map(|_| { - state ^= state << 13; - state ^= state >> 7; - state ^= state << 17; - (state >> 56) as u8 - }) - .collect() -} - #[given("a paced ComfyUI source archive fixture")] async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { let data = data_dir(world); diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 0e8ea730b..9a56be0e8 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -28,7 +28,7 @@ use std::time::Duration; use cucumber::{given, then, when}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; -use e2e_cucumber::paced_download::PacedDownloadServer; +use e2e_cucumber::paced_download::{PacedDownloadServer, xorshift_payload}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -335,24 +335,6 @@ const PACED_TARBALL_CHUNK_DELAY: Duration = Duration::from_millis(150); /// `engines_steps.rs`'s file-local `SCREEN_TIMEOUT` convention. const PTY_SCREEN_TIMEOUT: Duration = Duration::from_secs(30); -/// High-entropy filler bytes for the paced tarball fixture's payload. -/// -/// Deliberately not just "looks scrambled" — see the comment at its call -/// site: an earlier multiplicative-hash sequence looked pseudo-random but -/// gzip still compressed it by over 99%. A fixed seed keeps the fixture -/// (and therefore the archive's compressed size) deterministic across runs. -fn xorshift_payload(len: usize) -> Vec { - let mut state: u64 = 0x9E37_79B9_7F4A_7C15; - (0..len) - .map(|_| { - state ^= state << 13; - state ^= state >> 7; - state ^= state << 17; - (state >> 56) as u8 - }) - .collect() -} - #[given("a paced canonical release tarball fixture")] async fn paced_tarball_fixture(world: &mut E2eWorld) { let served = root(world).join("therock-paced-tarball-fixture"); From ebea49453d0622eafba946dd13445937156a9c66 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 09:22:55 +0000 Subject: [PATCH 04/25] refactor(e2e): dedup tarball fixture building and tighten paced-download details - extract build_gzip_tarball() into paced_download.rs, shared by therock_steps.rs and comfyui_steps.rs, which each shelled out to tar and read the archive back with near-identical boilerplate - debug_assert! that paced_response's chunk_size is nonzero instead of silently degrading to 1-byte chunking on a misconfigured scenario - correct paced_tarball_fixture's doc comment: the margin for catching the extraction spinner frame comes from tar's subprocess-spawn and real I/O time over a ~20MB archive, not the PTY's poll cadence, which only governs how often the already-rendered screen is re-sampled - make comfyui_steps.rs's archive build explicitly create source_dir instead of relying on an earlier write_fixture call's side effect Signed-off-by: Jussi Elo --- tests/e2e-cucumber/src/paced_download.rs | 26 ++++++++++++++++ tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 18 ++--------- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 30 +++++++------------ 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs index a7a462565..3ca0236ca 100644 --- a/tests/e2e-cucumber/src/paced_download.rs +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -93,10 +93,36 @@ pub fn xorshift_payload(len: usize) -> Vec { .collect() } +/// Builds a real gzip tarball and returns its bytes. +/// +/// Packages the single top-level directory `build_dir.join(dir_name)` into +/// `build_dir.join(archive_name)`. Shared by the TheRock tarball and ComfyUI +/// source-archive fixtures, which each need a genuine archive for their +/// installer's real `tar` extraction to unpack once the paced download +/// completes. +pub fn build_gzip_tarball(build_dir: &Path, archive_name: &str, dir_name: &str) -> Vec { + let archive_path = build_dir.join(archive_name); + let status = std::process::Command::new("tar") + .arg("-czf") + .arg(&archive_path) + .arg("-C") + .arg(build_dir) + .arg(dir_name) + .status(); + match status { + Ok(status) if status.success() => {} + Ok(status) => panic!("tar exited with {status} while building {archive_name}"), + Err(error) => panic!("tar is required to build {archive_name}: {error}"), + } + std::fs::read(&archive_path) + .unwrap_or_else(|error| panic!("failed to read built archive {archive_name}: {error}")) +} + /// Stream `contents` as an HTTP response with an explicit `Content-Length`, /// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the /// first. fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { + debug_assert!(chunk_size > 0, "chunk_size must be at least 1 byte"); let total_len = contents.len(); let chunk_size = chunk_size.max(1); let body = Body::from_stream(stream::unfold(0_usize, move |offset| { diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index ae637ee6e..9d5fd0bc4 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -53,7 +53,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use cucumber::{given, then, when}; -use e2e_cucumber::paced_download::{PacedDownloadServer, xorshift_payload}; +use e2e_cucumber::paced_download::{PacedDownloadServer, build_gzip_tarball, xorshift_payload}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -476,6 +476,7 @@ async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { // chunk. let build_dir = root(world).join("comfyui-fixture").join("archive-build"); let source_dir = build_dir.join("ComfyUI-master"); + std::fs::create_dir_all(&source_dir).expect("failed to create ComfyUI source directory"); write_fixture( &source_dir.join("requirements.txt"), "torch==2.4.0\ntorchvision==0.19.0\ntorchaudio==2.4.0\n", @@ -485,20 +486,7 @@ async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { xorshift_payload(PACED_ARCHIVE_PAYLOAD_BYTES), ) .expect("failed to write archive filler payload"); - let archive_path = build_dir.join("comfyui-source.tar.gz"); - let status = std::process::Command::new("tar") - .arg("-czf") - .arg(&archive_path) - .arg("-C") - .arg(&build_dir) - .arg("ComfyUI-master") - .status(); - match status { - Ok(status) if status.success() => {} - Ok(status) => panic!("tar exited with {status} while building the paced archive fixture"), - Err(error) => panic!("tar is required to build the paced archive fixture: {error}"), - } - let contents = std::fs::read(&archive_path).expect("failed to read the built archive"); + let contents = build_gzip_tarball(&build_dir, "comfyui-source.tar.gz", "ComfyUI-master"); let served = root(world).join("comfyui-fixture").join("archive-serve"); std::fs::create_dir_all(&served).expect("failed to create the archive fixture serve root"); diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 9a56be0e8..2af00aca3 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -28,7 +28,7 @@ use std::time::Duration; use cucumber::{given, then, when}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; -use e2e_cucumber::paced_download::{PacedDownloadServer, xorshift_payload}; +use e2e_cucumber::paced_download::{PacedDownloadServer, build_gzip_tarball, xorshift_payload}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -319,15 +319,18 @@ async fn tarball_index_fixtures(world: &mut E2eWorld) { /// Size and pacing for the paced tarball fixture below: large enough (versus /// the chunk size) that several chunk boundaries — and therefore several /// observable progress frames — land before the transfer completes, and slow -/// enough per chunk that the PTY's 20ms poll cadence reliably samples an +/// enough per chunk that the PTY's poll cadence reliably samples an /// intermediate, sub-100% frame rather than racing straight to completion. /// /// The payload is tens of MB, not a few hundred KB, so that `tar -xf` /// (spawned synchronously once the download completes — see -/// `extract_tarball`) takes long enough for the PTY's 20ms poll cadence to -/// reliably catch the "Extracting …" spinner frame before the process moves -/// on. The chunk size scales with it, so the number of paced chunks — and -/// therefore the download's observed wall time — stays the same as before. +/// `extract_tarball`) takes long enough, via its own subprocess-spawn and +/// real disk I/O over a ~20MB archive, for the "Extracting …" spinner frame +/// to still be on screen the next time the PTY's poll checks it — the poll +/// cadence only governs how often the already-rendered screen is sampled, not +/// how fast extraction itself runs. The chunk size scales with the payload, +/// so the number of paced chunks — and therefore the download's observed +/// wall time — stays the same as before. const PACED_TARBALL_PAYLOAD_BYTES: usize = 20_000_000; const PACED_TARBALL_CHUNK_BYTES: usize = 1_600_000; const PACED_TARBALL_CHUNK_DELAY: Duration = Duration::from_millis(150); @@ -358,20 +361,7 @@ async fn paced_tarball_fixture(world: &mut E2eWorld) { let payload = xorshift_payload(PACED_TARBALL_PAYLOAD_BYTES); std::fs::write(payload_dir.join("payload.bin"), &payload) .expect("failed to write tarball payload"); - let archive_path = build_dir.join(CURRENT_TARBALL); - let status = std::process::Command::new("tar") - .arg("-czf") - .arg(&archive_path) - .arg("-C") - .arg(&build_dir) - .arg("payload") - .status(); - match status { - Ok(status) if status.success() => {} - Ok(status) => panic!("tar exited with {status} while building the paced tarball fixture"), - Err(error) => panic!("tar is required to build the paced tarball fixture: {error}"), - } - let contents = std::fs::read(&archive_path).expect("failed to read the built tarball archive"); + let contents = build_gzip_tarball(&build_dir, CURRENT_TARBALL, "payload"); world.paced_download_server = Some(PacedDownloadServer::start( &served, From b444a2967fffa052073e188317fa6582092e1eee Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 09:35:16 +0000 Subject: [PATCH 05/25] style(e2e): satisfy cargo fmt (prek) Formatting drift left over from manual edits; no behavior change. Signed-off-by: Jussi Elo --- apps/rocm/src/comfyui.rs | 10 +++---- tests/e2e-cucumber/src/paced_download.rs | 26 ++++++++++++------- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 7 +++-- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 0a90d8531..6a27b4e1c 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -1412,13 +1412,9 @@ fn download_and_extract_source( writeln!(log, "Downloading {source_url}.")?; let download_label = "Fetching ComfyUI source archive…"; let spinner = AnimatedSpinner::start(download_label); - let download_result = download_file( - &source_url, - &archive_path, - &mut |bytes, total| { - spinner.set_progress(download_label, bytes, total); - }, - ); + let download_result = download_file(&source_url, &archive_path, &mut |bytes, total| { + spinner.set_progress(download_label, bytes, total); + }); drop(spinner); download_result?; } diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs index 3ca0236ca..2cb753361 100644 --- a/tests/e2e-cucumber/src/paced_download.rs +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -195,8 +195,7 @@ mod tests { // full transfer should take at least 2 delays. let contents = vec![0_u8; 30]; let delay = Duration::from_millis(50); - let server = - PacedDownloadServer::start(dir.path(), "paced.bin", contents, 10, delay); + let server = PacedDownloadServer::start(dir.path(), "paced.bin", contents, 10, delay); let started = Instant::now(); let response = get(&server, "paced.bin").await; @@ -216,22 +215,29 @@ mod tests { let dir = tempfile::tempdir().expect("failed to create temp dir"); std::fs::write(dir.path().join("index.html"), b"") .expect("failed to write fallback file"); - let server = - PacedDownloadServer::start(dir.path(), "archive.tar.gz", vec![1, 2, 3], 1, Duration::ZERO); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + vec![1, 2, 3], + 1, + Duration::ZERO, + ); let response = get(&server, "index.html").await; assert!(response.status().is_success()); - assert_eq!( - response.text().await.expect("no body"), - "" - ); + assert_eq!(response.text().await.expect("no body"), ""); } #[tokio::test] async fn missing_file_is_a_404() { let dir = tempfile::tempdir().expect("failed to create temp dir"); - let server = - PacedDownloadServer::start(dir.path(), "archive.tar.gz", vec![1, 2, 3], 1, Duration::ZERO); + let server = PacedDownloadServer::start( + dir.path(), + "archive.tar.gz", + vec![1, 2, 3], + 1, + Duration::ZERO, + ); let response = get(&server, "absent.zip").await; assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 2af00aca3..f02c3b463 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -397,10 +397,9 @@ async fn install_tarball_sdk_under_pty(world: &mut E2eWorld) { // package manager whenever the test host happens to lack it. That is slow, // network-dependent, and mutates host state, none of which this scenario // should depend on, so disable it here. - world.command_env.push(( - "ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS", - "1".into(), - )); + world + .command_env + .push(("ROCM_CLI_DISABLE_TORCH_RUNTIME_DEP_CHECKS", "1".into())); let session = TuiSession::spawn( world, &[ From ac3fbab78c8987344d51ff66194ec074aaa76510 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 10:08:42 +0000 Subject: [PATCH 06/25] fix(cli): keep the download percentage visible when the byte count grows 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 --- apps/rocm/src/cli_progress.rs | 118 ++++++++++++++---- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 22 ++-- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 26 ++-- 3 files changed, 119 insertions(+), 47 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 308cf6eb3..c9c3d09b6 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -37,6 +37,12 @@ pub(crate) struct Spinner { enabled: bool, idx: usize, label: String, + /// The byte-count/percentage tail of a progress label (e.g. + /// `" 1.5 MiB / 19.1 MiB (8%)"`), kept apart from `label` so + /// [`Self::render_current`] can always keep it intact — see its comment. + /// `None` outside of [`Self::set_progress`] (a plain [`Self::set_label`] + /// message has no such suffix to preserve). + progress_suffix: Option, active: bool, last_progress_paint: Option, max_progress_bytes: u64, @@ -48,6 +54,7 @@ impl Spinner { enabled: std::io::stderr().is_terminal(), idx: 0, label: label.into(), + progress_suffix: None, active: false, last_progress_paint: None, max_progress_bytes: 0, @@ -57,6 +64,7 @@ impl Spinner { /// Change the message shown next to the spinner (e.g. "Running smoke test…"). pub(crate) fn set_label(&mut self, label: impl Into) { self.label = label.into(); + self.progress_suffix = None; self.render_current(); } @@ -90,7 +98,8 @@ impl Spinner { } self.last_progress_paint = Some(now); self.idx = self.idx.wrapping_add(1); - self.label = format_download_progress(prefix, bytes, total); + self.label = prefix.to_owned(); + self.progress_suffix = Some(format_progress_suffix(bytes, total)); self.render_current(); } @@ -99,15 +108,24 @@ impl Spinner { return; } let frame = SPINNER_FRAMES[self.idx % SPINNER_FRAMES.len()]; - let mut line = format!("{frame} {}", self.label); - if let Ok((cols, _)) = crossterm::terminal::size() { + let line = match crossterm::terminal::size() { // A line that fits exactly at `cols` still wraps on some terminals // once the cursor lands in the last column, and `Clear::CurrentLine` // on the next repaint can only erase the row the cursor ends up on // — not a wrapped-over first row. Leaving one column of slack keeps // every repaint confined to a single row. - line = truncate_to_width(&line, cols.saturating_sub(1) as usize); - } + Ok((cols, _)) => assemble_status_line( + frame, + &self.label, + self.progress_suffix.as_deref(), + cols.saturating_sub(1) as usize, + ), + Err(_) => format!( + "{frame} {}{}", + self.label, + self.progress_suffix.as_deref().unwrap_or("") + ), + }; let mut err = std::io::stderr(); let _ = err.queue(MoveToColumn(0)); let _ = err.queue(Clear(ClearType::CurrentLine)); @@ -155,6 +173,30 @@ fn truncate_to_width(line: &str, max_width: usize) -> String { truncated } +/// Assembles `"{frame} {label}{suffix}"` within `max_width` columns. +/// +/// When `suffix` is present (a download's byte-count/percentage tail) and +/// the full line would overflow, truncates `label` — the operation's file +/// name, already printed in full elsewhere in the command's output — rather +/// than the assembled line as a whole, so `suffix` always survives intact. +/// Without truncating this way, `label`'s growth alone (e.g. `"0 B"` growing +/// into `"1.5 MiB"`) can push a line that fit at 0% past the terminal width, +/// and a blind tail-truncation would silently drop the percentage for the +/// rest of the transfer. +fn assemble_status_line( + frame: &str, + label: &str, + suffix: Option<&str>, + max_width: usize, +) -> String { + let Some(suffix) = suffix.filter(|s| !s.is_empty()) else { + return truncate_to_width(&format!("{frame} {label}"), max_width); + }; + let reserved = frame.width() + 1 + suffix.width(); + let label_budget = max_width.saturating_sub(reserved); + format!("{frame} {}{suffix}", truncate_to_width(label, label_budget)) +} + /// A [`Spinner`] kept animating by a background thread, for callers whose /// progress signal can go quiet for long stretches — a stalled download's /// `on_progress` callback only fires when bytes actually arrive, unlike @@ -247,10 +289,11 @@ impl Drop for AnimatedSpinner { } } -/// e.g. `"Downloading SDK tarball… 842.1 MiB / 3.2 GiB (26%)"`, or -/// `"Downloading SDK tarball… 842.1 MiB"` when the total is unknown (the -/// server never reported a `Content-Length`). -pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option) -> String { +/// The trailing `" / (%)"` (or `" "` when the +/// total is unknown) portion of a progress label, kept separate from the +/// operation prefix so [`Spinner::render_current`] can always keep it +/// visible — see its comment. +fn format_progress_suffix(bytes: u64, total: Option) -> String { match total { Some(total) if total > 0 => { // Floor rather than round: a multi-gigabyte transfer sitting at @@ -265,12 +308,12 @@ pub(crate) fn format_download_progress(prefix: &str, bytes: u64, total: Option format!("{prefix} {}", rocm_core::format_bytes(bytes)), + _ => format!(" {}", rocm_core::format_bytes(bytes)), } } @@ -279,27 +322,26 @@ mod tests { use super::*; #[test] - fn format_download_progress_shows_bytes_and_percent_when_total_is_known() { + fn format_progress_suffix_shows_bytes_and_percent_when_total_is_known() { let gib = 1024 * 1024 * 1024; assert_eq!( - format_download_progress("Downloading…", gib, Some(4 * gib)), - "Downloading… 1.0 GiB / 4.0 GiB (25%)" + format_progress_suffix(gib, Some(4 * gib)), + " 1.0 GiB / 4.0 GiB (25%)" ); } #[test] - fn format_download_progress_omits_total_when_unknown() { - let rendered = format_download_progress("Downloading…", 883_147_264, None); + fn format_progress_suffix_omits_total_when_unknown() { + let rendered = format_progress_suffix(883_147_264, None); assert!( !rendered.contains('/') && !rendered.contains('%'), "no total means no fraction or percentage: {rendered}" ); - assert!(rendered.starts_with("Downloading… ")); } #[test] - fn format_download_progress_clamps_percent_at_100_when_bytes_exceeds_total() { - let rendered = format_download_progress("Downloading…", 105, Some(100)); + fn format_progress_suffix_clamps_percent_at_100_when_bytes_exceeds_total() { + let rendered = format_progress_suffix(105, Some(100)); assert!( rendered.contains("(100%)"), "a server sending a few bytes past its declared length must not report over 100%: {rendered}" @@ -307,8 +349,8 @@ mod tests { } #[test] - fn format_download_progress_does_not_round_up_to_100_before_completion() { - let rendered = format_download_progress("Downloading…", 995, Some(1000)); + fn format_progress_suffix_does_not_round_up_to_100_before_completion() { + let rendered = format_progress_suffix(995, Some(1000)); assert!( rendered.contains("(99%)"), "99.5% must floor to 99%, not round up to a premature 100%: {rendered}" @@ -316,11 +358,11 @@ mod tests { } #[test] - fn format_download_progress_does_not_round_up_to_100_for_huge_totals() { + fn format_progress_suffix_does_not_round_up_to_100_for_huge_totals() { // An f64 ratio can't distinguish adjacent values this close to // u64::MAX — it collapses to 1.0 and would misreport 100% while a // byte is still outstanding. Integer arithmetic must not. - let rendered = format_download_progress("Downloading…", u64::MAX - 1, Some(u64::MAX)); + let rendered = format_progress_suffix(u64::MAX - 1, Some(u64::MAX)); assert!( !rendered.contains("(100%)"), "a single outstanding byte out of u64::MAX must not show as complete: {rendered}" @@ -331,16 +373,38 @@ mod tests { fn set_progress_never_displays_fewer_bytes_than_already_shown() { let mut spinner = Spinner::new("Downloading…"); spinner.set_progress("Downloading…", 900, Some(1000)); - assert!(spinner.label.contains("900")); + assert!(spinner.progress_suffix.as_deref().unwrap().contains("900")); // A retried transfer restarts its own byte count from a lower offset. // Force this repaint past the throttle (via a small `total` that the // clamped byte count already exceeds) to prove the clamp itself, not // just that the repaint was skipped. spinner.set_progress("Downloading…", 100, Some(500)); + let suffix = spinner.progress_suffix.as_deref().unwrap(); + assert!( + suffix.contains("900"), + "progress must not regress after a retry: {suffix}" + ); + } + + #[test] + fn assemble_status_line_keeps_the_progress_suffix_intact_when_the_label_would_overflow() { + // Regression test: an early version truncated the whole assembled + // line from the tail, which — once the byte count grew past a couple + // of characters — cut off the "(NN%)" suffix entirely on an ordinary + // 80-column terminal, silently hiding the download's percentage for + // the rest of the transfer. Truncation must eat the (already + // fully-shown-elsewhere) file name instead. + let label = "Downloading therock-dist-linux-gfx120X-all-7.10.0.tar.gz…"; + let suffix = format_progress_suffix(1_608_192, Some(20_003_341)); + let line = assemble_status_line("⠋", label, Some(&suffix), 79); + assert!( + line.contains(&suffix), + "the progress suffix must survive truncation intact: {line:?}" + ); assert!( - spinner.label.contains("900"), - "progress must not regress after a retry: {}", - spinner.label + line.width() <= 79, + "the assembled line must still respect the terminal width: {line:?} (width {})", + line.width() ); } diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 9d5fd0bc4..b0c846cf2 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -521,16 +521,22 @@ async fn assert_intermediate_comfyui_download_progress_frame(world: &mut E2eWorl .tui .as_mut() .expect("no pty session for the ComfyUI install"); + // `download_file_streaming_with_progress` reports once, unthrottled, + // before the transfer starts (an immediate "(0%)" frame) and once per + // chunk after — so waiting for any "%)" frame while only excluding + // "(100%)" would pass on that very first callback even if pacing never + // let a real in-transfer frame render. Requiring a percentage strictly + // between 0 and 100 proves an actual mid-transfer frame was observed. session - .wait_for_screen("%)", PTY_SCREEN_TIMEOUT) + .wait_for_screen_where( + "an intermediate (neither 0% nor 100%) download progress frame", + |screen| { + screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") + }, + PTY_SCREEN_TIMEOUT, + ) .await - .unwrap_or_else(|e| panic!("download progress frame never appeared: {e}")); - let screen = session.screen_text(); - assert!( - !screen.contains("(100%)"), - "expected an intermediate (sub-100%) download progress frame, but the \ - first observed percent frame was already complete:\n{screen}" - ); + .unwrap_or_else(|e| panic!("intermediate download progress frame never appeared: {e}")); } #[then("the ComfyUI install exits cleanly")] diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index f02c3b463..a97f7a5c9 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -424,20 +424,22 @@ async fn assert_intermediate_download_progress_frame(world: &mut E2eWorld) { .tui .as_mut() .expect("no pty session for the tarball install"); - // Any progress-bearing frame contains "%)"; the label prefix alone - // (`Downloading {file}…`) can render before the first byte count does, so - // waiting on the percent marker is what actually proves a progress frame - // — not just the spinner — was observed. + // `download_file_streaming_with_progress` reports once, unthrottled, + // before the transfer starts (an immediate "(0%)" frame) and once per + // chunk after — so waiting for any "%)" frame while only excluding + // "(100%)" would pass on that very first callback even if pacing never + // let a real in-transfer frame render. Requiring a percentage strictly + // between 0 and 100 proves an actual mid-transfer frame was observed. session - .wait_for_screen("%)", PTY_SCREEN_TIMEOUT) + .wait_for_screen_where( + "an intermediate (neither 0% nor 100%) download progress frame", + |screen| { + screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") + }, + PTY_SCREEN_TIMEOUT, + ) .await - .unwrap_or_else(|e| panic!("download progress frame never appeared: {e}")); - let screen = session.screen_text(); - assert!( - !screen.contains("(100%)"), - "expected an intermediate (sub-100%) download progress frame, but the \ - first observed percent frame was already complete:\n{screen}" - ); + .unwrap_or_else(|e| panic!("intermediate download progress frame never appeared: {e}")); } #[then("the terminal shows the archive being extracted")] From 5525f377f8b49914b0db046f44ce55ece18e677a Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 10:24:56 +0000 Subject: [PATCH 07/25] fix(cli): clamp assemble_status_line to max_width when suffix alone overflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review of 6d884297 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 --- apps/rocm/src/cli_progress.rs | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index c9c3d09b6..d604a51d9 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -183,6 +183,12 @@ fn truncate_to_width(line: &str, max_width: usize) -> String { /// into `"1.5 MiB"`) can push a line that fit at 0% past the terminal width, /// and a blind tail-truncation would silently drop the percentage for the /// rest of the transfer. +/// +/// If `frame` and `suffix` alone already exceed `max_width` (an extremely +/// narrow terminal, or a suffix wider than the terminal), there is no +/// longer room to keep `suffix` intact either — falls back to truncating +/// `"{frame} {suffix}"` as a whole, same as the no-suffix case below, so the +/// result never exceeds `max_width` regardless of how narrow it is. fn assemble_status_line( frame: &str, label: &str, @@ -193,7 +199,10 @@ fn assemble_status_line( return truncate_to_width(&format!("{frame} {label}"), max_width); }; let reserved = frame.width() + 1 + suffix.width(); - let label_budget = max_width.saturating_sub(reserved); + if reserved > max_width { + return truncate_to_width(&format!("{frame} {suffix}"), max_width); + } + let label_budget = max_width - reserved; format!("{frame} {}{suffix}", truncate_to_width(label, label_budget)) } @@ -408,6 +417,24 @@ mod tests { ); } + #[test] + fn assemble_status_line_never_exceeds_max_width_when_suffix_alone_overflows() { + // Regression test: when the terminal is narrower than `frame + " " + + // suffix` alone, the label truncates to "" and an earlier version + // fell back to printing the untruncated suffix anyway, silently + // exceeding `max_width` — the same bug class this module exists to + // eliminate, just past the point where the suffix can stay intact. + let suffix = format_progress_suffix(1_608_192, Some(20_003_341)); + assert!(suffix.width() > 10, "test needs an overlong suffix"); + let line = assemble_status_line("⠋", "Downloading a file…", Some(&suffix), 10); + assert!( + line.width() <= 10, + "the assembled line must never exceed max_width, even when the \ + suffix alone doesn't fit: {line:?} (width {})", + line.width() + ); + } + #[test] fn truncate_to_width_leaves_short_lines_untouched() { assert_eq!(truncate_to_width("⠋ short", 40), "⠋ short"); From 2d6524f680d299716c45ce4a62fabb4c97bcc8b0 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 11:11:17 +0000 Subject: [PATCH 08/25] fix(e2e): run PTY progress scenarios serially, fix truncation-blind clear check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 " 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 --- tests/e2e-cucumber/features/comfyui.feature | 6 +++++- .../features/download_progress_pty.feature | 9 ++++++++- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 11 ++++++++++- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/tests/e2e-cucumber/features/comfyui.feature b/tests/e2e-cucumber/features/comfyui.feature index 3096095b4..407990928 100644 --- a/tests/e2e-cucumber/features/comfyui.feature +++ b/tests/e2e-cucumber/features/comfyui.feature @@ -70,7 +70,11 @@ Feature: ComfyUI install reports progress and makes failures actionable # PTY, fetching from a server paced slowly enough to observe an intermediate # progress frame, and confirms the spinner line is gone once the process # exits. See `download_progress_pty.feature` for the TheRock counterpart. - @id:comfyui-source-download-shows-live-progress @requires-os:linux + # @serial: same reasoning as `download_progress_pty.feature`'s + # `download-progress-01` — this scenario's intermediate progress frame + # depends on real wall-clock pacing that CPU contention from up to 63 + # concurrently-running scenarios can starve away entirely. + @id:comfyui-source-download-shows-live-progress @requires-os:linux @serial Scenario: comfyui-04 - The source-archive download spinner renders progress and clears on completion Given a paced ComfyUI source archive fixture When the user installs ComfyUI under a real terminal diff --git a/tests/e2e-cucumber/features/download_progress_pty.feature b/tests/e2e-cucumber/features/download_progress_pty.feature index 8a6c5a844..bab70dcc3 100644 --- a/tests/e2e-cucumber/features/download_progress_pty.feature +++ b/tests/e2e-cucumber/features/download_progress_pty.feature @@ -8,7 +8,14 @@ Feature: Download-progress spinner under a real terminal # intermediate progress frame, and confirms the spinner line is gone once # the process exits. - @id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux + # @serial: this scenario's progress frames depend on real wall-clock pacing + # between paced HTTP chunks and the PTY's polling cadence. Running alongside + # up to 63 other scenarios (the mock lane's default concurrency) starves it + # of CPU at unpredictable moments, letting the whole paced transfer (or the + # `tar` extraction) complete between polls with no intermediate frame ever + # observed — reproduced locally by running the full suite, never by running + # this scenario alone. Serial execution removes that contention. + @id:download-progress-linux-tarball-install-shows-live-progress @requires-os:linux @serial Scenario: download-progress-01 - The tarball download spinner renders progress and clears on completion Given a paced canonical release tarball fixture When the user installs the tarball SDK for family gfx120X-all under a real terminal diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index a97f7a5c9..1fd8d7689 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -473,8 +473,17 @@ async fn assert_spinner_lines_cleared(world: &mut E2eWorld) { .as_ref() .expect("no pty session for the tarball install"); let screen = session.screen_text(); + // A live progress suffix (kept intact by `assemble_status_line`) can + // truncate this label to a fraction of its length on an 80-column + // terminal, so the download line never actually contains the full + // "Downloading {CURRENT_TARBALL}" string while it's showing — checking + // for that full string here would pass trivially whether or not the line + // was cleared. Truncation always keeps the label's head intact and cuts + // its tail, so a short prefix is present whenever the line is live and + // gone once `Spinner::clear` erases it. + let downloading_prefix = &format!("Downloading {CURRENT_TARBALL}")[..30]; assert!( - !screen.contains(&format!("Downloading {CURRENT_TARBALL}")), + !screen.contains(downloading_prefix), "download spinner line was not cleared on completion:\n{screen}" ); assert!( From 5fa06a4d636e3c3eaae6ea7ddc3fcfeab8d01015 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 12:45:23 +0000 Subject: [PATCH 09/25] fix(e2e): grow the tarball PTY's rows so cleared spinner lines can't scroll away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 13 +++++++++++++ tests/e2e-cucumber/tests/e2e/tui_driver.rs | 16 ++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 1fd8d7689..d457f4901 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -416,6 +416,19 @@ async fn install_tarball_sdk_under_pty(world: &mut E2eWorld) { ) .unwrap_or_else(|e| panic!("failed to spawn `rocm install sdk` under a pty: {e}")); world.tui = Some(session); + // A successful install's summary is ~20 lines; on the default 24-row + // screen (no scrollback) it would scroll the download/extraction spinner + // rows off the top before `assert_spinner_lines_cleared` ever reads them, + // turning that assertion into a tautology regardless of whether + // `Spinner::clear` actually ran. Grow rows only — not `use_detail_size`, + // which also widens the terminal and would stop the label from + // truncating, defeating the whole point of this scenario. + world + .tui + .as_mut() + .expect("tui session was just set") + .grow_rows(60) + .unwrap_or_else(|e| panic!("failed to grow the pty's row count: {e}")); } #[then("the terminal shows an intermediate download progress frame")] diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 1e5fb21f9..c68ef1aa8 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -375,6 +375,22 @@ impl TuiSession { self.reader_failure.take_message() } + /// Like [`Self::use_detail_size`], but keeps the standard 80-column width + /// and only grows the row count. + /// + /// For a journey whose later output (e.g. a multi-line install summary) + /// would otherwise scroll an earlier row off the visible 24-row screen + /// before an assertion can read it — with no scrollback (`vt100::Parser` + /// is constructed with 0 lines of it), a scrolled-off row reads as absent + /// whether or not it was ever actually cleared, silently turning a + /// negative assertion (e.g. "this spinner line is gone") into a + /// tautology. Widening to `DETAIL_COLS` would also change how much of a + /// long label fits before truncation, which is exactly what some of + /// these journeys are testing — so only rows grow here. + pub fn grow_rows(&mut self, rows: u16) -> Result<(), String> { + self.resize_to(rows, COLS) + } + /// Resize both the real PTY and the emulated screen to `rows`x`cols`. The /// application receives the normal terminal resize event; assertions /// continue to inspect exactly what a user would see at the new geometry. From df64d41ce88341fab54c33767fad3c5264382d43 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 22 Sep 2026 05:19:01 +0000 Subject: [PATCH 10/25] fix(e2e): close non-blocking gaps from review 5270510051 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/rocm/src/cli_progress.rs | 16 ++++++++++++++++ tests/e2e-cucumber/src/expectation.rs | 7 +++++++ tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 14 ++++++++++++-- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 13 ++++++++++++- 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index d604a51d9..28cc7acb7 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -395,6 +395,22 @@ mod tests { ); } + #[test] + fn set_label_clears_a_stale_progress_suffix() { + // A caller that moves on to a plain (non-byte-progress) message must + // not have a previous transfer's byte count still glued to it — + // `render_current` would otherwise render an unrelated message with a + // stale suffix appended. + let mut spinner = Spinner::new("Downloading…"); + spinner.set_progress("Downloading…", 900, Some(1000)); + assert!(spinner.progress_suffix.is_some()); + spinner.set_label("Checking AMD GPU access…"); + assert!( + spinner.progress_suffix.is_none(), + "set_label must clear any progress suffix left over from a prior set_progress call" + ); + } + #[test] fn assemble_status_line_keeps_the_progress_suffix_intact_when_the_label_would_overflow() { // Regression test: an early version truncated the whole assembled diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index bf382a487..145d1e7be 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -36,6 +36,13 @@ const NIGHTLY_TAG: &str = "nightly"; const LIFECYCLE_TAG: &str = "lifecycle"; const MERGE_QUEUE_TAG: &str = "merge-queue"; +// `@serial` deliberately has no entry here, and `from_tags` below silently +// ignores it like any other unrecognized tag: it isn't an expectation- +// resolution concern, it's a cucumber-rs *runner* concern, consumed directly +// by its default `Runner::Basic::which_scenario` (unmodified by this crate) to +// force a scenario to run without any concurrent sibling. A feature file's +// `@serial` tag works whether or not it's listed here. + /// The resolved expectation for one scenario on one host. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Expectation { diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index b0c846cf2..7eff317ff 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -510,8 +510,18 @@ async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { #[when("the user installs ComfyUI under a real terminal")] async fn install_comfyui_under_pty(world: &mut E2eWorld) { - let session = TuiSession::spawn(world, &["comfyui", "install", "--runtime-id", RUNTIME_KEY]) - .unwrap_or_else(|e| panic!("failed to spawn `rocm comfyui install` under a pty: {e}")); + let mut session = + TuiSession::spawn(world, &["comfyui", "install", "--runtime-id", RUNTIME_KEY]) + .unwrap_or_else(|e| panic!("failed to spawn `rocm comfyui install` under a pty: {e}")); + // The completion report is only ~10 lines today, safely inside the default + // 24-row screen (0 lines of scrollback) — but nothing guards against that + // growing and silently turning `assert_comfyui_spinner_line_cleared`'s + // negative check into a tautology, the same trap `therock_steps.rs`'s + // identical check hit once its own (much longer) summary was added. Grow + // rows now, matching that fix, rather than waiting for it to recur here. + session + .grow_rows(60) + .unwrap_or_else(|e| panic!("failed to grow the pty's row count: {e}")); world.tui = Some(session); } diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index d457f4901..9ed221086 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -56,6 +56,14 @@ const NEXT_REAL_TARBALL: &str = "therock-dist-linux-gfx120X-all-10.0.0.tar.gz"; const NEXT_TESTS_TARBALL: &str = "therock-dist-linux-gfx120X-all-tests-10.0.0.tar.gz"; const CURRENT_TARBALL: &str = "therock-dist-linux-gfx120X-all-7.10.0.tar.gz"; +/// How many leading bytes of `"Downloading {CURRENT_TARBALL}"` to check for in +/// [`assert_spinner_lines_cleared`] below. Comfortably under the ~48-column +/// label budget `assemble_status_line` leaves on an 80-column terminal even at +/// the widest realistic progress suffix (see that assertion's comment) — the +/// full string would never fully render while the line is live, so a prefix +/// this short is what actually needs to disappear on clear. +const DOWNLOADING_PREFIX_LEN: usize = 30; + /// The `current/` fixture's served base, i.e. what the canonical release /// overrides are pointed at. fn current_pip_base(world: &E2eWorld) -> String { @@ -494,7 +502,10 @@ async fn assert_spinner_lines_cleared(world: &mut E2eWorld) { // was cleared. Truncation always keeps the label's head intact and cuts // its tail, so a short prefix is present whenever the line is live and // gone once `Spinner::clear` erases it. - let downloading_prefix = &format!("Downloading {CURRENT_TARBALL}")[..30]; + let downloading = format!("Downloading {CURRENT_TARBALL}"); + let downloading_prefix = downloading + .get(..DOWNLOADING_PREFIX_LEN) + .unwrap_or(&downloading); assert!( !screen.contains(downloading_prefix), "download spinner line was not cleared on completion:\n{screen}" From 54d274ca13ebe47923ebfa6b6d47c128178b0b28 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 22 Sep 2026 05:45:37 +0000 Subject: [PATCH 11/25] fix(cli): drop a doubled space in the narrow-terminal status-line fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /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 --- apps/rocm/src/cli_progress.rs | 21 ++++++++++++++++++- tests/e2e-cucumber/src/paced_download.rs | 14 +++++++++++++ tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 15 +++++-------- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 15 +++++-------- 4 files changed, 44 insertions(+), 21 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index 28cc7acb7..b27cdedac 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -200,7 +200,10 @@ fn assemble_status_line( }; let reserved = frame.width() + 1 + suffix.width(); if reserved > max_width { - return truncate_to_width(&format!("{frame} {suffix}"), max_width); + // No literal space here: `suffix` (from `format_progress_suffix`) + // already carries its own leading space, matching the spacing the + // label_budget branch below produces between `frame` and `suffix`. + return truncate_to_width(&format!("{frame}{suffix}"), max_width); } let label_budget = max_width - reserved; format!("{frame} {}{suffix}", truncate_to_width(label, label_budget)) @@ -451,6 +454,22 @@ mod tests { ); } + #[test] + fn assemble_status_line_fallback_does_not_double_the_space_before_suffix() { + // Regression test: `format_progress_suffix` already returns a string + // with its own leading space (e.g. " 883.1 MiB"). The narrow-terminal + // fallback used to insert another literal space before it, wasting a + // column of already-scarce width on a doubled-up gap. + let suffix = format_progress_suffix(883_147_264, None); + let max_width = 1 + suffix.width(); + let line = assemble_status_line("⠋", "irrelevant label", Some(&suffix), max_width); + assert_eq!( + line, + format!("⠋{suffix}"), + "the suffix's own leading space must not be doubled up: {line:?}" + ); + } + #[test] fn truncate_to_width_leaves_short_lines_untouched() { assert_eq!(truncate_to_width("⠋ short", 40), "⠋ short"); diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs index 2cb753361..5d8d72f1b 100644 --- a/tests/e2e-cucumber/src/paced_download.rs +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -118,6 +118,20 @@ pub fn build_gzip_tarball(build_dir: &Path, archive_name: &str, dir_name: &str) .unwrap_or_else(|error| panic!("failed to read built archive {archive_name}: {error}")) } +/// Whether `screen` shows a genuine in-transfer download progress frame: a +/// percentage strictly between 0% and 100%. +/// +/// `download_file_streaming_with_progress` reports once, unthrottled, before +/// the transfer starts (an immediate "(0%)" frame) and once per chunk after — +/// so a caller that only excluded "(100%)" would pass on that very first +/// callback even if pacing never let a real in-transfer frame render. +/// Requiring a percentage strictly between 0 and 100 proves an actual +/// mid-transfer frame was observed. Shared by every paced-download PTY +/// scenario so this heuristic can't drift between per-scenario copies. +pub fn is_intermediate_download_progress_frame(screen: &str) -> bool { + screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") +} + /// Stream `contents` as an HTTP response with an explicit `Content-Length`, /// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the /// first. diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 7eff317ff..2c0831c1b 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -53,7 +53,10 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use cucumber::{given, then, when}; -use e2e_cucumber::paced_download::{PacedDownloadServer, build_gzip_tarball, xorshift_payload}; +use e2e_cucumber::paced_download::{ + PacedDownloadServer, build_gzip_tarball, is_intermediate_download_progress_frame, + xorshift_payload, +}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -531,18 +534,10 @@ async fn assert_intermediate_comfyui_download_progress_frame(world: &mut E2eWorl .tui .as_mut() .expect("no pty session for the ComfyUI install"); - // `download_file_streaming_with_progress` reports once, unthrottled, - // before the transfer starts (an immediate "(0%)" frame) and once per - // chunk after — so waiting for any "%)" frame while only excluding - // "(100%)" would pass on that very first callback even if pacing never - // let a real in-transfer frame render. Requiring a percentage strictly - // between 0 and 100 proves an actual mid-transfer frame was observed. session .wait_for_screen_where( "an intermediate (neither 0% nor 100%) download progress frame", - |screen| { - screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") - }, + |screen| is_intermediate_download_progress_frame(screen), PTY_SCREEN_TIMEOUT, ) .await diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 9ed221086..012ac15f4 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -28,7 +28,10 @@ use std::time::Duration; use cucumber::{given, then, when}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; -use e2e_cucumber::paced_download::{PacedDownloadServer, build_gzip_tarball, xorshift_payload}; +use e2e_cucumber::paced_download::{ + PacedDownloadServer, build_gzip_tarball, is_intermediate_download_progress_frame, + xorshift_payload, +}; use crate::E2eWorld; use crate::e2e::tui_driver::TuiSession; @@ -445,18 +448,10 @@ async fn assert_intermediate_download_progress_frame(world: &mut E2eWorld) { .tui .as_mut() .expect("no pty session for the tarball install"); - // `download_file_streaming_with_progress` reports once, unthrottled, - // before the transfer starts (an immediate "(0%)" frame) and once per - // chunk after — so waiting for any "%)" frame while only excluding - // "(100%)" would pass on that very first callback even if pacing never - // let a real in-transfer frame render. Requiring a percentage strictly - // between 0 and 100 proves an actual mid-transfer frame was observed. session .wait_for_screen_where( "an intermediate (neither 0% nor 100%) download progress frame", - |screen| { - screen.contains("%)") && !screen.contains("(0%)") && !screen.contains("(100%)") - }, + |screen| is_intermediate_download_progress_frame(screen), PTY_SCREEN_TIMEOUT, ) .await From a97199a35d9cd8af292cf097b28ef6bd85f2043f Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 22 Sep 2026 05:54:52 +0000 Subject: [PATCH 12/25] fix(e2e): pass is_intermediate_download_progress_frame directly, not 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 --- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 2 +- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 2c0831c1b..7131db8cc 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -537,7 +537,7 @@ async fn assert_intermediate_comfyui_download_progress_frame(world: &mut E2eWorl session .wait_for_screen_where( "an intermediate (neither 0% nor 100%) download progress frame", - |screen| is_intermediate_download_progress_frame(screen), + is_intermediate_download_progress_frame, PTY_SCREEN_TIMEOUT, ) .await diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 012ac15f4..93cf7378b 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -451,7 +451,7 @@ async fn assert_intermediate_download_progress_frame(world: &mut E2eWorld) { session .wait_for_screen_where( "an intermediate (neither 0% nor 100%) download progress frame", - |screen| is_intermediate_download_progress_frame(screen), + is_intermediate_download_progress_frame, PTY_SCREEN_TIMEOUT, ) .await From 9ab627845137f18568bbf8f042701d60567d8bb4 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 23 Sep 2026 06:15:03 +0000 Subject: [PATCH 13/25] fix(cli): correct stale doc comments in cli_progress 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 --- apps/rocm/src/cli_progress.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/rocm/src/cli_progress.rs b/apps/rocm/src/cli_progress.rs index b27cdedac..31d10b353 100644 --- a/apps/rocm/src/cli_progress.rs +++ b/apps/rocm/src/cli_progress.rs @@ -39,7 +39,7 @@ pub(crate) struct Spinner { label: String, /// The byte-count/percentage tail of a progress label (e.g. /// `" 1.5 MiB / 19.1 MiB (8%)"`), kept apart from `label` so - /// [`Self::render_current`] can always keep it intact — see its comment. + /// [`assemble_status_line`] can always keep it intact — see its comment. /// `None` outside of [`Self::set_progress`] (a plain [`Self::set_label`] /// message has no such suffix to preserve). progress_suffix: Option, @@ -187,7 +187,8 @@ fn truncate_to_width(line: &str, max_width: usize) -> String { /// If `frame` and `suffix` alone already exceed `max_width` (an extremely /// narrow terminal, or a suffix wider than the terminal), there is no /// longer room to keep `suffix` intact either — falls back to truncating -/// `"{frame} {suffix}"` as a whole, same as the no-suffix case below, so the +/// `"{frame}{suffix}"` as a whole (no literal space; `suffix` already +/// carries its own leading space), same as the no-suffix case below, so the /// result never exceeds `max_width` regardless of how narrow it is. fn assemble_status_line( frame: &str, @@ -303,7 +304,7 @@ impl Drop for AnimatedSpinner { /// The trailing `" / (%)"` (or `" "` when the /// total is unknown) portion of a progress label, kept separate from the -/// operation prefix so [`Spinner::render_current`] can always keep it +/// operation prefix so [`assemble_status_line`] can always keep it /// visible — see its comment. fn format_progress_suffix(bytes: u64, total: Option) -> String { match total { @@ -349,6 +350,10 @@ mod tests { !rendered.contains('/') && !rendered.contains('%'), "no total means no fraction or percentage: {rendered}" ); + assert!( + rendered.starts_with(' '), + "format_progress_suffix's None-total branch must keep its own leading space: {rendered}" + ); } #[test] From 0fbaf6ea55f56516581ee0c315cf50d42040906b Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 23 Sep 2026 06:15:11 +0000 Subject: [PATCH 14/25] docs(release-trust): clarify ComfyUI override wording 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 --- docs/release-trust.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/release-trust.md b/docs/release-trust.md index a7fda9fd1..3fa23afce 100644 --- a/docs/release-trust.md +++ b/docs/release-trust.md @@ -239,10 +239,10 @@ ROCM_CLI_COMFYUI_SOURCE_ARCHIVE_URL_OVERRIDE ``` Unlike the TheRock base overrides above, this needs no separate "allow" gate: -the override accessor does not exist at all in a build without -`e2e-test-hooks`, so a stray environment variable can never redirect a -production install. A production build always resolves the hardcoded default -URL. +the override *logic* does not exist at all in a build without +`e2e-test-hooks` — the accessor compiled in that configuration ignores the +environment entirely and unconditionally returns the hardcoded default URL, so +a stray environment variable can never redirect a production install. ## Remaining Owner Step From 6cfa19337f4c61a6745d069b68523ed839ded656 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 23 Sep 2026 06:15:28 +0000 Subject: [PATCH 15/25] test(e2e): replace hand-rolled xorshift PRNG with workspace rand 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 --- Cargo.lock | 1 + tests/e2e-cucumber/Cargo.toml | 3 ++ tests/e2e-cucumber/src/paced_download.rs | 28 ++++++++++--------- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 8 +++--- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 10 +++---- 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4f560814a..531d21b17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1271,6 +1271,7 @@ dependencies = [ "e2e-report", "futures", "portable-pty", + "rand 0.9.4", "reqwest 0.13.4", "serde", "serde_json", diff --git a/tests/e2e-cucumber/Cargo.toml b/tests/e2e-cucumber/Cargo.toml index 65eeacced..89e1e6541 100644 --- a/tests/e2e-cucumber/Cargo.toml +++ b/tests/e2e-cucumber/Cargo.toml @@ -33,6 +33,9 @@ futures = "0.3" # emitted terminal stream into the current on-screen grid (`vt100`). This is the # only way to exercise the crossterm raw-mode event loop a piped `Command` can't. portable-pty = "0.9" +# Deterministic filler bytes for the paced-download fixtures (see +# `src/paced_download.rs`) — a seeded `StdRng` rather than a hand-rolled PRNG. +rand.workspace = true reqwest = { version = "0.13", features = ["json"] } serde.workspace = true serde_json.workspace = true diff --git a/tests/e2e-cucumber/src/paced_download.rs b/tests/e2e-cucumber/src/paced_download.rs index 5d8d72f1b..d1729f5aa 100644 --- a/tests/e2e-cucumber/src/paced_download.rs +++ b/tests/e2e-cucumber/src/paced_download.rs @@ -26,6 +26,7 @@ use axum::http::header; use axum::response::{IntoResponse, Response}; use axum::routing::get; use futures::stream; +use rand::{RngCore, SeedableRng}; use tower_http::services::ServeDir; use crate::http_server::{self, ServerHandle}; @@ -78,19 +79,17 @@ impl PacedDownloadServer { /// A naive multiplicative-hash sequence looked pseudo-random but gzip still /// compressed it by over 99%, collapsing a paced transfer into a single /// unpaced chunk. A fixed seed keeps the fixture (and therefore the archive's -/// compressed size) deterministic across runs. Shared by both the TheRock -/// tarball and ComfyUI source-archive fixtures, which each need enough -/// incompressible bytes to stream in more than one paced chunk. -pub fn xorshift_payload(len: usize) -> Vec { - let mut state: u64 = 0x9E37_79B9_7F4A_7C15; - (0..len) - .map(|_| { - state ^= state << 13; - state ^= state >> 7; - state ^= state << 17; - (state >> 56) as u8 - }) - .collect() +/// compressed size) deterministic across runs — though `rand` doesn't +/// guarantee `StdRng`'s algorithm is stable across crate versions, so a +/// future `rand` bump could change these bytes (and the compressed size) +/// even with the seed unchanged. Shared by both the TheRock tarball and +/// ComfyUI source-archive fixtures, which each need enough incompressible +/// bytes to stream in more than one paced chunk. +pub fn deterministic_payload(len: usize) -> Vec { + let mut rng = rand::rngs::StdRng::seed_from_u64(0x9E37_79B9_7F4A_7C15); + let mut buf = vec![0u8; len]; + rng.fill_bytes(&mut buf); + buf } /// Builds a real gzip tarball and returns its bytes. @@ -136,6 +135,9 @@ pub fn is_intermediate_download_progress_frame(screen: &str) -> bool { /// in `chunk_size`-byte pieces, sleeping `delay` before every chunk after the /// first. fn paced_response(contents: Arc>, chunk_size: usize, delay: Duration) -> Response { + // Fail fast on a fixture bug in debug builds, but degrade to 1 byte per + // chunk rather than panic (dividing the whole transfer into single-byte + // chunks is slow, not wrong) if this ever runs in a release test binary. debug_assert!(chunk_size > 0, "chunk_size must be at least 1 byte"); let total_len = contents.len(); let chunk_size = chunk_size.max(1); diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index 7131db8cc..eea066c64 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -54,8 +54,8 @@ use std::time::Duration; use cucumber::{given, then, when}; use e2e_cucumber::paced_download::{ - PacedDownloadServer, build_gzip_tarball, is_intermediate_download_progress_frame, - xorshift_payload, + PacedDownloadServer, build_gzip_tarball, deterministic_payload, + is_intermediate_download_progress_frame, }; use crate::E2eWorld; @@ -474,7 +474,7 @@ async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { // `requirements.txt` naming only the torch stack, so `install()`'s // dependency filter empties the spec list and skips `uv` entirely — this // scenario is about the download spinner, not the dependency install. - // Padded with high-entropy filler (`xorshift_payload`) so the paced + // Padded with high-entropy filler (`deterministic_payload`) so the paced // server has enough incompressible bytes to stream in more than one // chunk. let build_dir = root(world).join("comfyui-fixture").join("archive-build"); @@ -486,7 +486,7 @@ async fn paced_comfyui_source_archive_fixture(world: &mut E2eWorld) { ); std::fs::write( source_dir.join("payload.bin"), - xorshift_payload(PACED_ARCHIVE_PAYLOAD_BYTES), + deterministic_payload(PACED_ARCHIVE_PAYLOAD_BYTES), ) .expect("failed to write archive filler payload"); let contents = build_gzip_tarball(&build_dir, "comfyui-source.tar.gz", "ComfyUI-master"); diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 93cf7378b..90dae65ae 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -29,8 +29,8 @@ use cucumber::{given, then, when}; use e2e_cucumber::cli_failure_report; use e2e_cucumber::loopback_http::LoopbackServer; use e2e_cucumber::paced_download::{ - PacedDownloadServer, build_gzip_tarball, is_intermediate_download_progress_frame, - xorshift_payload, + PacedDownloadServer, build_gzip_tarball, deterministic_payload, + is_intermediate_download_progress_frame, }; use crate::E2eWorld; @@ -359,17 +359,17 @@ async fn paced_tarball_fixture(world: &mut E2eWorld) { // Build a real gzip tarball so `extract_tarball` (auto-detecting `-xf`) has // a genuine archive to unpack once the paced download completes. The - // payload bytes come from a small xorshift PRNG rather than a simple + // payload bytes come from a seeded CSPRNG (`StdRng`) rather than a simple // multiplicative-hash sequence: the latter looked scrambled but gzip still // crushed it down to under 2 KB (well under one paced chunk), collapsing // the whole "transfer" into a single unpaced chunk and defeating the - // pacing entirely. Xorshift output is high-entropy enough that gzip + // pacing entirely. `StdRng` output is high-entropy enough that gzip // cannot shrink it, keeping the wire transfer close to // `PACED_TARBALL_PAYLOAD_BYTES`. let build_dir = root(world).join("therock-paced-tarball-build"); let payload_dir = build_dir.join("payload"); std::fs::create_dir_all(&payload_dir).expect("failed to create tarball payload directory"); - let payload = xorshift_payload(PACED_TARBALL_PAYLOAD_BYTES); + let payload = deterministic_payload(PACED_TARBALL_PAYLOAD_BYTES); std::fs::write(payload_dir.join("payload.bin"), &payload) .expect("failed to write tarball payload"); let contents = build_gzip_tarball(&build_dir, CURRENT_TARBALL, "payload"); From ff913589a5c23cef6dca252a406ab9ceb1078310 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 23 Sep 2026 06:30:14 +0000 Subject: [PATCH 16/25] refactor(e2e): dedup the GPU-probe Python shim in comfyui_steps 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 --- tests/e2e-cucumber/tests/e2e/comfyui_steps.rs | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs index eea066c64..eb541f867 100644 --- a/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/comfyui_steps.rs @@ -107,6 +107,24 @@ fn write_shim(path: &Path, body: &str) { } } +/// Writes a fake runtime Python that answers both forms `probe_comfyui`'s +/// post-install GPU check shells out to: `-c