Uh oh!
There was an error while loading. Please reload this page.
Split TestMethodRunner.cs into focused partial-class files - #9729
Conversation
Splits the 591-line TestMethodRunner.cs into four partial-class files following the ClassName.Responsibility.cs convention already used by neighboring classes in the Execution/ folder: - TestMethodRunner.cs: entry points and routing (ExecuteAsync, RunTestMethodAsync, IsDataDrivenTest, GetAggregateOutcome) - TestMethodRunner.DataSource.cs: DataSourceAttribute-based tests - TestMethodRunner.DataRow.cs: folded / DynamicData-style tests - TestMethodRunner.Execution.cs: core single-test execution Pure structural refactor with no behavioral changes. Fixes#9711 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors TestMethodRunner in MSTestAdapter.PlatformServices to match the existing ClassName.Responsibility.cs partial-class organization used by other classes in Execution/, improving navigability without changing behavior.
Changes:
- Converted
TestMethodRunnerto apartialtype and retained entry-point/routing logic inTestMethodRunner.cs. - Extracted data-driven execution paths into dedicated partial files (
.DataSource.csand.DataRow.cs). - Moved core single-test execution helpers into
TestMethodRunner.Execution.cs.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs | Marks the type partial and keeps the primary entry points/routing plus aggregate outcome logic. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.DataSource.cs | Contains DataSourceAttribute-based data-driven execution logic. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.DataRow.cs | Contains folded / ITestDataSource (DynamicData-style) execution logic. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.Execution.cs | Contains core single-test execution (ExecuteTestAsync) and result parent/execution id updates. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Clean structural refactor — no issues found. All 22 review dimensions assessed.
Verification
| Check | Result |
|---|---|
| All original methods accounted for | ✅ 11/11 methods preserved with identical signatures |
| File naming convention | ✅ Follows established ClassName.Responsibility.cs pattern (matches TestMethodInfo, TestExecutionManager, TypeCache, etc.) |
using directives per file | ✅ Properly trimmed — each partial imports only what it needs |
Removed using from main file | ✅ Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices correctly removed — no longer directly referenced |
partial keyword | ✅ Applied to class declaration in all 4 files |
| CI build (Linux Release) | ✅ Passed |
Dimension Assessment
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | N/A — no logic changes |
| 2 | Threading & Concurrency | N/A — ConfigureAwait(false) patterns preserved verbatim |
| 3 | Security & IPC | N/A |
| 4 | Public API | N/A — class is internal sealed |
| 5 | Performance | N/A — zero runtime impact |
| 6 | Cross-TFM | N/A — no TFM-specific changes |
| 7 | Resource & IDisposable | N/A — dispose patterns preserved |
| 8 | Defensive Coding | N/A |
| 9 | Localization | N/A |
| 10 | Test Isolation | N/A — no test files |
| 11 | Assertion Quality | N/A |
| 12 | Flakiness | N/A |
| 13 | Test Completeness | N/A |
| 14 | Data-Driven Coverage | N/A |
| 15 | Code Structure | LGTM — this PR improves structure |
| 16 | Naming | LGTM — consistent with neighbors |
| 17 | Documentation | LGTM — XML doc summaries on each partial describe responsibility |
| 18 | Analyzer Quality | N/A |
| 19 | IPC Wire | N/A |
| 20 | Build Infrastructure | N/A |
| 21 | Scope & PR Discipline | LGTM — single concern, well-scoped |
| 22 | PowerShell | N/A |
No blocking or major issues. Well-executed refactor that brings TestMethodRunner in line with the established partial-class conventions in its directory.
Uh oh!
There was an error while loading. Please reload this page.
Apply two project-convention improvements to files added in recent PRs: - MSTestTestNodeConverter.cs (added in #9706): replace Substring calls with range operators per csharp_style_prefer_range_operator = true - TestMethodRunner.DataSource.cs (added in #9729): replace == null with is null per the project's 'always use is null / is not null' rule No behavioral change; builds verified to succeed with 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Overview
Fixes#9711.
TestMethodRunner.cshad grown to 591 lines and was the only class insrc/Adapter/MSTestAdapter.PlatformServices/Execution/not following theClassName.Responsibility.cspartial-class split pattern used by its neighbors (TestMethodInfo,TestExecutionManager,TypeCache,TestClassInfo,UnitTestRunner).Changes
Split into four focused partial-class files:
TestMethodRunner.csExecuteAsync,RunTestMethodAsync,IsDataDrivenTest,GetAggregateOutcome— entry points & routingTestMethodRunner.DataSource.csDataSourceAttribute-based data-driven testsTestMethodRunner.DataRow.csDynamicData-style (ITestDataSource) data-driven testsTestMethodRunner.Execution.csExecuteTestAsync,UpdateResultsWithParentInfo)This is a pure structural refactor — no logic changes. Method bodies were moved verbatim; the class is marked
partial; usings were trimmed per-file.GetAggregateOutcomewas kept in the entry-point file since it's used by the routing logic inRunTestMethodAsync.Acceptance criteria
ClassName.Responsibility.csconventionMSTestAdapter.PlatformServices.UnitTestspass