Skip to content

fix(ci): tighten PR test selection to avoid unnecessary full runs - #20322

Merged
Ankit Jain (radical) merged 2 commits into
microsoft:mainfrom
radical:test-selection-audit
Sep 24, 2026
Merged

Ankit Jain (radical) merged 2 commits into
microsoft:mainfrom
radical:test-selection-audit

Conversation

@radical

@radical Ankit Jain (radical) commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Description

This PR tightens PR test selection for inputs that were unnecessarily selecting the full regular PR-CI matrix. The audit used select-tests-selection-Linux artifacts so fork PRs and PRs without selector comments were included.

The rule of thumb stays conservative: if an input can affect broad build/test behavior, or if the consumer boundary is unclear, it still selects ALL.

What changed

Class New behavior
Artifact producer workflows .github/workflows/build-packages.yml selects NuGet artifact consumers; build-cli-native-archives.yml selects native CLI archive consumers. Native dashboard validation is only on the CLI-archive route.
Dev/AzDO/scheduled-only inputs .config/dotnet-tools.json, eng/Version.Details.xml, and scheduled/deployment-only local actions are skipped by the regular GitHub PR-CI prefilter.
eng/common All eng/common/** changes select ALL because this is shared Arcade/build infrastructure.
Regular PR-CI local actions enumerate-tests selects all managed .NET test projects only; check-changed-files and select-tests select Infrastructure.Tests; setup-deno selects Hosting.JavaScript and extension E2E; macOS keychain setup remains ALL.
Broad/safety-sensitive inputs Aspire.slnx, .gitattributes, shared MSBuild/build inputs, shared test fixtures, and matrix/runner infrastructure still select ALL.

Safety checks added

  • Artifact-consuming test projects and selector-gated jobs are derived from the evaluated graph and tests.yml wiring.
  • DOTNET_TESTS is pinned to managed test projects only and does not trigger non-.NET derived jobs.
  • Regular PR-CI local actions are tested against their explicit consumer routes.
  • eng/common/**, .gitattributes, and Aspire.slnx full-matrix behavior is pinned.
  • New skip-entirely paths have focused prefilter assertions.

Notes for reviewers

  • src/Shared/Utf8JsonWriterExtensions.cs was identified as an orphan shared-source file that can hit the run-all fallback when touched. This PR does not remove or enforce orphan shared files; that should be investigated separately if needed.
  • The catch-all ALL rules are split by rationale so future changes can narrow one class without re-auditing unrelated inputs.

Verification

dotnet test --project tests/Infrastructure.Tests/Infrastructure.Tests.csproj --no-launch-profile -- --filter-namespace "*.TestTriggerMap" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"

Result: 273/273 passed.

Fixes # (issue)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20322

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20322"

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 22, 2026
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot September 22, 2026 20:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One critical and four moderate issues remain in routing safety and regression-guard coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity

Open (4)
What changed in this PR

Narrows CI test selection and adds safeguards against unnecessary full-matrix runs.

Changes:

  • Routes artifact workflows to known consumers.
  • Ignores .gitattributes and Aspire.slnx.
  • Removes unused shared code and updates tests/documentation.
File Description
tests/​Infrastructure.Tests/​TestTriggerMap/​TestTriggerMapTests.cs Adds routing and shared-source guards; coverage and path matching remain incomplete.
tests/​Infrastructure.Tests/​TestTriggerMap/​SelectTestsAcceptanceTests.cs Updates selector expectations.
src/​Shared/​Utf8JsonWriterExtensions.cs Removes unused shared code.
eng/​github-ci/​test-trigger-map.yml Narrows routing, including unsafe ignore rules requiring correction.
docs/​ci/​test-trigger-selector-design.md Documents selector audit findings.
docs/​ci/​test-trigger-map.md Documents trigger-map behavior.

Comment thread eng/github-ci/test-trigger-map.yml Outdated
Comment thread eng/github-ci/test-trigger-map.yml Outdated
Comment thread tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs Outdated
Comment thread tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Solution edits can under-select tests, and several new safety guards have coverage gaps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Shared path matcher accepts directories outside src/Shared

tests/​Infrastructure.Tests/​TestTriggerMap/​TestTriggerMapTests.cs:34

This pattern accepts any directory segment named Shared, not specifically src/Shared. For example, the existing $(RepoRoot)tests\Shared\Playwright\TestUtils.cs include matches and contributes Playwright/TestUtils.cs; an orphan with that relative path under src/Shared would therefore pass this guard. Resolve or otherwise validate each captured include against src/Shared before adding its tail to the matcher.

Comment thread tests/Infrastructure.Tests/TestTriggerMap/SelectTestsAcceptanceTests.cs Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

Ankit Jain (radical) added a commit to radical/aspire that referenced this pull request Sep 23, 2026
The audit's own exemption list treated Aspire.slnx as correct-by-design
alongside genuinely build-wide inputs like Directory.Packages.props. A
local dry run against main -- run with the in-flight check disabled and
no knowledge of PR microsoft#20322 -- proved that reasoning wrong: Layer 1 already
roots its project graph at Aspire.slnx at the PR head, so an added
project carries no signal the graph does not already have, and only the
removal direction still needs the run-all fallback.

That analogy is exactly the failure mode the exemption list already
warns against for .gitattributes: a file exempted because it "sounds
build-wide" rather than because its actual consumers were read. Drop
Aspire.slnx from the list and cite the reasoning inline, so it is judged
like any other candidate.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@radical Ankit Jain (radical) changed the title fix(ci): stop four inputs from escalating PR test selection to ALL fix(ci): tighten PR test selection to avoid unnecessary full runs Sep 23, 2026
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot September 23, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A critical matcher defect and multiple moderate routing and validation gaps remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 2 Medium severity · 1 Low severity

Open (5)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Git attributes guard accepts inexact patterns and tokens

tests/​Infrastructure.Tests/​GitAttributesTests.cs:31

The guard can pass without protecting the intended pattern: StartsWith("*.verified.*") accepts a different pattern such as *.verified.*.bak, and substring matching accepts an unrelated attribute such as foo=eol=lf. Parse whitespace-delimited fields and require the exact pattern and exact eol=lf token.

This issue also appears on line 41 of the same file.

Medium severity Consumer discovery ignores imported CI properties

tests/​Infrastructure.Tests/​TestTriggerMap/​TestTriggerMapTests.cs:923

This guard only finds consumers when the flags appear literally in each .csproj. The repository's CI-property contract also allows RequiresNugets/RequiresCliArchive to be set in Directory.Build.props (eng/testing/CITestsProperties.props:13), so a future consumer declared there can be omitted from BUILD_ARTIFACT_CONSUMERS while this test still passes. Evaluate the effective MSBuild properties or include the supported imported props in the consumer discovery.

Comment thread tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

CI routing and validation gaps can under-select required tests and jobs.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 2 Medium severity · 2 Low severity

Open (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Shared path matching can falsely consume unrelated source files

tests/​Infrastructure.Tests/​TestTriggerMap/​TestTriggerMapTests.cs:35

This pattern accepts any path containing a Shared segment, including tests/Shared, and then matches only the captured tail against src/Shared. A tests/Shared/Foo.cs Compile Include could therefore make an unrelated orphaned src/Shared/Foo.cs appear consumed, allowing the guard to pass while the selector still escalates that source change to ALL. Resolve each Include against its containing project/import file and only add entries whose resolved path is under src/Shared.

Comment thread tests/Infrastructure.Tests/TestTriggerMap/SelectTestsAcceptanceTests.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Moderate CI under-selection risks and insufficient regression guards remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity · 2 Low severity

Open (6)
Resolved since last review (2)

Comment thread tests/Infrastructure.Tests/GitAttributesTests.cs Outdated
Comment thread tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Add missing prefilter tests and split artifact-producer routing to avoid unrelated jobs.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (3)

Comment thread eng/github-ci/ci-skip-entirely-patterns.txt

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The artifact-producer routes must be split so package-only changes do not select unrelated native Dashboard validation jobs.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (5)

Comment thread eng/github-ci/test-trigger-map.yml Outdated
Tighten the curated CI selector map so clear-cut infrastructure changes
no longer force the whole PR matrix when a narrower consumer set is known.

Artifact producer workflows now route to the regular PR jobs that consume
those artifacts instead of selecting ALL. Dev-only, AzDO-only, and
scheduled-only inputs are either ignored by the selector or dropped by the
shared CI prefilter so they do not fall into the run-all fallback.

Keep conservative routing for inputs that can affect the whole build or
where the boundary is intentionally broad. In particular, eng/common/** now
selects ALL because it is shared Arcade/build infrastructure that can be
updated as a unit outside this repo.

Add DOTNET_TESTS as a selector sentinel for infrastructure that enumerates
only the managed test-project matrix. enumerate-tests uses this target so it
re-runs every .NET test project without also selecting extension, installer,
polyglot, or other non-.NET PR-gated jobs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add regression coverage for the narrowed selector routes so future map
changes fail loudly when they over-select or under-select.

The tests now pin artifact producer consumers, local composite action
routing, eng/common full-matrix behavior, and the DOTNET_TESTS sentinel. The
DOTNET_TESTS coverage verifies the exact failure mode this target is meant to
avoid: selecting every managed test project without selecting non-.NET jobs
or triggering derived job targets.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Moderate routing and safety-test issues must be resolved before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (1)

Comment thread eng/github-ci/test-trigger-map.yml
Comment thread eng/github-ci/test-trigger-map.yml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Producer-specific consumer guards and safe routing for check-changed-files must be corrected before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)

Comment thread eng/github-ci/test-trigger-map.yml
@radical
Ankit Jain (radical) marked this pull request as ready for review September 23, 2026 23:56
@radical Ankit Jain (radical) added area-engineering-systems infrastructure helix infra engineering repo stuff and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 23, 2026
@radical
Ankit Jain (radical) merged commit 5cc2f85 into microsoft:main Sep 24, 2026
54 checks passed
@microsoft-github-policy-service microsoft-github-policy-service Bot added this to the 14.0 milestone Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-engineering-systems infrastructure helix infra engineering repo stuff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants