Merge pull request #96 from ErichDonGubler/filter-skip-outcomes

`update-expected`: ignore aggregated `SKIP` outcomes in tests if other outcomes are also reported
erichdongubler-mozilla · Apr 23, 2024 · c4a011f · c4a011f
2 parents 7ed6b85 + b34f4ed
commit c4a011f
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 17 deletions.
diff --git a/moz-webgpu-cts/src/main.rs b/moz-webgpu-cts/src/main.rs
@@ -532,18 +532,12 @@ fn run(cli: Cli) -> ExitCode {
                 .chain(other_entries_by_test)
                 .filter_map(|(test_path, test_entry)| {
                     fn reconcile<Out>(
-                        entry: Entry<Out>,
+                        meta_props: &mut TestProps<Out>,
+                        reported: BTreeMap<Platform, BTreeMap<BuildProfile, Expected<Out>>>,
                         preset: ReportProcessingPreset,
-                    ) -> TestProps<Out>
-                    where
+                    ) where
                         Out: Debug + Default + EnumSetType,
                     {
-                        let Entry {
-                            meta_props,
-                            reported,
-                        } = entry;
-
-                        let mut meta_props = meta_props.unwrap_or_default();
                         let reconciled = 'resolve: {
                             let reported = |platform, build_profile| {
                                 reported
@@ -587,19 +581,22 @@ fn run(cli: Cli) -> ExitCode {
                             }
                         };
                         meta_props.expected = Some(reconciled);
-                        meta_props
                     }
 
                     let TestEntry {
-                        entry: test_entry,
+                        entry:
+                            Entry {
+                                meta_props: properties,
+                                reported: mut test_reported,
+                            },
                         subtests: subtest_entries,
                     } = test_entry;
 
-                    if test_entry.meta_props.is_none() {
+                    if properties.is_none() {
                         log::info!("new test entry: {test_path:?}")
                     }
 
-                    if test_entry.reported.is_empty() && using_reports {
+                    if test_reported.is_empty() && using_reports {
                         let test_path = &test_path;
                         let msg = lazy_format!("no entries found in reports for {:?}", test_path);
                         match preset {
@@ -612,7 +609,44 @@ fn run(cli: Cli) -> ExitCode {
                         }
                     }
 
-                    let properties = reconcile(test_entry, preset);
+                    let mut properties = properties.unwrap_or_default();
+
+                    for (platform, build_profile, reported) in
+                        test_reported.iter_mut().flat_map(|(p, by_bp)| {
+                            by_bp
+                                .iter_mut()
+                                .map(move |(bp, reported)| (p, bp, reported))
+                        })
+                    {
+                        let skip = TestOutcome::Skip;
+                        // Ignore `SKIP` outcomes if we have non-`SKIP` outcomes here.
+                        //
+                        // Do this so that test runs whose coverage _in aggregate_ includes actual
+                        // runs on this test are viable for processing. Otherwise, we'd have `SKIP`
+                        // outcomes be included that aren't actually wanted.
+                        if *reported != skip {
+                            let skip = skip.into();
+                            if reported.inner().is_superset(skip) {
+                                log::debug!(
+                                    concat!(
+                                        "encountered `{}` among other outcomes ",
+                                        "in aggregation of reported test outcomes ",
+                                        "for {:?} with platform {:?} and build profile {:?}, ",
+                                        " removing with the assumption that ",
+                                        "this is an artifact of disjoint test runs"
+                                    ),
+                                    skip,
+                                    test_path,
+                                    platform,
+                                    build_profile,
+                                );
+                                *reported = Expected::new(reported.inner() & !skip)
+                                    .expect("internal error: empty non-`SKIP` superset");
+                            }
+                        }
+                    }
+
+                    reconcile(&mut properties, test_reported, preset);
 
                     let mut subtests = BTreeMap::new();
                     for (subtest_name, subtest) in subtest_entries {
@@ -622,8 +656,13 @@ fn run(cli: Cli) -> ExitCode {
                             log::error!("internal error: duplicate test path {test_path:?}");
                         }
 
-                        let mut properties = reconcile(subtest, preset);
+                        let Entry {
+                            meta_props: properties,
+                            reported: subtest_reported,
+                        } = subtest;
 
+                        let mut properties = properties.unwrap_or_default();
+                        reconcile(&mut properties, subtest_reported, preset);
                         for (_, expected) in properties.expected.as_mut().unwrap().iter_mut() {
                             taint_subtest_timeouts_by_suspicion(expected);
                         }

diff --git a/moz-webgpu-cts/src/metadata.rs b/moz-webgpu-cts/src/metadata.rs
@@ -745,7 +745,7 @@ where
             where
                 Out: Default + EnumSetType + Eq + PartialEq,
             {
-                if exp != &Default::default() {
+                if exp != &Out::default() {
                     f()
                 } else {
                     Ok(())

diff --git a/moz-webgpu-cts/src/shared.rs b/moz-webgpu-cts/src/shared.rs
@@ -24,7 +24,7 @@ use crate::metadata::{BuildProfile, Platform};
 ///
 /// [`Test`]: crate::metadata::Test
 /// [`Subtest`]: crate::metadata::Subtest
-#[derive(Clone, Copy, Eq, PartialEq)]
+#[derive(Clone, Copy)]
 pub struct Expected<Out>(EnumSet<Out>)
 where
     Out: EnumSetType;
@@ -191,6 +191,26 @@ where
     }
 }
 
+impl<Out> PartialEq for Expected<Out>
+where
+    Out: EnumSetType + Eq,
+{
+    fn eq(&self, other: &Self) -> bool {
+        self.inner() == other.inner()
+    }
+}
+
+impl<Out> PartialEq<Out> for Expected<Out>
+where
+    Out: EnumSetType + Eq,
+{
+    fn eq(&self, other: &Out) -> bool {
+        self.inner() == *other
+    }
+}
+
+impl<Out> Eq for Expected<Out> where Out: EnumSetType + Eq {}
+
 /// Similar to the ubiquitous `enum Either`, but with the implication that `Collapsed` values are
 /// abbreviations of equivalent `Expanded` values.
 #[derive(Clone, Debug, Eq, PartialEq)]