diff --git a/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/PublicAPI/PublicAPI.Unshipped.txt b/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/PublicAPI/PublicAPI.Unshipped.txt index 7dc5c58110..da44d485bd 100644 --- a/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/PublicAPI/PublicAPI.Unshipped.txt @@ -1 +1,5 @@ #nullable enable +Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxTestDefinitionName +Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxTestDefinitionName.TestDefinitionName.get -> string! +Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxTestDefinitionName.TrxTestDefinitionName(string! testDefinitionName) -> void +override Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxTestDefinitionName.ToString() -> string! diff --git a/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs b/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs index 09b937a62c..58d45e4283 100644 --- a/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs +++ b/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs @@ -235,3 +235,35 @@ public override string ToString() return builder.ToString(); } } + +/// +/// A property that represents the value of name attribute on UnitTest XML elements under TestDefinitions XML element. +/// When the property is missing, TestNode.DisplayName is used instead. +/// This can cause issues when multiple test results are reported where different test results have different display names. +/// +public sealed class TrxTestDefinitionName : IProperty +{ + /// + /// Initializes a new instance of the class. + /// + /// The name to use. + public TrxTestDefinitionName(string testDefinitionName) + => TestDefinitionName = testDefinitionName; + + /// + /// Gets the name to use. + /// + public string TestDefinitionName { get; } + + /// + public override string ToString() + { + var builder = new StringBuilder(); + builder.Append(nameof(TrxTestDefinitionName)); + builder.Append(" { "); + builder.Append($"{nameof(TestDefinitionName)} = "); + builder.Append(TestDefinitionName); + builder.Append(" }"); + return builder.ToString(); + } +} diff --git a/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs b/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs index 5ea30af31c..d32cb388db 100644 --- a/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs +++ b/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs @@ -397,7 +397,10 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, var results = new XElement("Results"); // Duplicate test ids are not allowed inside the TestDefinitions element. - var uniqueTestDefinitionTestIds = new HashSet(); + // We create a dictionary to map test id to test definition name. + // It's not expected to get the same test id twice but with different test definition name. + // However, due to backcompat concerns, we will disallow this only for frameworks that start using TrxTestDefinitionName property. + var uniqueTestDefinitionTestIds = new Dictionary(); foreach (TestNodeUpdateMessage nodeMessage in testNodeUpdateMessages) { @@ -412,7 +415,11 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, // NOTE: In VSTest, MSTestDiscoverer.TmiTestId property is preferred if present. string id = guid.ToString(); - string displayName = RemoveInvalidXmlChar(testNode.DisplayName)!; + string testResultDisplayName = RemoveInvalidXmlChar(testNode.DisplayName)!; + (string testDefinitionName, bool isExplicitlyProvided) = testNode.Properties.SingleOrDefault() is { } trxTestDefinitionName + ? (RemoveInvalidXmlChar(trxTestDefinitionName.TestDefinitionName), true) + : (testResultDisplayName, false); + string executionId = Guid.NewGuid().ToString(); // Results @@ -420,7 +427,7 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, "UnitTestResult", new XAttribute("executionId", executionId), new XAttribute("testId", id), - new XAttribute("testName", displayName), + new XAttribute("testName", testResultDisplayName), new XAttribute("computerName", _environment.MachineName)); TimingProperty? timing = testNode.Properties.SingleOrDefault(); @@ -561,9 +568,31 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, // TestDefinitions // Add the test method to the test definitions if it's not already there - if (uniqueTestDefinitionTestIds.Add(id)) + if (uniqueTestDefinitionTestIds.TryGetValue(id, out (string ExistingTestDefinitionName, bool ExistingIsExplicitlyProvided) existing)) + { + // Value already exists. We only do a validation. + // Owner, Description, Priority, and TestCategory are also part of the test definition. + // Unfortunately, MSTest allows TestCategories via TestDataRow, which is one case where + // we might receive the same test id and same test definition name, but different categories. + // It's probably best if TRX is able to "merge" categories in this case (which we don't do yet). + // For Owner, Description, Priority, this needs investigation whether or not it's expected to be different, + // and what should we do in this case. + if ((isExplicitlyProvided || existing.ExistingIsExplicitlyProvided) && + existing.ExistingTestDefinitionName != testDefinitionName) + { + throw new InvalidOperationException($"Received two different test definition names ('{existing.ExistingTestDefinitionName}' and '{testDefinitionName}') for the same test id '{id}'."); + } + + if (!existing.ExistingIsExplicitlyProvided && isExplicitlyProvided) + { + // We got a first result that didn't have explicit test definition name, but a second result that has an explicit test definition name. + uniqueTestDefinitionTestIds[id] = (testDefinitionName, true); + } + } + else { - XElement unitTest = CreateUnitTestElementForTestDefinition(displayName, testAppModule, id, testNode, executionId); + uniqueTestDefinitionTestIds.Add(id, (testDefinitionName, isExplicitlyProvided)); + XElement unitTest = CreateUnitTestElementForTestDefinition(testDefinitionName, testAppModule, id, testNode, executionId); var testMethod = new XElement( "TestMethod", @@ -574,13 +603,13 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, (string className, string? testMethodName) = GetClassAndMethodName(testNode); testMethod.SetAttributeValue("className", className); - // NOTE: Historically, MTP used to always use displayName here. - // While VSTest never uses displayName. - // The use of displayName here is very wrong. + // NOTE: Historically, MTP used to always use testResultDisplayName here. + // While VSTest never uses testResultDisplayName. + // The use of testResultDisplayName here is very wrong. // We keep it as a fallback if we cannot determine the testMethodName (when TestMethodIdentifierProperty isn't present). // This will most likely be hit for NUnit. // However, this is very wrong and we probably should fail if TestMethodIdentifierProperty isn't present. - testMethod.SetAttributeValue("name", testMethodName ?? displayName); + testMethod.SetAttributeValue("name", testMethodName ?? testResultDisplayName); unitTest.Add(testMethod); @@ -626,11 +655,11 @@ private SummaryCounts AddResults(TestNodeUpdateMessage[] testNodeUpdateMessages, return (classNameFromIdentifierProperty, testMethodIdentifierProperty.MethodName); } - private static XElement CreateUnitTestElementForTestDefinition(string displayName, string testAppModule, string id, TestNode testNode, string executionId) + private static XElement CreateUnitTestElementForTestDefinition(string testDefinitionName, string testAppModule, string id, TestNode testNode, string executionId) { var unitTest = new XElement( "UnitTest", - new XAttribute("name", displayName), + new XAttribute("name", testDefinitionName), new XAttribute("storage", testAppModule.ToLowerInvariant()), new XAttribute("id", id)); @@ -804,7 +833,7 @@ private static Guid GuidFromString(string data) private static Regex BuildInvalidXmlCharReplace() => new(@"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD]"); #endif - private static string? RemoveInvalidXmlChar(string? str) => str is null ? null : InvalidXmlCharReplace.Replace(str, InvalidXmlEvaluator); + private static string RemoveInvalidXmlChar(string str) => InvalidXmlCharReplace.Replace(str, InvalidXmlEvaluator); private static string ReplaceInvalidCharacterWithUniCodeEscapeSequence(Match match) { diff --git a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs index 8ddc5ca5a7..756d18a989 100644 --- a/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs +++ b/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs @@ -145,6 +145,8 @@ public static TestNode ToTestNode( testNode.Properties.Add(new TrxExceptionProperty(testResult.ErrorMessage, testResult.ErrorStackTrace)); } + testNode.Properties.Add(new TrxTestDefinitionName(testResult.TestCase.DisplayName ?? testResult.TestCase.FullyQualifiedName)); + // TODO: Consider retrieving TestMethodIdentifierProperty first (which could have been added through addAdditionalProperties. // VSTest's TestCase.FQN is very non-standard. // We should avoid using it if we can. diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs index fcbae14b28..34729b1a8c 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs @@ -597,6 +597,72 @@ public async Task TrxReportEngine_GenerateReportAsync_WithMetadataProperties_Trx Assert.IsTrue(Regex.IsMatch(trxContent, trxContentsPattern), trxContent); } + [TestMethod] + public async Task TrxReportEngine_GenerateReportAsync_WithTrxTestDefinitionName_UnitTestNameIsFromTrxTestDefinitionName() + { + // Arrange + using MemoryFileStream memoryStream = new(); + var propertyBag = new PropertyBag( + new PassedTestNodeStateProperty(), + new TrxTestDefinitionName("ExplicitTestDefinitionName")); + TrxReportEngine trxReportEngine = GenerateTrxReportEngine(memoryStream); + + // Act + (string fileName, string? warning) = await trxReportEngine.GenerateReportAsync([CreateTestNodeUpdate("test()", "TestResultDisplayName", propertyBag)]); + + // Assert + Assert.IsNull(warning); + AssertExpectedTrxFileName(fileName); + Assert.IsNotNull(memoryStream.TrxContent); + XDocument xml = memoryStream.TrxContent; + AssertTrxOutcome(xml, "Completed"); + string trxContent = xml.ToString(); + // UnitTest/@name should use TrxTestDefinitionName, not the display name + Assert.Contains(@"(() => trxReportEngine.GenerateReportAsync(messages)); + } + + [TestMethod] + public async Task TrxReportEngine_GenerateReportAsync_WithFallbackNameFirstThenMatchingExplicitTestDefinitionName_Succeeds() + { + // Arrange + using MemoryFileStream memoryStream = new(); + + // First result has no TrxTestDefinitionName, so it falls back to the display name "MethodName". + // Second result provides an explicit TrxTestDefinitionName "MethodName" that matches the fallback. + TestNodeUpdateMessage[] messages = [ + CreateTestNodeUpdate("same-uid", "MethodName", new PropertyBag(new PassedTestNodeStateProperty())), + CreateTestNodeUpdate("same-uid", "MethodName", new PropertyBag(new PassedTestNodeStateProperty(), new TrxTestDefinitionName("MethodName"))), + ]; + TrxReportEngine trxReportEngine = GenerateTrxReportEngine(memoryStream); + + // Act + (string fileName, string? warning) = await trxReportEngine.GenerateReportAsync(messages); + + // Assert: no exception, UnitTest/@name is "MethodName" + Assert.IsNull(warning); + Assert.IsNotNull(memoryStream.TrxContent); + XDocument xml = memoryStream.TrxContent; + string trxContent = xml.ToString(); + Assert.Contains(@"