[fix] fix: auto-discover Code Coverage adapter from NuGet when using DLL paths

[nohwnd]

Summary

🤖 This is an automated fix generated by the Issue Triage agent.

Fixes #15351

Root Cause

When running dotnet test *.dll --collect:"Code Coverage", the MSBuild task (VSTestTask) is never invoked, so VSTestTraceDataCollectorDirectoryPath (set by Microsoft.CodeCoverage.props) is never injected as --testAdapterPath. vstest.console then cannot locate the Code Coverage data collector assembly and reports:

Unable to find a datacollector with friendly name 'Code Coverage'

When using a .csproj-based test run (dotnet test MyProject.csproj --collect:"Code Coverage"), the MSBuild task automatically adds the build/ directory of the microsoft.codecoverage NuGet package to the test adapter paths.

Fix

Added auto-discovery logic to CollectArgumentExecutor.AddDataCollectorToRunSettings: when --collect:"Code Coverage" is specified, the code now:

1. Determines the NuGet global packages path ($NUGET_PACKAGES env var, or ~/.nuget/packages on Linux/macOS, or %USERPROFILE%\.nuget\packages on Windows).
2. Scans for the microsoft.codecoverage package directory.
3. Selects the latest installed version (by stripping pre-release suffixes for numeric comparison).
4. Appends the build/ subdirectory to RunConfiguration.TestAdaptersPaths in run settings — mirroring exactly what the MSBuild task does.

If the package is not found (e.g. the user hasn't restored it yet, or is using a non-standard NuGet layout), the behavior degrades gracefully: no path is injected and the existing error message is preserved.

Tests

• Added 6 unit tests in CollectArgumentProcessorTests covering:
  • Missing package directory → returns false
  • Package dir without build/ subdirectory → returns false
  • Multiple versions installed → returns the latest
  • Pre-release suffix handling (strips suffix for numeric comparison)
  • TryAddCodeCoverageAdapterPath injects correctly and does not duplicate
  • AddDataCollectorToRunSettings end-to-end injection
• Updated 3 existing tests in EnableCodeCoverageArgumentProcessorTests that used exact XML comparison; they now check data collector state via XmlRunSettingsUtilities, making them resilient to whether the CC package happens to be installed in the test environment.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

This PR improves vstest.console’s --collect:"Code Coverage" behavior for dotnet test *.dll scenarios (where the MSBuild task doesn’t run) by auto-discovering the Microsoft Code Coverage adapter from the NuGet global packages folder and injecting its build/ directory into RunConfiguration.TestAdaptersPaths.

Changes:

• Added Code Coverage adapter auto-discovery/injection logic to CollectArgumentExecutor.AddDataCollectorToRunSettings.
• Implemented NuGet global packages resolution and microsoft.codecoverage “latest version” selection logic.
• Updated/added unit tests to validate Code Coverage runsettings enablement and adapter path injection behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/vstest.console/Processors/CollectArgumentProcessor.cs	Adds auto-discovery of microsoft.codecoverage package build/ path and injects it into TestAdaptersPaths when collecting Code Coverage.
test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs	Adds unit tests covering adapter path discovery, version selection, injection, and deduplication.
test/vstest.console.UnitTests/Processors/EnableCodeCoverageArgumentProcessorTests.cs	Makes tests resilient by asserting via XmlRunSettingsUtilities instead of exact XML string comparisons.

[nohwnd]

Code Review — PR #16215

This PR fixes a real gap: dotnet test *.dll --collect:"Code Coverage" fails today because VSTestTask is never invoked and the adapter path is never injected. The overall approach (auto-discovering the NuGet global package) is sound. However, there are two correctness bugs and one undescribed behavioral change that need addressing before merge.

---

🔴 Finding 1 — Pre-release wins over stable (correctness)

File: CollectArgumentProcessor.cs → ParseNuGetVersion / TryGetCodeCoverageAdapterPath

ParseNuGetVersion strips the pre-release suffix then compares numerically:

• 18.5.0 → Version(18,5,0)
• 18.6.0-preview-1 → stripped → 18.6.0 → Version(18,6,0) → wins

This violates NuGet SemVer: 18.6.0-preview-1 < 18.6.0. The test TryGetCodeCoverageAdapterPath_PreReleaseSuffixStripperPicksHigherNumerics locks in this incorrect behavior. A developer who inadvertently restores a preview alongside a stable release will silently get the preview adapter injected.

Fix: When numeric versions are equal after stripping, prefer the non-pre-release. When numerics differ, the higher number wins (current behavior is fine for that case).

---

🔴 Finding 2 — Injection bypasses explicit --testAdapterPath (correctness)

File: CollectArgumentProcessor.cs → TryAddCodeCoverageAdapterPath (line ~323–329)

See inline comment. The dedup guard only prevents adding the identical NuGet path twice. When the user explicitly passes --testAdapterPath /some/other/cc/path, the code appends the NuGet path alongside it — potentially loading two competing versions of the CC collector.

Fix: Skip auto-injection entirely when the run settings already have any TestAdaptersPaths set (i.e., the user has explicitly configured adapter discovery).

---

🟡 Finding 3 — --enable-code-coverage now also triggers NuGet injection (undescribed)

EnableCodeCoverageArgumentProcessor.Initialize() calls:

CollectArgumentExecutor.AddDataCollectorToRunSettings(FriendlyName, _runSettingsManager, _fileHelper);
// FriendlyName = "Code Coverage"

Because of this PR's change, that call now also triggers TryAddCodeCoverageAdapterPath. The --enable-code-coverage flag (a separate CLI option from --collect) now silently injects the NuGet adapter path too — this isn't described anywhere in the PR.

The three EnableCodeCoverageArgumentProcessorTests tests were updated from exact-XML to semantic assertions precisely because the XML now contains the injected path. These tests would fail in a CI environment where the microsoft.codecoverage NuGet package is installed, but pass in an environment where it isn't — that's environment-dependent test behavior.

Fix: Either document this intentional broadening, or gate the injection on --collect:"Code Coverage" by checking whether the call originates from CollectArgumentExecutor.Initialize() vs. the EnableCodeCoverage path (e.g., by not calling TryAddCodeCoverageAdapterPath from EnableCodeCoverageArgumentProcessor — or by making TryAddCodeCoverageAdapterPath opt-in rather than auto-triggered from AddDataCollectorToRunSettings).

---

i️ Minor — build/ directory check does not verify the adapter DLL exists

TryGetCodeCoverageAdapterPath accepts any directory named build/ inside a version folder. An empty build/ directory — or one from a corrupted package — will be returned as valid. Consider checking for the existence of the actual .targets / .dll file within it (the name is stable across NuGet releases).

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

[Correctness] Pre-release version is incorrectly preferred over a stable release

Stripping the pre-release suffix and comparing numerically means 18.6.0-preview-1 (stripped: 18.6.0) beats 18.5.0 (stable). This violates NuGet SemVer semantics where a pre-release is always less than the same numeric release (18.6.0-preview-1 < 18.6.0).

A developer who has both 18.5.0 (stable) and 18.6.0-preview-1 installed will silently get the preview adapter injected — exactly the opposite of what NuGet itself would resolve.

The test TryGetCodeCoverageAdapterPath_PreReleaseSuffixStripperPicksHigherNumerics documents this as intentional, but the intent conflicts with NuGet's own versioning contract.

Suggested fix — track whether the winning candidate is a pre-release and prefer stable when a higher numeric pre-release would otherwise win:

private static (Version? numeric, bool isPreRelease) ParseNuGetVersionWithStability(string versionName)
{
    var dashIndex = versionName.IndexOf('-');
    var isPreRelease = dashIndex >= 0;
    var versionString = isPreRelease ? versionName.Substring(0, dashIndex) : versionName;
    return Version.TryParse(versionString, out var version) ? (version, isPreRelease) : (null, false);
}

Then in the selection loop, when version == bestVersion, prefer the non-pre-release candidate.

[nohwnd]

Fixed in commit d7852db — ParseNuGetVersion now encodes stability into Version.Revision: stable → int.MaxValue, pre-release → 0. This ensures that when the numeric portion is identical (e.g. 18.5.0 vs 18.5.0-preview-1), the stable release wins — matching NuGet SemVer semantics. When the numeric portion differs (e.g. 18.6.0-preview-1 vs 18.5.0), the higher numeric version still wins regardless. A TryGetCodeCoverageAdapterPath_StableBeatsPreReleaseWhenSameNumerics test validates this tiebreaker.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

[Correctness] Auto-injection bypasses an explicit --testAdapterPath

The dedup check on line 323 only prevents adding the same exact path twice. It does not suppress injection when the user has already provided any adapter path.

Execution order matters here: TestAdapterPathArgumentProcessor runs at priority 9 and CollectArgumentExecutor runs at priority 11 (lower number = runs first). So when this code executes, a user-supplied --testAdapterPath /my/custom/cc/build is already in existingPaths. Because /my/custom/cc/build ≠ the NuGet-discovered path, both are appended:

TestAdaptersPaths = /my/custom/cc/build;/home/.nuget/packages/microsoft.codecoverage/18.5.0/build

With two paths that each contain the CC adapter DLL, vstest may discover and load two different versions of the same collector — or fail with a duplicate-assembly error.

Consider skipping injection entirely when existingPaths is already non-empty (the user has explicitly configured adapter paths and presumably knows what they're doing):

// If the user has already specified any adapter paths, respect their configuration.
if (existingPaths.Count > 0)
{
    return;
}

[nohwnd]

Fixed in the latest commit — TryAddCodeCoverageAdapterPath now returns early when existingPaths.Count > 0 (lines 323–327), so any user-configured --testAdapterPath values suppress auto-injection entirely. A corresponding test TryAddCodeCoverageAdapterPath_SkipsInjection_WhenExplicitAdapterPathAlreadySet was added to cover this case.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

[RunSettings Validation] The dedup guard only prevents injecting the same NuGet path twice. It doesn't cover the case where the user already specified --testAdapterPath /my/custom/codecoverage (a different CC adapter build): that path is in existingPaths, the NuGet CC path is not, so both end up registered. vstest will then probe both directories and potentially load two different versions of the CC data collector DLL into the same process — silent duplicate collection or TypeLoadException territory.

Simple guard: if any explicit adapter path is already set, assume the user is in control and skip auto-injection.

Suggested change

	if (existingPaths.Any(p => string.Equals(p, ccAdapterPath, StringComparison.OrdinalIgnoreCase)))
	if (existingPaths.Count > 0)
	{
	// User explicitly configured adapter paths — don't clobber with auto-discovered NuGet path.
	return;
	}

If that's too conservative (it would suppress injection even when paths came from some other unrelated collector), an alternative is to check whether any of the existing paths already contains the CC adapter DLL itself:

if (existingPaths.Any(p => File.Exists(Path.Combine(p, "Microsoft.CodeCoverage.DataCollection.dll"))))
{
    return;
}

[nohwnd]

Fixed in commit d7852db — TryAddCodeCoverageAdapterPath now returns early when existingPaths.Count > 0 (lines 323–327), so any user-configured --testAdapterPath values suppress auto-injection entirely. A corresponding TryAddCodeCoverageAdapterPath_SkipsInjection_WhenExplicitAdapterPathAlreadySet test was added.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

[Dependency & Package Integrity] The suffix-stripping approach has a determinism gap: when both 18.5.0 (stable) and 18.5.0-preview-1 (pre-release of the same release) are installed, both strip to 18.5.0 — a numeric tie — and the winner becomes whichever directory Directory.GetDirectories returns first (OS filesystem ordering, not documented). Stable should win there.

Note: 18.5.0 < 18.6.0-preview-1 is correct by NuGet SemVer (different major numeric), so picking the preview in that case is actually fine. The test name PreReleaseSuffixStripperPicksHigherNumerics is accurate — the issue is only the equal-numeric case.

Suggested change

	var dashIndex = versionName.IndexOf('-');
	private static Version? ParseNuGetVersion(string versionName)
	{
	// Strip pre-release suffix (e.g. "18.5.0-preview-1" → "18.5.0") so Version.TryParse works.
	var dashIndex = versionName.IndexOf('-');
	var isPreRelease = dashIndex >= 0;
	var versionString = isPreRelease ? versionName.Substring(0, dashIndex) : versionName;
	if (!Version.TryParse(versionString, out var version))
	{
	return null;
	}
	// Encode stability: subtract a tiny amount for pre-releases so that when the numeric
	// portion is identical, stable always wins. Use the Revision field as a tiebreaker
	// (stable = int.MaxValue, pre-release = 0) via a wrapper instead of mutating Version.
	// Simplest approach: callers compare (version, isPreRelease) as a tuple.
	return isPreRelease ? new Version(version.Major, version.Minor, version.Build, 0) : new Version(version.Major, version.Minor, version.Build, int.MaxValue);
	}

Or, keep ParseNuGetVersion returning (Version version, bool isPreRelease) and update the comparison at the call site — either way the point is to break ties in favour of stable.

[nohwnd]

Fixed in the latest commit — ParseNuGetVersion now encodes stability in the Revision field: stable → int.MaxValue, pre-release → 0. So 18.5.0 (Revision = MaxValue) beats 18.5.0-preview-1 (Revision = 0) on a numeric tie, while 18.6.0-preview-1 still beats 18.5.0 because the Major.Minor.Build portion is strictly higher. A TryGetCodeCoverageAdapterPath_StableBeatsPreReleaseWhenSameNumerics test validates this tiebreaker.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

[Backward Compatibility & Rollback Safety] This block is also reached via EnableCodeCoverageArgumentProcessor.Initialize(), which calls AddDataCollectorToRunSettings(FriendlyName, ...) — FriendlyName is "Code Coverage", so TryAddCodeCoverageAdapterPath now fires for --enable-code-coverage too. That's a behavioural change on a separate code path not mentioned in the PR description.

The diff in EnableCodeCoverageArgumentProcessorTests is the tell: three tests were rewritten from exact XML assertions to semantic XmlRunSettingsUtilities-based checks precisely because the new injection puts extra content into the settings XML and breaks the old Assert.AreEqual(expected-string) assertions.

Two questions:

1. Is auto-injecting the NuGet adapter path on --enable-code-coverage intentional? If yes, document it.
2. Is there a VSTEST_DISABLE_* env var or RunSettings opt-out for this new behaviour? If a regression is found (e.g. wrong adapter picked, TypeLoadException), there's currently no kill switch. Per our principles, new execution-semantic changes need a rollback path.

[nohwnd]

Fixed in commit d7852db — the TryAddCodeCoverageAdapterPath call was moved out of AddDataCollectorToRunSettings and into CollectArgumentExecutor.Initialize, guarded by a string.Equals(FriendlyName, ...) check. EnableCodeCoverageArgumentProcessor goes through AddDataCollectorToRunSettings directly and is no longer affected — there's no NuGet auto-discovery on the --enable-code-coverage path.

On the kill-switch question: the existing guard if (existingPaths.Count > 0) return means setting any --testAdapterPath disables auto-injection. That serves as the opt-out without a new env var.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Commit pushed: d7852db

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Addressed all three review findings in the latest commit:

Finding 1 — Stable beats pre-release on equal numerics: ParseNuGetVersion now encodes stability in the Version.Revision field (stable = int.MaxValue, pre-release = 0). This means 18.5.0 > 18.5.0-preview-1 (same numeric, stable wins) while 18.6.0-preview-1 > 18.5.0 (higher numeric wins regardless). Added TryGetCodeCoverageAdapterPath_StableBeatsPreReleaseWhenSameNumerics test.

Finding 2 — Skip injection when explicit adapter paths present: TryAddCodeCoverageAdapterPath now returns early when existingPaths.Count > 0, respecting any user-configured adapter paths. Added TryAddCodeCoverageAdapterPath_SkipsInjection_WhenExplicitAdapterPathAlreadySet test.

Finding 3 — Scope injection to --collect path only: Moved the TryAddCodeCoverageAdapterPath call out of AddDataCollectorToRunSettings and into CollectArgumentExecutor.Initialize. EnableCodeCoverageArgumentProcessor goes through AddDataCollectorToRunSettings and is no longer affected. The nugetPackagesOverride test hook was also removed from AddDataCollectorToRunSettings since it's no longer needed there.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Code Review — PR #16215 (iteration d7852db)

The PR Iteration Agent addressed all three findings from the prior review. Here is the verification:

---

✅ Finding 1 — Pre-release SemVer semantics (resolved)

ParseNuGetVersion now encodes stability in Version.Revision (stable = int.MaxValue, pre-release = 0):

• 18.5.0 → Version(18,5,0, int.MaxValue) beats 18.5.0-preview-1 → Version(18,5,0, 0) ✓
• 18.6.0-preview-1 → Version(18,6,0, 0) still beats 18.5.0 → Version(18,5,0, int.MaxValue) because 18.6 > 18.5 ✓

New test TryGetCodeCoverageAdapterPath_StableBeatsPreReleaseWhenSameNumerics covers the equal-numeric case. The approach is correct and well-commented.

---

✅ Finding 2 — Injection bypasses explicit --testAdapterPath (resolved)

TryAddCodeCoverageAdapterPath now returns early when existingPaths.Count > 0:

if (existingPaths.Count > 0)
{
    // User explicitly configured adapter paths — don't clobber with auto-discovered NuGet path.
    return;
}

Execution order (TestAdapterPath priority 9 → CollectArgumentExecutor priority 11) means user-supplied --testAdapterPath is already visible at injection time. New test TryAddCodeCoverageAdapterPath_SkipsInjection_WhenExplicitAdapterPathAlreadySet verifies this. ✓

---

✅ Finding 3 — Scope limited to --collect path only (resolved)

TryAddCodeCoverageAdapterPath is now exclusively called from CollectArgumentExecutor.Initialize, guarded by a friendly-name check:

if (string.Equals(collectArgumentList[0], MicrosoftCodeCoverageConstants.FriendlyName, StringComparison.OrdinalIgnoreCase))
{
    TryAddCodeCoverageAdapterPath(_runSettingsManager);
}

EnableCodeCoverageArgumentProcessor calls the static AddDataCollectorToRunSettings directly (not Initialize), so it is unaffected. ✓

---

[Description] Minor inaccuracy

The PR description states: "Added auto-discovery logic to CollectArgumentExecutor.AddDataCollectorToRunSettings". The final implementation places the logic in CollectArgumentExecutor.Initialize — the iteration commit deliberately moved it out of AddDataCollectorToRunSettings to fix Finding 3. The PR description was not updated to reflect the iteration changes. This is low-impact (the body comment by the iteration agent covers the changes), but worth a quick edit for historical accuracy.

---

No new issues were introduced in the iteration commit. The implementation is correct, the version-picking logic is well-documented, and all three fixes have targeted tests. Ready for human review.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠