From 2c71cffc4c770abf09d3455ebba849897c1cd8f8 Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Fri, 23 Sep 2022 11:07:07 -0700 Subject: [PATCH 1/9] Added new SkylineCmd arguments for building ion mobility libraries: --ionmobility-create-library and --ionmobility-create-library-name --- .../Skyline/CommandArgUsage.Designer.cs | 27 ++++++++++++ pwiz_tools/Skyline/CommandArgUsage.resx | 9 ++++ pwiz_tools/Skyline/CommandArgs.cs | 23 +++++++++++ pwiz_tools/Skyline/CommandLine.cs | 41 +++++++++++++++++++ .../Skyline/Properties/Resources.Designer.cs | 9 ++++ pwiz_tools/Skyline/Properties/Resources.resx | 3 ++ .../Skyline/TestPerf/PerfMeasuredInverseK0.cs | 10 ++++- 7 files changed, 120 insertions(+), 2 deletions(-) diff --git a/pwiz_tools/Skyline/CommandArgUsage.Designer.cs b/pwiz_tools/Skyline/CommandArgUsage.Designer.cs index 140767b534..7d38ac3b90 100644 --- a/pwiz_tools/Skyline/CommandArgUsage.Designer.cs +++ b/pwiz_tools/Skyline/CommandArgUsage.Designer.cs @@ -735,6 +735,24 @@ internal class CommandArgUsage { } } + /// + /// Looks up a localized string similar to Add an ion mobility library to the open document, based on its currently loaded chromatograms.. + /// + internal static string _ionmobility_create_library { + get { + return ResourceManager.GetString("_ionmobility_create_library", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Name to give the ion mobility library in an –-ionmobility-create-library operation.. + /// + internal static string _ionmobility_create_library_name { + get { + return ResourceManager.GetString("_ionmobility_create_library_name", resourceCulture); + } + } + /// /// Looks up a localized string similar to The name for the iRT calculator created during assay library import. (optional) The default name is the document base name.. /// @@ -1687,6 +1705,15 @@ internal class CommandArgUsage { } } + /// + /// Looks up a localized string similar to Creating an ion mobility library. + /// + internal static string CommandArgs_GROUP_CREATE_IMSDB_Ion_Mobility_Library { + get { + return ResourceManager.GetString("CommandArgs_GROUP_CREATE_IMSDB_Ion_Mobility_Library", resourceCulture); + } + } + /// /// Looks up a localized string similar to Adding decoy peptides. /// diff --git a/pwiz_tools/Skyline/CommandArgUsage.resx b/pwiz_tools/Skyline/CommandArgUsage.resx index 3bce6aef36..89de2c4854 100644 --- a/pwiz_tools/Skyline/CommandArgUsage.resx +++ b/pwiz_tools/Skyline/CommandArgUsage.resx @@ -123,6 +123,12 @@ Specify a spectral library to be added to the open document. + + Add an ion mobility library to the open document, based on its currently loaded chromatograms. + + + Name to give the ion mobility library in an –-ionmobility-create-library operation. + Runs a file line by line treating each line like a SkylineRunner/Cmd input. Useful for automating the execution of multiple commands. The open Skyline file remains active through all commands. @@ -387,6 +393,9 @@ Importing results replicates + + Creating an ion mobility library + Removing results replicates diff --git a/pwiz_tools/Skyline/CommandArgs.cs b/pwiz_tools/Skyline/CommandArgs.cs index 514d0afae8..0d3d189b9b 100644 --- a/pwiz_tools/Skyline/CommandArgs.cs +++ b/pwiz_tools/Skyline/CommandArgs.cs @@ -215,6 +215,17 @@ public bool Saving public string SharedFile { get; private set; } public ShareType SharedFileType { get; private set; } + // For creating a .imsdb ion mobility library + public static readonly Argument ARG_CREATE_IMSDB = new DocArgument(@"ionmobility-create-library", PATH_TO_FILE, + (c, p) => c.CreateIMSDB(p)); + + // For creating a .imsdb ion mobility library + public static readonly Argument ARG_CREATE_IMSDB_NAME = new DocArgument(@"ionmobility-create-library-name", NAME_VALUE, + (c, p) => c.IMSDbName = p.Value); + + private static readonly ArgumentGroup GROUP_CREATE_IMSDB = new ArgumentGroup(() => CommandArgUsage.CommandArgs_GROUP_CREATE_IMSDB_Ion_Mobility_Library, false, + ARG_CREATE_IMSDB, ARG_CREATE_IMSDB_NAME); + public static readonly Argument ARG_IMPORT_FILE = new DocArgument(@"import-file", PATH_TO_FILE, (c, p) => c.ParseImportFile(p)); public static readonly Argument ARG_IMPORT_REPLICATE_NAME = new DocArgument(@"import-replicate-name", NAME_VALUE, @@ -337,6 +348,8 @@ private bool ValidateMinimizeResultsArgs() return true; } + public MsDataFileUri IMSDbFile { get; private set; } + public string IMSDbName { get; private set; } public List ReplicateFile { get; private set; } public string ReplicateName { get; private set; } @@ -362,6 +375,11 @@ private bool ValidateMinimizeResultsArgs() public double? LockmassTolerance { get; private set; } public LockMassParameters LockMassParameters { get { return new LockMassParameters(LockmassPositive, LockmassNegative, LockmassTolerance); } } + private void CreateIMSDB(NameValuePair pair) + { + IMSDbFile = new MsDataFilePath(pair.ValueFullPath); + } + private void ParseImportFile(NameValuePair pair) { ReplicateFile.Add(new MsDataFilePath(pair.ValueFullPath)); @@ -556,6 +574,10 @@ public bool AddDecoys { get { return !string.IsNullOrEmpty(AddDecoysType); } } + public bool CreatingIMSDB + { + get { return IMSDbFile != null; } + } public bool ImportingResults { get { return ImportingReplicateFile || ImportingSourceDirectory; } @@ -1784,6 +1806,7 @@ public static IEnumerable UsageBlocks GROUP_IMPORT_SEARCH, GROUP_IMPORT_LIST, GROUP_ADD_LIBRARY, + GROUP_CREATE_IMSDB, GROUP_DECOYS, GROUP_REFINEMENT, GROUP_REFINEMENT_W_RESULTS, diff --git a/pwiz_tools/Skyline/CommandLine.cs b/pwiz_tools/Skyline/CommandLine.cs index 4d5432e390..5ae37f3d2f 100644 --- a/pwiz_tools/Skyline/CommandLine.cs +++ b/pwiz_tools/Skyline/CommandLine.cs @@ -376,6 +376,11 @@ private bool ProcessDocument(CommandArgs commandArgs) return false; } + if (!CreateIMSDB(commandArgs)) + { + return false; + } + if (commandArgs.Saving) { var saveFile = commandArgs.SaveFile ?? _skylineFile; @@ -588,6 +593,42 @@ private bool RefineDocument(CommandArgs commandArgs) } } + private bool CreateIMSDB(CommandArgs commandArgs) + { + if (commandArgs.IMSDbFile == null) + { + return true; // Nothing to do + } + _out.WriteLine(Resources.CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___); + try + { + ModifyDocumentWithLogging(doc => doc.ChangeSettings(doc.Settings.ChangeTransitionSettings(p => + { + var progressMonitor = new CommandProgressMonitor(_out, new ProgressStatus(string.Empty)); + var path = commandArgs.IMSDbFile.GetFilePath(); + var name = commandArgs.IMSDbName ?? Path.GetFileNameWithoutExtension(path); + var lib = IonMobilityLibrary.CreateFromResults( + doc, null, false, name, path, + progressMonitor); + + return p.ChangeIonMobilityFiltering(p.IonMobilityFiltering.ChangeLibrary(lib)); + })), AuditLogEntry.SettingsLogFunction); + return true; + } + catch (Exception x) + { + if (!_out.IsErrorReported) + { + _out.WriteLine(Resources.CommandLine_GeneralException_Error___0_, x.Message); + } + else + { + _out.WriteLine(x.Message); + } + return false; + } + } + private IsotopeLabelType GetLabelTypeHelper(string label) { var mods = _doc.Settings.PeptideSettings.Modifications; diff --git a/pwiz_tools/Skyline/Properties/Resources.Designer.cs b/pwiz_tools/Skyline/Properties/Resources.Designer.cs index 1e14b945df..1b759b0082 100644 --- a/pwiz_tools/Skyline/Properties/Resources.Designer.cs +++ b/pwiz_tools/Skyline/Properties/Resources.Designer.cs @@ -5415,6 +5415,15 @@ public class Resources { } } + /// + /// Looks up a localized string similar to Creating an ion mobility library.... + /// + public static string CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___ { + get { + return ResourceManager.GetString("CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___", resourceCulture); + } + } + /// /// Looks up a localized string similar to Error: Importing an assay library to a document without an iRT calculator cannot create {0}, because it exists.. /// diff --git a/pwiz_tools/Skyline/Properties/Resources.resx b/pwiz_tools/Skyline/Properties/Resources.resx index 6c8bbaffb3..f0d5625471 100644 --- a/pwiz_tools/Skyline/Properties/Resources.resx +++ b/pwiz_tools/Skyline/Properties/Resources.resx @@ -11838,4 +11838,7 @@ If you do not have the original file, you may build the library with embedded sp Submitting an unhandled error report + + Creating an ion mobility library... + \ No newline at end of file diff --git a/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs b/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs index edc72e038e..65c0b9a2d9 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs @@ -200,18 +200,24 @@ private void TestCommandlineImport(string skyFile, string testPath) { if (File.Exists(testPath)) File.Delete(testPath); + var imsdbPath = TestFilesDir.GetTestPath("testlib.imsdb"); // Show anyone watching that work is being performed + // Create an ion mobility library after import just to exercise that code RunUI(() => { using (var longWait = new LongWaitDlg(SkylineWindow) { Message = "Running command-line import" }) { longWait.PerformWork(SkylineWindow, 500, () => + { RunCommand("--in=" + skyFile, "--import-file=" + TestFilesDir.GetTestPath(BSA_50fmol_TIMS_InfusionESI_10precd), - "--out=" + testPath)); + "--ionmobility-create-library=" + imsdbPath, + "--ionmobility-create-library-name=testlib", + "--out=" + testPath); + }); } }); - + AssertEx.FileExists(imsdbPath); VerifySerialization(testPath, false); } From 3721ec9a1f8c88de9b19ea87b093e5f7c1a983fe Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Fri, 23 Sep 2022 16:55:27 -0700 Subject: [PATCH 2/9] Changes from Brendan's code review: --ionmobility-create-library and --ionmobility-create-library-name are changed to --ionmobility-library-create and --ionmobility-library-name added a proper test various code tweaks to conform to our conventions --- .../Skyline/CommandArgUsage.Designer.cs | 10 +- pwiz_tools/Skyline/CommandArgUsage.resx | 6 +- pwiz_tools/Skyline/CommandArgs.cs | 28 ++--- pwiz_tools/Skyline/CommandLine.cs | 24 ++--- .../Skyline/Properties/Resources.Designer.cs | 6 +- pwiz_tools/Skyline/Properties/Resources.resx | 4 +- .../PerfCommandlineCreateImsDbTest.cs | 100 ++++++++++++++++++ .../Skyline/TestPerf/PerfMeasuredInverseK0.cs | 10 +- pwiz_tools/Skyline/TestPerf/TestPerf.csproj | 1 + 9 files changed, 142 insertions(+), 47 deletions(-) create mode 100644 pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs diff --git a/pwiz_tools/Skyline/CommandArgUsage.Designer.cs b/pwiz_tools/Skyline/CommandArgUsage.Designer.cs index 7d38ac3b90..2157914f8f 100644 --- a/pwiz_tools/Skyline/CommandArgUsage.Designer.cs +++ b/pwiz_tools/Skyline/CommandArgUsage.Designer.cs @@ -738,18 +738,18 @@ internal class CommandArgUsage { /// /// Looks up a localized string similar to Add an ion mobility library to the open document, based on its currently loaded chromatograms.. /// - internal static string _ionmobility_create_library { + internal static string _ionmobility_library_create { get { - return ResourceManager.GetString("_ionmobility_create_library", resourceCulture); + return ResourceManager.GetString("_ionmobility_library_create", resourceCulture); } } /// - /// Looks up a localized string similar to Name to give the ion mobility library in an –-ionmobility-create-library operation.. + /// Looks up a localized string similar to Name to give the ion mobility library in an –-ionmobility-library-create operation.. /// - internal static string _ionmobility_create_library_name { + internal static string _ionmobility_library_name { get { - return ResourceManager.GetString("_ionmobility_create_library_name", resourceCulture); + return ResourceManager.GetString("_ionmobility_library_name", resourceCulture); } } diff --git a/pwiz_tools/Skyline/CommandArgUsage.resx b/pwiz_tools/Skyline/CommandArgUsage.resx index 89de2c4854..4c66791d73 100644 --- a/pwiz_tools/Skyline/CommandArgUsage.resx +++ b/pwiz_tools/Skyline/CommandArgUsage.resx @@ -123,11 +123,11 @@ Specify a spectral library to be added to the open document. - + Add an ion mobility library to the open document, based on its currently loaded chromatograms. - - Name to give the ion mobility library in an –-ionmobility-create-library operation. + + Name to give the ion mobility library in an –-ionmobility-library-create operation. Runs a file line by line treating each line like a SkylineRunner/Cmd input. Useful for automating the execution of multiple commands. The open Skyline file remains active through all commands. diff --git a/pwiz_tools/Skyline/CommandArgs.cs b/pwiz_tools/Skyline/CommandArgs.cs index 0d3d189b9b..e59ad0947f 100644 --- a/pwiz_tools/Skyline/CommandArgs.cs +++ b/pwiz_tools/Skyline/CommandArgs.cs @@ -34,6 +34,7 @@ using pwiz.Skyline.Model; using pwiz.Skyline.Model.DocSettings; using pwiz.Skyline.Model.GroupComparison; +using pwiz.Skyline.Model.IonMobility; using pwiz.Skyline.Model.Irt; using pwiz.Skyline.Model.Results; using pwiz.Skyline.Model.Results.Scoring; @@ -216,15 +217,21 @@ public bool Saving public ShareType SharedFileType { get; private set; } // For creating a .imsdb ion mobility library - public static readonly Argument ARG_CREATE_IMSDB = new DocArgument(@"ionmobility-create-library", PATH_TO_FILE, - (c, p) => c.CreateIMSDB(p)); + public static readonly Argument ARG_IMSDB_CREATE = new DocArgument(@"ionmobility-library-create", () => GetPathToFile(IonMobilityDb.EXT), + (c, p) => c.ImsDbFile = p.ValueFullPath); // For creating a .imsdb ion mobility library - public static readonly Argument ARG_CREATE_IMSDB_NAME = new DocArgument(@"ionmobility-create-library-name", NAME_VALUE, - (c, p) => c.IMSDbName = p.Value); + public static readonly Argument ARG_IMSDB_NAME = new DocArgument(@"ionmobility-library-name", NAME_VALUE, + (c, p) => c.ImsDbName = p.Value); private static readonly ArgumentGroup GROUP_CREATE_IMSDB = new ArgumentGroup(() => CommandArgUsage.CommandArgs_GROUP_CREATE_IMSDB_Ion_Mobility_Library, false, - ARG_CREATE_IMSDB, ARG_CREATE_IMSDB_NAME); + ARG_IMSDB_CREATE, ARG_IMSDB_NAME) + { + Dependencies = + { + { ARG_IMSDB_NAME, ARG_IMSDB_CREATE }, + }, + }; public static readonly Argument ARG_IMPORT_FILE = new DocArgument(@"import-file", PATH_TO_FILE, (c, p) => c.ParseImportFile(p)); @@ -348,8 +355,8 @@ private bool ValidateMinimizeResultsArgs() return true; } - public MsDataFileUri IMSDbFile { get; private set; } - public string IMSDbName { get; private set; } + public string ImsDbFile { get; private set; } + public string ImsDbName { get; private set; } public List ReplicateFile { get; private set; } public string ReplicateName { get; private set; } @@ -375,11 +382,6 @@ private bool ValidateMinimizeResultsArgs() public double? LockmassTolerance { get; private set; } public LockMassParameters LockMassParameters { get { return new LockMassParameters(LockmassPositive, LockmassNegative, LockmassTolerance); } } - private void CreateIMSDB(NameValuePair pair) - { - IMSDbFile = new MsDataFilePath(pair.ValueFullPath); - } - private void ParseImportFile(NameValuePair pair) { ReplicateFile.Add(new MsDataFilePath(pair.ValueFullPath)); @@ -576,7 +578,7 @@ public bool AddDecoys } public bool CreatingIMSDB { - get { return IMSDbFile != null; } + get { return !string.IsNullOrEmpty(ImsDbFile); } } public bool ImportingResults { diff --git a/pwiz_tools/Skyline/CommandLine.cs b/pwiz_tools/Skyline/CommandLine.cs index 5ae37f3d2f..0bf802c365 100644 --- a/pwiz_tools/Skyline/CommandLine.cs +++ b/pwiz_tools/Skyline/CommandLine.cs @@ -376,7 +376,7 @@ private bool ProcessDocument(CommandArgs commandArgs) return false; } - if (!CreateIMSDB(commandArgs)) + if (commandArgs.ImsDbFile != null && !CreateImsDb(commandArgs)) { return false; } @@ -593,25 +593,23 @@ private bool RefineDocument(CommandArgs commandArgs) } } - private bool CreateIMSDB(CommandArgs commandArgs) + private bool CreateImsDb(CommandArgs commandArgs) { - if (commandArgs.IMSDbFile == null) - { - return true; // Nothing to do - } - _out.WriteLine(Resources.CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___); + var libName = commandArgs.ImsDbName ?? Path.GetFileNameWithoutExtension(commandArgs.ImsDbFile); + var message = string.Format( + Resources.CommandLine_CreateImsDb_Creating_ion_mobility_library___0___in___1_____, libName, + commandArgs.ImsDbFile); + _out.WriteLine(Resources.CommandLine_CreateImsDb_Creating_ion_mobility_library___0___in___1_____, libName, commandArgs.ImsDbFile); try { - ModifyDocumentWithLogging(doc => doc.ChangeSettings(doc.Settings.ChangeTransitionSettings(p => + ModifyDocumentWithLogging(doc => doc.ChangeSettings(doc.Settings.ChangeTransitionIonMobilityFiltering(ionMobilityFiltering => { - var progressMonitor = new CommandProgressMonitor(_out, new ProgressStatus(string.Empty)); - var path = commandArgs.IMSDbFile.GetFilePath(); - var name = commandArgs.IMSDbName ?? Path.GetFileNameWithoutExtension(path); + var progressMonitor = new CommandProgressMonitor(_out, new ProgressStatus(message)); var lib = IonMobilityLibrary.CreateFromResults( - doc, null, false, name, path, + doc, null, false, libName, commandArgs.ImsDbFile, progressMonitor); - return p.ChangeIonMobilityFiltering(p.IonMobilityFiltering.ChangeLibrary(lib)); + return ionMobilityFiltering.ChangeLibrary(lib); })), AuditLogEntry.SettingsLogFunction); return true; } diff --git a/pwiz_tools/Skyline/Properties/Resources.Designer.cs b/pwiz_tools/Skyline/Properties/Resources.Designer.cs index 1b759b0082..54c07de579 100644 --- a/pwiz_tools/Skyline/Properties/Resources.Designer.cs +++ b/pwiz_tools/Skyline/Properties/Resources.Designer.cs @@ -5416,11 +5416,11 @@ public class Resources { } /// - /// Looks up a localized string similar to Creating an ion mobility library.... + /// Looks up a localized string similar to Creating ion mobility library "{0}" in "{1}".... /// - public static string CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___ { + public static string CommandLine_CreateImsDb_Creating_ion_mobility_library___0___in___1_____ { get { - return ResourceManager.GetString("CommandLine_CreateIMSDB_Creating_an_ion_mobility_library___", resourceCulture); + return ResourceManager.GetString("CommandLine_CreateImsDb_Creating_ion_mobility_library___0___in___1_____", resourceCulture); } } diff --git a/pwiz_tools/Skyline/Properties/Resources.resx b/pwiz_tools/Skyline/Properties/Resources.resx index f0d5625471..6ca6c261fc 100644 --- a/pwiz_tools/Skyline/Properties/Resources.resx +++ b/pwiz_tools/Skyline/Properties/Resources.resx @@ -11838,7 +11838,7 @@ If you do not have the original file, you may build the library with embedded sp Submitting an unhandled error report - - Creating an ion mobility library... + + Creating ion mobility library "{0}" in "{1}"... \ No newline at end of file diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs new file mode 100644 index 0000000000..e5451d0036 --- /dev/null +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -0,0 +1,100 @@ +/* + * Original author: Brian Pratt , + * MacCoss Lab, Department of Genome Sciences, UW + * + * Copyright 2022 University of Washington - Seattle, WA + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +using System.Globalization; +using System.IO; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using pwiz.Skyline.Util.Extensions; +using pwiz.SkylineTestUtil; + +namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the global RunPerfTests flag is set +{ + /// + /// Verify commandline handling for ion mobility library building + /// + [TestClass] + public class TestCommandlineCreateImsDbPerf : AbstractFunctionalTestEx + { + + private const string rawFile = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; + + [TestMethod] + public void CommandlineCreateImsDbPerfTest() + { + TestFilesZip = GetPerfTestDataURL(@"PerfCommandlineCreateImsDbTest.zip"); + TestFilesPersistent = new[] { rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place + + RunFunctionalTest(); + } + protected override void DoTest() + { + var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); + var inFile = TestFilesDirs[0].GetTestPath(rawFile); + var outSky = TestFilesDirs[0].GetTestPath("ImsDbTest.sky"); + var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); + var imsdb = TestFilesDirs[0].GetTestPath("ImsDbTest.imsdb"); + var outCSV = TestFilesDirs[0].GetTestPath("ImsDbTest.csv"); + var output = RunCommand($"--in={template}", + $"--out={outSky}", + $"--import-transition-list={transitionList}", + $"--import-file={inFile}", + $"--ionmobility-library-create={imsdb}", + "--ionmobility-library-name=ImsDbTest", + "--report-name=Precursor CCS", + $"--report-file={outCSV}"); + AssertEx.FileExists(imsdb); + + // Compare to expected result + var expected = File.ReadAllLines(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv")); + var actual = File.ReadAllLines(outCSV); + AssertEx.AreEqual(expected.Length, actual.Length, $"Report difference - expected same line count"); + + + var i = 0; + + string Localize(string s) + { + return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). + Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); + } + + foreach (var line in expected) + { + var expectedLine = line; + if (CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator != + CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) + { + if (line.StartsWith("\"")) + { + var parts = line.Split('"'); + expectedLine = parts[1] + Localize(parts[2]); + } + else + { + expectedLine = Localize(expectedLine); + } + } + + AssertEx.AreEqual(expectedLine, actual[i++], $"Report difference at line {i}"); + } + } + + } +} diff --git a/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs b/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs index 65c0b9a2d9..edc72e038e 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfMeasuredInverseK0.cs @@ -200,24 +200,18 @@ private void TestCommandlineImport(string skyFile, string testPath) { if (File.Exists(testPath)) File.Delete(testPath); - var imsdbPath = TestFilesDir.GetTestPath("testlib.imsdb"); // Show anyone watching that work is being performed - // Create an ion mobility library after import just to exercise that code RunUI(() => { using (var longWait = new LongWaitDlg(SkylineWindow) { Message = "Running command-line import" }) { longWait.PerformWork(SkylineWindow, 500, () => - { RunCommand("--in=" + skyFile, "--import-file=" + TestFilesDir.GetTestPath(BSA_50fmol_TIMS_InfusionESI_10precd), - "--ionmobility-create-library=" + imsdbPath, - "--ionmobility-create-library-name=testlib", - "--out=" + testPath); - }); + "--out=" + testPath)); } }); - AssertEx.FileExists(imsdbPath); + VerifySerialization(testPath, false); } diff --git a/pwiz_tools/Skyline/TestPerf/TestPerf.csproj b/pwiz_tools/Skyline/TestPerf/TestPerf.csproj index 83d6d1eca6..326152eaac 100644 --- a/pwiz_tools/Skyline/TestPerf/TestPerf.csproj +++ b/pwiz_tools/Skyline/TestPerf/TestPerf.csproj @@ -141,6 +141,7 @@ + From 34e493616a82c8b8d8fd8592faac31851b816bf9 Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Mon, 26 Sep 2022 10:42:52 -0700 Subject: [PATCH 3/9] Extend this test to include an expected failure, and use of optional library name argument. Also convert from AbstractFunctionalTestEx to AbstractUnitTestEx --- .../PerfCommandlineCreateImsDbTest.cs | 100 ++++++++++-------- 1 file changed, 55 insertions(+), 45 deletions(-) diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs index e5451d0036..b2268a6068 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -30,7 +30,7 @@ namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the /// Verify commandline handling for ion mobility library building /// [TestClass] - public class TestCommandlineCreateImsDbPerf : AbstractFunctionalTestEx + public class TestCommandlineCreateImsDbPerf : AbstractUnitTestEx { private const string rawFile = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; @@ -38,62 +38,72 @@ public class TestCommandlineCreateImsDbPerf : AbstractFunctionalTestEx [TestMethod] public void CommandlineCreateImsDbPerfTest() { + TestFilesZip = GetPerfTestDataURL(@"PerfCommandlineCreateImsDbTest.zip"); TestFilesPersistent = new[] { rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place + TestFilesDir = new TestFilesDir(TestContext, TestFilesZip, ".", TestFilesPersistent); - RunFunctionalTest(); - } - protected override void DoTest() - { - var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); - var inFile = TestFilesDirs[0].GetTestPath(rawFile); - var outSky = TestFilesDirs[0].GetTestPath("ImsDbTest.sky"); - var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); - var imsdb = TestFilesDirs[0].GetTestPath("ImsDbTest.imsdb"); - var outCSV = TestFilesDirs[0].GetTestPath("ImsDbTest.csv"); - var output = RunCommand($"--in={template}", - $"--out={outSky}", - $"--import-transition-list={transitionList}", - $"--import-file={inFile}", - $"--ionmobility-library-create={imsdb}", - "--ionmobility-library-name=ImsDbTest", - "--report-name=Precursor CCS", - $"--report-file={outCSV}"); - AssertEx.FileExists(imsdb); - - // Compare to expected result - var expected = File.ReadAllLines(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv")); - var actual = File.ReadAllLines(outCSV); - AssertEx.AreEqual(expected.Length, actual.Length, $"Report difference - expected same line count"); + var badName = @"blorf"; + foreach (var libName in new[]{@"_myname", string.Empty, badName}) // Try with and without explicit library name, and with an illegal name + { + var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); + var inFile = TestFilesDirs[0].GetTestPath(rawFile); + var root = $"ImsDbTest{libName}"; + var outSky = TestFilesDirs[0].GetTestPath($"{root}.sky"); + var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); + var imsdb = TestFilesDirs[0].GetTestPath($"{(root.EndsWith(badName) ? root + @":?" : root)}.imsdb"); + var outCSV = TestFilesDirs[0].GetTestPath($"{root}.csv"); + var output = RunCommand($"--in={template}", + $"--out={outSky}", + $"--import-transition-list={transitionList}", + $"--import-file={inFile}", + $"--ionmobility-library-create={imsdb}", + string.IsNullOrEmpty(libName) ? string.Empty : $"--ionmobility-library-name={libName}", + "--report-name=Precursor CCS", + $"--report-file={outCSV}"); + if (Equals(libName, badName)) + { + // Expect failure because of illegal filename characters + AssertEx.Contains(output, $"--ionmobility-library-create failed"); + continue; + } + AssertEx.FileExists(imsdb); + AssertEx.Contains(output, $"Ion mobility library changed from \"None\" to \"{(string.IsNullOrEmpty(libName) ? root : libName)}\""); + // Compare to expected result + var expected = File.ReadAllLines(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv")); + var actual = File.ReadAllLines(outCSV); + AssertEx.AreEqual(expected.Length, actual.Length, $"Report difference - expected same line count"); - var i = 0; + var i = 0; - string Localize(string s) - { - return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). - Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); - } + string Localize(string s) + { + return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). + Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); + } - foreach (var line in expected) - { - var expectedLine = line; - if (CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator != - CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) + foreach (var line in expected) { - if (line.StartsWith("\"")) - { - var parts = line.Split('"'); - expectedLine = parts[1] + Localize(parts[2]); - } - else + var expectedLine = line; + if (CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator != + CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) { - expectedLine = Localize(expectedLine); + if (line.StartsWith("\"")) + { + var parts = line.Split('"'); + expectedLine = parts[1] + Localize(parts[2]); + } + else + { + expectedLine = Localize(expectedLine); + } } - } - AssertEx.AreEqual(expectedLine, actual[i++], $"Report difference at line {i}"); + AssertEx.AreEqual(expectedLine, actual[i++], $"Report difference at line {i}"); + } } + } } From 4aa851fa6107974231f5790145972d91463151da Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Tue, 27 Sep 2022 14:32:29 -0700 Subject: [PATCH 4/9] Add error handling for trying to create an imsdb file in a nonexistent directory Add more test conditions for commandline imsdb creation - now tests: normal operation support for not specifying "--ionmobility-library-name" error handling for illegal characters in imsdb filename error handling for non-existent subdirectories in imsdb file path error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" --- .../Model/IonMobility/IonMobilityDb.cs | 8 +++ .../PerfCommandlineCreateImsDbTest.cs | 72 +++++++++++++------ pwiz_tools/Skyline/TestUtil/AssertEx.cs | 22 ++++++ 3 files changed, 81 insertions(+), 21 deletions(-) diff --git a/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs b/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs index 4a91db9d14..97269fdc79 100644 --- a/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs +++ b/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs @@ -435,6 +435,14 @@ public override int GetHashCode() public static IonMobilityDb CreateIonMobilityDb(string path, string libraryName, bool minimized) { + if (!Directory.Exists(Path.GetDirectoryName(path))) + { + var message = + string.Format( + Resources.CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_, + path); + throw new DirectoryNotFoundException(message); + } const string libAuthority = BiblioSpecLiteLibrary.DEFAULT_AUTHORITY; const int majorVer = 0; // This will increment when we add data const int minorVer = DbLibInfo.SCHEMA_VERSION_CURRENT; diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs index b2268a6068..25c65364aa 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -21,6 +21,7 @@ using System.Globalization; using System.IO; using Microsoft.VisualStudio.TestTools.UnitTesting; +using pwiz.Skyline.Properties; using pwiz.Skyline.Util.Extensions; using pwiz.SkylineTestUtil; @@ -28,6 +29,12 @@ namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the { /// /// Verify commandline handling for ion mobility library building + /// Tests for: + /// normal operation + /// support for not specifying "--ionmobility-library-name" + /// error handling for illegal characters in imsdb filename + /// error handling for non-existent subdirectories in imsdb file path + /// error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" /// [TestClass] public class TestCommandlineCreateImsDbPerf : AbstractUnitTestEx @@ -43,8 +50,16 @@ public void CommandlineCreateImsDbPerfTest() TestFilesPersistent = new[] { rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place TestFilesDir = new TestFilesDir(TestContext, TestFilesZip, ".", TestFilesPersistent); - var badName = @"blorf"; - foreach (var libName in new[]{@"_myname", string.Empty, badName}) // Try with and without explicit library name, and with an illegal name + var badName = @"badname"; + var badPath = @"badpath"; + var badArgs = @"badargs"; + foreach (var libName in new[]{ + @"_myname", // normal operation + string.Empty, // support for not specifying "--ionmobility-library-name" + badName, // error handling for illegal characters in imsdb filename + badPath, // error handling for non-existent subdirectories in imsdb file path + badArgs } // error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" + ) { var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); var inFile = TestFilesDirs[0].GetTestPath(rawFile); @@ -52,46 +67,61 @@ public void CommandlineCreateImsDbPerfTest() var outSky = TestFilesDirs[0].GetTestPath($"{root}.sky"); var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); var imsdb = TestFilesDirs[0].GetTestPath($"{(root.EndsWith(badName) ? root + @":?" : root)}.imsdb"); + if (Equals(libName, badPath)) + { + imsdb = imsdb.Replace(TestFilesDirs[0].GetTestPath(string.Empty), badPath + "\\" + badPath); + } var outCSV = TestFilesDirs[0].GetTestPath($"{root}.csv"); var output = RunCommand($"--in={template}", $"--out={outSky}", $"--import-transition-list={transitionList}", $"--import-file={inFile}", - $"--ionmobility-library-create={imsdb}", + Equals(libName, badArgs) ? string.Empty : $"--ionmobility-library-create={imsdb}", string.IsNullOrEmpty(libName) ? string.Empty : $"--ionmobility-library-name={libName}", "--report-name=Precursor CCS", $"--report-file={outCSV}"); if (Equals(libName, badName)) { - // Expect failure because of illegal filename characters - AssertEx.Contains(output, $"--ionmobility-library-create failed"); + // Expect failure because of illegal characters in imsdb filename + AssertEx.Contains(output, + string.Format(Resources.ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, + imsdb, "--ionmobility-library-create")); + continue; + } + else if (Equals(libName, badPath)) + { + // Expect failure because of nonexistent subdirectories in path + AssertEx.ContainsSimilar(output, Resources.CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_); + continue; + } + else if (Equals(libName, badArgs)) + { + // Expect failure because of missing "--ionmobility-library-create" + AssertEx.Contains(output, + string.Format(Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, + "--ionmobility-library-name", "--ionmobility-library-create")); continue; } AssertEx.FileExists(imsdb); - AssertEx.Contains(output, $"Ion mobility library changed from \"None\" to \"{(string.IsNullOrEmpty(libName) ? root : libName)}\""); - // Compare to expected result + // Compare to expected result - may need to localize the expected copy to match the actual copy var expected = File.ReadAllLines(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv")); var actual = File.ReadAllLines(outCSV); AssertEx.AreEqual(expected.Length, actual.Length, $"Report difference - expected same line count"); - - var i = 0; - - string Localize(string s) + for (var i = 1; i < expected.Length; i++) // Skipping header, which may be localized { - return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). - Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); - } - - foreach (var line in expected) - { - var expectedLine = line; + var expectedLine = expected[i]; if (CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator != CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) { - if (line.StartsWith("\"")) + string Localize(string s) + { + return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). + Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); + } + if (expectedLine.StartsWith("\"")) { - var parts = line.Split('"'); + var parts = expectedLine.Split('"'); expectedLine = parts[1] + Localize(parts[2]); } else @@ -100,7 +130,7 @@ string Localize(string s) } } - AssertEx.AreEqual(expectedLine, actual[i++], $"Report difference at line {i}"); + AssertEx.AreEqual(expectedLine, actual[i], $"Difference in actual and expected report at line {i}"); } } diff --git a/pwiz_tools/Skyline/TestUtil/AssertEx.cs b/pwiz_tools/Skyline/TestUtil/AssertEx.cs index cd429d52a3..ae37ce7b27 100644 --- a/pwiz_tools/Skyline/TestUtil/AssertEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AssertEx.cs @@ -236,6 +236,28 @@ public static void Contains(string value, params string[] parts) } } + // Checks that string value could have been partially created with given format + // e.g. value "Oh no! My dog has fleas! Now what?" agrees with format "My {0} has {1}!" because it contains + // "My ", " has ", and "!" in that order. + public static void ContainsSimilar(string value, string format) + { + var testString = value; + foreach (var part in format.Split('{')) + { + var close = part.IndexOf('}'); + var find = part.Substring(close+1); + if (!string.IsNullOrEmpty(find)) + { + var index = testString.IndexOf(find, StringComparison.Ordinal); + if (index < 0) + { + Fail("The text '{0}' does not have expected format '{1}'", value, format); + } + testString = testString.Substring(index + find.Length); + } + } + } + public static void FileExists(string filePath, string message = null) { if (!File.Exists(filePath)) From d96b2fb18b54c6ae8c49f6765b55bc3adfc713b3 Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Fri, 30 Sep 2022 11:57:48 -0700 Subject: [PATCH 5/9] Centralize some code for comparing DSV files created with different culture settings, including automatically determining the kind of separator in use --- .../Skyline/Properties/Resources.Designer.cs | 10 ++ pwiz_tools/Skyline/Properties/Resources.resx | 3 + .../Skyline/Test/MProphetScoringModelTest.cs | 11 +- pwiz_tools/Skyline/Test/UtilTest.cs | 7 +- .../PerfCommandlineCreateImsDbTest.cs | 154 ++++++++---------- pwiz_tools/Skyline/TestUtil/AssertEx.cs | 75 ++++++--- pwiz_tools/Skyline/Util/Extensions/Text.cs | 45 +++++ 7 files changed, 186 insertions(+), 119 deletions(-) diff --git a/pwiz_tools/Skyline/Properties/Resources.Designer.cs b/pwiz_tools/Skyline/Properties/Resources.Designer.cs index 54c07de579..2c06f39bf6 100644 --- a/pwiz_tools/Skyline/Properties/Resources.Designer.cs +++ b/pwiz_tools/Skyline/Properties/Resources.Designer.cs @@ -32193,6 +32193,16 @@ public class Resources { } } + /// + /// Looks up a localized string similar to Unable to determine format of delimiter separated value file. + /// + public static string TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file { + get { + return ResourceManager.GetString("TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_v" + + "alue_file", resourceCulture); + } + } + /// /// Looks up a localized string similar to All Files. /// diff --git a/pwiz_tools/Skyline/Properties/Resources.resx b/pwiz_tools/Skyline/Properties/Resources.resx index 6ca6c261fc..231f15a9bd 100644 --- a/pwiz_tools/Skyline/Properties/Resources.resx +++ b/pwiz_tools/Skyline/Properties/Resources.resx @@ -11841,4 +11841,7 @@ If you do not have the original file, you may build the library with embedded sp Creating ion mobility library "{0}" in "{1}"... + + Unable to determine format of delimiter separated value file + \ No newline at end of file diff --git a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs index 9fcc2fa278..d9478924f4 100644 --- a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs +++ b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs @@ -377,14 +377,7 @@ public Data(string dataFile) // Determine separator (comma, space, or tab). var headerTest = lines[0].Trim(); - var commaCount = headerTest.Split(TextUtil.SEPARATOR_CSV).Length; - var spaceCount = headerTest.Split(TextUtil.SEPARATOR_SPACE).Length; - var tabCount = headerTest.Split(TextUtil.SEPARATOR_TSV).Length; - var maxCount = Math.Max(Math.Max(commaCount, spaceCount), tabCount); - var separator = - commaCount == maxCount - ? TextUtil.SEPARATOR_CSV - : spaceCount == maxCount ? TextUtil.SEPARATOR_SPACE : TextUtil.SEPARATOR_TSV; + var separator = TextUtil.DetermineDsvDelimiter(lines, out var columnCount); // Find header labels. If all headings are numeric, then no header. Header = headerTest.ParseDsvFields(separator); @@ -408,7 +401,7 @@ public Data(string dataFile) } // Fill out data matrix. - Items = new string[lineCount - dataIndex,maxCount]; + Items = new string[lineCount - dataIndex,columnCount]; for (int i = 0; i < Items.GetLength(0); i++) { var items = lines[i + dataIndex].Trim().ParseDsvFields(separator); diff --git a/pwiz_tools/Skyline/Test/UtilTest.cs b/pwiz_tools/Skyline/Test/UtilTest.cs index 835127d5d2..6873c3818c 100644 --- a/pwiz_tools/Skyline/Test/UtilTest.cs +++ b/pwiz_tools/Skyline/Test/UtilTest.cs @@ -90,8 +90,13 @@ private static void TestDsvFields(char punctuation, char separator) writer.Write(separator); writer.WriteDsvField(field, separator); } - var fieldsOut = sb.ToString().ParseDsvFields(separator); + + var line = sb.ToString(); + var fieldsOut = line.ParseDsvFields(separator); Assert.IsTrue(ArrayUtil.EqualsDeep(fields, fieldsOut), "while parsing:\n"+sb+"\nexpected:\n" + string.Join("\n", fields) + "\n\ngot:\n" + string.Join("\n", fieldsOut)); + var detectedSeparator = TextUtil.DetermineDsvDelimiter(new[] { line }, out var detectedColumnCount); + Assert.AreEqual(separator, detectedSeparator); + Assert.AreEqual(fields.Length, detectedColumnCount); } [TestMethod] diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs index 25c65364aa..6f97aaf37d 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -18,11 +18,8 @@ */ -using System.Globalization; -using System.IO; using Microsoft.VisualStudio.TestTools.UnitTesting; using pwiz.Skyline.Properties; -using pwiz.Skyline.Util.Extensions; using pwiz.SkylineTestUtil; namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the global RunPerfTests flag is set @@ -31,7 +28,7 @@ namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the /// Verify commandline handling for ion mobility library building /// Tests for: /// normal operation - /// support for not specifying "--ionmobility-library-name" + /// support for optionally not specifying "--ionmobility-library-name" /// error handling for illegal characters in imsdb filename /// error handling for non-existent subdirectories in imsdb file path /// error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" @@ -40,101 +37,84 @@ namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the public class TestCommandlineCreateImsDbPerf : AbstractUnitTestEx { - private const string rawFile = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; + private const string _rawFile = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; + private const string _normal = @"_normal"; + private const string _badName = @"badname"; + private const string _impliedName = @"impliedname"; + private const string _badPath = @"badpath"; + private const string _badArgs = @"badargs"; [TestMethod] public void CommandlineCreateImsDbPerfTest() { TestFilesZip = GetPerfTestDataURL(@"PerfCommandlineCreateImsDbTest.zip"); - TestFilesPersistent = new[] { rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place + TestFilesPersistent = new[] { _rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place TestFilesDir = new TestFilesDir(TestContext, TestFilesZip, ".", TestFilesPersistent); - var badName = @"badname"; - var badPath = @"badpath"; - var badArgs = @"badargs"; - foreach (var libName in new[]{ - @"_myname", // normal operation - string.Empty, // support for not specifying "--ionmobility-library-name" - badName, // error handling for illegal characters in imsdb filename - badPath, // error handling for non-existent subdirectories in imsdb file path - badArgs } // error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" - ) - { - var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); - var inFile = TestFilesDirs[0].GetTestPath(rawFile); - var root = $"ImsDbTest{libName}"; - var outSky = TestFilesDirs[0].GetTestPath($"{root}.sky"); - var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); - var imsdb = TestFilesDirs[0].GetTestPath($"{(root.EndsWith(badName) ? root + @":?" : root)}.imsdb"); - if (Equals(libName, badPath)) - { - imsdb = imsdb.Replace(TestFilesDirs[0].GetTestPath(string.Empty), badPath + "\\" + badPath); - } - var outCSV = TestFilesDirs[0].GetTestPath($"{root}.csv"); - var output = RunCommand($"--in={template}", - $"--out={outSky}", - $"--import-transition-list={transitionList}", - $"--import-file={inFile}", - Equals(libName, badArgs) ? string.Empty : $"--ionmobility-library-create={imsdb}", - string.IsNullOrEmpty(libName) ? string.Empty : $"--ionmobility-library-name={libName}", - "--report-name=Precursor CCS", - $"--report-file={outCSV}"); - if (Equals(libName, badName)) - { - // Expect failure because of illegal characters in imsdb filename - AssertEx.Contains(output, - string.Format(Resources.ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, - imsdb, "--ionmobility-library-create")); - continue; - } - else if (Equals(libName, badPath)) - { - // Expect failure because of nonexistent subdirectories in path - AssertEx.ContainsSimilar(output, Resources.CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_); - continue; - } - else if (Equals(libName, badArgs)) - { - // Expect failure because of missing "--ionmobility-library-create" - AssertEx.Contains(output, - string.Format(Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, - "--ionmobility-library-name", "--ionmobility-library-create")); - continue; - } - AssertEx.FileExists(imsdb); + TestCreateImsdb(_normal); // normal operation + TestCreateImsdb(_impliedName); // support for optionally not specifying "--ionmobility-library-name" + TestCreateImsdb(_badName); // error handling for illegal characters in imsdb filename + TestCreateImsdb(_badPath); // error handling for non-existent subdirectories in imsdb file path + TestCreateImsdb(_badArgs); // error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" - // Compare to expected result - may need to localize the expected copy to match the actual copy - var expected = File.ReadAllLines(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv")); - var actual = File.ReadAllLines(outCSV); - AssertEx.AreEqual(expected.Length, actual.Length, $"Report difference - expected same line count"); - for (var i = 1; i < expected.Length; i++) // Skipping header, which may be localized - { - var expectedLine = expected[i]; - if (CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator != - CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) - { - string Localize(string s) - { - return s.Replace(TextUtil.SEPARATOR_CSV, TextUtil.SEPARATOR_CSV_INTL). - Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator); - } - if (expectedLine.StartsWith("\"")) - { - var parts = expectedLine.Split('"'); - expectedLine = parts[1] + Localize(parts[2]); - } - else - { - expectedLine = Localize(expectedLine); - } - } + } - AssertEx.AreEqual(expectedLine, actual[i], $"Difference in actual and expected report at line {i}"); - } + private void TestCreateImsdb(string testMode) + { + var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); + var inFile = TestFilesDirs[0].GetTestPath(_rawFile); + var root = $"ImsDbTest{testMode}"; + var outSky = TestFilesDirs[0].GetTestPath($"{root}.sky"); + var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); + var imsdb = TestFilesDirs[0].GetTestPath($"{(root.EndsWith(_badName) ? root + @":?" : root)}.imsdb"); + if (Equals(testMode, _badPath)) + { + imsdb = imsdb.Replace(TestFilesDirs[0].GetTestPath(string.Empty), _badPath + "\\" + _badPath); } - } + var outCSV = TestFilesDirs[0].GetTestPath($"{root}.csv"); + var output = RunCommand($"--in={template}", + $"--out={outSky}", + $"--import-transition-list={transitionList}", + $"--import-file={inFile}", + Equals(testMode, _badArgs) ? string.Empty : $"--ionmobility-library-create={imsdb}", + Equals(testMode, _impliedName) ? string.Empty : $"--ionmobility-library-name={testMode}", + "--report-name=Precursor CCS", + $"--report-file={outCSV}"); + if (Equals(testMode, _badName)) + { + // Expect failure because of illegal characters in imsdb filename + AssertEx.Contains(output, + string.Format( + Resources + .ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, + imsdb, "--ionmobility-library-create")); + return; + } + else if (Equals(testMode, _badPath)) + { + // Expect failure because of nonexistent subdirectories in path + AssertEx.AreComparableStrings( + Resources + .CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_, + output); + return; + } + else if (Equals(testMode, _badArgs)) + { + // Expect failure because of missing "--ionmobility-library-create" + AssertEx.Contains(output, + string.Format( + Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, + "--ionmobility-library-name", "--ionmobility-library-create")); + return; + } + + AssertEx.FileExists(imsdb); + // Compare to expected report - may need to localize the expected copy to match the actual copy + AssertEx.AreEquivalentDsvFiles(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv"), outCSV, true); + } } } diff --git a/pwiz_tools/Skyline/TestUtil/AssertEx.cs b/pwiz_tools/Skyline/TestUtil/AssertEx.cs index ae37ce7b27..76f9ca1823 100644 --- a/pwiz_tools/Skyline/TestUtil/AssertEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AssertEx.cs @@ -236,28 +236,6 @@ public static void Contains(string value, params string[] parts) } } - // Checks that string value could have been partially created with given format - // e.g. value "Oh no! My dog has fleas! Now what?" agrees with format "My {0} has {1}!" because it contains - // "My ", " has ", and "!" in that order. - public static void ContainsSimilar(string value, string format) - { - var testString = value; - foreach (var part in format.Split('{')) - { - var close = part.IndexOf('}'); - var find = part.Substring(close+1); - if (!string.IsNullOrEmpty(find)) - { - var index = testString.IndexOf(find, StringComparison.Ordinal); - if (index < 0) - { - Fail("The text '{0}' does not have expected format '{1}'", value, format); - } - testString = testString.Substring(index + find.Length); - } - } - } - public static void FileExists(string filePath, string message = null) { if (!File.Exists(filePath)) @@ -936,6 +914,59 @@ public static void FileEquals(string path1, string path2, Dictionary + /// Compare two DSV files, accounting for possible L10N differences + /// + public static void AreEquivalentDsvFiles(string path1, string path2, bool hasHeaders) + { + var lines1 = File.ReadAllLines(path1); + var lines2 = File.ReadAllLines(path2); + AssertEx.AreEqual(lines1.Length, lines2.Length, "Expected same line count"); + if (lines1.Length == 0) + { + return; + } + + var sep1 = TextUtil.DetermineDsvDelimiter(lines1, out var colCount1); + var sep2 = TextUtil.DetermineDsvDelimiter(lines2, out var colCount2); + var reader1 = new DsvFileReader(path1, sep1, hasHeaders); + var reader2 = new DsvFileReader(path2, sep2, hasHeaders); + for (var lineNum = 0; lineNum < lines1.Length; lineNum++) + { + var cols1 = reader1.ReadLine(); + var cols2 = reader2.ReadLine(); + if ((cols1 == null) && (cols2 == null)) + { + return; + } + if ((cols1 == null) || (cols2 == null)) + { + AssertEx.Fail("Unexpected failure comparing DSV files"); + return; // Avoids a null check warning below + } + for (var colNum = 0; colNum < cols1.Length; colNum++) + { + var same = Equals(cols1[colNum], cols2[colNum]); + + if (!same) + { + // Possibly a decimal value, or even a field like "1.234[M+H]" vs "1,234[M+H]" + string Dotted(string val) + { + return val.Replace(CultureInfo.InvariantCulture.NumberFormat.NumberDecimalSeparator, @"_dot_"). + Replace(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator, @"_dot_"); + } + same = Equals(Dotted(cols1[colNum]), Dotted(cols2[colNum])); + } + + if (!same) + { + AssertEx.AreEqual(cols1[colNum], cols2[colNum], $"Difference at row {lineNum} column {colNum}"); + } + } + } + } + public static void FieldsEqual(string target, string actual, int countFields, bool allowForNumericPrecisionDifferences = false) { FieldsEqual(target, actual, countFields, null, allowForNumericPrecisionDifferences); diff --git a/pwiz_tools/Skyline/Util/Extensions/Text.cs b/pwiz_tools/Skyline/Util/Extensions/Text.cs index 43d4aa60e3..f254e4959e 100644 --- a/pwiz_tools/Skyline/Util/Extensions/Text.cs +++ b/pwiz_tools/Skyline/Util/Extensions/Text.cs @@ -716,6 +716,51 @@ string DomainMapper(Match match) return false; } } + + /// + /// Examine the lines of a DSV file an attempt to determine what kind of delimiter it uses + /// + /// lines of th file + /// return value: column count + /// the identified delimiter + /// thrown when we can't figure it out + public static char DetermineDsvDelimiter(string[] lines, out int columnCount) + { + + // If a candidate delimiter yields different column counts line to line, it's probably not the right one. + // So parse some distance in to see which delimiters give a consistent column count. + var countsPerLinePerCandidateDelimiter = new Dictionary> + { + { TextUtil.SEPARATOR_CSV, new List()}, + { TextUtil.SEPARATOR_SPACE, new List()}, + { TextUtil.SEPARATOR_TSV, new List()}, + { TextUtil.SEPARATOR_CSV_INTL, new List()} + }; + + for (var lineNum = 0; lineNum < Math.Min(100, lines.Length); lineNum++) + { + foreach (var sep in countsPerLinePerCandidateDelimiter.Keys) + { + countsPerLinePerCandidateDelimiter[sep].Add((new DsvFileReader(new StringReader(lines[lineNum]), sep)).NumberOfFields); + } + } + + var likelyCandidates = + countsPerLinePerCandidateDelimiter.Where(kvp => kvp.Value.Distinct().Count() == 1).ToArray(); + if (likelyCandidates.Length > 0) + { + // The candidate that yields the highest column count wins + var maxColumnCount = likelyCandidates.Max(kvp => kvp.Value[0]); + if (likelyCandidates.Count(kvp => Equals(maxColumnCount, kvp.Value[0])) == 1) + { + var delimiter = likelyCandidates.First(kvp => Equals(maxColumnCount, kvp.Value[0])).Key; + columnCount = maxColumnCount; + return delimiter; + } + } + + throw new LineColNumberedIoException(Resources.TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file, 1, 1); + } } /// From a2d2824a76637578ab7f23ce5a33eb8a347769bb Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Fri, 30 Sep 2022 13:57:38 -0700 Subject: [PATCH 6/9] previous commit broke writing .imsb to implied current working directory --- pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs b/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs index 97269fdc79..4e89900c6e 100644 --- a/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs +++ b/pwiz_tools/Skyline/Model/IonMobility/IonMobilityDb.cs @@ -435,7 +435,8 @@ public override int GetHashCode() public static IonMobilityDb CreateIonMobilityDb(string path, string libraryName, bool minimized) { - if (!Directory.Exists(Path.GetDirectoryName(path))) + var directoryName = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(directoryName) && !Directory.Exists(directoryName)) { var message = string.Format( From 54213eebfa1b93ac0bd72fd2f43d652f821449e8 Mon Sep 17 00:00:00 2001 From: brendanx67 Date: Sun, 2 Oct 2022 12:30:39 -0700 Subject: [PATCH 7/9] - clean-up test with increased abstraction --- .../PerfCommandlineCreateImsDbTest.cs | 138 ++++++++++-------- 1 file changed, 76 insertions(+), 62 deletions(-) diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs index 6f97aaf37d..b3f38027b4 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -18,7 +18,9 @@ */ +using System.IO; using Microsoft.VisualStudio.TestTools.UnitTesting; +using pwiz.Skyline; using pwiz.Skyline.Properties; using pwiz.SkylineTestUtil; @@ -37,84 +39,96 @@ namespace TestPerf // Note: tests in the "TestPerf" namespace only run when the public class TestCommandlineCreateImsDbPerf : AbstractUnitTestEx { - private const string _rawFile = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; - private const string _normal = @"_normal"; - private const string _badName = @"badname"; - private const string _impliedName = @"impliedname"; - private const string _badPath = @"badpath"; - private const string _badArgs = @"badargs"; + private const string RAW_FILE = "010521_Enamine_U6601911_A1_f100_pos_1_1_1086.d"; + + private string GetTestPath(string relativePath) + { + return TestFilesDirs[0].GetTestPath(relativePath); + } + + private string GetImsDbFileName(string testMode) + { + return $"ImsDbTest{testMode}.imsdb"; + } + + private string GetImsDbFilePath(string testMode) + { + return GetTestPath(GetImsDbFileName(testMode)); + } [TestMethod] public void CommandlineCreateImsDbPerfTest() { - TestFilesZip = GetPerfTestDataURL(@"PerfCommandlineCreateImsDbTest.zip"); - TestFilesPersistent = new[] { _rawFile }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place + TestFilesPersistent = new[] { RAW_FILE }; // list of files that we'd like to unzip alongside parent zipFile, and (re)use in place TestFilesDir = new TestFilesDir(TestContext, TestFilesZip, ".", TestFilesPersistent); - TestCreateImsdb(_normal); // normal operation - TestCreateImsdb(_impliedName); // support for optionally not specifying "--ionmobility-library-name" - TestCreateImsdb(_badName); // error handling for illegal characters in imsdb filename - TestCreateImsdb(_badPath); // error handling for non-existent subdirectories in imsdb file path - TestCreateImsdb(_badArgs); // error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create" + // Normal use + const string normal = @"normal"; + TestCreateImsdb(normal, GetImsDbFilePath(normal)); + + // Support for optionally not specifying "--ionmobility-library-name" + TestCreateImsdb(null, GetImsDbFilePath(@"implied-name")); + // Expect failure because of illegal characters in imsdb filename + string imsdbPathBadName = GetImsDbFilePath("bad-name:?"); + string output = TestCreateImsdb(@"bad-name", imsdbPathBadName, true); + AssertEx.Contains(output, + string.Format( + Resources.ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, + imsdbPathBadName, CommandArgs.ARG_IMSDB_CREATE.ArgumentText)); + + // Expect failure because of nonexistent subdirectories in path + const string badPath = @"bad-path"; + output = TestCreateImsdb(badPath, Path.Combine(badPath, GetImsDbFileName(badPath)), true); + AssertEx.AreComparableStrings( + Resources.CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_, + output); + + // Expect failure because of missing "--ionmobility-library-create" + output = TestCreateImsdb(@"bad-args", null, true); + AssertEx.Contains(output, + string.Format( + Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, + CommandArgs.ARG_IMSDB_NAME.ArgumentText, CommandArgs.ARG_IMSDB_CREATE.ArgumentText)); } - private void TestCreateImsdb(string testMode) + private string TestCreateImsdb(string imsdbName, string imsdbPath, bool errorExpected = false) { - var template = TestFilesDirs[0].GetTestPath("Scripps_IMS_Template.sky"); - var inFile = TestFilesDirs[0].GetTestPath(_rawFile); - var root = $"ImsDbTest{testMode}"; - var outSky = TestFilesDirs[0].GetTestPath($"{root}.sky"); - var transitionList = TestFilesDirs[0].GetTestPath("test_run_1_transition_list.csv"); - var imsdb = TestFilesDirs[0].GetTestPath($"{(root.EndsWith(_badName) ? root + @":?" : root)}.imsdb"); - if (Equals(testMode, _badPath)) - { - imsdb = imsdb.Replace(TestFilesDirs[0].GetTestPath(string.Empty), _badPath + "\\" + _badPath); - } + string reportFilePath = GetTestPath("Scripps_CCS_report.csv"); + var output = RunCommand(GetPathArg(CommandArgs.ARG_IN, "Scripps_IMS_Template.sky"), + GetPathArg(CommandArgs.ARG_OUT, "Scripps_IMS_DB.sky"), + GetPathArg(CommandArgs.ARG_IMPORT_TRANSITION_LIST, "test_run_1_transition_list.csv"), + GetPathArg(CommandArgs.ARG_IMPORT_FILE, RAW_FILE), + GetOptionalArg(CommandArgs.ARG_IMSDB_CREATE, imsdbPath), + GetOptionalArg(CommandArgs.ARG_IMSDB_NAME, imsdbName), + GetArg(CommandArgs.ARG_REPORT_NAME, "Precursor CCS"), + GetArg(CommandArgs.ARG_REPORT_FILE, reportFilePath)); - var outCSV = TestFilesDirs[0].GetTestPath($"{root}.csv"); - var output = RunCommand($"--in={template}", - $"--out={outSky}", - $"--import-transition-list={transitionList}", - $"--import-file={inFile}", - Equals(testMode, _badArgs) ? string.Empty : $"--ionmobility-library-create={imsdb}", - Equals(testMode, _impliedName) ? string.Empty : $"--ionmobility-library-name={testMode}", - "--report-name=Precursor CCS", - $"--report-file={outCSV}"); - if (Equals(testMode, _badName)) - { - // Expect failure because of illegal characters in imsdb filename - AssertEx.Contains(output, - string.Format( - Resources - .ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, - imsdb, "--ionmobility-library-create")); - return; - } - else if (Equals(testMode, _badPath)) + if (!errorExpected) { - // Expect failure because of nonexistent subdirectories in path - AssertEx.AreComparableStrings( - Resources - .CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_, - output); - return; - } - else if (Equals(testMode, _badArgs)) - { - // Expect failure because of missing "--ionmobility-library-create" - AssertEx.Contains(output, - string.Format( - Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, - "--ionmobility-library-name", "--ionmobility-library-create")); - return; + AssertEx.FileExists(imsdbPath); + + // Compare to expected report - may need to localize the expected copy to match the actual copy + AssertEx.AreEquivalentDsvFiles(GetTestPath("ImsDbTest_expected.csv"), reportFilePath, true); } - AssertEx.FileExists(imsdb); + return output; + } + + private string GetArg(CommandArgs.Argument arg, string value) + { + return arg.GetArgumentTextWithValue(value); + } - // Compare to expected report - may need to localize the expected copy to match the actual copy - AssertEx.AreEquivalentDsvFiles(TestFilesDirs[0].GetTestPath("ImsDbTest_expected.csv"), outCSV, true); + private string GetOptionalArg(CommandArgs.Argument arg, string value) + { + return string.IsNullOrEmpty(value) ? string.Empty : GetArg(arg, value); + } + + private string GetPathArg(CommandArgs.Argument arg, string value) + { + return GetArg(arg, GetTestPath(value)); } } } From 05550628efaaac0ad8114d233cf83890d36003e4 Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Mon, 3 Oct 2022 12:21:26 -0700 Subject: [PATCH 8/9] More code cleanup from Brendan's review. In particular move the DSV separator detection to the test support namespace and make it clear in comments that it isn't robust enough for general use. --- .../Skyline/Test/MProphetScoringModelTest.cs | 4 +- pwiz_tools/Skyline/Test/UtilTest.cs | 7 ++- .../Skyline/TestUtil/AbstractUnitTestEx.cs | 51 +++++++++++++++++++ pwiz_tools/Skyline/TestUtil/AssertEx.cs | 20 +++----- pwiz_tools/Skyline/Util/Extensions/Text.cs | 45 ---------------- 5 files changed, 65 insertions(+), 62 deletions(-) diff --git a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs index d9478924f4..6c409da63e 100644 --- a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs +++ b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs @@ -32,7 +32,7 @@ namespace pwiz.SkylineTest { [TestClass] - public class MProphetScoringModelTest : AbstractUnitTest + public class MProphetScoringModelTest : AbstractUnitTestEx { private const string ZIP_FILE = @"Test\MProphetScoringModelTest.zip"; // Not L10N @@ -377,7 +377,7 @@ public Data(string dataFile) // Determine separator (comma, space, or tab). var headerTest = lines[0].Trim(); - var separator = TextUtil.DetermineDsvDelimiter(lines, out var columnCount); + var separator = DetermineDsvDelimiter(lines, out var columnCount); // Find header labels. If all headings are numeric, then no header. Header = headerTest.ParseDsvFields(separator); diff --git a/pwiz_tools/Skyline/Test/UtilTest.cs b/pwiz_tools/Skyline/Test/UtilTest.cs index 6873c3818c..acc17db310 100644 --- a/pwiz_tools/Skyline/Test/UtilTest.cs +++ b/pwiz_tools/Skyline/Test/UtilTest.cs @@ -93,8 +93,11 @@ private static void TestDsvFields(char punctuation, char separator) var line = sb.ToString(); var fieldsOut = line.ParseDsvFields(separator); - Assert.IsTrue(ArrayUtil.EqualsDeep(fields, fieldsOut), "while parsing:\n"+sb+"\nexpected:\n" + string.Join("\n", fields) + "\n\ngot:\n" + string.Join("\n", fieldsOut)); - var detectedSeparator = TextUtil.DetermineDsvDelimiter(new[] { line }, out var detectedColumnCount); + Assert.IsTrue(ArrayUtil.EqualsDeep(fields, fieldsOut), + TextUtil.LineSeparate("while parsing:", line, string.Empty, + "expected:", TextUtil.LineSeparate(fields), string.Empty, + "got:",TextUtil.LineSeparate(fieldsOut))); + var detectedSeparator = AbstractUnitTestEx.DetermineDsvDelimiter(new[] { line }, out var detectedColumnCount); Assert.AreEqual(separator, detectedSeparator); Assert.AreEqual(fields.Length, detectedColumnCount); } diff --git a/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs b/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs index 2f72c1e60a..a8bb144fad 100644 --- a/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs @@ -17,6 +17,7 @@ * limitations under the License. */ +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -25,6 +26,7 @@ using pwiz.Skyline; using pwiz.Skyline.Model; using pwiz.Skyline.Model.Results; +using pwiz.Skyline.Properties; using pwiz.Skyline.Util.Extensions; @@ -129,5 +131,54 @@ protected static string RunCommand(params string[] inputArgs) AssertEx.ConvertedSmallMoleculeDocumentIsSimilar(docOriginal, doc, Path.GetDirectoryName(docPath), mode); return doc; } + + /// + /// Examine the lines of a DSV file an attempt to determine what kind of delimiter it uses + /// N.B. NOT ROBUST ENOUGH FOR GENERAL USE - would likely fail, for example, on data that has + /// irregular column counts. But still useful in the test context where we aren't handed random + /// data sets from users. + /// + /// lines of the file + /// return value: column count + /// the identified delimiter + /// thrown when we can't figure it out + public static char DetermineDsvDelimiter(string[] lines, out int columnCount) + { + + // If a candidate delimiter yields different column counts line to line, it's probably not the right one. + // So parse some distance in to see which delimiters give a consistent column count. + // NOTE we do see files like that in the wild, but not in our test suite + var countsPerLinePerCandidateDelimiter = new Dictionary> + { + { TextUtil.SEPARATOR_CSV, new List()}, + { TextUtil.SEPARATOR_SPACE, new List()}, + { TextUtil.SEPARATOR_TSV, new List()}, + { TextUtil.SEPARATOR_CSV_INTL, new List()} + }; + + for (var lineNum = 0; lineNum < Math.Min(100, lines.Length); lineNum++) + { + foreach (var sep in countsPerLinePerCandidateDelimiter.Keys) + { + countsPerLinePerCandidateDelimiter[sep].Add((new DsvFileReader(new StringReader(lines[lineNum]), sep)).NumberOfFields); + } + } + + var likelyCandidates = + countsPerLinePerCandidateDelimiter.Where(kvp => kvp.Value.Distinct().Count() == 1).ToArray(); + if (likelyCandidates.Length > 0) + { + // The candidate that yields the highest column count wins + var maxColumnCount = likelyCandidates.Max(kvp => kvp.Value[0]); + if (likelyCandidates.Count(kvp => Equals(maxColumnCount, kvp.Value[0])) == 1) + { + var delimiter = likelyCandidates.First(kvp => Equals(maxColumnCount, kvp.Value[0])).Key; + columnCount = maxColumnCount; + return delimiter; + } + } + + throw new LineColNumberedIoException(Resources.TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file, 1, 1); + } } } diff --git a/pwiz_tools/Skyline/TestUtil/AssertEx.cs b/pwiz_tools/Skyline/TestUtil/AssertEx.cs index 76f9ca1823..116b797096 100644 --- a/pwiz_tools/Skyline/TestUtil/AssertEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AssertEx.cs @@ -927,22 +927,16 @@ public static void AreEquivalentDsvFiles(string path1, string path2, bool hasHea return; } - var sep1 = TextUtil.DetermineDsvDelimiter(lines1, out var colCount1); - var sep2 = TextUtil.DetermineDsvDelimiter(lines2, out var colCount2); - var reader1 = new DsvFileReader(path1, sep1, hasHeaders); - var reader2 = new DsvFileReader(path2, sep2, hasHeaders); + var sep1 = AbstractUnitTestEx.DetermineDsvDelimiter(lines1, out var colCount1); + var sep2 = AbstractUnitTestEx.DetermineDsvDelimiter(lines2, out var colCount2); for (var lineNum = 0; lineNum < lines1.Length; lineNum++) { - var cols1 = reader1.ReadLine(); - var cols2 = reader2.ReadLine(); - if ((cols1 == null) && (cols2 == null)) + var cols1 = lines1[lineNum].ParseDsvFields(sep1); + var cols2 = lines2[lineNum].ParseDsvFields(sep2); + AssertEx.AreEqual(cols1.Length, cols2.Length, $"Expected same column count at line {lineNum}"); + if (hasHeaders && Equals(lineNum, 0) && !Equals(CultureInfo.CurrentCulture.TwoLetterISOLanguageName, @"en")) { - return; - } - if ((cols1 == null) || (cols2 == null)) - { - AssertEx.Fail("Unexpected failure comparing DSV files"); - return; // Avoids a null check warning below + continue; // Don't expect localized headers to match } for (var colNum = 0; colNum < cols1.Length; colNum++) { diff --git a/pwiz_tools/Skyline/Util/Extensions/Text.cs b/pwiz_tools/Skyline/Util/Extensions/Text.cs index f254e4959e..43d4aa60e3 100644 --- a/pwiz_tools/Skyline/Util/Extensions/Text.cs +++ b/pwiz_tools/Skyline/Util/Extensions/Text.cs @@ -716,51 +716,6 @@ string DomainMapper(Match match) return false; } } - - /// - /// Examine the lines of a DSV file an attempt to determine what kind of delimiter it uses - /// - /// lines of th file - /// return value: column count - /// the identified delimiter - /// thrown when we can't figure it out - public static char DetermineDsvDelimiter(string[] lines, out int columnCount) - { - - // If a candidate delimiter yields different column counts line to line, it's probably not the right one. - // So parse some distance in to see which delimiters give a consistent column count. - var countsPerLinePerCandidateDelimiter = new Dictionary> - { - { TextUtil.SEPARATOR_CSV, new List()}, - { TextUtil.SEPARATOR_SPACE, new List()}, - { TextUtil.SEPARATOR_TSV, new List()}, - { TextUtil.SEPARATOR_CSV_INTL, new List()} - }; - - for (var lineNum = 0; lineNum < Math.Min(100, lines.Length); lineNum++) - { - foreach (var sep in countsPerLinePerCandidateDelimiter.Keys) - { - countsPerLinePerCandidateDelimiter[sep].Add((new DsvFileReader(new StringReader(lines[lineNum]), sep)).NumberOfFields); - } - } - - var likelyCandidates = - countsPerLinePerCandidateDelimiter.Where(kvp => kvp.Value.Distinct().Count() == 1).ToArray(); - if (likelyCandidates.Length > 0) - { - // The candidate that yields the highest column count wins - var maxColumnCount = likelyCandidates.Max(kvp => kvp.Value[0]); - if (likelyCandidates.Count(kvp => Equals(maxColumnCount, kvp.Value[0])) == 1) - { - var delimiter = likelyCandidates.First(kvp => Equals(maxColumnCount, kvp.Value[0])).Key; - columnCount = maxColumnCount; - return delimiter; - } - } - - throw new LineColNumberedIoException(Resources.TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file, 1, 1); - } } /// From b8af30c4b8f2f48866f679e12dda576dbc0ddd3a Mon Sep 17 00:00:00 2001 From: brendanx67 Date: Tue, 4 Oct 2022 12:03:54 -0700 Subject: [PATCH 9/9] - move DetermineDsvDelimiter to AssertEx as the best location for now - add optional expectedSuccess parameter to RunCommand which reports a mismatch with the logged output - added more testing to PerfCommandlineCreateImsDbTest mostly as another example of inspecting the output SrmDocument from a command-line run --- .../Skyline/Test/MProphetScoringModelTest.cs | 2 +- pwiz_tools/Skyline/Test/UtilTest.cs | 2 +- .../PerfCommandlineCreateImsDbTest.cs | 65 ++++++++++-- .../Skyline/TestUtil/AbstractUnitTestEx.cs | 99 +++++++------------ pwiz_tools/Skyline/TestUtil/AssertEx.cs | 67 +++++++++++-- 5 files changed, 154 insertions(+), 81 deletions(-) diff --git a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs index 6c409da63e..87db58dc20 100644 --- a/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs +++ b/pwiz_tools/Skyline/Test/MProphetScoringModelTest.cs @@ -377,7 +377,7 @@ public Data(string dataFile) // Determine separator (comma, space, or tab). var headerTest = lines[0].Trim(); - var separator = DetermineDsvDelimiter(lines, out var columnCount); + var separator = AssertEx.DetermineDsvDelimiter(lines, out var columnCount); // Find header labels. If all headings are numeric, then no header. Header = headerTest.ParseDsvFields(separator); diff --git a/pwiz_tools/Skyline/Test/UtilTest.cs b/pwiz_tools/Skyline/Test/UtilTest.cs index acc17db310..1c461064d5 100644 --- a/pwiz_tools/Skyline/Test/UtilTest.cs +++ b/pwiz_tools/Skyline/Test/UtilTest.cs @@ -97,7 +97,7 @@ private static void TestDsvFields(char punctuation, char separator) TextUtil.LineSeparate("while parsing:", line, string.Empty, "expected:", TextUtil.LineSeparate(fields), string.Empty, "got:",TextUtil.LineSeparate(fieldsOut))); - var detectedSeparator = AbstractUnitTestEx.DetermineDsvDelimiter(new[] { line }, out var detectedColumnCount); + var detectedSeparator = AssertEx.DetermineDsvDelimiter(new[] { line }, out var detectedColumnCount); Assert.AreEqual(separator, detectedSeparator); Assert.AreEqual(fields.Length, detectedColumnCount); } diff --git a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs index b3f38027b4..d7ece48cfb 100644 --- a/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs +++ b/pwiz_tools/Skyline/TestPerf/PerfCommandlineCreateImsDbTest.cs @@ -19,8 +19,11 @@ using System.IO; +using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; +using pwiz.Common.Chemistry; using pwiz.Skyline; +using pwiz.Skyline.Model; using pwiz.Skyline.Properties; using pwiz.SkylineTestUtil; @@ -72,7 +75,7 @@ public void CommandlineCreateImsDbPerfTest() // Expect failure because of illegal characters in imsdb filename string imsdbPathBadName = GetImsDbFilePath("bad-name:?"); - string output = TestCreateImsdb(@"bad-name", imsdbPathBadName, true); + string output = TestCreateImsdb(@"bad-name", imsdbPathBadName, ExpectedResult.error); AssertEx.Contains(output, string.Format( Resources.ValueInvalidPathException_ValueInvalidPathException_The_value___0___is_not_valid_for_the_argument__1__failed_attempting_to_convert_it_to_a_full_file_path_, @@ -80,24 +83,34 @@ public void CommandlineCreateImsDbPerfTest() // Expect failure because of nonexistent subdirectories in path const string badPath = @"bad-path"; - output = TestCreateImsdb(badPath, Path.Combine(badPath, GetImsDbFileName(badPath)), true); + output = TestCreateImsdb(badPath, Path.Combine(badPath, GetImsDbFileName(badPath)), ExpectedResult.error); AssertEx.AreComparableStrings( Resources.CommandLine_SaveFile_Error__The_file_could_not_be_saved_to__0____Check_that_the_directory_exists_and_is_not_read_only_, output); // Expect failure because of missing "--ionmobility-library-create" - output = TestCreateImsdb(@"bad-args", null, true); + output = TestCreateImsdb(@"bad-args", null, ExpectedResult.warning); AssertEx.Contains(output, string.Format( Resources.CommandArgs_WarnArgRequirment_Warning__Use_of_the_argument__0__requires_the_argument__1_, CommandArgs.ARG_IMSDB_NAME.ArgumentText, CommandArgs.ARG_IMSDB_CREATE.ArgumentText)); } - private string TestCreateImsdb(string imsdbName, string imsdbPath, bool errorExpected = false) + private enum ExpectedResult { success, warning, error } + + private string TestCreateImsdb(string imsdbName, string imsdbPath, ExpectedResult expectedResult = ExpectedResult.success) { + // Clean-up after possible prior runs - also ensures they are not locked + string outputPath = GetTestPath("Scripps_IMS_DB.sky"); + File.Delete(outputPath); string reportFilePath = GetTestPath("Scripps_CCS_report.csv"); - var output = RunCommand(GetPathArg(CommandArgs.ARG_IN, "Scripps_IMS_Template.sky"), - GetPathArg(CommandArgs.ARG_OUT, "Scripps_IMS_DB.sky"), + File.Delete(reportFilePath); + if (!string.IsNullOrEmpty(imsdbPath) && File.Exists(imsdbPath)) + File.Delete(imsdbPath); + + var output = RunCommand(expectedResult != ExpectedResult.error, + GetPathArg(CommandArgs.ARG_IN, "Scripps_IMS_Template.sky"), + GetArg(CommandArgs.ARG_OUT, outputPath), GetPathArg(CommandArgs.ARG_IMPORT_TRANSITION_LIST, "test_run_1_transition_list.csv"), GetPathArg(CommandArgs.ARG_IMPORT_FILE, RAW_FILE), GetOptionalArg(CommandArgs.ARG_IMSDB_CREATE, imsdbPath), @@ -105,12 +118,50 @@ private string TestCreateImsdb(string imsdbName, string imsdbPath, bool errorExp GetArg(CommandArgs.ARG_REPORT_NAME, "Precursor CCS"), GetArg(CommandArgs.ARG_REPORT_FILE, reportFilePath)); - if (!errorExpected) + if (expectedResult == ExpectedResult.error) + { + // These files get created in the case of a warning, even if that + // may seem a bit undesirable. + Assert.IsFalse(File.Exists(outputPath)); + Assert.IsFalse(File.Exists(reportFilePath)); + } + else if (expectedResult == ExpectedResult.success) { AssertEx.FileExists(imsdbPath); // Compare to expected report - may need to localize the expected copy to match the actual copy AssertEx.AreEquivalentDsvFiles(GetTestPath("ImsDbTest_expected.csv"), reportFilePath, true); + + // Finally, check the persisted document to make sure it loads the IMS library + // information that was just added. + var doc = ResultsUtil.DeserializeDocument(outputPath); + + AssertEx.IsDocumentState(doc, 0, 1, 53, 53); + + using var docContainer = new ResultsTestDocumentContainer(null, outputPath, true); + docContainer.SetDocument(doc, null, true); + docContainer.AssertComplete(); + + doc = docContainer.Document; + + AssertResult.IsDocumentResultsState(doc, Path.GetFileNameWithoutExtension(RAW_FILE), 53, 53, 0, 53, 0); + + var imFiltering = doc.Settings.TransitionSettings.IonMobilityFiltering; + Assert.IsNotNull(imFiltering); + Assert.IsTrue(imFiltering.IonMobilityLibrary != null && !imFiltering.IonMobilityLibrary.IsNone); + + foreach (var ppp in doc.MoleculePrecursorPairs) + { + AssertEx.AreEqual(ExplicitTransitionGroupValues.EMPTY, ppp.NodeGroup.ExplicitValues, + "Expected no explicit values to be set, should all be in library"); + var libKey = ppp.NodeGroup.GetLibKey(doc.Settings, ppp.NodePep); + var libEntries = imFiltering.GetIonMobilityInfoFromLibrary(libKey); + Assert.IsNotNull(libEntries); + Assert.AreEqual(1, libEntries.Count); + var libInfo = libEntries.First(); + AssertEx.AreEqual(eIonMobilityUnits.inverse_K0_Vsec_per_cm2, libInfo.IonMobility.Units); + Assert.IsNotNull(libInfo.CollisionalCrossSectionSqA); + } } return output; diff --git a/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs b/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs index a8bb144fad..4ba46b414a 100644 --- a/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AbstractUnitTestEx.cs @@ -17,7 +17,6 @@ * limitations under the License. */ -using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -26,7 +25,6 @@ using pwiz.Skyline; using pwiz.Skyline.Model; using pwiz.Skyline.Model.Results; -using pwiz.Skyline.Properties; using pwiz.Skyline.Util.Extensions; @@ -39,24 +37,48 @@ namespace pwiz.SkylineTestUtil public class AbstractUnitTestEx : AbstractUnitTest { protected static string RunCommand(params string[] inputArgs) + { + return RunCommand(null, inputArgs); + } + + protected static string RunCommand(bool? expectSuccess, params string[] inputArgs) { var consoleBuffer = new StringBuilder(); - var consoleOutput = new CommandStatusWriter(new StringWriter(consoleBuffer)); - var exitStatus = CommandLineRunner.RunCommand(inputArgs, consoleOutput, true); + var consoleWriter = new CommandStatusWriter(new StringWriter(consoleBuffer)); + + var exitStatus = CommandLineRunner.RunCommand(inputArgs, consoleWriter, true); + + var consoleOutput = consoleBuffer.ToString(); + bool errorReported = consoleWriter.IsErrorReported; + + ValidateRunExitStatus(expectSuccess, exitStatus, errorReported, consoleOutput); + + return consoleOutput; + } - var fail = exitStatus == Program.EXIT_CODE_SUCCESS && consoleOutput.IsErrorReported || - exitStatus != Program.EXIT_CODE_SUCCESS && !consoleOutput.IsErrorReported; - if (fail) + private static void ValidateRunExitStatus(bool? expectSuccess, int exitStatus, bool errorReported, string consoleOutput) + { + string message = null; + // Make sure exist status and text error reporting match + if (exitStatus == Program.EXIT_CODE_SUCCESS && errorReported || + exitStatus != Program.EXIT_CODE_SUCCESS && !errorReported) + { + message = string.Format("{0} reported but exit status was {1}.", + errorReported ? "Error" : "No error", exitStatus); + } + else if (expectSuccess.HasValue) { - var message = - TextUtil.LineSeparate( - string.Format("{0} reported but exit status was {1}.", - consoleOutput.IsErrorReported ? "Error" : "No error", exitStatus), - "Output: ", consoleBuffer.ToString()); - Assert.Fail(message); + // Make sure expected exit status matches actual + if (expectSuccess.Value && exitStatus != Program.EXIT_CODE_SUCCESS) + message = string.Format("Expecting successful command-line execution but got {0} exit code.", exitStatus); + else if (!expectSuccess.Value && exitStatus == Program.EXIT_CODE_SUCCESS) + message = "Expecting command-line error but execution was successful."; } - return consoleBuffer.ToString(); + if (message != null) + { + Assert.Fail(TextUtil.LineSeparate(message, "Output: ", consoleOutput)); + } } public SrmDocument ConvertToSmallMolecules(SrmDocument doc, ref string docPath, IEnumerable dataPaths, @@ -131,54 +153,5 @@ protected static string RunCommand(params string[] inputArgs) AssertEx.ConvertedSmallMoleculeDocumentIsSimilar(docOriginal, doc, Path.GetDirectoryName(docPath), mode); return doc; } - - /// - /// Examine the lines of a DSV file an attempt to determine what kind of delimiter it uses - /// N.B. NOT ROBUST ENOUGH FOR GENERAL USE - would likely fail, for example, on data that has - /// irregular column counts. But still useful in the test context where we aren't handed random - /// data sets from users. - /// - /// lines of the file - /// return value: column count - /// the identified delimiter - /// thrown when we can't figure it out - public static char DetermineDsvDelimiter(string[] lines, out int columnCount) - { - - // If a candidate delimiter yields different column counts line to line, it's probably not the right one. - // So parse some distance in to see which delimiters give a consistent column count. - // NOTE we do see files like that in the wild, but not in our test suite - var countsPerLinePerCandidateDelimiter = new Dictionary> - { - { TextUtil.SEPARATOR_CSV, new List()}, - { TextUtil.SEPARATOR_SPACE, new List()}, - { TextUtil.SEPARATOR_TSV, new List()}, - { TextUtil.SEPARATOR_CSV_INTL, new List()} - }; - - for (var lineNum = 0; lineNum < Math.Min(100, lines.Length); lineNum++) - { - foreach (var sep in countsPerLinePerCandidateDelimiter.Keys) - { - countsPerLinePerCandidateDelimiter[sep].Add((new DsvFileReader(new StringReader(lines[lineNum]), sep)).NumberOfFields); - } - } - - var likelyCandidates = - countsPerLinePerCandidateDelimiter.Where(kvp => kvp.Value.Distinct().Count() == 1).ToArray(); - if (likelyCandidates.Length > 0) - { - // The candidate that yields the highest column count wins - var maxColumnCount = likelyCandidates.Max(kvp => kvp.Value[0]); - if (likelyCandidates.Count(kvp => Equals(maxColumnCount, kvp.Value[0])) == 1) - { - var delimiter = likelyCandidates.First(kvp => Equals(maxColumnCount, kvp.Value[0])).Key; - columnCount = maxColumnCount; - return delimiter; - } - } - - throw new LineColNumberedIoException(Resources.TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file, 1, 1); - } } } diff --git a/pwiz_tools/Skyline/TestUtil/AssertEx.cs b/pwiz_tools/Skyline/TestUtil/AssertEx.cs index 116b797096..3480324afd 100644 --- a/pwiz_tools/Skyline/TestUtil/AssertEx.cs +++ b/pwiz_tools/Skyline/TestUtil/AssertEx.cs @@ -848,8 +848,8 @@ public static void NoDiff(string target, string actual, string helpMsg=null, Dic var matchExpected = regexGUID.Match(lineExpected); var matchActual = regexGUID.Match(lineActual); if (matchExpected.Success && matchActual.Success - && Equals(matchExpected.Groups[1].ToString(), matchActual.Groups[1].ToString()) - && Equals(matchExpected.Groups[2].ToString(), matchActual.Groups[2].ToString())) + && Equals(matchExpected.Groups[1].ToString(), matchActual.Groups[1].ToString()) + && Equals(matchExpected.Groups[2].ToString(), matchActual.Groups[2].ToString())) { return true; } @@ -861,8 +861,8 @@ public static void NoDiff(string target, string actual, string helpMsg=null, Dic matchExpected = regexTimestamp.Match(lineExpected); matchActual = regexTimestamp.Match(lineActual); if (matchExpected.Success && matchActual.Success - && Equals(matchExpected.Groups[1].ToString(), matchActual.Groups[1].ToString()) - && Equals(matchExpected.Groups[2].ToString(), matchActual.Groups[2].ToString())) + && Equals(matchExpected.Groups[1].ToString(), matchActual.Groups[1].ToString()) + && Equals(matchExpected.Groups[2].ToString(), matchActual.Groups[2].ToString())) { return true; } @@ -921,19 +921,19 @@ public static void AreEquivalentDsvFiles(string path1, string path2, bool hasHea { var lines1 = File.ReadAllLines(path1); var lines2 = File.ReadAllLines(path2); - AssertEx.AreEqual(lines1.Length, lines2.Length, "Expected same line count"); + AreEqual(lines1.Length, lines2.Length, "Expected same line count"); if (lines1.Length == 0) { return; } - var sep1 = AbstractUnitTestEx.DetermineDsvDelimiter(lines1, out var colCount1); - var sep2 = AbstractUnitTestEx.DetermineDsvDelimiter(lines2, out var colCount2); + var sep1 = DetermineDsvDelimiter(lines1, out var colCount1); + var sep2 = DetermineDsvDelimiter(lines2, out var colCount2); for (var lineNum = 0; lineNum < lines1.Length; lineNum++) { var cols1 = lines1[lineNum].ParseDsvFields(sep1); var cols2 = lines2[lineNum].ParseDsvFields(sep2); - AssertEx.AreEqual(cols1.Length, cols2.Length, $"Expected same column count at line {lineNum}"); + AreEqual(cols1.Length, cols2.Length, $"Expected same column count at line {lineNum}"); if (hasHeaders && Equals(lineNum, 0) && !Equals(CultureInfo.CurrentCulture.TwoLetterISOLanguageName, @"en")) { continue; // Don't expect localized headers to match @@ -955,12 +955,61 @@ string Dotted(string val) if (!same) { - AssertEx.AreEqual(cols1[colNum], cols2[colNum], $"Difference at row {lineNum} column {colNum}"); + AreEqual(cols1[colNum], cols2[colNum], $"Difference at row {lineNum} column {colNum}"); } } } } + /// + /// Examine the lines of a DSV file an attempt to determine what kind of delimiter it uses + /// N.B. NOT ROBUST ENOUGH FOR GENERAL USE - would likely fail, for example, on data that has + /// irregular column counts. But still useful in the test context where we aren't handed random + /// data sets from users. + /// + /// lines of the file + /// return value: column count + /// the identified delimiter + /// thrown when we can't figure it out + public static char DetermineDsvDelimiter(string[] lines, out int columnCount) + { + + // If a candidate delimiter yields different column counts line to line, it's probably not the right one. + // So parse some distance in to see which delimiters give a consistent column count. + // NOTE we do see files like that in the wild, but not in our test suite + var countsPerLinePerCandidateDelimiter = new Dictionary> + { + { TextUtil.SEPARATOR_CSV, new List()}, + { TextUtil.SEPARATOR_SPACE, new List()}, + { TextUtil.SEPARATOR_TSV, new List()}, + { TextUtil.SEPARATOR_CSV_INTL, new List()} + }; + + for (var lineNum = 0; lineNum < Math.Min(100, lines.Length); lineNum++) + { + foreach (var sep in countsPerLinePerCandidateDelimiter.Keys) + { + countsPerLinePerCandidateDelimiter[sep].Add((new DsvFileReader(new StringReader(lines[lineNum]), sep)).NumberOfFields); + } + } + + var likelyCandidates = + countsPerLinePerCandidateDelimiter.Where(kvp => kvp.Value.Distinct().Count() == 1).ToArray(); + if (likelyCandidates.Length > 0) + { + // The candidate that yields the highest column count wins + var maxColumnCount = likelyCandidates.Max(kvp => kvp.Value[0]); + if (likelyCandidates.Count(kvp => Equals(maxColumnCount, kvp.Value[0])) == 1) + { + var delimiter = likelyCandidates.First(kvp => Equals(maxColumnCount, kvp.Value[0])).Key; + columnCount = maxColumnCount; + return delimiter; + } + } + + throw new LineColNumberedIoException(Resources.TextUtil_DeterminDsvSeparator_Unable_to_determine_format_of_delimiter_separated_value_file, 1, 1); + } + public static void FieldsEqual(string target, string actual, int countFields, bool allowForNumericPrecisionDifferences = false) { FieldsEqual(target, actual, countFields, null, allowForNumericPrecisionDifferences);