vmm_tests: use libtest-mimic to define and run tests (#762)

Instead of using the built-in test harness, use libtest-mimic to define tests. This makes tests easier to define: * We can define tests programmatically at runtime instead of just statically at compile time; this will make it possible to reduce/eliminate the use of the domain-specific language in the `vmm_test` macro to define test constraints (supported VMMs, architectures, etc.). * We can define and query artifacts requirements separately from running the tests, making it easier to integrate with flowey.
microsoft · Feb 3, 2025 · dc00c7d · dc00c7d
1 parent 72d4de3
commit dc00c7d
Show file tree

Hide file tree

Showing 21 changed files with 411 additions and 220 deletions.
diff --git a/Cargo.lock b/Cargo.lock
@@ -90,23 +90,24 @@ checksum = "4b46cbb362ab8752921c97e041f5e366ee6297bd428a31275b9fcf1e380f7299"
 
 [[package]]
 name = "anstream"
-version = "0.6.11"
+version = "0.6.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6e2e1ebcb11de5c03c67de28a7df593d32191b44939c482e97702baaaa6ab6a5"
+checksum = "64e15c1ab1f89faffbf04a634d5e1962e9074f2741eef6d97f3c4e322426d526"
 dependencies = [
  "anstyle",
  "anstyle-parse",
  "anstyle-query",
  "anstyle-wincon",
  "colorchoice",
+ "is_terminal_polyfill",
  "utf8parse",
 ]
 
 [[package]]
 name = "anstyle"
-version = "1.0.4"
+version = "1.0.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7079075b41f533b8c61d2a4d073c4676e1f8b249ff94a393b0595db304e0dd87"
+checksum = "55cc3b69f167a1ef2e161439aa98aed94e6028e5f9a59be9a6ffb47aef1651f9"
 
 [[package]]
 name = "anstyle-parse"
@@ -1501,6 +1502,12 @@ version = "3.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "281e452d3bad4005426416cdba5ccfd4f5c1280e10099e21db27f7c1c28347fc"
 
+[[package]]
+name = "escape8259"
+version = "0.5.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5692dd7b5a1978a5aeb0ce83b7655c58ca8efdcb79d21036ea249da95afec2c6"
+
 [[package]]
 name = "event-listener"
 version = "5.3.1"
@@ -3378,6 +3385,12 @@ dependencies = [
  "windows-sys 0.52.0",
 ]
 
+[[package]]
+name = "is_terminal_polyfill"
+version = "1.70.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7943c866cc5cd64cbc25b2e01621d07fa8eb2a1a23160ee81ce38704e97b8ecf"
+
 [[package]]
 name = "itertools"
 version = "0.10.5"
@@ -3498,6 +3511,18 @@ dependencies = [
  "vcpkg",
 ]
 
+[[package]]
+name = "libtest-mimic"
+version = "0.8.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5297962ef19edda4ce33aaa484386e0a5b3d7f2f4e037cbeee00503ef6b29d33"
+dependencies = [
+ "anstream",
+ "anstyle",
+ "clap",
+ "escape8259",
+]
+
 [[package]]
 name = "linkme"
 version = "0.3.31"
@@ -5083,6 +5108,7 @@ dependencies = [
  "anyhow",
  "async-trait",
  "chipset_resources",
+ "clap",
  "diag_client",
  "disk_backend_resources",
  "disk_vhd1",
@@ -5103,6 +5129,8 @@ dependencies = [
  "ide_resources",
  "image",
  "inspect",
+ "libtest-mimic",
+ "linkme",
  "mbrman",
  "mesh",
  "mesh_process",
@@ -8423,8 +8451,6 @@ dependencies = [
 name = "vmm_test_petri_support"
 version = "0.0.0"
 dependencies = [
- "anyhow",
- "parking_lot",
  "petri",
  "petri_artifacts_common",
  "petri_artifacts_vmm_test",

diff --git a/Cargo.toml b/Cargo.toml
@@ -448,6 +448,7 @@ kvm-bindings = "0.7"
 landlock = "0.3.1"
 libc = "0.2"
 libfuzzer-sys = "0.4"
+libtest-mimic = "0.8"
 linkme = "0.3.9"
 log = "0.4"
 macaddr = "1.0"

diff --git a/petri/Cargo.toml b/petri/Cargo.toml
@@ -57,13 +57,16 @@ sparse_mmap.workspace = true
 
 anyhow.workspace = true
 async-trait.workspace = true
+clap.workspace = true
 fatfs = { workspace = true, features = ["std", "alloc"] }
 fs-err.workspace = true
 fscommon.workspace = true
 futures.workspace = true
 futures-concurrency.workspace = true
 gptman.workspace = true
 image = { workspace = true, features = ["png"] }
+libtest-mimic.workspace = true
+linkme.workspace = true
 mbrman.workspace = true
 prost.workspace = true
 tempfile.workspace = true

diff --git a/petri/petri_artifacts_core/src/lib.rs b/petri/petri_artifacts_core/src/lib.rs
@@ -16,6 +16,7 @@ use std::sync::Arc;
 // exported to support the `declare_artifacts!` macro
 #[doc(hidden)]
 pub use paste;
+use std::path::Path;
 
 /// A trait that marks a type as being the type-safe ID for a petri artifact.
 ///
@@ -141,38 +142,35 @@ macro_rules! declare_artifacts {
     };
 }
 
-/// An object-safe trait used to abstract details of artifact resolution.
+/// A trait to resolve artifacts to paths.
 ///
-/// Test authors are expected to use the [`TestArtifactResolver`] and
-/// [`TestArtifacts`] abstractions to interact with resolvers, and should not
+/// Test authors are expected to use the [`TestArtifactRequirements`] and
+/// [`TestArtifacts`] abstractions to interact with artifacts, and should not
 /// use this API directly.
-pub trait TestArtifactResolverBackend {
+pub trait ResolveTestArtifact {
     /// Given an artifact handle, return its corresponding PathBuf.
     ///
     /// This method must use type-erased handles, as using typed artifact
     /// handles in this API would cause the trait to no longer be object-safe.
     fn resolve(&self, id: ErasedArtifactHandle) -> anyhow::Result<PathBuf>;
+}
 
-    /// Invoked once all calls to `resolve` has gone through.
-    ///
-    /// Callers must ensure that this method is called after their final call to
-    /// `resolve`. By doing this, it is possible to implement "dry-run"
-    /// resolvers, which simply list a test's required dependencies, and then
-    /// terminate the test run.
-    fn finalize(self: Box<Self>) {}
+impl<T: ResolveTestArtifact + ?Sized> ResolveTestArtifact for &T {
+    fn resolve(&self, id: ErasedArtifactHandle) -> anyhow::Result<PathBuf> {
+        (**self).resolve(id)
+    }
 }
 
 /// A set of dependencies required to run a test.
-pub struct TestArtifactResolver {
-    backend: Box<dyn TestArtifactResolverBackend>,
+#[derive(Clone)]
+pub struct TestArtifactRequirements {
     artifacts: Vec<(ErasedArtifactHandle, bool)>,
 }
 
-impl TestArtifactResolver {
+impl TestArtifactRequirements {
     /// Create an empty set of dependencies.
-    pub fn new(backend: Box<dyn TestArtifactResolverBackend>) -> Self {
-        TestArtifactResolver {
-            backend,
+    pub fn new() -> Self {
+        TestArtifactRequirements {
             artifacts: Vec::new(),
         }
     }
@@ -189,13 +187,27 @@ impl TestArtifactResolver {
         self
     }
 
-    /// Finalize the set of dependencies.
-    pub fn finalize(self) -> TestArtifacts {
+    /// Returns the current list of required depencencies.
+    pub fn required_artifacts(&self) -> impl Iterator<Item = ErasedArtifactHandle> + '_ {
+        self.artifacts
+            .iter()
+            .filter_map(|&(a, optional)| (!optional).then_some(a))
+    }
+
+    /// Returns the current list of optional dependencies.
+    pub fn optional_artifacts(&self) -> impl Iterator<Item = ErasedArtifactHandle> + '_ {
+        self.artifacts
+            .iter()
+            .filter_map(|&(a, optional)| optional.then_some(a))
+    }
+
+    /// Resolve the set of dependencies.
+    pub fn resolve(&self, resolver: impl ResolveTestArtifact) -> anyhow::Result<TestArtifacts> {
         let mut failed = String::new();
         let mut resolved = HashMap::new();
 
-        for (a, optional) in self.artifacts {
-            match self.backend.resolve(a) {
+        for &(a, optional) in &self.artifacts {
+            match resolver.resolve(a) {
                 Ok(p) => {
                     resolved.insert(a, p);
                 }
@@ -204,36 +216,34 @@ impl TestArtifactResolver {
             }
         }
 
-        self.backend.finalize();
-
         if !failed.is_empty() {
-            panic!("Artifact resolution failed:\n{}", failed);
+            anyhow::bail!("Artifact resolution failed:\n{}", failed);
         }
 
-        TestArtifacts {
+        Ok(TestArtifacts {
             artifacts: Arc::new(resolved),
-        }
+        })
     }
 }
 
 /// A resolved set of test artifacts, returned by
-/// [`TestArtifactResolver::finalize`].
+/// [`TestArtifactRequirements::resolve`].
 #[derive(Clone)]
 pub struct TestArtifacts {
     artifacts: Arc<HashMap<ErasedArtifactHandle, PathBuf>>,
 }
 
 impl TestArtifacts {
-    /// Try to resolve an artifact to a path.
+    /// Try to get the resolved path of an artifact.
     #[track_caller]
-    pub fn try_resolve(&self, artifact: impl AsArtifactHandle) -> Option<PathBuf> {
-        self.artifacts.get(&artifact.erase()).cloned()
+    pub fn try_get(&self, artifact: impl AsArtifactHandle) -> Option<&Path> {
+        self.artifacts.get(&artifact.erase()).map(|p| p.as_ref())
     }
 
-    /// Resolve an artifact to a path.
+    /// Get the resolved path of an artifact.
     #[track_caller]
-    pub fn resolve(&self, artifact: impl AsArtifactHandle) -> PathBuf {
-        self.try_resolve(artifact.erase())
+    pub fn get(&self, artifact: impl AsArtifactHandle) -> &Path {
+        self.try_get(artifact.erase())
             .unwrap_or_else(|| panic!("Artifact not initially required: {:?}", artifact.erase()))
     }
 }
diff --git a/petri/src/disk_image.rs b/petri/src/disk_image.rs
@@ -20,7 +20,7 @@ use std::path::Path;
 pub fn build_agent_image(
     arch: MachineArch,
     os_flavor: OsFlavor,
-    resolver: &TestArtifacts,
+    artifacts: &TestArtifacts,
 ) -> anyhow::Result<tempfile::NamedTempFile> {
     match os_flavor {
         OsFlavor::Windows => {
@@ -30,7 +30,7 @@ pub fn build_agent_image(
                 b"pipette    ",
                 &[(
                     "pipette.exe",
-                    PathOrBinary::Path(&resolver.resolve(match arch {
+                    PathOrBinary::Path(artifacts.get(match arch {
                         MachineArch::X86_64 => common_artifacts::PIPETTE_WINDOWS_X64.erase(),
                         MachineArch::Aarch64 => common_artifacts::PIPETTE_WINDOWS_AARCH64.erase(),
                     })),
@@ -45,7 +45,7 @@ pub fn build_agent_image(
                 &[
                     (
                         "pipette",
-                        PathOrBinary::Path(&resolver.resolve(match arch {
+                        PathOrBinary::Path(artifacts.get(match arch {
                             MachineArch::X86_64 => common_artifacts::PIPETTE_LINUX_X64.erase(),
                             MachineArch::Aarch64 => common_artifacts::PIPETTE_LINUX_AARCH64.erase(),
                         })),

diff --git a/petri/src/lib.rs b/petri/src/lib.rs
@@ -12,17 +12,22 @@
 mod disk_image;
 mod linux_direct_serial_agent;
 mod openhcl_diag;
+mod test;
 mod tracing;
 mod vm;
 mod worker;
 
 pub use petri_artifacts_core::ArtifactHandle;
 pub use petri_artifacts_core::AsArtifactHandle;
 pub use petri_artifacts_core::ErasedArtifactHandle;
-pub use petri_artifacts_core::TestArtifactResolver;
-pub use petri_artifacts_core::TestArtifactResolverBackend;
+pub use petri_artifacts_core::ResolveTestArtifact;
+pub use petri_artifacts_core::TestArtifactRequirements;
 pub use petri_artifacts_core::TestArtifacts;
 pub use pipette_client as pipette;
+pub use test::test_macro_support;
+pub use test::test_main;
+pub use test::RunTest;
+pub use test::SimpleTest;
 pub use vm::*;
 
 /// 1 kibibyte's worth of bytes.