Cache the parsed managed name per TestMethod to avoid redundant parsing

[Amaury Levé (Evangelink)]

Fixes#10363

Problem

MSTestTestNodeConverter.AddTestMethodIdentifier called ManagedNameParser.ParseManagedMethodName and re-derived the namespace/type substrings on every node conversion — even though the parse is a pure function of TestMethod.ManagedTypeName and TestMethod.ManagedMethodName, both of which are immutable for the lifetime of the instance.

The same TestMethod instance is converted several times per executed test: TestExecutionManager.ExecuteTestsWithTestRunnerAsync reports the originalcurrentTest element to RecordStartAsync (in-progress node) and then to SendTestResultsAsync, which loops over the results and calls RecordResultAsync once per row. So a plain test parses twice, and a data-driven test parses once per data row plus one.

Change

Cache the parsed pieces — namespace, type name, method name, arity and parameter types — in a static ConditionalWeakTable<TestMethod, ParsedManagedName> keyed on the TestMethod instance. Per repeated conversion this removes:

• 1× ParseManagedMethodName call (scans the managed signature, allocates the parameter-type list/array)
• 2× string allocations (the namespace and typeName substrings)

Each node still builds its ownTestMethodIdentifierProperty, with its own parameter array. That type publicly exposes ParameterTypeFullNames, and TestNode.Properties is handed to arbitrary platform extensions, so caching and sharing the property instance itself would let any consumer that writes to the array corrupt every other node built from the same test method. An empty array cannot be mutated, so the (overwhelmingly common) parameterless case still allocates no array.

The cache lives in the converter rather than as a lazy property on TestMethod (the way TestMethod.ManagedTypeName caches itself) because TestMethodIdentifierProperty is a Microsoft.Testing.Platform type and MSTestAdapter.PlatformServices does not reference the platform; within MSTest.TestAdapter the platform is only reachable from the TestingPlatformAdapter folder via the file-level RS0030 suppression.

Behavior is unchanged: the null/empty guards are equivalent (HasManagedMethodAndTypeProperties already subsumes the removed IsNullOrEmpty(ManagedMethodName) check), and InvalidManagedNameException still propagates out of the factory with nothing cached, so it is re-thrown identically on the next call.

Safety notes

• Key stability — ManagedMethodName and FullClassName are assigned only in the TestMethod constructor; Clone(), CloneWithSource(), CloneWithUpdatedSource() and the [Serializable] app-domain path all produce new key objects with identical values, so the cache can only miss, never go stale.
• No aliasing — the cached object is private to the converter and never escapes; every TestNode gets a freshly built property and a non-aliased parameter array.
• Lifetime — weak keys mean entries disappear as soon as the TestMethod becomes unreachable, so there is no unbounded growth.
• Thread safety — ConditionalWeakTable.GetValue is thread-safe; concurrent misses may each run the factory, but a single value is published.
• The #pragma warning disable IDE0028 is required: a collection expression on ConditionalWeakTable is CS9174 on net462, which lacks IEnumerable<KeyValuePair<,>> on that type.

Tests

Four tests added to MSTestTestNodeConverterTests:

• ToResultTestNode_ProducesEquivalentTestMethodIdentifier_AsInProgressNode — the cached parse must yield an identifier equal on every field across nodes.
• ToResultTestNode_DoesNotShareParameterTypeArray_WithInProgressNode — pins the non-aliasing guarantee using a parameterized managed name.
• ToDiscoveredTestNode_DoesNotShareTestMethodIdentifier_AcrossDistinctTestMethods — the cache must not conflate two test methods that share a class name.
• ToDiscoveredTestNode_DoesNotAddTestMethodIdentifier_WhenManagedTypeNameIsEmpty — pins the pre-existing empty-ManagedTypeName guard that moved into the fast path.

Full repo build.cmd is clean (0 warnings, 0 errors); MSTestAdapter.UnitTests is 68/68 green on both net9.0 and net462.

Note: the perf claim is based on code inspection (call-path analysis + removed allocations), not a benchmark.

[Copilot]

Pull request overview

Caches parsed MSTest method identifiers during MTP node conversion to reduce repeated parsing and allocations.

Changes:

• Adds a weak per-TestMethod identifier cache.
• Adds regression and cache-isolation tests.

Show a summary per file

File	Description
MSTestTestNodeConverter.cs	Implements identifier caching.
MSTestTestNodeConverterTests.cs	Tests reuse, isolation, and empty type handling.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Summary

Clean, well-motivated performance optimization. No issues found.

Correctness ✅ — The guard logic is preserved: HasManagedMethodAndTypeProperties already ensures ManagedMethodName is non-null/non-whitespace, and the new StringEx.IsNullOrEmpty(testMethod.ManagedTypeName) check covers the edge case where FullClassName is empty (previously caught by the second null check inside the method). The ! suppressions in BuildTestMethodIdentifier are safe given the caller's pre-validation.

Thread safety ✅ — ConditionalWeakTable.GetValue guarantees a single published value even under concurrent factory execution. No mutable shared state is introduced.

Lifetime / leaks ✅ — Weak keys ensure entries are collected with the TestMethod. No strong reference cycle.

Shared array note — The remarks correctly call out that ParameterTypeFullNames is a shared array. Since all consumers are read-only today this is safe, but worth keeping in mind for future changes.

Tests ✅ — Good coverage: reuse across node types, distinct instances for distinct keys, and the new empty-ManagedTypeName edge case.

No blocking concerns. LGTM.

[Copilot]

Review details

Suppressed comments (2)

test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs:346

• This assertion only checks structural equality, so it also passes with the pre-change implementation that reparses every node; none of these tests currently fail if ParsedManagedNameCache is removed. Add reference-identity assertions for cached parse outputs such as Namespace and TypeName so the intended reuse is covered while retaining the separate parameter-array assertion below.

 result.Should().Be(inProgress);

src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs:196

• The implementation caches ParsedManagedName but still calls ToProperty() for every conversion, allocating a new TestMethodIdentifierProperty (and cloning non-empty parameter arrays). This materially differs from the PR title/description, which says the built property is cached and counts its allocation among the savings. Please update the PR metadata and performance accounting to describe the parse-only cache and its actual allocation savings.

 // 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();

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Amaury Levé (Evangelink)]

Both suppressed comments from the latest review were valid. Acted on both in e8e0600.

This assertion only checks structural equality, so it also passes with the pre-change implementation that reparses every node; none of these tests currently fail if ParsedManagedNameCache is removed.

Correct, and my miss: when I moved from caching the property to caching the parse, the old BeSameAs guard became value equality and stopped pinning reuse. Fixed as suggested by asserting reference identity on the cached parse outputs. Namespace and TypeName are partial Substring results of ManagedTypeName, so a re-parse hands back fresh string instances, which makes reference equality a genuine cache guard. Deliberately not asserting it on MethodName: for a parameterless name the parser captures the whole string, and String.Substring(0, Length) returns the same instance, so it would be reference-equal with or without the cache. Verified by fault injection - swapping the lookup for a direct ParsedManagedName.Parse call makes the test fail, and it passes again once the cache is restored. Test renamed to ToResultTestNode_ReusesCachedManagedNameParse_FromInProgressNode.

This materially differs from the PR title/description, which says the built property is cached and counts its allocation among the savings. Please update the PR metadata and performance accounting.

Agreed. The description was already rewritten for the parse-only cache, and I have now updated the title to match. The accounting claims only what is actually saved per repeated conversion: one ParseManagedMethodName call plus the two substring allocations. The TestMethodIdentifierProperty allocation is intentionally no longer claimed, since each node builds its own to avoid sharing the publicly exposed ParameterTypeFullNames array.

[Copilot]

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

🧪 Test quality grade — PR #10366

Grade	Test	Mutation	Notes	How to improve
A (90–100)	new MSTestTestNodeConverterTests. ToDiscoveredTestNode_ DoesNotAddTestMethodIdentifier_ WhenManagedTypeNameIsEmpty	1/1 killed	Directly exercises the new empty-ManagedTypeName guard; removing it causes the identifier to be added and the assertion fails.	—
A (90–100)	new MSTestTestNodeConverterTests. ToDiscoveredTestNode_ DoesNotShareTestMethodIdentifier_ AcrossDistinctTestMethods	2/2 killed	Reference-inequality plus per-method-name assertions verify the cache is keyed by TestMethod instance, not shared across them.	—
A (90–100)	new MSTestTestNodeConverterTests. ToResultTestNode_ DoesNotShareParameterTypeArray_ WithInProgressNode	1/1 killed	NotBeSameAs on the parameter array directly pins the defensive copy in ToProperty(); removing the copy kills this assertion.	—
A (90–100)	new MSTestTestNodeConverterTests. ToResultTestNode_ ReusesCachedManagedNameParse_ FromInProgressNode	2/2 killed	Clever pairing of value equality (correctness) with BeSameAs reference equality (proves the cache path, not just correct values).	—

_{This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with /grade-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 62.4 AIC · ⌖ 4.24 AIC · ⊞ 11.8K · [◷]( · ◷)