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.

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

Fixed TRX file overwrite in certain circumstances #2508

Merged
16 commits merged into from
Aug 7, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Modified the internal file helper use a DateTime provider func for te…
…sting purposes.
  • Loading branch information
Haplois committed Aug 7, 2020
commit 09dfc1629d009c6ca87ab1e7434da32d0f267e6e
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ internal class TestResult : ITestResult, IXmlTestStore
/// Directory containing the test result files, relative to the root test results directory
/// </summary>
private string relativeTestResultsDirectory;
private readonly InternalFileHelper internalFileHelper;

/// <summary>
/// Paths to test result files, relative to the test results folder, sorted in increasing order
Expand Down Expand Up @@ -270,7 +271,8 @@ public TestResult(
string computerName,
TestOutcome outcome,
TestType testType,
TestListCategoryId testCategoryId)
TestListCategoryId testCategoryId,
InternalFileHelper internalFileHelper)
{
Debug.Assert(computerName != null, "computername is null");
Debug.Assert(!Guid.Empty.Equals(executionId), "ExecutionId is empty");
Expand All @@ -285,6 +287,7 @@ public TestResult(
this.outcome = outcome;
this.categoryId = testCategoryId;
this.relativeTestResultsDirectory = TestRunDirectories.GetRelativeTestResultsDirectory(executionId);
this.internalFileHelper = internalFileHelper;
}

#endregion
Expand Down Expand Up @@ -532,7 +535,7 @@ internal void AddResultFiles(IEnumerable<string> resultFileList)
Debug.Assert(!string.IsNullOrEmpty(resultFile), "'resultFile' is null or empty");
Debug.Assert(resultFile.Trim() == resultFile, "'resultFile' has whitespace at the ends");

this.resultFiles[FileHelper.MakePathRelative(resultFile, testResultsDirectory)] = null;
this.resultFiles[internalFileHelper.MakePathRelative(resultFile, testResultsDirectory)] = null;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.TestPlatform.Extensions.TrxLogger.ObjectModel
{
using System;
using System.Collections.Generic;
using Microsoft.TestPlatform.Extensions.TrxLogger.Utility;
using Microsoft.TestPlatform.Extensions.TrxLogger.XML;

/// <summary>
Expand All @@ -23,7 +24,8 @@ public TestResultAggregation(
string computerName,
TestOutcome outcome,
TestType testType,
TestListCategoryId testCategoryId) : base(runId, testId, executionId, parentExecutionId, resultName, computerName, outcome, testType, testCategoryId) { }
TestListCategoryId testCategoryId,
InternalFileHelper internalFileHelper) : base(runId, testId, executionId, parentExecutionId, resultName, computerName, outcome, testType, testCategoryId, internalFileHelper) { }

/// <summary>
/// Gets the inner results.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ internal class TestRunConfiguration : IXmlTestStore, IXmlTestStoreCustom

#region Fields
private TestRunConfigurationId id;
private readonly InternalFileHelper internalFileHelper;

[StoreXmlSimpleField(DefaultValue = "")]
private string name;
Expand All @@ -34,14 +35,17 @@ internal class TestRunConfiguration : IXmlTestStore, IXmlTestStoreCustom
/// <param name="name">
/// The name of Run Configuration.
/// </param>
public TestRunConfiguration(string name)
/// <param name="internalFileHelper">
/// InternalFileHelper instance to use in file operations.
/// </param>
internal TestRunConfiguration(string name, InternalFileHelper internalFileHelper)
{
EqtAssert.ParameterNotNull(name, "name");

this.name = name;

this.runDeploymentRoot = string.Empty;
this.id = new TestRunConfigurationId();
this.internalFileHelper = internalFileHelper;
}

#region IXmlTestStoreCustom Members
Expand Down Expand Up @@ -141,7 +145,7 @@ public void Save(XmlElement element, XmlTestStoreParameters parameters)
helper.SaveSimpleField(
element,
"Deployment/@runDeploymentRoot",
FileHelper.MakePathRelative(this.runDeploymentRoot, Path.GetDirectoryName(this.runDeploymentRoot)),
internalFileHelper.MakePathRelative(this.runDeploymentRoot, Path.GetDirectoryName(this.runDeploymentRoot)),
string.Empty);
}
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
namespace Microsoft.TestPlatform.Extensions.TrxLogger.ObjectModel
{
using System;
using Microsoft.TestPlatform.Extensions.TrxLogger.Utility;

/// <summary>
/// Class for unit test result.
Expand All @@ -19,6 +20,8 @@ public UnitTestResult(
string computerName,
TestOutcome outcome,
TestType testType,
TestListCategoryId testCategoryId) : base(runId, testId, executionId, parentExecutionId, resultName, computerName, outcome, testType, testCategoryId) { }
TestListCategoryId testCategoryId,
InternalFileHelper internalFileHelper
) : base(runId, testId, executionId, parentExecutionId, resultName, computerName, outcome, testType, testCategoryId, internalFileHelper) { }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Microsoft.TestPlatform.Extensions.TrxLogger.ObjectModel
/// </summary>
internal class UriDataAttachment : IDataAttachment, IXmlTestStore
{
private readonly InternalFileHelper internalFileHelper;
#region Private fields

/// <summary>
Expand All @@ -36,11 +37,14 @@ internal class UriDataAttachment : IDataAttachment, IXmlTestStore
/// </summary>
/// <param name="description">Short description for the attachment</param>
/// <param name="uri">The URI pointing to the resource</param>
/// <param name="internalFileHelper">InternalFileHelper class instance to use in file operations.</param>
/// <exception cref="ArgumentException">'name' is null or empty</exception>
/// <exception cref="ArgumentNullException">'uri' is null</exception>
public UriDataAttachment(string description, Uri uri)
public UriDataAttachment(string description, Uri uri, InternalFileHelper internalFileHelper)
{
this.Initialize(description, uri);
this.internalFileHelper = internalFileHelper;

Initialize(description, uri);
}

#region IDataAttachment Members
Expand Down Expand Up @@ -121,10 +125,10 @@ internal UriDataAttachment Clone(string baseDirectory, bool useAbsoluteUri)
}
else
{
uriToUse = new Uri(FileHelper.MakePathRelative(this.uri.OriginalString, baseDirectory), UriKind.Relative);
uriToUse = new Uri(internalFileHelper.MakePathRelative(this.uri.OriginalString, baseDirectory), UriKind.Relative);
}

return new UriDataAttachment(this.description, uriToUse);
return new UriDataAttachment(this.description, uriToUse, internalFileHelper);
}

// The URI in this instance is already how we want it, and since this class is immutable, no need to clone
Expand Down
26 changes: 14 additions & 12 deletions src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,30 @@ namespace Microsoft.VisualStudio.TestPlatform.Extensions.TrxLogger
[ExtensionUri(TrxLoggerConstants.ExtensionUri)]
public class TrxLogger : ITestLoggerWithParameters
{
#region Fields

#region Constructor

/// <summary>
/// Initializes a new instance of the <see cref="TrxLogger"/> class.
/// </summary>
public TrxLogger() :
this(new Utilities.Helpers.FileHelper())
{
}
public TrxLogger() : this(new Utilities.Helpers.FileHelper()) { }

/// <summary>
/// Initializes a new instance of the <see cref="TrxLogger"/> class.
/// Constructor with Dependency injection. Used for unit testing.
/// </summary>
/// <param name="fileHelper">The file helper interface.</param>
protected TrxLogger(IFileHelper fileHelper)
protected TrxLogger(IFileHelper fileHelper) : this(new Utilities.Helpers.FileHelper(), new InternalFileHelper()) { }

internal TrxLogger(IFileHelper fileHelper, InternalFileHelper internalFileHelper)
Haplois marked this conversation as resolved.
Show resolved Hide resolved
{
this.converter = new Converter(fileHelper);
this.converter = new Converter(fileHelper, internalFileHelper);
this.internalFileHelper = internalFileHelper;
}

#endregion

#region Fields

/// <summary>
/// Cache the TRX file path
/// </summary>
Expand All @@ -74,6 +74,8 @@ protected TrxLogger(IFileHelper fileHelper)
private ConcurrentDictionary<Guid, TrxLoggerObjectModel.ITestResult> innerResults;
private ConcurrentDictionary<Guid, TestEntry> innerTestEntries;

private readonly InternalFileHelper internalFileHelper;

/// <summary>
/// Specifies the run level "out" messages
/// </summary>
Expand Down Expand Up @@ -511,7 +513,7 @@ private string AcquireTrxFileNamePath(out bool shouldOverwrite)
logFilePrefixValue = logFilePrefixValue + "_" + framework;
}

filePath = FileHelper.GetNextTimestampFileName(this.testResultsDirPath, logFilePrefixValue + this.trxFileExtension, "_yyyyMMddHHmmss");
filePath = internalFileHelper.GetNextTimestampFileName(this.testResultsDirPath, logFilePrefixValue + this.trxFileExtension, "_yyyyMMddHHmmss");
}

else if (isLogFileNameParameterExists)
Expand All @@ -538,7 +540,7 @@ private string SetDefaultTrxFilePath()
{
var defaultTrxFileName = this.testRun.RunConfiguration.RunDeploymentRootDirectory + ".trx";

return FileHelper.GetNextIterationFileName(this.testResultsDirPath, defaultTrxFileName, false);
return internalFileHelper.GetNextIterationFileName(this.testResultsDirPath, defaultTrxFileName, false);
}

/// <summary>
Expand All @@ -560,8 +562,8 @@ private void CreateTestRun()
this.testRun.Started = this.testRunStartTime;

// Save default test settings
string runDeploymentRoot = Microsoft.TestPlatform.Extensions.TrxLogger.Utility.FileHelper.ReplaceInvalidFileNameChars(this.testRun.Name);
TestRunConfiguration testrunConfig = new TestRunConfiguration("default");
string runDeploymentRoot = internalFileHelper.ReplaceInvalidFileNameChars(this.testRun.Name);
TestRunConfiguration testrunConfig = new TestRunConfiguration("default", internalFileHelper);
testrunConfig.RunDeploymentRootDirectory = runDeploymentRoot;
this.testRun.RunConfiguration = testrunConfig;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ namespace Microsoft.TestPlatform.Extensions.TrxLogger.Utility
/// </summary>
internal class Converter
{
private readonly InternalFileHelper internalFileHelper;
private IFileHelper fileHelper;

/// <summary>
/// Initializes a new instance of the <see cref="Converter"/> class.
/// </summary>
public Converter(IFileHelper fileHelper)
public Converter(IFileHelper fileHelper, InternalFileHelper internalFileHelper)
{
this.internalFileHelper = internalFileHelper;
this.fileHelper = fileHelper;
}

Expand Down Expand Up @@ -68,6 +70,8 @@ public ITestElement ToTestElement(
return testElement;
}



/// <summary>
/// Converts the rockSteady result to unit test result
/// </summary>
Expand Down Expand Up @@ -473,7 +477,7 @@ private CollectorDataEntry ToCollectorEntry(ObjectModel.AttachmentSet attachment
Debug.Assert(Path.IsPathRooted(sourceFile), "Source file is not rooted");

// copy the source file to the target location
string targetFileName = FileHelper.GetNextIterationFileName(targetDirectory, Path.GetFileName(sourceFile), false);
string targetFileName = internalFileHelper.GetNextIterationFileName(targetDirectory, Path.GetFileName(sourceFile), false);

try
{
Expand All @@ -483,7 +487,7 @@ private CollectorDataEntry ToCollectorEntry(ObjectModel.AttachmentSet attachment
// (Trx viewer automatically adds In\ to the collected file.
string fileName = Path.Combine(Environment.MachineName, Path.GetFileName(targetFileName));
Uri sourceFileUri = new Uri(fileName, UriKind.Relative);
TrxObjectModel.UriDataAttachment dataAttachment = new TrxObjectModel.UriDataAttachment(uriDataAttachment.Description, sourceFileUri);
TrxObjectModel.UriDataAttachment dataAttachment = new TrxObjectModel.UriDataAttachment(uriDataAttachment.Description, sourceFileUri, internalFileHelper);

uriDataAttachments.Add(dataAttachment);
}
Expand Down Expand Up @@ -535,7 +539,7 @@ private IList<string> ToResultFiles(ObjectModel.AttachmentSet attachmentSet, Gui

Debug.Assert(Path.IsPathRooted(sourceFile), "Source file is not rooted");
// copy the source file to the target location
string targetFileName = FileHelper.GetNextIterationFileName(testResultDirectory, Path.GetFileName(sourceFile), false);
string targetFileName = internalFileHelper.GetNextIterationFileName(testResultDirectory, Path.GetFileName(sourceFile), false);

try
{
Expand Down Expand Up @@ -719,8 +723,8 @@ private TrxObjectModel.TestResult CreateTestResult(
TestListCategoryId testCategoryId)
{
return testType.Equals(Constants.OrderedTestType) ?
new TestResultAggregation(runId, testId, executionId, parentExecutionId, resultName, Environment.MachineName, outcome, testType, testCategoryId) :
new UnitTestResult(runId, testId, executionId, parentExecutionId, resultName, Environment.MachineName, outcome, testType, testCategoryId);
new TestResultAggregation(runId, testId, executionId, parentExecutionId, resultName, Environment.MachineName, outcome, testType, testCategoryId, internalFileHelper) :
new UnitTestResult(runId, testId, executionId, parentExecutionId, resultName, Environment.MachineName, outcome, testType, testCategoryId, internalFileHelper);
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,21 @@ namespace Microsoft.TestPlatform.Extensions.TrxLogger.Utility
/// <summary>
/// Helper function to deal with file name.
/// </summary>
internal static class FileHelper
internal class InternalFileHelper

{
private const string RelativeDirectorySeparator = "..";

private static readonly Dictionary<char, object> InvalidFileNameChars;
private static readonly Dictionary<char, object> AdditionalInvalidFileNameChars;
private static readonly Regex ReservedFileNamesRegex = new Regex(@"(?i:^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9]|CLOCK\$)(\..*)?)$");
private readonly Func<DateTime> TimeProvider;

#region Constructors
[SuppressMessage("Microsoft.Performance", "CA1810:InitializeReferenceTypeStaticFieldsInline", Justification = "Reviewed. Suppression is OK here.")]

// Have to init InvalidFileNameChars dynamically.
static FileHelper()
static InternalFileHelper()
{
// Create a hash table of invalid chars. On Windows, this should match the contents of System.IO.Path.GetInvalidFileNameChars.
// See https://github.com/dotnet/coreclr/blob/8e99cd8031b2f568ea69116e7cf96d55e32cb7f5/src/mscorlib/shared/System/IO/Path.Windows.cs#L12-L19
Expand Down Expand Up @@ -62,14 +64,21 @@ static FileHelper()
AdditionalInvalidFileNameChars.Add(' ', null);
}

public InternalFileHelper() : this(() => DateTime.Now) { }

public InternalFileHelper(Func<DateTime> timeProvider)
{
TimeProvider = timeProvider ?? (() => DateTime.Now);
}

#endregion

/// <summary>
/// Replaces invalid file name chars in the specified string and changes it if it is a reserved file name.
/// </summary>
/// <param name="fileName">the name of the file</param>
/// <returns>Replaced string.</returns>
public static string ReplaceInvalidFileNameChars(string fileName)
public string ReplaceInvalidFileNameChars(string fileName)
{
EqtAssert.StringNotNullOrEmpty(fileName, "fileName");

Expand Down Expand Up @@ -125,7 +134,7 @@ public static string ReplaceInvalidFileNameChars(string fileName)
/// <returns>
/// The <see cref="string"/>.
/// </returns>
public static string GetNextIterationFileName(string parentDirectoryName, string originalFileName, bool checkMatchingDirectory)
public string GetNextIterationFileName(string parentDirectoryName, string originalFileName, bool checkMatchingDirectory)
{
EqtAssert.StringNotNullOrEmpty(parentDirectoryName, "parentDirectoryName");
EqtAssert.StringNotNullOrEmpty(originalFileName, "originalFileName");
Expand All @@ -143,14 +152,14 @@ public static string GetNextIterationFileName(string parentDirectoryName, string
/// <example>
/// <code>GetNextTimestampFileName("c:\data", "log.txt", "_yyyyMMddHHmmss")</code> will return "c:\data\log_20200801185521.txt", if available.
/// </example>
public static string GetNextTimestampFileName(string directoryName, string fileName, string timestampFormat)
public string GetNextTimestampFileName(string directoryName, string fileName, string timestampFormat)
{
EqtAssert.StringNotNullOrEmpty(directoryName, "parentDirectoryName");
EqtAssert.StringNotNullOrEmpty(fileName, "fileName");
EqtAssert.StringNotNullOrEmpty(timestampFormat, "timestampFormat");

ushort iteration = 0;
var iterationStamp = DateTime.Now;
var iterationStamp = TimeProvider();
var fileNamePrefix = Path.GetFileNameWithoutExtension(fileName);
var extension = Path.GetExtension(fileName);
do
Expand All @@ -171,7 +180,7 @@ public static string GetNextTimestampFileName(string directoryName, string fileN
throw new Exception(string.Format(CultureInfo.CurrentCulture, TrxLoggerResources.Common_CannotGetNextTimestampFileName, fileName, directoryName, timestampFormat));
}

public static string MakePathRelative(string path, string basePath)
public string MakePathRelative(string path, string basePath)
{
EqtAssert.StringNotNullOrEmpty(path, "path");

Expand Down
Loading