Vendor a neutral SuspendCodeCoverage in PlatformServices (Phase 6e-4c3)

[Evangelink]

Phase 6e-4c3 — vendor a neutral SuspendCodeCoverage

Part of the initiative to make Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices platform-agnostic by removing its dependency on Microsoft.TestPlatform.ObjectModel. Strict byte-for-byte, no behavior change.

What this changes

TestDeployment (netfx) wraps the deployment file-copy in using (new SuspendCodeCoverage()) to pause dynamic code-coverage instrumentation of modules loaded while files are copied. That type came from Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities.

This vendors an internal neutral copy at Utilities/SuspendCodeCoverage.cs (namespace ...PlatformServices.Utilities), reproducing the VSTest behavior exactly:

• On construction: capture the current value of the process environment variable __VANGUARD_SUSPEND_INSTRUMENT__, then set it to TRUE.
• On dispose: restore the captured previous value (idempotent).

The environment-variable name and value are the collector IPC contract the dynamic code-coverage (Vanguard) engine reads, so they are preserved byte-identical. The wrapper is internal sealed with a straightforward idempotent Dispose — the original's Dispose(bool) / GC.SuppressFinalize plumbing has no finalizer to suppress and is behavior-equivalent to the direct restore.

TestDeployment now resolves SuspendCodeCoverage through its already-present using ...PlatformServices.Utilities;; the VSTest ObjectModel.Utilities using (and its deferral comment) is removed.

Fidelity proof and test-net limitation

The mechanism is a process environment variable (__VANGUARD_SUSPEND_INSTRUMENT__), not a named event / mutex / EventWaitHandle. There is no signal/listener model — the dynamic code-coverage (Vanguard) collector reads this variable from the process environment when deciding whether to instrument a module being loaded. The fidelity of the vendored copy therefore rests entirely on replicating that wire contract byte-identically against the OSS source (vstest v18.4.0 SuspendCodeCoverage.cs).

Source-diff (fidelity proof of record) — OSS original vs vendored copy:

Contract element	OSS (Microsoft.TestPlatform.ObjectModel)	Vendored copy
env var name	"__VANGUARD_SUSPEND_INSTRUMENT__"	"__VANGUARD_SUSPEND_INSTRUMENT__"
set value	"TRUE"	"TRUE"
target (all 3 accesses)	EnvironmentVariableTarget.Process	EnvironmentVariableTarget.Process
ctor	GetEnvironmentVariable(name, Process) → capture; SetEnvironmentVariable(name, "TRUE", Process)	identical
dispose	SetEnvironmentVariable(name, prev, Process), guarded by _isDisposed	identical

The only intentional difference is the collapse of the OSS Dispose() / protected virtual Dispose(bool) / GC.SuppressFinalize into a single idempotent Dispose() — behavior-equivalent because the OSS type declares no finalizer (so GC.SuppressFinalize is a no-op and disposing is always true on the public path). The name/value/target/sequence — the entire wire contract the collector keys off — are verbatim.

Note on the test net: PlatformServices.Desktop.IntegrationTests runs with no coverage collector attached, so its green result proves the vendored code does not crash / the deploy path still works — it does not exercise a real collector reading the variable. That is acceptable here precisely because there is no signaling to get wrong: correctness is fully determined by the (source-identical) variable name/value/target above. There is no clean seam to assert the variable mid-deploy without contorting the copy loop, so no brittle probe was added.

Result: PlatformServices is ObjectModel-type-free

After this change, PlatformServices has zero using/type references to the Microsoft.TestPlatform.ObjectModel package — only string-literal assembly names (used for by-name runtime assembly lookup in the AppDomain/source-host wiring) remain. This clears the way to drop the package reference in the capstone (Phase 7).

Verification

• Builds 0-warning (net462 and all real TFMs; netfx-guarded change).
• MSTestAdapter.PlatformServices.UnitTests: 935/935 (net462), 897/897 (net8.0).
• PlatformServices.Desktop.IntegrationTests: 15/15 (net462) — exercises the deployment path that runs inside the SuspendCodeCoverage scope.
• Expert-reviewer pass.

Stacking

Stacks on #9631 (Phase 6e-4c2); base branch dev/amauryleve/vstest-decoupling-sourcehandler. Review/merge after the earlier PRs in the chain reach the base. Do not squash-rebase 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 — PR #9632 · Vendor a neutral SuspendCodeCoverage in PlatformServices (Phase 6e-4c3)

Summary

This PR accomplishes two related goals under the ObjectModel-decoupling initiative:

1. Vendors SuspendCodeCoverage — a faithful internal copy that preserves the Vanguard IPC contract (__VANGUARD_SUSPEND_INSTRUMENT__ / "TRUE" / EnvironmentVariableTarget.Process) byte-identically, gated behind #if NETFRAMEWORK. The PR description's fidelity proof is thorough and accurate.

2. Vendors DoesSourceReferenceAssembly — replaces the call to AssemblyHelper.DoesReferencesAssembly (vstest ObjectModel) with a local implementation. The original code carried a comment noting this in-AppDomain optimization was acceptable; the new implementation honors that pre-approval.

One minor code-quality issue is noted; no blocking or major findings.

---

Verdict Table

#	Dimension	Status	Notes
1	Algorithmic Correctness	⚠️ Minor	ArePublicKeyTokensEqual does not handle null from GetPublicKeyToken(); NullReferenceException is silently caught → conservative fallback. See inline comment.
2	Threading & Concurrency	✅ Clean	SuspendCodeCoverage is used in a single-threaded deployment context; plain bool _isDisposed is sufficient. SetEnvironmentVariable concurrency matches original behavior.
3	Security & IPC Contract Safety	✅ Clean	Env-var name, value, and target are preserved verbatim. No path traversal or injection vectors introduced.
4	Public API & Binary Compatibility	✅ Clean	Both new types are internal sealed. No public API surface changed. PublicAPI.Unshipped.txt update not needed.
5	Performance & Allocations	✅ Clean	No LINQ, no unnecessary allocations. The ReflectionOnlyLoadFrom path is equivalent in cost to the original (same assembly load, minus the AppDomain round-trip).
6	Cross-TFM Compatibility	✅ Clean	All new code is guarded by #if NETFRAMEWORK. Other TFMs are unaffected.
7	Error Handling & Resilience	✅ Clean	Bare catch in DoesSourceReferenceAssembly is intentional and carries a comment; it mirrors the original vstest CheckAssemblyReference pattern. Conservative fallback (null → discover anyway) is correct.
8	Resource Management	✅ Clean	SuspendCodeCoverage is IDisposable, idempotent, and used via using. No finalizer is needed (none exists in the original either; GC.SuppressFinalize was a no-op).
9	Naming & Code Style	✅ Clean	All names follow conventions. _isDisposed, _previousEnvironmentValue, constants are appropriately cased.
10	Documentation & Comments	✅ Clean	Both new files have XML doc comments. The fidelity comparison table in the PR description is exemplary.
11	Test Coverage & Quality	✅ Clean	Four existing unit tests in DesktopTestSourceTests exercise the refactored IsAssemblyReferenced path, including the null-source/null-assembly, found, and not-found cases. PlatformServices.Desktop.IntegrationTests exercises the deploy path with SuspendCodeCoverage.
12	Localization & Resource Strings	N/A	No user-facing strings added.
13	Logging & Diagnostics	N/A	No logging changes.
14	Dependency & Package Hygiene	✅ Clean	The Microsoft.TestPlatform.ObjectModel type-reference is eliminated as intended. No new packages introduced.
15	Code Complexity & Maintainability	✅ Clean	Both new methods are compact and well-commented. DoesSourceReferenceAssembly is ~35 LOC with clear phases.
16	Test Infrastructure & Acceptance Tests	N/A	No CLI options, output formats, or acceptance-test expectations were changed.
17	Configuration & Defaults	N/A	No configuration changes.
18	Scope Discipline	✅ Clean	The TestSourceHandler.cs refactoring is logically part of the same ObjectModel-decoupling goal and is covered by the PR description, even if the title focuses on SuspendCodeCoverage.
19	Build & Project File Quality	N/A	No .csproj/.props changes needed; the new .cs file is picked up by the existing glob.
20	Invariant/Contract Violations	⚠️ Minor	byte[] parameters in ArePublicKeyTokensEqual should be byte[]?; GetPublicKeyToken() returns nullable and the call sites do not null-check. See inline comment.
21	Behavioral Regression Risk	✅ Clean	AppDomain isolation is intentionally dropped (the original code carried a "we can optimize this" comment). ReflectionOnlyLoadFrom in the current AppDomain is safe: it does not execute code from the loaded assembly.
22	PowerShell / Script Quality	N/A	No scripts modified.

---

Key Finding

⚠️ Minor — ArePublicKeyTokensEqual null-handling gap (TestSourceHandler.cs line 142)

GetPublicKeyToken() returns byte[]?; both call sites pass the result directly to byte[] parameters. For unsigned assemblies (null token) this throws NullReferenceException, silently caught → null → conservative discovery. The behavior is correct by accident rather than by design. The original vstest CheckAssemblyReference had the same gap, so this is a clean-up opportunity only, not a regression. An inline suggestion is attached.

---

Positive Notes

• The fidelity proof table (env-var name / value / target / ctor / dispose sequence) is an excellent record for future maintainers.
• Collapsing Dispose(bool disposing) + GC.SuppressFinalize to a direct Dispose() is the correct simplification given the absence of a finalizer.
• The AppDomain drop in DoesSourceReferenceAssembly is safe and was pre-authorized by the inline comment in the original code.

[github-actions]

ArePublicKeyTokensEqual declares non-nullable byte[] parameters, but both callers pass AssemblyName.GetPublicKeyToken() which returns byte[]? (null for unsigned assemblies). When either token is null, left.Length on line 144 throws NullReferenceException, caught by the outer try/catch, which returns null → conservative path → discovery proceeds. Functionally safe, but the correctness depends on exception routing rather than explicit logic.

Suggested fix (nullable-aware):

private static bool ArePublicKeyTokensEqual(byte[]? left, byte[]? right)
{
    if (left is null && right is null) return true;
    if (left is null || right is null) return false;
    if (left.Length != right.Length) return false;
    for (int i = 0; i < left.Length; ++i)
    {
        if (left[i] != right[i]) return false;
    }
    return true;
}

This makes "both sides unsigned → match by name" explicit and eliminates the implicit NullReferenceException. Also update referenceAssemblyPublicKeyToken (line 113) to byte[]? to satisfy the nullable annotation.

Not a regression: the original vstest CheckAssemblyReference had the same gap — byte[] publicKeyToken1 = referencedAssembly.GetPublicKeyToken() without null-guard. This is a clean-up opportunity only.