[Perf Improver] perf: eliminate LINQ allocations in IsIgnored hot path by Evangelink · Pull Request #8095 · microsoft/testfx

Evangelink · 2026-05-11T09:06:28Z

docs: add AwesomeAssertions to glossary
docs: update glossary - weekly full scan 2026-05-04
perf: eliminate yield-iterator allocations in test execution hot path
feat(fv): Task 1+2 — research two new targets; informal spec for UnitTestOutcomeHelper.ToTestOutcome
perf: eliminate LINQ allocations in IsIgnored hot path
test: add unit tests for AttributeExtensions.IsIgnored grouping semantics

AwesomeAssertions was bumped from 9.3.0 to 9.4.0 (PR #8008) and is the mandated assertion library for this project's test suites. Adding a glossary entry to explain its purpose and relationship to FluentAssertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add four new terms identified in the 7-day full scan: - DynamicData: MSTest attribute for data-driven tests (RFC 006; highlighted by PR #7925 log-accumulation bug fix) - Lean–C# Correspondence (FV): new FV artifact introduced by PR #7936 FV docs validation workflow - PropertyBag: core MTP extension API for typed test node metadata (PR #7940 TestNodeProperties refactor brought these types into focus) - TestNode: core MTP class representing a discovered/executed test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

GetRetryAttribute() called GetAttributes<RetryBaseAttribute>() which allocates a compiler-generated state machine (IEnumerator<T> via yield return) on every test method construction. Replace with direct iteration of the cached Attribute[] from GetCustomAttributesCached(). Preserves duplicate-attribute detection and the custom TypeInspectionException. TryExecuteDataSourceBasedTestsAsync() called _testMethodInfo.GetAttributes<DataSourceAttribute>() which allocates a yield iterator state machine AND a DataSourceAttribute[] array. Replace with ReflectHelper.Instance.IsAttributeDefined<DataSourceAttribute>(), which iterates the same cached array without any allocation. Since DataSourceAttribute is sealed and does not allow multiple, presence == exactly one instance. Proxy metric: heap allocation count - GetRetryAttribute: 1 allocation eliminated per TestMethodInfo construction - TryExecuteDataSourceBasedTestsAsync: 2 allocations eliminated per test execution (for tests without a DataSourceAttribute -- the common case) - Estimated: ~480 KB GC pressure reduction per 10,000-test run GSF principle: Hardware Efficiency -- fewer allocations = less DRAM and GC CPU per joule; Energy Proportionality -- GC cost scales with allocation rate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…TestOutcomeHelper.ToTestOutcome Task 1 (Research & Target Identification): - TARGETS.md: expanded from 7 to 15 targets; all phases and statuses corrected to match merged PRs; full Findings table added; priority order updated with UnitTestOutcomeHelper as top Task-3 candidate. - RESEARCH.md: added three new target entries (Target 8 UnitTestOutcomeHelper, Target 14 EnvironmentVariableParser.ParseBool, Target 15 PasteArguments.ContainsNoWhitespaceOrQuotes) with rationale, FV properties to verify, and approximation notes. Updated Approach Notes to document Lean toolchain blocker. Task 2 (Informal Spec Extraction — UnitTestOutcomeHelper.ToTestOutcome): - New file: formal-verification/specs/unittestoutcomehelper_totestoutcome_informal.md - Full mapping table (11 UnitTestOutcome values × 2 Boolean flags → 14 rows) - 12 numbered postconditions - 4 invariants (pure, total, none→only default, monotonicity) - 6 Lean-ready property previews (all decidable via 'decide') - 3 open questions (InProgress→None intent, lossy round-trip, OOR telemetry) - Test-coverage gap analysis: 5 untested paths identified Task 3 BLOCKED: elan/Lean toolchain install blocked by network firewall (confirmed again this run). No .lean files written. 🔬 Lean Squad — automated FV agent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Replace GetAttributes<ConditionBaseAttribute> (yield iterator) + GroupBy (LINQ operator/Lookup) with direct iteration over GetCustomAttributesCached(). Fast path (no ConditionBaseAttribute - common case): exits with zero allocations after a single O(n) pass over the cached Attribute[] array. Slow path (condition attribute present): manual Dictionary-based grouping instead of LINQ Lookup, allocated only when needed. IsIgnored is called twice per test execution (class + method). For a suite of 1,000 tests this eliminates ~2,000-4,000 short-lived LINQ objects per run in the typical case. Follows the allocation-free pattern from GetTestPropertiesAsTraits and GetTestCategories in the same codebase. Closes #7992 Closes #7993 Closes #8000 Closes #8016 Closes #8028 Closes #8044 Closes #8055 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR reduces per-test execution allocations in MSTest adapter hot paths by replacing LINQ/yield-based attribute enumeration with direct iteration over cached Attribute[] arrays, and it updates repository documentation (glossary + formal-verification target tracking/spec artifacts).

Changes:

Optimized attribute-based execution paths by avoiding GetAttributes<T>()/LINQ allocations and using ReflectHelper.GetCustomAttributesCached(...) + manual loops.
Updated formal-verification tracking docs and added a new informal spec markdown artifact.
Expanded docs/glossary.md with new terms (including an AwesomeAssertions entry).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Helpers/AttributeHelpers.cs	Reworked `IsIgnored` to avoid LINQ allocations via cached attribute array + manual grouping.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Avoids `GetAttributes<DataSourceAttribute>()` array allocation by using `IsAttributeDefined`.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Avoids `GetAttributes<RetryBaseAttribute>()` yield iterator by scanning cached attributes directly.
formal-verification/TARGETS.md	Restructures the FV target list/status and priority ordering.
formal-verification/specs/unittestoutcomehelper_totestoutcome_informal.md	Adds a new informal spec document for `UnitTestOutcomeHelper.ToTestOutcome`.
formal-verification/RESEARCH.md	Adds/revises FV research notes for new targets and toolchain status.
docs/glossary.md	Adds multiple glossary entries (AwesomeAssertions, DynamicData, PropertyBag, TestNode, etc.).

Copilot's findings

Files reviewed: 7/7 changed files
Comments generated: 3

Evangelink

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The PR cleanly eliminates LINQ allocations in the IsIgnored hot path and two related call sites. The logic is preserved correctly. Two minor nitpicks:

Important — TestMethodRunner.cs L182: The comment conflates the roles of sealed and AllowMultiple = false. sealed prevents a subclass co-existence scenario that would break the IsAttributeDefined-based equivalence; AllowMultiple = false prevents duplicate base-type instances. Both are necessary and deserve separate explanations.
Minor — AttributeHelpers.cs L50: When marking a group satisfied, groupState.FirstMessage is preserved in the new tuple even though it can never be read in that state. null would be cleaner.

Positive Highlights

✅ The two-pass fast/slow-path split is a well-considered design: the first pass avoids a Dictionary allocation in the common case (no ConditionBaseAttribute present).
✅ The GetRetryAttribute refactoring in TestMethodInfo.cs is clean with no behavioral changes.
✅ Good inline comments throughout explaining why the allocation-avoiding approach is used.

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

Evangelink

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

[Coverage — IsIgnored grouping logic] The most impactful change in this PR is the replacement of LINQ GroupBy with a manual Dictionary<string, (bool Satisfied, string? FirstMessage)> grouping in AttributeExtensions.IsIgnored. The new logic is non-trivial (handling first-message tracking across null/non-null IgnoreMessage values, multi-group cross-cutting semantics, and early-exit once a group is satisfied). There are no unit tests for AttributeExtensions.IsIgnored in the entire test suite — only acceptance/integration tests provide indirect coverage. The PR is a pure performance refactoring with no test additions to validate the preserved semantics.

Recommendations

Add a AttributeHelpersTests.cs unit test file covering the grouping semantics of IsIgnored: single group (all unsatisfied, at least one satisfied), multiple groups, IgnoreMessage null/non-null first-message tracking. This protects against any future regression of the refactored path.

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Evangelink

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Analysis

Reviewed three code-level changes in this PR across AttributeHelpers.cs, TestMethodInfo.cs, and TestMethodRunner.cs.

`AttributeHelpers.IsIgnored` (main change)

The refactor replaces LINQ GroupBy + yield-iterator with a two-pass manual scan over the already-cached Attribute[]:

Fast path — a single scan to detect any ConditionBaseAttribute. If none is found, returns immediately with zero allocations. Correct: matches the old GetAttributes<ConditionBaseAttribute>() returning empty.
Slow path — a Dictionary<string,(bool,string?)> replaces the Lookup<string,ConditionBaseAttribute> that GroupBy produced.

I traced the grouping semantics in detail against the original code, including edge cases:

First attribute in group is satisfied, rest are not: both paths mark the group satisfied ✓
All attributes unsatisfied, first has null message: new code correctly picks up the first non-null message via groupState.FirstMessage is null guard ✓
Attribute interleaving across multiple groups: Dictionary insertion-order is respected (same as LINQ GroupBy source order), so the returned ignoreMessage is identical ✓

`TestMethodInfo.GetRetryAttribute`

Direct iteration over GetCustomAttributesCached() replaces the yield-iterator path. Duplicate-detection (ThrowMultipleAttributesException when found is not null) is structurally equivalent to the old enumerator.MoveNext() second-check. The [DoesNotReturn] method makes it impossible to accidentally reach found = retryAttr after a throw. ✓

`TestMethodRunner.TryExecuteDataSourceBasedTestsAsync`

The old code checked dataSourceAttribute is { Length: 1 }, implying exactly-one semantics. The new code uses IsAttributeDefined<DataSourceAttribute>, skipping the array allocation. This is safe because DataSourceAttribute is sealed and declared with [AttributeUsage(AllowMultiple = false, Inherited = false)], making the "at most one" invariant enforced at compile time and unavoidable at runtime. ✓

Positive Observations

The fast-path scan is a textbook hot-path optimization: zero allocations in the overwhelmingly common case where no condition attributes are present.
GetCustomAttributesCached is used consistently across all three change sites, keeping reflection cost amortised.
IsAttributeDefined is already a well-established helper in ReflectHelper; repurposing it here is idiomatic.

Conclusion

No correctness, threading, performance regression, API compatibility, or resource management issues were found. The changes deliver the stated allocation reduction with semantically equivalent behavior.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Evangelink · 2026-05-11T09:56:30Z

@copilot address review comments

…, add IsIgnored unit tests Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Evangelink · 2026-05-11T12:56:10Z

@copilot resolve the merge conflicts in this pull request

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-11T13:03:41Z

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The only conflict was in formal-verification/TARGETS.md — the merge keeps our 15-entry table (including the new UnitTestOutcomeHelper target at #8 and entries 9–15), incorporates main's status updates ("Informal spec extracted" wording, phase adjustments for targets 4 and 5), and retains our Findings table and Priority Order.

github-actions Bot and others added 5 commits May 11, 2026 10:40

Copilot AI review requested due to automatic review settings May 11, 2026 09:06

Copilot started reviewing on behalf of Evangelink May 11, 2026 09:07 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Helpers/AttributeHelpers.cs

Comment thread formal-verification/TARGETS.md Outdated

Comment thread docs/glossary.md

Evangelink commented May 11, 2026

View reviewed changes

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs Outdated

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Helpers/AttributeHelpers.cs Outdated

Evangelink commented May 11, 2026

View reviewed changes

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Helpers/AttributeHelpers.cs

Evangelink commented May 11, 2026

View reviewed changes

Copilot started work on behalf of Evangelink May 11, 2026 09:56 View session

Address review comments: fix null in satisfied group, improve comment…

8d5dc4c

…, add IsIgnored unit tests Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot finished work on behalf of Evangelink May 11, 2026 10:10

Copilot started work on behalf of Evangelink May 11, 2026 12:56 View session

Merge origin/main into PR branch, resolving TARGETS.md conflict

5f04eaf

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 11, 2026 13:03

Evangelink review requested due to automatic review settings May 11, 2026 13:03

Copilot finished work on behalf of Evangelink May 11, 2026 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Perf Improver] perf: eliminate LINQ allocations in IsIgnored hot path#8095

[Perf Improver] perf: eliminate LINQ allocations in IsIgnored hot path#8095
Evangelink wants to merge 7 commits into
mainfrom
perf-assist/isignored-linq-elimination-35ee59ba224761b9

Evangelink commented May 11, 2026 •

edited by Copilot AI

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Evangelink left a comment

Uh oh!

Uh oh!

Uh oh!

Evangelink left a comment

Uh oh!

Uh oh!

Evangelink left a comment

Uh oh!

Evangelink commented May 11, 2026

Uh oh!

Evangelink commented May 11, 2026

Uh oh!

Copilot AI commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Evangelink commented May 11, 2026 • edited by Copilot AI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Summary

Key Findings

Positive Highlights

Uh oh!

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Summary

Key Findings

Recommendations

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Summary

Analysis

AttributeHelpers.IsIgnored (main change)

TestMethodInfo.GetRetryAttribute

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync

Positive Observations

Conclusion

Uh oh!

Evangelink commented May 11, 2026

Uh oh!

Evangelink commented May 11, 2026

Uh oh!

Copilot AI commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Evangelink commented May 11, 2026 •

edited by Copilot AI

Loading

`AttributeHelpers.IsIgnored` (main change)

`TestMethodInfo.GetRetryAttribute`

`TestMethodRunner.TryExecuteDataSourceBasedTestsAsync`