Optimize TestMethodRunner: fix double enumeration, reduce allocations by Evangelink · Pull Request #7541 · microsoft/testfx

Evangelink · 2026-03-12T21:09:07Z

Fix double enumeration in TryExecuteFoldedDataDrivenTestsAsync by materializing GetData() into an IReadOnlyList once instead of calling .Any() then re-enumerating
Remove unnecessary List allocation in UpdateResultsWithParentInfo by mutating in place and returning void
Cache result[result.Length - 1] into a local in ExecuteAsync catch block
Skip redundant first-element self-comparison in GetAggregateOutcome
Use Stopwatch.StartNew() in ExecuteTestFromDataSourceAttributeAsync

- Fix double enumeration in TryExecuteFoldedDataDrivenTestsAsync by materializing GetData() into an IReadOnlyList once instead of calling .Any() then re-enumerating - Remove unnecessary List allocation in UpdateResultsWithParentInfo by mutating in place and returning void - Cache result[result.Length - 1] into a local in ExecuteAsync catch block - Skip redundant first-element self-comparison in GetAggregateOutcome - Use Stopwatch.StartNew() in ExecuteTestFromDataSourceAttributeAsync

- Hoist loop-invariant 'testContext as TestContextImplementation' cast above the foreach loop in RunAssemblyCleanupAsync - Remove unnecessary nested scope in IsTestMethodRunnable - Use 'is null' pattern instead of '== null' - Simplify ignore message logic by inlining into the conditional block

Copilot

Pull request overview

This PR optimizes TestMethodRunner execution paths in the MSTest adapter by reducing redundant work (enumerations/allocations) during data-driven execution and result aggregation.

Changes:

Materialize folded data-driven GetData() results once to avoid double enumeration and enable Count checks.
Remove an unnecessary list allocation by mutating results in place in UpdateResultsWithParentInfo.
Minor micro-optimizations (cache last result in exception path, skip redundant first element in aggregate outcome, use Stopwatch.StartNew()).

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-12T21:11:37Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

+            IReadOnlyList<object?[]> dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo) is IReadOnlyList<object?[]> dataList
+                ? dataList
+                : testDataSource.GetData(_testMethodInfo.MethodInfo).ToList();


testDataSource.GetData(...) is invoked twice here (once for the is IReadOnlyList check and again for .ToList()), which can re-run expensive logic and can change behavior if GetData is not idempotent. Store the GetData(...) result in a local once, then either cast to IReadOnlyList<object?[]> or materialize it.

Suggested change

IReadOnlyList<object?[]> dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo) is IReadOnlyList<object?[]> dataList

? dataList

: testDataSource.GetData(_testMethodInfo.MethodInfo).ToList();

var data = testDataSource.GetData(_testMethodInfo.MethodInfo);

IReadOnlyList<object?[]> dataSource = data is IReadOnlyList<object?[]> dataList

? dataList

: data.ToList();

Copilot · 2026-03-12T21:11:38Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

    /// <summary>
    /// Updates given results with parent info if results are greater than 1.
    /// Add parent results as first result in updated result.
    /// </summary>


The XML doc summary for UpdateResultsWithParentInfo is now misleading: it says it only updates results when count > 1 and that it "Add parent results as first result", but the implementation always iterates all results and does not add/reorder anything. Please update the summary to match the current behavior (or adjust the implementation if the comment describes intended behavior).

See below for a potential fix:

/// Updates each given result with new execution and parent execution identifiers.

Youssef1313 · 2026-03-12T21:35:14Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

        }
        catch (Exception ex)
        {
            if (result == null || result.Length == 0)


In case of exception, I would expect result to always be null here. Looks like we have some dead code here and we can simplify to just create a single TestResult right away, unless I'm missing something very obvious. Cleaning up in #7546

Youssef1313 · 2026-03-12T21:36:32Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

            // This code is to execute tests. To discover the tests code is in AssemblyEnumerator.ProcessTestDataSourceTests.
            // Any change made here should be reflected in AssemblyEnumerator.ProcessTestDataSourceTests as well.
-            dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo);
+            IReadOnlyList<object?[]> dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo) is IReadOnlyList<object?[]> dataList


Aside from double calling GetData, I'm not really sure if this is more optimized or if it's worse.

What I probably would do is call GetData normally, without materializing to a list, but still enumerate once by keeping a bool like dataSourceHasData = false before the loop, and set it to true inside the loop. After the loop, if it remained false, we know we didn't have data. That way, we didn't need to materialize to a list, and we didn't double enumerate.

Youssef1313 · 2026-03-12T21:40:52Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

        // Get aggregate outcome.
        UnitTestOutcome aggregateOutcome = results[0].Outcome;
-        foreach (TestResult result in results)
+        for (int i = 1; i < results.Count; i++)


This should have roughly the same perf as before IMO, but I'm not opposed to the change.

Youssef1313 · 2026-03-12T21:46:23Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs

+        var testContextImpl = testContext as TestContextImplementation;
        IEnumerable<TestAssemblyInfo> assemblyInfoCache = typeCache.AssemblyInfoListWithExecutableCleanupMethods;


assemblyInfoCache is always a single-element today AFAIK. This is not saving anything IMO, but I don't mind the change.

Youssef1313 · 2026-03-12T21:51:20Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs

-
        if (shouldIgnoreClass || shouldIgnoreMethod)
        {
+            string? ignoreMessage = StringEx.IsNullOrEmpty(ignoreMessageOnClass) ? ignoreMessageOnMethod : ignoreMessageOnClass;


There is a slight behavior change here.

If class is ignored with empty message, and method is not ignored.

Previous behavior: we create ignore result with empty message.

New behavior: we create ignore result with null message.

I have no idea if this will have any user-visible impact or not, but it's probably safer to keep the old behavior.

Suggested change

string? ignoreMessage = StringEx.IsNullOrEmpty(ignoreMessageOnClass) ? ignoreMessageOnMethod : ignoreMessageOnClass;

string? ignoreMessage = shouldIgnoreMethod && StringEx.IsNullOrEmpty(ignoreMessageOnClass) ? ignoreMessageOnMethod : ignoreMessageOnClass;

Youssef1313 · 2026-03-13T06:44:49Z

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs

            }

 #pragma warning disable IDE0056 // Use index operator
+            TestResult lastResult = result[result.Length - 1];


Eliminating this part completely in #7546

Copilot AI review requested due to automatic review settings March 12, 2026 21:09

Evangelink enabled auto-merge March 12, 2026 21:09

Copilot started reviewing on behalf of Evangelink March 12, 2026 21:10 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Youssef1313 reviewed Mar 12, 2026

View reviewed changes

Youssef1313 reviewed Mar 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize TestMethodRunner: fix double enumeration, reduce allocations#7541

Optimize TestMethodRunner: fix double enumeration, reduce allocations#7541
Evangelink wants to merge 2 commits intomainfrom
optimize-test-method-runner

Evangelink commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Youssef1313 Mar 12, 2026 •

edited

Loading

Uh oh!

Youssef1313 Mar 12, 2026

Uh oh!

Youssef1313 Mar 12, 2026

Uh oh!

Youssef1313 Mar 12, 2026 •

edited

Loading

Uh oh!

Youssef1313 Mar 12, 2026

Uh oh!

Youssef1313 Mar 12, 2026

Uh oh!

Youssef1313 Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		var testContextImpl = testContext as TestContextImplementation;
		IEnumerable<TestAssemblyInfo> assemblyInfoCache = typeCache.AssemblyInfoListWithExecutableCleanupMethods;

	string? ignoreMessage = StringEx.IsNullOrEmpty(ignoreMessageOnClass) ? ignoreMessageOnMethod : ignoreMessageOnClass;
	string? ignoreMessage = shouldIgnoreMethod && StringEx.IsNullOrEmpty(ignoreMessageOnClass) ? ignoreMessageOnMethod : ignoreMessageOnClass;

Conversation

Evangelink commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Youssef1313 Mar 12, 2026 •

edited

Loading

Youssef1313 Mar 12, 2026 •

edited

Loading