From ee34ea1cecc1cf3d171b291892173ecfefea152a Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 07:27:50 +0000 Subject: [PATCH 1/5] Gate comfyui-01/02/03 on the required Windows lane PR #637 made these ComfyUI install scenarios hermetic and Windows-portable, but no required CI lane actually ran them on Windows: windows-build-and-test only runs @lifecycle scenarios, and the only Windows lane that did run them (e2e-selfhosted.yml) is optional and not a required check (#689). Add E2E_ALSO_RUN_IDS, a small opt-in mechanism that folds specific non-@lifecycle scenarios by @id into an E2E_ONLY_LIFECYCLE lane without retagging them, and point windows-build-and-test's lifecycle step at the three comfyui ids. Tagging them @lifecycle instead was rejected: it would silently drop them from nightly.yml's and e2e-selfhosted.yml's full-suite runs, neither of which sets E2E_INCLUDE_LIFECYCLE. Signed-off-by: Jussi Elo --- .github/workflows/ci.yml | 13 ++++++ docs/testing.md | 20 ++++++++ tests/e2e-cucumber/README.md | 2 +- tests/e2e-cucumber/features/comfyui.feature | 7 +++ tests/e2e-cucumber/src/expectation.rs | 51 +++++++++++++++++++++ tests/e2e-cucumber/tests/e2e.rs | 17 ++++++- xtask/src/workflow_contract.rs | 35 ++++++++++++++ 7 files changed, 142 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bfeb860c0..e2fd0c4c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -790,9 +790,22 @@ jobs: # of which touch a seam — and this lane packages and installs the binary # through the real installer, so it should ship exactly what a release # ships rather than a build carrying test hooks. + # + # `E2E_ALSO_RUN_IDS` folds the comfyui-01/02/03 scenarios into this + # otherwise-lifecycle-only selection (#689). PR #637 made them hermetic + # (no GPU, no network, no `e2e-test-hooks` dependency — their `uv`/Python + # subprocesses are planted fixture binaries) and Windows-portable, but + # this required lane was the only place that guarantee could actually be + # enforced: the self-hosted Windows lane that otherwise runs them + # (e2e-selfhosted.yml) is optional and not a required check. They are not + # tagged `@lifecycle` themselves — they are not release packaging/ + # install/uninstall scenarios, and tagging them so would drop them out of + # nightly.yml's and e2e-selfhosted.yml's full-suite runs, neither of + # which sets `E2E_INCLUDE_LIFECYCLE`. env: E2E_INCLUDE_LIFECYCLE: "1" E2E_ONLY_LIFECYCLE: "1" + E2E_ALSO_RUN_IDS: "comfyui-uv-install-failure-names-log,comfyui-uv-install-progress-streamed,comfyui-ambiguous-runtime-actionable" run: | # Derive from CARGO_TARGET_DIR when it is set, falling back to the # default target dir, so a future job-level CARGO_TARGET_DIR cannot diff --git a/docs/testing.md b/docs/testing.md index 1a241280c..51c6f8c04 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1532,6 +1532,26 @@ The harness applies the matching OS requirement itself; do not select these with cucumber's `--tags`/`-n` flags, because those bypass the suite's custom expectation and applicability resolver. +The required `windows-build-and-test` lane additionally folds in the hermetic +`comfyui-01`/`comfyui-02`/`comfyui-03` scenarios (not themselves `@lifecycle`) +via `E2E_ALSO_RUN_IDS`, a comma-separated list of `@id`s: + +```powershell +$env:E2E_INCLUDE_LIFECYCLE = "1" +$env:E2E_ONLY_LIFECYCLE = "1" +$env:E2E_ALSO_RUN_IDS = "comfyui-uv-install-failure-names-log,comfyui-uv-install-progress-streamed,comfyui-ambiguous-runtime-actionable" +cargo xtask e2e +``` + +These three were made hermetic and Windows-portable without being tagged +`@lifecycle` — they're ComfyUI install scenarios, not release packaging/ +install/uninstall ones, and tagging them `@lifecycle` would drop them out of +the full-suite runs in `nightly.yml` and `e2e-selfhosted.yml`, neither of +which sets `E2E_INCLUDE_LIFECYCLE`. `E2E_ALSO_RUN_IDS` selects them into this +lane instead, so this is the one required lane that guarantees Windows +coverage for them; the self-hosted Windows lane that otherwise runs them is +optional. + Heavy CI runs the matching set per platform. The scenarios reject untrusted signers, bad checksums, bad detached signatures, and missing required signature sidecars before activation; cover generated private-key PEM packaging and diff --git a/tests/e2e-cucumber/README.md b/tests/e2e-cucumber/README.md index dfadad44f..809f7cca9 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -145,7 +145,7 @@ Scenarios carry stable-id and capability tags: | `@requires-os:` | Premise is OS-specific; skip on other OSes. | | `@serve-timeout:` | Lengthen the serve-readiness wait for a genuinely slow serve (e.g. a large model). | | `@nightly` | Expensive scenario skipped by default; included when `E2E_INCLUDE_NIGHTLY=1`. | -| `@lifecycle` | Expensive, OS-mutating release-lifecycle scenario (packaging + real installer + install/uninstall). Skipped by default; included when `E2E_INCLUDE_LIFECYCLE=1`. `E2E_ONLY_LIFECYCLE=1` selects only this set without bypassing expectation resolution. | +| `@lifecycle` | Expensive, OS-mutating release-lifecycle scenario (packaging + real installer + install/uninstall). Skipped by default; included when `E2E_INCLUDE_LIFECYCLE=1`. `E2E_ONLY_LIFECYCLE=1` selects only this set without bypassing expectation resolution. A caller can additionally fold specific non-`@lifecycle` scenarios into that restricted set via `E2E_ALSO_RUN_IDS` (comma-separated `@id`s), without tagging them `@lifecycle` — which would also pull them out of the full-suite lanes that don't set `E2E_INCLUDE_LIFECYCLE`. `ci.yml`'s Windows lane uses this to fold in the hermetic comfyui-01/02/03 scenarios. | Known bugs are **not** tagged in the `.feature` files — they live in `expectations.toml`, keyed by `@id`, each with a `when = { ... }` condition (e.g. diff --git a/tests/e2e-cucumber/features/comfyui.feature b/tests/e2e-cucumber/features/comfyui.feature index 6fad64192..e26c3f0d3 100644 --- a/tests/e2e-cucumber/features/comfyui.feature +++ b/tests/e2e-cucumber/features/comfyui.feature @@ -11,6 +11,13 @@ Feature: ComfyUI install reports progress and makes failures actionable # The runtime, its rocm_sdk probe, the Python interpreter and `uv` itself are # all planted so the scenario is hermetic: no GPU, no network, no real # package resolution. + # + # This scenario and comfyui-02/-03 below are hermetic and Windows-portable + # (no `@requires-os:linux`), but nothing in required CI ran them on Windows + # until the required `windows-build-and-test` lane started selecting their + # ids explicitly via `E2E_ALSO_RUN_IDS` (see `docs/testing.md`'s "Install + # lifecycle" section) — before that, Windows coverage depended entirely on + # the optional, self-hosted `e2e-selfhosted.yml` lane. @id:comfyui-uv-install-failure-names-log Scenario: comfyui-01 - A failed dependency install names the log it wrote Given a ready ROCm runtime with a ComfyUI checkout pending dependencies diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 2ffe4e1ed..8d3c75171 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -625,6 +625,17 @@ pub fn resolve( Expectation::ExpectPass } +/// Whether a scenario runs in an `E2E_ONLY_LIFECYCLE=1` lane. +/// +/// True for a scenario that is itself `@lifecycle`, or whose `@id` is one of +/// `extra_ids` — scenarios a caller has explicitly opted into this lane +/// without tagging them `@lifecycle` (which would also pull them out of +/// `nightly.yml` and `e2e-selfhosted.yml`, neither of which restricts to the +/// lifecycle set). +pub fn lifecycle_lane_includes(decl: &ScenarioDecl, extra_ids: &[String]) -> bool { + decl.lifecycle || decl.id.as_deref().is_some_and(|id| extra_ids.iter().any(|e| e == id)) +} + /// Tiny glob: `*` matches any run of chars. Used only for `therock_family`. fn glob_match(pattern: &str, text: &str) -> bool { let pattern = pattern.to_ascii_lowercase(); @@ -947,6 +958,46 @@ serve_timeout_secs = 90 )); } + #[test] + fn lifecycle_lane_includes_lifecycle_scenarios_regardless_of_extra_ids() { + let d = decl(&["id:lifecycle-linux-install", "lifecycle"]); + assert!(lifecycle_lane_includes(&d, &[])); + assert!(lifecycle_lane_includes( + &d, + &["some-other-id".to_owned()] + )); + } + + #[test] + fn lifecycle_lane_includes_a_non_lifecycle_scenario_named_in_extra_ids() { + let d = decl(&["id:comfyui-uv-install-failure-names-log"]); + assert!(!d.lifecycle); + assert!(lifecycle_lane_includes( + &d, + &["comfyui-uv-install-failure-names-log".to_owned()] + )); + } + + #[test] + fn lifecycle_lane_excludes_a_non_lifecycle_scenario_not_named_in_extra_ids() { + let d = decl(&["id:comfyui-uv-install-failure-names-log"]); + assert!(!lifecycle_lane_includes( + &d, + &["some-other-id".to_owned()] + )); + assert!(!lifecycle_lane_includes(&d, &[])); + } + + #[test] + fn lifecycle_lane_excludes_a_scenario_with_no_id_even_with_extra_ids_set() { + let d = decl(&[]); + assert!(d.id.is_none()); + assert!(!lifecycle_lane_includes( + &d, + &["comfyui-uv-install-failure-names-log".to_owned()] + )); + } + #[test] fn docker_scenario_skips_unless_included() { // A container-backed scenario is opt-in, not merely "docker is here". diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 987eef8cd..263fa807a 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -1171,7 +1171,7 @@ async fn main() { use cucumber::writer::{self, Stats as _}; use e2e_cucumber::capability::host_capability; - use e2e_cucumber::expectation::{Expectation, ScenarioDecl, resolve}; + use e2e_cucumber::expectation::{Expectation, ScenarioDecl, lifecycle_lane_includes, resolve}; use e2e_cucumber::monotonic_clock::MonotonicClockWriter; let dir = results_dir(); @@ -1210,6 +1210,19 @@ async fn main() { // either its CLI filter OR this closure, so CLI selection would bypass OS, // nightly/lifecycle, ID, and expectation resolution entirely. let only_lifecycle = std::env::var_os("E2E_ONLY_LIFECYCLE").is_some_and(|v| v == "1"); + // A `E2E_ONLY_LIFECYCLE` lane can fold in specific non-`@lifecycle` + // scenarios by `@id` (comma-separated) instead of running the full suite — + // e.g. the Windows lane adding the hermetic comfyui scenarios so they get + // required-CI coverage there without being tagged `@lifecycle`, which would + // also pull them out of `nightly.yml` and `e2e-selfhosted.yml` (neither sets + // `E2E_INCLUDE_LIFECYCLE`, so a `@lifecycle` tag would drop them from both). + let also_run_ids: Vec = std::env::var("E2E_ALSO_RUN_IDS") + .unwrap_or_default() + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_owned) + .collect(); // Heavy `@merge-queue` serves run only in the merge queue (a cheaper // per-engine canary covers them on the PR fast path); set by // e2e-selfhosted.yml on the `merge_group` event. @@ -1328,7 +1341,7 @@ async fn main() { merge_queue: include_merge_queue, }, ); - let run = (!only_lifecycle || decl.lifecycle) + let run = (!only_lifecycle || lifecycle_lane_includes(&decl, &also_run_ids)) && !matches!(expectation, Expectation::Skip { .. }); if let Some(id) = &decl.id { let engine = decl.effective_engine(cap).to_owned(); diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 4962284e7..6ac572391 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -3919,6 +3919,41 @@ fi ); } + /// The hermetic comfyui-01/02/03 scenarios (no GPU, no network, no + /// `e2e-test-hooks` dependency, Windows-portable since #637) must stay + /// explicitly selected into this otherwise-`@lifecycle`-only Windows lane + /// (#689) — it is required CI, unlike the self-hosted lane that otherwise + /// runs them. This checks the whole step (not just the `run: |` body, since + /// the selection lives in `env:`), so dropping the env var — or narrowing + /// its id list — fails here rather than silently reopening the coverage gap. + #[test] + fn ci_windows_lifecycle_lane_also_selects_the_hermetic_comfyui_scenarios() { + let ci = read_workflow("ci.yml"); + let windows = job_block(&ci, "windows-build-and-test"); + let lines: Vec<&str> = windows.lines().collect(); + let at = lines + .iter() + .position(|line| line.trim_start() == "- name: Acceptance install lifecycle") + .expect("windows-build-and-test must have an Acceptance install lifecycle step"); + let step = step_block(&lines, at); + + assert!( + step.contains("E2E_ALSO_RUN_IDS"), + "the Windows lifecycle lane must additionally select the hermetic \ + comfyui scenarios so they get required-CI Windows coverage (#689):\n{step}" + ); + for id in [ + "comfyui-uv-install-failure-names-log", + "comfyui-uv-install-progress-streamed", + "comfyui-ambiguous-runtime-actionable", + ] { + assert!( + step.contains(id), + "the Windows lifecycle lane must select {id} via E2E_ALSO_RUN_IDS:\n{step}" + ); + } + } + /// Extract the version token right after `marker` in `line`, up to the /// next whitespace (trailing sentence punctuation is trimmed too, so a /// doc sentence ending right after the version with no space, e.g. From bbb390290ed463dbdc8b15367b893d304961aac7 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 07:52:45 +0000 Subject: [PATCH 2/5] fmt: satisfy cargo fmt for lifecycle_lane_includes prek's cargo-fmt hook failed CI; rustfmt wraps the condition and two test call sites differently than written. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/src/expectation.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 8d3c75171..633ba5fac 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -633,7 +633,11 @@ pub fn resolve( /// `nightly.yml` and `e2e-selfhosted.yml`, neither of which restricts to the /// lifecycle set). pub fn lifecycle_lane_includes(decl: &ScenarioDecl, extra_ids: &[String]) -> bool { - decl.lifecycle || decl.id.as_deref().is_some_and(|id| extra_ids.iter().any(|e| e == id)) + decl.lifecycle + || decl + .id + .as_deref() + .is_some_and(|id| extra_ids.iter().any(|e| e == id)) } /// Tiny glob: `*` matches any run of chars. Used only for `therock_family`. @@ -962,10 +966,7 @@ serve_timeout_secs = 90 fn lifecycle_lane_includes_lifecycle_scenarios_regardless_of_extra_ids() { let d = decl(&["id:lifecycle-linux-install", "lifecycle"]); assert!(lifecycle_lane_includes(&d, &[])); - assert!(lifecycle_lane_includes( - &d, - &["some-other-id".to_owned()] - )); + assert!(lifecycle_lane_includes(&d, &["some-other-id".to_owned()])); } #[test] @@ -981,10 +982,7 @@ serve_timeout_secs = 90 #[test] fn lifecycle_lane_excludes_a_non_lifecycle_scenario_not_named_in_extra_ids() { let d = decl(&["id:comfyui-uv-install-failure-names-log"]); - assert!(!lifecycle_lane_includes( - &d, - &["some-other-id".to_owned()] - )); + assert!(!lifecycle_lane_includes(&d, &["some-other-id".to_owned()])); assert!(!lifecycle_lane_includes(&d, &[])); } From 8f0e37e401c93ded1a8516f584af2577e1ef014f Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 08:08:08 +0000 Subject: [PATCH 3/5] test(therock): hold the env lock in the SDK archive property tests sdk_tarball_unpack_never_writes_outside_the_install_root and sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root extract through extract_tarball_and_discard_archive, which spawns bare-name tar via PATH, but did not hold PROCESS_ENV_TEST_LOCK. A test that temporarily narrows PATH (line ~9320) could make a concurrent run of either property test fail to launch tar with ENOENT -- intermittently, and only in a full run, which also made the pre-push cargo test hook fail on unrelated changes (the same failure mode PR #762 fixed for spawn_test_sleep). Widen PROCESS_ENV_TEST_LOCK to pub(super) so archive_properties, a sibling module to tests, can take it, and hold it for the whole of each property test's run. Signed-off-by: Jussi Elo --- apps/rocm/src/therock.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 5918af4e5..5b7143e81 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -7262,7 +7262,11 @@ mod tests { // they run. Because env is shared across all test threads, any test that spawns a // bare-name binary (e.g. `tar`) via `PATH` lookup must also hold this lock, or it // can fail with ENOENT while another test has temporarily narrowed `PATH`. - static PROCESS_ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + // + // `pub(super)` rather than private: `archive_properties` below is a sibling + // module (not a descendant of `tests`), and its property tests extract through + // `extract_tarball_and_discard_archive`, which spawns the same bare-name `tar`. + pub(super) static PROCESS_ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); fn family_for_test(family: &str, raw_arch: Option<&str>) -> FamilyResolution { FamilyResolution { @@ -12425,6 +12429,7 @@ exit 1 #[cfg(all(test, unix))] mod archive_properties { use super::extract_tarball_and_discard_archive; + use super::tests::PROCESS_ENV_TEST_LOCK; use crate::archive_props::{ Expect, Modes, entries, note_mode_scope, run_case, run_property, run_round_trip, tar_gz_bytes, @@ -12435,6 +12440,13 @@ mod archive_properties { /// judged when this runs as root (see `Modes`). #[test] fn sdk_tarball_unpack_never_writes_outside_the_install_root() { + // Every generated case below extracts through `extract_tarball_and_ + // discard_archive`, which spawns bare-name `tar` via `PATH`; hold the + // lock for the whole property run so a concurrent test that narrows + // `PATH` cannot make any case fail to launch. + let _guard = PROCESS_ENV_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let expect = Expect { modes: Modes::SafeUnlessRoot, no_outward_links: false, @@ -12461,6 +12473,10 @@ mod archive_properties { /// entirely escapes the oracle above; this catches it. #[test] fn sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root() { + // Same bare-name `tar` spawn as above; same reason to hold the lock. + let _guard = PROCESS_ENV_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); run_round_trip("therock-benign", |layout, entries| { let archive = layout.base.join("sdk.tar.gz"); std::fs::write(&archive, tar_gz_bytes(entries))?; From a47c944999f9fabf47947032c6194cf0f23b1d5d Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 11:30:35 +0000 Subject: [PATCH 4/5] test(e2e-cucumber): fail loudly on an unmatched E2E_ALSO_RUN_IDS entry An id listed in E2E_ALSO_RUN_IDS that matches no scenario's @id was silently ignored, so a renamed comfyui @id would quietly drop the Windows lifecycle lane's coverage while CI stayed green. Check every listed id against the ids actually seen in the suite once the run completes, and panic naming whichever didn't match. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/src/expectation.rs | 36 +++++++++++++++++++++++++++ tests/e2e-cucumber/tests/e2e.rs | 19 +++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 633ba5fac..71c1c93bc 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -640,6 +640,19 @@ pub fn lifecycle_lane_includes(decl: &ScenarioDecl, extra_ids: &[String]) -> boo .is_some_and(|id| extra_ids.iter().any(|e| e == id)) } +/// `extra_ids` entries that match no scenario's `@id` in `known_ids`. +/// +/// `lifecycle_lane_includes` folds an unmatched id in silently — a renamed or +/// typo'd `@id:` tag would otherwise drop a scenario out of the lane with CI +/// staying green. The caller should treat a non-empty result as fatal. +pub fn unmatched_also_run_ids<'a>(extra_ids: &'a [String], known_ids: &[String]) -> Vec<&'a str> { + extra_ids + .iter() + .filter(|id| !known_ids.contains(*id)) + .map(String::as_str) + .collect() +} + /// Tiny glob: `*` matches any run of chars. Used only for `therock_family`. fn glob_match(pattern: &str, text: &str) -> bool { let pattern = pattern.to_ascii_lowercase(); @@ -996,6 +1009,29 @@ serve_timeout_secs = 90 )); } + #[test] + fn unmatched_also_run_ids_is_empty_when_every_extra_id_matches_a_known_id() { + let known = vec!["comfyui-01".to_owned(), "comfyui-02".to_owned()]; + let extra = vec!["comfyui-01".to_owned()]; + assert!(unmatched_also_run_ids(&extra, &known).is_empty()); + } + + #[test] + fn unmatched_also_run_ids_names_an_extra_id_no_scenario_carries() { + let known = vec!["comfyui-01".to_owned()]; + let extra = vec!["comfyui-01".to_owned(), "comfyui-02-renamed".to_owned()]; + assert_eq!( + unmatched_also_run_ids(&extra, &known), + vec!["comfyui-02-renamed"] + ); + } + + #[test] + fn unmatched_also_run_ids_is_empty_when_extra_ids_is_empty() { + let known = vec!["comfyui-01".to_owned()]; + assert!(unmatched_also_run_ids(&[], &known).is_empty()); + } + #[test] fn docker_scenario_skips_unless_included() { // A container-backed scenario is opt-in, not merely "docker is here". diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 263fa807a..18c82bbed 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -1171,7 +1171,9 @@ async fn main() { use cucumber::writer::{self, Stats as _}; use e2e_cucumber::capability::host_capability; - use e2e_cucumber::expectation::{Expectation, ScenarioDecl, lifecycle_lane_includes, resolve}; + use e2e_cucumber::expectation::{ + Expectation, ScenarioDecl, lifecycle_lane_includes, resolve, unmatched_also_run_ids, + }; use e2e_cucumber::monotonic_clock::MonotonicClockWriter; let dir = results_dir(); @@ -1328,6 +1330,10 @@ async fn main() { // host — e.g. a required engine can't start) are filtered out and never // run; their resolution is still recorded so platform.json can show N/A. .filter_run(concat!(env!("CARGO_MANIFEST_DIR"), "/features/"), { + // Cloned so the outer `also_run_ids` survives this `move` closure — + // it is checked against every recorded scenario id after the run, + // to catch a listed id that matched nothing. + let also_run_ids = also_run_ids.clone(); move |feature, _rule, scenario| { let decl = ScenarioDecl::from_tags(&scenario.tags); let expectation = resolve( @@ -1392,6 +1398,17 @@ async fn main() { .expect("failed to read scenario results"); let resolutions = resolutions.lock().expect("resolutions poisoned"); + // Every scenario's id was recorded above regardless of whether it ran, so + // an `E2E_ALSO_RUN_IDS` entry missing from `resolutions` means no scenario + // in the suite carries that `@id` — a rename or typo that would otherwise + // silently drop the scenario from this lane while CI stayed green. + let known_ids: Vec = resolutions.keys().cloned().collect(); + let unmatched = unmatched_also_run_ids(&also_run_ids, &known_ids); + assert!( + unmatched.is_empty(), + "E2E_ALSO_RUN_IDS listed id(s) that no scenario carries (rename or typo?): {unmatched:?}" + ); + // Collect component versions (OS/ROCm/vLLM/lemonade) for the report heading. // OS is always collected (every platform, incl. mock); ROCm/vLLM/lemonade come // from the shared runtimes tree CI provides (E2E_SHARED_RUNTIMES_DIR) — absent From 54e522e8e0a1b0f6fb822399c93f113d27321c72 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 11:33:41 +0000 Subject: [PATCH 5/5] test(xtask): assert the real E2E_ALSO_RUN_IDS env line, not a comment The Windows lifecycle lane's contract test checked step.contains the bare var name, which the explanatory comment above env: also satisfies. Use flattened_values (comments stripped) so only the real env: entry can pass, and assert the three comfyui ids against its value rather than the whole step text. Signed-off-by: Jussi Elo --- xtask/src/workflow_contract.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 6ac572391..6fe45c31f 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -3937,10 +3937,17 @@ fi .expect("windows-build-and-test must have an Acceptance install lifecycle step"); let step = step_block(&lines, at); - assert!( - step.contains("E2E_ALSO_RUN_IDS"), - "the Windows lifecycle lane must additionally select the hermetic \ - comfyui scenarios so they get required-CI Windows coverage (#689):\n{step}" + // `flattened_values` strips comments before matching the key, so a + // mention of `E2E_ALSO_RUN_IDS` in the explanatory comment above the + // `env:` block (there to say why these ids are selected here) cannot + // satisfy this on its own — only the real `env:` entry can. + let also_run_ids = flattened_values(&step, "E2E_ALSO_RUN_IDS"); + assert_eq!( + also_run_ids.len(), + 1, + "the Windows lifecycle lane must set env.E2E_ALSO_RUN_IDS exactly \ + once to additionally select the hermetic comfyui scenarios, so \ + they get required-CI Windows coverage (#689):\n{step}" ); for id in [ "comfyui-uv-install-failure-names-log", @@ -3948,8 +3955,8 @@ fi "comfyui-ambiguous-runtime-actionable", ] { assert!( - step.contains(id), - "the Windows lifecycle lane must select {id} via E2E_ALSO_RUN_IDS:\n{step}" + also_run_ids[0].contains(id), + "the Windows lifecycle lane must select {id} via env.E2E_ALSO_RUN_IDS:\n{step}" ); } }