[Efficiency Improver] perf: eliminate yield-iterator allocations in test execution hot path

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.

[Amaury Levé (Evangelink)]

✨ Enhancement

What does this improve?
This PR reduces allocations in MSTest adapter test-execution hot paths while preserving behavior:

• RetryBaseAttribute discovery now iterates cached method attributes directly instead of using a yield iterator path.
• DataSourceAttribute handling now uses a direct cached-attribute scan that remains allocation-free and preserves prior cardinality semantics (execute only when exactly one DataSourceAttribute is present).

Why is this valuable?
These paths run during test execution, so avoiding iterator/array allocations reduces overhead in common runtime scenarios without changing intended outcomes.

Implementation approach:

• Replaced allocation-heavy attribute enumeration patterns with direct iteration over ReflectHelper.Instance.GetCustomAttributesCached(...).
• Kept/clarified guard behavior for multiple retry attributes.
• Added unit-test coverage for the multiple-RetryBaseAttribute error path (ThrowMultipleAttributesException) to lock in behavior.
• Applied minor naming/comment clarity improvements related to the refactor.

Testing

• Added targeted unit coverage for the retry multi-attribute exception path.
• Targeted adapter unit test runs were attempted, but restore/test execution is currently blocked in this environment by transient package feed download failures.

[Copilot]

Pull request overview

This PR targets runtime efficiency in the MSTest adapter execution hot path by removing allocation-heavy iterator/array creation when checking for certain attributes, and also expands the project glossary with several new terms.

Changes:

• Updated test execution to avoid allocating DataSourceAttribute[] in the common “no DataSource” path by switching to an allocation-free attribute presence check.
• Updated retry-attribute discovery to iterate the cached Attribute[] directly instead of using a yield-based iterator.
• Expanded docs/glossary.md with new glossary entries (e.g., AwesomeAssertions, DynamicData, PropertyBag, TestNode, FV correspondence terms).

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs	Removes per-test allocations by using IsAttributeDefined<DataSourceAttribute> in the DataSource execution decision.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Removes iterator allocations by scanning cached attributes directly to find a single RetryBaseAttribute.
docs/glossary.md	Adds/updates glossary entries for testing libraries and platform concepts.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

This PR contains no test file changes. The two production-code changes are allocation-free refactorings in the test execution hot path:

1. TestMethodInfo.GetRetryAttribute() — replaces a yield-iterator enumeration with a direct foreach over the cached Attribute[]. Semantically equivalent, but the "throw when multiple RetryBaseAttribute are present" guard is not covered by any existing test (confirmed via grep of TestMethodInfoTests.cs and ThrowMultipleAttributesException).

2. TestMethodRunner.TryExecuteDataSourceBasedTestsAsync() — replaces GetAttributes<DataSourceAttribute>()[Length == 1] with IsAttributeDefined<DataSourceAttribute>(). Safe because DataSourceAttribute has AllowMultiple = false by default; the subtle semantic shift from "exactly one" to "any" is therefore unreachable in practice.

Recommendations

• Add a unit test in TestMethodInfoTests.cs verifying that applying two RetryBaseAttribute subclass instances to a method causes the expected exception. This would pin the refactored iteration logic against regression. (See inline comment.)

---

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

The two code changes are well-motivated and the logic is correct. Two minor comment/naming issues are worth a quick pass:

1. TestMethodRunner.cs L182 – comment precision: The phrase "sealed and does not allow multiple" conflates sealed (prevents subclassing) with AllowMultiple = false (prevents multiple attribute instances). Both properties matter but for different reasons, and the current wording could mislead a reader about which one controls what.

2. TestMethodInfo.cs L293 – variable name leaks implementation detail: cachedAttributes mirrors the helper method name's caching detail. A neutral name (methodAttributes) would be more resilient to future renames of the underlying API.

Positive Highlights

• The guard-clause inversion in TryExecuteDataSourceBasedTestsAsync is a clear improvement in readability.
• Both explanatory comments accurately describe why the allocation is avoided — helpful for future maintainers.
• The glossary additions are thorough and well-written.

Recommendations

• Clarify the comment on L182 to separate the roles of sealed and AttributeUsage.AllowMultiple = false.
• Rename cachedAttributes → methodAttributes (or attributes) to avoid coupling the local name to the helper's implementation detail.

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Amaury Levé (Evangelink)]

Summary

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

Key Findings

No correctness, threading, security, or API-compatibility issues found. The two code changes are semantically equivalent to what they replace — the optimizations are sound.

TestMethodInfo.GetRetryAttribute — replaces a yield-iterator chain (GetAttributes<RetryBaseAttribute> → GetEnumerator) with a direct foreach over the already-cached Attribute[]. The filtering (is RetryBaseAttribute), the dual-match detection (ThrowMultipleAttributesException, [DoesNotReturn]), and the null-return for no match are all preserved. Because ThrowMultipleAttributesException never returns, the loop exits on the second match exactly as the old enumerator code did.

TestMethodRunner.TryExecuteDataSourceBasedTestsAsync — replaces GetAttributes<DataSourceAttribute>() (yield iterator + spread-to-array) and a { Length: 1 } check with IsAttributeDefined<DataSourceAttribute>. The comment's claim "presence == exactly one" is verified: DataSourceAttribute is sealed and [AttributeUsage(Inherited = false)] with AllowMultiple = false, so it is physically impossible to have more than one instance in the cached array.

Both paths ultimately route through ReflectHelper.Instance.GetCustomAttributesCached(MethodInfo), so there is no change in which attributes are seen or how inheritance is handled.

Recommendations

None required — the changes are correct and the allocation savings are genuine hot-path wins (these methods run for every test execution).

---

Generated by Expert Code Reviewer 🧠 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Amaury Levé (Evangelink)]

Copilot address review comments

[Copilot]

Copilot address review comments

Done in ee2c9f5. I addressed the actionable review items (coverage test for multiple RetryBaseAttribute, DataSource comment wording, and local variable naming).

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[Amaury Levé (Evangelink)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved in 6d3d58a by merging the latest origin/main into this branch and fixing the conflict in TestMethodRunner.cs while preserving the reviewed DataSourceAttribute cardinality behavior.