Skip to content

Escape semicolons in conditional test scope pipeline variable #55401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mthalman merged 1 commit into from
Jul 22, 2026
Merged

Escape semicolons in conditional test scope pipeline variable #55401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion documentation/project-docs/pr-test-filtering.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,12 @@ projects they control.
2. **`scripts/EvaluateConditionalTestScopes.cs`** — C# script that runs before test
submission. It reads `ConditionalTests.props`, computes the git diff against the
target branch, and determines which scopes to skip. It outputs the
`SkippedTestScopes` Azure DevOps pipeline variable.
`SkippedTestScopes` Azure DevOps pipeline variable. When more than one scope is
skipped, the scope names are joined with `|` rather than `;`. Azure DevOps decodes
`%3B` back to `;` when it parses the `##vso[task.setvariable]` command, so an escaped
semicolon cannot survive, and a raw `;` would be split by MSBuild's `/p:` property-list
parser when the pipeline passes `/p:SkippedTestScopes=<value>`. `ConditionalTests.targets`
normalizes `|` back to `;`.

3. **`test/UnitTests.proj`** — imports `ConditionalTests.props` and defines a Target
(`RemoveSkippedConditionalTestProjects`) that removes skipped test projects from
Expand Down
10 changes: 8 additions & 2 deletions scripts/EvaluateConditionalTestScopes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
// Output variable format (set via ##vso when running in Azure Pipelines):
// - Empty string: no scopes skipped, all tests run.
// - "__all__": every defined scope is skipped.
// - Semicolon-separated scope names (e.g. "TemplateEngine;ILLink"): only listed scopes are skipped.
// - '|'-separated scope names (e.g. "TemplateEngine|ILLink"): only listed scopes are skipped.
// '|' is used rather than ';' so the list survives Azure DevOps (which decodes '%3B' back to ';'
// in ##vso commands) and MSBuild's /p: property-list parser. ConditionalTests.targets converts
// '|' back to ';'.
// The human-readable log line below ("Skipped test scopes: ...") still uses ';' for readability.

using System.Diagnostics;
using System.Text.RegularExpressions;
Expand Down Expand Up @@ -161,7 +165,9 @@
// Set Azure DevOps pipeline variable if running in CI and output variable was specified
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("TF_BUILD")) && !string.IsNullOrEmpty(outputVariable))
{
Console.WriteLine($"##vso[task.setvariable variable={outputVariable}]{result}");
// Join with '|' as the separator (see the output-format note at the top of this file).
var pipelineValue = result.Replace(";", "|");
Console.WriteLine($"##vso[task.setvariable variable={outputVariable}]{pipelineValue}");
}

Console.WriteLine($"Skipped test scopes: {(result == "" ? "(none)" : result)}");
Expand Down
6 changes: 5 additions & 1 deletion test/ConditionalTests.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@
property functions in ItemGroup conditions. -->
<Target Name="_CollectSkippedTestProjects" Outputs="%(ConditionalTestScope.Identity)" Condition="'$(SkippedTestScopes)' != ''">
<PropertyGroup>
<!-- The pipeline passes a '|'-separated list (see EvaluateConditionalTestScopes.cs); normalize
it to ';' here. This must be in the target, not a static <PropertyGroup>, because
SkippedTestScopes is a global /p: property that a project-level assignment can't override. -->
<_NormalizedSkippedScopes>$(SkippedTestScopes.Replace('|',';'))</_NormalizedSkippedScopes>
<_CurrentScope>%(ConditionalTestScope.Identity)</_CurrentScope>
<_CurrentTestProjects>%(ConditionalTestScope.TestProjects)</_CurrentTestProjects>
<_CurrentMechanism>%(ConditionalTestScope.Mechanism)</_CurrentMechanism>
<_ShouldSkip Condition="'$(_CurrentMechanism)' == 'project' and ('$(SkippedTestScopes)' == '__all__' or $([System.String]::Concat(';','$(SkippedTestScopes)',';').Contains(';$(_CurrentScope);')))">true</_ShouldSkip>
<_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. -->
Expand Down
61 changes: 60 additions & 1 deletion test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,54 @@ public async Task PRBuild_AllScopesSkipped_OutputsAll()
Assert.Contains("Skipped test scopes: __all__", result.StdOut);
}

[TestMethod]
public async Task PRBuild_MultipleScopesSkipped_PipelineVariableUsesPipeSeparator()
{
// Two of three scopes are skipped, so the pipeline variable lists both, separated by '|'
// (see EvaluateConditionalTestScopes.cs for why '|' rather than ';').
var props = """
<Project>
<PropertyGroup>
<GlobalTriggerPaths>shared/**</GlobalTriggerPaths>
</PropertyGroup>
<ItemGroup>
<ConditionalTestScope Include="FeatureA">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureA.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureA/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
<ConditionalTestScope Include="FeatureB">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureB.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureB/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
<ConditionalTestScope Include="FeatureC">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureC.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureC/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
</ItemGroup>
</Project>
""";

using var repo = new TestRepo(props, "shared", "src/FeatureA", "src/FeatureB", "src/FeatureC",
"test/FeatureA.Tests", "test/FeatureB.Tests", "test/FeatureC.Tests");
repo.AddAndCommitFiles("main", "src/Unrelated/file.cs");
// Only change files in FeatureC → FeatureA and FeatureB are skipped
repo.CreateBranchWithChanges("pr-branch", "src/FeatureC/Code.cs");

var result = await RunScript(repo.Root, targetBranch: "main", buildReason: "PullRequest", outputVariable: "SkippedTestScopes");

Assert.AreEqual(0, result.ExitCode, result.StdErr);
// The '|'-separated pipeline variable carries both skipped scopes.
Assert.Contains("##vso[task.setvariable variable=SkippedTestScopes]FeatureA|FeatureB", result.StdOut);
// The human-readable log line stays semicolon-separated for readability.
Assert.Contains("Skipped test scopes: FeatureA;FeatureB", result.StdOut);
}

[TestMethod]
public async Task GlobMatches_DoubleStarSlash_MatchesZeroSegments()
{
Expand Down Expand Up @@ -392,13 +440,17 @@ private static string CreateBasicProps() => """
private static readonly string[] BasicPropsDirs =
["test/Microsoft.NET.TestFramework", "src/MyFeature", "test/MyFeature.Tests"];

private async Task<ScriptResult> RunScript(string repoRoot, string? targetBranch, string buildReason)
private async Task<ScriptResult> RunScript(string repoRoot, string? targetBranch, string buildReason, string? outputVariable = null)
{
var args = $"run \"{_scriptPath}\" -- --repo-root \"{repoRoot}\" --build-reason \"{buildReason}\"";
if (targetBranch != null)
{
args += $" --target-branch \"{targetBranch}\"";
}
if (outputVariable != null)
{
args += $" --output-variable \"{outputVariable}\"";
}

var psi = new ProcessStartInfo(_dotnetPath, args)
{
Expand All @@ -407,6 +459,13 @@ private async Task<ScriptResult> RunScript(string repoRoot, string? targetBranch
RedirectStandardError = true,
};

// The script only emits the ##vso[task.setvariable] line when TF_BUILD is set
// (i.e. running under Azure Pipelines). Simulate that when an output variable is requested.
if (outputVariable != null)
{
psi.Environment["TF_BUILD"] = "true";
}

// Remove test-framework env vars that interfere with child dotnet processes
// (e.g., causing NETSDK1207 AOT errors). SdkTestContext.Initialize() sets these
// for in-process MSBuild use, but they break out-of-process `dotnet run`.
Expand Down
55 changes: 49 additions & 6 deletions test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,60 @@ public async Task MultipleScopesSkipped_AllMatchingScopesRemoved()
{
using var env = new TestEnvironment(TestAssetsManager, CreateTwoScopeProps());

var remaining = await RunRemovalTarget(env, skippedScopes: "FeatureA;FeatureB");
var remaining = await RunRemovalTarget(env, skippedScopes: "FeatureA|FeatureB");

AssertNoneContain(remaining, "FeatureA", "FeatureA should be removed");
AssertNoneContain(remaining, "FeatureB", "FeatureB should be removed");
AssertAnyContains(remaining, "Unrelated", "Unrelated should remain");
}

[TestMethod]
public async Task MultipleScopesSkipped_UnskippedScopeKept()
{
// Three defined scopes; skip two of them and verify both are removed while the third
// (not in the skip list) keeps its test project.
var props = """
<Project>
<PropertyGroup>
<GlobalTriggerPaths>shared/**</GlobalTriggerPaths>
</PropertyGroup>
<ItemGroup>
<ConditionalTestScope Include="FeatureA">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureA.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureA/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
<ConditionalTestScope Include="FeatureB">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureB.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureB/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
<ConditionalTestScope Include="FeatureC">
<Mechanism>project</Mechanism>
<TestProjects>test/FeatureC.Tests/*.csproj</TestProjects>
<TriggerPaths>src/FeatureC/**</TriggerPaths>
<RunAlways>CI</RunAlways>
</ConditionalTestScope>
</ItemGroup>
</Project>
""";

using var env = new TestEnvironment(TestAssetsManager, props, new[]
{
"test/FeatureA.Tests/FeatureA.Tests.csproj",
"test/FeatureB.Tests/FeatureB.Tests.csproj",
"test/FeatureC.Tests/FeatureC.Tests.csproj"
});

var remaining = await RunRemovalTarget(env, skippedScopes: "FeatureA|FeatureB");

AssertNoneContain(remaining, "FeatureA", "FeatureA should be removed");
AssertNoneContain(remaining, "FeatureB", "FeatureB should be removed");
AssertAnyContains(remaining, "FeatureC", "FeatureC should remain (not in skip list)");
}

[TestMethod]
public async Task AllKeyword_AllConditionalProjectsRemoved()
{
Expand Down Expand Up @@ -185,14 +232,10 @@ private async Task<List<string>> RunRemovalTarget(TestEnvironment env, string sk
// Use forward slash to avoid the trailing-backslash-before-quote issue on Windows.
var testRepoRoot = env.Root.TrimEnd(Path.DirectorySeparatorChar) + "/";

// Escape semicolons in SkippedTestScopes with %3B for the command line.
// ConditionalTestRemoval.proj unescapes them via $([MSBuild]::Unescape(...)).
var escapedScopes = skippedScopes.Replace(";", "%3B");

var args = $"msbuild \"{_testProjPath}\" /t:VerifyRemoval " +
$"/p:TestRepoRoot={testRepoRoot} " +
$"/p:RealRepoRoot={_targetsRoot} " +
$"/p:SkippedTestScopes={escapedScopes} " +
$"/p:SkippedTestScopes={skippedScopes} " +
$"/p:TestProjectItemsFile={itemsFile} " +
$"/v:normal";

Expand Down
5 changes: 3 additions & 2 deletions test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
Usage:
dotnet msbuild ConditionalTestRemoval.proj /p:SkippedTestScopes=<value> /p:TestRepoRoot=<path> /p:RealRepoRoot=<path> /p:TestProjectItemsFile=<path> /t:VerifyRemoval

<value> is a single scope name, "__all__", or multiple scopes separated by '|' (e.g.
"FeatureA|FeatureB"). Use '|', not ';': a raw ';' is split by MSBuild's /p: parser.

This project simulates what UnitTests.proj does: imports ConditionalTests.props and
ConditionalTests.targets, defines SDKCustomTestProject items, runs the removal target,
and prints what remains.
Expand All @@ -14,8 +17,6 @@
<PropertyGroup>
<!-- RepoRoot is what the targets use for glob expansion and to locate ConditionalTests.props -->
<RepoRoot>$(TestRepoRoot)</RepoRoot>
<!-- Unescape %3B back to ; since MSBuild's CLI parser splits on raw semicolons -->
<SkippedTestScopes>$([MSBuild]::Unescape('$(SkippedTestScopes)'))</SkippedTestScopes>
</PropertyGroup>
Comment thread
MichaelSimons marked this conversation as resolved.

<!-- Import the REAL removal targets from the repo (single source of truth).
Expand Down
Loading