From 95f32cdb9890a893010ef5ccf999fab2bcd38733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 31 Jul 2026 20:18:40 +0200 Subject: [PATCH 1/3] Cache TestMethodIdentifierProperty per TestMethod MSTestTestNodeConverter.AddTestMethodIdentifier called ManagedNameParser.ParseManagedMethodName and allocated a new TestMethodIdentifierProperty on every node conversion, even though the result is a pure function of TestMethod.ManagedTypeName and TestMethod.ManagedMethodName, which are immutable for the lifetime of the instance. The same TestMethod is converted several times per executed test: once for the in-progress node and once per result node (one per data row for data-driven tests). Cache the built property in a static ConditionalWeakTable keyed on the TestMethod instance so the parse, the two substring allocations and the property allocation are paid once per test method instead of once per node. The cache lives in the converter rather than as a lazy property on TestMethod because TestMethodIdentifierProperty is a Microsoft.Testing.Platform type and MSTestAdapter.PlatformServices does not reference the platform. Fixes #10363 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01fd9297-53cd-4532-b294-a8da5723825d --- .../MSTestTestNodeConverter.cs | 55 +++++++++++++++---- .../MSTestTestNodeConverterTests.cs | 41 ++++++++++++++ 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs b/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs index 2642da9395..e06f8a2bcb 100644 --- a/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs +++ b/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs @@ -32,6 +32,38 @@ namespace Microsoft.VisualStudio.TestTools.UnitTesting; [SuppressMessage("ApiDesign", "RS0030:Do not use banned APIs", Justification = "We can use MTP from this folder")] internal static class MSTestTestNodeConverter { + /// + /// Caches the built for a given . + /// + /// + /// The property is a pure function of and + /// , both immutable for the lifetime of the instance, and building it + /// parses the managed method signature and allocates the namespace/type substrings. The same + /// is converted several times per executed test (the in-progress node, then one result + /// node per data row), so the parse and its allocations are paid once per test method rather than once per node. + /// + /// The cache lives here rather than as a lazy property on (the way + /// caches itself) because + /// is a Microsoft.Testing.Platform type, and MSTestAdapter.PlatformServices - where + /// lives - does not reference the platform at all. Even within this assembly the platform is only reachable + /// from this folder, through the file-level RS0030 suppression above (see this project's BannedSymbols.txt). + /// + /// + /// Every consumer only ever reads the property, so handing the same instance to several nodes is safe. Note + /// that the type is not deeply immutable: is + /// an array that used to be freshly allocated per node and is now shared, so consumers must keep treating it + /// as read-only. + /// + /// + /// The table holds only weak references to its keys, so entries disappear as soon as the + /// becomes unreachable. GetValue is thread-safe: concurrent misses may each run + /// the factory, but a single value is published to every caller. + /// + /// +#pragma warning disable IDE0028 // ConditionalWeakTable is not collection-expression-constructible on .NET Framework (CS9174). + private static readonly ConditionalWeakTable TestMethodIdentifierCache = new(); +#pragma warning restore IDE0028 + /// /// Builds a discovered-state for a discovered test. /// @@ -155,17 +187,22 @@ private static void AddCategoriesAndTraits(TestNode testNode, UnitTestElement el { // NOTE: ManagedMethodName, in case of MSTest, carries the parameter types, so we prefer it to display the // parameter types in Test Explorer. This mirrors what the VSTest bridge did in AddAdditionalProperties. - if (!testMethod.HasManagedMethodAndTypeProperties) + if (!testMethod.HasManagedMethodAndTypeProperties || StringEx.IsNullOrEmpty(testMethod.ManagedTypeName)) { return null; } - string? managedType = testMethod.ManagedTypeName; - string? managedMethod = testMethod.ManagedMethodName; - if (StringEx.IsNullOrEmpty(managedType) || StringEx.IsNullOrEmpty(managedMethod)) - { - return null; - } + // The method group conversion is cached by the compiler, so the lookup does not allocate a delegate. + TestMethodIdentifierProperty testMethodIdentifier = TestMethodIdentifierCache.GetValue(testMethod, BuildTestMethodIdentifier); + testNode.Properties.Add(testMethodIdentifier); + return testMethodIdentifier; + } + + private static TestMethodIdentifierProperty BuildTestMethodIdentifier(TestMethod testMethod) + { + // AddTestMethodIdentifier is the only caller and has already validated both managed names. + string managedType = testMethod.ManagedTypeName!; + string managedMethod = testMethod.ManagedMethodName!; ManagedNameParser.ParseManagedMethodName(managedMethod, out string methodName, out int arity, out string[]? parameterTypes); parameterTypes ??= []; @@ -176,9 +213,7 @@ private static void AddCategoriesAndTraits(TestNode testNode, UnitTestElement el // AssemblyFullName and ReturnTypeFullName are not carried by the neutral model today; kept empty to match // the current (bridge) behavior. Populating them is a follow-up enabled by this native path. - var testMethodIdentifier = new TestMethodIdentifierProperty(assemblyFullName: string.Empty, @namespace, typeName, methodName, arity, parameterTypes, returnTypeFullName: string.Empty); - testNode.Properties.Add(testMethodIdentifier); - return testMethodIdentifier; + return new TestMethodIdentifierProperty(assemblyFullName: string.Empty, @namespace, typeName, methodName, arity, parameterTypes, returnTypeFullName: string.Empty); } private static void AddOutcome(TestNode testNode, TestOutcome outcome, string? errorMessage, string? errorStackTrace) diff --git a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs index a9394ccbbb..9f952b3df5 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs @@ -91,6 +91,16 @@ public void ToDiscoveredTestNode_DoesNotAddTestMethodIdentifier_WhenNoManagedMet node.Properties.Any().Should().BeFalse(); } + public void ToDiscoveredTestNode_DoesNotAddTestMethodIdentifier_WhenManagedTypeNameIsEmpty() + { + // ManagedTypeName is derived from FullClassName, so an empty class name leaves no usable type identity. + UnitTestElement element = CreateElement(fullClassName: string.Empty); + + TestNode node = MSTestTestNodeConverter.ToDiscoveredTestNode(element, isTrxEnabled: false); + + node.Properties.Any().Should().BeFalse(); + } + public void ToDiscoveredTestNode_AddsFileLocation_WhenDeclaringFileKnown() { UnitTestElement element = CreateElement(); @@ -320,6 +330,37 @@ public void ToResultTestNode_DoesNotAddTrxProperties_WhenTrxDisabled() node.Properties.Any().Should().BeFalse(); } + // --- TestMethodIdentifier caching ------------------------------------------------------------------------- + public void ToResultTestNode_ReusesTestMethodIdentifier_FromInProgressNode() + { + // The property is a pure function of the immutable managed names, so it is parsed and allocated once per + // TestMethod: the in-progress node and every result node are handed the very same cached instance. + UnitTestElement element = CreateElement(); + + TestMethodIdentifierProperty? inProgress = MSTestTestNodeConverter.ToInProgressTestNode(element, isTrxEnabled: false) + .Properties.SingleOrDefault(); + TestMethodIdentifierProperty? result = MSTestTestNodeConverter.ToResultTestNode(element, new FrameworkTestResult { Outcome = UnitTestOutcome.Passed }, DateTimeOffset.Now, DateTimeOffset.Now, isTrxEnabled: false, new MSTestSettings()) + .Properties.SingleOrDefault(); + + inProgress.Should().NotBeNull(); + result.Should().BeSameAs(inProgress); + } + + public void ToDiscoveredTestNode_DoesNotShareTestMethodIdentifier_AcrossDistinctTestMethods() + { + // The cache is keyed on the TestMethod instance, so two different test methods must never be conflated. + TestMethodIdentifierProperty? first = MSTestTestNodeConverter.ToDiscoveredTestNode(CreateElement(managedMethodName: "MethodA", name: "MethodA"), isTrxEnabled: false) + .Properties.SingleOrDefault(); + TestMethodIdentifierProperty? second = MSTestTestNodeConverter.ToDiscoveredTestNode(CreateElement(managedMethodName: "MethodB", name: "MethodB"), isTrxEnabled: false) + .Properties.SingleOrDefault(); + + first.Should().NotBeNull(); + second.Should().NotBeNull(); + second.Should().NotBeSameAs(first); + first!.MethodName.Should().Be("MethodA"); + second!.MethodName.Should().Be("MethodB"); + } + // --- GetTestId caching ------------------------------------------------------------------------------------ public void GetTestId_CachesComputedId_AndReturnsSameValueOnSubsequentCalls() { From c6dcf276037d883b3e204130df1251d8fb362b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Sat, 1 Aug 2026 09:39:47 +0200 Subject: [PATCH 2/3] Cache only the managed-name parse, not the identifier property TestMethodIdentifierProperty publicly exposes its ParameterTypeFullNames array, so handing a single cached instance to several TestNodes let any consumer that writes to that array corrupt every other node built from the same TestMethod - a behavior change for external platform extensions. Cache the parsed pieces (namespace, type name, method name, arity and parameter types) instead and build a fresh property, with its own parameter array, per node. The expensive part - ParseManagedMethodName plus the two substring allocations - is still paid once per TestMethod. An empty parameter array cannot be mutated, so the common parameterless case still allocates no array. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01fd9297-53cd-4532-b294-a8da5723825d --- .../MSTestTestNodeConverter.cs | 84 +++++++++++++------ .../MSTestTestNodeConverterTests.cs | 24 +++++- 2 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs b/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs index e06f8a2bcb..96d5ab72d2 100644 --- a/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs +++ b/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs @@ -33,14 +33,20 @@ namespace Microsoft.VisualStudio.TestTools.UnitTesting; internal static class MSTestTestNodeConverter { /// - /// Caches the built for a given . + /// Caches the parsed pieces of a 's managed name. /// /// - /// The property is a pure function of and - /// , both immutable for the lifetime of the instance, and building it - /// parses the managed method signature and allocates the namespace/type substrings. The same - /// is converted several times per executed test (the in-progress node, then one result - /// node per data row), so the parse and its allocations are paid once per test method rather than once per node. + /// The parse is a pure function of and + /// , both immutable for the lifetime of the instance, and it scans the + /// managed method signature and allocates the namespace/type substrings. The same is + /// converted several times per executed test (the in-progress node, then one result node per data row), so the + /// parse is paid once per test method rather than once per node. + /// + /// Only the parse is cached, not the resulting : that type publicly + /// exposes its array, so handing one instance + /// to several nodes would let any consumer that writes to the array corrupt every other node built from the + /// same test method. Each node therefore still gets its own property (see ). + /// /// /// The cache lives here rather than as a lazy property on (the way /// caches itself) because @@ -49,19 +55,13 @@ internal static class MSTestTestNodeConverter /// from this folder, through the file-level RS0030 suppression above (see this project's BannedSymbols.txt). /// /// - /// Every consumer only ever reads the property, so handing the same instance to several nodes is safe. Note - /// that the type is not deeply immutable: is - /// an array that used to be freshly allocated per node and is now shared, so consumers must keep treating it - /// as read-only. - /// - /// /// The table holds only weak references to its keys, so entries disappear as soon as the /// becomes unreachable. GetValue is thread-safe: concurrent misses may each run /// the factory, but a single value is published to every caller. /// /// #pragma warning disable IDE0028 // ConditionalWeakTable is not collection-expression-constructible on .NET Framework (CS9174). - private static readonly ConditionalWeakTable TestMethodIdentifierCache = new(); + private static readonly ConditionalWeakTable ParsedManagedNameCache = new(); #pragma warning restore IDE0028 /// @@ -193,27 +193,59 @@ private static void AddCategoriesAndTraits(TestNode testNode, UnitTestElement el } // The method group conversion is cached by the compiler, so the lookup does not allocate a delegate. - TestMethodIdentifierProperty testMethodIdentifier = TestMethodIdentifierCache.GetValue(testMethod, BuildTestMethodIdentifier); + TestMethodIdentifierProperty testMethodIdentifier = ParsedManagedNameCache.GetValue(testMethod, ParsedManagedName.Parse).ToProperty(); testNode.Properties.Add(testMethodIdentifier); return testMethodIdentifier; } - private static TestMethodIdentifierProperty BuildTestMethodIdentifier(TestMethod testMethod) + /// + /// The parsed pieces of a 's managed type and method names, cached by + /// . + /// + private sealed class ParsedManagedName { - // AddTestMethodIdentifier is the only caller and has already validated both managed names. - string managedType = testMethod.ManagedTypeName!; - string managedMethod = testMethod.ManagedMethodName!; + private readonly string _namespace; + private readonly string _typeName; + private readonly string _methodName; + private readonly int _arity; + private readonly string[] _parameterTypeFullNames; - ManagedNameParser.ParseManagedMethodName(managedMethod, out string methodName, out int arity, out string[]? parameterTypes); - parameterTypes ??= []; + private ParsedManagedName(string @namespace, string typeName, string methodName, int arity, string[] parameterTypeFullNames) + { + _namespace = @namespace; + _typeName = typeName; + _methodName = methodName; + _arity = arity; + _parameterTypeFullNames = parameterTypeFullNames; + } - int lastIndexOfDot = managedType.LastIndexOf('.'); - string @namespace = lastIndexOfDot == -1 ? string.Empty : managedType[..lastIndexOfDot]; - string typeName = lastIndexOfDot == -1 ? managedType : managedType[(lastIndexOfDot + 1)..]; + public static ParsedManagedName Parse(TestMethod testMethod) + { + // AddTestMethodIdentifier is the only caller and has already validated both managed names. + string managedType = testMethod.ManagedTypeName!; + string managedMethod = testMethod.ManagedMethodName!; + + ManagedNameParser.ParseManagedMethodName(managedMethod, out string methodName, out int arity, out string[]? parameterTypes); - // AssemblyFullName and ReturnTypeFullName are not carried by the neutral model today; kept empty to match - // the current (bridge) behavior. Populating them is a follow-up enabled by this native path. - return new TestMethodIdentifierProperty(assemblyFullName: string.Empty, @namespace, typeName, methodName, arity, parameterTypes, returnTypeFullName: string.Empty); + int lastIndexOfDot = managedType.LastIndexOf('.'); + string @namespace = lastIndexOfDot == -1 ? string.Empty : managedType[..lastIndexOfDot]; + string typeName = lastIndexOfDot == -1 ? managedType : managedType[(lastIndexOfDot + 1)..]; + + return new ParsedManagedName(@namespace, typeName, methodName, arity, parameterTypes ?? []); + } + + public TestMethodIdentifierProperty ToProperty() + { + // Every node gets its own parameter array. TestMethodIdentifierProperty exposes it publicly, so + // aliasing one array across nodes would let a consumer that writes to it corrupt every other node + // built from the same test method. An empty array cannot be mutated, so the common parameterless + // case still allocates nothing here. + string[] parameterTypeFullNames = _parameterTypeFullNames.Length == 0 ? _parameterTypeFullNames : [.. _parameterTypeFullNames]; + + // AssemblyFullName and ReturnTypeFullName are not carried by the neutral model today; kept empty to + // match the current (bridge) behavior. Populating them is a follow-up enabled by this native path. + return new TestMethodIdentifierProperty(assemblyFullName: string.Empty, _namespace, _typeName, _methodName, _arity, parameterTypeFullNames, returnTypeFullName: string.Empty); + } } private static void AddOutcome(TestNode testNode, TestOutcome outcome, string? errorMessage, string? errorStackTrace) diff --git a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs index 9f952b3df5..b946977fb8 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs @@ -331,10 +331,10 @@ public void ToResultTestNode_DoesNotAddTrxProperties_WhenTrxDisabled() } // --- TestMethodIdentifier caching ------------------------------------------------------------------------- - public void ToResultTestNode_ReusesTestMethodIdentifier_FromInProgressNode() + public void ToResultTestNode_ProducesEquivalentTestMethodIdentifier_AsInProgressNode() { - // The property is a pure function of the immutable managed names, so it is parsed and allocated once per - // TestMethod: the in-progress node and every result node are handed the very same cached instance. + // The managed-name parse is cached per TestMethod, so the in-progress node and every result node must + // still agree on every field of the identifier. UnitTestElement element = CreateElement(); TestMethodIdentifierProperty? inProgress = MSTestTestNodeConverter.ToInProgressTestNode(element, isTrxEnabled: false) @@ -343,7 +343,23 @@ public void ToResultTestNode_ReusesTestMethodIdentifier_FromInProgressNode() .Properties.SingleOrDefault(); inProgress.Should().NotBeNull(); - result.Should().BeSameAs(inProgress); + result.Should().Be(inProgress); + } + + public void ToResultTestNode_DoesNotShareParameterTypeArray_WithInProgressNode() + { + // TestMethodIdentifierProperty exposes ParameterTypeFullNames publicly, so nodes must never alias one + // array: a consumer that writes to it would otherwise corrupt every other node of the same test method. + UnitTestElement element = CreateElement(managedMethodName: "MyMethod(System.String)"); + + TestMethodIdentifierProperty inProgress = MSTestTestNodeConverter.ToInProgressTestNode(element, isTrxEnabled: false) + .Properties.Single(); + TestMethodIdentifierProperty result = MSTestTestNodeConverter.ToResultTestNode(element, new FrameworkTestResult { Outcome = UnitTestOutcome.Passed }, DateTimeOffset.Now, DateTimeOffset.Now, isTrxEnabled: false, new MSTestSettings()) + .Properties.Single(); + + inProgress.ParameterTypeFullNames.Should().Equal("System.String"); + result.ParameterTypeFullNames.Should().Equal("System.String"); + result.ParameterTypeFullNames.Should().NotBeSameAs(inProgress.ParameterTypeFullNames); } public void ToDiscoveredTestNode_DoesNotShareTestMethodIdentifier_AcrossDistinctTestMethods() From e8e060027c560dca383a43c0dfdd412a52d43628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Sat, 1 Aug 2026 09:55:56 +0200 Subject: [PATCH 3/3] Make the caching test an actual regression guard The value-equality assertion alone passed even with ParsedManagedNameCache removed, so it only pinned correctness, not reuse. Namespace and TypeName are partial Substring results of ManagedTypeName, so re-running the parse hands back fresh string instances; asserting reference equality on them fails when the cache is bypassed and therefore guards the caching itself. Verified by temporarily calling ParsedManagedName.Parse directly: the test fails, and passes again once the cache lookup is restored. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01fd9297-53cd-4532-b294-a8da5723825d --- .../MSTestTestNodeConverterTests.cs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs index b946977fb8..5cd09a6e85 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs @@ -331,19 +331,25 @@ public void ToResultTestNode_DoesNotAddTrxProperties_WhenTrxDisabled() } // --- TestMethodIdentifier caching ------------------------------------------------------------------------- - public void ToResultTestNode_ProducesEquivalentTestMethodIdentifier_AsInProgressNode() + public void ToResultTestNode_ReusesCachedManagedNameParse_FromInProgressNode() { // The managed-name parse is cached per TestMethod, so the in-progress node and every result node must // still agree on every field of the identifier. UnitTestElement element = CreateElement(); - TestMethodIdentifierProperty? inProgress = MSTestTestNodeConverter.ToInProgressTestNode(element, isTrxEnabled: false) - .Properties.SingleOrDefault(); - TestMethodIdentifierProperty? result = MSTestTestNodeConverter.ToResultTestNode(element, new FrameworkTestResult { Outcome = UnitTestOutcome.Passed }, DateTimeOffset.Now, DateTimeOffset.Now, isTrxEnabled: false, new MSTestSettings()) - .Properties.SingleOrDefault(); + TestMethodIdentifierProperty inProgress = MSTestTestNodeConverter.ToInProgressTestNode(element, isTrxEnabled: false) + .Properties.Single(); + TestMethodIdentifierProperty result = MSTestTestNodeConverter.ToResultTestNode(element, new FrameworkTestResult { Outcome = UnitTestOutcome.Passed }, DateTimeOffset.Now, DateTimeOffset.Now, isTrxEnabled: false, new MSTestSettings()) + .Properties.Single(); - inProgress.Should().NotBeNull(); result.Should().Be(inProgress); + + // Namespace and TypeName are partial Substring results of ManagedTypeName, so re-running the parse would + // hand back fresh string instances. Reference equality is therefore what proves the cached parse was + // reused rather than redone - without the cache these assertions fail while the value equality above + // still passes. + result.Namespace.Should().BeSameAs(inProgress.Namespace); + result.TypeName.Should().BeSameAs(inProgress.TypeName); } public void ToResultTestNode_DoesNotShareParameterTypeArray_WithInProgressNode()