Inject TestSessionPool into the CrossPlatEngine session proxies - #16225
Merged
Jakub Jareš (nohwnd) merged 1 commit intoJul 7, 2026
Merged
Conversation
`TestSessionPool` is reached through the mutable public static `TestSessionPool.Instance` from every site that coordinates a test session: the writer `ProxyTestSessionManager.AddSession`, and the readers `TestEngine.TryTakeProxy`, `ProxyDiscoveryManager.ReturnProxy` and `ProxyExecutionManager.ReturnProxy`. Because it is process-wide state, the writer and readers of a given session only line up by virtue of all resolving the same static, which is the kind of implicit shared state that makes this code hard to test in isolation. Thread one `TestSessionPool?` from `TestEngine` (the single place that constructs the session-mode proxies) into `ProxyTestSessionManager`, `ProxyDiscoveryManager` and `ProxyExecutionManager`. Each holds the injected pool in a nullable field and reads `(_testSessionPool ?? TestSessionPool.Instance)` at the call site, so when nothing is injected the behavior is byte-for-byte what it was before. The new parameters live on internal constructors/overloads only, so no shipped public signature changes and `TestSessionPool.Instance` keeps working for the callers that still use it. `TestRequestManager.KillSession` in vstest.console is intentionally left on the static - it is not one of the objects `TestEngine` builds, so injecting there would be a separate seam. Adds a `ProxyTestSessionManagerTests` case that injects one pool as the shared instance and a different pool behind the static default: `StartSession` writes to the injected pool and a `TryTakeProxy` off the same injected instance observes the proxy, while the static default never sees the session. That guards the same-instance contract the refactor relies on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces reliance on the mutable global TestSessionPool.Instance by allowing TestEngine (the composition root for session-mode proxies) to optionally inject a TestSessionPool instance into the session-aware proxy components. This improves testability and reduces hidden coupling while keeping the default runtime behavior unchanged when nothing is injected.
Changes:
- Thread an optional
TestSessionPool?fromTestEngineintoProxyTestSessionManager,ProxyDiscoveryManager, andProxyExecutionManager. - Update session-mode proxy acquisition/return paths to use
(_testSessionPool ?? TestSessionPool.Instance)at the call sites. - Add a unit test ensuring the writer (
StartSession/AddSession) and reader (TryTakeProxy) operate on the same injected pool instance (and not the static singleton).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyTestSessionManagerTests.cs | Adds a unit test verifying injected pool usage (writer/reader coherence). |
| src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs | Allows injecting a pool for AddSession while preserving the static fallback. |
| src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs | Threads an optional pool through session-mode proxy creation and proxy take/return logic. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs | Returns proxies to the injected pool (or falls back to the static pool). |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs | Returns proxies to the injected pool (or falls back to the static pool). |
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
TestSessionPoolis reached everywhere through the mutable public staticTestSessionPool.Instance. The sites that coordinate a test session - the writerProxyTestSessionManager.AddSessionand the readersTestEngine.TryTakeProxy,ProxyDiscoveryManager.ReturnProxyandProxyExecutionManager.ReturnProxy- only agree on the same pool because they all resolve the same static. That is process-wide state feeding a request-scoped concern, and it makes the session proxies awkward to exercise in isolation.Change
TestEngineis the single place that constructs the session-mode proxies, so I thread oneTestSessionPool?from there intoProxyTestSessionManager,ProxyDiscoveryManagerandProxyExecutionManager. Each stores it in a nullable field and reads(_testSessionPool ?? TestSessionPool.Instance)at the call site.nulland every site readsTestSessionPool.Instance, exactly as before - runtime behavior is byte-for-byte identical.PublicAPIedit.TestSessionPool.Instanceis untouched and keeps working for the callers that still use it (no[Obsolete]).TestRequestManager.KillSessioninvstest.consoleis intentionally left on the static. It is not one of the objectsTestEnginebuilds, so wiring it would be a separate seam; in production it resolves the same static instance, so it is already correct.This follows the same "inject one instance from the composition root, default it to the existing static" shape as the earlier argument-processor changes.
Verification
Added a
ProxyTestSessionManagerTestscase that injects one pool as the shared instance and puts a different pool behind the static default.StartSessionwrites to the injected pool, and aTryTakeProxyoff that same injected instance observes the proxy the writer added, while the static default never sees the session. If a writer and a reader ever ended up on different instances, that read side would come back empty - so the test guards the same-instance contract the refactor depends on.Locally on Windows:
build.cmd -c Release- 0 errors, noIDE0005(only the pre-existingCommunicationUtilitiesIL trim warnings).Microsoft.TestPlatform.CrossPlatEngine.UnitTests(net481) - 675 total, 674 passed, 1 skipped, 0 failed.build.cmd -pack- binding-redirect and DLL-target-framework checks pass.test.cmd -smokeTest-Library.IntegrationTestsandAcceptance.IntegrationTestsall green.