Add testing helper to compare stable vs preview snapshots (#19715)

## Summary
This PR implements a diff test helper `assert_diagnostics_diff` as
described in #19351. The diff file includes both the settings ( e.g.
`+linter.preview = enabled`) and the snapshot data itself.

The current implementation looks for each old diagnostic in the new
snapshot. This works when the preview behavior adds/removes a couple
diagnostics. This implementation does not work well when every
diagnostic is modified (e.g. a "fix" is added).
https://github.com/astral-sh/ruff/pull/19715#discussion_r2259410763 has
ideas for future improvements to this implementation.

The example usage in this PR writes the diff to `preview_diff` file
instead of `preview` file, which might be a useful convention to keep.


## Test Plan
- Included a unit test at:
https://github.com/astral-sh/ruff/pull/19715/files#diff-d49487fe3e8a8585529f62c2df2a2b0a4c44267a1f93d1e859dff1d9f8771d36R523
- Example usage of this new test helper:
https://github.com/astral-sh/ruff/pull/19715/files#diff-2a33ac11146d1794c01a29549a6041d3af6fb6f9b423a31ade12a88d1951b0c2R1
This commit is contained in:
Vivek Dasari 2025-08-22 10:49:34 -07:00 committed by GitHub
parent 0be3e1fbbf
commit 5508e8e528
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 324 additions and 1197 deletions

View file

@ -2,6 +2,7 @@
//! Helper functions for the tests of rule implementations.
use std::borrow::Cow;
use std::fmt;
use std::path::Path;
#[cfg(not(fuzzing))]
@ -32,6 +33,85 @@ use crate::source_kind::SourceKind;
use crate::{Applicability, FixAvailability};
use crate::{Locator, directives};
/// Represents the difference between two diagnostic runs.
#[derive(Debug)]
pub(crate) struct DiagnosticsDiff {
/// Diagnostics that were removed (present in 'before' but not in 'after')
removed: Vec<Diagnostic>,
/// Diagnostics that were added (present in 'after' but not in 'before')
added: Vec<Diagnostic>,
/// Settings used before the change
settings_before: LinterSettings,
/// Settings used after the change
settings_after: LinterSettings,
}
impl fmt::Display for DiagnosticsDiff {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
writeln!(f, "--- Linter settings ---")?;
let settings_before_str = format!("{}", self.settings_before);
let settings_after_str = format!("{}", self.settings_after);
let diff = similar::TextDiff::from_lines(&settings_before_str, &settings_after_str);
for change in diff.iter_all_changes() {
match change.tag() {
similar::ChangeTag::Delete => write!(f, "-{change}")?,
similar::ChangeTag::Insert => write!(f, "+{change}")?,
similar::ChangeTag::Equal => (),
}
}
writeln!(f)?;
writeln!(f, "--- Summary ---")?;
writeln!(f, "Removed: {}", self.removed.len())?;
writeln!(f, "Added: {}", self.added.len())?;
writeln!(f)?;
if !self.removed.is_empty() {
writeln!(f, "--- Removed ---")?;
for diagnostic in &self.removed {
writeln!(f, "{}", print_messages(std::slice::from_ref(diagnostic)))?;
}
writeln!(f)?;
}
if !self.added.is_empty() {
writeln!(f, "--- Added ---")?;
for diagnostic in &self.added {
writeln!(f, "{}", print_messages(std::slice::from_ref(diagnostic)))?;
}
writeln!(f)?;
}
Ok(())
}
}
/// Compare two sets of diagnostics and return the differences
fn diff_diagnostics(
before: Vec<Diagnostic>,
after: Vec<Diagnostic>,
settings_before: &LinterSettings,
settings_after: &LinterSettings,
) -> DiagnosticsDiff {
let mut removed = Vec::new();
let mut added = after;
for old_diag in before {
let Some(pos) = added.iter().position(|diag| diag == &old_diag) else {
removed.push(old_diag);
continue;
};
added.remove(pos);
}
DiagnosticsDiff {
removed,
added,
settings_before: settings_before.clone(),
settings_after: settings_after.clone(),
}
}
#[cfg(not(fuzzing))]
pub(crate) fn test_resource_path(path: impl AsRef<Path>) -> std::path::PathBuf {
Path::new("./resources/test/").join(path)
@ -49,6 +129,30 @@ pub(crate) fn test_path(
Ok(test_contents(&source_kind, &path, settings).0)
}
/// Test a file with two different settings and return the differences
#[cfg(not(fuzzing))]
pub(crate) fn test_path_with_settings_diff(
path: impl AsRef<Path>,
settings_before: &LinterSettings,
settings_after: &LinterSettings,
) -> Result<DiagnosticsDiff> {
assert!(
format!("{settings_before}") != format!("{settings_after}"),
"Settings must be different for differential testing"
);
let diagnostics_before = test_path(&path, settings_before)?;
let diagnostic_after = test_path(&path, settings_after)?;
let diff = diff_diagnostics(
diagnostics_before,
diagnostic_after,
settings_before,
settings_after,
);
Ok(diff)
}
#[cfg(not(fuzzing))]
pub(crate) struct TestedNotebook {
pub(crate) diagnostics: Vec<Diagnostic>,
@ -400,3 +504,59 @@ macro_rules! assert_diagnostics {
});
}};
}
#[macro_export]
macro_rules! assert_diagnostics_diff {
($snapshot:expr, $path:expr, $settings_before:expr, $settings_after:expr) => {{
let diff = $crate::test::test_path_with_settings_diff($path, $settings_before, $settings_after)?;
insta::with_settings!({ omit_expression => true }, {
insta::assert_snapshot!($snapshot, format!("{}", diff));
});
}};
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_diff_diagnostics() -> Result<()> {
use crate::codes::Rule;
use ruff_db::diagnostic::{DiagnosticId, LintName};
let settings_before = LinterSettings::for_rule(Rule::Print);
let settings_after = LinterSettings::for_rule(Rule::UnusedImport);
let test_code = r#"
import sys
import unused_module
def main():
print(sys.version)
"#;
let temp_dir = std::env::temp_dir();
let test_file = temp_dir.join("test_diff.py");
std::fs::write(&test_file, test_code)?;
let diff =
super::test_path_with_settings_diff(&test_file, &settings_before, &settings_after)?;
assert_eq!(diff.removed.len(), 1, "Should remove 1 print diagnostic");
assert_eq!(
diff.removed[0].id(),
DiagnosticId::Lint(LintName::of("print")),
"Should remove the print diagnostic"
);
assert_eq!(diff.added.len(), 1, "Should add 1 unused import diagnostic");
assert_eq!(
diff.added[0].id(),
DiagnosticId::Lint(LintName::of("unused-import")),
"Should add the unused import diagnostic"
);
std::fs::remove_file(test_file)?;
Ok(())
}
}