From 4419db8078b77a507855d04ec44e314ce5a2dd5c Mon Sep 17 00:00:00 2001 From: brendanx67 Date: Wed, 19 Jun 2024 13:21:42 -0700 Subject: [PATCH] Skyline: Improve failure reporting in AssertEx.FileEquals --- .../Skyline/Test/ChromatogramExporterTest.cs | 9 +- .../Skyline/Test/EncyclopeDiaHelpersTest.cs | 3 +- .../Skyline/TestPerf/FeatureDetectionTest.cs | 2 +- pwiz_tools/Skyline/TestUtil/AssertEx.cs | 190 +++++++++++++++--- 4 files changed, 172 insertions(+), 32 deletions(-) diff --git a/pwiz_tools/Skyline/Test/ChromatogramExporterTest.cs b/pwiz_tools/Skyline/Test/ChromatogramExporterTest.cs index cace564209..8a53337b59 100644 --- a/pwiz_tools/Skyline/Test/ChromatogramExporterTest.cs +++ b/pwiz_tools/Skyline/Test/ChromatogramExporterTest.cs @@ -94,11 +94,12 @@ public void ChromatogramExportTest() SaveChrom(docResults, fileActual2, FILE_NAMES_2.ToList(), LocalizationHelper.CurrentCulture, EXTRACTOR_2, SOURCES_2); SaveChrom(docResults, fileActualAll, FILE_NAMES_ALL.ToList(), LocalizationHelper.CurrentCulture, EXTRACTOR_ALL, SOURCES_ALL); - var tolerance = new Dictionary{{3,.0001}}; // Allow a little wiggle in mz column since we tweak the calculation with Adduct work + var columnTolerances = new AssertEx.ColumnTolerances(); + columnTolerances.AddTolerance(3, 0.0001); // Allow a little wiggle in mz column since we tweak the calculation with Adduct work - AssertEx.FileEquals(fileExpected1, fileActual1, tolerance); - AssertEx.FileEquals(fileExpected2, fileActual2, tolerance); - AssertEx.FileEquals(fileExpectedAll, fileActualAll, tolerance); + AssertEx.FileEquals(fileExpected1, fileActual1, columnTolerances); + AssertEx.FileEquals(fileExpected2, fileActual2, columnTolerances); + AssertEx.FileEquals(fileExpectedAll, fileActualAll, columnTolerances); } } diff --git a/pwiz_tools/Skyline/Test/EncyclopeDiaHelpersTest.cs b/pwiz_tools/Skyline/Test/EncyclopeDiaHelpersTest.cs index 7818eacd47..d51a7bbdcb 100644 --- a/pwiz_tools/Skyline/Test/EncyclopeDiaHelpersTest.cs +++ b/pwiz_tools/Skyline/Test/EncyclopeDiaHelpersTest.cs @@ -17,7 +17,6 @@ */ using Microsoft.VisualStudio.TestTools.UnitTesting; -using System.Collections.Generic; using System.IO; using System.Linq; using System.Threading; @@ -70,7 +69,7 @@ public void TestKoinaOutputToEncyclopediaLibraries() string dlibFilepath = TestFilesDir.GetTestPath("pan_human_library_690to705-z3_nce33.dlib"); string elibFilepath = TestFilesDir.GetTestPath("pan_human_library_690to705-z3_nce33.elib"); string elibQuantFilepath = TestFilesDir.GetTestPath("pan_human_library_690to705-z3_nce33-expected-quant-elib.elib"); - var columnTolerances = new Dictionary() { { -1, 0.000005 } }; // Allow some numerical wiggle in any column numerical column ("-1" means all columns) + var columnTolerances = new AssertEx.ColumnTolerances(0.000005); // Allow some numerical wiggle in any column numerical IProgressStatus status = new ProgressStatus(); // test koina output to dlib diff --git a/pwiz_tools/Skyline/TestPerf/FeatureDetectionTest.cs b/pwiz_tools/Skyline/TestPerf/FeatureDetectionTest.cs index 9901210846..ec62a50c0a 100644 --- a/pwiz_tools/Skyline/TestPerf/FeatureDetectionTest.cs +++ b/pwiz_tools/Skyline/TestPerf/FeatureDetectionTest.cs @@ -425,7 +425,7 @@ void ExpectError(Action act) foreach (var hkExpectedFilePath in Directory.EnumerateFiles(expectedFilesPath)) { var hkActualFilePath = hkExpectedFilePath.Replace(expectedHardklorFiles, Path.Combine(expectedHardklorFiles, @"..")); - var columnTolerances = new Dictionary() { { -1, .00015 } }; // Allow a little rounding wiggle in the decimal values + var columnTolerances = new AssertEx.ColumnTolerances(0.00015, true); // Allow a little rounding wiggle in the numeric values AssertEx.FileEquals(hkExpectedFilePath, hkActualFilePath, columnTolerances, true); } diff --git a/pwiz_tools/Skyline/TestUtil/AssertEx.cs b/pwiz_tools/Skyline/TestUtil/AssertEx.cs index cb4c234be4..513e9f31df 100644 --- a/pwiz_tools/Skyline/TestUtil/AssertEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AssertEx.cs @@ -835,7 +835,7 @@ private static string DuplicateAndReverseLines(string transitionList, bool hasHe } public static void NoDiff(string target, string actual, string helpMsg=null, - Dictionary columnTolerances = null, // Per-column numerical tolerances if strings can be read as TSV, "-1" means any column + ColumnTolerances columnTolerances = null, bool ignorePathDifferences = false) { if (helpMsg == null) @@ -873,11 +873,52 @@ public static void NoDiff(string target, string actual, string helpMsg=null, RemovePathDifferences(ref lineTarget, ref lineActual); } // If only difference appears to be generated GUIDs or timestamps, let it pass - if (!LinesEquivalentIgnoringTimeStampsAndGUIDs(lineTarget, lineActual, columnTolerances)) + if (!LinesEquivalentIgnoringTimeStampsAndGUIDs(lineTarget, lineActual, columnTolerances, out var failureMessage)) { - int pos; - for (pos = 0; pos < expectedLine?.Length && pos < actualLine?.Length && expectedLine[pos] == actualLine[pos];) {pos++;} - Fail(helpMsg + $@" Diff found at line {count} position {pos}: expected{Environment.NewLine}{expectedLine}{Environment.NewLine}actual{Environment.NewLine}{actualLine}"); + var sbEnd = new StringBuilder(); + var sbStart = new StringBuilder(); + if (lineActual != null && lineTarget != null) + { + var sharedLen = Math.Min(lineActual.Length, lineTarget.Length); + for (int i = 0; i < sharedLen; i++) + { + var endCh = lineActual[lineActual.Length - 1 - i]; + if (endCh != lineTarget[lineTarget.Length - 1 - i]) + break; + sbEnd.Insert(0, endCh); + } + for (int i = 0; i < sharedLen; i++) + { + var startCh = lineActual[i]; + if (startCh != lineTarget[i]) + break; + sbStart.Append(startCh); + } + } + + // Build an informative failure message + string assertFailMessage = TextUtil.LineSeparate( + helpMsg + $@" Diff found at line {count} position {sbStart.Length}:", + "expected", + expectedLine, + "actual", + actualLine); + if (!Equals(expectedLine, lineTarget) || !Equals(actualLine, lineActual)) + { + // Paths were removed, so report the text after removal + assertFailMessage = TextUtil.LineSeparate(assertFailMessage, + "expected with paths removed", + lineTarget, + "actual with paths removed", + lineActual); + } + assertFailMessage = TextUtil.LineSeparate(assertFailMessage, + $"matching prefix: '{sbStart}'", + $"matching suffix: '{sbEnd}'"); + if (!string.IsNullOrEmpty(failureMessage)) + assertFailMessage = TextUtil.LineSeparate(assertFailMessage, "decimal matching: " + failureMessage); + + Fail(assertFailMessage); } lineEqualLast = expectedLine; count++; @@ -930,8 +971,11 @@ private static void RemovePathDifferences(ref string lineExpected, ref string li if (string.Equals(fileE, fileA) || (Path.GetExtension(fileE) == @".tmp") && Path.GetExtension(fileE) == Path.GetExtension(fileA)) // Tmp file names will always vary { - lineExpected = lineExpected.Replace(pathE, string.Empty); - lineActual = lineActual.Replace(pathA, string.Empty); + // Empty strings are harder to see as columns. + // So, replace the matching paths with visible matching text. + const string pathSubstitutionText = "path"; + lineExpected = lineExpected.Replace(pathE, pathSubstitutionText); + lineActual = lineActual.Replace(pathA, pathSubstitutionText); } } catch @@ -941,10 +985,11 @@ private static void RemovePathDifferences(ref string lineExpected, ref string li } } - private static bool LinesEquivalentIgnoringTimeStampsAndGUIDs(string lineExpected, string lineActual, - Dictionary columnTolerances = null) // Per-column numerical tolerances if strings can be read as TSV, "-1" means any column + ColumnTolerances columnTolerances, out string failureMessage) // Per-column numerical tolerances if strings can be read as TSV, "-1" means any column { + failureMessage = string.Empty; // For all the return true cases + if (string.Equals(lineExpected, lineActual)) { return true; // Identical @@ -977,32 +1022,127 @@ private static bool LinesEquivalentIgnoringTimeStampsAndGUIDs(string lineExpecte } if (columnTolerances != null) + return columnTolerances.LinesEquivalent(lineExpected, lineActual, out failureMessage); + + return false; // Could not account for difference + } + + public class ColumnTolerances + { + private ColumnToleranceValue _defaultTolerance { get; } + private Dictionary _explicitTolerances = new Dictionary(); + + public ColumnTolerances() + { + } + + public ColumnTolerances(double defaultTolerance, bool forceScientificNotation = false) + { + _defaultTolerance = new ColumnToleranceValue(defaultTolerance, forceScientificNotation); + } + + public void AddTolerance(int column, double tolerance, bool forceScientificNotation = false) { + _explicitTolerances.Add(column, new ColumnToleranceValue(tolerance, forceScientificNotation)); + } + + public bool LinesEquivalent(string lineExpected, string lineActual, out string failureMessage) + { + failureMessage = string.Empty; + // ReSharper disable PossibleNullReferenceException var colsActual = lineActual.Split('\t'); var colsExpected = lineExpected.Split('\t'); // ReSharper restore PossibleNullReferenceException - if (colsExpected.Length == colsActual.Length) + if (colsExpected.Length != colsActual.Length) + return false; + for (var c = 0; c < colsActual.Length; c++) { - for (var c = 0; c < colsActual.Length; c++) + if (!ColumnsEquivalent(c, colsExpected[c], colsActual[c], out failureMessage)) + return false; + } + + return true; // Differences accounted for + } + + private bool ColumnsEquivalent(int i, string textExpected, string textActual, out string failureMessage) + { + failureMessage = string.Empty; + if (Equals(textExpected, textActual)) + return true; + + // See if there's a tolerance for this column, or a default tolerance (column "-1" in the dictionary) + if (!_explicitTolerances.TryGetValue(i, out var toleranceValue)) + { + toleranceValue = _defaultTolerance; + if (toleranceValue == null) + return false; // No tolerance given for this column + } + if (!TextUtil.TryParseDoubleUncertainCulture(textActual, out var valActual) || + !TextUtil.TryParseDoubleUncertainCulture(textExpected, out var valExpected)) + { + return false; + } + + char[] expChars = { 'E', 'e' }; + var textSplitActual = textActual; + if (toleranceValue.ForceScientificNotation && textSplitActual.Split(expChars).Length != 2) + textSplitActual = valActual.ToString("E"); + var textSplitExpected = textExpected; + if (toleranceValue.ForceScientificNotation && textSplitExpected.Split(expChars).Length != 2) + textSplitExpected = valExpected.ToString("E"); + + var actualParts = textSplitActual.Split(expChars); + var expectedParts = textSplitExpected.Split(expChars); + + if (actualParts.Length == 2 && expectedParts.Length == 2) + { + // Both strings have exponent, so check if they are equal + if (!Equals(expectedParts[1], actualParts[1]) || + // Then check the mantissas match to the expected tolerance + !TextUtil.TryParseDoubleUncertainCulture(actualParts[0], out valActual) || + !TextUtil.TryParseDoubleUncertainCulture(expectedParts[0], out valExpected)) { - if (colsActual[c] != colsExpected[c]) - { - // See if there's a tolerance for this column, or a default tolerance (column "-1" in the dictionary) - if ((!columnTolerances.TryGetValue(c, out var tolerance) && !columnTolerances.TryGetValue(-1, out tolerance)) || // No tolerance given for this column - !(TextUtil.TryParseDoubleUncertainCulture(colsActual[c], out var valActual) && - TextUtil.TryParseDoubleUncertainCulture(colsExpected[c], out var valExpected)) || // One or both don't parse as doubles - (Math.Abs(valActual - valExpected) > tolerance + tolerance / 1000)) // Allow for rounding cruft - { - return false; // Can't account for difference - } - } + failureMessage = string.Format( + "Expected decimal value: {0} does not match actual {1}", + textExpected, textActual); + return false; // One or both mantissas don't parse as doubles } - return true; // Differences accounted for } + + double tolerance = toleranceValue.Tolerance; + tolerance += tolerance / 1000; // Allow for rounding cruft + if (Math.Abs(valActual - valExpected) > tolerance) + { + if (expectedParts.Length == 2) + { + failureMessage = string.Format( + "Expected decimal mantissa: {0} does not match actual {1} to within {2}", + valExpected, valActual, tolerance); + } + else + { + failureMessage = string.Format( + "Expected decimal value: {0} does not match actual {1} to within {2}", + textSplitExpected, textSplitActual, tolerance); + } + return false; // Can't account for difference + } + + return true; } + } - return false; // Could not account for difference + private class ColumnToleranceValue + { + public ColumnToleranceValue(double tolerance, bool forceScientificNotation = false) + { + Tolerance = tolerance; + ForceScientificNotation = forceScientificNotation; + } + + public double Tolerance { get; } + public bool ForceScientificNotation { get; } } private static string GetEarlyEndingMessage(string helpMsg, string name, int count, string lineEqualLast, string lineNext, TextReader reader) @@ -1015,7 +1155,7 @@ private static string GetEarlyEndingMessage(string helpMsg, string name, int cou name, count, lineEqualLast, lineNext, linesRemaining); } - public static void FileEquals(string pathExpectedFile, string pathActualFile, Dictionary columnTolerances = null, bool ignorePathDifferences = false ) + public static void FileEquals(string pathExpectedFile, string pathActualFile, ColumnTolerances columnTolerances = null, bool ignorePathDifferences = false ) { string file1 = File.ReadAllText(pathExpectedFile); string file2 = File.ReadAllText(pathActualFile);