tests: Move known_failure logic inside of TestRunner

This'll make it easier to tweak it in the future.

Author: moulins

tests/framework/src/runner.rs

@@ -154,8 +154,64 @@ 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.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),
+        }
+    }
+    
+    fn tick_inner(&mut self) -> Result<TestStatus> {
+        self.do_tick();
+
+        match (self.test(), self.options.known_failure) {
+            (Ok(()), _) => (),
+            (Err(_), true) => return Ok(TestStatus::Finished),
+            (Err(err), false) => return Err(err),
+        }
+
+        match self.remaining_iterations {
+            0 => match (self.last_test(), self.options.known_failure) {
+                (Ok(()), true) => 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),
+            },
+            _ 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 +235,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 {
@@ -225,86 +277,58 @@ 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,
+                comp,
                 self.options.known_failure,
                 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
 
-            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.options.known_failure,
+                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();
+
+        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(())
+    }
+
+    fn take_image_comparison_by_trigger(
+        &mut self,
+        trigger: ImageTrigger,
+    ) -> Option<(String, ImageComparison)> {
+        self.images.extract_if(|_k, v| v.trigger == trigger).next()
     }
 }

tests/tests/external_interface/tests.rs

@@ -30,17 +30,16 @@ pub fn external_interface_avm1(
     let mut first = true;
 
     loop {
-        runner.tick();
-        if !runner.is_preloaded() {
-            continue;
-        }
-
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
         }
 
+        if !runner.is_preloaded() {
+            continue;
+        }
+
         if first {
             first = false;
             let mut player_locked = runner.player().lock().unwrap();
@@ -102,17 +101,16 @@ pub fn external_interface_avm2(
     let mut first = true;
 
     loop {
-        runner.tick();
-        if !runner.is_preloaded() {
-            continue;
-        }
-
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
         }
 
+        if !runner.is_preloaded() {
+            continue;
+        }
+
         if first {
             first = false;
             let mut player_locked = runner.player().lock().unwrap();

tests/tests/regression_tests.rs

@@ -6,7 +6,6 @@ use crate::environment::NativeEnvironment;
 use crate::external_interface::tests::{external_interface_avm1, external_interface_avm2};
 use crate::shared_object::{shared_object_avm1, shared_object_avm2, shared_object_self_ref_avm1};
 use anyhow::Context;
-use anyhow::Result;
 use clap::Parser;
 use libtest_mimic::Trial;
 use ruffle_fs_tests_runner::FsTestsRunner;
@@ -15,7 +14,6 @@ use ruffle_test_framework::runner::TestStatus;
 use ruffle_test_framework::test::Test;
 use ruffle_test_framework::vfs::VfsPath;
 use std::borrow::Cow;
-use std::panic::{AssertUnwindSafe, catch_unwind, resume_unwind};
 use std::path::PathBuf;
 use std::thread::sleep;
 
@@ -116,32 +114,13 @@ fn trial_for_test(opts: &RuffleTestOpts, test: Test, list_only: bool) -> Trial {
     }
 
     let trial = Trial::test(test.name.clone(), move || {
-        let test = AssertUnwindSafe(test);
-        let unwind_result = catch_unwind(|| {
-            let mut runner = test.create_test_runner(&NativeEnvironment)?;
-
-            loop {
-                runner.tick();
-                match runner.test()? {
-                    TestStatus::Continue => {}
-                    TestStatus::Sleep(duration) => sleep(duration),
-                    TestStatus::Finished => break,
-                }
-            }
+        let mut runner = test.create_test_runner(&NativeEnvironment)?;
 
-            Result::<_>::Ok(())
-        });
-        if test.options.known_failure {
-            match unwind_result {
-                Ok(Ok(())) => Err(
-                    format!("{} was known to be failing, but now passes successfully. Please update it and remove `known_failure = true`!", test.name).into()
-                ),
-                Ok(Err(_)) | Err(_) => Ok(()),
-            }
-        } else {
-            match unwind_result {
-                Ok(r) => Ok(r?),
-                Err(e) => resume_unwind(e),
+        loop {
+            match runner.tick()? {
+                TestStatus::Continue => (),
+                TestStatus::Sleep(duration) => sleep(duration),
+                TestStatus::Finished => break Ok(()),
             }
         }
     });

tests/tests/shared_object/mod.rs

@@ -25,8 +25,7 @@ pub fn shared_object_avm1(environment: &impl Environment) -> Result<(), libtest_
     let mut runner = test1.create_test_runner(environment)?;
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
@@ -67,8 +66,7 @@ pub fn shared_object_avm1(environment: &impl Environment) -> Result<(), libtest_
     }
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
@@ -99,8 +97,7 @@ pub fn shared_object_self_ref_avm1(
     let mut runner = test1.create_test_runner(environment)?;
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
@@ -140,8 +137,7 @@ pub fn shared_object_self_ref_avm1(
     }
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
@@ -170,8 +166,7 @@ pub fn shared_object_avm2(environment: &impl Environment) -> Result<(), libtest_
     let mut runner = test1.create_test_runner(environment)?;
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,
@@ -211,8 +206,7 @@ pub fn shared_object_avm2(environment: &impl Environment) -> Result<(), libtest_
     }
 
     loop {
-        runner.tick();
-        match runner.test()? {
+        match runner.tick()? {
             TestStatus::Continue => {}
             TestStatus::Sleep(duration) => sleep(duration),
             TestStatus::Finished => break,