tests: make known_failure = true check-specific

Root-level `known_failure = true` now only except the trace output check to
fail; if other checks (e.g. image comparisons) are expected to fail, they should
specify their own `known_failure = true`.

Author: moulins

tests/README.md

@@ -35,7 +35,8 @@ 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"`
@@ -100,6 +101,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

@@ -182,6 +182,7 @@ impl TestOptions {
 
     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> {

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

@@ -9,7 +9,7 @@ use serde::{
 pub enum KnownFailure {
     #[default]
     None,
-    AnyCheck,
+    TraceOutput,
     Panic {
         message: String,
     },
@@ -32,7 +32,7 @@ impl<'de> de::Visitor<'de> for KnownFailureVisitor {
 
     fn visit_bool<E: de::Error>(self, v: bool) -> Result<Self::Value, E> {
         if v {
-            Ok(KnownFailure::AnyCheck)
+            Ok(KnownFailure::TraceOutput)
         } else {
             Ok(KnownFailure::None)
         }

tests/framework/src/runner.rs

@@ -183,23 +183,10 @@ impl TestRunner {
             (Err(panic), _) => resume_unwind(panic),
         }
 
-        match (self.test(), &self.options.known_failure) {
-            (Ok(()), _) => (),
-            (Err(_), KnownFailure::AnyCheck) => return Ok(TestStatus::Finished),
-            (Err(err), _) => return Err(err),
-        }
+        self.test()?;
 
-        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`!",
-                )),
-                Err(_) => Ok(TestStatus::Finished),
-            },
+        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
@@ -267,7 +254,6 @@ impl TestRunner {
                             &self.player,
                             &name,
                             image_comparison,
-                            matches!(self.options.known_failure, KnownFailure::AnyCheck),
                             self.render_interface.as_deref(),
                         )?;
                     } else {
@@ -293,7 +279,6 @@ impl TestRunner {
                 &self.player,
                 &name,
                 comp,
-                matches!(self.options.known_failure, KnownFailure::AnyCheck),
                 self.render_interface.as_deref(),
             )?;
         }
@@ -303,6 +288,12 @@ impl TestRunner {
 
     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 = '...'`!",
+            ));
+        }
 
         let trigger = ImageTrigger::LastFrame;
         if let Some((name, comp)) = self.take_image_comparison_by_trigger(trigger) {
@@ -311,7 +302,6 @@ impl TestRunner {
                 &self.player,
                 &name,
                 comp,
-                matches!(self.options.known_failure, KnownFailure::AnyCheck),
                 self.render_interface.as_deref(),
             )?;
         }
@@ -325,13 +315,12 @@ impl TestRunner {
 
         self.executor.run();
 
-        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)?;
-        Ok(())
+        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(

tests/framework/src/runner/image_test.rs

@@ -14,49 +14,56 @@ pub fn capture_and_compare_image(
     player: &Arc<Mutex<Player>>,
     name: &String,
     image_comparison: ImageComparison,
-    known_failure: bool,
     render_interface: Option<&dyn RenderInterface>,
 ) -> anyhow::Result<()> {
     use anyhow::Context;
 
-    if let Some(render_interface) = render_interface {
+    let Some(render_interface) = render_interface else {
+        return Ok(());
+    };
+
+    let actual_image = {
         let mut player_lock = player.lock().unwrap();
         player_lock.render();
+        render_interface.capture(player_lock.renderer_mut())
+    };
 
-        let actual_image = render_interface.capture(player_lock.renderer_mut());
-
-        let expected_image_path = base_path.join(format!("{name}.expected.png"))?;
-        if expected_image_path.is_file()? {
-            let expected_image = image::load_from_memory(&read_bytes(&expected_image_path)?)
+    let expected_image = {
+        let path = base_path.join(format!("{name}.expected.png"))?;
+        if path.is_file()? {
+            image::load_from_memory(&read_bytes(&path)?)
                 .context("Failed to open expected image")?
-                .into_rgba8();
-
-            test(
-                &image_comparison,
-                name,
-                actual_image,
-                expected_image,
-                base_path,
-                render_interface.name(),
-                known_failure,
-            )?;
-        } else if known_failure {
+                .into_rgba8()
+        } else if image_comparison.known_failure {
+            // If we're expecting this to be wrong, don't save a likely wrong image
+            return Err(anyhow!("Image '{name}': No image to compare to!"));
+        } else {
+            write_image(&path, &actual_image, ImageFormat::Png)?;
             return Err(anyhow!(
-                "No image to compare to, pretending this failed since we don't know if it worked."
+                "Image '{name}': No image to compare to! Saved actual image as expected."
             ));
-        } else {
-            // If we're expecting this to be wrong, don't save a likely wrong image
-            write_image(&expected_image_path, &actual_image, ImageFormat::Png)?;
         }
-    } else if known_failure {
-        // It's possible that the trace output matched but the image might not.
-        // If we aren't checking the image, pretend the match failed (which makes it actually pass, since it's expecting failure).
-        return Err(anyhow!(
-            "Not checking images, pretending this failed since we don't know if it worked."
-        ));
-    }
+    };
 
-    Ok(())
+    let result = test(
+        &image_comparison,
+        name,
+        actual_image,
+        expected_image,
+        base_path,
+        render_interface.name(),
+        // If we're expecting failure, spamming files isn't productive.
+        !image_comparison.known_failure,
+    );
+
+    match (result, image_comparison.known_failure) {
+        (result, false) => result,
+        (Ok(()), true) => Err(anyhow!(
+            "Image '{name}': Check was known to be failing, but now passes successfully. \
+            Please update the test and remove `known_failure = true`!",
+        )),
+        (Err(_), true) => Ok(()),
+    }
 }
 
 pub fn test(
@@ -66,13 +73,12 @@ pub fn test(
     expected_image: image::RgbaImage,
     test_path: &VfsPath,
     environment_name: String,
-    known_failure: bool,
+    save_failures: bool,
 ) -> anyhow::Result<()> {
     use anyhow::Context;
 
     let save_actual_image = || {
-        if !known_failure {
-            // If we're expecting failure, spamming files isn't productive.
+        if save_failures {
             write_image(
                 &test_path.join(format!("{name}.actual-{environment_name}.png"))?,
                 &actual_image,
@@ -141,8 +147,7 @@ pub fn test(
             difference_color.extend_from_slice(&p[..3]);
         }
 
-        if !known_failure {
-            // If we're expecting failure, spamming files isn't productive.
+        if save_failures {
             let difference_image = image::RgbImage::from_raw(
                 actual_image.width(),
                 actual_image.height(),
@@ -163,8 +168,7 @@ pub fn test(
                 difference_alpha.push(p[3])
             }
 
-            if !known_failure {
-                // If we're expecting failure, spamming files isn't productive.
+            if save_failures {
                 let difference_image = image::GrayImage::from_raw(
                     actual_image.width(),
                     actual_image.height(),

tests/framework/src/runner/trace.rs

@@ -1,19 +1,42 @@
-use crate::options::TestOptions;
+use crate::backends::TestLogBackend;
+use crate::options::approximations::Approximations;
 use anyhow::{Error, anyhow};
 use pretty_assertions::Comparison;
 use vfs::VfsPath;
 
 pub fn compare_trace_output(
+    log: &TestLogBackend,
     expected_path: &VfsPath,
-    options: &TestOptions,
-    actual_output: &str,
+    approximations: Option<&Approximations>,
+    known_failure: bool,
 ) -> anyhow::Result<()> {
-    let expected_output = expected_path.read_to_string()?.replace("\r\n", "\n");
+    let expected_trace = expected_path.read_to_string()?.replace("\r\n", "\n");
+
+    // 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 actual_trace = log.trace_output().replace('\0', "");
 
-    if let Some(approximations) = &options.approximations {
+    let result = test(&expected_trace, approximations, &actual_trace);
+    match (result, known_failure) {
+        (res, false) => res,
+        (Ok(()), true) => Err(anyhow!(
+            "Trace output check was known to be failing, but now passes successfully. \
+            Please update the test and remove `known_failure = true`!",
+        )),
+        (Err(_), true) => Ok(()),
+    }
+}
+
+pub fn test(
+    expected_output: &str,
+    approximations: Option<&Approximations>,
+    actual_output: &str,
+) -> anyhow::Result<()> {
+    if let Some(approximations) = approximations {
         let add_comparison_to_err = |err: Error| -> Error {
             let left_pretty = PrettyString(actual_output);
-            let right_pretty = PrettyString(&expected_output);
+            let right_pretty = PrettyString(expected_output);
             let comparison = Comparison::new(&left_pretty, &right_pretty);
 
             anyhow!("{}\n\n{}\n", err, comparison)
@@ -90,7 +113,7 @@ pub fn compare_trace_output(
             }
         }
     } else {
-        assert_text_matches(actual_output, &expected_output)?;
+        assert_text_matches(actual_output, expected_output)?;
     }
 
     Ok(())

tests/tests/swfs/avm2/bitmapdata_draw_filters/test.toml

@@ -1,9 +1,9 @@
 num_ticks = 1
 
+[image_comparisons.output]
 # FIXME Ruffle does not use CAB in BitmapData.draw
 known_failure = true
 
-[image_comparisons.output]
 tolerance = 0
 
 [player_options]

tests/tests/swfs/fonts/embed_matching/no_font_found/test.toml

@@ -1,9 +1,9 @@
 # There are no fonts embedded in this swf. It should not render anything at all, or error.
 
 num_frames = 1
-known_failure = true # Right now we intentionally fall back, because we don't support DefineFont4 embedded fonts yet
 
 [image_comparisons.output]
+known_failure = true # Right now we intentionally fall back, because we don't support DefineFont4 embedded fonts yet
 tolerance = 0
 
 [player_options]

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

@@ -1,7 +1,7 @@
 num_frames = 30
-known_failure = true
 
 [image_comparisons.output]
+known_failure = true
 tolerance = 50
 max_outliers = 100