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/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))?; 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..71c1c93bc 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -625,6 +625,34 @@ 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)) +} + +/// `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(); @@ -947,6 +975,63 @@ 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 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 987eef8cd..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, 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(); @@ -1210,6 +1212,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. @@ -1315,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( @@ -1328,7 +1347,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(); @@ -1379,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 diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 4962284e7..6fe45c31f 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -3919,6 +3919,48 @@ 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); + + // `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", + "comfyui-uv-install-progress-streamed", + "comfyui-ambiguous-runtime-actionable", + ] { + assert!( + also_run_ids[0].contains(id), + "the Windows lifecycle lane must select {id} via env.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.