Uh oh!
There was an error while loading. Please reload this page.
Neutralize run-settings input in PlatformServices host layer (Phase 6c2) - #9590
Conversation
Replace the VSTest IRunSettings with a neutral settings-XML string through the isolation-host layer, removing IRunSettings from MSTestAdapter.PlatformServices. - IPlatformServiceProvider.CreateTestSourceHost, TestSourceHost (both ctors) and AssemblyEnumeratorWrapper.GetTests/GetTestsInIsolation now take string? settingsXml. - TestExecutionManager.CacheSessionParameters takes the settings-XML string directly. - Callers extract runContext?.RunSettings?.SettingsXml / discoveryContext?.RunSettings?.SettingsXml at the point they already had the (still VSTest) run/discovery context; only .SettingsXml (a string) was ever read off IRunSettings, so this is byte-for-byte. The remaining IRunContext/IDiscoveryContext usage is the test-case filter (deferred to the filter sub-phase). No behavior change: the appdomain DisableAppDomain decision and the run-parameter caching read the same settings XML as before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0c0970d to
1a37ffaCompareThere was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review — Phase 6c2: Neutralize run-settings input in PlatformServices host layer
This is a clean, focused mechanical refactoring. All 22 review dimensions were evaluated; only one minor finding.
Verdict table
| # | Dimension | Result |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM |
| 2 | Threading & Concurrency | ✅ N/A |
| 3 | Security & IPC Contract Safety | ✅ LGTM |
| 4 | Public API & Binary Compatibility | ✅ LGTM — all changed interfaces/classes are internal; no public surface broken |
| 5 | Performance & Allocations | ✅ LGTM |
| 6 | Cross-TFM Compatibility | ✅ LGTM |
| 7 | Resource & IDisposable Management | ✅ LGTM — using ITestSourceHost pattern preserved |
| 8 | Defensive Coding at Boundaries | ✅ LGTM |
| 9 | Localization & Resources | ✅ N/A |
| 10 | Test Isolation | ✅ LGTM |
| 11 | Assertion Quality | ✅ LGTM — AwesomeAssertions used correctly per BannedSymbols.txt |
| 12 | Flakiness Patterns | ✅ N/A |
| 13 | Test Completeness & Coverage | ✅ LGTM — null and non-null settingsXml paths both covered |
| 14 | Data-Driven Test Coverage | ✅ N/A |
| 15 | Code Structure & Simplification | ✅ LGTM — is not null convention respected |
| 16 | Naming & Conventions | ✅ LGTM — settingsXml is unambiguous |
| 17 | Documentation Accuracy | |
| 18 | Analyzer Quality | ✅ N/A |
| 19 | IPC Wire Compatibility | ✅ N/A |
| 20 | Build Infrastructure & Dependencies | ✅ LGTM |
| 21 | Scope & PR Discipline | ✅ LGTM — single concern, mechanically consistent |
| 22 | PowerShell Scripting Hygiene | ✅ N/A |
Notes
Behavioral equivalence confirmed. The guard change runSettings != null && MSTestAdapterSettings.IsAppDomainCreationDisabled(runSettings.SettingsXml) → settingsXml is not null && MSTestAdapterSettings.IsAppDomainCreationDisabled(settingsXml) produces identical results for all realistic VSTest inputs. The one theoretical edge-case (non-null IRunSettings with a null SettingsXml but a non-null MSTestAdapterSettings.Configuration) is not a VSTest production scenario and the PR description explicitly acknowledges this. ✅
Test cleanup is correct. The integration tests (DesktopTestSourceHostTests.cs) now call LoadMSTestSettings before constructing TestSourceHost, preserving the original side-effect of initialising the MSTestSettingsProvider. The Moq mock layer was truly unnecessary after the extraction. ✅
Single NIT (inline): IPlatformServiceProvider.cs and PlatformServiceProvider.cs have their <param name="settingsXml"> description body still reading "The run Settings for the session." — compare with TestSourceHost.cs which was correctly updated to "The run-settings XML provided for this session." Trivial to fix in a follow-up or squash.
| /// </param> | ||
| /// <param name="runSettings"> | ||
| /// <param name="settingsXml"> | ||
| /// The run Settings for the session. |
There was a problem hiding this comment.
NIT — doc description not updated to match new parameter type
The <param name="settingsXml"> tag was renamed, but the description body still reads "The run Settings for the session.". Compare with TestSourceHost.cs where the same parameter already uses "The run-settings XML provided for this session."
The same applies to the copy in PlatformServiceProvider.cs (line 133).
/// <paramname="settingsXml">
-/// The run Settings for the session.
+/// The run-settings XML for the session.
/// </param>Co-authored-by: Amaury Leveque <amauryleve@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4eee0bf
into
dev/amauryleve/vstest-decoupling-baseUh oh!
There was an error while loading. Please reload this page.
Phase 6c2 of the PlatformServices platform-agnostic initiative
Part of removing the VSTest object model (
Microsoft.TestPlatform.ObjectModel) dependency fromMSTestAdapter.PlatformServices, moving VSTest coupling up intoMSTest.TestAdapter.This phase removes the VSTest
IRunSettingstype: it threads a neutral settings-XML string through the isolation-host layer instead. OnlyIRunSettings.SettingsXml(a string) was ever read across PlatformServices, so this is a mechanical, byte-for-byte substitution.What changed
IPlatformServiceProvider.CreateTestSourceHost(string, IRunSettings?)→(string, string? settingsXml).TestSourceHost(both ctors, incl. the netfx AppDomain path) andAssemblyEnumeratorWrapper.GetTests/GetTestsInIsolation→string? settingsXml.TestExecutionManager.CacheSessionParameterstakes the settings-XML string directly.runContext?.RunSettings?.SettingsXml/discoveryContext?.RunSettings?.SettingsXmlat the point they already held the (still-VSTest) run/discovery context.No behavior change
The AppDomain
DisableAppDomaindecision and run-parameter caching read the same settings XML as before. The one guard (runSettings != null && …→settingsXml is not null && …) is byte-for-byte for all realistic inputs and matches the existingDeploymentUtilityBaseprecedent (expert-reviewer confirmed the only theoretical divergence is unreachable).IRunSettingsis now fully gone from PlatformServices code (only a doc-comment mention remains). The remainingIRunContext/IDiscoveryContextusage is the test-case filter, deferred to the filter sub-phase.Verification
MSTestAdapter.PlatformServices.UnitTests: 897 (net8.0) / 935 (net462);MSTest.IntegrationTests: 47 pass / 1 skip;PlatformServices.Desktop.IntegrationTests: 15 pass (exercises the AppDomain assembly-resolution-from-runsettings path — validates the settings-xml threads correctly through the child domain).Base
dev/amauryleve/vstest-decoupling-base, which already contains Phase 3 (Abstract VSTest discovery sink in PlatformServices (Phase 3 of platform-agnostic effort) #9566), 4 (Abstract VSTest execution input in PlatformServices (Phase 4 of platform-agnostic effort) #9572), 6a (Neutralize deployment input in PlatformServices (Phase 6a of platform-agnostic effort) #9576), 6b (Neutralize test result recording in PlatformServices (Phase 6b) #9579), and 6c (Neutralize test message logging in PlatformServices execution engine (Phase 6c) #9585). Developed stacked on 6c; 6c has since merged into the base, so the diff here is 6c2 only (11 files).Remaining work
IRunContext/IDiscoveryContext/ITestCaseFilterExpression→ boundary; folds in Avoid double TestCase construction when filtering during discovery/execution (platform-agnostic refactor follow-up) #9568).UnitTestElement↔TestCaseconversion +EngineConstantsVSTest properties.Microsoft.TestPlatform.ObjectModelPackageReference + guard test.