Enforce consistent snapshot updates (#7744)

* Closes #7647


This collects SnapshotResults within the Harness and adds a check to
enforce snapshot results are merged in case multiple Harnesses are
constructed within a test.

This should make snapshot updates via kitdiff/accept_snapshots.sh way
more useful since it should now always update all snapshots instead of
only the first one per test.

Author: lucasmerlin

crates/egui_demo_lib/src/demo/tests/tessellation_test.rs

@@ -357,11 +357,13 @@ fn rect_shape_ui(ui: &mut egui::Ui, shape: &mut RectShape) {
 #[cfg(test)]
 mod tests {
     use crate::View as _;
+    use egui_kittest::SnapshotResults;
 
     use super::*;
 
     #[test]
     fn snapshot_tessellation_test() {
+        let mut results = SnapshotResults::new();
         for (name, shape) in TessellationTest::interesting_shapes() {
             let mut test = TessellationTest {
                 shape,
@@ -375,6 +377,7 @@ mod tests {
             harness.run();
 
             harness.snapshot(format!("tessellation_test/{name}"));
+            results.extend_harness(&mut harness);
         }
     }
 }

crates/egui_demo_lib/src/demo/widget_gallery.rs

@@ -310,7 +310,7 @@ mod tests {
     use super::*;
     use crate::View as _;
     use egui::Vec2;
-    use egui_kittest::Harness;
+    use egui_kittest::{Harness, SnapshotResults};
 
     #[test]
     pub fn should_match_screenshot() {
@@ -320,6 +320,8 @@ mod tests {
             ..Default::default()
         };
 
+        let mut results = SnapshotResults::new();
+
         for pixels_per_point in [1, 2] {
             for theme in [egui::Theme::Light, egui::Theme::Dark] {
                 let mut harness = Harness::builder()
@@ -339,6 +341,7 @@ mod tests {
                 };
                 let image_name = format!("widget_gallery_{theme_name}_x{pixels_per_point}");
                 harness.snapshot(&image_name);
+                results.extend_harness(&mut harness);
             }
         }
     }

crates/egui_demo_lib/tests/image_blending.rs

@@ -3,6 +3,7 @@ use egui_kittest::Harness;
 
 #[test]
 fn test_image_blending() {
+    let mut results = egui_kittest::SnapshotResults::new();
     for pixels_per_point in [1.0, 2.0] {
         let mut harness = Harness::builder()
             .with_pixels_per_point(pixels_per_point)
@@ -21,5 +22,6 @@ fn test_image_blending() {
         harness.run();
         harness.fit_contents();
         harness.snapshot(format!("image_blending/image_x{pixels_per_point}"));
+        results.extend_harness(&mut harness);
     }
 }

crates/egui_demo_lib/tests/misc.rs

@@ -3,6 +3,7 @@ use egui_kittest::{Harness, kittest::Queryable as _};
 
 #[test]
 fn test_kerning() {
+    let mut results = egui_kittest::SnapshotResults::new();
     for pixels_per_point in [1.0, 2.0] {
         for theme in [egui::Theme::Dark, egui::Theme::Light] {
             let mut harness = Harness::builder()
@@ -24,12 +25,14 @@ fn test_kerning() {
                     egui::Theme::Light => "light",
                 }
             ));
+            results.extend_harness(&mut harness);
         }
     }
 }
 
 #[test]
 fn test_italics() {
+    let mut results = egui_kittest::SnapshotResults::new();
     for pixels_per_point in [1.0, 2.0_f32.sqrt(), 2.0] {
         for theme in [egui::Theme::Dark, egui::Theme::Light] {
             let mut harness = Harness::builder()
@@ -49,6 +52,7 @@ fn test_italics() {
                     egui::Theme::Light => "light",
                 }
             ));
+            results.extend_harness(&mut harness);
         }
     }
 }

crates/egui_kittest/src/builder.rs

@@ -166,6 +166,7 @@ impl<State> HarnessBuilder<State> {
     ///
     /// assert_eq!(*harness.state(), true);
     /// ```
+    #[track_caller]
     pub fn build_state<'a>(
         self,
         app: impl FnMut(&egui::Context, &mut State) + 'a,
@@ -195,6 +196,7 @@ impl<State> HarnessBuilder<State> {
     ///
     /// assert_eq!(*harness.state(), true);
     /// ```
+    #[track_caller]
     pub fn build_ui_state<'a>(
         self,
         app: impl FnMut(&mut egui::Ui, &mut State) + 'a,
@@ -206,6 +208,7 @@ impl<State> HarnessBuilder<State> {
     /// Create a new [Harness] from the given eframe creation closure.
     /// The app can be accessed via the [`Harness::state`] / [`Harness::state_mut`] methods.
     #[cfg(feature = "eframe")]
+    #[track_caller]
     pub fn build_eframe<'a>(
         self,
         build: impl FnOnce(&mut eframe::CreationContext<'a>) -> State,
@@ -247,6 +250,7 @@ impl HarnessBuilder {
     ///     });
     /// ```
     #[must_use]
+    #[track_caller]
     pub fn build<'a>(self, app: impl FnMut(&egui::Context) + 'a) -> Harness<'a> {
         Harness::from_builder(self, AppKind::Context(Box::new(app)), (), None)
     }
@@ -267,6 +271,7 @@ impl HarnessBuilder {
     ///     });
     /// ```
     #[must_use]
+    #[track_caller]
     pub fn build_ui<'a>(self, app: impl FnMut(&mut egui::Ui) + 'a) -> Harness<'a> {
         Harness::from_builder(self, AppKind::Ui(Box::new(app)), (), None)
     }

crates/egui_kittest/src/lib.rs

@@ -84,6 +84,8 @@ pub struct Harness<'a, State = ()> {
 
     #[cfg(feature = "snapshot")]
     default_snapshot_options: SnapshotOptions,
+    #[cfg(feature = "snapshot")]
+    snapshot_results: SnapshotResults,
 }
 
 impl<State> Debug for Harness<'_, State> {
@@ -93,6 +95,7 @@ impl<State> Debug for Harness<'_, State> {
 }
 
 impl<'a, State> Harness<'a, State> {
+    #[track_caller]
     pub(crate) fn from_builder(
         builder: HarnessBuilder<State>,
         mut app: AppKind<'a, State>,
@@ -162,6 +165,9 @@ impl<'a, State> Harness<'a, State> {
 
             #[cfg(feature = "snapshot")]
             default_snapshot_options,
+
+            #[cfg(feature = "snapshot")]
+            snapshot_results: SnapshotResults::default(),
         };
         // Run the harness until it is stable, ensuring that all Areas are shown and animations are done
         harness.run_ok();
@@ -197,6 +203,7 @@ impl<'a, State> Harness<'a, State> {
     ///
     /// assert_eq!(*harness.state(), true);
     /// ```
+    #[track_caller]
     pub fn new_state(app: impl FnMut(&egui::Context, &mut State) + 'a, state: State) -> Self {
         Self::builder().build_state(app, state)
     }
@@ -222,12 +229,14 @@ impl<'a, State> Harness<'a, State> {
     ///
     /// assert_eq!(*harness.state(), true);
     /// ```
+    #[track_caller]
     pub fn new_ui_state(app: impl FnMut(&mut egui::Ui, &mut State) + 'a, state: State) -> Self {
         Self::builder().build_ui_state(app, state)
     }
 
     /// Create a new [Harness] from the given eframe creation closure.
     #[cfg(feature = "eframe")]
+    #[track_caller]
     pub fn new_eframe(builder: impl FnOnce(&mut eframe::CreationContext<'a>) -> State) -> Self
     where
         State: eframe::App,
@@ -725,6 +734,7 @@ impl<'a> Harness<'a> {
     ///     });
     /// });
     /// ```
+    #[track_caller]
     pub fn new(app: impl FnMut(&egui::Context) + 'a) -> Self {
         Self::builder().build(app)
     }
@@ -745,6 +755,7 @@ impl<'a> Harness<'a> {
     ///     ui.label("Hello, world!");
     /// });
     /// ```
+    #[track_caller]
     pub fn new_ui(app: impl FnMut(&mut egui::Ui) + 'a) -> Self {
         Self::builder().build_ui(app)
     }

crates/egui_kittest/src/snapshot.rs

@@ -663,16 +663,16 @@ impl<State> Harness<'_, State> {
     /// If the new image didn't match the snapshot, a diff image will be saved under `{output_path}/{name}.diff.png`.
     ///
     /// # Panics
-    /// Panics if the image does not match the snapshot, if there was an error reading or writing the
+    /// The result is added to the [`Harness`]'s internal [`SnapshotResults`].
+    ///
+    /// The harness will panic when dropped if there were any snapshot errors.
+    ///
+    /// Errors happen if the image does not match the snapshot, if there was an error reading or writing the
     /// snapshot, if the rendering fails or if no default renderer is available.
     #[track_caller]
     pub fn snapshot_options(&mut self, name: impl Into<String>, options: &SnapshotOptions) {
-        match self.try_snapshot_options(name, options) {
-            Ok(_) => {}
-            Err(err) => {
-                panic!("{err}");
-            }
-        }
+        let result = self.try_snapshot_options(name, options);
+        self.snapshot_results.add(result);
     }
 
     /// Render an image using the setup [`crate::TestRenderer`] and compare it to the snapshot.
@@ -688,12 +688,8 @@ impl<State> Harness<'_, State> {
     /// snapshot, if the rendering fails or if no default renderer is available.
     #[track_caller]
     pub fn snapshot(&mut self, name: impl Into<String>) {
-        match self.try_snapshot(name) {
-            Ok(_) => {}
-            Err(err) => {
-                panic!("{err}");
-            }
-        }
+        let result = self.try_snapshot(name);
+        self.snapshot_results.add(result);
     }
 
     /// Render a snapshot, save it to a temp file and open it in the default image viewer.
@@ -739,6 +735,12 @@ impl<State> Harness<'_, State> {
             }
         }
     }
+
+    /// This removes the snapshot results from the harness. Useful if you e.g. want to merge it
+    /// with the results from another harness (using [`SnapshotResults::add`]).
+    pub fn take_snapshot_results(&mut self) -> SnapshotResults {
+        std::mem::take(&mut self.snapshot_results)
+    }
 }
 
 /// Utility to collect snapshot errors and display them at the end of the test.
@@ -765,9 +767,22 @@ impl<State> Harness<'_, State> {
 /// Panics if there are any errors when dropped (this way it is impossible to forget to call `unwrap`).
 /// If you don't want to panic, you can use [`SnapshotResults::into_result`] or [`SnapshotResults::into_inner`].
 /// If you want to panic early, you can use [`SnapshotResults::unwrap`].
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct SnapshotResults {
     errors: Vec<SnapshotError>,
+    handled: bool,
+    location: std::panic::Location<'static>,
+}
+
+impl Default for SnapshotResults {
+    #[track_caller]
+    fn default() -> Self {
+        Self {
+            errors: Vec::new(),
+            handled: true, // If no snapshots were added, we should consider this handled.
+            location: *std::panic::Location::caller(),
+        }
+    }
 }
 
 impl Display for SnapshotResults {
@@ -785,17 +800,30 @@ impl Display for SnapshotResults {
 }
 
 impl SnapshotResults {
+    #[track_caller]
     pub fn new() -> Self {
         Default::default()
     }
 
     /// Check if the result is an error and add it to the list of errors.
     pub fn add(&mut self, result: SnapshotResult) {
+        self.handled = false;
         if let Err(err) = result {
             self.errors.push(err);
         }
     }
 
+    /// Add all errors from another `SnapshotResults`.
+    pub fn extend(&mut self, other: Self) {
+        self.handled = false;
+        self.errors.extend(other.into_inner());
+    }
+
+    /// Add all errors from a [`Harness`].
+    pub fn extend_harness<T>(&mut self, harness: &mut Harness<'_, T>) {
+        self.extend(harness.take_snapshot_results());
+    }
+
     /// Check if there are any errors.
     pub fn has_errors(&self) -> bool {
         !self.errors.is_empty()
@@ -807,13 +835,14 @@ impl SnapshotResults {
         if self.has_errors() { Err(self) } else { Ok(()) }
     }
 
+    /// Consume this and return the list of errors.
     pub fn into_inner(mut self) -> Vec<SnapshotError> {
+        self.handled = true;
         std::mem::take(&mut self.errors)
     }
 
     /// Panics if there are any errors, displaying each.
     #[expect(clippy::unused_self)]
-    #[track_caller]
     pub fn unwrap(self) {
         // Panic is handled in drop
     }
@@ -826,7 +855,6 @@ impl From<SnapshotResults> for Vec<SnapshotError> {
 }
 
 impl Drop for SnapshotResults {
-    #[track_caller]
     fn drop(&mut self) {
         // Don't panic if we are already panicking (the test probably failed for another reason)
         if std::thread::panicking() {
@@ -836,5 +864,32 @@ impl Drop for SnapshotResults {
         if self.has_errors() {
             panic!("{}", self);
         }
+
+        thread_local! {
+            static UNHANDLED_SNAPSHOT_RESULTS_COUNTER: std::cell::RefCell<usize> = const { std::cell::RefCell::new(0) };
+        }
+
+        if !self.handled {
+            let count = UNHANDLED_SNAPSHOT_RESULTS_COUNTER.with(|counter| {
+                let mut count = counter.borrow_mut();
+                *count += 1;
+                *count
+            });
+
+            #[expect(clippy::manual_assert)]
+            if count >= 2 {
+                panic!(
+                    r#"
+Multiple SnapshotResults were dropped without being handled.
+
+In order to allow consistent snapshot updates, all snapshot results within a test should be merged in a single SnapshotResults instance.
+Usually this is handled internally in a harness. If you have multiple harnesses, you can merge the results using `Harness::take_snapshot_results` and `SnapshotResults::extend`.
+
+The SnapshotResult was constructed at {}
+                    "#,
+                    self.location
+                );
+            }
+        }
     }
 }