Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skyline: Improve failure reporting in AssertEx.FileEquals #3021

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Skyline: Improve failure reporting in AssertEx.FileEquals #3021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4419db8
Skyline: Improve failure reporting in AssertEx.FileEquals
brendanx67 Jun 19, 2024
13f19d1
Merge branch 'master' into Skyline/work/20240619_assertex_files_improved
brendanx67 Jul 1, 2024
b443e77
Merge remote-tracking branch 'remotes/origin/master' into Skyline/wor…
bspratt Aug 6, 2024
c7f9203
Merge branch 'master' into Skyline/work/20240619_assertex_files_improved
brendanx67 Aug 6, 2024
4eb9419
Merge branch 'master' into Skyline/work/20240619_assertex_files_improved
brendanx67 Sep 30, 2024
54cbe91
Merge branch 'master' into Skyline/work/20240619_assertex_files_improved
brendanx67 Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions pwiz_tools/Skyline/Test/ChromatogramExporterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<int, double>{{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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion pwiz_tools/Skyline/TestPerf/FeatureDetectionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ void ExpectError(Action act)
foreach (var hkExpectedFilePath in Directory.EnumerateFiles(expectedFilesPath))
{
var hkActualFilePath = hkExpectedFilePath.Replace(expectedHardklorFiles, Path.Combine(expectedHardklorFiles, @".."));
var columnTolerances = new Dictionary<int, double>() { { -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);
}

Expand Down
201 changes: 169 additions & 32 deletions pwiz_tools/Skyline/TestUtil/AssertEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ private static string DuplicateAndReverseLines(string transitionList, bool hasHe
}

public static void NoDiff(string target, string actual, string helpMsg=null,
Dictionary<int, double> 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)
Expand Down Expand Up @@ -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++;
Expand Down Expand Up @@ -930,11 +971,10 @@ private static void RemovePathDifferences(ref string lineExpected, ref string li
{
return; // No way we're cleaning this up to make a match
}

for (var p = 0; p < partsE.Length; p++)
{
var partE = partsE[p];
var partA = partsA[p];
var partE = partsE[p].Trim();
var partA = partsA[p].Trim();
if (string.Equals(partE, partA))
{
continue;
Expand All @@ -944,13 +984,14 @@ private static void RemovePathDifferences(ref string lineExpected, ref string li
{
var fileE = Path.GetFileName(partE);
var fileA = Path.GetFileName(partA);
var tmpExt = @".tmp";
if (string.Equals(fileE, fileA) || // Same filename, different path
(Path.GetExtension(fileE) == tmpExt) && Path.GetExtension(fileA) == tmpExt) // Tmp file names will always vary
if (string.Equals(fileE, fileA) ||
(Path.GetExtension(fileE) == @".tmp") && Path.GetExtension(fileE) == Path.GetExtension(fileA)) // Tmp file names will always vary
{
var ignoredPath = @"<ignored_path_difference>";
lineExpected = lineExpected.Replace(partE, ignoredPath);
lineActual = lineActual.Replace(partA, ignoredPath);
// 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
Expand All @@ -961,10 +1002,11 @@ private static void RemovePathDifferences(ref string lineExpected, ref string li
}
}


private static bool LinesEquivalentIgnoringTimeStampsAndGUIDs(string lineExpected, string lineActual,
Dictionary<int, double> 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
Expand Down Expand Up @@ -997,32 +1039,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<int, ColumnToleranceValue> _explicitTolerances = new Dictionary<int, ColumnToleranceValue>();

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++)
{
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)
{
for (var c = 0; c < colsActual.Length; c++)
// 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)
Expand All @@ -1035,7 +1172,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<int, double> 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);
Expand Down