From 7f36fefbc362037992b9f2ae808b960547d6b2ad Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Tue, 21 Jul 2026 21:46:54 -0500 Subject: [PATCH] Fix conditional test scope list dropping scopes in CI The conditional test filtering framework skips whole test suites on PRs when relevant files haven't changed. When more than one scope was skipped, CI failed with "MSB1006: Property is not valid. Switch: ". EvaluateConditionalTestScopes joined the skipped scopes with ';' and set them as a pipeline variable that sdk-build.yml forwards via /p:SkippedTestScopes=. MSBuild's /p: parser treats ';' as a property-list separator, so only the first scope survived and the remaining tokens were interpreted as bogus switches. Escaping ';' as %3B did not help because Azure DevOps percent-decodes %3B back to ';' when it parses the ##vso[task.setvariable] logging command. Emit the pipeline variable with '|' as the separator (Azure DevOps leaves it untouched and MSBuild's /p: parser does not split on it), and normalize '|' back to ';' inside the _CollectSkippedTestProjects target. The normalization must happen in the target rather than a static PropertyGroup because SkippedTestScopes arrives as a global /p: property that project-level assignments cannot override. Adds unit tests covering the multi-scope pipeline variable format and the end-to-end MSBuild removal with multiple scopes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f81a469-1934-44a2-9805-f74fee614159 --- .../project-docs/pr-test-filtering.md | 7 ++- scripts/EvaluateConditionalTestScopes.cs | 10 ++- test/ConditionalTests.targets | 6 +- .../EvaluateConditionalTestScopesTests.cs | 61 ++++++++++++++++++- ...moveSkippedConditionalTestProjectsTests.cs | 55 +++++++++++++++-- .../ConditionalTestRemoval.proj | 5 +- 6 files changed, 131 insertions(+), 13 deletions(-) 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)'))