Conditional test filtering framework for PR builds#55166
Conditional test filtering framework for PR builds#55166MichaelSimons wants to merge 24 commits into
Conversation
Introduces a framework for conditionally running tests on PR builds based on what source files changed, reducing Helix compute consumption. - Design document (documentation/project-docs/pr-test-filtering.md) - C# evaluation script (scripts/EvaluateConditionalTestScopes.cs) - Central config (test/ConditionalTests.props) with TemplateEngine scope - MSBuild + pipeline integration - Agent instructions update Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nagilson
left a comment
There was a problem hiding this comment.
Overall, I'm in favor of the proposal.
However, I want to poke on two points:
- How will we define
TriggerPaths?
I think this should be more than the folder containing relevant source code for that repo - e.g.dotnet-watchproject should not be conditioned on justdotnet-watchbut also include theProjectReference(s) such as..\..\Cli\Microsoft.DotNet.Cli.Definitions\Microsoft.DotNet.Cli.Definitions.csproj. Changes toPackageReference(s) such asMicrosoft.Build.Locator"may also result in a break.
dotnetup already uses conditional test filtering to a degree but we've seen more breaks than expected when running all of the tests. So, there is a risk involved in disabling these tests though on average I think it's the right approach. We should try to minimize that risk.
There have been times where we caught product bugs (e.g. system command line, roslyn, and others) because tests in other areas started failing even when roslyn's tests were all passing. But we shouldn't rely on test coverage of others like that.
- When can we ignore 'CoditionalTests' - I think we should be able to ignore that. We can still get a lot of the time saving on codeflow or certain dev PRs... but I think for example in dotnet/dotnet flow we will still want to run all of the tests so we get full filtering.
I'd propose to have the conditional tests condition test runs out on code flow for specific repos - e.g. a change to dotnet/templating code flow shouldn't need to run the watch tests. If we are all encompassing on dependencies with 1 then maybe we can be looser on that?
- Added 'Choosing trigger paths' subsection covering dependency awareness - Moved dependency flow RunAlways future inline with the metadata description - Consistent with how class/method-level futures are described inline Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@noahfalk, thanks for the insightful feedback.
You are correct that defining just the src path for the affected area is often not sufficient. I did not elaborate on this in the documentation but should call it out in order to help devs and AI make better decisions when defining new test scopes.
Yes this is in ways a big hammer so it must be used correctly. Having good documentation calling these gotchas is imperative.
I do see it valuable to be able to specify a I updated the doc in d518adb in response to this feedback. |
Simplifies the evaluation script by requiring the caller to pass the props file path explicitly. Removes the FindPropsFile fallback. The pipeline YAML now passes the full path via Build.SourcesDirectory. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Added one-third minimum goal for scope coverage - Clarified 'initial candidates' framing for example scopes - Added 'Choosing trigger paths' section covering dependency awareness - Inlined dependency flow RunAlways future with metadata description Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Notes AI-based test selection and incremental build improvements as aspirational alternatives, framing PR velocity as the primary driver for the current pragmatic approach. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The script: task runs via cmd.exe on Windows where relative paths like .dotnet/dotnet don't resolve. Use Build.SourcesDirectory for the full path. Also update display name to purple dot convention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When no scopes match, the script now outputs '__none__' instead of an empty string. This distinguishes 'script ran, nothing active' (remove scoped tests) from 'variable never set' (safe fallback, run all tests). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Logs build reason, target branch, changed file count (with file list for small diffs), and per-scope active/inactive decisions with reasons. Makes CI failures diagnosable from the step log. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Changes to shared test infrastructure (Microsoft.NET.TestFramework, TestFramework.MSTest, evaluation script) now force all conditional scopes to run, since these are dependencies of all test projects. GlobalTriggerPaths is defined in ConditionalTests.props as a PropertyGroup and checked before per-scope evaluation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tting - Invert variable semantics: list scopes to skip instead of scopes that are active - Rename ActiveConditionalTestScopes -> SkippedTestScopes (clearer intent) - Use __all__ sentinel when all scopes should be skipped - Global triggers now simply don't set the variable (natural safe default) - Restructure ConditionalTests.props with section headers for clarity - Use if/else chain in evaluation script instead of repeated !shouldRun guards - Update documentation decision flow and safe defaults Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Include test/TestAssets/TestPackages/TemplateEngine/** so changes to template engine test packages also trigger the TemplateEngine scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
90f3554 to
35b65d6
Compare
Validates that all TriggerPaths and GlobalTriggerPaths in ConditionalTests.props reference directories that actually exist in the repo. Catches stale paths after folder renames or refactors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@MichaelSimons Perhaps ambitious, but as a starting point would we want to consider a one-time build of the msbuild project dependency graph - and rebuild it whenever the project file changes? This would give us a better idea of what other |
d8c0efc to
25bf3eb
Compare
@nagilson - Dependency graph resolution is an interesting idea. For now though, I'd like to get a few scopes defined and validate the realized filtering gains before investing in additional infrastructure. It's also worth keeping in mind that this doesn't have to be 100% perfect — these tests still run on every CI build. We're only filtering on PR validation. If we start seeing that PR validation is frequently missing things that CI catches, that's the signal to adjust/invest in more tooling. But I don't think we need that up front. |
25bf3eb to
4b56d28
Compare
Move path validation into EvaluateConditionalTestScopes.cs which runs in the pipeline with full repo access. Emits ##[warning] for stale paths. Remove the Microsoft.NET.Infra.Tests project since Helix payloads don't include the source tree needed for directory existence checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The repo root is the primary input since all trigger paths are relative to it. The script now derives the props file location (test/ConditionalTests.props) from the repo root rather than computing the repo root from the props file path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ching - Change BeforeTargets from CoreTest to CreateSDKCustomTestWorkItems for deterministic target ordering - Use MatchOnMetadata=FullPath on Remove to handle mixed relative/absolute SDKCustomTestProject identities - Put TestProjects on single line to support MSBuild glob expansion - Use *.csproj glob for broader test project matching - Remove redundant Condition on Message task Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Replace per-file removal messages with single summary message - Add future enhancements section noting PR label override capability Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ine scope This change should cause TemplateEngine tests to run (trigger path: test/TemplateEngine/**) and all other conditional scopes to be skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Add MatchOnMetadataOptions=PathLike for OS-appropriate path comparison - Guard __all__ sentinel against zero-scope edge case (0 == 0) - Handle ? wildcard in GlobMatches regex conversion Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Add MatchOnMetadataOptions=PathLike for OS-appropriate path comparison - Guard __all__ sentinel against zero-scope edge case (0 == 0) - Handle ? wildcard in GlobMatches regex conversion Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…imons-conditionalize-illinker-tests
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
This is now ready for a review of the implementation. |
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to conditionally skip selected test projects in PR validation based on the set of files changed, while keeping full test execution on non-PR CI builds. This aims to reduce Helix usage and shorten PR turnaround by filtering unrelated test suites.
Changes:
- Introduces a central scope configuration file (
test/ConditionalTests.props) and integrates it intotest/UnitTests.projto remove skipped test projects before Helix submission. - Adds a C# evaluation script (
scripts/EvaluateConditionalTestScopes.cs) and wires it into the AzDO pipeline to setSkippedTestScopes. - Adds design documentation and updates Copilot agent guidance to reference the new filtering system.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests.proj | Imports conditional scope config and removes skipped test projects via a new MSBuild target. |
| test/ConditionalTests.props | Defines global trigger paths and the first conditional scope (TemplateEngine) with trigger/test project patterns. |
| scripts/EvaluateConditionalTestScopes.cs | Computes changed files vs PR target branch, evaluates scopes, and sets SkippedTestScopes pipeline variable. |
| eng/pipelines/templates/jobs/sdk-build.yml | Runs the evaluation script before queuing Helix tests and passes SkippedTestScopes into MSBuild. |
| documentation/project-docs/pr-test-filtering.md | Documents motivation, design, and instructions for adding scopes. |
| .github/copilot-instructions.md | Notes the existence of conditional test filtering and points contributors to the configuration/doc. |
- Use ;-delimited Contains check to prevent substring false positives - Strip refs/heads/ prefix from AzDO target branch variable - Fix typo in design doc Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Introduces a framework for conditionally running tests on PR builds based on what source files changed. Tests that are unrelated to the PR's changes can be skipped, reducing Helix compute consumption and improving PR validation times. All tests continue to run unconditionally on CI builds (
main, release branches).This is posted as a draft to seek feedback on the approach. The implementation is included to illustrate the direction — it is intentionally simple and small despite addressing a complex problem.Received positive feedback on the approach — proceeding with the full implementation of conditional test filtering.
Design
See
documentation/project-docs/pr-test-filtering.mdfor the full motivation, mechanisms, and instructions for adding new scopes.What's included
ConditionalTests.props) with TemplateEngine as the first scopeRelated to #55203