phd: be picky about allowed test skips

phd-tests/framework/src/artifacts/store.rs

@@ -20,28 +20,30 @@ use std::collections::BTreeMap;
 use std::fs::File;
 use std::io::{BufReader, Cursor, Read, Seek, SeekFrom, Write};
 use std::time::Duration;
-use tokio::sync::Mutex;
+use tokio::sync::{Mutex, MutexGuard};
 use tracing::{debug, info, warn};
 
 #[derive(Debug)]
 struct StoredArtifact {
     description: super::Artifact,
-    cached_path: Option<Utf8PathBuf>,
+    cached_path: Mutex<Option<Utf8PathBuf>>,
 }
 
 impl StoredArtifact {
     fn new(description: super::Artifact) -> Self {
-        Self { description, cached_path: None }
+        Self { description, cached_path: Mutex::new(None) }
     }
 
     async fn ensure(
-        &mut self,
+        &self,
         local_dir: &Utf8Path,
         downloader: &DownloadConfig,
     ) -> anyhow::Result<Utf8PathBuf> {
+        let mut path_guard = self.cached_path.lock().await;
+
         // If the artifact already exists and has been verified, return the path
         // to it straightaway.
-        if let Some(path) = &self.cached_path {
+        if let Some(path) = path_guard.as_ref() {
             debug!(%path, "Verified artifact already exists");
             return Ok(path.clone());
         }
@@ -67,7 +69,7 @@ impl StoredArtifact {
             // The file is in the right place and has the right hash (if that
             // was checked), so mark it as cached and return the cached path.
             debug!(%path, "Locally-sourced artifact is valid, caching its path");
-            self.cached_path = Some(path.clone());
+            *path_guard = Some(path.clone());
             return Ok(path.clone());
         }
 
@@ -91,7 +93,7 @@ impl StoredArtifact {
             if file_hash_equals(&maybe_path, expected_digest).is_ok() {
                 debug!(%maybe_path,
                       "Valid artifact already exists, caching its path");
-                return self.cache_path(maybe_path);
+                return self.cache_path(path_guard, maybe_path);
             } else {
                 warn!(%maybe_path, "Existing artifact is invalid, deleting it");
                 std::fs::remove_file(&maybe_path)?;
@@ -140,11 +142,12 @@ impl StoredArtifact {
         permissions.set_readonly(true);
         new_file.set_permissions(permissions)?;
 
-        self.cache_path(maybe_path)
+        self.cache_path(path_guard, maybe_path)
     }
 
     fn cache_path(
-        &mut self,
+        &self,
+        mut path_guard: MutexGuard<'_, Option<Utf8PathBuf>>,
         mut path: Utf8PathBuf,
     ) -> anyhow::Result<Utf8PathBuf> {
         if let Some(ref untar_path) = self.description.untar {
@@ -166,15 +169,15 @@ impl StoredArtifact {
             }
         };
 
-        self.cached_path = Some(path.clone());
+        *path_guard = Some(path.clone());
         Ok(path)
     }
 }
 
 #[derive(Debug)]
 pub struct Store {
     local_dir: Utf8PathBuf,
-    artifacts: BTreeMap<String, Mutex<StoredArtifact>>,
+    artifacts: BTreeMap<String, StoredArtifact>,
     downloader: DownloadConfig,
 }
 
@@ -199,7 +202,7 @@ impl Store {
         let Manifest { artifacts, remote_server_uris } = manifest;
         let artifacts = artifacts
             .into_iter()
-            .map(|(k, v)| (k, Mutex::new(StoredArtifact::new(v))))
+            .map(|(k, v)| (k, StoredArtifact::new(v)))
             .collect();
 
         let buildomat_backoff = backoff::ExponentialBackoffBuilder::new()
@@ -281,7 +284,7 @@ impl Store {
 
         let _old = self.artifacts.insert(
             BASE_PROPOLIS_ARTIFACT.to_string(),
-            Mutex::new(StoredArtifact::new(artifact)),
+            StoredArtifact::new(artifact),
         );
         assert!(_old.is_none());
         Ok(())
@@ -335,24 +338,36 @@ impl Store {
 
                 let _old = self.artifacts.insert(
                     CRUCIBLE_DOWNSTAIRS_ARTIFACT.to_string(),
-                    Mutex::new(StoredArtifact::new(artifact)),
+                    StoredArtifact::new(artifact),
                 );
                 assert!(_old.is_none());
                 Ok(())
             }
         }
     }
 
+    pub fn get_guest_os_image_kind(
+        &self,
+        artifact_name: &str,
+    ) -> anyhow::Result<GuestOsKind> {
+        let entry = self.get_artifact(artifact_name)?;
+        match entry.description.kind {
+            super::ArtifactKind::GuestOs(kind) => Ok(kind),
+            _ => Err(anyhow::anyhow!(
+                "artifact {artifact_name} is not a guest OS image"
+            )),
+        }
+    }
+
     pub async fn get_guest_os_image(
         &self,
         artifact_name: &str,
     ) -> anyhow::Result<(Utf8PathBuf, GuestOsKind)> {
         let entry = self.get_artifact(artifact_name)?;
-        let mut guard = entry.lock().await;
-        match guard.description.kind {
+        match entry.description.kind {
             super::ArtifactKind::GuestOs(kind) => {
                 let path =
-                    guard.ensure(&self.local_dir, &self.downloader).await?;
+                    entry.ensure(&self.local_dir, &self.downloader).await?;
                 Ok((path, kind))
             }
             _ => Err(anyhow::anyhow!(
@@ -366,10 +381,9 @@ impl Store {
         artifact_name: &str,
     ) -> anyhow::Result<Utf8PathBuf> {
         let entry = self.get_artifact(artifact_name)?;
-        let mut guard = entry.lock().await;
-        match guard.description.kind {
+        match entry.description.kind {
             super::ArtifactKind::Bootrom => {
-                guard.ensure(&self.local_dir, &self.downloader).await
+                entry.ensure(&self.local_dir, &self.downloader).await
             }
             _ => Err(anyhow::anyhow!(
                 "artifact {artifact_name} is not a bootrom"
@@ -382,10 +396,9 @@ impl Store {
         artifact_name: &str,
     ) -> anyhow::Result<Utf8PathBuf> {
         let entry = self.get_artifact(artifact_name)?;
-        let mut guard = entry.lock().await;
-        match guard.description.kind {
+        match entry.description.kind {
             super::ArtifactKind::PropolisServer => {
-                guard.ensure(&self.local_dir, &self.downloader).await
+                entry.ensure(&self.local_dir, &self.downloader).await
             }
             _ => Err(anyhow::anyhow!(
                 "artifact {artifact_name} is not a Propolis server"
@@ -395,21 +408,17 @@ impl Store {
 
     pub async fn get_crucible_downstairs(&self) -> anyhow::Result<Utf8PathBuf> {
         let entry = self.get_artifact(CRUCIBLE_DOWNSTAIRS_ARTIFACT)?;
-        let mut guard = entry.lock().await;
-        match guard.description.kind {
+        match entry.description.kind {
             super::ArtifactKind::CrucibleDownstairs => {
-                guard.ensure(&self.local_dir, &self.downloader).await
+                entry.ensure(&self.local_dir, &self.downloader).await
             }
             _ => Err(anyhow::anyhow!(
                 "artifact {CRUCIBLE_DOWNSTAIRS_ARTIFACT} is not a Crucible downstairs binary",
             )),
         }
     }
 
-    fn get_artifact(
-        &self,
-        name: &str,
-    ) -> anyhow::Result<&Mutex<StoredArtifact>> {
+    fn get_artifact(&self, name: &str) -> anyhow::Result<&StoredArtifact> {
         self.artifacts.get(name).ok_or_else(|| {
             anyhow::anyhow!("artifact {name} not found in store")
         })
@@ -449,10 +458,9 @@ impl Store {
             untar: None,
         };
 
-        let _old: Option<Mutex<StoredArtifact>> = self.artifacts.insert(
-            artifact_name.to_string(),
-            Mutex::new(StoredArtifact::new(artifact)),
-        );
+        let _old: Option<StoredArtifact> = self
+            .artifacts
+            .insert(artifact_name.to_string(), StoredArtifact::new(artifact));
         assert!(_old.is_none());
 
         Ok(())

phd-tests/framework/src/guest_os/mod.rs

@@ -59,7 +59,7 @@ impl<'a> CommandSequenceEntry<'a> {
     }
 }
 
-pub(super) struct CommandSequence<'a>(pub Vec<CommandSequenceEntry<'a>>);
+pub struct CommandSequence<'a>(pub(crate) Vec<CommandSequenceEntry<'a>>);
 
 impl<'a> CommandSequence<'a> {
     fn extend(mut self, other: CommandSequence<'a>) -> CommandSequence<'a> {
@@ -68,15 +68,15 @@ impl<'a> CommandSequence<'a> {
     }
 }
 
-pub(super) trait GuestOs: Send + Sync {
+pub trait GuestOs: Send + Sync {
     /// Retrieves the command sequence used to wait for the OS to boot and log
     /// into it.
     fn get_login_sequence(&self) -> CommandSequence<'_>;
 
     /// Retrieves the default shell prompt for this OS.
     fn get_shell_prompt(&self) -> &'static str;
 
-    /// Indicates whether the guest has a read-only filesystem.
+    /// Indicates whether the guest is expected to have a read-only filesystem.
     fn read_only_fs(&self) -> bool;
 
     /// Returns the sequence of serial console operations a test VM should issue

phd-tests/framework/src/lib.rs

@@ -35,7 +35,7 @@ use camino::Utf8PathBuf;
 
 use disk::DiskFactory;
 use futures::{stream::FuturesUnordered, StreamExt};
-use guest_os::GuestOsKind;
+use guest_os::{GuestOs, GuestOsKind};
 use log_config::LogConfig;
 use port_allocator::PortAllocator;
 pub use test_vm::TestVm;
@@ -150,10 +150,14 @@ impl TestCtx {
         self.framework.default_guest_os_artifact()
     }
 
-    /// Yields the guest OS adapter corresponding to the default guest OS
-    /// artifact.
-    pub async fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
-        self.framework.default_guest_os_kind().await
+    /// Returns the guest OS kind corresponding to the default guest OS artifact.
+    pub fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
+        self.framework.default_guest_os_kind()
+    }
+
+    /// Returns the guest OS adapter corresponding to the default guest OS artifact.
+    pub fn default_guest_os_adapter(&self) -> anyhow::Result<Box<dyn GuestOs>> {
+        self.default_guest_os_kind().map(guest_os::get_guest_os_adapter)
     }
 
     /// Indicates whether the disk factory in this framework supports the
@@ -366,12 +370,9 @@ impl Framework {
 
     /// Yields the guest OS adapter corresponding to the default guest OS
     /// artifact.
-    pub async fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
-        Ok(self
-            .artifact_store
-            .get_guest_os_image(&self.default_guest_os_artifact)
-            .await?
-            .1)
+    pub fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
+        self.artifact_store
+            .get_guest_os_image_kind(&self.default_guest_os_artifact)
     }
 
     /// Indicates whether the disk factory in this framework supports the

phd-tests/testcase/src/lib.rs

@@ -50,6 +50,10 @@ pub struct TestCase {
 
     /// The test function to execute to run this test.
     pub(crate) function: TestFunction,
+
+    /// A function used to check if a test's skip or pass was actually expected
+    /// for the given `TestCtx`.
+    pub(crate) check_skip_fn: Option<fn(&TestCtx) -> bool>,
 }
 
 #[allow(dead_code)]
@@ -59,8 +63,9 @@ impl TestCase {
         module_path: &'static str,
         name: &'static str,
         function: TestFunction,
+        check_skip_fn: Option<fn(&TestCtx) -> bool>,
     ) -> Self {
-        Self { module_path, name, function }
+        Self { module_path, name, function, check_skip_fn }
     }
 
     /// Returns the test case's fully qualified name, i.e. `module_path::name`.
@@ -76,7 +81,48 @@ impl TestCase {
     /// Runs the test case's body with the supplied test context and returns its
     /// outcome.
     pub async fn run(&self, ctx: &TestCtx) -> TestOutcome {
-        (self.function.f)(ctx).await
+        let outcome = (self.function.f)(ctx).await;
+        match (outcome, self.check_skip_fn) {
+            (TestOutcome::Passed, check_skip_fn) => {
+                let wanted_skip = if let Some(check_skip_fn) = check_skip_fn {
+                    check_skip_fn(ctx)
+                } else {
+                    false
+                };
+
+                if wanted_skip {
+                    return TestOutcome::Failed(Some(
+                        "test passed but expected skip?".to_string(),
+                    ));
+                }
+
+                TestOutcome::Passed
+            }
+            (TestOutcome::Failed(skip_msg), _) => TestOutcome::Failed(skip_msg),
+            (TestOutcome::Skipped(skip_msg), None) => {
+                let fail_msg = match skip_msg {
+                    Some(msg) => {
+                        format!("skipped without check_skip attribute: {msg}")
+                    }
+                    None => "skipped without check_skip attribute".to_string(),
+                };
+                TestOutcome::Failed(Some(fail_msg))
+            }
+            (TestOutcome::Skipped(skip_msg), Some(check_skip_fn)) => {
+                if check_skip_fn(ctx) {
+                    return TestOutcome::Skipped(skip_msg);
+                }
+
+                let fail_msg = match skip_msg {
+                    Some(msg) => {
+                        format!("skipped but did not expect skip: {msg}")
+                    }
+                    None => "skipped but did not expect skip".to_string(),
+                };
+
+                TestOutcome::Failed(Some(fail_msg))
+            }
+        }
     }
 }