tests: Make known_failure more precise

tests/README.md

@@ -35,8 +35,13 @@ sleep_to_meet_frame_rate = false
 # Prefer setting `known_failure = true` to ignoring the test.
 ignore = false
 
-# If true, this test is known to fail and the test runner will expect it to fail.
+# If true, this test is known to fail and the test runner will expect the check against
+# the trace output (specified `output_path`) to fail.
 # When the test passes in the future, it'll fail and alert that it now passes.
+# This will not catch Ruffle panics; if the test is expected to panic, use
+#   `known_failure.panic = "panic message"`
+# instead (note that 'panicky' tests will be skipped if the test harness is run
+# with debug assertions disabled, e.g. with `--release`).
 known_failure = false
 
 # Path (relative to the directory containing test.toml) to the expected output
@@ -97,6 +102,10 @@ with_default_font = false
 # This requires a render to be setup for this test
 [image_comparisons.COMPARISON_NAME] # COMPARISON_NAME is a name of this particular image
 
+# If true, this image comparison is known to fail and the test runner will expect it to fail.
+# When the comparison passes in the future, it'll fail and alert that it now passes.
+known_failure = false
+
 # The tolerance per pixel channel to be considered "the same".
 # Increase as needed with tests that aren't pixel perfect across platforms.
 # Prefer running tests with higher sample count to make a better use of this option.

tests/framework/src/options.rs

@@ -2,12 +2,14 @@ pub mod approximations;
 pub mod expression;
 pub mod font;
 pub mod image_comparison;
+pub mod known_failure;
 pub mod player;
 
 use crate::image_trigger::ImageTrigger;
 use crate::options::approximations::Approximations;
 use crate::options::font::{DefaultFontsOptions, FontOptions, FontSortOptions};
 use crate::options::image_comparison::ImageComparison;
+use crate::options::known_failure::KnownFailure;
 use crate::options::player::PlayerOptions;
 use anyhow::{Result, bail};
 use serde::Deserialize;
@@ -68,7 +70,7 @@ pub struct TestOptions {
     pub sleep_to_meet_frame_rate: bool,
     pub image_comparisons: HashMap<String, ImageComparison>,
     pub ignore: bool,
-    pub known_failure: bool,
+    pub known_failure: KnownFailure,
     pub approximations: Option<Approximations>,
     pub player_options: PlayerOptions,
     pub log_fetch: bool,
@@ -89,7 +91,7 @@ impl Default for TestOptions {
             sleep_to_meet_frame_rate: false,
             image_comparisons: Default::default(),
             ignore: false,
-            known_failure: false,
+            known_failure: KnownFailure::None,
             approximations: None,
             player_options: PlayerOptions::default(),
             log_fetch: false,
@@ -178,6 +180,11 @@ impl TestOptions {
         Ok(())
     }
 
+    pub fn has_known_failure(&self) -> bool {
+        !matches!(self.known_failure, KnownFailure::None)
+            || self.image_comparisons.values().any(|cmp| cmp.known_failure)
+    }
+
     pub fn output_path(&self, test_directory: &VfsPath) -> Result<VfsPath> {
         Ok(test_directory.join(&self.output_path)?)
     }

tests/framework/src/options/image_comparison.rs

@@ -11,6 +11,7 @@ pub struct ImageComparison {
     max_outliers: Option<usize>,
     checks: Vec<ImageComparisonCheck>,
     pub trigger: ImageTrigger,
+    pub known_failure: bool,
 }
 
 impl ImageComparison {

tests/framework/src/options/known_failure.rs

@@ -0,0 +1,53 @@
+use std::fmt;
+
+use serde::{
+    Deserialize, Deserializer,
+    de::{self, value::MapAccessDeserializer},
+};
+
+#[derive(Clone, Debug, Default)]
+pub enum KnownFailure {
+    #[default]
+    None,
+    TraceOutput,
+    Panic {
+        message: String,
+    },
+}
+
+impl<'de> Deserialize<'de> for KnownFailure {
+    fn deserialize<D: Deserializer<'de>>(deser: D) -> Result<Self, D::Error> {
+        deser.deserialize_any(KnownFailureVisitor)
+    }
+}
+
+struct KnownFailureVisitor;
+
+impl<'de> de::Visitor<'de> for KnownFailureVisitor {
+    type Value = KnownFailure;
+
+    fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str("a boolean, or `.panic = 'message'`")
+    }
+
+    fn visit_bool<E: de::Error>(self, v: bool) -> Result<Self::Value, E> {
+        if v {
+            Ok(KnownFailure::TraceOutput)
+        } else {
+            Ok(KnownFailure::None)
+        }
+    }
+
+    fn visit_map<A: de::MapAccess<'de>>(self, map: A) -> Result<Self::Value, A::Error> {
+        #[derive(Deserialize)]
+        #[serde(deny_unknown_fields)]
+        enum Raw {
+            #[serde(rename = "panic")]
+            Panic(String),
+        }
+
+        match Raw::deserialize(MapAccessDeserializer::new(map))? {
+            Raw::Panic(message) => Ok(KnownFailure::Panic { message }),
+        }
+    }
+}

tests/framework/src/runner.rs

@@ -8,6 +8,7 @@ use crate::fs_commands::{FsCommand, TestFsCommandProvider};
 use crate::image_trigger::ImageTrigger;
 use crate::options::TestOptions;
 use crate::options::image_comparison::ImageComparison;
+use crate::options::known_failure::KnownFailure;
 use crate::runner::automation::perform_automated_event;
 use crate::runner::image_test::capture_and_compare_image;
 use crate::runner::trace::compare_trace_output;
@@ -20,6 +21,8 @@ use ruffle_core::{Player, PlayerBuilder};
 use ruffle_input_format::InputInjector;
 use ruffle_render::backend::{RenderBackend, ViewportDimensions};
 use ruffle_socket_format::SocketEvent;
+use std::any::Any;
+use std::borrow::Cow;
 use std::collections::HashMap;
 use std::sync::{Arc, Mutex, mpsc};
 use std::time::Duration;
@@ -154,8 +157,57 @@ impl TestRunner {
         self.remaining_iterations == 1
     }
 
-    /// Tick this test forward, running any actionscript and progressing the timeline by one.
-    pub fn tick(&mut self) {
+    pub fn is_preloaded(&self) -> bool {
+        self.preloaded
+    }
+
+    /// Ticks this test forward: runs actionscript, progresses the timeline by one,
+    /// executes custom FsCommands and performs scheduled tests.
+    pub fn tick(&mut self) -> Result<TestStatus> {
+        use std::panic::{AssertUnwindSafe, catch_unwind, resume_unwind};
+
+        let unwind_result = catch_unwind(AssertUnwindSafe(|| self.do_tick()));
+        match (unwind_result, &self.options.known_failure) {
+            (Ok(()), _) => (),
+            (Err(panic), KnownFailure::Panic { message }) => {
+                let actual = panic_payload_as_string(panic);
+                if actual.contains(message) {
+                    return Ok(TestStatus::Finished);
+                }
+
+                let mut actual = actual.into_owned();
+                actual.push_str("\n\nnote: expected panic message to contain: ");
+                actual.push_str(message);
+                resume_unwind(Box::new(actual))
+            }
+            (Err(panic), _) => resume_unwind(panic),
+        }
+
+        self.test()?;
+
+        match self.remaining_iterations {
+            0 => self.last_test().map(|_| TestStatus::Finished),
+            _ if self.options.sleep_to_meet_frame_rate => {
+                // If requested, ensure that the 'expected' amount of
+                // time actually elapses between frames. This is useful for
+                // tests that call 'flash.utils.getTimer()' and use
+                // 'setInterval'/'flash.utils.Timer'
+                //
+                // Note that when Ruffle actually runs frames, we can
+                // execute frames faster than this in order to 'catch up'
+                // if we've fallen behind. However, in order to make regression
+                // tests deterministic, we always call 'update_timers' with
+                // an elapsed time of 'frame_time'. By sleeping for 'frame_time_duration',
+                // we ensure that the result of 'flash.utils.getTimer()' is consistent
+                // with timer execution (timers will see an elapsed time of *at least*
+                // the requested timer interval).
+                Ok(TestStatus::Sleep(self.frame_time_duration))
+            }
+            _ => Ok(TestStatus::Continue),
+        }
+    }
+
+    fn do_tick(&mut self) {
         if !self
             .player
             .lock()
@@ -179,14 +231,10 @@ impl TestRunner {
         self.executor.run();
     }
 
-    pub fn is_preloaded(&self) -> bool {
-        self.preloaded
-    }
-
     /// After a tick, run any custom fdcommands that were queued up and perform any scheduled tests.
-    pub fn test(&mut self) -> Result<TestStatus> {
+    fn test(&mut self) -> Result<()> {
         if !self.preloaded {
-            return Ok(TestStatus::Continue);
+            return Ok(());
         }
         for command in self.fs_commands.try_iter() {
             match command {
@@ -206,7 +254,6 @@ impl TestRunner {
                             &self.player,
                             &name,
                             image_comparison,
-                            self.options.known_failure,
                             self.render_interface.as_deref(),
                         )?;
                     } else {
@@ -225,86 +272,71 @@ impl TestRunner {
         // Rendering has side-effects (such as processing 'DisplayObject.scrollRect' updates)
         self.player.lock().unwrap().render();
 
-        if let Some(name) = self
-            .images
-            .iter()
-            .find(|(_k, v)| v.trigger == ImageTrigger::SpecificIteration(self.current_iteration))
-            .map(|(k, _v)| k.to_owned())
-        {
-            let image_comparison = self
-                .images
-                .remove(&name)
-                .expect("Name was just retrieved from map, should not be missing!");
+        let trigger = ImageTrigger::SpecificIteration(self.current_iteration);
+        if let Some((name, comp)) = self.take_image_comparison_by_trigger(trigger) {
             capture_and_compare_image(
                 &self.root_path,
                 &self.player,
                 &name,
-                image_comparison,
-                self.options.known_failure,
+                comp,
                 self.render_interface.as_deref(),
             )?;
         }
 
-        if self.remaining_iterations == 0 {
-            // Last iteration, let's check everything went well
-
-            if let Some(name) = self
-                .images
-                .iter()
-                .find(|(_k, v)| v.trigger == ImageTrigger::LastFrame)
-                .map(|(k, _v)| k.to_owned())
-            {
-                let image_comparison = self
-                    .images
-                    .remove(&name)
-                    .expect("Name was just retrieved from map, should not be missing!");
-
-                capture_and_compare_image(
-                    &self.root_path,
-                    &self.player,
-                    &name,
-                    image_comparison,
-                    self.options.known_failure,
-                    self.render_interface.as_deref(),
-                )?;
-            }
+        Ok(())
+    }
 
-            if !self.images.is_empty() {
-                return Err(anyhow!(
-                    "Image comparisons didn't trigger: {:?}",
-                    self.images.keys()
-                ));
-            }
+    fn last_test(&mut self) -> Result<()> {
+        // Last iteration, let's check everything went well
+        if let KnownFailure::Panic { .. } = &self.options.known_failure {
+            return Err(anyhow!(
+                "Test was known to be panicking, but now finishes successfully. \
+                Please update it and remove `known_failure.panic = '...'`!",
+            ));
+        }
 
-            self.executor.run();
+        let trigger = ImageTrigger::LastFrame;
+        if let Some((name, comp)) = self.take_image_comparison_by_trigger(trigger) {
+            capture_and_compare_image(
+                &self.root_path,
+                &self.player,
+                &name,
+                comp,
+                self.render_interface.as_deref(),
+            )?;
+        }
 
-            let trace = self.log.trace_output();
-            // Null bytes are invisible, and interfere with constructing
-            // the expected output.txt file. Any tests dealing with null
-            // bytes should explicitly test for them in ActionScript.
-            let normalized_trace = trace.replace('\0', "");
-            compare_trace_output(&self.output_path, &self.options, &normalized_trace)?;
+        if !self.images.is_empty() {
+            return Err(anyhow!(
+                "Image comparisons didn't trigger: {:?}",
+                self.images.keys()
+            ));
         }
 
-        Ok(match self.remaining_iterations {
-            0 => TestStatus::Finished,
-            _ if self.options.sleep_to_meet_frame_rate => {
-                // If requested, ensure that the 'expected' amount of
-                // time actually elapses between frames. This is useful for
-                // tests that call 'flash.utils.getTimer()' and use
-                // 'setInterval'/'flash.utils.Timer'
-                //
-                // Note that when Ruffle actually runs frames, we can
-                // execute frames faster than this in order to 'catch up'
-                // if we've fallen behind. However, in order to make regression
-                // tests deterministic, we always call 'update_timers' with
-                // an elapsed time of 'frame_time'. By sleeping for 'frame_time_duration',
-                // we ensure that the result of 'flash.utils.getTimer()' is consistent
-                // with timer execution (timers will see an elapsed time of *at least*
-                // the requested timer interval).
-                TestStatus::Sleep(self.frame_time_duration)
-            }
-            _ => TestStatus::Continue,
-        })
+        self.executor.run();
+
+        compare_trace_output(
+            &self.log,
+            &self.output_path,
+            self.options.approximations.as_ref(),
+            matches!(self.options.known_failure, KnownFailure::TraceOutput),
+        )
+    }
+
+    fn take_image_comparison_by_trigger(
+        &mut self,
+        trigger: ImageTrigger,
+    ) -> Option<(String, ImageComparison)> {
+        self.images.extract_if(|_k, v| v.trigger == trigger).next()
+    }
+}
+
+fn panic_payload_as_string(panic: Box<dyn Any + Send + 'static>) -> Cow<'static, str> {
+    if let Some(s) = panic.downcast_ref::<&str>() {
+        (*s).into()
+    } else if let Ok(s) = panic.downcast::<String>() {
+        (*s).into()
+    } else {
+        "<opaque payload>".into()
     }
 }