perf: eliminate eager testFullName allocation in MSTestTestNodeConverter#9824
perf: eliminate eager testFullName allocation in MSTestTestNodeConverter#9824Evangelink wants to merge 1 commit into
Conversation
In CreateBaseTestNode, testFullName was computed eagerly even though it is only needed as a final fallback when both displayNameOverride and TestMethod.DisplayName are null. DisplayName is always initialized, so the allocation was never used. Inline the interpolation so it is only evaluated if the earlier operands are null. In AddTrxResultProperties, use testMethod.DisplayName directly for TrxTestDefinitionName and testMethod.FullClassName directly in the TestMethodIdentifierProperty-absent fallback, removing the now-dead TryParseFullyQualifiedType helper. Fixes #9823 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Clean performance optimization — no issues found across applicable dimensions.
| Dimension | Verdict |
|---|---|
| 1. Algorithmic Correctness | ✅ Pass |
| 2. Performance | ✅ Pass (this is the improvement) |
| 3. Public API | N/A (no public API changes) |
| 4. Backward Compatibility | ✅ Pass — behavior is identical |
| 5–22. Remaining dimensions | ✅ Pass / N/A |
Key observations
-
Correctness of
FullClassNamesubstitution: The oldTryParseFullyQualifiedTypereconstructedFullClassName.Namethen parsed back out the part before the last dot — which is justFullClassName. UsingFullClassNamedirectly is both simpler and more correct (e.g., ifNamecontained a.from data-driven parameters, the old parser could produce wrong results). -
Dead-code removal is safe:
TryParseFullyQualifiedTypehad no other callers, so removing it has no impact. -
Null-coalescing fallback: The PR correctly notes
DisplayNameis always non-null per theTestMethodconstructor, so the interpolated string is never evaluated in practice. The fallback is retained as a defensive measure, which is appropriate.
No action items — LGTM.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the MSTest adapter’s MSTestTestNodeConverter by eliminating an eager testFullName string interpolation that was effectively always unused, reducing transient allocations during test discovery/execution/result reporting.
Changes:
- In
CreateBaseTestNode, removed the eagerly-allocatedtestFullNamelocal and made the interpolation lazy (only evaluated if earlier operands are null). - In
AddTrxResultProperties, removed dead uses oftestFullNameand simplified the fallback path by usingtestMethod.FullClassNamedirectly. - Deleted the now-unused
TryParseFullyQualifiedTypehelper.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs | Removes eager testFullName allocation and simplifies TRX property population to avoid redundant work. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Low
| // TestMethod.DisplayName is always initialized (constructor sets it to displayName ?? name). | ||
| testNode.Properties.Add(new TrxTestDefinitionName(testMethod.DisplayName ?? $"{testMethod.FullClassName}.{testMethod.Name}")); |
| // TestMethod.DisplayName is always initialized (the constructor sets it to displayName ?? name), so the | ||
| // interpolated fallback is only evaluated (and only allocates) in the theoretically-null case. | ||
| DisplayName = displayNameOverride ?? testMethod.DisplayName ?? $"{testMethod.FullClassName}.{testMethod.Name}", |
| else | ||
| { | ||
| throw new InvalidOperationException("Unable to parse fully qualified type name from test: " + testFullName); | ||
| throw new InvalidOperationException("Unable to parse fully qualified type name from test: " + $"{testMethod.FullClassName}.{testMethod.Name}"); |
🔴 Build Failure AnalysisBuild status: FAILED (249.9s) — 5 errors, 1 warning Root CauseAll 4 compilation errors are CA1416 (platform compatibility) in
The FixAdd
|
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 97.7 AIC · ⌖ 5.83 AIC · ⊞ 7.3K · ◷
Fixes #9823
Summary
MSTestTestNodeConverter.CreateBaseTestNodeeagerly computedstring testFullName = $"{testMethod.FullClassName}.{testMethod.Name}";on every call, but the string was only used as a final null-coalescing fallback forDisplayName. BecauseTestMethod.DisplayNameis always non-null (the constructor sets it todisplayName ?? name), this allocation was thrown away on every call — 3× per test per run (discovery, in-progress, result).AddTrxResultPropertieshad the same variable for two additional dead uses.Changes
CreateBaseTestNode: inlinetestFullNameinto the null-coalescing chain so the interpolation is only evaluated when both prior operands are null (they never are).AddTrxResultProperties: usetestMethod.DisplayNamedirectly forTrxTestDefinitionName; usetestMethod.FullClassNamedirectly in theTestMethodIdentifierProperty-absent fallback path (it is the source of truth for the type name, avoiding a re-parse of a string that already concatenatedFullClassName); remove the now-deadTryParseFullyQualifiedTypeprivate method.Performance
For a 10,000-test suite this eliminates ~30,000 transient string allocations per run in
CreateBaseTestNode, plus one string allocation and the associated parse work per TRX-result node in the fallback path.Trade-offs
None.
DisplayNameis documented as always initialized andFullClassNameis the source of truth for the type name, so behavior is identical.Test status
MSTest.TestAdapterbuilds cleanly (0 warnings, 0 errors). All 29MSTestTestNodeConverterunit tests pass (net9.0).