Remove IFrameworkHandle from the PlatformServices execution engine (Phase 6e-1)#9622
Conversation
…hase 6e-1) The execution engine used the VSTest IFrameworkHandle exclusively to obtain an IAdapterMessageLogger via ToAdapterMessageLogger(). Replace the IFrameworkHandle parameter with the neutral IAdapterMessageLogger throughout TestExecutionManager (RunTestsAsync both overloads, ExecuteTestsAsync, ExecuteTestsInSourceAsync, Deploy); the adapter boundary (MSTestExecutor) now calls frameworkHandle.ToAdapterMessageLogger() once and injects the result. This removes the last VSTest ObjectModel.Adapter reference from the execution engine. No behavior change: the logger wrapper is stateless, so injecting one instance is identical to building one per call site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
e3dac94
into
dev/amauryleve/vstest-decoupling-runcontext
There 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.
Expert Review — PR #9622 · Phase 6e-1: Remove IFrameworkHandle from execution engine
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM |
| 2 | Threading & Concurrency | ✅ LGTM |
| 3 | Security & IPC Contract Safety | — N/A |
| 4 | Public API & Binary Compatibility | — N/A (all internal) |
| 5 | Performance & Allocations | ✅ LGTM (minor improvement: 1 wrapper/run instead of 2–4) |
| 6 | Cross-TFM Compatibility | ✅ LGTM |
| 7 | Resource & IDisposable Management | — N/A |
| 8 | Defensive Coding at Boundaries | ✅ LGTM |
| 9 | Localization & Resources | — N/A |
| 10 | Test Isolation | ✅ LGTM |
| 11 | Assertion Quality | ✅ LGTM (AwesomeAssertions, per BannedSymbols.txt) |
| 12 | Flakiness Patterns | — N/A |
| 13 | Test Completeness & Coverage | ✅ LGTM |
| 14 | Data-Driven Test Coverage | — N/A |
| 15 | Code Structure & Simplification | 🟡 2 NIT (inline) |
| 16 | Naming & Conventions | ✅ LGTM |
| 17 | Documentation Accuracy | ✅ LGTM |
| 18 | Analyzer & Code Fix Quality | — N/A |
| 19 | IPC Wire Compatibility | — N/A |
| 20 | Build Infrastructure & Dependencies | — N/A |
| 21 | Scope & PR Discipline | ✅ LGTM |
| 22 | PowerShell Scripting Hygiene | — N/A |
✅ 13/13 applicable dimensions clean — 2 NIT items flagged inline.
Notes
Correctness confirmation: HostMessageLogger is a stateless value-object (single readonly field delegating directly to the underlying IMessageLogger). Collapsing multiple per-call-site ToAdapterMessageLogger() invocations into one shared instance at the adapter boundary is byte-for-byte equivalent. The RemotingMessageLogger cross-AppDomain wrapper correctly wraps any IAdapterMessageLogger, so the shared-instance pattern is safe there too.
Threading: The shared adapterMessageLogger instance is used across parallel workers in ExecuteTestsInSourceAsync. Since HostMessageLogger.SendMessage is a pure delegation and IMessageLogger implementations provided by VSTest hosts are thread-safe, no race condition is introduced.
2 NIT items (inline): Both TestExecutionManager.Parallelization.cs:68 and TestExecutionManager.cs:133 now hold trivial local-variable aliases (adapterMessageLogger = messageLogger and logger = messageLogger). These were meaningful before (each called frameworkHandle.ToAdapterMessageLogger()); now they're no-op assignments. A follow-up cleanup would rename the parameter or remove the local and update downstream uses within the method — both are purely cosmetic.
| var tests = new List<UnitTestElement>(); | ||
|
|
||
| IAdapterMessageLogger logger = frameworkHandle.ToAdapterMessageLogger(); | ||
| IAdapterMessageLogger logger = messageLogger; |
There was a problem hiding this comment.
NIT (Dim 15 — Code Structure): Same trivial alias pattern as in ExecuteTestsInSourceAsync: logger is now identically equal to messageLogger. This line was meaningful before (it called frameworkHandle.ToAdapterMessageLogger()) but is now dead.
Follow-up cleanup: rename the parameter or remove the local and update the handful of downstream uses in this method.
| #endif | ||
|
|
||
| IAdapterMessageLogger adapterMessageLogger = frameworkHandle.ToAdapterMessageLogger(); | ||
| IAdapterMessageLogger adapterMessageLogger = messageLogger; |
There was a problem hiding this comment.
NIT (Dim 15 — Code Structure): adapterMessageLogger is now a trivial alias for the incoming parameter messageLogger; the two names exist only because the earlier code called frameworkHandle.ToAdapterMessageLogger() here to produce a new wrapper.
Follow-up cleanup option: rename the parameter from messageLogger to adapterMessageLogger (matching the existing local variable) and drop this assignment, keeping the rest of the method body unchanged. Alternatively, remove the local and replace all downstream uses of adapterMessageLogger with messageLogger. Either way no behavior change.
…ervices Squashed rebase of the vstest-decoupling PlatformServices stack onto main. Phases 1, 2 and 5 already landed on main via #9548/#9567/#9550; this commit carries the remaining net-new work: - Phase 3 (#9566): abstract the VSTest discovery sink (IUnitTestElementSink). - Phase 4 (#9572): abstract VSTest execution input. - Phase 6a (#9576): neutralize deployment input (DeploymentContext). - Phase 6b (#9579): neutralize test result recording (ITestResultRecorder). - Phase 6c (#9585): neutralize test message logging. - Phase 6c2: neutralize run-settings input in the host layer (settingsXml). - Phase 6d-1: move test-case filter parsing to the adapter boundary (ITestElementFilterProvider / TestElementFilterProvider). - Phase 6d-2: remove IRunContext/IDiscoveryContext from PlatformServices. - Phase 6e-1 (#9622): remove IFrameworkHandle from the execution engine. - Phase 6e-2 (#9623): relocate VSTest logger/sink bridges to the adapter. - Phase 6e-3a (#9624): neutralize the trait type on UnitTestElement. Result: MSTestAdapter.PlatformServices no longer references the VSTest run/discovery context or result object model; those types live only at the MSTest.TestAdapter boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ervices Squashed rebase of the vstest-decoupling PlatformServices stack onto main. Phases 1, 2 and 5 already landed on main via #9548/#9567/#9550; this commit carries the remaining net-new work: - Phase 3 (#9566): abstract the VSTest discovery sink (IUnitTestElementSink). - Phase 4 (#9572): abstract VSTest execution input. - Phase 6a (#9576): neutralize deployment input (DeploymentContext). - Phase 6b (#9579): neutralize test result recording (ITestResultRecorder). - Phase 6c (#9585): neutralize test message logging. - Phase 6c2: neutralize run-settings input in the host layer (settingsXml). - Phase 6d-1: move test-case filter parsing to the adapter boundary (ITestElementFilterProvider / TestElementFilterProvider). - Phase 6d-2: remove IRunContext/IDiscoveryContext from PlatformServices. - Phase 6e-1 (#9622): remove IFrameworkHandle from the execution engine. - Phase 6e-2 (#9623): relocate VSTest logger/sink bridges to the adapter. - Phase 6e-3a (#9624): neutralize the trait type on UnitTestElement. Result: MSTestAdapter.PlatformServices no longer references the VSTest run/discovery context or result object model; those types live only at the MSTest.TestAdapter boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Phase 6e-1 of the PlatformServices platform-agnostic initiative
Part of removing the VSTest object model dependency from
MSTestAdapter.PlatformServices.The execution engine used the VSTest
IFrameworkHandle(fromObjectModel.Adapter) exclusively to obtain anIAdapterMessageLoggervia.ToAdapterMessageLogger(). This phase replaces theIFrameworkHandleparameter with the neutralIAdapterMessageLoggerthroughoutTestExecutionManager(bothRunTestsAsyncoverloads,ExecuteTestsAsync,ExecuteTestsInSourceAsync,Deploy); the boundary (MSTestExecutor) now callsframeworkHandle.ToAdapterMessageLogger()once and injects the result.This removes the last
ObjectModel.Adapterreference from the execution engine (IFrameworkHandleis now entirely absent from PlatformServices).No behavior change
ToAdapterMessageLogger()returns a statelessHostMessageLogger(single readonly field forwarding to the same underlying VSTest logger), so collapsing the previous per-call-site wrappers into one shared instance is observationally identical — including for theRemotingMessageLoggerthat marshals it into the child AppDomain.Verification
MSTestAdapter.PlatformServices.UnitTests: 897 (net8.0) / 935 (net462);MSTest.IntegrationTests: 47 pass / 1 skip.Base / stacking
Stacked on Phase 6d-2 (#9621). Base =
dev/amauryleve/vstest-decoupling-runcontext.Remaining work (6e continues)
AdapterMessageLoggerExtensions,MessageLevel,UnitTestElementSinkExtensions,TestCaseExtensions) toMSTest.TestAdapter.UnitTestElement↔TestCaseconversion (ToTestCase/GetOrCreateHostTestCase) +EngineConstantsVSTestTestPropertydefs +TcmTestPropertiesProvider; close Avoid double TestCase construction when filtering during discovery/execution (platform-agnostic refactor follow-up) #9568/CloneWithUpdatedSource mutatesthisinstead of the returned clone #9573.XmlRunSettingsUtilities,Constants,SuspendCodeCoverage, thetypeof(TestCase)version check).Microsoft.TestPlatform.ObjectModel(+ObjectModel.Utilities) PackageReference + guard test.