Fix test definition names in TRX to correspond to test case display names and not test result display names

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!

src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs

@@ -235,3 +235,35 @@ public override string ToString()
         return builder.ToString();
     }
 }
+
+/// <summary>
+/// A property that represents the value of <c>name</c> attribute on <c>UnitTest</c> XML elements under <c>TestDefinitions</c> 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.
+/// </summary>
+public sealed class TrxTestDefinitionName : IProperty
+{
+    /// <summary>
+    /// Initializes a new instance of the <see cref="TrxTestDefinitionName"/> class.
+    /// </summary>
+    /// <param name="testDefinitionName">The name to use.</param>
+    public TrxTestDefinitionName(string testDefinitionName)
+        => TestDefinitionName = testDefinitionName;
+
+    /// <summary>
+    /// Gets the name to use.
+    /// </summary>
+    public string TestDefinitionName { get; }
+
+    /// <inheritdoc />
+    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();
+    }
+}

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<string>();
+        // 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<string, (string TestDefinitionName, bool IsExplicitlyProvided)>();
 
         foreach (TestNodeUpdateMessage nodeMessage in testNodeUpdateMessages)
         {
@@ -412,15 +415,19 @@ 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<TrxTestDefinitionName>() is { } trxTestDefinitionName
+                ? (RemoveInvalidXmlChar(trxTestDefinitionName.TestDefinitionName), true)
+                : (testResultDisplayName, false);
+
             string executionId = Guid.NewGuid().ToString();
 
             // Results
             var unitTestResult = new XElement(
                 "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<TimingProperty>();
@@ -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)
     {

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.

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(@"<UnitTest name=""ExplicitTestDefinitionName""", trxContent, trxContent);
+        // UnitTestResult/@testName should use the display name
+        Assert.Contains(@"testName=""TestResultDisplayName""", trxContent, trxContent);
+    }
+
+    [TestMethod]
+    public async Task TrxReportEngine_GenerateReportAsync_WithDuplicateTestIdAndDifferentExplicitTestDefinitionNames_ThrowsInvalidOperationException()
+    {
+        // Arrange
+        using MemoryFileStream memoryStream = new();
+        TestNodeUpdateMessage[] messages = [
+            CreateTestNodeUpdate("same-uid", "DisplayName1", new PropertyBag(new PassedTestNodeStateProperty(), new TrxTestDefinitionName("ExplicitName1"))),
+            CreateTestNodeUpdate("same-uid", "DisplayName2", new PropertyBag(new PassedTestNodeStateProperty(), new TrxTestDefinitionName("ExplicitName2"))),
+        ];
+        TrxReportEngine trxReportEngine = GenerateTrxReportEngine(memoryStream);
+
+        // Act & Assert
+        await Assert.ThrowsExactlyAsync<InvalidOperationException>(() => 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(@"<UnitTest name=""MethodName""", trxContent, trxContent);
+    }
+
     private static void AssertTrxOutcome(XDocument xml, string expectedOutcome)
     {
         Assert.IsNotNull(xml);