Skip to content

Conversation

@dpaliulis-msft
Copy link
Contributor

Description:
This PR adjusts the memory validation tests to compare test results on the release build against their own set of baseline memory usage values.

Changes:

  • Added small and large GP memory validation test for release only builds
  • Added new specifier for "build_version" in memstat baseline comparison method
  • Added "release" memory usage baseline values to memstat_baseline.json

@dpaliulis-msft dpaliulis-msft requested a review from a team as a code owner October 16, 2025 23:21
Copilot AI review requested due to automatic review settings October 16, 2025 23:21
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 separates memory validation tests by build type (debug vs release) to enable more accurate baseline comparisons. The changes allow memory usage validation tests to compare against build-specific baseline values rather than using a single set of baseline values for both debug and release builds.

  • Added build version parameter to memory validation baseline comparison logic
  • Split existing memory validation tests into separate debug and release variants using conditional compilation
  • Added release-specific baseline memory usage values to the test data configuration

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
vmm_tests/vmm_tests/tests/tests/multiarch/memstat.rs Added build_version parameter to compare_to_baseline method and updated all baseline JSON lookups
vmm_tests/vmm_tests/tests/tests/multiarch.rs Split memory validation tests into debug/release variants with conditional compilation and renamed existing functions
vmm_tests/vmm_tests/test_data/memstat_baseline.json Added release baseline values alongside existing debug values in nested JSON structure

@github-actions
Copy link

@mattkur mattkur added the release-ci-required Add to a PR to trigger PR gates in release mode label Oct 17, 2025
@mattkur
Copy link
Contributor

mattkur commented Oct 17, 2025

Thanks for this, and I see where you're going here. Adding memory usage tests with release config is important and valuable!

I think it would make more sense to have your test explicitly depend upon the release-built variant of openhcl. I suspect there are a few things we'd need to think through to make that work, and maybe this is why you chose to implement the tests this way :)

  1. Our default CI does not build a release variant of the openhcl binary (I think that's what happens when someone tags a PR as release-ci-required, which I just did here). Maybe that's okay: this PR adds tests that should only run in release builds (which means the CI pipe, not per PR).
  2. We would need to explicitly declare an artifact for the release-built IGVM, and resolve it only on release builds. I see you chose the most appropriate built-in cfg() flag, but I suspect we will want to be more explicit.

Interested in what you, and also what other reviewers, think.

@github-actions
Copy link

@smalis-msft
Copy link
Contributor

I initially suggested he do things this way for exactly those reasons Matt haha. Since we only produce a release build in the CI and optional release gates, this test can only really run there. This is somewhat of an ongoing experiment to gather data and refine what we're looking for here. If we see it failing a lot in CI we'll adjust.

@mattkur
Copy link
Contributor

mattkur commented Oct 17, 2025

I initially suggested he do things this way for exactly those reasons Matt haha. Since we only produce a release build in the CI and optional release gates, this test can only really run there. This is somewhat of an ongoing experiment to gather data and refine what we're looking for here. If we see it failing a lot in CI we'll adjust.

I must be on the right track if you and I are sharing thoughts :)

I only have a few nits then, @dpaliulis-msft :

  1. This matrix is getting large, so suggest that you define a macro to get rid of much of the boilerplate.
  2. "version" is typically used to mean something akin to the contents of the code that was compiled, suggest you use something more explicit (like optimizations, config, etc.)

@smalis-msft
Copy link
Contributor

smalis-msft commented Oct 17, 2025

For your point 2, i commonly see "flavor" as well.

@smalis-msft
Copy link
Contributor

For 1 I'm not sure a macro is really that needed yet, but maybe it's worth moving the tests themselves into memstat.rs just to keep things contained.

@github-actions
Copy link

@dpaliulis-msft dpaliulis-msft merged commit 7dcaf4b into microsoft:main Oct 21, 2025
76 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-ci-required Add to a PR to trigger PR gates in release mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants