Uh oh!
There was an error while loading. Please reload this page.
Eliminate delegate allocation in AbortForMaxFailedTestsExtension - #7923
Conversation
…stsExtension Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/7398bf36-606a-4496-99d7-ac5f8d270ec3 Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes a hot-path check in AbortForMaxFailedTestsExtension by removing a per-call LINQ delegate allocation when determining whether a TestNodeStateProperty represents a failure outcome type.
Changes:
- Replace
Any(t => t == ...)withArray.IndexOf(..., ...) != -1for membership testing against the well-known “failed outcome”Type[].
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/Extensions/AbortForMaxFailedTestsExtension.cs | Removes LINQ-based type membership check in ConsumeAsync to avoid delegate allocation in a hot path. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Summary
Workflow: Expert Code Reviewer 🧠
Date: 2026-04-29
Repository: microsoft/testfx
Key Findings
No issues found. The change is correct, semantically equivalent, and consistent with the existing codebase pattern.
Verification points:
Any(t => t == x)andArray.IndexOf(..., x) != -1both use reference equality forTypeobjects.Typeinstances are runtime singletons in .NET, so both return identical results.- The same
Array.IndexOfpattern is already used inTestApplicationResult.cs,RetryDataConsumer.cs, andTrxReportEngine.csfor the exact sameWellKnownTestNodeTestRunOutcomeFailedPropertiesarray — this PR bringsAbortForMaxFailedTestsExtensioninto alignment with those. - The optimization is real: removing the
Func<Type, bool>delegate allocation on everyConsumeAsynccall in a hot path (one message per test node update) is a meaningful improvement at scale. - No threading concerns introduced — the changed expression is side-effect-free and stateless.
- No public API surface is affected; the class is
internal sealed.
Recommendations
None — the change is minimal, purposeful, and well-aligned with the existing codebase idiom.
Generated by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
AbortForMaxFailedTestsExtension.ConsumeAsyncused.Any(t => t == ...)to check whether a test node's state property is a known failure type, allocating aFunc<Type, bool>delegate on every call.Change
Replace the LINQ call with
Array.IndexOf, consistent with the same check already inTestApplicationResult.cs:One heap object saved per consumed
TestNodeUpdateMessagein the hot path.