diff --git a/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs b/src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs index 2642da9395..96d5ab72d2 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 parsed pieces of a 's managed name. + /// + /// + /// 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 + /// 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). + /// + /// + /// 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 ParsedManagedNameCache = new(); +#pragma warning restore IDE0028 + /// /// Builds a discovered-state for a discovered test. /// @@ -155,30 +187,65 @@ 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)) + // The method group conversion is cached by the compiler, so the lookup does not allocate a delegate. + TestMethodIdentifierProperty testMethodIdentifier = ParsedManagedNameCache.GetValue(testMethod, ParsedManagedName.Parse).ToProperty(); + testNode.Properties.Add(testMethodIdentifier); + return testMethodIdentifier; + } + + /// + /// The parsed pieces of a 's managed type and method names, cached by + /// . + /// + private sealed class ParsedManagedName + { + private readonly string _namespace; + private readonly string _typeName; + private readonly string _methodName; + private readonly int _arity; + private readonly string[] _parameterTypeFullNames; + + private ParsedManagedName(string @namespace, string typeName, string methodName, int arity, string[] parameterTypeFullNames) { - return null; + _namespace = @namespace; + _typeName = typeName; + _methodName = methodName; + _arity = arity; + _parameterTypeFullNames = parameterTypeFullNames; } - ManagedNameParser.ParseManagedMethodName(managedMethod, out string methodName, out int arity, out string[]? parameterTypes); - parameterTypes ??= []; + 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!; - int lastIndexOfDot = managedType.LastIndexOf('.'); - string @namespace = lastIndexOfDot == -1 ? string.Empty : managedType[..lastIndexOfDot]; - string typeName = lastIndexOfDot == -1 ? managedType : managedType[(lastIndexOfDot + 1)..]; + 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. - var testMethodIdentifier = new TestMethodIdentifierProperty(assemblyFullName: string.Empty, @namespace, typeName, methodName, arity, parameterTypes, returnTypeFullName: string.Empty); - testNode.Properties.Add(testMethodIdentifier); - return testMethodIdentifier; + 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 a9394ccbbb..5cd09a6e85 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,59 @@ public void ToResultTestNode_DoesNotAddTrxProperties_WhenTrxDisabled() node.Properties.Any().Should().BeFalse(); } + // --- TestMethodIdentifier caching ------------------------------------------------------------------------- + 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.Single(); + TestMethodIdentifierProperty result = MSTestTestNodeConverter.ToResultTestNode(element, new FrameworkTestResult { Outcome = UnitTestOutcome.Passed }, DateTimeOffset.Now, DateTimeOffset.Now, isTrxEnabled: false, new MSTestSettings()) + .Properties.Single(); + + 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() + { + // 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() + { + // 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() {