Fix "Code Coverage" collector not found with dotnet test *.dll

[Jakub Jareš (nohwnd)]

Fixes #15351

dotnet test *.dll --collect:"Code Coverage" fails with Unable to find a datacollector with friendly name 'Code Coverage'. In DLL mode the MSBuild task never runs, so the adapter path that Microsoft.CodeCoverage.props normally injects as --testAdapterPath is missing, and vstest.console cannot locate the collector. A .csproj run works because the MSBuild task adds the folder that props file sits in.

CollectArgumentExecutor.Initialize now does the same for --collect:"Code Coverage". It finds microsoft.codecoverage in the NuGet global packages folder (NUGET_PACKAGES, else ~/.nuget/packages), and appends the folder holding Microsoft.VisualStudio.TraceDataCollector.dll to TestAdaptersPaths. That is the same leaf folder the MSBuild path uses, so the run does not depend on the adapter loading strategy searching directories recursively. The folder is looked up rather than hardcoded, older packages ship the collector under netstandard1.0 instead of netstandard2.0.

There is no project to ask which version to use, so the highest installed one wins, preferring a stable release over a pre-release so that a preview left in the package cache does not take over a run that never referenced it.

It does nothing when --testAdapterPath is already set, when the package is not installed, or when the folder cannot be read, so a run that already works is untouched. Scoped to --collect, --enable-code-coverage still relies on the MSBuild setup. Set VSTEST_DISABLE_CODE_COVERAGE_ADAPTER_DISCOVERY=1 to turn the discovery off without having to pass an adapter path. NuGet.Config globalPackagesFolder is not consulted, reading it means taking a dependency on the NuGet libraries.

Tests in CollectArgumentProcessorTests cover version selection, the collector folder lookup, injection, and the skip cases.

🤖

[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.

[Jakub Jareš (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 🧠

[Jakub Jareš (nohwnd)]

Commit pushed: d7852db

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (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 🔧

[Jakub Jareš (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 🧠

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/vstest.console/Processors/CollectArgumentProcessor.cs:381

• TryGetCodeCoverageAdapterPath enumerates the NuGet package directory with Directory.GetDirectories(ccPackagePath) without handling IO/permission failures. If the global packages folder exists but cannot be enumerated (e.g. access denied, transient IO error), this will throw and abort --collect:"Code Coverage" processing instead of silently no-oping as intended.

        foreach (var versionDir in Directory.GetDirectories(ccPackagePath))
        {
            var buildDir = Path.Combine(versionDir, "build");
            if (!Directory.Exists(buildDir))
            {

test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs:13

• The newly added using Microsoft.VisualStudio.TestPlatform.Common.Utilities; is unused in this test file and is likely to trip IDE0005 (unnecessary using) in CI (warnings-as-errors).

using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;

src/vstest.console/Processors/CollectArgumentProcessor.cs:321

• TryAddCodeCoverageAdapterPath treats any non-empty TestAdaptersPaths inner text as an explicitly configured adapter path. If the runsettings contains only whitespace/newlines in <TestAdaptersPaths> (common with formatted XML), IsNullOrEmpty() returns false and injection is skipped even though there are no real paths.

        var existingPathsRaw = runSettingsManager.QueryRunSettingsNode(TestAdapterPathArgumentExecutor.RunSettingsPath);
        var existingPaths = existingPathsRaw.IsNullOrEmpty()
            ? []
            : existingPathsRaw.Split(';').Where(p => !p.IsNullOrEmpty()).ToList();

[github-actions]

Code Review — PR #16215 (iteration 60a96ac)

This iteration refactors the version-comparison logic from the Version.Revision stability-encoding hack into a proper two-field (Version numeric, string? preRelease) approach with a full SemVer 2.0 comparator. All three findings from the prior review and all observations from the iteration review remain resolved.

---

✅ Prior findings — still resolved

• Finding 1 (pre-release SemVer semantics): CompareNuGetVersions now handles every case correctly — numeric-major wins, then stable beats pre-release at equal numerics, then SemVer 2.0 label ordering, then ordinal folder-name tiebreaker. ✓
• Finding 2 (injection bypasses explicit --testAdapterPath): existingPaths.Count > 0 guard still in place. ✓
• Finding 3 (scope limited to --collect path): TryAddCodeCoverageAdapterPath called only from Initialize, guarded by the FriendlyName equality check. ✓

---

New edge cases handled in this iteration

• Two-component folder names (18.5) — Version.TryParse succeeds; 18.5 sorts below 18.5.0 (Build -1 < 0), consistent with NuGet. ✓
• Four-component versions (18.5.0.1, 18.5.0.2) — Version.CompareTo uses all four components; no longer overwritten. ✓
• Build metadata (18.6.0+abc123) — stripped before parsing; treated as stable 18.6.0. ✓
• Same-numerics two pre-releases (18.6.0-preview-1 vs 18.6.0-preview-2) — SemVer 2.0 ComparePreReleaseLabels; deterministic regardless of Directory.GetDirectories order. ✓
• Dotted numeric pre-release identifiers (rc.2 vs rc.10) — ComparePreReleaseIdentifiers parses with NumberStyles.None so 10 > 2. ✓

---

🔵 Minor — filesystem I/O before cheap early-exit guard

See inline comment at line 312. TryGetCodeCoverageAdapterPath (three I/O calls) runs before existingPaths.Count > 0 is checked. Reordering costs nothing in the common case. Not a correctness issue.

---

[Description] Still inaccurate (carry-over from prior review)

The PR description still says "Added auto-discovery logic to CollectArgumentExecutor.AddDataCollectorToRunSettings". The logic lives in Initialize, not in AddDataCollectorToRunSettings. The prior review noted this; it hasn't been updated. Low-impact but worth a quick edit for historical accuracy.

---

The SemVer 2.0 comparator is correct and well-tested. Ready for human review.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

63f19f5 answers the review on 60a96ac. The inline finding and the older threads are answered in place, this covers the three suppressed comments, which have no thread to reply in.

Directory.GetDirectories is unguarded. Correct, and worth taking. Discovery now runs inside a try for IOException, UnauthorizedAccessException and ArgumentException and reports not found instead of throwing out of Initialize. ArgumentException is in the set because Path.Combine rejects invalid characters on .NET Framework, so a broken NUGET_PACKAGES would have failed the run before it reached Directory.Exists. TryGetCodeCoverageAdapterPath_ReturnsFalse_WhenNuGetPackagesPathIsUnusable covers it.

The using Microsoft.VisualStudio.TestPlatform.Common.Utilities in the test file is unused and will trip IDE0005 in CI. This one is wrong, and acting on it breaks the build. That namespace holds RunSettingsProviderExtensions, so it is what makes the extension methods AddDefaultRunSettings, UpdateRunSettingsNode and QueryRunSettingsNode resolve. Removing it fails compilation with 8 CS1061 errors on both net481 and net11.0. Extension methods are invisible to a grep for the namespace name, which is probably how it got flagged.

Whitespace only <TestAdaptersPaths> is treated as configured. Does not reproduce, details in the thread on the guard. The check is whitespace aware now anyway.

On the description note: the body already says CollectArgumentExecutor.Initialize. The AddDataCollectorToRunSettings wording is in the Copilot reviewer overview of the first commit, not in the PR body.

Release build is clean and vstest.console.UnitTests is green on both target frameworks, 653 tests, 0 failed.

🤖

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/vstest.console/Processors/CollectArgumentProcessor.cs:316

• The check for pre-existing adapter paths hardcodes ';' and duplicates parsing logic. Using the existing TestAdapterPathArgumentExecutor.SplitPaths helper keeps path splitting behavior consistent (and centralizes the separator definition) while preserving the intended whitespace-only behavior.

        var existingPathsRaw = runSettingsManager.QueryRunSettingsNode(TestAdapterPathArgumentExecutor.RunSettingsPath);
        if (!existingPathsRaw.IsNullOrWhiteSpace() && existingPathsRaw.Split(';').Any(p => !p.IsNullOrWhiteSpace()))

src/vstest.console/Processors/CollectArgumentProcessor.cs:349

• TryGetCodeCoverageAdapterPath is intended to be best-effort, but it does not catch SecurityException / NotSupportedException, which can be thrown by file system/path APIs and would crash --collect:"Code Coverage". Other argument processors (e.g., ResultsDirectoryArgumentExecutor) explicitly handle these cases.

        catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or ArgumentException)
        {
            // Auto-discovery is best effort. A packages folder we are not allowed to read, a transient
            // IO error, or an unusable NUGET_PACKAGES value must not take down the whole run.
            EqtTrace.Verbose("CollectArgumentExecutor.TryGetCodeCoverageAdapterPath: Could not inspect the NuGet global packages folder: {0}", ex);
        }

src/vstest.console/Processors/CollectArgumentProcessor.cs:326

• This verbose trace implies the NuGet package is missing, but TryGetCodeCoverageAdapterPath can also fail for other reasons (e.g., no build/ subdirectory, unreadable directories, or unparseable version folders). Wording it as "adapter path not found" is more accurate for diagnostics.

        {
            EqtTrace.Verbose("CollectArgumentExecutor.TryAddCodeCoverageAdapterPath: Microsoft.CodeCoverage package not found in NuGet global packages; skipping auto-injection.");
            return;

test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs:13

• using Microsoft.VisualStudio.TestPlatform.Common.Utilities; appears to be unused in this test file (and can trigger IDE0005/unnecessary-using failures in CI). Please remove it if it's not needed.

using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors;
using Microsoft.VisualStudio.TestPlatform.CommandLine.UnitTests;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;

[github-actions]

Code Review — PR #16215 (iteration 63f19f5)

The two open findings from the prior review have been addressed in commit 63f19f5:

✅ Performance: guard before I/O (resolved)

TryAddCodeCoverageAdapterPath now reads TestAdaptersPaths from run settings and returns early before touching the filesystem when a non-empty, non-whitespace path is already configured. The Split(';').Any(p => !p.IsNullOrWhiteSpace()) check correctly treats a whitespace-only XML node (e.g. a pretty-printed empty <TestAdaptersPaths> element) as "not configured", and the new TryInjects_WhenExistingAdapterPathsNodeIsOnlyWhitespace test validates exactly that.

✅ Exception handling: ArgumentException on .NET Framework (resolved)

TryGetCodeCoverageAdapterPath now wraps FindCodeCoverageAdapterPath in a catch for IOException | UnauthorizedAccessException | ArgumentException, so an invalid NUGET_PACKAGES value with characters that Path.Combine rejects on .NET Framework returns false rather than propagating out of Initialize. The new TryGetCodeCoverageAdapterPath_ReturnsFalse_WhenNuGetPackagesPathIsUnusable test exercises this path with "|:invalid\0path".

✅ PR description accuracy (resolved)

The description correctly says CollectArgumentExecutor.Initialize owns the new logic, the existing-paths guard is accurately described, and the --enable-code-coverage scope exclusion matches the code.

No new findings

The full diff has been reviewed across all dimensions. The algorithmic correctness, version-comparison logic, resource management, and test coverage are all sound. No issues were found.

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

6f19d14 takes three of the four suppressed comments from the Copilot reviewer on 63f19f5.

Hardcoded ';' duplicates the path splitting. Fair, and there is a better helper than I realised. The guard now calls TestAdapterPathArgumentExecutor.SplitPaths, which is the same splitter TestAdapterPathArgumentExecutor uses on this very node, so the separator stays defined in one place (ArgumentSeparators). The whitespace behaviour is unchanged, SplitPaths only drops empty entries, so the Any(p => !p.IsNullOrWhiteSpace()) on top is still what makes a whitespace only node count as unset.

SecurityException and NotSupportedException are not caught. Taken. ResultsDirectoryArgumentExecutor catches NotSupportedException or SecurityException or ArgumentException or PathTooLongException or IOException for the same kind of path work, so the discovery catch list now matches that idiom.

The trace claims the package is missing when discovery can fail for other reasons. Taken, it says the adapter path was not found now.

The using Microsoft.VisualStudio.TestPlatform.Common.Utilities in the test file is unused. This is the second time it has been raised and it is still wrong. That namespace holds RunSettingsProviderExtensions, so it is what makes AddDefaultRunSettings, UpdateRunSettingsNode and QueryRunSettingsNode resolve as extension methods. Removing it fails compilation with 8 CS1061 errors across net481 and net11.0. I tried it to be sure. Extension methods do not show up in a search for the namespace name, which is likely why it keeps getting flagged.

Release build clean, vstest.console.UnitTests green on both target frameworks, 653 tests, 0 failed.

🤖

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs:13

• The using Microsoft.VisualStudio.TestPlatform.Common.Utilities; directive appears unused in this test file and will trigger IDE0005 (unnecessary using) in builds that treat analyzer warnings as errors.

using Microsoft.VisualStudio.TestPlatform.Common.Utilities;

[Jakub Jareš (nohwnd)]

fhnaseer if this is good idea please approve.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[github-actions]

Review

Dimension	Status
RunSettings Validation & Inference	✅
Environment Variable & Feature Flag Contracts	⚠️
Backward Compatibility & Rollback Safety	⚠️
Null Safety & Boundary Validation	✅
Error Reporting & Diagnostic Clarity	✅
Algorithmic Correctness (version comparison)	✅
Acceptance Test Coverage	⚠️

Both July 5 correctness bugs are fixed. The CompareNuGetVersions implementation is clean: numeric version first, stable beats pre-release of identical numerics, SemVer 2.0 label comparison by dot-separated segments, ordinal folder name as final tiebreaker. TryParseNuGetVersion correctly preserves all four Version components and avoids the ArgumentOutOfRangeException the earlier Revision-encoding approach had on two-component folder names. The explicit path guard short-circuits before any filesystem I/O. Good.

Two things worth addressing before merge:

1. Kill switch (inline comment at line 125) — --testAdapterPath is a functional opt-out but not a discoverable one. If auto-discovery picks the wrong version (or the right package but a broken build), there's no VSTEST_DISABLE_* env var to reach for. One-line fix.

2. No acceptance test for the motivating scenario — CodeCoverageTests.cs has no test that runs dotnet test *.dll --collect:"Code Coverage" and asserts the collector is found. This is the exact regression that prompted the PR; without an acceptance test it can regress silently. The CollectArgumentProcessorTests unit tests are comprehensive for the discovery logic, but they don't cover the end-to-end path from Initialize through adapter loading.

3. NuGet.Config gap (inline comment at line 404) — minor; just a doc comment ask so the limitation is visible.

🧠 Reviewed by Expert Code Reviewer 🧠

[fhnaseer]

A couple of observations from reviewing the discovery path, both non-blocking,

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[github-actions]

Code Review — PR #16215 (re-review)

The three findings from the previous review have all been addressed in the current revision:

1. Pre-release over stable — CompareNuGetVersions now explicitly places any stable release above any pre-release, regardless of numeric version. ✅
2. --testAdapterPath bypass — TryAddCodeCoverageAdapterPath checks for existing paths, and because TestAdapterPathArgumentProcessor runs at priority 9 (before CollectArgumentProcessor at priority 11), by the time the guard executes the user's explicit paths are already in run settings. ✅
3. --enable-code-coverage triggering injection — TryAddCodeCoverageAdapterPath is now called only from CollectArgumentExecutor.Initialize(), not from inside AddDataCollectorToRunSettings. EnableCodeCoverageArgumentProcessor calls the single-string overload, which delegates to the array overload without calling TryAddCodeCoverageAdapterPath. ✅

---

Minor observation — I/O before version parsing in FindCodeCoverageAdapterPath

foreach (var versionDir in Directory.GetDirectories(ccPackagePath))
{
    var collectorDir = FindCollectorDirectory(Path.Combine(versionDir, "build"));  // I/O here
    if (collectorDir is null)
        continue;

    var directoryName = Path.GetFileName(versionDir);
    if (!TryParseNuGetVersion(directoryName, out var version, out var preRelease))  // cheap
        continue;
    ...
}

The directory I/O (FindCollectorDirectory) is performed before the cheap version string parsing (TryParseNuGetVersion). For directories with unparseable names that happen to contain the collector DLL, this wastes a filesystem probe. Given that microsoft.codecoverage typically has only a handful of version entries and all names should be valid SemVer, this has no practical impact — just noting the inversion.

---

Minor observation — EnableCodeCoverageArgumentProcessorTests assertion coverage

The three tests were changed from asserting the exact serialised XML to asserting individual collector properties. Since EnableCodeCoverageArgumentProcessor does not trigger injection, the XML structure itself is unchanged and the old assertions should still hold. The looser assertions reduce the signal the tests provide: if a future change introduces an unexpected extra node via the Enable path, the tests would not catch it. Not a blocker — just something to be aware of when future changes touch the Enable path.

---

Overall the implementation is correct and well-tested. The version-selection logic is thorough (stable-over-prerelease, numeric comparison, dotted pre-release identifiers, build-metadata stripping), the guard against double-injection is correctly positioned given processor priorities, and the escape hatch (VSTEST_DISABLE_CODE_COVERAGE_ADAPTER_DISCOVERY=1) and --testAdapterPath override are properly honoured.

🧠 Reviewed by expert-reviewing

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

a5f8843 takes both observations from the re-review.

I/O before version parsing. Taken. FindCodeCoverageAdapterPath parses the folder name first now and only probes the disk for names that are versions. You are right that it makes no practical difference with a handful of entries, but a folder whose name is not a version is not a candidate no matter what is in it, so doing the cheap check first is the right order anyway.

The EnableCodeCoverageArgumentProcessorTests assertions. Good catch, and it is leftover from an intermediate state of this PR rather than something intentional. The three tests compared the whole run settings xml, and I loosened them when the injection still went through AddDataCollectorToRunSettings, which changed the xml on the --enable-code-coverage path too. Moving the call to CollectArgumentExecutor.Initialize fixed that, and I did not go back and undo the loosening.

Restoring the original assertions, they pass unchanged, which is also the proof that the enable path really does produce the same xml as before. That drops the file from the change, so this is now three files and purely additive, 789 insertions and no deletions.

Release build clean, vstest.console.UnitTests green on both target frameworks, 662 tests, 0 failed.

🤖

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[github-actions]

Code Review — PR #16215

Dimensions checked: RunSettings Validation & Inference · Environment Variable & Feature Flag Contracts · Backward Compatibility & Rollback Safety · Null Safety & Boundary Validation · Process Architecture & Host Resolution

---

What was verified

Priority ordering is correct. TestAdapterPath runs at priority 9, AutoUpdateRunSettings (which includes CollectArgumentProcessor) runs at priority 11. Lower number = processed first, so any --testAdapterPath supplied on the command line is already in RunSettings by the time TryAddCodeCoverageAdapterPath checks existingPaths.Any(...). The guard reliably skips auto-injection when the user has provided an explicit path.

TestAdapterPathArgumentExecutor.Initialize appends, not replaces. It reads existing RunSettings paths and merges them with the new argument before writing back. So even if both injection and --testAdapterPath end up running, the resulting TestAdaptersPaths holds both, and .Distinct() deduplicates.

RunSettings populated from a .runsettings file are also respected. The RunSettings processor runs at priority 6 (before both TestAdapterPath and Collect), so file-configured adapter paths are visible at injection time.

Exception handling is appropriate. TryGetCodeCoverageAdapterPath catches the full set of I/O-related exceptions (IOException, UnauthorizedAccessException, ArgumentException, NotSupportedException, SecurityException), making discovery best-effort without leaking failures into the test run.

Feature flag follows established naming convention. VSTEST_DISABLE_CODE_COVERAGE_ADAPTER_DISCOVERY is consistent with the VSTEST_DISABLE_* pattern used by other flags in FeatureFlag.cs. FeatureFlag is internal, so no PublicAPI.Unshipped.txt entry is required.

Version comparison logic is correct. The stable-over-pre-release preference, Version.CompareTo for numeric parts, and SemVer-2.0-style dotted pre-release identifier comparison all behave as documented. The CompareNuGetVersions branch if (leftPreRelease is null || rightPreRelease is null) is logically equivalent to if (leftPreRelease is null) at that point (the mixed stable/pre-release case returns earlier), but is not a bug.

PR description matches the diff. Title, description, and code changes are aligned. The documented escape hatch (VSTEST_DISABLE_CODE_COVERAGE_ADAPTER_DISCOVERY=1) and skip conditions (no-op when --testAdapterPath is set, best-effort on I/O errors) are all present in the implementation.

---

Findings

One observation is noted inline (TFM sort direction in FindCollectorDirectory). No blocking issues found.

---

🧠 Reviewed by expert-reviewer (RunSettings / Feature Flag / Backward Compat / Null Safety / Process Arch dimensions) · workflow run

🧠 Reviewed by Expert Code Reviewer 🧠

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs:15

• Unused using directive Microsoft.VisualStudio.TestPlatform.Common.Utilities (IDE0005 risk when warnings are treated as errors). Remove it to keep the usings clean and avoid CI failures.

using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.Utilities;
using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces;

[github-actions]

Code Review — PR #16215 (follow-up pass)

Dimensions checked: RunSettings Validation & Inference · Environment Variable & Feature Flag Contracts · Backward Compatibility & Rollback Safety · Null Safety & Boundary Validation · Process Architecture & Host Resolution

---

What was reviewed

This is a follow-up pass after the earlier review (previous thread resolved). The previous blocking finding — non-deterministic version selection when two entries parsed to the same base Version — has been addressed: CompareNuGetVersions now applies a stable-beats-prerelease rule, then numeric comparison, then pre-release label comparison via ComparePreReleaseLabels/ComparePreReleaseIdentifiers, and falls back to an ordinal folder-name tiebreaker. Test coverage for all those cases is thorough.

Findings

Argument-processor priority ordering — ✅ verified correct. TestAdapterPathArgumentExecutor has priority 9 (TestAdapterPath); CollectArgumentExecutor has priority 11 (AutoUpdateRunSettings). Lower numbers execute first, so --testAdapterPath is always written to RunSettings before TryAddCodeCoverageAdapterPath runs its existingPaths check. The "skip if adapter path already set" invariant holds regardless of command-line argument order.

FindCollectorDirectory TFM tie-break — observation only. The OrderByDescending(StringComparer.Ordinal) puts netstandard2.0 ahead of net8.0 because 's' > '8' ordinally. If a future package ever ships the collector under both TFMs, the wrong one would win. The comment acknowledges this ("Real packages hold exactly one such folder"), so it is a documented assumption, not a current bug.

CompareNuGetVersions stable-wins rule — intentional semantic, but worth documenting for callers. 17.0.0 stable beats 18.6.0-preview-1. That is the desired behaviour (described in the PR), but it is a deliberate deviation from SemVer ordering that could surprise future maintainers. The existing XML doc comment already calls this out, so no action needed.

Exception filter in TryGetCodeCoverageAdapterPath — complete. DirectoryNotFoundException is a subclass of IOException and is therefore covered. PathTooLongException (also IOException) is covered. The filter is correct.

PR description alignment

Title and description accurately describe the change. The "does nothing when --testAdapterPath is already set" claim is verified correct (priority ordering above). The NuGet.Config globalPackagesFolder limitation is clearly documented.

Summary

No blocking issues found. The implementation is correct, deterministic, best-effort, and has comprehensive unit test coverage. The previous version-selection finding was fully addressed.

🧠 Reviewed by Copilot Expert Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠