tests: distinguish between panicky and non-panicky known_failure tests

Tests that are expected to panic are now required to explicitely say so
in their `test.toml`, by using `known_failure.panic = "expected message"`.

Additionally, only panics raised during player ticking are caught, and not those
happenning elsewhere (e.g. in rendering, or in test framework code).

Author: moulins

tests/README.md

@@ -37,6 +37,9 @@ ignore = false
 
 # If true, this test is known to fail and the test runner will expect it 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.
 known_failure = false
 
 # Path (relative to the directory containing test.toml) to the expected output

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,10 @@ impl TestOptions {
         Ok(())
     }
 
+    pub fn has_known_failure(&self) -> bool {
+        !matches!(self.known_failure, KnownFailure::None)
+    }
+
     pub fn output_path(&self, test_directory: &VfsPath) -> Result<VfsPath> {
         Ok(test_directory.join(&self.output_path)?)
     }

tests/framework/src/options/known_failure.rs

@@ -0,0 +1,52 @@
+use std::fmt;
+
+use serde::{
+    Deserialize, Deserializer,
+    de::{self, value::MapAccessDeserializer},
+};
+
+#[derive(Clone, Debug, Default)]
+pub enum KnownFailure {
+    #[default]
+    None,
+    AnyCheck,
+    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> {
+        Ok(match v {
+            true => KnownFailure::AnyCheck,
+            false => 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;
@@ -163,33 +166,39 @@ impl TestRunner {
     pub fn tick(&mut self) -> Result<TestStatus> {
         use std::panic::{AssertUnwindSafe, catch_unwind, resume_unwind};
 
-        let unwind_result = catch_unwind(AssertUnwindSafe(|| self.tick_inner()));
-    
-        match (unwind_result, self.options.known_failure) {
-            (Ok(ret), _) => ret,
-            // known_failure tests may pass by panicking.
-            (Err(_), true) =>  Ok(TestStatus::Finished),
-            (Err(panic), false) => resume_unwind(panic),
+        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),
         }
-    }
-    
-    fn tick_inner(&mut self) -> Result<TestStatus> {
-        self.do_tick();
 
-        match (self.test(), self.options.known_failure) {
+        match (self.test(), &self.options.known_failure) {
             (Ok(()), _) => (),
-            (Err(_), true) => return Ok(TestStatus::Finished),
-            (Err(err), false) => return Err(err),
+            (Err(_), KnownFailure::AnyCheck) => return Ok(TestStatus::Finished),
+            (Err(err), _) => return Err(err),
         }
 
-        match self.remaining_iterations {
-            0 => match (self.last_test(), self.options.known_failure) {
-                (Ok(()), true) => Err(anyhow!(
+        match (self.remaining_iterations, &self.options.known_failure) {
+            (0, KnownFailure::None) => self.last_test().map(|_| TestStatus::Finished),
+            (0, KnownFailure::Panic { .. }) => Err(anyhow!(
+                "Test was known to be panicking, but now finishes successfully. Please update it and remove `known_failure.panic = '...'`!",
+            )),
+            (0, KnownFailure::AnyCheck) => match self.last_test() {
+                Ok(()) => Err(anyhow!(
                     "Test was known to be failing, but now passes successfully. Please update it and remove `known_failure = true`!",
                 )),
-                (Ok(()), false) => Ok(TestStatus::Finished),
-                (Err(_), true) => Ok(TestStatus::Finished),
-                (Err(err), false) => Err(err),
+                Err(_) => Ok(TestStatus::Finished),
             },
             _ if self.options.sleep_to_meet_frame_rate => {
                 // If requested, ensure that the 'expected' amount of
@@ -258,7 +267,7 @@ impl TestRunner {
                             &self.player,
                             &name,
                             image_comparison,
-                            self.options.known_failure,
+                            matches!(self.options.known_failure, KnownFailure::AnyCheck),
                             self.render_interface.as_deref(),
                         )?;
                     } else {
@@ -284,7 +293,7 @@ impl TestRunner {
                 &self.player,
                 &name,
                 comp,
-                self.options.known_failure,
+                matches!(self.options.known_failure, KnownFailure::AnyCheck),
                 self.render_interface.as_deref(),
             )?;
         }
@@ -302,7 +311,7 @@ impl TestRunner {
                 &self.player,
                 &name,
                 comp,
-                self.options.known_failure,
+                matches!(self.options.known_failure, KnownFailure::AnyCheck),
                 self.render_interface.as_deref(),
             )?;
         }
@@ -332,3 +341,13 @@ impl TestRunner {
         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()
+    }
+}

tests/framework/src/test.rs

@@ -142,7 +142,7 @@ impl Test {
         if self.options.ignore {
             return false;
         }
-        if ignore_known_failures && self.options.known_failure {
+        if ignore_known_failures && self.options.has_known_failure() {
             return false;
         }
         self.options.required_features.can_run()

tests/tests/regression_tests.rs

@@ -106,7 +106,7 @@ fn trial_for_test(opts: &RuffleTestOpts, test: Test, list_only: bool) -> Trial {
     // Put extra info into the test 'kind' instead of appending it to the test name,
     // to not break `cargo test some/test -- --exact` and `cargo test -- --list`.
     let mut test_kind = String::new();
-    if test.options.known_failure {
+    if test.options.has_known_failure() {
         test_kind.push('!');
     }
     if let Some(name) = &test.options.subtest_name {

tests/tests/swfs/avm1/bitmap_data_thorough/pixelDissolve/test.toml

@@ -1,2 +1,2 @@
 num_frames = 1
-known_failure = true
+known_failure.panic = "attempt to add with overflow"

tests/tests/swfs/from_gnash/misc-ming.all/action_order/PlaceAndRemove/test.toml

@@ -1,5 +1,5 @@
 num_frames = 30
-known_failure = true
+known_failure.panic = "Gotos must start from the correct tag position for frame 1"
 
 [subtests.fp9]
 output_path = "output.fp9.txt"

tests/tests/swfs/from_gnash/misc-ming.all/get_frame_number_test/test.toml

@@ -1,2 +1,2 @@
 num_frames = 30
-known_failure = true
+known_failure.panic = "attempt to add with overflow"

tests/tests/swfs/from_gnash/misc-swfc.all/matrix_accuracy_test1/test.toml

@@ -1,5 +1,5 @@
 num_frames = 30
-known_failure = true
+known_failure.panic = "attempt to subtract with overflow"
 
 [subtests.fp9]
 output_path = "output.fp9.txt"