Escape semicolons in conditional test scope pipeline variable

[MichaelSimons]

Problem

The conditional test filtering framework (see #55203) skips whole test scopes on a PR when no relevant files changed. When more than one scope is skipped, the SkippedTestScopes value carried the scope names as a semicolon-separated list. The pipeline passes it to MSBuild as /p:SkippedTestScopes="<value>", and MSBuild's /p: parser treats ; as a property-list separator. So only the first scope survived; the remaining (should-be-skipped) suites still ran — and, depending on the scope, MSBuild treated the trailing token as a bogus switch:

MSBUILD : error MSB1006: Property is not valid.
Switch: NetAnalyzers

Single-scope and __all__ cases have no separator, which is why the bug stayed hidden.

Why %3B escaping does not work

The intuitive fix — emit the separator as the MSBuild escape %3B — fails on Azure DevOps. AzDO percent-decodes %3B (and %0D, %0A, %5D, %25) back to ; when it parses the ##vso[task.setvariable] logging command, so the escape is gone before the value is ever stored. The variable reaches MSBuild with raw semicolons again → same MSB1006.

Fix

Carry the list with | as the separator:

• scripts/EvaluateConditionalTestScopes.cs — the pipeline variable value joins scopes with | (AzDO leaves it untouched; MSBuild's /p: parser does not split on it). The human-readable log line keeps ; for readability.
• test/ConditionalTests.targets — normalizes | back to ; into a local property inside the batching target. It cannot be done in a static <PropertyGroup> because SkippedTestScopes arrives as a global /p: property, which a project-level assignment cannot override.
• test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj — comment updated to match.

Tests

• EvaluateConditionalTestScopesTests.PRBuild_MultipleScopesSkipped_PipelineVariableUsesPipeSeparator — asserts the ##vso variable is |-separated (not raw ;), and the log line stays ;.
• RemoveSkippedConditionalTestProjectsTests.MultipleScopesSkipped_UnskippedScopeKept — end-to-end MSBuild removal driving the real ConditionalTests.targets with a global /p:SkippedTestScopes=FeatureA|FeatureB, verifying skipped scopes are removed and the unskipped one is kept.

All 23 tests in the two affected classes pass locally.

Related to #55203

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Updates the conditional test filtering pipeline integration so SkippedTestScopes can safely carry multiple skipped scopes through Azure DevOps and into MSBuild without being split or mis-parsed, preventing unintended test execution and MSB1006 failures.

Changes:

• Emit the Azure DevOps SkippedTestScopes variable using | as the separator (while keeping human-readable logs semicolon-separated).
• Normalize | back to ; inside ConditionalTests.targets within the batching target so scope matching continues to work.
• Add/adjust unit and end-to-end tests to validate multi-scope skipping behavior and the exact pipeline logging output.

Reviewed changes

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

Show a summary per file

File	Description
test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj	Removes %3B unescape logic so the test asset relies on the same normalization as production targets.
test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs	Updates the MSBuild invocation to use `
test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs	Adds coverage asserting the ##vso[task.setvariable] output uses `
test/ConditionalTests.targets	Normalizes SkippedTestScopes from `
scripts/EvaluateConditionalTestScopes.cs	Writes ##vso[task.setvariable] using `
documentation/project-docs/pr-test-filtering.md	Documents why `

Comments suppressed due to low confidence (1)

test/ConditionalTests.targets:35

• The final RemoveSkippedConditionalTestProjects message logs $(SkippedTestScopes) verbatim, which will now contain | separators. Since this target normalizes | back to ; for matching, the message should also print the normalized value to keep the log human-readable when multiple scopes are skipped.

      <_ShouldSkip Condition="'$(_CurrentMechanism)' == 'project' and ('$(_NormalizedSkippedScopes)' == '__all__' or $([System.String]::Concat(';','$(_NormalizedSkippedScopes)',';').Contains(';$(_CurrentScope);')))">true</_ShouldSkip>
      <_ShouldSkip Condition="'$(_ShouldSkip)' != 'true'">false</_ShouldSkip>
      <!-- Prepend $(RepoRoot) to each semicolon-separated relative path for glob resolution.
           Replace() escapes semicolons internally, so Unescape restores them for item splitting. -->
      <_AbsoluteTestProjects Condition="'$(_ShouldSkip)' == 'true'">$(RepoRoot)$(_CurrentTestProjects.Replace(';',';$(RepoRoot)'))</_AbsoluteTestProjects>