Skip to content

Conversation

@damanm24
Copy link
Contributor

The build_requirements function did not take into account all the possible combinations for a VmmBackend to be configured. This allowed for some discrepancy when generating the test case host capabilities requirements depending on how the vmm backend was being configured. This PR refactors the make_vmm_test so that resolving the vmm backend happens earlier in the function, and the resolved value is the one that is used throughout.

@damanm24 damanm24 requested a review from a team as a code owner October 20, 2025 19:30
Copilot AI review requested due to automatic review settings October 20, 2025 19:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the VMM test macro where build requirements were not correctly accounting for all possible VMM backend configurations. The fix resolves the VMM backend (HyperV or OpenVMM) earlier in the process and uses that resolved value consistently throughout the test generation.

Key Changes

  • VMM backend resolution now happens at the start of the config loop, combining specific_vmm and config.vmm into a single resolved_vmm value
  • The build_requirements function signature changed to accept the resolved VMM backend directly instead of the entire config
  • Duplicate VMM resolution logic in the match statement for cfg_conditions was removed

@github-actions
Copy link

for config in args.configs {
let name = format!("{}_{original_name}", config.name_prefix(specific_vmm));
// Resolve the VMM backend early by combining specific_vmm and config.vmm
let resolved_vmm = match (specific_vmm, config.vmm) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spitballing here, feel free to say no if you like the match better

if specific_vmm == config.vmm || specific_vmm.is_some() != config.vmm.is_some() {
    specific_vmm.or(config.vmm).ok_or(Error::new(config.span, "vmm must be specified"))?
}
else {
    return Err(Error::new(config.span, "vmm mismatch"))
}

@damanm24 damanm24 merged commit 78d45a3 into microsoft:main Oct 21, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants