Context
This is a deliberately-deferred design follow-up from the VSTest-object-model decoupling of MSTestAdapter.PlatformServices (the SettingsException neutralization in Phase 6e-4a).
Today the adapter has a two-tier settings-exception topology, which the decoupling work preserved byte-for-byte rather than changing:
AdapterSettingsException — thrown for invalid MSTest settings values (e.g. <Parallelize><Scope>/Workers). It is caught by MSTestDiscovererHelpers.InitializeDiscovery (which wraps MSTestSettings.PopulateSettings), which logs the error and returns false → discovery reports "no tests" / execution bails gracefully.InvalidRunSettingsException (introduced in 6e-4a as the neutral replacement for the VSTest SettingsException; derives directly from Exception) — thrown for structural runsettings errors (bad attribute, unexpected element, malformed <AssemblyResolution>, wrong root). It is not caught by that handler and escapes to the MSTestExecutor/MSTestDiscoverer boundary, propagating to the host — exactly as the VSTest SettingsException did historically.
The distinct exception (not derived from AdapterSettingsException) is intentional: it keeps where a settings-format error surfaces identical to the pre-decoupling behavior. An expert review during 6e-4a confirmed that reusing AdapterSettingsException for the structural-error sites would have made a malformed <AssemblyResolution> block newly caught (log + "no tests") instead of escaping — an observable behavior change.
The idea to consider (separately)
Unifying these two onto a single settings-exception model (so all settings-format errors are handled consistently — e.g. all caught + logged early, or all escaping) is arguably a cleaner design. But it is a deliberate behavior change, not a mechanical refactor, so it was kept out of the decoupling PRs.
If pursued, it needs its own review + reasoning:
- Decide the desired single behavior (caught-and-logged vs escape-to-host) for all settings-format errors.
- Update the throw sites (
RunSettingsUtilities, TestRunParameters, MSTestAdapterSettings) and the MSTestDiscovererHelpers.InitializeDiscovery catch accordingly.
- Update/repurpose the
…BailOutOnSettingsException tests, which currently pin the two-tier topology.
Scope
Design/behavior change only — no functional urgency. Filed so the thought isn't lost.
Context
This is a deliberately-deferred design follow-up from the VSTest-object-model decoupling of
MSTestAdapter.PlatformServices(theSettingsExceptionneutralization in Phase 6e-4a).Today the adapter has a two-tier settings-exception topology, which the decoupling work preserved byte-for-byte rather than changing:
AdapterSettingsException— thrown for invalid MSTest settings values (e.g.<Parallelize><Scope>/Workers). It is caught byMSTestDiscovererHelpers.InitializeDiscovery(which wrapsMSTestSettings.PopulateSettings), which logs the error and returnsfalse→ discovery reports "no tests" / execution bails gracefully.InvalidRunSettingsException(introduced in 6e-4a as the neutral replacement for the VSTestSettingsException; derives directly fromException) — thrown for structural runsettings errors (bad attribute, unexpected element, malformed<AssemblyResolution>, wrong root). It is not caught by that handler and escapes to theMSTestExecutor/MSTestDiscovererboundary, propagating to the host — exactly as the VSTestSettingsExceptiondid historically.The distinct exception (not derived from
AdapterSettingsException) is intentional: it keepswhere a settings-format error surfacesidentical to the pre-decoupling behavior. An expert review during 6e-4a confirmed that reusingAdapterSettingsExceptionfor the structural-error sites would have made a malformed<AssemblyResolution>block newly caught (log + "no tests") instead of escaping — an observable behavior change.The idea to consider (separately)
Unifying these two onto a single settings-exception model (so all settings-format errors are handled consistently — e.g. all caught + logged early, or all escaping) is arguably a cleaner design. But it is a deliberate behavior change, not a mechanical refactor, so it was kept out of the decoupling PRs.
If pursued, it needs its own review + reasoning:
RunSettingsUtilities,TestRunParameters,MSTestAdapterSettings) and theMSTestDiscovererHelpers.InitializeDiscoverycatch accordingly.…BailOutOnSettingsExceptiontests, which currently pin the two-tier topology.Scope
Design/behavior change only — no functional urgency. Filed so the thought isn't lost.