From 16b5402fd7f22bfaec73ba9063fc63a4fbf3b370 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 16 Jun 2021 15:40:09 -0700 Subject: [PATCH] testsuite: Switch to colored diffs with Myers diff. --- crates/cargo-test-support/Cargo.toml | 1 + crates/cargo-test-support/src/compare.rs | 308 +++++++++++------------ crates/cargo-test-support/src/diff.rs | 174 +++++++++++++ crates/cargo-test-support/src/lib.rs | 1 + tests/testsuite/doc.rs | 4 +- tests/testsuite/metabuild.rs | 2 +- 6 files changed, 325 insertions(+), 165 deletions(-) create mode 100644 crates/cargo-test-support/src/diff.rs diff --git a/crates/cargo-test-support/Cargo.toml b/crates/cargo-test-support/Cargo.toml index 99f8d271785..7329441aaf5 100644 --- a/crates/cargo-test-support/Cargo.toml +++ b/crates/cargo-test-support/Cargo.toml @@ -21,5 +21,6 @@ lazy_static = "1.0" remove_dir_all = "0.5" serde_json = "1.0" tar = { version = "0.4.18", default-features = false } +termcolor = "1.1.2" toml = "0.5.7" url = "2.2.2" diff --git a/crates/cargo-test-support/src/compare.rs b/crates/cargo-test-support/src/compare.rs index bd62e79b768..b1b5c3fa448 100644 --- a/crates/cargo-test-support/src/compare.rs +++ b/crates/cargo-test-support/src/compare.rs @@ -31,10 +31,12 @@ //! a problem. //! - Carriage returns are removed, which can help when running on Windows. +use crate::diff; use crate::paths; use anyhow::{bail, Context, Result}; use serde_json::Value; use std::env; +use std::fmt; use std::path::Path; use std::str; use url::Url; @@ -165,23 +167,21 @@ pub fn match_exact( ) -> Result<()> { let expected = normalize_expected(expected, cwd); let actual = normalize_actual(actual, cwd); - let e = expected.lines(); - let a = actual.lines(); - - let diffs = diff_lines(a, e, false); - if diffs.is_empty() { - Ok(()) - } else { - bail!( - "{} did not match:\n\ - {}\n\n\ - other output:\n\ - {}\n", - description, - diffs.join("\n"), - other_output, - ) + let e: Vec<_> = expected.lines().map(WildStr::new).collect(); + let a: Vec<_> = actual.lines().map(WildStr::new).collect(); + if e == a { + return Ok(()); } + let diff = diff::colored_diff(&e, &a); + bail!( + "{} did not match:\n\ + {}\n\n\ + other output:\n\ + {}\n", + description, + diff, + other_output, + ); } /// Convenience wrapper around [`match_exact`] which will panic on error. @@ -199,7 +199,8 @@ pub fn assert_match_exact(expected: &str, actual: &str) { pub fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> { let expected = normalize_expected(expected, cwd); let actual = normalize_actual(actual, cwd); - let mut a = actual.lines().collect::>(); + let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect(); + let mut a: Vec<_> = actual.lines().map(|line| WildStr::new(line)).collect(); // match more-constrained lines first, although in theory we'll // need some sort of recursive match here. This handles the case // that you expect "a\n[..]b" and two lines are printed out, @@ -207,31 +208,35 @@ pub fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Resu // search fails to find this. This simple sort at least gets the // test suite to pass for now, but we may need to get more fancy // if tests start failing again. - a.sort_by_key(|s| s.len()); - let mut failures = Vec::new(); - - for e_line in expected.lines() { - match a.iter().position(|a_line| lines_match(e_line, a_line)) { + a.sort_by_key(|s| s.line.len()); + let mut changes = Vec::new(); + let mut a_index = 0; + let mut failure = false; + + use crate::diff::Change; + for (e_i, e_line) in e.into_iter().enumerate() { + match a.iter().position(|a_line| e_line == *a_line) { Some(index) => { - a.remove(index); + let a_line = a.remove(index); + changes.push(Change::Keep(e_i, index, a_line)); + a_index += 1; + } + None => { + failure = true; + changes.push(Change::Remove(e_i, e_line)); } - None => failures.push(e_line), } } - if !failures.is_empty() { - bail!( - "Did not find expected line(s):\n{}\n\ - Remaining available output:\n{}\n", - failures.join("\n"), - a.join("\n") - ); + for unmatched in a { + failure = true; + changes.push(Change::Add(a_index, unmatched)); + a_index += 1; } - if !a.is_empty() { + if failure { bail!( - "Output included extra lines:\n\ - {}\n", - a.join("\n") - ) + "Expected lines did not match (ignoring order):\n{}\n", + diff::render_colored_changes(&changes) + ); } else { Ok(()) } @@ -244,28 +249,24 @@ pub fn match_unordered(expected: &str, actual: &str, cwd: Option<&Path>) -> Resu pub fn match_contains(expected: &str, actual: &str, cwd: Option<&Path>) -> Result<()> { let expected = normalize_expected(expected, cwd); let actual = normalize_actual(actual, cwd); - let e = expected.lines(); - let mut a = actual.lines(); - - let mut diffs = diff_lines(a.clone(), e.clone(), true); - while a.next().is_some() { - let a = diff_lines(a.clone(), e.clone(), true); - if a.len() < diffs.len() { - diffs = a; - } + let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect(); + let a: Vec<_> = actual.lines().map(|line| WildStr::new(line)).collect(); + if e.len() == 0 { + bail!("expected length must not be zero"); } - if diffs.is_empty() { - Ok(()) - } else { - bail!( - "expected to find:\n\ - {}\n\n\ - did not find in output:\n\ - {}", - expected, - actual - ) + for window in a.windows(e.len()) { + if window == e { + return Ok(()); + } } + bail!( + "expected to find:\n\ + {}\n\n\ + did not find in output:\n\ + {}", + expected, + actual + ); } /// Checks that the given string does not contain the given contiguous lines @@ -299,28 +300,23 @@ pub fn match_contains_n( ) -> Result<()> { let expected = normalize_expected(expected, cwd); let actual = normalize_actual(actual, cwd); - let e = expected.lines(); - let mut a = actual.lines(); - - let mut matches = 0; - - while let Some(..) = { - if diff_lines(a.clone(), e.clone(), true).is_empty() { - matches += 1; - } - a.next() - } {} - + let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect(); + let a: Vec<_> = actual.lines().map(|line| WildStr::new(line)).collect(); + if e.len() == 0 { + bail!("expected length must not be zero"); + } + let matches = a.windows(e.len()).filter(|window| *window == e).count(); if matches == number { Ok(()) } else { bail!( - "expected to find {} occurrences:\n\ + "expected to find {} occurrences of:\n\ {}\n\n\ - did not find in output:\n\ + but found {} matches in the output:\n\ {}", number, expected, + matches, actual ) } @@ -340,16 +336,17 @@ pub fn match_with_without( cwd: Option<&Path>, ) -> Result<()> { let actual = normalize_actual(actual, cwd); - let contains = |s, line| { - let mut s = normalize_expected(s, cwd); - s.insert_str(0, "[..]"); - s.push_str("[..]"); - lines_match(&s, line) - }; - let matches: Vec<&str> = actual + let norm = |s: &String| format!("[..]{}[..]", normalize_expected(s, cwd)); + let with: Vec<_> = with.iter().map(norm).collect(); + let without: Vec<_> = without.iter().map(norm).collect(); + let with_wild: Vec<_> = with.iter().map(|w| WildStr::new(w)).collect(); + let without_wild: Vec<_> = without.iter().map(|w| WildStr::new(w)).collect(); + + let matches: Vec<_> = actual .lines() - .filter(|line| with.iter().all(|with| contains(with, line))) - .filter(|line| !without.iter().any(|without| contains(without, line))) + .map(WildStr::new) + .filter(|line| with_wild.iter().all(|with| with == line)) + .filter(|line| !without_wild.iter().any(|without| without == line)) .collect(); match matches.len() { 0 => bail!( @@ -371,7 +368,7 @@ pub fn match_with_without( {}\n", with, without, - matches.join("\n") + itertools::join(matches, "\n") ), } } @@ -454,73 +451,6 @@ fn collect_json_objects( Ok((expected_objs, actual_objs)) } -fn diff_lines<'a>(actual: str::Lines<'a>, expected: str::Lines<'a>, partial: bool) -> Vec { - let actual = actual.take(if partial { - expected.clone().count() - } else { - usize::MAX - }); - zip_all(actual, expected) - .enumerate() - .filter_map(|(i, (a, e))| match (a, e) { - (Some(a), Some(e)) => { - if lines_match(e, a) { - None - } else { - Some(format!("{:3} - |{}|\n + |{}|\n", i, e, a)) - } - } - (Some(a), None) => Some(format!("{:3} -\n + |{}|\n", i, a)), - (None, Some(e)) => Some(format!("{:3} - |{}|\n +\n", i, e)), - (None, None) => unreachable!(), - }) - .collect() -} - -struct ZipAll { - first: I1, - second: I2, -} - -impl, I2: Iterator> Iterator for ZipAll { - type Item = (Option, Option); - fn next(&mut self) -> Option<(Option, Option)> { - let first = self.first.next(); - let second = self.second.next(); - - match (first, second) { - (None, None) => None, - (a, b) => Some((a, b)), - } - } -} - -/// Returns an iterator, similar to `zip`, but exhausts both iterators. -/// -/// Each element is `(Option, Option)` where `None` indicates an -/// iterator ended early. -fn zip_all, I2: Iterator>(a: I1, b: I2) -> ZipAll { - ZipAll { - first: a, - second: b, - } -} - -fn lines_match(expected: &str, mut actual: &str) -> bool { - for (i, part) in expected.split("[..]").enumerate() { - match actual.find(part) { - Some(j) => { - if i == 0 && j != 0 { - return false; - } - actual = &actual[j + part.len()..]; - } - None => return false, - } - } - actual.is_empty() || expected.ends_with("[..]") -} - /// Compares JSON object for approximate equality. /// You can use `[..]` wildcard in strings (useful for OS-dependent things such /// as paths). You can use a `"{...}"` string literal as a wildcard for @@ -550,12 +480,10 @@ fn find_json_mismatch_r<'a>( (&Bool(l), &Bool(r)) if l == r => None, (&String(ref l), _) if l == "{...}" => None, (&String(ref l), &String(ref r)) => { - let l = normalize_expected(l, cwd); - let r = normalize_actual(r, cwd); - if lines_match(&l, &r) { - None - } else { + if match_exact(l, r, "", "", cwd).is_err() { Some((expected, actual)) + } else { + None } } (&Array(ref l), &Array(ref r)) => { @@ -585,15 +513,71 @@ fn find_json_mismatch_r<'a>( } } +/// A single line string that supports `[..]` wildcard matching. +pub struct WildStr<'a> { + has_meta: bool, + line: &'a str, +} + +impl<'a> WildStr<'a> { + pub fn new(line: &'a str) -> WildStr<'a> { + WildStr { + has_meta: line.contains("[..]"), + line, + } + } +} + +impl<'a> PartialEq for WildStr<'a> { + fn eq(&self, other: &Self) -> bool { + match (self.has_meta, other.has_meta) { + (false, false) => self.line == other.line, + (true, false) => meta_cmp(self.line, other.line), + (false, true) => meta_cmp(other.line, self.line), + (true, true) => panic!("both lines cannot have [..]"), + } + } +} + +fn meta_cmp(a: &str, mut b: &str) -> bool { + for (i, part) in a.split("[..]").enumerate() { + match b.find(part) { + Some(j) => { + if i == 0 && j != 0 { + return false; + } + b = &b[j + part.len()..]; + } + None => return false, + } + } + b.is_empty() || a.ends_with("[..]") +} + +impl fmt::Display for WildStr<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.line) + } +} + +impl fmt::Debug for WildStr<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:?}", self.line) + } +} + #[test] -fn lines_match_works() { - assert!(lines_match("a b", "a b")); - assert!(lines_match("a[..]b", "a b")); - assert!(lines_match("a[..]", "a b")); - assert!(lines_match("[..]", "a b")); - assert!(lines_match("[..]b", "a b")); - - assert!(!lines_match("[..]b", "c")); - assert!(!lines_match("b", "c")); - assert!(!lines_match("b", "cb")); +fn wild_str_cmp() { + for (a, b) in &[ + ("a b", "a b"), + ("a[..]b", "a b"), + ("a[..]", "a b"), + ("[..]", "a b"), + ("[..]b", "a b"), + ] { + assert_eq!(WildStr::new(a), WildStr::new(b)); + } + for (a, b) in &[("[..]b", "c"), ("b", "c"), ("b", "cb")] { + assert_ne!(WildStr::new(a), WildStr::new(b)); + } } diff --git a/crates/cargo-test-support/src/diff.rs b/crates/cargo-test-support/src/diff.rs new file mode 100644 index 00000000000..f3b283b109f --- /dev/null +++ b/crates/cargo-test-support/src/diff.rs @@ -0,0 +1,174 @@ +//! A simple Myers diff implementation. +//! +//! This focuses on being short and simple, and the expense of being +//! inefficient. A key characteristic here is that this supports cargotest's +//! `[..]` wildcard matching. That means things like hashing can't be used. +//! Since Cargo's output tends to be small, this should be sufficient. + +use std::fmt; +use std::io::Write; +use termcolor::{Ansi, Color, ColorSpec, NoColor, WriteColor}; + +/// A single line change to be applied to the original. +#[derive(Debug, Eq, PartialEq)] +pub enum Change { + Add(usize, T), + Remove(usize, T), + Keep(usize, usize, T), +} + +pub fn diff<'a, T>(a: &'a [T], b: &'a [T]) -> Vec> +where + T: PartialEq, +{ + if a.is_empty() && b.is_empty() { + return vec![]; + } + let mut diff = vec![]; + for (prev_x, prev_y, x, y) in backtrack(&a, &b) { + if x == prev_x { + diff.push(Change::Add(prev_y + 1, &b[prev_y])); + } else if y == prev_y { + diff.push(Change::Remove(prev_x + 1, &a[prev_x])); + } else { + diff.push(Change::Keep(prev_x + 1, prev_y + 1, &a[prev_x])); + } + } + diff.reverse(); + diff +} + +fn shortest_edit(a: &[T], b: &[T]) -> Vec> +where + T: PartialEq, +{ + let max = a.len() + b.len(); + let mut v = vec![0; 2 * max + 1]; + let mut trace = vec![]; + for d in 0..=max { + trace.push(v.clone()); + for k in (0..=(2 * d)).step_by(2) { + let mut x = if k == 0 || (k != 2 * d && v[max - d + k - 1] < v[max - d + k + 1]) { + // Move down + v[max - d + k + 1] + } else { + // Move right + v[max - d + k - 1] + 1 + }; + let mut y = x + d - k; + // Step diagonally as far as possible. + while x < a.len() && y < b.len() && a[x] == b[y] { + x += 1; + y += 1; + } + v[max - d + k] = x; + // Return if reached the bottom-right position. + if x >= a.len() && y >= b.len() { + return trace; + } + } + } + panic!("finished without hitting end?"); +} + +fn backtrack(a: &[T], b: &[T]) -> Vec<(usize, usize, usize, usize)> +where + T: PartialEq, +{ + let mut result = vec![]; + let mut x = a.len(); + let mut y = b.len(); + let max = x + y; + for (d, v) in shortest_edit(a, b).iter().enumerate().rev() { + let k = x + d - y; + let prev_k = if k == 0 || (k != 2 * d && v[max - d + k - 1] < v[max - d + k + 1]) { + k + 1 + } else { + k - 1 + }; + let prev_x = v[max - d + prev_k]; + let prev_y = (prev_x + d).saturating_sub(prev_k); + while x > prev_x && y > prev_y { + result.push((x - 1, y - 1, x, y)); + x -= 1; + y -= 1; + } + if d > 0 { + result.push((prev_x, prev_y, x, y)); + } + x = prev_x; + y = prev_y; + } + return result; +} + +pub fn colored_diff<'a, T>(a: &'a [T], b: &'a [T]) -> String +where + T: PartialEq + fmt::Display, +{ + let changes = diff(a, b); + render_colored_changes(&changes) +} + +pub fn render_colored_changes(changes: &[Change]) -> String { + // termcolor is not very ergonomic, but I don't want to bring in another dependency. + let mut red = ColorSpec::new(); + red.set_fg(Some(Color::Red)); + let mut green = ColorSpec::new(); + green.set_fg(Some(Color::Green)); + let mut dim = ColorSpec::new(); + dim.set_dimmed(true); + let mut v = Vec::new(); + let mut result: Box = if crate::is_ci() { + // Don't use color on CI. Even though GitHub can display colors, it + // makes reading the raw logs more difficult. + Box::new(NoColor::new(&mut v)) + } else { + Box::new(Ansi::new(&mut v)) + }; + + for change in changes { + let (nums, sign, color, text) = match change { + Change::Add(i, s) => (format!(" {:<4} ", i), '+', &green, s), + Change::Remove(i, s) => (format!("{:<4} ", i), '-', &red, s), + Change::Keep(x, y, s) => (format!("{:<4}{:<4} ", x, y), ' ', &dim, s), + }; + result.set_color(&dim).unwrap(); + write!(result, "{}", nums).unwrap(); + let mut bold = color.clone(); + bold.set_bold(true); + result.set_color(&bold).unwrap(); + write!(result, "{}", sign).unwrap(); + result.reset().unwrap(); + result.set_color(&color).unwrap(); + write!(result, "{}", text).unwrap(); + result.reset().unwrap(); + writeln!(result).unwrap(); + } + drop(result); + String::from_utf8(v).unwrap() +} + +#[cfg(test)] +pub fn compare(a: &str, b: &str) { + let a: Vec<_> = a.chars().collect(); + let b: Vec<_> = b.chars().collect(); + let changes = diff(&a, &b); + let mut result = vec![]; + for change in changes { + match change { + Change::Add(_, s) => result.push(*s), + Change::Remove(_, _s) => {} + Change::Keep(_, _, s) => result.push(*s), + } + } + assert_eq!(b, result); +} + +#[test] +fn basic_tests() { + compare("", ""); + compare("A", ""); + compare("", "B"); + compare("ABCABBA", "CBABAC"); +} diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 8082ba61fd8..992446f7fc8 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -52,6 +52,7 @@ pub use cargo_test_macro::cargo_test; pub mod compare; pub mod cross_compile; +mod diff; pub mod git; pub mod install; pub mod paths; diff --git a/tests/testsuite/doc.rs b/tests/testsuite/doc.rs index 1dcf7735b6d..f376f62d6b5 100644 --- a/tests/testsuite/doc.rs +++ b/tests/testsuite/doc.rs @@ -1470,8 +1470,8 @@ fn doc_message_format() { "children": "{...}", "code": "{...}", "level": "error", - "message": "[..]", - "rendered": "[..]", + "message": "{...}", + "rendered": "{...}", "spans": "{...}" }, "package_id": "foo [..]", diff --git a/tests/testsuite/metabuild.rs b/tests/testsuite/metabuild.rs index d23c171daff..96fcbd75dd6 100644 --- a/tests/testsuite/metabuild.rs +++ b/tests/testsuite/metabuild.rs @@ -737,7 +737,7 @@ fn metabuild_failed_build_json() { "code": "{...}", "level": "error", "message": "cannot find function `metabuild` in [..] `mb`", - "rendered": "[..]", + "rendered": "{...}", "spans": "{...}" }, "package_id": "foo [..]",