Fix #9762: share RunSettings/TestRunParameters provider logic between MSTest adapter and VSTestBridge via dependency-free linked source#9775
Conversation
…ked source Extract the resource-independent logic duplicated between the MSTest adapter's native Microsoft.Testing.Platform providers and the VSTestBridge providers into a new dependency-free linked shared file (src/Platform/SharedExtensionHelpers/RunSettingsProviderHelper.cs), linked into both projects. This removes the near-identical copies without reintroducing a dependency on Microsoft.Testing.Extensions.VSTestBridge. - CanReadFile, TryLoadRunSettingsAsync, HasEnvironmentVariables, ApplyEnvironmentVariables and FindInvalidTestParameter now live once. - Unifies the string.IsNullOrEmpty vs RoslynString.IsNullOrEmpty drift on string.IsNullOrEmpty. - Each package keeps its own resource strings, option registration and UWP guards; the shared type is fully guarded by #if !WINDOWS_UWP. - No public/behavioral change: option names, descriptions, arities and validation messages are unchanged. TestCaseFilter pair left untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR resolves issue #9762 (duplicate-code) by de-duplicating the RunSettings / TestRunParameters provider logic that the MSTest adapter's native Microsoft.Testing.Platform (MTP) providers had copied from the VSTestBridge. Rather than re-introducing a dependency on Microsoft.Testing.Extensions.VSTestBridge (which the RFC-018 native-MTP work #9706/#9743/#9748 deliberately removed), it follows the issue's recommended dependency-free approach: a single internal static helper in src/Platform/SharedExtensionHelpers that both projects compile via linked Compile items, exactly like the existing shared helpers in that folder. Each package keeps its own resource strings, option registration, and UWP guards.
Changes:
- Adds
RunSettingsProviderHelper(namespaceMicrosoft.Testing.Extensions,#if !WINDOWS_UWP) holding the resource-independent logic:CanReadFile,TryLoadRunSettingsAsync,HasEnvironmentVariables,ApplyEnvironmentVariables,FindInvalidTestParameter. - Links the shared file into both
Microsoft.Testing.Extensions.VSTestBridge.csproj(asHelpers\…) andMSTest.TestAdapter.csproj(asShared\…) with no new project reference. - Refactors the two RunSettings CLI providers, two env-var providers, and two TestRunParameters providers to call the shared helper; unifies the previously drifted
string.IsNullOrEmptyvsRoslynString.IsNullOrEmptyonstring.IsNullOrEmpty(removing the now-unusedusing Microsoft.Testing.Platform;in the bridge env-var provider).
I verified the behavior is preserved (identical resolution ordering, NETCOREAPP/non-core branch, and env-var enablement gate), all types resolve via repo-wide global usings and enclosing-namespace lookup, and netstandard2.0 string.Contains(char) is satisfied by the linked Polyfills. No concrete defects were found.
Show a summary per file
| File | Description |
|---|---|
src/Platform/SharedExtensionHelpers/RunSettingsProviderHelper.cs |
New shared helper with the extracted, resource-independent provider logic. |
src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Microsoft.Testing.Extensions.VSTestBridge.csproj |
Links the shared helper into the bridge project. |
src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj |
Links the shared helper into the adapter project. |
src/Platform/Microsoft.Testing.Extensions.VSTestBridge/TestHostControllers/RunSettingsEnvironmentVariableProvider.cs |
Delegates runsettings load + env-var handling to the shared helper; drops unused using. |
src/Platform/Microsoft.Testing.Extensions.VSTestBridge/CommandLine/RunSettingsCommandLineOptionsProvider.cs |
Uses shared CanReadFile; removes the private copy. |
src/Platform/Microsoft.Testing.Extensions.VSTestBridge/CommandLine/TestRunParametersCommandLineOptionsProvider.cs |
Uses shared FindInvalidTestParameter. |
src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestRunSettingsEnvironmentVariableProvider.cs |
Delegates to shared helper; drops unused IFileStream alias. |
src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestRunSettingsCommandLineOptionsProvider.cs |
Uses shared CanReadFile; removes the private copy. |
src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestRunParametersCommandLineOptionsProvider.cs |
Uses shared FindInvalidTestParameter. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Medium
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.
Summary
Clean deduplication PR. Reviewed across correctness, resource management, threading, cross-TFM compatibility, and MSBuild authoring dimensions.
No blocking issues found. One low-severity suggestion posted inline about broadening the exception catch in CanReadFile to handle UnauthorizedAccessException (preserves existing behavior, so non-blocking).
Verified clean:
- Namespace visibility:
Microsoft.Testing.Extensionsis reachable fromMicrosoft.Testing.Extensions.VSTestBridge.*via C# parent-namespace resolution; adapter files correctly add the explicitusing. - Null safety: All
!operators are guarded by prior checks or call-site preconditions (HasEnvironmentVariables→ApplyEnvironmentVariables). - Thread safety:
_runSettingsassignment/read follows MTP's guaranteed sequentialIsEnabledAsync→UpdateAsynccall order. - Cross-TFM:
#if !WINDOWS_UWPand#if NETCOREAPPguards are correctly placed. - Implicit usings:
Directory.Build.propsprovidesSystem.Xml.Linq,System.Diagnostics.CodeAnalysis, etc. — no fragile assumptions. - MSBuild linking:
<Compile Include="..." Link="..." />follows the establishedSharedExtensionHelperspattern with clear issue-referencing comments. - Drift resolution: Unification on
string.IsNullOrEmptyoverRoslynString.IsNullOrEmptyis a sound simplification for the shared helper.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 217 AIC · ⌖ 9.54 AIC · ⊞ 7.3K · ◷
- Track RunSettingsProviderHelper in InternalAPI.Unshipped.txt for both the VSTestBridge and MSTest.TestAdapter projects (RS0051). Both projects use InternalsVisibleTo, so the internal-API analyzer tracks internal symbols; entries go in InternalAPI.Unshipped.txt, not PublicAPI.Unshipped.txt. - Also add the pre-existing untracked PlatformServicesConfigurationAdapter (non-UWP) to the adapter InternalAPI.Unshipped.txt to unblock RS0051. - Broaden CanReadFile catch to also handle UnauthorizedAccessException per review, so a permission error yields the friendly 'cannot be read' validation message instead of an unhandled exception. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the review feedback and the red pipeline: RS0051 build failure (fixed). The failure is the InternalAPI analyzer (declared-API file
Review nit (applied). Merged current Heads-up (out of scope, main-side): the |
…RS0051 Microsoft.Testing.Platform's BaseSerializer.ReadFields and WriteListPayload<T> (added by #9774) were never added to InternalAPI.Unshipped.txt, so once #9752 enabled RS0051 internal-API enforcement both landed on main and left main red. This foundational project's failure cascades and blocks the whole build, so track the two methods in the base InternalAPI.Unshipped.txt (the diagnostic fires on netstandard2.0 too, so it belongs in the base file, not net/). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🔍 Build Failure AnalysisSummary — All 25 build errors are Root cause:
|
| Project | InternalAPI file needing update |
|---|---|
Microsoft.Testing.Extensions.HangDump |
src/Platform/Microsoft.Testing.Extensions.HangDump/InternalAPI/InternalAPI.Unshipped.txt |
Microsoft.Testing.Extensions.MSBuild |
src/Platform/Microsoft.Testing.Extensions.MSBuild/InternalAPI.Unshipped.txt |
Microsoft.Testing.Extensions.Retry |
src/Platform/Microsoft.Testing.Extensions.Retry/InternalAPI/InternalAPI.Unshipped.txt |
Microsoft.Testing.Extensions.TrxReport |
src/Platform/Microsoft.Testing.Extensions.TrxReport/InternalAPI/InternalAPI.Unshipped.txt |
Proposed fix — Append the same two lines to each of the 4 files above:
static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.ReadFields(System.IO.Stream! stream, System.Func<ushort, int, bool>! tryReadField) -> void
static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.WriteListPayload<T>(System.IO.Stream! stream, ushort fieldId, T[]? list, System.Action<System.IO.Stream!, T>! writeItem) -> void
Build overview
- Build status: FAILED
- Duration: 229.8s
- MSBuild version: 18.8.0-preview-26302-115
- Total projects: 49 | Errors: 25 | Warnings: 0
- Failed projects:
Microsoft.Testing.Extensions.HangDump,Microsoft.Testing.Extensions.MSBuild,Microsoft.Testing.Extensions.Retry,Microsoft.Testing.Extensions.TrxReport,NonWindowsTests.slnf,Build.proj
All MSBuild errors (25 — 2 unique, repeated across 4 projects × multiple TFMs)
| Code | Project | File:Line | Message |
|---|---|---|---|
RS0051 |
HangDump |
BaseSerializer.cs:359 |
ReadFields is not part of the declared API |
RS0051 |
HangDump |
BaseSerializer.cs:380 |
WriteListPayload<T> is not part of the declared API |
RS0051 |
MSBuild |
BaseSerializer.cs:359 |
ReadFields is not part of the declared API |
RS0051 |
MSBuild |
BaseSerializer.cs:380 |
WriteListPayload<T> is not part of the declared API |
RS0051 |
Retry |
BaseSerializer.cs:359 |
ReadFields is not part of the declared API |
RS0051 |
Retry |
BaseSerializer.cs:380 |
WriteListPayload<T> is not part of the declared API |
RS0051 |
TrxReport |
BaseSerializer.cs:359 |
ReadFields is not part of the declared API |
RS0051 |
TrxReport |
BaseSerializer.cs:380 |
WriteListPayload<T> is not part of the declared API |
(Each row repeats per target framework — 3 TFMs for most projects = 24 errors + 1 "Build failed" sentinel.)
🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit fdaa831
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 161.3 AIC · ⌖ 6.76 AIC · ⊞ 7.3K · [◷]( · ◷)
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 161.3 AIC · ⌖ 6.76 AIC · ⊞ 7.3K · ◷
BaseSerializer.cs is compiled as linked shared source into HangDump, MSBuild, TrxReport and Retry (in addition to Microsoft.Testing.Platform), and each of those projects does its own RS0051 internal-API tracking. My previous commit only tracked ReadFields/WriteListPayload<T> in Microsoft.Testing.Platform, so the four linking projects still failed RS0051 under CI's -warnaserror (locally these surface as warnings, which is why an earlier per-project build looked green). Add the same two entries to each project's InternalAPI.Unshipped.txt. Verified by reproduce-then-clear with CI-style flags (/p:ContinuousIntegrationBuild=true /p:RunAnalyzers=true --no-incremental): removing the lines re-emits RS0051; with the lines all five projects report zero RS0051/RS0016/RS0017. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…edup-providers # Conflicts: # src/Adapter/MSTest.TestAdapter/InternalAPI/InternalAPI.Unshipped.txt
The shared RunSettingsProviderHelper.CanReadFile intentionally broadens the caught exceptions to include UnauthorizedAccessException (a permission error on an existing .runsettings file now surfaces the friendly 'cannot be read' validation message). Add a unit test exercising that branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #9775
This advisory comment was generated automatically. Grades are heuristic
|
Summary
The MSTest adapter's native Microsoft.Testing.Platform providers (
src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/) contained near-identical copies of four VSTestBridge providers. Those copies were created deliberately during the "Native MTP integration" work (#9706/#9743/#9748) to remove the adapter's dependency on the VSTestBridge package, so re-adding a bridge reference is off the table.This PR removes the duplicated logic the dependency-free way (issue recommendation #1): a single linked shared source file that both projects compile, with no new project dependency.
What changed
src/Platform/SharedExtensionHelpers/RunSettingsProviderHelper.cs— aninternal static class(namespaceMicrosoft.Testing.Extensions, matching the sibling shared helpers) holding the resource-independent logic:CanReadFile(IFileSystem, string)TryLoadRunSettingsAsync(ICommandLineOptions, IFileSystem, IEnvironment, string)— CLI option →TESTINGPLATFORM_EXPERIMENTAL_VSTEST_RUNSETTINGScontent →TESTINGPLATFORM_VSTESTBRIDGE_RUNSETTINGS_FILEpath resolution, with the existingNETCOREAPPvs non-coreXDocumentload branching preservedHasEnvironmentVariables(XDocument)ApplyEnvironmentVariables(XDocument, IEnvironmentVariables)FindInvalidTestParameter(string[])#if !WINDOWS_UWP(MTP types aren't available on the adapter's UWP TFMs, which is why the adapter providers are UWP-guarded) and carries[SuppressMessage("ApiDesign", "RS0030")].Linkitems (VSTestBridge:Helpers\...; adapter:Shared\...).RunSettingsCommandLineOptionsProvider/MSTestRunSettingsCommandLineOptionsProvider) — sharedCanReadFile; each keeps its ownExistFilecheck and resource messages.RunSettingsEnvironmentVariableProvider/MSTestRunSettingsEnvironmentVariableProvider) — sharedTryLoadRunSettingsAsync+HasEnvironmentVariables+ApplyEnvironmentVariables, each passing its own option-name constant.FindInvalidTestParameter, each formatting with its own resource string.Decoupling preserved
No dependency on
Microsoft.Testing.Extensions.VSTestBridgeis reintroduced. The adapter and the bridge simply compile the same source file fromsrc/Platform/SharedExtensionHelpers, exactly like the existing shared helpers in that folder. Each package still owns its own resource strings (PlatformAdapterResourcesvsExtensionResources), option registration and UWP guards.Drift resolved
The env-var providers had diverged: the adapter used
string.IsNullOrEmptywhile the bridge usedRoslynString.IsNullOrEmpty. The logic now lives in one place and is unified onstring.IsNullOrEmpty.Behavior
No option-surface change: option names (
settings,filter,test-parameter), descriptions, arities, validation messages and registration are unchanged. No resx/xlf/PublicAPI edits (text unchanged; new type isinternal).One intentional behavioral improvement in the shared
CanReadFile: it now catchesUnauthorizedAccessExceptionin addition toIOException. Previously a--settingsfile that exists but is unreadable due to permissions would let the exception propagate out ofValidateOptionArgumentsAsync; it now surfaces the friendlyRunsettingsFileCannotBeReadvalidation message. This is a strict superset of the prior behavior and is covered by a new unit test (RunSettingsOption_WhenFileCannotBeReadDueToPermissions_IsNotValid).Validation
.\build.cmd -pack— succeeded, 0 warnings / 0 errors; all shipping NuGets (incl. MSTest.TestAdapter and Microsoft.Testing.Extensions.VSTestBridge) produced.net8.0,net9.0,netstandard2.0) and MSTest.TestAdapter (net462,net8.0,net9.0,net8.0-windows,net9.0-windows,uap10.0.16299).Microsoft.Testing.Extensions.VSTestBridge.UnitTests— 45/45 passing (covers the RunSettings CLI — including the new permission-error branch — env-var and TestRunParameters providers).Microsoft.Testing.Platform.Acceptance.IntegrationTests→HelpInfoTests21/21,HelpInfoAllExtensionsTests9/9MSTest.Acceptance.IntegrationTests→HelpInfoTests6/6Fixes #9762.