Neutralize the runsettings-parsing helpers (Phase 6e-4a)

[Evangelink]

Phase 6e-4a — Neutralize the runsettings-parsing helpers

Part of the initiative to make MSTestAdapter.PlatformServices platform-agnostic by removing its dependency on the VSTest object model (Microsoft.TestPlatform.ObjectModel). Strict byte-for-byte refactor.

What this does

Decouples the four runsettings-XML parsing files (RunSettingsUtilities, TestRunParameters, MSTestAdapterSettings, MSTestSettings.RunSettingsXml):

1. SettingsException → new neutral InvalidRunSettingsException (derives directly from Exception, not from AdapterSettingsException).
2. Inline the ObjectModel.Constants node names ("RunConfiguration", "TestRunParameters").
3. Repoint XmlRunSettingsUtilities.ReaderSettings at the neutral RunSettingsUtilities.ReaderSettings.
4. New neutral XmlReaderUtilities (ReadToRootNode + ReadToNextElement/SkipToNextElement), reusing the exact navigation semantics the adapter already vendored privately in RunConfigurationSettings.

Coupling drops 10 → 6 files.

Fidelity — exception-handler trace (the proof)

The adapter has a two-tier settings-exception topology this change preserves. MSTestDiscovererHelpers.InitializeDiscovery catches AdapterSettingsException (invalid MSTest settings values → log + "no tests"); a structural runsettings error escapes that handler to the host. Per-site — which handler catches it, before vs after:

Throw site	Sole reachable path	As VSTest SettingsException (today)	As InvalidRunSettingsException (after)
RunSettingsUtilities.ThrowOnHasAttributes	TestRunParameters.FromXml → GetTestRunParameters → TestExecutionManager.CacheSessionParameters	broad catch (Exception)	same broad catch (Exception)
TestRunParameters.FromXml	GetTestRunParameters → CacheSessionParameters	broad catch (Exception)	same broad catch (Exception)
MSTestAdapterSettings.ToSettings (AssemblyResolution)	SettingsProvider.Load (from PopulateSettings, wrapped by InitializeDiscovery's catch (AdapterSettingsException))	escapes that catch → DiscoverTests/RunTests (all try/finally, no catch) → host	escapes (distinct type) → host

A distinct type (not AdapterSettingsException) is required for site 3: reusing AdapterSettingsException would make a malformed <AssemblyResolution> newly caught at InitializeDiscovery (log + "no tests") instead of escaping — an observable behavior change.

Cross-process note (site 3 escapes to the host). Perfect type-and-stack fidelity on this path is not achievable while decoupling, because SettingsException is itself an object-model type and must leave PlatformServices before the package-reference drop. The neutral InvalidRunSettingsException preserves the message, the throw origin/stack, and the escape topology (still escapes InitializeDiscovery and DiscoverTests/RunTests exactly as before); only the exception type name differs, and only on a rare, user-authored malformed-<AssemblyResolution> runsettings that no test exercises. The test platform surfaces adapter-thrown exceptions during Discover/RunTests uniformly as TestMessageLevel.Error (its runsettings-validation special-casing lives in the host's own session-start parsing, not adapter invocation), so this is behavior-neutral on observable output. Adding a dedicated cross-process TRX/error-parity regression test is tracked by #9587. Unifying the two exceptions onto one model is a separate deliberate behavior change, tracked in #9629.

MSTestExecutor/MSTestDiscoverer have no typed settings-exception catch, so there is no boundary catch to update. The …BailOutOnSettingsException tests (which exercise the AdapterSettingsException path) pass unchanged.

Verification

• Full build.cmd -c Debug green all TFMs, IDE0005 clean.
• MSTestAdapter.PlatformServices.UnitTests: 897 (net8.0) / 935 (net462), 0 failed.
• MSTestAdapter.UnitTests: 21, 0 failed (incl. BailOut tests).
• MSTest.IntegrationTests (cross-proc, net462): 48 total, 0 failed, 1 skipped (pre-existing known-flaky).

Stacking

6c2 #9590 → 6d-1 #9591 → 6d-2 #9621 → 6e-1 #9622 → 6e-2 #9623 → 6e-3a #9624 → 6e-3b #9626 → 6e-4a (this). Base = 6e-3b branch. Do not rebase/reset the base.

[github-actions]

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.

---

Expert Review — Phase 6e-4a: Neutralize the runsettings-parsing helpers

Verdict: ✅ Informational / COMMENT (PR already merged; no blocking issues found.)

---

22-Dimension Verdict Table

#	Dimension	Severity	Verdict	Notes
1	Algorithmic Correctness	MAJOR	✅ PASS	ReadToNextElement/SkipToNextElement/ReadToRootNode semantics byte-for-byte match the RunConfigurationSettings private copies they were modelled on.
2	Threading & Concurrency	BLOCKING	✅ N/A	Pure synchronous XML parsing, no shared mutable state.
3	Security & IPC Contract Safety	BLOCKING	✅ PASS	XmlReaderSettings default in .NET 4.5+ is DtdProcessing.Prohibit; not setting it explicitly is safe. MSTestAdapterSettings.IsAppDomainCreationDisabled keeps its XmlResolver = null.
4	Public API & Binary Compatibility	BLOCKING	✅ PASS	Both new types (InvalidRunSettingsException, XmlReaderUtilities) are internal. No PublicAPI.Unshipped.txt changes needed.
5	Performance & Allocations	MAJOR	✅ PASS	GetObjectModelAssembly() scans all loaded assemblies, but it sits on the low-frequency AppDomain setup path (#if NETFRAMEWORK). Acceptable.
6	Cross-TFM Compatibility	MAJOR	✅ PASS	All AppDomainUtilities changes are properly gated behind #if NETFRAMEWORK. The rest of the diff is TFM-neutral.
7	Exception Handling	MAJOR	✅ PASS	The two-tier exception topology is preserved. InvalidRunSettingsException intentionally does not derive from AdapterSettingsException; the PR description includes a per-site proof table that is correct and thorough.
8	Test Coverage & Quality	MAJOR	⚠️ SEE BELOW	The diff shows an unchanged context line still asserting Throw<SettingsException>() after the ObjectModel using was removed. Disk state is clean. See inline comment.
9	Naming & Style	MINOR	✅ PASS	InvalidRunSettingsException, XmlReaderUtilities, RunConfigurationSettingsName, TestRunParametersName are all clear and consistent.
10	Code Duplication	MINOR	⚠️ NOTE	RunConfigurationSettings retains private ReadToRootNode/ReadToNextElement/SkipToNextElement clones that now mirror the new public XmlReaderUtilities methods. Consolidating would change the exception type on the RunConfiguration path (FormatException → InvalidRunSettingsException). Leaving it as-is in this phase is intentional and correct; a later phase can align.
11	Documentation	MINOR	✅ PASS	Both new types carry XML docs. The InvalidRunSettingsException doc clearly explains the two-tier design rationale.
12	Resource Management	MAJOR	✅ N/A	No new IDisposable resources; all existing XmlReader/StringReader uses already have using in their callers.
13	Null Safety	MAJOR	✅ PASS	InvalidRunSettingsException(string? message) is null-safe. GetObjectModelAssembly() either succeeds or throws — never returns null.
14	Pattern Matching	MINOR	✅ N/A	No opportunities in the changed code.
15	LINQ Usage	MINOR	✅ PASS	FirstOrDefault(predicate) in GetObjectModelAssembly() is appropriate.
16	Localization	MAJOR	⚠️ MINOR	XmlReaderUtilities.ReadToRootNode uses a hardcoded interpolated string instead of a Resource.* entry — inconsistent with the rest of the settings-error surface. See inline comment.
17	Build & Project Files	MAJOR	✅ PASS	SDK-style project (MSTestAdapter.PlatformServices.csproj) auto-includes all *.cs files; no explicit <Compile> entries needed for the two new files.
18	PR Scope & Focus	MAJOR	✅ PASS	Strictly scoped to the four runsettings-parsing files. The PR description is exemplary in its fidelity analysis.
19	Backward Compatibility	BLOCKING	✅ PASS	The per-site exception-handler table in the PR description demonstrates that observable behaviour is unchanged. Site 3 (MSTestAdapterSettings.ReadAssemblyResolutionPath) correctly escapes InitializeDiscovery both before and after.
20	Error Message Quality	MINOR	✅ PASS	All messages are actionable. The hardcoded XmlReaderUtilities message is clear even if not resource-backed (see dim 16).
21	Design & Architecture	MAJOR	✅ PASS	Extension methods on XmlReader is idiomatic; the GetObjectModelAssembly() late-binding approach avoids a hard compile-time dependency on a type that is scheduled to be removed.
22	Overarching Principles	—	✅ PASS	Backward compatibility is sacred and demonstrably preserved. Scope is disciplined (one phase, one responsibility). No new public API.

---

Key Findings

⚠️ Test assertion not updated (dim 8, dim 16)

The diff shows a context (unchanged) line inside GetTestRunParametersThrowsOnInvalidSettingsXml that still asserts .Throw<SettingsException>() after the using Microsoft.VisualStudio.TestPlatform.ObjectModel import was removed. That would be a compilation error. The current disk state is clean (no SettingsException reference), which implies the base branch (6e-3b) already had this method restructured and the git diff's "nearest function" context header is misleading. No action required, but worth an explicit regression note: if the 6e-3b base is ever rebased, this context line should not re-appear.

i️ Hardcoded error message (dim 16)

XmlReaderUtilities.ReadToRootNode throws $"Could not find '...' node..." as an interpolated string. Every peer error in this area (ThrowOnHasAttributes, FromXml, ReadAssemblyResolutionPath) uses Resource.* strings. Low priority, but a resource entry would keep this consistent for any future localization pass.

i️ RunConfigurationSettings duplication (dim 10)

RunConfigurationSettings still carries three private helpers (ReadToRootNode/ReadToNextElement/SkipToNextElement) that are now semantically identical to XmlReaderUtilities. The only observable difference is that RunConfigurationSettings.ReadToRootNode throws FormatException while XmlReaderUtilities.ReadToRootNode throws InvalidRunSettingsException. Consolidating them is a deliberate future-phase decision and is tracked correctly.

---

Overall: Well-executed, carefully analysed refactoring. The exception-topology proof table in the PR description sets a high bar for this kind of decoupling work. The two non-blocking observations above are worth noting but do not affect correctness or behaviour.

Comments that could not be inline-anchored

test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Helpers/RunSettingsUtilitiesTests.cs:33

The diff shows this assertion as an unchanged context line (no - prefix) while the using Microsoft.VisualStudio.TestPlatform.ObjectModel import was removed in the same hunk. As written after the patch, SettingsException is no longer resolvable — this should be a compilation error.

The disk state does not exhibit the problem (no SettingsException reference remains, and the method was restructured), which suggests it was resolved either within the same commit or via a prior phase…

src/Adapter/MSTestAdapter.PlatformServices/Helpers/XmlReaderUtilities.cs:29

The error message is a hardcoded interpolated string. Every other settings-parse error in this codebase flows through Resource.* strings (e.g., Resource.InvalidSettingsXmlElement, Resource.InvalidSettingsXmlAttribute). While this path is rare and the message itself is perfectly clear, it is inconsistent with the localization policy of the codebase.

Suggested fix: add a resource entry (e.g. InvalidRunSettingsRootNode) and use `string.Format(CultureInfo.CurrentCulture, Resource.InvalidRu…