[WIP] flowey: add new vmm-tests-run command #3167
[WIP] flowey: add new vmm-tests-run command #3167chris-oo wants to merge 12 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an end-to-end local workflow for running vmm_tests via Flowey by introducing artifact discovery (from Petri test metadata), mapping discovered artifacts to build/download selections, and a convenience vmm-tests-run command that stitches discovery + run together.
Changes:
- Add
petri --list-required-artifactsJSON output (optionally scoped to specific tests via stdin) and Flowey jobs/pipelines to discover artifacts from a nextest filter. - Introduce
artifact_to_build_mappingand CI validation to ensure all discovered Petri artifact IDs map to Flowey build/download selections. - Add
cargo xflowey vmm-tests-rundocumentation and CLI plumbing for--artifacts-file-driven dependency minimization.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| petri/src/test.rs | Emit machine-readable JSON of required/optional artifacts; add --tests-from-stdin for per-test querying. |
| Guide/src/dev_guide/tests/vmm.md | Update VMM test-running docs to prefer vmm-tests-run and describe targeting/cross-compilation. |
| Guide/src/dev_guide/dev_tools/xflowey.md | Document vmm-tests-run as a notable cargo xflowey pipeline. |
| flowey/flowey_lib_hvlite/src/lib.rs | Export the new artifact mapping module. |
| flowey/flowey_lib_hvlite/src/install_vmm_tests_deps.rs | Add AutoInstall request and allow skipping admin-only checks when not auto-installing. |
| flowey/flowey_lib_hvlite/src/download_openvmm_vmm_tests_artifacts.rs | Improve local download prompt behavior (terminal detection + timeout + explicit cancel). |
| flowey/flowey_lib_hvlite/src/artifact_to_build_mapping.rs | New mapping from Petri artifact ID strings to Flowey build selections + downloads (with unit tests). |
| flowey/flowey_lib_hvlite/src/_jobs/test_artifact_mapping_completeness.rs | New CI job to discover artifacts and fail if any are unmapped. |
| flowey/flowey_lib_hvlite/src/_jobs/mod.rs | Register new jobs (local_discover_vmm_tests_artifacts, test_artifact_mapping_completeness). |
| flowey/flowey_lib_hvlite/src/_jobs/local_discover_vmm_tests_artifacts.rs | New job that resolves nextest filters to test names and queries Petri for artifact JSON. |
| flowey/flowey_lib_hvlite/src/_jobs/local_build_and_run_nextest_vmm_tests.rs | Wire in needs_release_igvm and conditional release-IGVM download; add BuildSelections::none(). |
| flowey/flowey_lib_hvlite/src/_jobs/cfg_common.rs | Ensure install_vmm_tests_deps receives AutoInstall config. |
| flowey/flowey_lib_hvlite/Cargo.toml | Add crossterm dependency (events + Windows support). |
| flowey/flowey_hvlite/src/pipelines/vmm_tests.rs | Add --artifacts-file mode and resolve build/download selections from discovered artifact JSON. |
| flowey/flowey_hvlite/src/pipelines/vmm_tests_run.rs | New meta-command that runs discover then tests in sequence. |
| flowey/flowey_hvlite/src/pipelines/vmm_tests_discover.rs | New local-only pipeline to discover artifacts into a JSON file/stdout. |
| flowey/flowey_hvlite/src/pipelines/mod.rs | Register new subcommands; implement VmmTestsRun by executing directly (process exit). |
| flowey/flowey_hvlite/src/pipelines/checkin_gates.rs | Add GitHub CI job to validate artifact mapping completeness. |
| Cargo.lock | Lockfile updates for crossterm (and transitive deps). |
| .github/workflows/openvmm-pr.yaml | Regenerated workflow adding “test artifact mapping completeness” job. |
| .github/workflows/openvmm-pr-release.yaml | Regenerated workflow adding “test artifact mapping completeness” job in release CI. |
| .github/workflows/openvmm-ci.yaml | Regenerated workflow updates (bootstrap artifact uid bump + new mapping completeness job). |
| .github/copilot-instructions.md | Document cargo xflowey vmm-tests-run as the preferred dev validation path. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
flowey/flowey_lib_hvlite/src/install_vmm_tests_deps.rs:121
- This uses
assert!(features.status.success())on the result of an external command (DISM.exe). If DISM fails due to environment/state, this will panic instead of producing a recoverable error, which is undesirable for a CLI/pipeline tool. Replace the assert withanyhow::ensure!/bail!and include stderr/stdout in the error for diagnosis.
// Check if features are already enabled (requires admin, so skip if not auto_install)
if installing && auto_install && !features_to_enable.is_empty() {
let features = flowey::shell_cmd!(rt, "DISM.exe /Online /Get-Features").output()?;
assert!(features.status.success());
let features = String::from_utf8_lossy(&features.stdout).to_string();
let mut feature = None;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
flowey/flowey_lib_hvlite/src/install_vmm_tests_deps.rs:132
- The DISM parsing path uses
assert!(features.status.success())/assert_eq!on command output. These can panic at runtime if DISM fails or output format changes. Preferanyhow::ensure!/graceful error handling so the tool fails with a readable error instead of a panic.
// Check if features are already enabled (requires admin, so skip if not auto_install)
if installing && auto_install && !features_to_enable.is_empty() {
let features = flowey::shell_cmd!(rt, "DISM.exe /Online /Get-Features").output()?;
assert!(features.status.success());
let features = String::from_utf8_lossy(&features.stdout).to_string();
let mut feature = None;
for line in features.lines() {
if let Some((k, v)) = line.split_once(":") {
if let Some(f) = feature {
assert_eq!(k.trim(), "State");
match v.trim() {
"Enabled" => {
assert!(features_to_enable.remove(f));
}
"Disabled" => {}
_ => anyhow::bail!("Unknown feature enablement state"),
}
flowey/flowey_lib_hvlite/src/_jobs/local_discover_vmm_tests_artifacts.rs
Show resolved
Hide resolved
flowey/flowey_lib_hvlite/src/_jobs/local_discover_vmm_tests_artifacts.rs
Show resolved
Hide resolved
| // Validate target if present in the JSON | ||
| if let Some(ref file_target) = parsed.target { | ||
| let expected_target = format!( | ||
| "{}-{}", | ||
| match target_arch { | ||
| target_lexicon::Architecture::X86_64 => "x86_64", | ||
| target_lexicon::Architecture::Aarch64(_) => "aarch64", | ||
| _ => "unknown", | ||
| }, | ||
| match target_os { | ||
| target_lexicon::OperatingSystem::Windows => "pc-windows-msvc", | ||
| target_lexicon::OperatingSystem::Linux => "unknown-linux-gnu", | ||
| _ => "unknown", | ||
| } | ||
| ); | ||
|
|
||
| // Check if the target in the file is compatible with what we're building for | ||
| if !file_target.contains(expected_target.split('-').next().unwrap_or("")) | ||
| || (target_os == target_lexicon::OperatingSystem::Windows | ||
| && !file_target.contains("windows")) | ||
| || (target_os == target_lexicon::OperatingSystem::Linux | ||
| && !file_target.contains("linux")) | ||
| { | ||
| anyhow::bail!( | ||
| "Target mismatch: artifacts file was generated for '{}', but building for '{}'", | ||
| file_target, | ||
| expected_target | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new target validation behavior is untested. Since this is parsing user-provided JSON files and bailing on mismatches, please add unit tests that (1) pass a JSON with a mismatched target and assert it errors, and (2) pass a matching target and assert it succeeds. This will help prevent regressions if the target normalization logic changes.
| FlowBackend::Local => auto_install.ok_or_else(|| { | ||
| anyhow::anyhow!("Missing essential request: AutoInstall") | ||
| })?, |
There was a problem hiding this comment.
AutoInstall is required for FlowBackend::Local unconditionally here, even when installing is false (e.g., the caller only requests GetCommands). Since the admin-only checks are already guarded by if installing && auto_install, you can avoid failing local GetCommands runs by only requiring AutoInstall when installing is true, or by defaulting auto_install to false when it’s not provided and no privileged actions are being performed.
| FlowBackend::Local => auto_install.ok_or_else(|| { | |
| anyhow::anyhow!("Missing essential request: AutoInstall") | |
| })?, | |
| FlowBackend::Local => { | |
| if installing { | |
| auto_install.ok_or_else(|| { | |
| anyhow::anyhow!("Missing essential request: AutoInstall") | |
| })? | |
| } else { | |
| // When not installing (e.g., only GetCommands), default to | |
| // false if AutoInstall was not explicitly provided. | |
| auto_install.unwrap_or(false) | |
| } | |
| } |
| OpenvmmPipelines::VmmTestsRun(cmd) => { | ||
| // VmmTestsRun is a meta-command that runs discover + tests sequentially | ||
| // It doesn't return a pipeline, it executes directly | ||
| let result = cmd.run(); | ||
| match result { | ||
| Ok(()) => std::process::exit(0), | ||
| Err(e) => { | ||
| log::error!("{:?}", e); | ||
| std::process::exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementing VmmTestsRun by executing subprocesses and calling std::process::exit inside IntoPipeline breaks the expectation that this path constructs a pipeline. This can cause surprising behavior when generating pipelines/YAML for non-local backends (the command would run immediately instead of producing a pipeline), and it ignores backend_hint. Prefer making VmmTestsRun implement IntoPipeline (even if local-only) and bail! for non-local backends, or keep it out of the IntoPipeline/pipeline-subcommand surface entirely and expose it as a separate local-only entry point.
| OpenvmmPipelines::VmmTestsRun(cmd) => { | |
| // VmmTestsRun is a meta-command that runs discover + tests sequentially | |
| // It doesn't return a pipeline, it executes directly | |
| let result = cmd.run(); | |
| match result { | |
| Ok(()) => std::process::exit(0), | |
| Err(e) => { | |
| log::error!("{:?}", e); | |
| std::process::exit(1); | |
| } | |
| } | |
| OpenvmmPipelines::VmmTestsRun(_cmd) => { | |
| // VmmTestsRun is a meta-command that runs discover + tests sequentially. | |
| // It is not suitable for constructing a declarative pipeline; use the | |
| // dedicated local-only entry point instead of the pipeline interface. | |
| anyhow::bail!( | |
| "VmmTestsRun cannot be used to construct a pipeline; \ | |
| invoke the local vmm-tests-run command directly instead" | |
| ); |
| let mut binary_path = None; | ||
|
|
||
| // Navigate to rust-suites -> vmm_tests::tests -> testcases | ||
| if let Some(vmm_tests) = json | ||
| .get("rust-suites") | ||
| .and_then(|s| s.get("vmm_tests::tests")) | ||
| { | ||
| // Get the binary path | ||
| if let Some(path) = vmm_tests.get("binary-path").and_then(|v| v.as_str()) { | ||
| binary_path = Some(PathBuf::from(path)); | ||
| } | ||
|
|
||
| if let Some(testcases_obj) = vmm_tests.get("testcases").and_then(|t| t.as_object()) { | ||
| for (test_name, test_info) in testcases_obj { | ||
| // Check if filter-match.status == "matches" | ||
| let matches = test_info | ||
| .get("filter-match") | ||
| .and_then(|fm| fm.get("status")) | ||
| .and_then(|s| s.as_str()) | ||
| == Some("matches"); | ||
|
|
||
| if matches { | ||
| test_names.push(test_name.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let binary_path = binary_path | ||
| .ok_or_else(|| anyhow::anyhow!("Could not find test binary path in nextest output"))?; |
There was a problem hiding this comment.
parse_nextest_output is hard-coded to the rust-suites.vmm_tests::tests entry. The vmm_tests package contains multiple test binaries/suites (e.g. vmm_tests::tmks under vmm_tests/vmm_tests/tests/tmks.rs), so filters that match tests in other suites will be silently ignored and artifact discovery will be incomplete. Consider iterating over all entries in rust-suites (or otherwise discovering the relevant suite(s) dynamically), and if multiple binaries match, run --list-required-artifacts per binary and merge the results.
| let mut binary_path = None; | |
| // Navigate to rust-suites -> vmm_tests::tests -> testcases | |
| if let Some(vmm_tests) = json | |
| .get("rust-suites") | |
| .and_then(|s| s.get("vmm_tests::tests")) | |
| { | |
| // Get the binary path | |
| if let Some(path) = vmm_tests.get("binary-path").and_then(|v| v.as_str()) { | |
| binary_path = Some(PathBuf::from(path)); | |
| } | |
| if let Some(testcases_obj) = vmm_tests.get("testcases").and_then(|t| t.as_object()) { | |
| for (test_name, test_info) in testcases_obj { | |
| // Check if filter-match.status == "matches" | |
| let matches = test_info | |
| .get("filter-match") | |
| .and_then(|fm| fm.get("status")) | |
| .and_then(|s| s.as_str()) | |
| == Some("matches"); | |
| if matches { | |
| test_names.push(test_name.clone()); | |
| } | |
| } | |
| } | |
| } | |
| let binary_path = binary_path | |
| .ok_or_else(|| anyhow::anyhow!("Could not find test binary path in nextest output"))?; | |
| let mut binary_path: Option<PathBuf> = None; | |
| // Navigate to rust-suites and iterate all vmm_tests::* suites. | |
| if let Some(rust_suites) = json.get("rust-suites").and_then(|s| s.as_object()) { | |
| for (suite_name, suite_val) in rust_suites { | |
| // We only care about suites from the vmm_tests package. | |
| if !suite_name.starts_with("vmm_tests::") { | |
| continue; | |
| } | |
| // Get the binary path for this suite. | |
| if let Some(path_str) = suite_val.get("binary-path").and_then(|v| v.as_str()) { | |
| let path = PathBuf::from(path_str); | |
| match &binary_path { | |
| None => { | |
| binary_path = Some(path); | |
| } | |
| Some(existing) => { | |
| if existing != &path { | |
| return Err(anyhow::anyhow!( | |
| "multiple vmm_tests binaries matched in nextest output: {:?} and {:?}", | |
| existing, | |
| path | |
| )); | |
| } | |
| } | |
| } | |
| } | |
| if let Some(testcases_obj) = suite_val.get("testcases").and_then(|t| t.as_object()) { | |
| for (test_name, test_info) in testcases_obj { | |
| // Check if filter-match.status == "matches" | |
| let matches = test_info | |
| .get("filter-match") | |
| .and_then(|fm| fm.get("status")) | |
| .and_then(|s| s.as_str()) | |
| == Some("matches"); | |
| if matches { | |
| test_names.push(test_name.clone()); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| let binary_path = binary_path.ok_or_else(|| { | |
| anyhow::anyhow!("Could not find vmm_tests test binary path in nextest output") | |
| })?; |
|
|
||
| - `cargo xflowey build-igvm` - primarily dev-tool used to build OpenHCL IGVM files locally | ||
| - `cargo xflowey restore-packages` - restores external packages needed to compile and run OpenVMM / OpenHCL | ||
| - `cargo xflowey vmm-tests-run` - build and run VMM tests with automatic artifact discovery. Use `--filter "test(name)"` to run specific tests |
There was a problem hiding this comment.
This bullet uses escaped quotes inside inline code (--filter \"test(name)\"), which will render the backslashes literally in Markdown. It also exceeds the Guide style guide’s 80-column wrap. Suggest updating to --filter "test(name)" (without backslashes in the source) and wrapping the sentence onto a continuation line.
| - `cargo xflowey vmm-tests-run` - build and run VMM tests with automatic artifact discovery. Use `--filter "test(name)"` to run specific tests | |
| - `cargo xflowey vmm-tests-run` - build and run VMM tests with automatic | |
| artifact discovery. Use `--filter "test(name)"` to run specific tests |
|
Could we not expose the |
we definitely should be able to do this, what issues were you hitting? |
Add a new
vmm-tests-runthat accepts a nextest filter string, and only downloads the artifacts & resources needed for the tests identified before running the specified vmm test. This itself is a meta command that runs two other flowey commands, the existingvmm-testswhich is used as a lower level command and avmm-tests-discovercommand to create the list of artifacts needed by petri. This is due to the fact that we cannot dynamically create a pipeline from within a pipeline itself, so we need some kind of meta command in order to use the output from the artifact discovery for the actualvmm-testscommand.For now, there is a large table mapping petri artifacts to strings emitted by the petri binary. Enforce that this mapping is up to date by adding new CI passes to validate all cases are covered. This isn't ideal, but we can iterate further to at least unblock getting this basic command in.
Update the docs to note how to use this command, and note that
vmm-testsis now a lower level command not normally intended to be used.