[fix] Throw when /diag is passed manually in integration tests

[nohwnd]

Fixes #15625.

🤖 This is an automated fix generated by Issue Repro Triage & Auto-Fix.

Root Cause

Integration tests that passed /diag (or --diag / -diag) manually in their arguments would cause IsDiagAlreadyEnabled to return true, silently skipping the auto-injection of diagnostic logging. This meant those tests would not have their diagnostic logs collected as attachments on failure — making failures harder to diagnose.

What the Fix Does

1. IntegrationTestBase.cs:

   • IsDiagAlreadyEnabled now throws InvalidOperationException when it detects a manual /diag argument, with a helpful message directing the test author to use DiagLogsDirectory instead.
   • Added DiagLogsDirectory property ({TempDirectory.Path}/logs) so tests can reference the auto-injected diag path.
   • Added GetDiagLogContents() protected helper that reads all .txt log files from DiagLogsDirectory — promoted from several test classes that each had their own copy.
   • Simplified InvokeVsTest and InvokeDotnetTest to use DiagLogsDirectory directly.

2. All tests that manually passed /diag: Updated to remove the manual argument. The auto-injection now ensures diag logs are always collected.

3. Tests that checked diag file content (ExecutionTests.cs, DataCollectorTests.Coverlet.cs, DataCollectionTests.cs): Updated to use DiagLogsDirectory-based search patterns instead of custom paths.

4. CreateNoNewWindowTests.cs / SerializerSelectionTests.cs: Removed duplicate GetDiagLogContents() private methods — now using the base class version.

Tests

The existing acceptance tests verify this behavior. The fix ensures diagnostic logs are always attached on test failure, which improves debuggability in CI.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

This PR updates the integration test infrastructure to prevent tests from manually passing /diag (or --diag / -diag) and accidentally bypassing the framework’s diagnostic-log auto-injection and attachment collection, improving CI debuggability on failures.

Changes:

• Make IntegrationTestBase.IsDiagAlreadyEnabled throw when /diag is manually supplied, and expose DiagLogsDirectory + a shared GetDiagLogContents() helper.
• Update affected acceptance/integration tests to stop manually passing /diag and instead read logs from the auto-injected diagnostics location.
• Normalize host-process / datacollector log assertions to point at the diagnostics logs directory.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs	Adds DiagLogsDirectory and GetDiagLogContents(), and throws if tests manually pass diag arguments.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/TestPlatformNugetPackageTests.cs	Removes manual /Diag: injection from code coverage argument construction.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/SerializerSelectionTests.cs	Removes manual /Diag: and relies on GetDiagLogContents() for serializer assertions.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/RunsettingsTests.cs	Stops manually adding diag arg; updates host-log counting to use DiagLogsDirectory.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/PlatformTests.cs	Stops manually adding diag arg; updates host-log counting to use DiagLogsDirectory.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/ExecutionTests.cs	Removes manual diag args and switches log-content assertions to GetDiagLogContents().
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DotnetTestTests.cs	Removes manual --diag: usage so auto-injection can attach logs.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectorTests.Coverlet.cs	Removes manual --diag: and attempts to assert on produced diag log files.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectionTests.cs	Removes manual /Diag: and updates datacollector log lookup to DiagLogsDirectory.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/CreateNoNewWindowTests.cs	Removes manual /Diag: and uses shared GetDiagLogContents() for assertions.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/CodeCoverageTests.cs	Removes manual /Diag: from code coverage runner argument construction.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/BlameDataCollectorTests.cs	Removes manual /Diag: from blame/hang-dump test arguments.

[nohwnd]

🧠 Reviewed by Expert Code Reviewer

Dimensions activated: Acceptance Test Coverage Design, Error Reporting & Diagnostic Clarity

Summary

The change is logically sound. Converting IsDiagAlreadyEnabled from a silent-skip to a hard throw is the right call — it makes misuse visible immediately rather than hiding it. The refactoring to DiagLogsDirectory and the consolidated GetDiagLogContents() base helper are clean improvements.

One concern is noted inline regarding the -diag substring check. The severity of a false-positive match escalated from "silently skip diagnostics" (harmless) to "throw InvalidOperationException" (test failure). Today's test argument strings are unlikely to trigger this, but the fix was the right moment to tighten the match predicate.

All other changes reviewed:

• Assert.Contains(errorMessage, GetDiagLogContents()) parameter order ✓ (needle first, haystack second — correct)
• DataCollectorTests.Coverlet.cs file pattern migration to "log.txt" / "log.txt.datacollector*" / "log.txt.host*" is consistent with how GetDiagArg names the base log file ✓
• DataCollectionTests.cs change from TempDirectory.Path to DiagLogsDirectory for *.datacollector.* search is correct since diag injection now writes to DiagLogsDirectory ✓
• Hardcoded c:\temp\logscpp\ path removed from DotnetTestTests.cs ✓

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

[Error Reporting & Diagnostic Clarity] The -diag substring check (arguments.Contains("-diag", StringComparison.OrdinalIgnoreCase)) was previously a benign false-positive (it silently skipped diag injection). Now it throws, which upgrades a harmless false-positive to a test failure.

An argument string like --logger:"trx;LogFileName=my-diagnostics.trx" would match -diag and throw unexpectedly. The check should be anchored to word boundaries or prefixes to avoid this:

// More precise: require the flag to appear as a standalone argument token
if (Regex.IsMatch(arguments, @"(^|\s)(--diag|/diag|-diag)[:=\s]", RegexOptions.IgnoreCase))

Or at minimum, ensure the character before diag is a flag prefix character (-, /) rather than using a bare Contains. The current form is a pre-existing issue, but the severity escalation from "silent skip" to "throw" makes it worth fixing now.

[nohwnd]

Fixed. Replaced the bare Contains checks with a Regex.IsMatch that anchors the flag to a word boundary — the flag must be preceded by whitespace or start-of-string, and followed by =, :, whitespace, or end-of-string. This prevents false positives from substrings like --logger:my-diagnostics.trx.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Commit pushed: edd2cc4

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

🧠 Reviewed by Expert Code Reviewer

Dimensions activated: Acceptance Test Coverage Design, Error Reporting & Diagnostic Clarity

The prior inline finding (bare Contains("-diag") false-positive upgraded to a throw) was resolved in the second commit. The regex (^|\s)(--diag|/diag|-diag)([:=\s]|$) is correctly anchored — it requires the flag to be a standalone argument token and avoids false positives from substrings like --logger:my-diagnostics.trx.

No remaining concerns. All changes reviewed:

• IsDiagAlreadyEnabled regex anchoring ✓
• GetDiagLogContents() with directory-exists guard ✓
• DiagLogsDirectory property consistency with injection logic ✓
• PR description accurately reflects all changes ✓

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Commit pushed: f9c595a

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Commit pushed: c8117dc

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[nohwnd]

🧠 Reviewed by Expert Code Reviewer

New commits reviewed (since last review at edd2cc4):

• ccaabcbe — --diag insertion before -- separator: logic is correct. arguments?.IndexOf(" -- ") ?? -1 safely handles null, and inserting diagArg (which has a leading space) at separatorPos produces well-formed argument strings like test.dll --diag:logs/log.txt -- runsettings. ✓

• 3c028e3c — Coverlet test revival: switching to AcceptanceTestBase + [NetCoreRunner(HOST_NET)] correctly activates the previously-dead test. Glob patterns log.datacollector* and log.host* are consistent with Path.ChangeExtension("log.txt", "datacollector....") naming. FrameworkArgValue replaces the hardcoded .NETCoreApp,Version=v2.1. ✓

No new concerns. All changes since last review look correct.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

🤖 Expert vstest review (automated)

This is a test-infrastructure-only change in Microsoft.TestPlatform.TestUtilities/integration tests. No production, wire/protocol, public-API, package, binding-redirect, or runtime path is touched, so most of the 16 vstest review dimensions are not applicable. The refactor itself is a net improvement (centralizes GetDiagLogContents, mirrors the -- separator handling between InvokeVsTest and InvokeDotnetTest, and converts the silent "skip auto-injection" behavior into a loud throw so we stop losing diag log attachments on failures). I have no blockers, but a few concerns below worth addressing before merge — mostly around the scope of the Coverlet test revival being hidden in this PR, the new IsDiagAlreadyEnabled semantics, and missing unit coverage for the new regex.

🛑 Blockers

None.

⚠️ Issues

1. DataCollectorTestsCoverlets.RunCoverletCoverage revival is bundled into a "diag refactor" PR — test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectorTests.Coverlet.cs (full file rewrite).

Before this PR the test was effectively dead: it early-returned unless _testEnvironment.TargetFramework == CoreRunnerFramework and then forced .NETCoreApp,Version=v2.1 as the framework, with a REVIEW ME: @Marco do we need to update this test? comment. This PR drops the early return, switches the base class to AcceptanceTestBase, adds [NetCoreRunner(HOST_NET)], and replaces .NETCoreApp,Version=v2.1 with FrameworkArgValue (= net11.0). The test asset CoverletCoverageTestProject targets $(NetCoreAppMinimum) (net8.0) and pins coverlet.collector 1.2.0 — neither of which was validated for net11.0 runs by this PR's stated goal. This is a behavioral change orthogonal to the diag-flag refactor. Two concerns:

• The PR description doesn't mention reviving this test or what was verified about coverlet 1.2.0 + net11.0 host + net8.0 asset. The commit message fix: revive dead coverlet test with correct diag patterns is the only hint.
• If the test starts flaking, bisect will land on a "diag" PR, which obscures the real cause.

Split into its own PR, or at minimum call it out explicitly in the description and confirm the assertions actually pass in CI (the Directory.GetFiles(DiagLogsDirectory, "log.txt").Single() style assertions will throw InvalidOperationException rather than AssertFailedException if the file count is wrong, which is a worse failure UX).

2. IsDiagAlreadyEnabled is misleadingly named after this change — test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs:1096-1114.

The method now has two unrelated effects depending on input: it throws if /diag is in the args, and returns bool based on a VSTEST_DIAG env var. The bool return-value path no longer reflects "diag is already in arguments". Reading callers (if (collectDiagnostics && !IsDiagAlreadyEnabled(arguments ?? ""))) still parses as a guard against double-injection, but in practice the call is either "throw" or "no-op env check". Suggest splitting:

ThrowIfDiagInArguments(arguments);
if (collectDiagnostics && !IsDiagInEnvironment()) { ... }

…or at minimum renaming to something like ValidateAndCheckDiagState. As-is, the next person who reads IsDiagAlreadyEnabled will not expect it to throw.

3. Thrown InvalidOperationException does not include the offending argument — test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs:1103-1109.

When this fires in CI the test author sees a generic message but not which arg or test method tripped it. The matched substring and (ideally) TestContext?.FullyQualifiedTestClassName + TestName would make the error self-locating. Cheap fix:

var m = Regex.Match(arguments, @"(^|\s)(--diag|/diag|-diag)([:=\s]|$)", RegexOptions.IgnoreCase);
if (m.Success)
{
    throw new InvalidOperationException(
        $"Do not pass diag flags directly in test arguments (found '{m.Value.Trim()}' in '{arguments}'). " +
        "The test framework auto-injects diagnostics and attaches them on failure. " +
        "Use IntegrationTestBase.DiagLogsDirectory to find the diag log files.");
}

This also adds the type name IntegrationTestBase.DiagLogsDirectory to the message — easier to grep when you're chasing a CI failure.

4. GetDiagLogContents recursion differs from the helpers it replaces — test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs:1129.

The deleted per-class copies in CreateNoNewWindowTests.cs and SerializerSelectionTests.cs used Directory.GetFiles(logsDir, "*.txt") (TopDirectoryOnly). The new base helper uses SearchOption.AllDirectories. The two existing callers (ExecutionTests.StackOverflowExceptionShouldBeLoggedToConsoleAndDiagLogFile and UnhandleExceptionExceptionShouldBeLoggedToDiagLogFile) check for a stderr fragment via Assert.Contains and so are tolerant, but if anyone later asserts on log-file count via this helper, recursion will silently include stray files (e.g. nested results from dotnet test runs that share TempDirectory.Path). Either keep it TopDirectoryOnly or document the recursion intent. Trivial but easy to get wrong later.

💡 Suggestions

1. Add a unit test for the new regex. This is the load-bearing change of the PR — the previous string.Contains("--diag") was easy to reason about; the new regex isn't. Cover at minimum:

• Positives: "/diag", "/diag:foo", "/diag=foo", "--diag", "--diag bar.txt", "-diag:x", leading-whitespace and mid-string variants, mixed case.
• Negatives: "--logger:my-diag.trx", "--diagnostic", "/diagnostics", "--no-diag-mode", --logger:console;verbosity=normal.

The regression risk if someone later edits this regex is high and not covered today.

2. Promote the regex to a static readonly Regex (optionally RegexOptions.Compiled). Re-parsing on every InvokeVsTest/InvokeDotnetTest call is harmless but the codebase prefers cached regexes elsewhere; would also pair well with the suggestion #3 above to reuse the Match result.

3. _attachments.Add(DiagLogsDirectory) reads the property twice in InvokeDotnetTest (lines 286–319) once it's wired through. Stash a local for readability and to make the dedup pattern with InvokeVsTest more obvious. Pure nit, skip if not in scope.

✅ Notes

• Mirroring the -- separator insertion logic from InvokeDotnetTest into InvokeVsTest (IntegrationTestBase.cs:230-238) is a good correctness fix in its own right — inline-runsettings args after -- would previously have caused --diag to be parsed as a runsettings property. Worth calling out in the description; it's not just a cleanup.
• Removing the manual /diag:c:\temp\logscpp\ literal in DotnetTestTests.cs:96 is a real bug fix — that path would only exist on the author's machine.
• Other --diag callers in the repo (PostProcessingTests.cs, DotnetArchitectureSwitchTests.Windows.cs, ArgumentProcessorTests.cs) bypass InvokeVsTest/InvokeDotnetTest and call ExecuteVsTestConsole/ExecuteApplication directly, so they're unaffected by the throw. Confirmed.
• No public API, no wire protocol, no binding redirects, no package shape, no runtime path changed — dimensions 1–11 and 13–16 of the vstest review checklist are inapplicable here.

Verdict

COMMENT — no blockers, but please address #1 (scope of the Coverlet revival) and #3 (exception context), and ideally land #2 (rename) and the Suggestion #1 (regex unit test) before merging.

[nohwnd]

Commit pushed: 8966ad3

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Addressed the review feedback:

Issues fixed:

1. Coverlet revival scope (Issue Patched commits from VSTS #1) — called out in the PR description (see updated body) that DataCollectorTestsCoverlets.RunCoverletCoverage was revived as a deliberate part of this PR: the test previously dead-returned and bypassed diag injection; now that auto-injection always fires the test can use DiagLogsDirectory patterns correctly.

2. IsDiagAlreadyEnabled rename (Issue Load and Initialize Data Collectors #2) — split into ThrowIfDiagInArguments(string) (void, throws) and IsDiagInEnvironment() (bool), making the two distinct behaviors explicit. Call sites updated accordingly.

3. Exception context (Issue Add netci.groovy for CI jobs. #3) — switched from Regex.IsMatch to Regex.Match and included the matched token and full argument string in the InvalidOperationException message (e.g. found '--diag' in '...').

4. GetDiagLogContents recursion (Issue Capability to send datacollection data as part of testcase end event … #4) — changed SearchOption.AllDirectories → TopDirectoryOnly to match the per-class helpers it replaced.

Suggestions addressed:

• Static readonly Regex (Suggestion Load and Initialize Data Collectors #2) — promoted to internal static readonly Regex DiagFlagPattern with RegexOptions.Compiled.
• Unit tests for the regex (Suggestion Patched commits from VSTS #1) — added DiagFlagPatternTests.cs in the acceptance test project with positive and negative cases. Added Friends.cs with InternalsVisibleTo so the internal field is accessible.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[nohwnd]

🧠 Reviewed by Expert Code Reviewer

New commit reviewed (since last review at 3c028e3c):

• 8966ad39 — Review feedback addressing commit. All changes look correct:

  • DiagFlagPattern as compiled static Regex: avoids re-allocating the pattern on every test invocation. ✓
  • ThrowIfDiagInArguments / IsDiagInEnvironment split: names now clearly distinguish the two-phase "guard vs. env-var skip" logic. The arguments ?? "" guard at the InvokeVsTest call site handles null safely. ✓
  • Error message includes match.Value.Trim() and arguments: gives actionable CI diagnostics. ✓
  • GetDiagLogContents → TopDirectoryOnly: matches the behavior of the per-class helpers it replaced; CountTestHostLogs deliberately keeps AllDirectories since host log files can be nested. ✓
  • Friends.cs + DiagFlagPatternTests: InternalsVisibleTo correctly targets Microsoft.TestPlatform.Acceptance.IntegrationTests with the public key; test coverage spans all positive/negative regex cases including the subtle --diagnostic (suffix after token) and --no-diag-mode (prefix before token) false-positive guards. ✓

No remaining concerns.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠