From 520fae19c53ebac00bad533e7ff03f2fa4c651ea Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 5 Jan 2023 11:13:14 +0100 Subject: [PATCH] Implement better force update behavior that ignores no-ops --- src/runtime.rs | 35 +++++++++++++-------- src/snapshot.rs | 81 ++++++++++++++++++++++++++++++++++++------------- 2 files changed, 83 insertions(+), 33 deletions(-) diff --git a/src/runtime.rs b/src/runtime.rs index 1e54f624..15e2cb2f 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -356,8 +356,8 @@ impl<'a> SnapshotAssertionContext<'a> { match snapshot_update { SnapshotUpdateBehavior::InPlace => { if let Some(ref snapshot_file) = self.snapshot_file { - new_snapshot.save(snapshot_file)?; - if should_print { + let saved = new_snapshot.save(snapshot_file)?; + if should_print && saved { elog!( "{} {}", if unseen { @@ -382,15 +382,14 @@ impl<'a> SnapshotAssertionContext<'a> { } SnapshotUpdateBehavior::NewFile => { if let Some(ref snapshot_file) = self.snapshot_file { - let mut new_path = snapshot_file.to_path_buf(); - new_path.set_extension("snap.new"); - new_snapshot.save_new(&new_path)?; - if should_print { - elog!( - "{} {}", - style("stored new snapshot").green(), - style(new_path.display()).cyan().underlined(), - ); + if let Some(new_path) = new_snapshot.save_new(snapshot_file)? { + if should_print { + elog!( + "{} {}", + style("stored new snapshot").green(), + style(new_path.display()).cyan().underlined(), + ); + } } } else if self.is_doctest { if should_print { @@ -401,7 +400,19 @@ impl<'a> SnapshotAssertionContext<'a> { .bold(), ); } - } else { + + // special case for pending inline snapshots. Here we really only want + // to write the contents if the snapshot contents changed as the metadata + // is not retained for inline snapshots. This used to have different + // behavior in the past where we did indeed want to rewrite the snapshots + // entirely since we used to change the canonical snapshot format, but now + // this is significantly less likely to happen and seeing hundreds of unchanged + // inline snapshots in the review screen is not a lot of fun. + } else if self + .old_snapshot + .as_ref() + .map_or(true, |x| x.contents() != new_snapshot.contents()) + { PendingInlineSnapshot::new( Some(new_snapshot), self.old_snapshot.clone(), diff --git a/src/snapshot.rs b/src/snapshot.rs index 83348772..6167a772 100644 --- a/src/snapshot.rs +++ b/src/snapshot.rs @@ -244,6 +244,17 @@ impl MetaData { Content::Struct("MetaData", fields) } + + /// Trims the metadata for persistence. + fn trim_for_persistence(&self) -> Cow<'_, MetaData> { + if self.assertion_line.is_some() { + let mut rv = self.clone(); + rv.assertion_line = None; + Cow::Owned(rv) + } else { + Cow::Borrowed(self) + } + } } /// A helper to work with stored snapshots. @@ -428,37 +439,65 @@ impl Snapshot { &self.snapshot.0 } - fn save_with_metadata(&self, path: &Path, md: &MetaData) -> Result<(), Box> { + fn serialize_snapshot(&self, md: &MetaData) -> String { + let mut buf = yaml::to_string(&md.as_content()); + buf.push_str("---\n"); + buf.push_str(self.contents_str()); + buf.push('\n'); + buf + } + + fn save_with_metadata( + &self, + path: &Path, + ref_file: Option<&Path>, + md: &MetaData, + ) -> Result> { if let Some(folder) = path.parent() { fs::create_dir_all(folder)?; } - let mut f = fs::File::create(path)?; - let blob = yaml::to_string(&md.as_content()); - f.write_all(blob.as_bytes())?; - f.write_all(b"---\n")?; - f.write_all(self.contents_str().as_bytes())?; - f.write_all(b"\n")?; - Ok(()) + + let serialized_snapshot = self.serialize_snapshot(md); + + // check the reference file for contents. Note that we always want to + // compare snapshots that were trimmed to persistence here. + if let Ok(old) = fs::read_to_string(ref_file.unwrap_or(path)) { + let persisted = match md.trim_for_persistence() { + Cow::Owned(trimmed) => Cow::Owned(self.serialize_snapshot(&trimmed)), + Cow::Borrowed(_) => Cow::Borrowed(&serialized_snapshot), + }; + if old == persisted.as_str() { + return Ok(false); + } + } + + fs::write(path, serialized_snapshot)?; + Ok(true) } /// Saves the snapshot. + /// + /// Returns `true` if the snapshot was saved. This will return `false` if there + /// was already a snapshot with matching contents. #[doc(hidden)] - pub fn save(&self, path: &Path) -> Result<(), Box> { - // we do not want to retain the assertion line on the metadata when storing - // as a regular snapshot. - if self.metadata.assertion_line.is_some() { - let mut metadata = self.metadata.clone(); - metadata.assertion_line = None; - self.save_with_metadata(path, &metadata) + pub fn save(&self, path: &Path) -> Result> { + self.save_with_metadata(path, None, &self.metadata.trim_for_persistence()) + } + + /// Same as `save` but instead of writing a normal snapshot file this will write + /// a `.snap.new` file with additional information. + /// + /// If the existing snapshot matches the new file, then `None` is returned, otherwise + /// the name of the new snapshot file. + pub(crate) fn save_new(&self, path: &Path) -> Result, Box> { + let mut new_path = path.to_path_buf(); + new_path.set_extension("snap.new"); + if self.save_with_metadata(&new_path, Some(path), &self.metadata)? { + Ok(Some(new_path)) } else { - self.save_with_metadata(path, &self.metadata) + Ok(None) } } - - /// Same as `save` but also holds information only relevant for `.new` files. - pub(crate) fn save_new(&self, path: &Path) -> Result<(), Box> { - self.save_with_metadata(path, &self.metadata) - } } /// The contents of a Snapshot