-
Notifications
You must be signed in to change notification settings - Fork 3
add similar library to render diffs from regression test results #1296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
38eb020
50ad023
797fdf8
3d5b4a7
c9da3d5
3bc9bf4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,12 @@ | ||
| //! Common code for running regression tests. | ||
| use anyhow::Result; | ||
| use colored::Colorize; | ||
| use float_cmp::approx_eq; | ||
| use itertools::Itertools; | ||
| use ordered_float::NotNan; | ||
| use similar::{Algorithm, DiffOp, DiffTag, capture_diff_slices}; | ||
| use std::env; | ||
| use std::fmt::Write as _; | ||
| use std::fs::{File, read_dir}; | ||
| use std::io::{BufRead, BufReader}; | ||
| use std::path::{Path, PathBuf}; | ||
|
|
@@ -95,50 +99,151 @@ fn compare_lines( | |
| let lines1 = read_lines(&output_dir1.join(file_name)); | ||
| let lines2 = read_lines(&output_dir2.join(file_name)); | ||
|
|
||
| // Check for different number of lines | ||
| if lines1.len() != lines2.len() { | ||
| errors.push(format!( | ||
| "{file_name}: Different number of lines: {} vs {}", | ||
| lines1.len(), | ||
| lines2.len() | ||
| )); | ||
| // Check whether files differ using the existing field-by-field tolerance rules. | ||
| let mut has_mismatch = lines1.len() != lines2.len(); | ||
| if !has_mismatch { | ||
| has_mismatch = lines1 | ||
| .iter() | ||
| .zip(&lines2) | ||
| .any(|(line1, line2)| !compare_line(line1, line2)); | ||
| } | ||
|
|
||
| // Compare each line | ||
| for (idx, (line1, line2)) in lines1.into_iter().zip(lines2).enumerate() { | ||
| let line_num = idx + 1; // (1-based) line number | ||
| if !compare_line(line_num, &line1, &line2, file_name, errors) { | ||
| errors.push(format!( | ||
| "{file_name}: line {line_num}:\n + \"{line1}\"\n - \"{line2}\"" | ||
| )); | ||
| } | ||
| if has_mismatch { | ||
| let diff_ops = capture_csv_diff_ops(&lines1, &lines2); | ||
| let diff = render_diff(&diff_ops, &lines1, &lines2); | ||
| errors.push(format!("{file_name}: output differs\n{diff}")); | ||
| } | ||
| } | ||
|
|
||
| fn compare_line( | ||
| num: usize, | ||
| line1: &str, | ||
| line2: &str, | ||
| file_name: &str, | ||
| errors: &mut Vec<String>, | ||
| ) -> bool { | ||
| fn compare_line(line1: &str, line2: &str) -> bool { | ||
| let fields1 = line1.split(',').collect_vec(); | ||
| let fields2 = line2.split(',').collect_vec(); | ||
| if fields1.len() != fields2.len() { | ||
| errors.push(format!( | ||
| "{}: line {}: Different number of fields: {} vs {}", | ||
| file_name, | ||
| num, | ||
| fields1.len(), | ||
| fields2.len() | ||
| )); | ||
| return false; | ||
| } | ||
|
|
||
| // Check every field matches | ||
| fields1.into_iter().zip(fields2).all(|(f1, f2)| { | ||
| // First try to compare fields as floating-point values, falling back on string comparison | ||
| try_compare_floats(f1, f2).unwrap_or_else(|| f1 == f2) | ||
| }) | ||
| fields1 | ||
| .into_iter() | ||
| .zip(fields2) | ||
| .all(|(f1, f2)| try_compare_floats(f1, f2).unwrap_or_else(|| f1 == f2)) | ||
| } | ||
|
|
||
| fn capture_csv_diff_ops(lines1: &[String], lines2: &[String]) -> Vec<DiffOp> { | ||
| let parsed1 = parse_csv_lines(lines1); | ||
| let parsed2 = parse_csv_lines(lines2); | ||
| capture_diff_slices(Algorithm::Myers, &parsed1, &parsed2) | ||
| } | ||
|
|
||
| fn has_non_equal_diff_ops(diff_ops: &[DiffOp]) -> bool { | ||
| diff_ops.iter().any(|op| op.tag() != DiffTag::Equal) | ||
| } | ||
|
|
||
| /// Render a line-based diff from `DiffOp`s, including old/new line numbers. | ||
| /// For replaced lines, pairs that are equal under `compare_line` are omitted. | ||
| fn render_diff(diff_ops: &[DiffOp], lines1: &[String], lines2: &[String]) -> String { | ||
| let mut out = String::new(); | ||
| for op in diff_ops { | ||
| let (tag, old_range, new_range) = op.as_tag_tuple(); | ||
| match tag { | ||
| DiffTag::Equal => {} | ||
| DiffTag::Delete => { | ||
| for old_idx in old_range { | ||
| let _ = writeln!( | ||
| out, | ||
| "{}", | ||
| format!("-L{}: {}", old_idx + 1, lines1[old_idx]).red() | ||
| ); | ||
| } | ||
| } | ||
| DiffTag::Insert => { | ||
| for new_idx in new_range { | ||
| let _ = writeln!( | ||
| out, | ||
| "{}", | ||
| format!("+L{}: {}", new_idx + 1, lines2[new_idx]).green() | ||
| ); | ||
| } | ||
| } | ||
| DiffTag::Replace => { | ||
| let old_start = old_range.start; | ||
| let new_start = new_range.start; | ||
| let paired_len = old_range.len().min(new_range.len()); | ||
|
|
||
| for idx in 0..paired_len { | ||
| let old_idx = old_start + idx; | ||
| let new_idx = new_start + idx; | ||
| if !compare_line(&lines1[old_idx], &lines2[new_idx]) { | ||
| let _ = writeln!( | ||
| out, | ||
| "{}", | ||
| format!("-L{}: {}", old_idx + 1, lines1[old_idx]).red() | ||
| ); | ||
| let _ = writeln!( | ||
| out, | ||
| "{}", | ||
| format!("+L{}: {}", new_idx + 1, lines2[new_idx]).green() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| for (old_idx, line) in lines1 | ||
| .iter() | ||
| .enumerate() | ||
| .take(old_range.end) | ||
| .skip(old_start + paired_len) | ||
| { | ||
| let _ = writeln!(out, "{}", format!("-L{}: {}", old_idx + 1, line).red()); | ||
| } | ||
| for (new_idx, line) in lines2 | ||
| .iter() | ||
| .enumerate() | ||
| .take(new_range.end) | ||
| .skip(new_start + paired_len) | ||
| { | ||
| let _ = writeln!(out, "{}", format!("+L{}: {}", new_idx + 1, line).green()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| out | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| struct CsvLine(Vec<CsvField>); | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| enum CsvField { | ||
| Float(NotNan<f64>), | ||
| Text(String), | ||
| } | ||
|
|
||
| fn parse_csv_lines(lines: &[String]) -> Vec<CsvLine> { | ||
| lines.iter().map(|line| parse_csv_line(line)).collect() | ||
| } | ||
|
|
||
| fn parse_csv_line(line: &str) -> CsvLine { | ||
| CsvLine(line.split(',').map(parse_csv_field).collect()) | ||
| } | ||
|
|
||
| fn parse_csv_field(field: &str) -> CsvField { | ||
| if let Some(float) = parse_finite(field) { | ||
| CsvField::Float(quantise_float(float)) | ||
| } else { | ||
| CsvField::Text(field.to_string()) | ||
| } | ||
| } | ||
|
|
||
| fn quantise_float(value: f64) -> NotNan<f64> { | ||
| let scaled = value / FLOAT_CMP_TOLERANCE; | ||
| let quantised = if scaled.is_finite() { | ||
| scaled.round() * FLOAT_CMP_TOLERANCE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jamesturner246 As the resident floating-point guru, does this seem like an ok way to round a number to the nearest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it's not be the answer you want, but I'd go with the approx equals routes. Absolute tolerance: The issue with this code is that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Aurashk It seems like this approach to quantisation isn't going to work because... floating-point numbers 😞. Are you happy to have another go at this using the NB: You can use different rounding strategies with this crate (see here). Dunno if this is something we'll want to change or whether the default Btw I have verified that we do see these rounding errors with our actual data, e.g. the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried another thing too which seems to work and is much simpler, see if you are happy with this. If not I can have a go with the decimal crate.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As @AdrianDAlessandro suggested, this would be a lot faster if we only do this extra work in the case that we already know the files are different, because currently it's comparing every line of one file with every line in another using our probably quite slow comparison method, so when comparing two files that are 1000 lines long, you'd get 1m comparisons, rather than 1000. This still isn't quite correct though. If you have two identical lines in your CSV file and one of them is deleted, it won't appear in the diff, because it will find a match somewhere else in the file. For it to be totally right, the diffing algorithm has to be the single source of truth. That said, I suspect it will probably work fine in practice, at least most of the time. You probably wouldn't need to change much to use the decimal type I mentioned though. It would mostly be a case of swapping out the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the slow response, managed to have another look at this now. I fiddled around with the I applied the suggestion to only do floating point replacements if there is a mismatch between files and managed to improve on the other approach a bit by binning the 'equivalent' lines in maps before replacing instead of doing a brute force check-and-replace of each line. This seems to work okay. I did a stress test by turning down the tolerance to 1e-14 and it's still rendering all the diffs in a few seconds. Let me know what you think. Note that, because of the binning, if there are two lines which are exact duplicates within the tolerance, it is possible for the reported diff to contain a slightly different line (within tolerance) to what is actually outputted to the file, although I'm not sure this is going to matter much in practice, I added a warning so it doesn't confuse people.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nw! I'm off in a mo though so won't have a chance to look at this again. The thing about using a base-10 type is that while it does still have some of the other problems associated with binary floating-point numbers, you should be able to represent the rounded version exactly. E.g. in a file, you have 1.23456789, but we only care about the first two DPs. If you represent it with the There might be something I'm missing... anyway, shall we park this one until I get back?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with what you are saying - once you are in fixed precision land everything will work. It just doesn't seem to fit the problem when you try it though. The rounding from floating point causes problems, for example: rounded to 10 dp, come out to different numbers even though they are only In reference to what James says above too, it would be even better to use a relative floating point tolerance as well as an absolute tolerance, as No problem, no rush to get back to me!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see what you mean. Yes, it doesn't seem right that a difference on the order of I actually think that fiddling with the number of DPs to make it work is the right solution though! I know it's a hack, but it's not like we picked the threshold for a principled reason. It's just the smallest number that means the tests pass really (though we wouldn't want to keep cranking it up indefinitely). Remember that whatever solution we come up with also has to result in similar numbers having the same hash -- i.e. the bytes representing them have to be exactly the same -- as those are the semantics required by the diffing library. It's annoying that it requires inputs to implement Good point about relative comparisons being better. Could we achieve this by rounding to SFs rather than DPs (see here)? |
||
| } else { | ||
| value | ||
| }; | ||
|
|
||
| let quantised = if quantised == -0.0 { 0.0 } else { quantised }; | ||
| NotNan::new(quantised).expect("quantised float should always be finite") | ||
| } | ||
|
|
||
| /// Parse a string into an `f64`, returning `None` if parsing fails or value is infinite/NaN | ||
|
|
@@ -185,3 +290,98 @@ fn read_lines(path: &Path) -> Vec<String> { | |
| .map_while(Result::ok) | ||
| .collect() | ||
| } | ||
|
|
||
| #[test] | ||
| fn tolerated_float_change_yields_no_diff_ops() { | ||
| let old_lines = vec!["asset,1.00000000001".to_string()]; | ||
| let new_lines = vec!["asset,1.00000000002".to_string()]; | ||
|
|
||
| let diff_ops = capture_csv_diff_ops(&old_lines, &new_lines); | ||
| assert!(!has_non_equal_diff_ops(&diff_ops)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn parse_csv_lines_normalises_floats_within_tolerance() { | ||
| let lines1 = vec![ | ||
| "asset_a,1.000000000001,region_1".to_string(), | ||
| "asset_b,2.5,region_2".to_string(), | ||
| ]; | ||
| let lines2 = vec![ | ||
| "asset_a,1.000000000002,region_1".to_string(), | ||
| "asset_b,2.5,region_2".to_string(), | ||
| ]; | ||
|
|
||
| let parsed1 = parse_csv_lines(&lines1); | ||
| let parsed2 = parse_csv_lines(&lines2); | ||
|
|
||
| assert_eq!(parsed1, parsed2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_diff_ignores_tolerated_float_changes() { | ||
| let old_lines = vec![ | ||
| "asset_a,1.00000000001".to_string(), | ||
| "asset_b,2.0".to_string(), | ||
| ]; | ||
| let new_lines = vec![ | ||
| "asset_a,1.00000000002".to_string(), | ||
| "asset_b,3.0".to_string(), | ||
| ]; | ||
|
|
||
| let diff_ops = capture_csv_diff_ops(&old_lines, &new_lines); | ||
| let diff = render_diff(&diff_ops, &old_lines, &new_lines); | ||
|
|
||
| assert!(!diff.contains("asset_a")); | ||
| assert!(diff.contains("asset_b,2.0")); | ||
| assert!(diff.contains("asset_b,3.0")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_diff_ignores_tolerated_float_differences_one_line_missing() { | ||
| let old_lines = vec![ | ||
| "asset_a,1.000000000001".to_string(), | ||
| "asset_b,2.000000000001".to_string(), | ||
| "asset_c,3.0".to_string(), | ||
| "asset_d,4.000000000001".to_string(), | ||
| "asset_e,5.000000000001".to_string(), | ||
| "asset_f,6.000000000001".to_string(), | ||
| ]; | ||
| let new_lines = vec![ | ||
| "asset_a,1.000000000002".to_string(), | ||
| "asset_b,2.000000000002".to_string(), | ||
| "asset_d,4.000000000002".to_string(), | ||
| "asset_e,5.000000000002".to_string(), | ||
| "asset_f,6.000000000002".to_string(), | ||
| ]; | ||
|
|
||
| let diff_ops = capture_csv_diff_ops(&old_lines, &new_lines); | ||
| let diff = render_diff(&diff_ops, &old_lines, &new_lines); | ||
|
|
||
| assert!(!diff.contains("asset_a")); | ||
| assert!(!diff.contains("asset_b")); | ||
| assert!(!diff.contains("asset_d")); | ||
| assert!(!diff.contains("asset_e")); | ||
| assert!(!diff.contains("asset_f")); | ||
| assert!(diff.contains("-L3: asset_c,3.0")); | ||
| assert!(!diff.contains("asset_c,9.0")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_diff_ignores_quantisation_boundary_tolerance_case() { | ||
| let old_lines = vec![ | ||
| "asset_a,25.852906323049822".to_string(), | ||
| "asset_b,2.0".to_string(), | ||
| ]; | ||
| let new_lines = vec![ | ||
| "asset_a,25.852906323050078".to_string(), | ||
| "asset_b,3.0".to_string(), | ||
| ]; | ||
|
|
||
| // `asset_a` differs only within tolerance, while `asset_b` is a real change. | ||
| let diff_ops = capture_csv_diff_ops(&old_lines, &new_lines); | ||
| let diff = render_diff(&diff_ops, &old_lines, &new_lines); | ||
|
|
||
| assert!(!diff.contains("asset_a")); | ||
| assert!(diff.contains("-L2: asset_b,2.0")); | ||
| assert!(diff.contains("+L2: asset_b,3.0")); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra comparison shouldn't be necessary 😞. The diff algo thinks they're different and if
compare_linesays otherwise, then it's not doing the same comparison that we're getting with theEqimplementation forCSVLine, which is a problem.