phd: be pedantic about what test skips are OK

This gives us some additional confidence that all tests that run either:
* pass
* skip, but were expected to given the test hardware/guest configuration

Importantly, not all PHD tests pass on all hardware/guest OS
combinations, and not all PHD tests even *run* on all hardware/guest
combinations. As a bit of a fluke, *Intel* happens to run one additional
test that is skipped on AMD processors, even.

The strategy is to run "skipped" tests and verify they skip because if
guest artifacts are changed unexpectedly we may need to better
understand how the artifact differs from PHD expectations. Or if
Propolis/PHD implementation changes such that the skip is no longer
taken, failing the test instead of silently passing will force us to
notice if that change is wanted, if the test *should* pass, etc.

As an example, the test Alpine image is currently expected to be an
`alpine-virt` on an ISO 9660 (read-only) filesystem. Since this means
the EFI system partition is also read-only,
`guest_can_adjust_boot_order` is not a meaningful test (for now). We
run this test anyway and *expect* a skip, because if it passed then
either:
* the guest Alpine is on a writable root FS; are we missing test
  coverage involving read-only guest images?
* we've moved EFI NvVars to out-of-guest storage, and efivarfs writes
  succeed.

In either case, the corresponding changes should have some consideration
for how they relate Propolis and guest operations.

Author: iximeow

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;
@@ -155,6 +155,11 @@ impl TestCtx {
         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
     /// creation of Crucible disks. This can be used to skip tests that require
     /// Crucible support.

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,52 @@ 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))
+            }
+        }
     }
 }
 

phd-tests/testcase_macro/src/lib.rs

@@ -6,7 +6,61 @@ use heck::ToShoutySnakeCase;
 use proc_macro::TokenStream;
 use proc_macro_error::{abort, proc_macro_error};
 use quote::{format_ident, quote};
-use syn::{parse_macro_input, spanned::Spanned, ItemFn};
+use syn::parse::{Parse, ParseStream};
+use syn::punctuated::Punctuated;
+use syn::spanned::Spanned;
+use syn::{parse_macro_input, ItemFn, Token};
+
+// More or less directly inspired by Omicron's
+// `nexus/test-utils-macros/src/lib.rs`
+struct PhdTestAttrs {
+    /// The top-level function determining if a `phd_skip!()` test conclusion is
+    /// acceptable or not. See the documentation of `fn phd_testcase` below for
+    /// more.
+    check_skip_fn: Option<syn::Path>,
+}
+
+#[derive(Debug, PartialEq, Eq, Hash)]
+pub(crate) enum PhdTestArg {
+    CheckSkipFn(syn::Path),
+}
+
+impl Parse for PhdTestArg {
+    fn parse(input: ParseStream) -> syn::Result<Self> {
+        let name: syn::Path = input.parse()?;
+        let _eq_token: syn::token::Eq = input.parse()?;
+
+        if name.is_ident("check_skip") {
+            let fn_ident: syn::Path = input.parse()?;
+            return Ok(Self::CheckSkipFn(fn_ident));
+        }
+
+        Err(syn::Error::new(name.span(), "unrecognized argument to phd_test"))
+    }
+}
+
+impl Parse for PhdTestAttrs {
+    fn parse(input: ParseStream) -> syn::Result<Self> {
+        let vars =
+            Punctuated::<PhdTestArg, Token![,]>::parse_terminated(input)?;
+
+        let mut check_skip_fn = None;
+
+        for var in vars {
+            match var {
+                PhdTestArg::CheckSkipFn(path) => {
+                    assert!(
+                        check_skip_fn.is_none(),
+                        "check_skip function can be provided at most once"
+                    );
+                    check_skip_fn = Some(path);
+                }
+            }
+        }
+
+        Ok(PhdTestAttrs { check_skip_fn })
+    }
+}
 
 /// The macro for labeling PHD testcases.
 ///
@@ -15,23 +69,37 @@ use syn::{parse_macro_input, spanned::Spanned, ItemFn};
 /// wrapper function that returns a `phd_testcase::TestOutcome` and creates an
 /// entry in the test case inventory that allows the PHD runner to enumerate the
 /// test.
+///
+/// PHD checks if a test is allowed to skip for a given test configuration. A
+/// test that may skip must have a `check_skip(some::check::function)`; if the
+/// test skips then the provided function is called to determine if skipping is
+/// allowed. If such a test passes instead of skipping, the will also be failed
+/// if it was expected to skip in the test environment.
 #[proc_macro_error]
 #[proc_macro_attribute]
-pub fn phd_testcase(_attrib: TokenStream, input: TokenStream) -> TokenStream {
+pub fn phd_testcase(attrs: TokenStream, input: TokenStream) -> TokenStream {
     let item_fn = parse_macro_input!(input as ItemFn);
 
+    let test_attrs = parse_macro_input!(attrs as PhdTestAttrs);
+
     // Build the inventory record for this test. The `module_path!()` in the
     // generated code allows the test case to report the fully-qualified path to
     // itself regardless of where it's located.
     let fn_ident = item_fn.sig.ident.clone();
     let fn_name = fn_ident.to_string();
     let static_ident = format_ident!("{}", fn_name.to_shouty_snake_case());
+    let check_skip_fn = if let Some(check_skip_fn) = test_attrs.check_skip_fn {
+        quote! { Some(#check_skip_fn) }
+    } else {
+        quote! { None }
+    };
     let submit: proc_macro2::TokenStream = quote! {
         #[linkme::distributed_slice(phd_testcase::TEST_CASES)]
         static #static_ident: phd_testcase::TestCase = phd_testcase::TestCase::new(
             module_path!(),
             #fn_name,
-            phd_testcase::TestFunction { f: |ctx| Box::pin(#fn_ident(ctx)) }
+            phd_testcase::TestFunction { f: |ctx| Box::pin(#fn_ident(ctx)) },
+            #check_skip_fn,
         );
     };
 

phd-tests/tests/src/boot_order.rs

@@ -20,6 +20,14 @@ use efi_utils::{
     EDK2_FIRMWARE_VOL_GUID, EDK2_UI_APP_GUID,
 };
 
+fn no_efivarfs(ctx: &TestCtx) -> bool {
+    // A predicate function most boot order tests are gated on. There is some
+    // minimum Linux version where efivarfs first shipped, but we're assuming
+    // that no one is testing such an old Linux under PHD; kernel version's
+    // aren't directly represented in the guest OS kind anyway.
+    !ctx.default_guest_os_kind().expect("has default guest os kind").is_linux()
+}
+
 // This test checks that with a specified boot order, the guest boots whichever
 // disk we wanted to come first. This is simple enough, until you want to know
 // "what you booted from"..
@@ -48,7 +56,7 @@ use efi_utils::{
 //
 // Unlike later tests, this test does not manipulate boot configuration from
 // inside the guest OS.
-#[phd_testcase]
+#[phd_testcase(check_skip = no_efivarfs)]
 async fn configurable_boot_order(ctx: &TestCtx) {
     let mut cfg = ctx.vm_config_builder("configurable_boot_order");
 
@@ -114,7 +122,7 @@ async fn configurable_boot_order(ctx: &TestCtx) {
 // specifically asserts that the unbootable disk is first in the boot order; the
 // system booting means that boot order is respected and a non-bootable disk
 // does not wedge startup.
-#[phd_testcase]
+#[phd_testcase(check_skip = no_efivarfs)]
 async fn unbootable_disk_skipped(ctx: &TestCtx) {
     let mut cfg = ctx.vm_config_builder("unbootable_disk_skipped");
 
@@ -233,7 +241,12 @@ async fn unbootable_disk_skipped(ctx: &TestCtx) {
 // Start with the boot order being `["boot-disk", "unbootable"]`, then change it
 // so that next boot we'll boot from `unbootable` first. Then reboot and verify
 // that the boot order is still "boot-disk" first.
-#[phd_testcase]
+//
+// TODO: this test will `skip` if the guest is a read-only Alpine, for example.
+// if the guest doesn't have an efivarfs, this test warns loudly and then
+// passes. The predicate function checks for a much simpler "is linux". Both of
+// these are hard to discover from a predicate function (probably?)
+#[phd_testcase(check_skip = no_efivarfs)]
 async fn guest_can_adjust_boot_order(ctx: &TestCtx) {
     let mut cfg = ctx.vm_config_builder("guest_can_adjust_boot_order");
 
@@ -400,7 +413,7 @@ async fn guest_can_adjust_boot_order(ctx: &TestCtx) {
 // If `bootorder` is removed for subsequent reboots, the EFI System Partition's
 // store of NvVar variables is the source of boot order, and guests can control
 // their boot fates.
-#[phd_testcase]
+#[phd_testcase(check_skip = no_efivarfs)]
 async fn boot_order_source_priority(ctx: &TestCtx) {
     let mut cfg = ctx.vm_config_builder("boot_order_source_priority");
 
@@ -507,7 +520,7 @@ async fn boot_order_source_priority(ctx: &TestCtx) {
     );
 }
 
-#[phd_testcase]
+#[phd_testcase(check_skip = no_efivarfs)]
 async fn nvme_boot_option_description(ctx: &TestCtx) {
     let mut cfg = ctx.vm_config_builder("nvme_boot_option_description");
 

phd-tests/tests/src/cpuid.rs

@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues};
+use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues, CpuidVendor};
 use itertools::Itertools;
 use phd_framework::{test_vm::MigrationTimeout, TestVm};
 use phd_testcase::*;
@@ -276,7 +276,16 @@ impl<'a> LinuxGuestTopo<'a> {
     }
 }
 
-#[phd_testcase]
+fn host_not_intel(_ctx: &TestCtx) -> bool {
+    let base_leaf: CpuidIdent = CpuidIdent::leaf(0);
+
+    let values = cpuid_utils::host::query(base_leaf);
+    let vendor = CpuidVendor::try_from(values).expect("recognized CPU vendor");
+
+    vendor != CpuidVendor::Intel
+}
+
+#[phd_testcase(check_skip = host_not_intel)]
 async fn guest_cpu_topo_test(ctx: &TestCtx) {
     let vm = launch_cpuid_smoke_test_vm(ctx, "guest_cpu_topo_test").await?;
 

phd-tests/tests/src/crucible/migrate.rs

@@ -7,7 +7,7 @@ use phd_testcase::*;
 use tracing::info;
 use uuid::Uuid;
 
-#[phd_testcase]
+#[phd_testcase(check_skip = super::crucible_disabled)]
 async fn smoke_test(ctx: &TestCtx) {
     let mut config = ctx.vm_config_builder("crucible_migrate_smoke_source");
     super::add_default_boot_disk(ctx, &mut config)?;
@@ -37,7 +37,7 @@ async fn smoke_test(ctx: &TestCtx) {
     assert_eq!(lsout, "foo.bar");
 }
 
-#[phd_testcase]
+#[phd_testcase(check_skip = super::crucible_disabled)]
 async fn load_test(ctx: &TestCtx) {
     let mut config = ctx.vm_config_builder("crucible_load_test_source");
     super::add_default_boot_disk(ctx, &mut config)?;

phd-tests/tests/src/crucible/mod.rs

@@ -13,6 +13,12 @@ use phd_testcase::{
 mod migrate;
 mod smoke;
 
+// This predicate is useful for tests depending on these functions
+// which need to report if they expected to be skipped or not.
+pub(crate) fn crucible_disabled(ctx: &TestCtx) -> bool {
+    !ctx.crucible_enabled()
+}
+
 fn add_crucible_boot_disk_or_skip<'a>(
     ctx: &TestCtx,
     config: &mut VmConfig<'a>,