diff --git a/documentation/project-docs/pr-test-filtering.md b/documentation/project-docs/pr-test-filtering.md index dc0ab03f5d81..96e0fa2164c0 100644 --- a/documentation/project-docs/pr-test-filtering.md +++ b/documentation/project-docs/pr-test-filtering.md @@ -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=`. `ConditionalTests.targets` + normalizes `|` back to `;`. 3. **`test/UnitTests.proj`** — imports `ConditionalTests.props` and defines a Target (`RemoveSkippedConditionalTestProjects`) that removes skipped test projects from diff --git a/scripts/EvaluateConditionalTestScopes.cs b/scripts/EvaluateConditionalTestScopes.cs index b306e87aba7c..685e6238bfc3 100644 --- a/scripts/EvaluateConditionalTestScopes.cs +++ b/scripts/EvaluateConditionalTestScopes.cs @@ -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; @@ -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)}"); diff --git a/test/ConditionalTests.targets b/test/ConditionalTests.targets index 9724be8f73b8..fb2dca956cf8 100644 --- a/test/ConditionalTests.targets +++ b/test/ConditionalTests.targets @@ -21,10 +21,14 @@ property functions in ItemGroup conditions. --> + + <_NormalizedSkippedScopes>$(SkippedTestScopes.Replace('|',';')) <_CurrentScope>%(ConditionalTestScope.Identity) <_CurrentTestProjects>%(ConditionalTestScope.TestProjects) <_CurrentMechanism>%(ConditionalTestScope.Mechanism) - <_ShouldSkip Condition="'$(_CurrentMechanism)' == 'project' and ('$(SkippedTestScopes)' == '__all__' or $([System.String]::Concat(';','$(SkippedTestScopes)',';').Contains(';$(_CurrentScope);')))">true + <_ShouldSkip Condition="'$(_CurrentMechanism)' == 'project' and ('$(_NormalizedSkippedScopes)' == '__all__' or $([System.String]::Concat(';','$(_NormalizedSkippedScopes)',';').Contains(';$(_CurrentScope);')))">true <_ShouldSkip Condition="'$(_ShouldSkip)' != 'true'">false diff --git a/test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs b/test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs index 84a942793047..c428e16f90e6 100644 --- a/test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs +++ b/test/Microsoft.NET.Infrastructure.Tests/EvaluateConditionalTestScopesTests.cs @@ -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 = """ + + + shared/** + + + + project + test/FeatureA.Tests/*.csproj + src/FeatureA/** + CI + + + project + test/FeatureB.Tests/*.csproj + src/FeatureB/** + CI + + + project + test/FeatureC.Tests/*.csproj + src/FeatureC/** + CI + + + + """; + + 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() { @@ -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 RunScript(string repoRoot, string? targetBranch, string buildReason) + private async Task 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) { @@ -407,6 +459,13 @@ private async Task 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`. diff --git a/test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs b/test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs index 23d0e39785f3..ac5b28cdf307 100644 --- a/test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs +++ b/test/Microsoft.NET.Infrastructure.Tests/RemoveSkippedConditionalTestProjectsTests.cs @@ -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 = """ + + + shared/** + + + + project + test/FeatureA.Tests/*.csproj + src/FeatureA/** + CI + + + project + test/FeatureB.Tests/*.csproj + src/FeatureB/** + CI + + + project + test/FeatureC.Tests/*.csproj + src/FeatureC/** + CI + + + + """; + + 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() { @@ -185,14 +232,10 @@ private async Task> 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"; diff --git a/test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj b/test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj index 2b176a6f188a..e44841eb334a 100644 --- a/test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj +++ b/test/TestAssets/TestProjects/ConditionalTestRemoval/ConditionalTestRemoval.proj @@ -5,6 +5,9 @@ Usage: dotnet msbuild ConditionalTestRemoval.proj /p:SkippedTestScopes= /p:TestRepoRoot= /p:RealRepoRoot= /p:TestProjectItemsFile= /t:VerifyRemoval + 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. @@ -14,8 +17,6 @@ $(TestRepoRoot) - - $([MSBuild]::Unescape('$(SkippedTestScopes)'))