[Efficiency Improver] perf: avoid iterator allocation in GetRetryAttribute()

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Replace GetAttributes() yield-return iterator with
direct iteration over GetCustomAttributesCached() to eliminate one heap
allocation per test execution.

GetAttributes() is a yield-return method: every call allocates a
compiler-generated state machine object (~48 bytes). GetRetryAttribute()
is called from the TestMethodInfo constructor, which is created fresh
for every test execution. For a 10,000-test suite this avoids ~480 KB
of iterator state machine allocations, reducing GC pressure on the
common path where RetryAttribute is absent.

The new pattern is identical to GetFirstAttributeOrDefault() and
GetSingleAttributeOrDefault() in ReflectHelper, which already use
direct array iteration for the same reason.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Fixes#8040

[Copilot]

Pull request overview

Optimizes the MSTest adapter execution hot path by removing a per-test heap allocation when resolving RetryBaseAttribute on a test method, reducing GC pressure for large test suites.

Changes:

• Replaces enumeration via ReflectHelper.GetAttributes<RetryBaseAttribute>() (yield iterator) with direct iteration over GetCustomAttributesCached() to avoid iterator state-machine allocations.
• Preserves existing behavior: return the single RetryBaseAttribute if present, throw when multiple are present, return null when absent.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Reworks GetRetryAttribute() to scan the cached attribute array directly instead of using a yield-based iterator.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No issues found.

Correctness ✅ — The new code is semantically equivalent to the old code. GetAttributes<T>() internally delegates to GetCustomAttributesCached() and applies the same is TAttributeType filter via a yield return loop; the new code inlines that loop directly. All three outcomes (zero → null, one → return it, two+ → throw) are preserved identically.

Threading ✅ — GetCustomAttributesCached uses a ConcurrentDictionary with GetOrAdd, so the shared cache is accessed safely. No new mutable state is introduced.

Performance ✅ — The optimization is real and correctly motivated. The old using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator() call allocates a compiler-generated state machine object on every invocation (even when no RetryBaseAttribute is present, which is the common case). The new foreach over Attribute[] avoids that allocation entirely, consistent with the approach already used by GetFirstAttributeOrDefault<T> and GetSingleAttributeOrDefault<T>.

Resources ✅ — The using on the old IEnumerator<T> was required to correctly handle finally blocks inside the yield return iterator. The new loop iterates a plain Attribute[]; arrays do not implement IDisposable and have no finally-block semantics, so no cleanup is needed.

API Compat / Cross-TFM / Security / Defensive ✅ — Private method, no public surface area change. Only basic foreach and is pattern matching used — compatible with all target frameworks.

Positive Observations

The change follows the established pattern in ReflectHelper (GetFirstAttributeOrDefault, GetSingleAttributeOrDefault) and consolidates to a single, consistent idiom for attribute iteration throughout the hot path. The PR description accurately explains both the mechanism and the magnitude of the saving.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

• [Coverage] No unit tests cover GetRetryAttribute() — the refactored private method called from the TestMethodInfo constructor. Three paths exist (null, single attribute, multiple attributes/throw) and none are exercised by TestMethodInfoTests.cs or any other test file. This is a pre-existing gap, but the refactor is a natural opportunity to add coverage. See the inline comment for a concrete suggestion.

Recommendations

1. Add tests to TestMethodInfoTests.cs that create a TestMethodInfo wrapping a method with: (a) no RetryBaseAttribute, (b) one RetryBaseAttribute, (c) two RetryBaseAttribute attributes — asserting the RetryAttribute property value or expected exception respectively.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

✅ No significant nitpicks found. This is a well-crafted, surgical performance improvement.

Highlights:

• The new pattern is consistent with the already-established GetSingleAttributeOrDefault<T>() in ReflectHelper — same loop structure, same blank-line separation between the throw guard and the happy-path assignment.
• ThrowMultipleAttributesException is already annotated [DoesNotReturn], so the compiler correctly understands that found = retryAttribute is only reachable on the success path.
• The explanatory comment is well-placed and educational — it prevents future maintainers from "simplifying" back to the allocating iterator form.
• Variable names (found, attribute, retryAttribute) are clear and idiomatic for C# pattern-matching style.

Recommendations

None required. The implementation is idiomatic, correct, and consistent with existing codebase patterns.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍