perf: skip TestMethodIdentifierProperty for in-progress test nodes#9867
perf: skip TestMethodIdentifierProperty for in-progress test nodes#9867Evangelink wants to merge 2 commits into
Conversation
In the native MTP path, AddTestMethodIdentifier (which calls ManagedNameParser.ParseManagedMethodName and allocates a TestMethodIdentifierProperty + string[]) is called for every node type: discovered, in-progress, and result. In-progress nodes are 'test started' signals -- no consumer reads TestMethodIdentifierProperty from them (IDEs already have the full identity from the discovery node). Skipping this parse for in-progress nodes eliminates ~1/3 of all ParseManagedMethodName calls. Fixes #9864 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f277e70-6fc7-472a-a46f-f7746f505a97
There was a problem hiding this comment.
Pull request overview
Optimizes native MTP test-start events by omitting unused method-identifier metadata.
Changes:
- Adds an opt-out flag when creating base test nodes.
- Skips method-name parsing for in-progress nodes.
- Preserves discovery and result node behavior.
Show a summary per file
| File | Description |
|---|---|
MSTestTestNodeConverter.cs |
Avoids creating method identifiers for in-progress nodes. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Review by Copilot — workflow run
Overall: Clean, well-scoped performance optimization. The change is minimal, correct, and well-documented in the inline comment.
Correctness ✅ — The boolean parameter with default: true ensures discovery and result nodes are unaffected. Only ToInProgressTestNode opts out.
Design ✅ — Using a defaulted parameter keeps the diff small and avoids a breaking change to the internal API signature. The call site is self-documenting with the named argument addMethodIdentifier: false.
Trade-off acknowledgment ✅ — The PR description correctly notes that extensions reading TestMethodIdentifierProperty from in-progress events would break. This is acceptable since no known consumer does so, and the property remains on discovery/result nodes.
No issues found — LGTM.
…de keeps it Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f277e70-6fc7-472a-a46f-f7746f505a97
| // Skip AddTestMethodIdentifier: no consumer reads TestMethodIdentifierProperty from | ||
| // in-progress nodes (IDEs and reporters already hold the full identity from the discovery | ||
| // node). Skipping it avoids a ManagedNameParser.ParseManagedMethodName parse and the | ||
| // associated TestMethodIdentifierProperty / string[] allocation for every started test. | ||
| TestNode testNode = CreateBaseTestNode(element, isTrxEnabled, displayNameOverride: null, addMethodIdentifier: false); |
🧪 Test quality grade — PR #9867
This advisory comment was generated automatically. Grades are heuristic
|
Goal and rationale
In the native MTP path (
MSTestTestNodeConverter),AddTestMethodIdentifieris called for every node type: discovered, in-progress, and result. This method callsManagedNameParser.ParseManagedMethodName, which parses the managed method name string and allocates astring[] parameterTypesand aTestMethodIdentifierPropertyobject.In-progress nodes are "test started" signals — no consumer reads
TestMethodIdentifierPropertyfrom them. IDEs (VS Test Explorer) and reporters already hold the full test identity from the discovery node. Skipping this allocation for in-progress nodes eliminates ~1⁄3 of allParseManagedMethodNamecalls per test run.Approach
Added an
addMethodIdentifierparameter (defaulttrue) toCreateBaseTestNode.ToInProgressTestNodepassesaddMethodIdentifier: false; discovered and result nodes are unaffected.Trade-offs
TestMethodIdentifierProperty. Any extension that reads this property from in-progress events would need to switch to the discovery event instead (which is the correct place for structural metadata).Test Status
.\build.cmd→ Build succeeded, 0 warnings, 0 errorsFixes #9864